Serve BIP 157 compact filters #16442

jimpo · 2019-07-23T16:49:00Z

This enables nodes to serve block filters compliant with BIP 157. This is the last PR required for BIP 157/158 compatibility.

If the node has the block filter index enabled, once the index is in sync, the node will begin signaling the BIP 158 service bit and responding to BIP 157 requests. On invalid requests, the node disconnects the offending peer.

This is tested using functional tests. I have also synced an lnd testnet node using the neutrino backend connected to a Core node running this code.

Questions:

Should there be a separate CLI flag other than -blockfilterindex to enable serving of filters from the index?
Is the mechanism to only signal the service flag once the index is in sync OK?
Is the use of boost::shared_lock around the checkpoint cache acceptable?

DrahtBot · 2019-07-23T18:29:33Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net: Use mockable time for ping/pong, add tests #18638 (net: Use mockable time for ping/pong, add tests by MarcoFalke)
util: Use locale independent ToString(…) instead of locale dependent strprintf(…) for low-level string formatting #18450 (util: Use locale independent ToString(…) instead of locale dependent strprintf(…) for low-level string formatting by practicalswift)
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)
Consolidate service flag bit-to-name conversion to a shared serviceFlagToStr function #18165 (Consolidate service flag bit-to-name conversion to a shared serviceFlagToStr function by luke-jr)
[WIP] Index for UTXO Set Statistics #18000 (Index for UTXO Set Statistics by fjahr)
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.

jonasschnelli · 2019-07-23T18:41:54Z

Nice! Thanks for working on this.

Should there be a separate CLI flag other than -blockfilterindex to enable serving of filters from the index?

I think so,... because some users may want to use the blockfilters only "internal", more specific, to rescan wallets (especially in prune mode), though functionality hasn't been added to Bitcoin Core yet

Is the mechanism to only signal the service flag once the index is in sync OK?

I'd say yes. But unsure since I think we also signal NODE_NETWORK when in IBD.

jamesob · 2019-07-24T13:58:07Z

Concept ACK - will review within the next few days.

TheBlueMatt

Just reviewed the first bit but I presume some of these comments will apply later too.

TheBlueMatt · 2019-07-29T20:10:24Z

src/net.cpp

@@ -2597,7 +2599,18 @@ uint64_t CConnman::GetTotalBytesSent()

 ServiceFlags CConnman::GetLocalServices() const
 {
-    return nLocalServices;
+    uint64_t local_services = nLocalServices;
+    if (local_services & NODE_COMPACT_FILTERS) {


Please dont add more node logic in net.cpp - if you want node logic put it in net_processing. But, honestly, this should be handled either by init checking these conditions or by the filtering code itself.

The point of this code is that the filter index may be building in the background and we don't want to signal NODE_COMPACT_FILTERS to peers until it is in sync. Then as soon as it does finish syncing, turn it on.

I'm not sure this is the best way to do this so, I'm open to suggestions (this is the subject of two of the questions in the PR description). Another option would be to have a separate CLI flag for signalling NODE_COMPACT_FILTERS, which also blocks the net threads until the filter indices are in sync. That way the user can sync the filter index in the background, then shut down the node and restart with the BIP 157 net stuff enabled. Thoughts?

I spent a few minutes writing an indignant comment about how we should be okay with taking this small hack on as technical debt, but it actually looks like it might be pretty easy to address @TheBlueMatt's concerns by adding a method to CConman (eg AddLocalService(ServiceFlags flag)) that just modifies nLocalServices. The indexing code could call it on sync completion. Any reason not to do that?

(FWIW I hate the idea of blocking net threads while waiting for filter indices to sync, and slightly less hate requiring a restart to serve the filters.)

@jamesob Yes, that would work too, but I wouldn't want to index to call CConmann directly. It should do it through some sort of callback to avoid coupling them, which just creates more indirection. I think this approach is a bit easier, but I'd be OK with a callback that updates nLocalServices as well.

Just had an offline conversation with @sdaftuar here (with regard to #16847) which I think is relevant. Dynamically flipping nLocalServices bits doesn't make a ton of sense because we'd need to disconnect and reconnect to all of our peers in order for the flip to have any effect.

When thinking about this, it's instructive to look at how IBD works: we don't flip NODE_NETWORK off and on once we've started and finished IBD; instead net_processing just behaves differently during IBD and peers are able to gracefully handle nodes that signal NODE_NETWORK but haven't completed the sync yet and therefore drop getheaders messages.

I think the way forward in this case is to do something analogous: signal NODE_COMPACT_FILTERS from startup when appropriate and ensure that nodes seeking block filters account for the fact that a peer they request filters from may be in the process of building them.

Dynamically flipping nLocalServices bits doesn't make a ton of sense because we'd need to disconnect and reconnect to all of our peers in order for the flip to have any effect.

I disagree with this. New inbound connections will see that the service bit has been flipped. And clients who need the filters wouldn't connect until it is on, so it's not like any inbound connections need to periodically disconnect/reconnect. And if I understand correctly, we re-advertise our local address with service bits to each peer every day on average, so the network should see the change eventually without the node operator having to do anything.

instead net_processing just behaves differently during IBD and peers are able to gracefully handle nodes that signal NODE_NETWORK but haven't completed the sync yet and therefore drop getheaders messages.

This feels a bit different because the version message includes a start height indicating how far behind the node is. I suppose clients could also just request checkpoints and determine how far behind a node's filter index is at least to within 1,000 blocks though.

In any case, can you simply modify nLocalServices directly when the index has finished syncing? Or does other code that accesses it directly need to see the bit set even before sync is done?

I have removed the dynamic flipping in accordance with reviewers' suggestions.

src/net_processing.cpp

jamesob

3499e88

Code looks great! Especially the tests. A few small comments, but this is very close to an ACK from me.

Review checklist

Tests

Built and run locally for about a day so far with no problems. Synced filters
from somewhere in the mid 500ks.

Confirmed that the service bit flips appropriately: During filter syncing:

$ ./src/bitcoin-cli -datadir=/data/bitcoin getnetworkinfo
  ...
  "localservices": "0000000000000409",

After filter syncing:

$ ./src/bitcoin-cli -datadir=/data/bitcoin getnetworkinfo
  ...
  "localservices": "0000000000000449",

The comprehensive functional test coverage is cause for optimism.

jamesob · 2019-08-08T14:48:52Z

src/blockfilter.h

-        s << m_block_hash
-          << static_cast<uint8_t>(m_filter_type)
+        s << static_cast<uint8_t>(m_filter_type)
+          << m_block_hash


These changes don't require reindexing because we manually spell out the serialization format when writing to disk (and same with reads) instead of using these serialization ops, right?

src/net_processing.cpp

Sjors · 2019-08-13T16:01:12Z

Concept ACK

Should there be a separate CLI flag other than -blockfilterindex to enable serving of filters from the index?

Yes, suggest defaulting to true. It can wait until #15845.

Is the mechanism to only signal the service flag once the index is in sync OK?

I guess it's already built, but I'd be fine with moving that to a followup (and kick the src/net.cpp discussion can down the road). For fresh nodes the index is synced along with IBD. For existing nodes which add the index there's a slight lag between when this setting is enabled and when it's available. These flags take time to get gossiped around. The odds of another node wasting time because of that seems orders of magnitude smaller than the odds of the node just being offline.

On macOS 10.14.6 when configured with --enable-debug the newly added p2p_cfilters.py ignites my fan and failed the first time with Block sync timed out: (passed the second time).

luke-jr · 2019-08-20T00:38:44Z

Should there be a separate CLI flag other than -blockfilterindex to enable serving of filters from the index?

BIP 157 is controversial, so I think yes.

marcinja

Concept ACK. Reviewed each commit and ran tests for each of them. I also did a full IBD and verified that the NODE_COMPACT_FILTERS flag is flipped on.

test/functional/p2p_cfilters.py

src/net_processing.cpp

marcinja · 2019-08-27T02:11:37Z

src/net_processing.cpp

+ * is updated asynchronously, has not been synchronized with the net processing thread. In that
+ * case, block for a short time until the filter index is updated, then retry the lookup.
+ */
+static bool QueryFilterIndexWithRetry(BaseIndex* index, bool& in_sync, std::function<bool()> fn)


Rather than block the message handler thread until the filter index has synced, is there much downside to ignoring messages when a filter query fails because it is not yet synced? (i.e. not sending anything back to the peer as implemented before 77653ec)

By the spec in BIP 157, clients should be downloading from multiple peers so they wouldn't be wasting much bandwidth because of dropped messages caused by this. Blocking the message handler thread just seems to have more downsides IMO.

It just makes the client logic more complicated as they need to have timeouts and if the index is active (like it has gotten in sync once since the node has been running), it should catch up to the tip pretty fast.

Sjors · 2019-09-01T11:13:53Z

I spun up a temporary testnet node in case anyone wants to test the p2p behavior: uskjxnd7tud7qkeg.onion:18333

practicalswift · 2019-09-07T09:32:13Z

test/functional/p2p_cfilters.py

+            computed_cfhash = uint256_from_str(hash256(cfilter.filter_data))
+            assert_equal(computed_cfhash, cfhash)
+
+        # Check thet peers can fetch cfilters for stale blocks.


Should be "that"? :-)

Recent questions have come up regarding dynamic service registration (see bitcoin#16442 (comment) and the assumeutxo project, which needs to dynamically flip NODE_NETWORK). While investigating how dynamic service registration might work, I was confused about how we convey local services to peers. This adds some documentation that hopefully clarifies this process.

…ertised 82e53f3 doc: add comments clarifying how local services are advertised (James O'Beirne) Pull request description: Recent questions have come up regarding dynamic service registration (see #16442 (comment) and the assumeutxo project ~~which needs to dynamically flip NODE_NETWORK~~). While investigating how dynamic service registration might work, I was confused about how we convey local services to peers. This adds some documentation that hopefully clarifies this process. ACKs for top commit: laanwj: ACK 82e53f3 darosior: ACK 82e53f3 Tree-SHA512: a30c1020387d7a75b5b3cdde45f8b7f2ae46293da97e6227b2ee17e290b93deb5b16c0bbc2b1676972300e5c3c2ad74eb8b3910d6b93e028dac1ae2700468ef9

…are advertised 82e53f3 doc: add comments clarifying how local services are advertised (James O'Beirne) Pull request description: Recent questions have come up regarding dynamic service registration (see bitcoin#16442 (comment) and the assumeutxo project ~~which needs to dynamically flip NODE_NETWORK~~). While investigating how dynamic service registration might work, I was confused about how we convey local services to peers. This adds some documentation that hopefully clarifies this process. ACKs for top commit: laanwj: ACK 82e53f3 darosior: ACK 82e53f3 Tree-SHA512: a30c1020387d7a75b5b3cdde45f8b7f2ae46293da97e6227b2ee17e290b93deb5b16c0bbc2b1676972300e5c3c2ad74eb8b3910d6b93e028dac1ae2700468ef9

jamesob · 2019-09-27T15:33:59Z

@jimpo any plans to address feedback here? It'd be great to see this PR move towards merge.

ariard · 2020-05-04T20:34:28Z

@MarcoFalke It's not a question of node resources management, but a more broader concern about light clients current scalability model. Even with a low-bandwidth protocol such as BIP157/158, you may still have a huge discrepancy between the numbers of requesting clients and opt-in public peers to serve them.

At its heart, you will always have a incentives conflict between clients asking for resources and offering nothing back and full-node operators. Unless your light client protocol is so ridiculous cheap to rely on niceness of a subset of nodes operators offering free resources, it won't scale for thousands or millions of clients. And it's likely you will always have a ratio disequilibrium between numbers of clients and numbers of full-node, even worst their growth rate won't be the same, first one are so much easier to setup.

I have been through mailing posts about BIP157/158 design but haven't seen any realistic projections and specially what people do aim to achieve by integrating this at the p2p layer. I do understand people may be interested to use compact filters for their own Neutrino-style client and if so they should use the RPC interface. I'm worried about wallet vendors building such chain access backend for people not connecting to their own full-node but directly to the broader network, which maybe fine now because you have really few BIP157 clients, but going to hit a bandwidth scalability wall few years from now instead of pursing better solutions.

Maybe I'm completely wrong, missing some numbers and I would be super glad to read about what have been previously thought on this issue. In the meanwhile I'm Concept NACK on this and happy to bring it during next meeting and explained issues as I perceived it on the ml.

Note, that doesn't restrain to get this PR ready for merge.

jnewbery · 2020-05-04T20:53:04Z

@ariard please take discussion of light-client scalability to the mailing list. Comments on this PR should be for review of the implementation.

luke-jr · 2020-05-04T21:39:19Z

@ariard This is an optional feature, not enabled by default. It might make some sense to support it for trusted peers only (missing from this PR), although protocols like Stratum are probably better for that purpose (but nobody has implemented it for Core to date).

@jnewbery Concept NACKs, and the explanation therefore (required by dev notes!) do belong on PRs...

I think I'm going to mirror @ariard's Concept NACK. We do have bloom already for trusted peers, and using this for public peers does indeed harm the network. We can't stop anyone else from deploying it, but with Core's monopoly, that hardly matters.

sipa · 2020-05-04T21:56:10Z

@ariard Of course there are always questions about incentives to provide public data for free by nodes, but that problem isn't unique to BIP157 - you can also question if nodes have reason to provide blocks, transactions, to participate in relay, ... etc. Perhaps there is a future where all these things eventually have to be paid for, but for now, trying to solve this perfectly would just lead you to the conclusion that all non-miner nodes should be turned off, and if someone wants to audit the chain they can connect directly to miners. The relevant question is how BIP157 costs compare to services already offered, which doesn't seem very bad to me.

sipa · 2020-05-04T22:03:17Z

@luke-jr Certainly reasons to reject an implementation of a BIP should go on a PR, but you have principled objections to a BIP - which seems to be the case here - it is much more useful to have that criticism be visible more widely. It would be strange if there is apparent consensus that a BIP is a good idea, and then having it not be implemented in Core because of an argument only presented there.

jnewbery · 2020-05-05T02:59:53Z

Github doesn't allow PRs to be transferred, so I've opened #18876 for the new branch.

fanquake · 2020-05-05T03:08:47Z

Closing this for #18876. Discussion / review should move to #18876 and the sub-PRs.

ariard · 2020-05-05T10:26:36Z

@luke-jr Yes I noticed that's an optional feature, used the term opt-in somehow IIRC.

@sipa that's the right but you may have an indirect incentive to relay blocks and likely-to-be-blocks to foster network robustness and make your transactions more censorship-resistant. Killing a light client doesn't increase or decrease network security ?

Anyway, thanks for your comments, throw a lengthy argumentation on the mailing list on this.

…g on index sync state Github-Pull: bitcoin#16442 Rebased-From: 9e182b0

…advertised Summary: PR description: > Recent questions have come up regarding dynamic service registration > (see bitcoin/bitcoin#16442 (comment) > and the assumeutxo project > While investigating how dynamic service registration might work, I was > confused about how we convey local services to peers. This adds some > documentation that hopefully clarifies this process. Backport of [[bitcoin/bitcoin#16847 | PR16847]] Test Plan: `ninja` (code comments only) Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8010

…are advertised 82e53f3 doc: add comments clarifying how local services are advertised (James O'Beirne) Pull request description: Recent questions have come up regarding dynamic service registration (see bitcoin#16442 (comment) and the assumeutxo project ~~which needs to dynamically flip NODE_NETWORK~~). While investigating how dynamic service registration might work, I was confused about how we convey local services to peers. This adds some documentation that hopefully clarifies this process. ACKs for top commit: laanwj: ACK 82e53f3 darosior: ACK 82e53f3 Tree-SHA512: a30c1020387d7a75b5b3cdde45f8b7f2ae46293da97e6227b2ee17e290b93deb5b16c0bbc2b1676972300e5c3c2ad74eb8b3910d6b93e028dac1ae2700468ef9

jimpo force-pushed the bip157-net branch from 6dfdd94 to cdbf8ac Compare July 23, 2019 17:04

fanquake added the P2P label Jul 23, 2019

jimpo force-pushed the bip157-net branch from 98331d7 to 3d398a8 Compare July 24, 2019 07:24

DrahtBot added the Needs rebase label Jul 24, 2019

TheBlueMatt reviewed Jul 29, 2019

View reviewed changes

jimpo force-pushed the bip157-net branch from 3d398a8 to 3499e88 Compare July 30, 2019 20:14

DrahtBot removed the Needs rebase label Jul 30, 2019

Christewart mentioned this pull request Jul 30, 2019

BIP 157 P2P messages bitcoin-s/bitcoin-s#533

Closed

torkelrogstad mentioned this pull request Aug 1, 2019

WIP: Initial BIP157 support bitcoin-s/bitcoin-s#668

Closed

5 tasks

jamesob reviewed Aug 7, 2019

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jamesob reviewed Aug 7, 2019

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jamesob reviewed Aug 8, 2019

View reviewed changes

marcinja reviewed Aug 27, 2019

View reviewed changes

Sjors mentioned this pull request Sep 1, 2019

Compact Client Side Filtering for Light Clients (Neutrino server-side support) #11863

Closed

practicalswift reviewed Sep 7, 2019

View reviewed changes

jamesob mentioned this pull request Sep 10, 2019

doc: add comments clarifying how local services are advertised #16847

Merged

laanwj added the Feature label Sep 30, 2019

DrahtBot added the Needs rebase label Oct 9, 2019

jnewbery mentioned this pull request May 5, 2020

[WIP] Serve BIP 157 compact filters #18876

Closed

fanquake closed this May 5, 2020

fanquake removed this from the 0.21.0 milestone May 5, 2020

luke-jr mentioned this pull request May 11, 2020

Serve cfcheckpt requests #18877

Merged

jnewbery mentioned this pull request May 12, 2020

indexes: Add compact block filter headers cache #18960

Merged

jnewbery mentioned this pull request May 26, 2020

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

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 9, 2020

net: Dynamically set local NODE_COMPACT_FILTERS service flag dependin…

b75ee9b

…g on index sync state Github-Pull: bitcoin#16442 Rebased-From: 9e182b0

jonatack mentioned this pull request Feb 22, 2022

doc: Update bips.md for 23.x #24420

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Serve BIP 157 compact filters #16442

Serve BIP 157 compact filters #16442

Uh oh!

Conversation

jimpo commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonasschnelli commented Jul 23, 2019

Uh oh!

jamesob commented Jul 24, 2019

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Review checklist

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sjors commented Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Aug 20, 2019

Uh oh!

marcinja left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimpo Oct 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Sep 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimpo commented Jul 23, 2019 •

edited

Loading

DrahtBot commented Jul 23, 2019 •

edited

Loading

Sjors commented Aug 13, 2019 •

edited

Loading

jimpo Oct 19, 2019 •

edited

Loading

Sjors commented Sep 1, 2019 •

edited

Loading

ariard commented May 4, 2020 •

edited

Loading