[WIP] Serve BIP 157 compact filters

[jnewbery]

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

[luke-jr]

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]

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

[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:

• net: Use C++11 member initialization in protocol #19020 (net: Use C++11 member initialization in protocol by MarcoFalke)
• net processing: Add support for getcfheaders #19010 (net processing: Add support for getcfheaders by jnewbery)
• net: Remove un-actionable TODO #18996 (net: Remove un-actionable TODO by MarcoFalke)
• indexes: Add compact block filter headers cache #18960 (indexes: Add compact block filter headers cache by jnewbery)
• Use shared pointers only in validation interface #18354 (Protect wallet by using shared pointers by bvbfan)
• 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)

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.

[Empact]

Concept ACK

Current Travis linter error:

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

[ariard]

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]

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]

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

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]

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]

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.

[maflcko]

re-𝔸ℂ𝕂 0ab77d4 🐯

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:

# Check that nodes have signalled NODE_COMPACT_FILTERS correctly.
assert node0.nServices & NODE_COMPACT_FILTERS != 0
assert node1.nServices & NODE_COMPACT_FILTERS == 0

# Check that the localservices is as expected.
assert int(self.nodes[0].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS != 0
assert int(self.nodes[1].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS == 0

Show signature and timestamp

Signature:

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

re-𝔸ℂ𝕂 0ab77d46a0 🐯

Only change since my previous review https://github.com/bitcoin/bitcoin/pull/16442#issuecomment-623415561 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:

# Check that nodes have signalled NODE_COMPACT_FILTERS correctly.
assert node0.nServices & NODE_COMPACT_FILTERS != 0
assert node1.nServices & NODE_COMPACT_FILTERS == 0

# Check that the localservices is as expected.
assert int(self.nodes[0].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS != 0
assert int(self.nodes[1].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS == 0

-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUj5BgwAzu7Z+RNM0h2GPe3S/9ft5ZmqVl/U7OKuuwXNzoeMb9gk7RiM2HUPgZY0
q+8TxkMRtENG5MvIBJyGa6WvVdFGTTJMK93rLGAYdnq73lRj/D132cM6sxmbfOqt
AkBYl655mk73mPMaSbuSZfk1c4C+mXlWnk3W4dDgfMATclsttSf9fgzJ2nAaPUST
luw/ReUAzUiur4kXyDLX9CN8/VdnpAW9kan3RE5K1nyZ2uAPHkKqPfGff3TRp6l4
+mmHT6+d/H2MlQ8kH+0pjFKw+CNDpbi2rd9bJiNSJ9hDgnRzzC6B2q71VvoxCY43
+dYv541QqiTptd5PMByNN17d6kHiq9kCRlcs0t0YLGMkXDfB9aWxZp5jKRnhMa4Q
76LN1I5DUK7R8/qvdBgEOc7yqr2dfELba8wUlGpAeZNrsFpW3/Y50JeHZihpBJdY
KBgaKR1MU6uUjId7EAOpBz1YmXyRf0+2iNtH/syIW3opctzpNHaQHO6JiHOEF3qr
XeyEdd5x
=rpCM
-----END PGP SIGNATURE-----

Timestamp of file with hash cebd0143a8bfb0f77090cd044ec018bea0df73270bf57843fb892d5d6d9ac941 -

[jnewbery]

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.

[prusnak]

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]

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

[maflcko]

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]

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

[jnewbery]

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

[jnewbery]

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

[jnewbery]

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

[kilrau]

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

[jnewbery]

[jnewbery]

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).