Disable bloom filtering by default.

TheBlueMatt commented at 3:11 pm on June 5, 2019: member

BIP 37 bloom filters have been well-known to be a significant DoS target for some time. However, in order to provide continuity for SPV clients relying on it, the NODE_BLOOM service flag was added, and left as a default, to ensure sufficient nodes exist with such a flag.

NODE_BLOOM is, at this point, well-established and, as long as there exist 0.18 nodes with default config (which I’d anticipate will be true for many years), will be available from some peers. By that time, the continued slowdown of BIP 37-based filtering will likely have rendered it useless (though this is already largely the case). Further, BIP 37 was deliberately never updated to support witness-based filtering as newer wallets are expected to migrate to some yet-to-be-network-exposed filters.

fanquake added the label Validation on Jun 5, 2019

kallewoof commented at 3:13 pm on June 5, 2019: member

Concept ACK

in src/validation.h:130 in e5d95662bf outdated

126@@ -127,7 +127,7 @@ static const unsigned int MAX_BLOCKS_TO_ANNOUNCE = 8;
127 /** Maximum number of unconnecting headers announcements before DoS score */
128 static const int MAX_UNCONNECTING_HEADERS = 10;
129 
130-static const bool DEFAULT_PEERBLOOMFILTERS = true;
131+static const bool DEFAULT_PEERBLOOMFILTERS = false;

MarcoFalke commented at 3:27 pm on June 5, 2019:

This has nothing to do with validation, please move it to net or something.

JosuGZ commented at 4:13 pm on June 5, 2019:

Wouldn’t that be something for a different pull request?

PastaPastaPasta commented at 2:37 am on June 6, 2019:

agree on that probably being better in different pr

TheBlueMatt commented at 9:43 am on June 6, 2019:

No, we already have more than enough volume of useless “move code around” PRs. This is precisely the kind of thing that should be in the same PR in a separate commit.

laanwj commented at 10:15 am on June 6, 2019:

let’s not shedpaint this but imo it’s fine to ombine this small code structure improvement with the change

MarcoFalke removed the label Validation on Jun 5, 2019

MarcoFalke added the label P2P on Jun 5, 2019

MarcoFalke commented at 6:30 pm on June 5, 2019: member

Also needs release notes and a post to the mailing list

MarcoFalke added the label Needs release note on Jun 5, 2019

TheBlueMatt force-pushed on Jun 6, 2019

laanwj commented at 10:18 am on June 6, 2019: member

concept and code-review ACK, only point of discussion is what release to set as milestone here

TheBlueMatt force-pushed on Jun 6, 2019

TheBlueMatt commented at 10:52 am on June 6, 2019: member

I’d presume next-major. No need to rush it into a backport, let alone to ensure there is always gonna be reasonable coverage of NODE_BLOOM nodes available.

fanquake added this to the milestone 0.19.0 on Jun 6, 2019

TheBlueMatt force-pushed on Jun 6, 2019

Disable bloom filtering by default.

BIP 37 bloom filters have been well-known to be a significant DoS
target for some time. However, in order to provide continuity for
SPV clients relying on it, the NODE_BLOOM service flag was added,
and left as a default, to ensure sufficient nodes exist with such a
flag.

NODE_BLOOM is, at this point, well-established and, as long as
there exist 0.18 nodes with default config (which I'd anticipate
will be true for many years), will be available from some peers. By
that time, the continued slowdown of BIP 37-based filtering will
likely have rendered it useless (though this is already largely the
case). Further, BIP 37 was deliberately never updated to support
witness-based filtering as newer wallets are expected to migrate to
some yet-to-be-network-exposed filters.

5efcb77283

Move DEFAULT_PEERBLOOMFILTERS from validation.h to net_processing.h f27309f55c

TheBlueMatt force-pushed on Jun 6, 2019

jtimon commented at 11:02 am on June 7, 2019: contributor

utACK f27309f55c4fa2b115525d72abb280757a568709

TheBlueMatt commented at 1:20 pm on June 8, 2019: member

Also needs release notes and a post to the mailing list

This is not a protocol change, only a default argument change. No need to do a mailing list post. I added a release notes entry.

fanquake removed the label Needs release note on Jun 8, 2019

luke-jr commented at 2:10 pm on June 8, 2019: member

utACK, without affirmation of future plans mentioned in PR description.

jnewbery commented at 8:18 am on June 9, 2019: member

ACK 048e0dcaea555e11bbdc8c56de0b9a523e42d351

I’ve read the code and it seems reasonable to me. It’s surprising that bloom filters aren’t tested by the functional tests.

This line could be removed: https://github.com/bitcoin/bitcoin/blob/8a503a6c6dd419921373f45d7aa6f1787c9b8884/test/functional/p2p_mempool.py#L20 since -peerbloomfilters now defaults to false, but no harm in leaving it.

NicolasDorier commented at 11:47 am on June 9, 2019: contributor

~NACK~, @TheBlueMatt bloom filter should always be enabled for white listed peers, as this is the most effective way to sync a lightweight client from your own full node without the need of additional middleware like electrum. (regardless of the value of the flag)

Even BIP157 come short for this usecase as it needlessly waste bandwidth. (and I think BIP157, as implemented now does not work in pruned mode -might be wrong here-)

TheBlueMatt commented at 11:50 am on June 9, 2019: member

@NicolasDorier That seems like an orthogonal issue. There is currently no code which enables it by default for whitelisted peers, so feel free to open a separate PR that does that.

NicolasDorier commented at 11:53 am on June 9, 2019: contributor

I don’t believe it is completely orthogonal.

Changing this default will break existing software which rely on the old default, at least we can limit damage to non-whitelisted peers only.

But yeh, anyway Concept ACK, not worth wasting ink on this, would just prefer if it was done here as well.

TheBlueMatt commented at 12:09 pm on June 9, 2019: member

0.19 is a ways off, no big deal either way :p

in test/functional/test_framework/messages.py:47 in 048e0dcaea outdated

43@@ -44,7 +44,7 @@
44 
45 NODE_NETWORK = (1 << 0)
46 # NODE_GETUTXO = (1 << 1)
47-NODE_BLOOM = (1 << 2)

NicolasDorier commented at 12:38 pm on June 9, 2019:

Do not disable this, I will make a test later for allowing NODE_BLOOM services to whitelisted peers.

jnewbery commented at 12:54 pm on June 9, 2019:

No need to invalidate reviews. Just uncomment it in your PR.

jnewbery commented at 12:55 pm on June 9, 2019:

(also linting tools like vulture may flag an error if this constant is defined but not used)

luke-jr commented at 4:48 am on June 10, 2019:

That sounds like a bug in the linting tool IMO.

TheBlueMatt commented at 3:47 pm on June 9, 2019: member

Exactly, if you don’t comment that line the linters decide that you’ve added a Clear Bug and must be shot.

On Jun 9, 2019, at 14:55, John Newbery notifications@github.com wrote:

@jnewbery commented on this pull request.

In test/functional/test_framework/messages.py:

@@ -44,7 +44,7 @@

NODE_NETWORK = (1 « 0)

NODE_GETUTXO = (1 « 1)

-NODE_BLOOM = (1 « 2) (also linting tools like vulture may flag an error if this constant is defined but not used)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Empact commented at 2:00 am on June 10, 2019: member

Concept ACK

in doc/release-notes/release-notes-16152.md:1 in 048e0dcaea outdated

0@@ -0,0 +1,3 @@
1+Miscellaneous CLI Changes

MarcoFalke commented at 11:42 am on June 11, 2019:

This is not a CLI change, but a change of default p2p behavior. (You remove the NODE_BLOOM service bit by default)

TheBlueMatt commented at 5:43 pm on July 12, 2019:

I dont know what the list of categories are, but I think a default parameter change is a CLI change. Happy to change to whatever.

MarcoFalke approved

MarcoFalke commented at 11:44 am on June 11, 2019: member

ACK, but could squash the commits, since the code changes are too trivial to justify three separate commits.

ajtowns commented at 9:22 am on June 12, 2019: member

ACK 048e0dcaea555e11bbdc8c56de0b9a523e42d351 ; light code review, checked it compiled.

NODE_BLOOM is, at this point, well-established and, as long as there exist 0.18 nodes with default config (which I’d anticipate will be true for many years), will be available from some peers.

I don’t think this necessarily holds up – if percentage p of nodes disable bloom filters (by upgrading to 0.19), while bloom users remain constant, then the load on the remaining nodes goes up by a factor of 1/(1-p); at some threshold that will probably cause node operators to manually disable bloom filters, leading to a vicious cycle where p rises much faster than expected, so that bloom filters degrade to being only offered by some small quasi-centralised set of servers, likely run by the providers of wallets that use bloom filters. If bloom users don’t/can’t authenticate the peers they’re getting bloom filters from, that’s probably a pretty big hole in the security assumptions.

Even if that occurs, I think making bloom filters opt-in (or default enabled but only for whitelisted peers) is still the right call.

Agree with @MarcoFalke that calling it a command line change doesn’t seem quite right.

jonasschnelli commented at 9:26 am on June 12, 2019: contributor

Concept ACK

practicalswift commented at 11:00 am on June 12, 2019: contributor

Concept ACK

laanwj commented at 1:10 pm on June 13, 2019: member

so that bloom filters degrade to being only offered by some small quasi-centralised set of servers, likely run by the providers of wallets that use bloom filters.

… after which they might as well switch to a protocol that is efficient for that use such as electrum (which doesn’t really have any worse privacy expectations), and everyone, including users of that wallet software will be better off :blush:

MarcoFalke added the label Waiting for author on Jul 11, 2019

DrahtBot commented at 9:38 pm on July 12, 2019: 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:

#15437 (p2p: Remove BIP61 reject messages by MarcoFalke)

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 commented at 3:11 pm on July 15, 2019: member

Release notes fixed here: https://github.com/jnewbery/bitcoin/tree/pr16152_release_notes @TheBlueMatt - I think if you reset to that branch, this should be mergeable, since the code changes have already been ACKed by @ajtowns , @jnewbery , @MarcoFalke, @luke-jr, @jtimon and @laanwj, and the release notes will be re-reviewed at release time.

MarcoFalke commented at 5:17 pm on July 15, 2019: member

As mentioned earlier (https://github.com/bitcoin/bitcoin/pull/16152#issuecomment-499202044), this probably requires a very brief post to the mailing list, that explains the change in p2p network behavior and mentions how long wallet developers have until their software breaks.

luke-jr commented at 6:59 pm on July 15, 2019: member

It won’t break. People will continue to use old versions for years into the future. Some might turn NODE_BLOOM back on, too.

MarcoFalke commented at 9:03 pm on July 15, 2019: member

Running versions of Bitcoin Core that are EOL is not supported. So if there was a DOS vector against them, it wouldn’t be fixed and the nodes could be shut down remotely. After those are shut down, the ones that have it turned on explicitly might also die off because they are flooded with filters at that point. So effectively after 0.18 is EOL, this p2p feature should be considered dead. Implying that it will be available forever is not helpful. I’d rather put a clear date on its expiry to give 3rd party developers a timescale to fix things. Otherwise they will rely on it never dying off and keep shipping software that might break at any point in time.

jnewbery commented at 10:07 pm on July 15, 2019: member

I’d rather put a clear date on its expiry to give 3rd party developers a timescale to fix things.

I agree. Much better to overcommunicate to the ecosystem through the mailing list than undercommunicate.

luke-jr commented at 10:10 pm on July 15, 2019: member

Sure, no harm notifying people. My point was simply that 0.19 isn’t suddenly a cut off. It will disappear gradually at most. And this only disables it by default - it’s still there if someone wants to enable it (eg, for their own usage). It would even make sense to enable it for trusted peers by default, until a better solution exists. So it’s not really dead at all yet.

ajtowns commented at 7:18 am on July 16, 2019: member

ACK 2518c0643ba99f7da80bfe24aaad0921184ead78 ; just changes the release notes which now look good to me. Unsurprisingly, code still looks good, still compiles and tests pass for me.

Add release notes for DEFAULT_BLOOM change bead32e31e

TheBlueMatt force-pushed on Jul 18, 2019

TheBlueMatt commented at 9:31 pm on July 18, 2019: member

Tweaked the release notes to use the “P2P” header (thanks @jnewbery for answering my question of what the proper header is, happy to credit you in the commit if you want!), and rewrote the text to indicate that we don’t anticipate it being a fast process (if anything, I’m happy to run a few of these, they aren’t that bad if you have a super beefy machine). Happy to send a mail to the ML with the release notes text if this gets merged.

jnewbery commented at 10:58 pm on July 18, 2019: member

ACK bead32e31e399090af30b2ee3539995d4105a66d

The only difference from previous branch is updated release notes, which look good to me.

Please also consider this an ongoing ACK for any release notes on top of f27309f55c4fa2b115525d72abb280757a568709 (which are the code changes)

happy to credit you in the commit if you want

No need

kallewoof approved

kallewoof commented at 0:58 am on July 19, 2019: member

ACK bead32e31e399090af30b2ee3539995d4105a66d

fanquake removed the label Waiting for author on Jul 19, 2019

NicolasDorier commented at 8:02 am on July 19, 2019: contributor

ACK

fanquake merged this on Jul 19, 2019

fanquake closed this on Jul 19, 2019

fanquake referenced this in commit 59ce537a49 on Jul 19, 2019

MarcoFalke commented at 3:25 pm on July 19, 2019: member

Happy to send a mail to the ML with the release notes text if this gets merged.

Yes, please

in doc/release-notes/release-notes-16152.md:7 in bead32e31e

0@@ -0,0 +1,7 @@
1+P2P Changes
2+-----------
3+- The default value for the -peerbloomfilters configuration option (and, thus, NODE_BLOOM support) has been changed to false.
4+  This resolves well-known DoS vectors in Bitcoin Core, especially for nodes with spinning disks. It is not anticipated that
5+  this will result in a significant lack of availability of NODE_BLOOM-enabled nodes in the coming years, however, clients
6+  which rely on the availability of NODE_BLOOM-supporting nodes on the P2P network should consider the process of migrating
7+  to a more modern (and less trustful and privacy-violating) alternative over the coming years.

luke-jr commented at 12:15 pm on July 21, 2019:

No such alternative exists…

ShadowOfHarbringer commented at 1:29 pm on July 22, 2019: none

ACK

SmartArray commented at 2:31 pm on July 22, 2019: none

I fully understand the purpose of having bloom filters disabled by default. Are we trying to serve an alternative? Like this one: https://github.com/bitcoin/bips/blob/master/bip-0157.mediawiki

runvnc commented at 7:21 pm on July 22, 2019: none

Bloom filters are the way that lightweight wallets operate without centralized servers.

There is no evidence of a significant DoS issue.

This is a clear effort to sabotage Bitcoin.

cculianu commented at 11:45 pm on July 22, 2019: none

What’s the matter? Liquid network is not the smash hit Blockstream hoped it would be and you need to do this to cripple SPV wallets? Or what?

jnewbery locked this on Jul 23, 2019

Disable bloom filtering by default. #16152

NODE_GETUTXO = (1 « 1)

Conflicts