-
Notifications
You must be signed in to change notification settings - Fork 137
Topics: add new topics (jnewbery) #246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing:
- compact-block-filters.md
- countersign.md
- cpfp-carve-out.md
- cve-2018-17144.md
- erlay.md
- replace-by-fee.md
- transaction-pinning.md
- v2-p2p-transport.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed:
- compact-block-filters.md
- countersign.md
- cpfp-carve-out.md
- cve-2018-17144.md
- erlay.md
- replace-by-fee.md
- transaction-pinning.md
- v2-p2p-transport.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done a first pass of everything:
- compact-block-filters.md
- countersign.md
- cpfp-carve-out.md
- cve-2018-17144.md
- erlay.md
- replace-by-fee.md
- transaction-pinning.md
- v2-p2p-transport.md
This is ready for merge once the minisketch topic is merged (#243) |
_topics/en/compact-block-filters.md
Outdated
in the block and the scriptPubKey for each output created in the | ||
block). The GCS (called a filter) is then distributed to wallets | ||
(such as via the P2P protocol as described in [BIP157][]), allowing | ||
them to search for any matches to their scripts. If a match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnewbery this commit confuses me; why did you restore the prevout terminology after arguing against it?
Oops. I just wanted to revert the change I was talking about here: #246 (comment), and carelessly also re-added the reference to prevout. I think the wording about 'the data necessary to validate the block' or 'some related data' is just confusing and unnecessary for the following reasons:
The fact that a full node can construct a block filter index asynchronously from old data is really an implementation detail. I don't think it's important for understanding the concept. I've added another commit that removes mention of 'additional data'. Let me know what you think. |
Your edit 76bb411 LGTM. Thanks. To nitpick your message a little:
Before the data is placed in the undo file, it's part of the set of data that's necessary to validate a block. After it's in the undo file, it's part of what's used to rewind a block. So I think it's fair to describe it either way.
I don't think I mentioned asynchronicity. I thought it was useful to mention the both data sources because otherwise it's hard for someone already familiar with the protocol to understand how data from just a single block can allow a wallet "to search for any matches to their scripts", since inputs don't necessary identify their scripts (e.g. the input spending a P2PK or bare multisig will just be a random-looking signature or set of signatures). I'm sure you're right about the description getting into the weeds, which is why I'm fine with this removal, but I wanted to explain why I thought it was useful to note. |
This is ready for merge once minisketch (#243) is merged |
76bb411
to
a9a208c
Compare
Squashed fixup commits and cherry-picked the commits from #243. |
Eight proposed pages for the new
/en/topics/
section of the site. Suggested reviewer is @jnewberyNote: tests expected to fail on the final "production" test because of the FIXME link pointing to a topic that's part of a different PR. That's the last test we run, so if it's the only failure, this should be ok.