Serve cfcheckpt requests #18877
Serve cfcheckpt requests #18877
Conversation
|
Seems the |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
63659fb to
493eb3b
Compare
493eb3b to
6fdf2ea
Compare
6fdf2ea to
18ad397
Compare
|
Note to reviewers: the Travis failures are timeouts and can be ignored. This PR is ready for review. |
18ad397 to
fccc857
Compare
test/functional/p2p_cfilters.py
Outdated
| wait_until, | ||
| ) | ||
|
|
||
| class CompactFiltersTest(BitcoinTestFramework): |
There was a problem hiding this comment.
Is this test verify the following spec requirement "StopHash MUST be known to belong to a block accepted by the receiving peer. This is the case if the peer had previously sent a headers or inv message with any descendent blocks" ?
This point is unclear to me with regards to the receiver requirement, what should we do in case of peer asking for a StopHash non-yet-announced to them. Do we check StopHash inferior ou equal to its pindexBestHeaderSent somewhere ?
There was a problem hiding this comment.
I don't think it matters. If a BIP 157 client pre-emptively requests a filter header for a block that the server hasn't yet accepted, then they would be disconnected.
Generally we don't consider block propagation timing to be a privacy/topology leak, since new block events are rare and too expensive to be controlled by an observer.
Requesting a block that the server hasn't accepted yet is logically equivalent to requesting a non-existent block, which is tested in p2p_cfilters.
There was a problem hiding this comment.
Generally we don't consider block propagation timing to be a privacy/topology leak, since new block events are rare and too expensive to be controlled by an observer.
I think relying on event scarcity in case of block variance may not hold, so I would rather consider the heavily-optimized block propagation as an obstruction for an attacker to fingerprint topology with confidence.
Requesting a block that the server hasn't accepted yet is logically equivalent to requesting a non-existent block, which is tested in p2p_cfilters.
I think there is logically a difference between accepting a block and header announcement, but AFAIK there is no types of connections for which we don't announce headers so doesn't make a difference in practice, I agree.
fccc857 to
f874115
Compare
|
Rebased on master to fix the spurious travis failures, and addressed @jkczyz's review comments. Thanks for the review! |
f874115 to
2a15f99
Compare
|
Addressed @ariard's review comments. |
There was a problem hiding this comment.
Code review, built/ran tests, and running a mainnet node with -peercfilters=1 -blockfilterindex=1... which took a bit more than an hour to build ~./bitcoin/indexes/blockfilter/basic for the first time, which at current tip height of 629407 takes 5.0 GB of disk space -- not strictly related to this PR, but I wanted to test it. Verified the behavior for "Error: Cannot set -peercfilters without -blockfilterindex." A few non-blocking comments follow below.
There was a problem hiding this comment.
ACK fccc857
Reviewed code, ran tests, and modified them to see them fail.
Had another nit. And this is also not a blocker for me but I would like to repeat my comment from the PR Review Club that I would prefer that we have a consistent name for this feature for user-facing communication. The connection between -blockfilterindex and -peercfilters does not seem obvious. I am happy with either naming scheme as long as it's consistent.
2a15f99 to
6691493
Compare
|
Code Review ACK 6691493, changes since last review are switch to |
|
Thanks for review @jonatack . I've addressed all your comments. |
Needs push. Also, please update the flag name in commit messages. |
|
re-ACK 2308385 Only change was fixing merge conflict in |
|
re-Code Review ACK 2308385 |
|
This remains Concept NACK since it is harmful to Bitcoin and should not be merged, no matter how well-reviewed. |
|
I also believe neutrino is harmful to bitcoin https://medium.com/@nicolasdorier/neutrino-is-dangerous-for-my-self-sovereignty-18fac5bcdc25 However, I believe it can be useful to whitelisted peers. (I think you should add a permission flag for that) Concept NACK, except if protected by a permission flag to force its use only to specific trusted peers. |
|
All three test suites passed here. |
|
Tested ACK 2308385 |
|
re-ACK 2308385 🌳 Only change since my previous review #18876 (comment)
Show signature and timestampSignature: Timestamp of file with hash |
The implementation here is incomplete. The follow-ups in #18876 need to be merged as well. And if another setting is seen to be needed for this to be safely shipped, this should also be done in a follow-up. However, I think we shouldn't block progress here based on Bitcoin Core related implementation details that can easily be fixed up later. There is enough time until the release to hash this out. (See #18947) If by then the implementation is still incomplete, it should be reverted. Given that, I think this is ready for merge, both code-wise and conceptually. |
2308385 [test] Add test for cfcheckpt (Jim Posen) f9e00bb [net processing] Message handling for getcfcheckpt. (Jim Posen) 9ccaaba [init] Add -peerblockfilters option (Jim Posen) Pull request description: Serve cfcheckpt messages if basic block filter index is enabled and `-peercfilters` is set. `NODE_COMPACT_FILTERS` is not signaled to peers, but functionality can be used for testing and serving pre-configured clients. ACKs for top commit: jonatack: Code review re-ACK 2308385 the only change since my review @ 967e2b1 is an update required for bitcoin#16224 that was merged yesterday. fjahr: re-ACK 2308385 jkczyz: re-ACK 2308385 ariard: re-Code Review ACK 2308385 clarkmoody: Tested ACK 2308385 MarcoFalke: re-ACK 2308385 🌳 theStack: ACK bitcoin@2308385 Tree-SHA512: 8c751bbd7d1c31a413096462ae025c3d2f3163c7016cbec472a5f5ec267f8dd19a2dfc4d749876d7409c1db546e6fdd16461c6863effcfa0d3e993edcfa92a08
|
post merge concept ACK. However, I think this merge was too fast. A 6 day window for a first concrete step in filter serving after BIP157 is IMO too fast for a reasonable discussion. I also still miss conceptual reviews from long term p2p contributors. There is no rush IMO. Edit: I probably underestimated the effort that went into #16442 (this PR is mostly recycled code from 16442) which could legitimate a quicker merge. |
When a node is configured with --blockfilterindex=basic and -peerblockfilters it can serve compact block filters to its peers. This commit adds the configuration option handling. Future commits add compact block serving and service bits signaling. Github-Pull: bitcoin#18877 Rebased-From: 9ccaaba
If -peerblockfilters is configured, handle requests for cfcheckpt. Github-Pull: bitcoin#18877 Rebased-From: f9e00bb
Github-Pull: bitcoin#18877 Rebased-From: 2308385
Summary: ``` Serve cfcheckpt messages if basic block filter index is enabled and -peercfilters is set. NODE_COMPACT_FILTERS is not signaled to peers, but functionality can be used for testing and serving pre-configured clients. ``` Backport of core [[bitcoin/bitcoin#18877 | PR18877]]. Depends on D8461 and D8462. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8463
Serve cfcheckpt messages if basic block filter index is enabled and
-peercfiltersis set.NODE_COMPACT_FILTERSis not signaled to peers, but functionality can be used for testing and serving pre-configured clients.