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

Disable bloom filtering by default. #16152

Merged
merged 3 commits into from
Jul 19, 2019

Conversation

TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Jun 5, 2019

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.

@kallewoof
Copy link
Contributor

Concept ACK

src/validation.h Outdated Show resolved Hide resolved
@maflcko maflcko added P2P and removed Validation labels Jun 5, 2019
@maflcko
Copy link
Member

maflcko commented Jun 5, 2019

Also needs release notes and a post to the mailing list

@laanwj
Copy link
Member

laanwj commented Jun 6, 2019

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

@TheBlueMatt
Copy link
Contributor Author

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 fanquake added this to the 0.19.0 milestone Jun 6, 2019
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.
@jtimon
Copy link
Contributor

jtimon commented Jun 7, 2019

utACK f27309f

@TheBlueMatt
Copy link
Contributor Author

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.

@luke-jr
Copy link
Member

luke-jr commented Jun 8, 2019

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

@jnewbery
Copy link
Contributor

jnewbery commented Jun 9, 2019

ACK 048e0dc

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:

self.extra_args = [["-peerbloomfilters=0"]]
since -peerbloomfilters now defaults to false, but no harm in leaving it.

@NicolasDorier
Copy link
Contributor

NicolasDorier commented Jun 9, 2019

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
Copy link
Contributor Author

@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
Copy link
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
Copy link
Contributor Author

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

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

NODE_NETWORK = (1 << 0)
# NODE_GETUTXO = (1 << 1)
NODE_BLOOM = (1 << 2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a bug in the linting tool IMO.

@TheBlueMatt
Copy link
Contributor Author

TheBlueMatt commented Jun 9, 2019 via email

@Empact
Copy link
Contributor

Empact commented Jun 10, 2019

Concept ACK

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -0,0 +1,3 @@
Miscellaneous CLI Changes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@practicalswift
Copy link
Contributor

Concept ACK

@laanwj
Copy link
Member

laanwj commented Jun 13, 2019

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 😊

@DrahtBot
Copy link
Contributor

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
Copy link
Contributor

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.

@maflcko
Copy link
Member

maflcko commented Jul 15, 2019

As mentioned earlier (#16152 (comment)), 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
Copy link
Member

luke-jr commented Jul 15, 2019

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

@maflcko
Copy link
Member

maflcko commented Jul 15, 2019

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
Copy link
Contributor

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
Copy link
Member

luke-jr commented Jul 15, 2019

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
Copy link
Contributor

ajtowns commented Jul 16, 2019

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

@TheBlueMatt
Copy link
Contributor Author

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
Copy link
Contributor

ACK bead32e

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 f27309f (which are the code changes)

happy to credit you in the commit if you want

No need

Copy link
Contributor

@kallewoof kallewoof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK bead32e

@NicolasDorier
Copy link
Contributor

ACK

@fanquake fanquake merged commit bead32e into bitcoin:master Jul 19, 2019
fanquake added a commit that referenced this pull request Jul 19, 2019
bead32e Add release notes for DEFAULT_BLOOM change (Matt Corallo)
f27309f Move DEFAULT_PEERBLOOMFILTERS from validation.h to net_processing.h (Matt Corallo)
5efcb77 Disable bloom filtering by default. (Matt Corallo)

Pull request description:

  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.

ACKs for top commit:
  jnewbery:
    ACK bead32e
  kallewoof:
    ACK bead32e

Tree-SHA512: ecd901898e8efe1a7c82b471af0acc2373c2282ac633eb58d9aae7c35deda1999d0f79fb0485e6cecbda7246aeda00206cd82c7fa36866e2ac64705ba93f9390
@maflcko
Copy link
Member

maflcko commented Jul 19, 2019

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

Yes, please

This resolves well-known DoS vectors in Bitcoin Core, especially for nodes with spinning disks. It is not anticipated that
this will result in a significant lack of availability of NODE_BLOOM-enabled nodes in the coming years, however, clients
which rely on the availability of NODE_BLOOM-supporting nodes on the P2P network should consider the process of migrating
to a more modern (and less trustful and privacy-violating) alternative over the coming years.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No such alternative exists...

@ShadowOfHarbringer
Copy link

ACK

@SmartArray
Copy link

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
Copy link

runvnc commented Jul 22, 2019

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
Copy link

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?

@bitcoin bitcoin locked as spam and limited conversation to collaborators Jul 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.