[WIP] Serve BIP 157 compact filters

jnewbery commented at 2:54 am on May 5, 2020: member

This is almost the same change set as #16442 but:

fixup commits have been squashed into their appropriate commits
some commits have been broken up into smaller pieces
ordering of commits has changed
a few other minor changes

This PR is marked WIP as it’s a demonstration of the entire change. I’ll split off commits into smaller PRs for merge.

The original PR has already received a lot of review, so I think it should be possible to get this merged over the next few weeks. To hold myself accountable, I’m targeting the following timeline. Obviously, the PRs shouldn’t be merged until they’ve received thorough review.

	Functionality	Commits	PR	Target merge date	Actual merge date
✓	Serve cfcheckpt requests	[init] Add -peercfilters option, [net processing] Message handling for getcfcheckpt., [test] Add test for cfcheckpt	#18877	2020-05-13	2020-05-12
✓	Cache cfcheckpt headers	[net processing] Cache compact filter checkpoints in memory.	#18960	2020-05-20	2020-05-21
✓	Serve cfheaders requests	[net processing] Message handling for getcfheaders., [test] Add test for cfheaders	#19010	2020-05-27	2020-05-26
✓	Serve cfilters requests	[indexes] Fix default [de]serialization of BlockFilter., [net processing] Message handling for getcfilters., [test] Add test for cfilters.	#19044	2020-06-03	2020-05-31
✓	Signal NODE_COMPACT_FILTERS support	[net] Signal NODE_COMPACT_FILTERS if we’re serving compact filters., [test] Add test for NODE_COMPACT_FILTER.	#19070	2020-06-10	2020-08-13

jnewbery renamed this:
~~Serve BIP 157 compact filters~~
[WIP] Serve BIP 157 compact filters
on May 5, 2020

fanquake added the label Feature on May 5, 2020

fanquake added the label P2P on May 5, 2020

luke-jr commented at 3:01 am on May 5, 2020: member

I think it should be possible to get this merged over the next few weeks.

This has Concept NACKs and should not be merged.

jnewbery commented at 3:05 am on May 5, 2020: member

@Empact @jamesob @talkless @sipa @fjahr @pinheadmz @jonasnick @jkczyz @practicalswift @marcinja @MarcoFalke @kilrau @TheBlueMatt - thank you for your reviews on the original PR. I’m going to try to respond to feedback quickly to make progress on this. Please help by reviewing the linked PRs!

fanquake added this to the milestone 0.21.0 on May 5, 2020

DrahtBot commented at 9:26 am on May 5, 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:

#19020 (net: Use C++11 member initialization in protocol by MarcoFalke)
#19010 (net processing: Add support for getcfheaders by jnewbery)
#18996 (net: Remove un-actionable TODO by MarcoFalke)
#18960 (indexes: Add compact block filter headers cache by jnewbery)
#18354 (Protect wallet by using shared pointers by bvbfan)
#18242 (Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli)
#18165 (Consolidate service flag bit-to-name conversion to a shared serviceFlagToStr function by luke-jr)
#18000 (Index for UTXO Set Statistics by fjahr)

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 force-pushed on May 5, 2020

jnewbery force-pushed on May 6, 2020

Empact commented at 9:11 pm on May 6, 2020: member

Concept ACK

Current Travis linter error:

0test/functional/p2p_cfilters.py:11:1: F401 'test_framework.messages.NODE_COMPACT_FILTERS' imported but unused
1^---- failure generated from test/lint/lint-python.sh

ariard commented at 9:08 am on May 7, 2020: member

Withdrawing my Concept NACK on the previous PR, ecosystem-wise, BIP157 is better that current light client solutions. Even if there is still long-term serious concerns, it’s a broader point that this specific protocol. I can also envision people always servicing filters-headers only to provide filters correctness cheap services.

Few points, maybe for follow-up works:

signal NODE_COMPACT_FILTERS_HEADERS only, both filter-headers and filters are quite different in size, first one is the same order of magnitude that header and even a more resource-preserving peer still want to offer them
it could be the opportunity to experiment on filters services some bandwidth DoS-protection, you may need to tie inbound peer addresses with some accounting logic to do so
dissociating messages path from net_processing in the future, ProcessMessages may fan-out between classes of messages like ProcessAddr, ProcessBlock, ProcessTransaction, ProcessFilters. It would make easier to reason on their buffer and timers.

michaelfolkson commented at 6:32 pm on May 7, 2020: contributor

Concept ACK.

I need a stronger, more convincing argument that improved light client protocols will drive the number of independent parties running and using a listening full node down.

If I can be convinced of that I’d consider reversing the Concept ACK. It is clear that that metric is critically important to network health, I don’t think there is any disagreement there. But I think a future where every mobile is a light client is just as likely to drive that number up.

I will continue to follow the discussion on the mailing list.

ariard commented at 7:12 am on May 8, 2020: member

@michaelfolkson for discussion clarity, and maybe it isn’t clear in my mail sorry, I don’t think in any case it will drive the number of independent parties running and using a listening full node down. If bandwidth cost is too high, node operators are just going to stop turn peerscfilters=false. The concern is for the light clients ecosystem in itself.

michaelfolkson commented at 9:08 am on May 8, 2020: contributor

Sure, that was clear from your mail @ariard. I’m referring to some of the responses to your mailing list post which make arguments for this specific PR to be NACKed. All this project (Bitcoin Core) can do is facilitate superior light client protocols (assuming they are not harmful to the full node ecosystem which I haven’t been convinced they are). If the light client ecosystem doesn’t take off or if node operators all choose to turn peerscfilters=false there isn’t much Core can do or should do. That is down to the node operators. But I’m trying to keep the arguments specific to this particular PR here. Any brainstorming on how we can make the light client ecosystem successful should definitely not be done here and should be done on the mailing list.

Personally, I think Concept ACK, Concept NACKs are fine to post here with a short explanation. But if we can keep as much of the discussion on the mailing list as possible I think that is preferable.

luke-jr commented at 4:18 pm on May 8, 2020: member

I need a stronger, more convincing argument that improved light client protocols will drive the number of independent parties running and using a listening full node down.

Currently, there is a strong incentive to use your own node for privacy. Widely-deployed on nodes, BIP157 destroys this incentive.

michaelfolkson commented at 4:52 pm on May 8, 2020: contributor

Running a full node will always be the superior option for privacy (and trust minimization) versus using a light client (improved or otherwise). If your primary objective is privacy the incentive is still there to run a full node. BIP157 reduces the gap between full node privacy and light client privacy that is true (light client privacy is improving). But this only affects a cohort of people who are wavering on whether to run a full node or not and see the privacy improvements of light clients as a reason not to. I have no idea how to quantify this cohort of people but I would be shocked if it was significant. Hence “destroys” seems to me to be a superlative.

On the upside, if the light client ecosystem was to grow and prosper post BIP157 it would need an increased number of full nodes to serve them filters. If the number of full nodes didn’t increase to serve this filter demand then the light client ecosystem will be stunted forcing the cohort discussed above to lean back towards running a full node.

If the metric is “number of independent parties running and using a listening full node” I think growth of one ecosystem (light clients) has a positive impact on the growth of another (full nodes). If the metric is the ratio of light clients to “number of independent parties running and using a listening full node” then I think you have a point. I was of the understanding that we are more interested in the former than the latter.

MarcoFalke commented at 12:50 pm on May 12, 2020: member

re-𝔸ℂ𝕂 0ab77d46a0 🐯

Only change since my previous review #16442 (comment) is:

Small simplification refactor in init.cpp
Removing unused includes and symbols
Add log categories to log statements
Change return type to void where possible
Rework documentation and test a bit

Though, I was wondering why you dropped this hunk in the test:

0# Check that nodes have signalled NODE_COMPACT_FILTERS correctly.
1assert node0.nServices & NODE_COMPACT_FILTERS != 0
2assert node1.nServices & NODE_COMPACT_FILTERS == 0
3
4# Check that the localservices is as expected.
5assert int(self.nodes[0].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS != 0
6assert int(self.nodes[1].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS == 0

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-𝔸ℂ𝕂 0ab77d46a0 🐯
 4
 5Only change since my previous review [#16442 (comment)](/bitcoin-bitcoin/16442/#issuecomment-623415561) is:
 6
 7* Small simplification refactor in init.cpp
 8* Removing unused includes and symbols
 9* Add log categories to log statements
10* Change return type to void where possible
11* Rework documentation and test a bit 
12
13Though, I was wondering why you dropped this hunk in the test:
14
15# Check that nodes have signalled NODE_COMPACT_FILTERS correctly.
16assert node0.nServices & NODE_COMPACT_FILTERS != 0
17assert node1.nServices & NODE_COMPACT_FILTERS == 0
18
19# Check that the localservices is as expected.
20assert int(self.nodes[0].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS != 0
21assert int(self.nodes[1].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS == 0
22
23-----BEGIN PGP SIGNATURE-----
24
25iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
26pUj5BgwAzu7Z+RNM0h2GPe3S/9ft5ZmqVl/U7OKuuwXNzoeMb9gk7RiM2HUPgZY0
27q+8TxkMRtENG5MvIBJyGa6WvVdFGTTJMK93rLGAYdnq73lRj/D132cM6sxmbfOqt
28AkBYl655mk73mPMaSbuSZfk1c4C+mXlWnk3W4dDgfMATclsttSf9fgzJ2nAaPUST
29luw/ReUAzUiur4kXyDLX9CN8/VdnpAW9kan3RE5K1nyZ2uAPHkKqPfGff3TRp6l4
30+mmHT6+d/H2MlQ8kH+0pjFKw+CNDpbi2rd9bJiNSJ9hDgnRzzC6B2q71VvoxCY43
31+dYv541QqiTptd5PMByNN17d6kHiq9kCRlcs0t0YLGMkXDfB9aWxZp5jKRnhMa4Q
3276LN1I5DUK7R8/qvdBgEOc7yqr2dfELba8wUlGpAeZNrsFpW3/Y50JeHZihpBJdY
33KBgaKR1MU6uUjId7EAOpBz1YmXyRf0+2iNtH/syIW3opctzpNHaQHO6JiHOEF3qr
34XeyEdd5x
35=rpCM
36-----END PGP SIGNATURE-----

Timestamp of file with hash cebd0143a8bfb0f77090cd044ec018bea0df73270bf57843fb892d5d6d9ac941 -

DrahtBot added the label Needs rebase on May 12, 2020

jnewbery commented at 1:43 pm on May 12, 2020: member

Though, I was wondering why you dropped this hunk in the test:

That was removed for the first PR (since NODE_COMPACT_FILTERS is not defined until the last PR in the sequence). Thank you for reminding me to add it back to the final PR.

jnewbery commented at 8:01 pm on May 12, 2020: member

The next PR in the series is #18960: Add compact block filter headers cache

[indexes] Add compact block filter headers cache 3c5f99d5bd

[net processing] Message handling for getcfheaders.

if -peercfilter is configured, handle requests for cfheaders.

a4bbc4fd28

[test] Add test for cfheaders 0ec5258acc

[indexes] Fix default [de]serialization of BlockFilter.

This only changes network serialization. Disk serialization is defined
in ReadFilterFromDisk() and WriteFilterToDisk() and does not include the
filter_type.

5335fa8ad7

[net processing] Message handling for getcfilters.

if -peercfilter is configured, handle requests for cfilters.

802c850bc6

[test] Add test for cfilters. 1e8a696036

[net] Signal NODE_COMPACT_FILTERS if we're serving compact filters.

if -peercfilters is configured, signal the NODE_COMPACT_FILTERS service
bit to indicate that we are able to serve compact block filters, headers
and checkpoints.

94167bf701

[test] Add test for NODE_COMPACT_FILTER.

Test that a node configured to serve compact filters will signal
NODE_COMPACT_FILTER service bit.

e8f40bbb0d

[net processing] Synchronize cfilter request handling with block filter index.

Since the block filter index is updated asynchronously by proxy of the
ValidationInterface queue, lazily synchronize the net thread with the
block filter index in case lookups fail.

5488ce98cf

jnewbery force-pushed on May 13, 2020

jnewbery commented at 1:54 am on May 13, 2020: member

Though, I was wondering why you dropped this hunk in the test:

That was removed for the first PR (since NODE_COMPACT_FILTERS is not defined until the last PR in the sequence). Thank you for reminding me to add it back to the final PR.

This has now been added back.

DrahtBot removed the label Needs rebase on May 13, 2020

jonasschnelli commented at 1:06 pm on May 13, 2020: contributor

Concept ACK

chris-belcher commented at 5:57 pm on May 13, 2020: contributor

As of April 2019 the size of all the filters is 4GB. I’m imagining a smartphone having to download 4GB from a raspberry pi node just to start up its bitcoin wallet. So would SPV wallets actually use this technology? You can hardly call it “lightweight”. Not to mention the wallet also has to download the blocks including false positives.

At 4GB it’s not far off from needing less bandwidth to download the UTXO set.

Yes, Wasabi Wallet already uses filters like these, but the only reason they made it work is their wallet only uses bech32 addresses, which are very rare on the blockchain. And if everyone used wallets that used those addresses then the filters would again become unusable large.

Obviously the filters are very useful for making wallets rescans faster (a fast machine does it in 10 hours right now), but filters being used by lightweight wallets seems like a pipe dream to me. Happy to be convinced otherwise.

jonasschnelli commented at 6:11 pm on May 13, 2020: contributor

@chris-belcher: it is a reasonable point. But does a light client really need to download the whole filterset (the 4GB)? If the app is freshly installed and a wallet has been created, is there a need to download anything from the past? Or would downloading the few MBs per week of filter data (and maybe some blocks including the false positives) be enough?

Things would obviously look different if one imports a key/seed/address/wallet with unknown birthday.

chris-belcher commented at 6:41 pm on May 13, 2020: contributor

If that’s the plan then such a wallet won’t be able to reasonably restore old seed phrases after a few years.

Sync’ing a brand new wallet has never been much of a problem I think, as even without filters the wallet could download all the new blocks (without witnesses) which would cost about 144 MB/day or just 1.7 kb/sec.

prusnak commented at 6:49 pm on May 13, 2020: contributor

Why would you download 144 MB/day if you could download 1MB/day? Think embedded/IoT devices, etc. Also, not everyone is privileged to have bandwidth where downloading 144 MB/day is nothing. It was not very far history when I had a FUP of 100MB/month on my cellphone plan.

ariard commented at 7:12 pm on May 13, 2020: member

@chris-belcher if you assume this kind of chain backend are going to be primarily used for LN wallets, you may just bookmark confirmation of every of your funding outpoint. Even this kind of wallet may not expose at all onchain addresses.

You already have to backup channel state, so their storage must already be stateful, you may just increase the scope of it to avoid rescan at all.

MarcoFalke commented at 7:22 pm on May 13, 2020: member

Just as a general note, I think we should use pull requests for code review of the code changes, not for general protocol and design discussions. Otherwise, it will be hard to follow the review process in the noise. Also, GitHub will start hiding comments if there are too many, making it painful to even read the code review and previous review comments. This can lead to review comments being overlooked and be left unaddressed.

DrahtBot commented at 1:16 pm on May 20, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on May 20, 2020

jnewbery commented at 10:56 pm on May 21, 2020: member

The next PR in this sequence is #19010: net processing: Add support for getcfheaders

jnewbery commented at 4:47 pm on May 26, 2020: member

#19010 is merged. The next PR in this sequence is #19044: net processing: Add support for getcfilters

jnewbery commented at 2:27 am on June 1, 2020: member

#19044 is merged. Next PR in the sequence is #19070: Signal NODE_COMPACT_FILTERS support.

luke-jr referenced this in commit c00e3045b9 on Jun 9, 2020

luke-jr referenced this in commit 9fa575a14d on Jun 9, 2020

luke-jr referenced this in commit ecfc1e38aa on Jun 9, 2020

luke-jr referenced this in commit eb91f2864a on Jun 9, 2020

luke-jr referenced this in commit f250151666 on Jun 9, 2020

luke-jr referenced this in commit a34126e7d1 on Jun 9, 2020

luke-jr referenced this in commit c76f4cd96f on Jun 9, 2020

luke-jr referenced this in commit 7cfee10028 on Jun 9, 2020

luke-jr referenced this in commit ea5a8aeef7 on Jun 9, 2020

kilrau commented at 12:52 pm on August 1, 2020: none

Forgive me if this was discussed before, but do the served cfilters count against maxuploadtarget? #16442 (comment) sounds like it.

jnewbery commented at 2:27 pm on August 13, 2020: member

jnewbery closed this on Aug 13, 2020

jnewbery commented at 2:32 pm on August 13, 2020: member

do the served cfilters count against maxuploadtarget? #16442 (comment) sounds like it.

Yes, the number of bytes will be counted towards the upload target (but peers will still be able to download compact filters after the target is reached, just as they can still download transactions).

DrahtBot locked this on Feb 15, 2022

[WIP] Serve BIP 157 compact filters #18876

Conflicts