Serve cfcheckpt requests

[jnewbery]

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.

[prusnak]

Seems the lint target fails in the Travis CI.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• [WIP] Serve BIP 157 compact filters #18876 ([WIP] Serve BIP 157 compact filters by jnewbery)
• Make g_chainman internal to validation #18698 (Make g_chainman internal to validation by MarcoFalke)
• net: Use mockable time for ping/pong, add tests #18638 (net: Use mockable time for ping/pong, add tests by MarcoFalke)
• Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242 (Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli)
• Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails #17479 (Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery)

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.

[jnewbery]

Note to reviewers: the Travis failures are timeouts and can be ignored. This PR is ready for review.

[jkczyz]

Concept ACK

[ariard]

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 ?

[jnewbery]

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.

[ariard]

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.

[jnewbery]

Rebased on master to fix the spurious travis failures, and addressed @jkczyz's review comments. Thanks for the review!

[jnewbery]

Addressed @ariard's review comments.

[fjahr]

Concept ACK

[jonatack]

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.

[fjahr]

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.

[jnewbery]

Addressed all review comments from @fjahr and @jonatack . Thanks for the review!

[ariard]

Code Review ACK 6691493, changes since last review are switch to int instead of uint32_t, renaming to peerblockfilters, test modifications, better comments.

[jonatack]

Re-built and tested successfully. A few comments following the change from -peercfilters to -peerblockfilters.

[jnewbery]

Thanks for review @jonatack . I've addressed all your comments.

[jkczyz]

Thanks for review @jonatack . I've addressed all your comments.

Needs push. Also, please update the flag name in commit messages.

[fjahr]

re-ACK 2308385

Only change was fixing merge conflict in init.cpp:

$ git range-diff f54753293fe7355e4280944d766f22054b560ba1..967e2b10a51b831e49abd9171e3f05843e00e76b 5b24f6084ede92d0f493ff416b4726245140b2c1..23083856a551ca13e8b142791c296ecb25cc4e7f

[jkczyz]

re-ACK 2308385

Only change required in init.cpp for #16224.

[ariard]

re-Code Review ACK 2308385

[luke-jr]

This remains Concept NACK since it is harmful to Bitcoin and should not be merged, no matter how well-reviewed.

[NicolasDorier]

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.

[ThomasBucaioni]

All three test suites passed here.

[clarkmoody]

Tested ACK 2308385

[theStack]

ACK 2308385

[jnewbery]

This PR now has 6 ACKs:

• @jonatack (Serve cfcheckpt requests #18877 (comment))
• @fjahr (Serve cfcheckpt requests #18877 (comment))
• @jkczyz (Serve cfcheckpt requests #18877 (comment))
• @ariard (Serve cfcheckpt requests #18877 (comment))
• @clarkmoody (Serve cfcheckpt requests #18877 (comment))
• @theStack (Serve cfcheckpt requests #18877 (review))

The PR that this is a subset of also has an ACK:

• @MarcoFalke (Serve BIP 157 compact filters #16442 (comment))

And there are 6 additional Concept ACKs for the change:

• @Empact ([WIP] Serve BIP 157 compact filters #18876 (comment))
• @michaelfolkson ([WIP] Serve BIP 157 compact filters #18876 (comment))
• @jamesob (Serve BIP 157 compact filters #16442 (comment))
• @Sjors (Serve BIP 157 compact filters #16442 (comment))
• @marcinja (Serve BIP 157 compact filters #16442 (review))
• @brakmic (Serve BIP 157 compact filters #16442 (comment))

That's 13 ACKs/Concept ACKs. There does appear to be widespread support to move forwards with this.

@luke-jr @NicolasDorier - your principled objection to BIP 157 has been heard and understood. Thank you for your input!

[maflcko]

re-ACK 2308385 🌳

Only change since my previous review #18876 (comment)

• Rename setting
• Small code and test refactoring

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 23083856a5 🌳

Only change since my previous review https://github.com/bitcoin/bitcoin/pull/18876#issuecomment-627322015

* Rename setting
* Small code and test refactoring
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgPkAv/fzn4E/8T8kcqmj3aMGV6rY1b0Kg0cqRZKVMrfapyWIJxuQ0+WiB7Leyl
TbKLG8hXCWHy/GNdtkJjhXKtQDH4NA/m+jlIhTMIYCsi1YZ9IPzKLPm9NvuEQSp0
MRmfkDFH/RMIgxSDROm96+StbWlunDDjXvy7recX7hLJ1pMAfufEiyVbqDHwuCjw
K9lG2YbdBWwUxo6zP1iQkBV+DwTl0GNuEtF47e6oQUozAWNosBw0efBEDGNN41c6
Li1aPBXnZUHd/Uwioxz0Ar0+N06VWz0b5Uz4NDOK//Bqu4sf3djZY4WEF/TdlrSs
sXy8LIhyL3uCnSWKUL2L4zoKkYjImJthLk8MIdcFnFLotQO/Za1yDZ/ur/yKI7ee
bYqLh7vbW8oupsKigJr25jtPZY1+p5EmdeFh+/4JYsSG/Da5gGryQ+AcWAi9Hk1/
7U4XhAwiwyLEvadJpPJGuCuFKZXl89KCCKKTY8x/nuNzWuvzSNqv183muNIhrCqk
IIprWPI3
=Dnd8
-----END PGP SIGNATURE-----

Timestamp of file with hash 286f8e8c18188195dc7b2da1cd9ec5c45e6b4ff0cfa7173476f9af073c383815 -

[maflcko]

Concept NACK, except if protected by a permission flag to force its use only to specific trusted peers.

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.

[jonasschnelli]

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.

[maflcko]

Edit: I probably underestimated the effort that went into #16442 (this PR is mostly recycled code from 16442) which could legitimate a quicker merge.

Thanks. Just wanted to reply that.

For reference, the permission flag pr is here, btw: #18972