p2p: Signal support for compact block filters with NODE_COMPACT_FILTERS #19070

pull jnewbery wants to merge 3 commits into bitcoin:master from jnewbery:2020-05-node-compact-filters changing 6 files +49 −34
  1. jnewbery commented at 4:26 pm on May 26, 2020: member
    If -peerblockfilters is configured, signal the NODE_COMPACT_FILTERS service bit to indicate that we are able to serve compact block filters, headers and checkpoints.
  2. jnewbery commented at 4:45 pm on May 26, 2020: member

    This is the final PR in the #18876 series of PRs.

    Note that there are a couple of differences from @jimpo’s original PR:

    1. I didn’t include the BlockUntilSyncedToCurrentChain() call from within PrepareBlockFilterRequest() (https://github.com/bitcoin/bitcoin/pull/16442/files#diff-eff7adeaec73a769788bb78858815c91R2068). I wanted to avoid having the message handler thread blocked on the scheduler thread, which is what services the validation interface queue callbacks. If the scheduler thread is doing some heavy work (eg flushing large wallet state to disk) then the message handler thread could be blocked for a long time, during which we don’t process or respond to messages from any peers. In such a case, I’d prefer simply to not provide the cfilter or cfheader to the requesting client, since they can just rerequest it after a short delay.

    2. I’ve removed the logic that prevents -peerblockfilters being set if the index isn’t synced (https://github.com/bitcoin/bitcoin/pull/16442/files#diff-c865a8939105e6350a50af02766291b7R1791). That code was added in response to review comments that the service bit shouldn’t be set dynamically (https://github.com/bitcoin/bitcoin/pull/16442#issuecomment-555256244 and #16442 (comment)), but it doesn’t address my concern, which is that a node on the network will change its service bits. Those service bits are cached by peers on the network and gossiped in addr messages, so it’s best for them to be as constant as possible. As well as that, it’s a pain for the node operator to have to stop-start the node multiple times to enable this feature. The downside of always signaling NODE_COMPACT_FILTERS is that clients might request cfilters or cfheaders before our index is ready and we won’t be able to respond. There’s no notfound equivalent message in BIP 157 to communicate that to the peer, so we just drop their request. I think this is acceptable because:

      • it’s a small window after startup
      • clients should be robust to not receiving responses to individual requests.

    Of course, both of those decisions are up for debate. If there’s a consensus that my changes aren’t desirable, I’ll happily revert them.

  3. DrahtBot added the label GUI on May 26, 2020
  4. DrahtBot added the label P2P on May 26, 2020
  5. DrahtBot added the label RPC/REST/ZMQ on May 26, 2020
  6. DrahtBot added the label Tests on May 26, 2020
  7. MarcoFalke removed the label GUI on May 26, 2020
  8. MarcoFalke removed the label RPC/REST/ZMQ on May 26, 2020
  9. MarcoFalke removed the label Tests on May 26, 2020
  10. jnewbery force-pushed on May 26, 2020
  11. DrahtBot commented at 7:56 pm on May 27, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18354 (Use shared pointers only in validation interface by bvbfan)

    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.

  12. jimpo commented at 9:07 pm on May 27, 2020: contributor

    Both of these changes from the original PR violate BIP 157 as it is currently written. The BIP was designed to expose a consistent interface to clients, which is conceptually simpler, making it easier to implement client code correctly. The guarantee is that if a node has the COMPACT_FILTER service bit on and advertises that some block is in its main chain, then it must be able to serve the block filter and filter header for it in subsequent requests.

    1. This violates the guarantee because now a block could be connected, a client requests its filter header, but it has not been indexed yet and the node doesn’t respond. I understand your concern, but from my perspective, changing to this behavior would be complicating the protocol because of an implementation limitation in Bitcoin Core’s design. If the networking code had better support for asynchronous/concurrent processing, there would be no need to make this tradeoff. I’m clearly not suggesting waiting until the entire networking stack is rewritten to deploy this, but my objection is that the BIP shouldn’t have to change because of the architecture of one (granted, clearly the most significant) implementation.

    2. I just really don’t understand why it’s preferable to falsely advertise support for some service that you are unable to provide. It seems crazy that a node would say “I support BIP 157”, then some client says “hey, give me BIP 157 data”, then radio silence. The addr advertisements are refreshed once per day or something like that IIRC. So assuming these nodes are up for a while, it’s not too long before their updated service bits are discoverable. But also, it’s not like the service bits are flapping – they just go from 0 to 1 at some point. If it’s a brand new node, then it’s on the whole time, and if it’s already in sync node then to toggles 0 to 1 once. So why make it toggle before the service is actually available? I should also say that this is different from NODE_NETWORK, which is used as a justification for this change. When you connect to a node, you get their start height in the version message, first of all. And also, if a node can’t serve a block, its responses aren’t inconsistent with each other as would be the case if you advertise you have a block but don’t have the block filter.

    Those are my justifications. But this has been debated before, and if others aren’t convinced, then we should at least update the BIP to reflect the more laissez faire behavior.

  13. jnewbery commented at 10:22 pm on May 27, 2020: member

    Thanks for summarising your thoughts @jimpo. As you point out, there’s already been debate about this. For the benefit for other reviewers, here are the comments from the original PR relevant to setting the service bits:

    To summarize my opposition to dynamically setting service bits:

    • it’s an abuse of the concept of service bits, which indicate capabilities, not state.
    • BIP 157 clients have to be robust to failed requests anyway, so failing in these window conditions shouldn’t add much complexity (although a notfound equivalent would be preferable).
    • the current version of BIP 157 adds some implicit state between the client and server (“StopHash MUST be known to belong to a block accepted by the receiving peer.”) which it’d be better to avoid.

    In fact, BIP 157 and BIP 158 seem inconsistent. BIP 158 states “If enabled, the node MUST respond to all BIP 157 messages for filter type 0x00”, but BIP 157 states “A node that receives getcfilters with an unknown StopHash SHOULD NOT respond.” (as well as listing several other conditions where the server shouldn’t respond).

    As I said, if the general consensus is that we should restore the behaviour from #16442, then I can do that. If not, I’m happy to open PRs to modify the BIPs.

  14. DrahtBot added the label Needs rebase on May 29, 2020
  15. Apply cfilters review fixups b3fbc94d4f
  16. [net] Signal NODE_COMPACT_FILTERS if we're serving compact filters. 
    If -peerblockfilters is configured, signal the NODE_COMPACT_FILTERS service
bit to indicate that we are able to serve compact block filters, headers
and checkpoints.
    132b30d9c8
  17. [test] Add test for NODE_COMPACT_FILTER. 
    Test that a node configured to serve compact filters will signal
NODE_COMPACT_FILTER service bit.
    f5c003d3ea
  18. jnewbery force-pushed on Jun 1, 2020
  20. DrahtBot removed the label Needs rebase on Jun 1, 2020
  21. MarcoFalke commented at 12:44 pm on June 1, 2020: member

    re-review and Concept ACK f5c003d3ead182335252558c5c6c9b9ca8968065

    Changes since my previous review #18876 (comment):

    • Adding back NODE_COMPACT_FILTERS test which was temporarily removed for the earlier prs: #18876 (comment)

    • Remove BlockUntilSyncedToCurrentChain: I liked the sync, because previously a light client could ask us for a filter if they received the block inv from us. Now it seems they will need to poll in a loop over p2p to achieve the same. And as you mention that loop doesn’t have a trivial upper bound. So I am not sure removing this call is the right thing.

    • Remove the call to IsSynced when starting the node. This should mostly be relevant for IBD. Some low-powered machines might advertise the service bit days in advance without actually delivering filters. I think we should be nice about this and disconnect light clients as early as possible, since their only choice is to wait and reconnect to us in a few days when we are done with ibd.

  22. jkczyz commented at 5:50 pm on June 1, 2020: contributor
    @jnewbery Does a node serve blocks during IBD? If not, would it be reasonable to simply remain in this state until the index is built?
  23. fjahr commented at 7:20 pm on June 1, 2020: member

    Code review ACK f5c003d3ead182335252558c5c6c9b9ca8968065

    Concerning the conceptual changes:

    • I prefer to not dynamically change the service bit, as you say they advertise capabilities, not state.
    • I am currently not sure about the removal of BlockUntilSyncedToCurrentChain(). I see the arguments for both but I am not sure which weighs heavier.
  24. jnewbery commented at 7:25 pm on June 1, 2020: member

    Does a node serve blocks during IBD?

    I believe the behaviour is:

    If not, would it be reasonable to simply remain in this state until the index is built?

    Yes that’s possible, but I’m not sure if it’s desirable. I don’t think we want block filter index building to interfere with block relay, even if it’s only temporarily.

  25. clarkmoody commented at 10:00 pm on June 3, 2020: none

    Concept ACK f5c003d3ead182335252558c5c6c9b9ca8968065

    I agree that service bits should not signal state.

    I’m also unsure on the BlockUntilSyncedToCurrentChain() story, as I don’t quite understand the implications either way. This seems like an issue that could be addressed in a follow-up PR.

    If BIPs 157 and 158 conflict with one another, then there should definitely be a clarification.

  26. in src/protocol.h:289 in f5c003d3ea 
    284@@ -285,6 +285,9 @@ enum ServiceFlags : uint64_t {
285     // NODE_WITNESS indicates that a node can be asked for blocks and transactions including
286     // witness data.
287     NODE_WITNESS = (1 << 3),
288+    // NODE_COMPACT_FILTERS means the node will service basic block filter requests.
289+    // See BIP157 and BIP158 for details on how this is implemented.
    ariard commented at 11:09 pm on June 3, 2020:
    If it’s likely implementation will diverge from BIP with regards to error handling, it should be hinted somewhere. Better in release notes than here ?
    jnewbery commented at 1:46 am on June 12, 2020:
    Yes, I’m happy to add release notes describing the behavior. I won’t do it in this branch unless I need to retouch it (to not invalidate the three ACKs).
    ysangkok commented at 11:26 pm on June 18, 2020:
    @jnewbery you just requested an ack from MarcoFalke so I dunno if the ACKs have been invalidated?
  27. ariard commented at 11:30 pm on June 3, 2020: member

    Concept and Code Review ACK f5c003d

    I agree with a static service bit for few reasons:

    • Any robust production-ready BIP 157 client will have to implement query timeout and rotation to handle network failures/malicious/buggy nodes. More I think you can have concurrent tip situations like server A receives tip T, announces T to client, server B receives tip T’ but before announcement is made to client this one query a filter-header for T to B. Block T have never been seen and a honest error must be replied. This is similar to a reorg on which BIP 157 is quite mute, and where you may expect a reorg’ed block filters being pruned out after being announced. Also AFAICT, Neutrino devs have already gauged network failures handling to be worthy of implementation.
    • A toggling bit means a BIP 157 service availability will take more time to propagate. It’s likely going to be worse after #18991. From the security viewpoint a faster growing set of BIP 157 servers means a higher cost to eclipse, as a client have now a bigger set to sample from. It’s a slight argument as the percentage increases of BIP 157 joining on the last ~48h are always going to be small compare to the overall already-running mass, but can serve as a tie-breaker with regards to increasing failures.
    • I favor network services bit being interpreted as static capacities rather than current node state. Those index-sync transition could be used as a new fingerprint, specially to map peer across reachable networks (e.g IPv4 and Tor). Note: with actual implementation, a privacy attacker can already do this by probing and logging query failure but at least that imply being active instead of passively listening for ADDR messages

    Starting height in VERSION message isn’t relied to query but only as a starting point to advertise headers from us.

  28. jonatack commented at 4:26 pm on June 4, 2020: member

    ACK f5c003d3e

    Tentatively agree with the arguments in favor of this PR as-is, disconnecting light clients asap when we’re not able to serve, and of updating BIPs 157/158.

    I think it’s also important that serving block filters remain opt-in and not on by default.

    Added test coverage (for a follow-up to not lose existing ACKs, unless you need to retouch and wish to pull it in) for behavior when users do…

    0$ bitcoind -peerblockfilters=1 -blockfilterindex=foo
1Error: Unknown -blockfilterindex value foo.
2
3$ bitcoind -peerblockfilters=1 -blockfilterindex=0
4Error: Cannot set -peerblockfilters without -blockfilterindex.

    Single data point: On a fairly slow ADSL connection and running a debug build with flagsola, it took 10.5 minutes to sync 3401 blocks from height 629635 to 633036. At that rate of ~324 blocks/minute, full IBD would be roughly 32.5 hours from block 0 to 633036. The variation could of course be high; ballpark of 1-2 days at that speed.

    0$ bitcoind -peerblockfilters=1 -blockfilterindex=basic
12020-06-04T14:14:43Z Bitcoin Core version v0.20.99.0-f5c003d3ea (debug build)
2...
32020-06-04T14:15:43Z basic block filter index thread start
42020-06-04T14:15:43Z Syncing basic block filter index with block chain from height 629635
5...
62020-06-04T14:26:03Z Syncing basic block filter index with block chain from height 632989
72020-06-04T14:26:20Z Pre-allocating up to position 0x200000 in fltr00319.dat
82020-06-04T14:26:22Z basic block filter index is enabled at height 633036
92020-06-04T14:26:22Z basic block filter index thread exit
  29. jnewbery commented at 2:08 am on June 12, 2020: member

    There was some discussion in the PR Review Club about the two questions above (blocking the message handler thread on background tasks and whether to signal NODE_COMPACT_FILTERS before the filter index is built). I still feel strongly that we shouldn’t add code in net_processing that blocks on the scheduler thread (and should make a rule that we never do that), and I also think we shouldn’t flip service bits.

    This PR currently has 3 ACKs. If anyone feels strongly that it’s a mistake, they should speak up here.

  30. MarcoFalke commented at 12:17 pm on June 12, 2020: member

    This is an optional service that is offered to light clients to provide them data that is hopefully useful to them. It seems a bit odd to signal support for that, but then degrade the user experience of light clients by making the service more fragile.

    Disconnecting early when the filter is still syncing should be a one-line check in our c++ code that helps light clients save 20 seconds. The added complexity here should be negligible compared to the overall complexity of blockfilters.

    Also, Bitcoin Core is run on less-than-high-end machines or virtual cloud providers, which are known to have slow storage. Turning all of these nodes that have p2p blockfilters enabled into nodes that might not respond to x% of requests is increasing delay and bandwidth for light clients. Light clients need to repeat and re-repeat requests to different nodes. This is added overhead on top of the overhead caused by redundancy for asking multiple nodes for increased “security”, if they implement that.

  31. jnewbery commented at 8:15 pm on June 12, 2020: member
    @MarcoFalke : what’s the request here? Will you be happy if I changed the behavior when a filter can’t be served to just disconnect immediately?
  32. MarcoFalke commented at 8:41 pm on June 12, 2020: member

    I haven’t written the code, but I am assuming that the disconnect can be done with minimal added complexity and it would indeed make me like the changes here more.

    Though, looking into the future, p2p filters might not only be serviced to random 3rd party clients, but maybe also to your own light client (via a trusted connection), in which case redundancy is neither required nor possible. So a failure to serve a filter can only be answered with a polling loop. So I’d also argue to bring back the block-until-synced call. Note that the block-until-synced call is a noop if the filter is already synced with the chain.

  33. jnewbery force-pushed on Jun 18, 2020
  34. jnewbery commented at 9:44 pm on June 18, 2020: member
    I’ve updated this so that we disconnect immediately if we’re not able to service a request. @MarcoFalke - care to rereview?
  35. in src/net_processing.cpp:2134 in fe2e6bb067 outdated 
    2130@@ -2131,6 +2131,7 @@ static void ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv, const CChainPar
2131         if (!filter_index->LookupFilterHeader(prev_block, prev_header)) {
2132             LogPrint(BCLog::NET, "Failed to find block filter header in index: filter_type=%s, block_hash=%s\n",
2133                          BlockFilterTypeName(filter_type), prev_block->GetBlockHash().ToString());
2134+            peer.fDisconnect = true;
    MarcoFalke commented at 2:34 pm on June 21, 2020:

    in commit fe2e6bb06705f72346ed636becc60c45075a451e

    It looks like this could disconnect a permissioned peer, which is probably unwanted. See the second part of #19070 (comment)

    I’d prefer to wait until we can reply to the peer with a proper result.

    jnewbery commented at 5:39 pm on June 21, 2020:
    Reverted
  36. jnewbery force-pushed on Jun 21, 2020
  37. jnewbery commented at 5:40 pm on June 21, 2020: member
    ok, I’ve reverted back to the version that has 3 ACKs.
  38. MarcoFalke commented at 9:47 am on June 22, 2020: member

    Yeah, sorry for being unclear in what situations to disconnect. I will create a follow-up pull later this week unless someone beats me to it.

    This might be ready for merge.

  39. jnewbery commented at 8:06 pm on July 22, 2020: member
    @MarcoFalke ready for merge, or does this require something else?
  41. laanwj merged this on Aug 13, 2020
  42. laanwj closed this on Aug 13, 2020
  43. jnewbery deleted the branch on Aug 13, 2020
  44. sidhujag referenced this in commit 3fa39d8821 on Aug 14, 2020
  45. Fabcien referenced this in commit 25e87193b5 on Dec 21, 2020
  46. Fabcien referenced this in commit a73a90d60a on Dec 21, 2020
  47. Fabcien referenced this in commit aa5b13600c on Dec 21, 2020
  48. kittywhiskers referenced this in commit 664f5a9cb0 on Aug 22, 2021
  49. kittywhiskers referenced this in commit 3ead9d5d16 on Aug 22, 2021
  50. kittywhiskers referenced this in commit 5cbdde7006 on Sep 16, 2021
  51. kittywhiskers referenced this in commit e73156984f on Sep 18, 2021
  52. kittywhiskers referenced this in commit c7f08018d9 on Sep 19, 2021
  53. UdjinM6 referenced this in commit 4ffd42de63 on Sep 21, 2021
  54. thelazier referenced this in commit da9dee669c on Sep 25, 2021
  55. kittywhiskers referenced this in commit 20433810dc on Oct 12, 2021
  56. DrahtBot locked this on Feb 15, 2022


jnewbery DrahtBot jimpo MarcoFalke jkczyz fjahr clarkmoody ariard ysangkok jonatack

Labels
P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me