Skip to content
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

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

Merged
merged 3 commits into from Aug 13, 2020

Conversation

@jnewbery
Copy link
Member

@jnewbery jnewbery commented May 26, 2020

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.

@jnewbery
Copy link
Member Author

@jnewbery jnewbery commented May 26, 2020

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 (#16442 (comment) 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.

Loading

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented May 27, 2020

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.

Loading

@jimpo
Copy link
Contributor

@jimpo jimpo commented May 27, 2020

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.

Loading

@jnewbery
Copy link
Member Author

@jnewbery jnewbery commented May 27, 2020

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.

Loading

jnewbery and others added 3 commits Jun 1, 2020
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.
Test that a node configured to serve compact filters will signal
NODE_COMPACT_FILTER service bit.
@jnewbery jnewbery force-pushed the 2020-05-node-compact-filters branch from e1ee720 to f5c003d Jun 1, 2020
@jnewbery
Copy link
Member Author

@jnewbery jnewbery commented Jun 1, 2020

Rebased on master and applied suggested changes by @fjahr (#19044 (comment)) and @jkczyz (#19044 (comment), #19044 (comment))

Loading

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jun 1, 2020

re-review and Concept ACK f5c003d

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.

Loading

@jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Jun 1, 2020

@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?

Loading

@fjahr
Copy link
Contributor

@fjahr fjahr commented Jun 1, 2020

Code review ACK f5c003d

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.

Loading

@jnewbery
Copy link
Member Author

@jnewbery jnewbery commented Jun 1, 2020

Does a node serve blocks during IBD?

I believe the behaviour is:

  • we don't relay headers from blocks when we connect them (obviously, since we're not yet at the most work tip):

    bitcoin/src/net_processing.cpp

    Lines 1311 to 1316 in 9bc7751

    void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {
    const int nNewHeight = pindexNew->nHeight;
    connman->SetBestHeight(nNewHeight);
    SetServiceFlagsIBDCache(!fInitialDownload);
    if (!fInitialDownload) {
  • we won't respond if a peer sends us a getheaders message:

    bitcoin/src/net_processing.cpp

    Lines 2746 to 2749 in 9bc7751

    if (::ChainstateActive().IsInitialBlockDownload() && !pfrom->HasPermission(PF_NOBAN)) {
    LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d because node is in initial block download\n", pfrom->GetId());
    return true;
    }
  • we'll still respond if a peer sends us a getdata message for a block that we've fully validated:
    void static ProcessGetBlockData(CNode* pfrom, const CChainParams& chainparams, const CInv& inv, CConnman* connman)

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.

Loading

@clarkmoody
Copy link

@clarkmoody clarkmoody commented Jun 3, 2020

Concept ACK f5c003d

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.

Loading

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jun 12, 2020

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.

Loading

@jnewbery
Copy link
Member Author

@jnewbery jnewbery commented Jun 12, 2020

@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?

Loading

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jun 12, 2020

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.

Loading

@jnewbery jnewbery force-pushed the 2020-05-node-compact-filters branch from f5c003d to fe2e6bb Jun 18, 2020
@jnewbery
Copy link
Member Author

@jnewbery jnewbery commented Jun 18, 2020

I've updated this so that we disconnect immediately if we're not able to service a request. @MarcoFalke - care to rereview?

Loading

src/net_processing.cpp Outdated Show resolved Hide resolved
Loading
@jnewbery jnewbery force-pushed the 2020-05-node-compact-filters branch from fe2e6bb to f5c003d Jun 21, 2020
@jnewbery
Copy link
Member Author

@jnewbery jnewbery commented Jun 21, 2020

ok, I've reverted back to the version that has 3 ACKs.

Loading

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jun 22, 2020

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.

Loading

@jnewbery
Copy link
Member Author

@jnewbery jnewbery commented Jul 22, 2020

@MarcoFalke ready for merge, or does this require something else?

Loading

@jnewbery
Copy link
Member Author

@jnewbery jnewbery commented Aug 11, 2020

This PR has ACKs/concept ACKs from:

I asked for people to speak up if they had any objection two months ago (#19070 (comment)) and there hasn't been any substantial opposition.

I think this PR is ready for merge.

Loading

@laanwj laanwj merged commit b4d0366 into bitcoin:master Aug 13, 2020
3 checks passed
Loading
@jnewbery jnewbery deleted the 2020-05-node-compact-filters branch Aug 13, 2020
sidhujag added a commit to syscoin/syscoin that referenced this issue Aug 14, 2020
…th NODE_COMPACT_FILTERS

f5c003d [test] Add test for NODE_COMPACT_FILTER. (Jim Posen)
132b30d [net] Signal NODE_COMPACT_FILTERS if we're serving compact filters. (Jim Posen)
b3fbc94 Apply cfilters review fixups (John Newbery)

Pull request description:

  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.

ACKs for top commit:
  MarcoFalke:
    re-review and Concept ACK f5c003d
  fjahr:
    Code review ACK f5c003d
  clarkmoody:
    Concept ACK f5c003d
  ariard:
    Concept and Code Review ACK f5c003d
  jonatack:
    ACK f5c003d

Tree-SHA512: 34d1c153530a0e55d09046fe548c9dc37344b5d6d50e00af1b4e1de1e7b49de770fca8471346a17c151de9fe164776296bb3dd5af331977f0c3ef1e6fc906f85
@buck54321 buck54321 mentioned this pull request Oct 22, 2020
10 tasks
Fabcien added a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Dec 21, 2020
Summary:
Partial backport (1/3) of core [[bitcoin/bitcoin#19070 | PR19070]]:
bitcoin/bitcoin@b3fbc94

Depends on D8729.

Test Plan:
  ninja

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D8730
Fabcien added a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Dec 21, 2020
Summary:
```
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.
```

Partial backport (2/3) of core [[bitcoin/bitcoin#19070 | PR19070]]:
bitcoin/bitcoin@132b30d

Depends on D8730.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D8731
Fabcien added a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Dec 21, 2020
Summary:
```
Test that a node configured to serve compact filters will signal
NODE_COMPACT_FILTER service bit.
```

Completes backport (3/3) of core [[bitcoin/bitcoin#19070 | PR19070]]:
bitcoin/bitcoin@f5c003d

Depends on D8731.

Test Plan:
  ninja check-functional

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D8732
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Aug 22, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Aug 22, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Sep 16, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Sep 18, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Sep 19, 2021
thelazier added a commit to thelazier/dash that referenced this issue Sep 25, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet