Add NODE_BLOOM service bit and bump protocol version #6579

TheBlueMatt · 2015-08-21T04:39:42Z

Rehashing #2900, BIP to be posted soon.

dcousens · 2015-08-21T05:20:56Z

concept ACK

petertodd · 2015-08-21T05:50:11Z

src/main.cpp

+            Misbehaving(pfrom->GetId(), 100);
+        //TODO: Enable this after reasonable network upgrade
+        //else
+        //    pfrom->fDisconnect = true;


Oh actually, I was thinking for the first release, not only allow pfrom->nVersion < NO_BLOOM_VERSION clients to send filterX messages, but also respond to them, to give a bit of time for those clients to be upgraded. (this could even be gated to some fixed deadline in the future)

My original logic re: disconnecting clients immediately was to give those clients an opportunity to find another node quickly.

I believe that there is sufficient time for clients that need the NODE_BLOOM commands to upgrade before a client with this patch is even significantly deployed to the network, much less bloom filters widely disabled.

True, and of course it's easy to patch the DNS seeds that the affected wallets use to only return nodes that have NODE_BLOOM set - BitcoinJ doesn't do any peer addr caching, relying totally on the DNS seeds every time.

Oops, yes, I meant to do what @petertodd said first, happens when you write code then change it without thinking :).

What is the point in ignoring filter* messages, instead of disconnecting the asking nodes? I don't see how that's any friendlier.
I like @petertodd's solution as well to respond to old clients< NO_BLOOM_VERSION for now then in a later release, add the disconnect.
But I don't see the point in this ignore behavior. It's worst of both worlds.

But yeah, a release that changes nothing for old version clients is 100% ok by me, and it should apply the filter to avoid bandwidth exhaustion. (which itself indicates stupid code in existing SPV wallets...)

ACK on "if not willing to serve a request, disconnect".

ACK on "if not willing to serve a request, disconnect" too.

@laanwj I think you misread, that behavior was a bug and was unintentional. The current code does not ignore filter requests, ever, I agree that would be a bad idea. As the BIP has always suggested, it currently serves filtered connections to clients which propose an old version number, and disconnects clients with a new version number (with DoS prejudice). If you follow the two TODO comments (remove one line, add two lines), it will disconnect all nodes, only giving DoS prejudice to nodes which propose a new version number.

@TheBlueMatt Ah you're right, did't notice the && from->nVersion >= NO_BLOOM_VERSION in the outer if.

jonasschnelli · 2015-08-21T06:11:28Z

utACK.

pstratem · 2015-08-21T06:14:56Z

utACK (testing now)

laanwj · 2015-08-21T08:00:34Z

Concept ACK.
Process-wise, this indeed does need a BIP.

evGUzQIEQaL4 · 2015-08-21T10:25:52Z

BIP on the mailing list (still unnumbered).

https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2015-August/010535.html

evoskuil · 2015-08-21T11:18:40Z

Libbitcoin doesn't support BIP37 (and doesn't intend to). This allows us to fix the resulting protocol break.

davecgh · 2015-08-21T17:59:49Z

I'm happy to see this being added. We've wanted this in btcd for quite some time as currently the only ways to not support bloom filters are either a protocol break or simply not applying the filters while claiming to do so.

petertodd · 2015-08-24T02:09:53Z

Tested ACK (tested w/ Android Bitcoin Wallet, rescans with and without NODE_BLOOM work as expected)

jgarzik · 2015-08-24T03:47:36Z

ACK conditional on not breaking notable groups of users in the field. @petertodd testing w/ Android Bitcoin Wallet is much appreciated and is excellent test coverage.

The "some wallets are moving to centralized servers" argument is weak. The desire is robust service for users - especially those using decentralized wallets; would not want to hurt active, decentralized solutions because some other wallets are abandoning DC. </general principle>

jgarzik · 2015-08-24T04:04:23Z

Hum. Some further thoughts:

The general goal is to phase out this feature, yes?

Currently we have bloom support + no node_bloom bit.

Seems silly to introduce a NODE_BLOOM bit only to eliminate bloom support - the natural end state of a default-bloom-disabled eventual ship mode after this + further changes, subsequently widely deployed.

Further, the end user client PoV is that P2P nodes that do support a protocol version with fRelayTx randomly do not respond to filter* NODE_BLOOM does not help the end user client. Old software will not know about this new bit, and will randomly fail.

Therefore, why add planned-obsolete bit?

Anyone wanting to keep bloom support could advertise the service via #4657 (NODE_EXT_SERVICE) or a similar optional-service advertisement & rendezvous mechanism. The breakage (explained above) is the same for end users as default-disabled NODE_BLOOM.

evGUzQIEQaL4 · 2015-08-24T04:10:59Z

The general goal is to phase out this feature, yes?

I didn't bump the BIP with that in mind. The intent is just to be able to signal to wallet users that there's no point wasting your time and bandwidth sending filterload messages that are never going to do anything. Many SPV wallets seem to blindly follow whatever DNS seeds return, so if you punt them they just come straight back with the same request. libbitcoin, bitcoin-ruby, pseudonode and btcd already break the expected P2P protocol by simply ignoring these messages.

If and when the network evolves to have partial history storage nodes, or some majority of pruned nodes this messaging will need to happen one way or another as nodes will simply not be able to respond with merkleblock messages for blocks they do not have on disk.

I don't really have an opinion on how this is executed, I would just like both to be able to

signal intent to ignore those messages so I don't denial of service attack peoples wallets
easily disable BIP37 on high reliability nodes without having to manually patch

laanwj · 2015-08-24T10:31:18Z

IMO, the goal of this is not to phase out the feature. Sure, it could be phased out at some point but some SPV wallets are using it so there is no hurry.

The goal is to make it optional for node implementations to implement it. As bloom filters are unnecessary for inter-full-node communications, IMO coupling NODE_NETWORK and support for bloom filters was a mistake and this corrects that.

mikehearn · 2015-08-24T14:20:45Z

NACK - we went around all this in 2013 and the same arguments were made then. Nothing has changed since that time.

There is no need to make this optional, it doesn't help anyone and it can easily hurt. For instance, the next thing that will happen is that Peter Todd runs around telling everyone it's dangerous and they should disable it (he is wrong, but he'll do so anyway).

Yes, obviously it adds work to fully implement the protocol. So does implementing the other bits. If you want to write a full serving node, then you should implement all of the Bitcoin protocol, not just the bits of it you happen to like. If you don't want to do that, or are worried about unpredictable resource usage, then don't open a listening port: very simple. You are then a client and can implement whatever minimum subset you like.

WRT phasing out - it's very obviously the intent to do exactly that, as Matt said on the BIP thread:

The proposal will not break any existing clients in the first release. After sufficient time to upgrade SPV clients, a new version will be released which will result in older SPV clients finding themselves disconnected from peers when they send filter* commands, so they can go find other peers which do support bloom filtering

SPV wallets will not stop using this feature. There is no "phasing out". There is only Bitcoin Core deciding it doesn't want to support P2P smartphone wallets anymore. And why would you do that?

schildbach · 2015-08-24T14:32:33Z

I agree with @mikehearn on this one. Bloom filters are important for smartphone wallets and nothing is won by not supporting them.

dcousens · 2015-08-24T14:44:59Z

I'm personally in support with the intent pointed out by @evGUzQIEQaL4, such that the PR should:

Signal intent to ignore those messages so I don't denial of service attack peoples wallets
[Allow ability to] easily disable BIP37 on high reliability nodes without having to manually patch

I also think that the current default behaviour of supporting BIP37 should not be disabled until a suitable replacement is found.
~~NACK for NO_BLOOM_VERSION~~, concept ACK for listed intents.

edit: As pointed out by @TheBlueMatt, NO_BLOOM_VERSION is just a protocol version to specify this change in the protocol, it doesn't disable BIP37 in any way, nor enforce a disabling default.

jgarzik · 2015-08-24T14:51:05Z

It sounds like this does negatively impact major user segments.
The problem of bit inversion & backwards compatibility remains. In-field client experience will be of nodes randomly failing to produce a proper filter* response as expected, because they are unaware of NODE_BLOOM.

gavinandresen · 2015-08-24T15:08:50Z

NACK from me, I'll repeat what I said last year: "no consensus this is a good idea."

laanwj · 2015-08-24T15:09:03Z

Exactly @dcousens.
FYI this doesn't change the default of supporting bloom filters in Bitcoin Core. It does make it possible to signal not supporting it for implementations that - generally already - do not support it (such as btcd, obelisk)

if (GetBoolArg("-peerbloomfilters", true))
    nLocalServices |= NODE_BLOOM;

Most SPV wallets are mobile applications, thus easily updated to pay attention to this bit.

jgarzik · 2015-08-24T15:10:44Z

"I assume applications may be easily updated" is a poor approach to backwards compat.

laanwj · 2015-08-24T15:12:02Z

Well the alternative (as currently used) is to ignore bloom commands without signaling it. Or disconnecting clients that request filtering functionality.
Is that better for backwards compatiblity?

mikehearn · 2015-08-24T15:14:58Z

The alternative is to not open a listening port. You won't appear in the DNS seeds, you won't get clients assuming you provide functionality you don't - it's a win.

AFAICT the only people who care about this are those who want to serve data to the network with an incomplete implementation of the protocol. Nobody is forcing them to serve data, and nobody is forcing them to use or write incomplete implementations.

Meanwhile, all the users benefit a lot from the abundant serving capacity we have on the production network. The last thing we need is a serving capacity crisis in future.

laanwj · 2015-08-24T15:20:37Z

Users should have the choice to not serve SPV clients. It's their node, and their choice. It's not like a controversial hardfork that everyone has to agree on...

jgarzik · 2015-08-24T15:31:01Z

@mikehearn

Agree on user benefit
Let's not assume about others motivations. It remains true that there is an attack vector.

mikehearn · 2015-08-24T15:32:26Z

I just posted some hard data on the DoS attack risk to the mailing list.

Summary is: I implemented a program that does a filter-a-lot attack. It turns out to not really affect anything, and 90% of the CPU usage is anyway in just loading the block from disk, not doing the filtering.

petertodd · 2015-08-25T17:20:53Z

@sipa That was my intent with NODE_BLOOM as well in the original BIP I wrote.

TheBlueMatt · 2015-08-25T17:23:51Z

Indeed, @sipa, that is what the current BIP text states.

On August 25, 2015 10:21:05 AM PDT, Peter Todd [email protected] wrote:

@sipa That was my intent with NODE_BLOOM as well in the original BIP I
wrote.

Reply to this email directly or view it on GitHub:
#6579 (comment)

laanwj · 2015-09-04T16:35:45Z

src/rpcmisc.cpp

@@ -49,12 +49,13 @@ UniValue getinfo(const UniValue& params, bool fHelp)
            "{\n"


Please see the comment at the top of getinfo(). This call is going to be deprecated in the future and should not be extended anymore: https://github.com/bitcoin/bitcoin/blob/master/src/rpcmisc.cpp#L30

The right place to add "services" woud be getnetworkinfo. But it already has 'localservices'.

Lol, how did I miss that? I added localservices in 99ddc6c

@laanwj Thanks for catching that!

laanwj · 2015-09-04T16:36:21Z

ACK, apart from nit about getinfo.

Lets nodes advertise that they offer bloom filter support explicitly. The protocol version bump allows SPV nodes to assume that NODE_BLOOM is set if NODE_NETWORK is set for pre-70011 nodes. Also adds an option to turn bloom filter support off for nodes which advertise a version number >= 70011. Nodes attempting to use bloom filters on such protocol versions are banned, and a later upgade should drop nodes of an older version which attempt to use bloom filters. Much code stolen from Peter Todd. Implements BIP 111

TheBlueMatt · 2015-09-06T06:29:34Z

Rebased and removed the getinfo commit from @petertodd's original pull.

dcousens · 2015-09-06T14:26:36Z

utACK (again)

petertodd · 2015-09-07T17:13:07Z

ACK (again)

afb0cca Add NODE_BLOOM service bit and bump protocol version (Matt Corallo)

…ion) Github-Pull: bitcoin#6579 Partial-Rebased-From: afb0cca

rebroad · 2016-08-25T06:53:40Z

src/main.cpp

+              pfrom->nVersion >= NO_BLOOM_VERSION)
+    {
+        if (pfrom->nVersion >= NO_BLOOM_VERSION)
+            Misbehaving(pfrom->GetId(), 100);


Not sure why Misbehaving is used here. I thought this function was for the protection of the node using it, but I can't see what harm there would be if the connection was kept open. I think that yes, disconnect makes sense if this is on an outgoing connection and using up one of the outgoing slots.

sipa · 2016-08-25T06:57:01Z

As answered elsewhere, you need to disconnect, or the peer will waste time waiting for your responses later. It is polite to disconnect then, so they can find a better peer.

rebroad · 2016-09-12T18:49:21Z

src/version.h

@@ -9,7 +9,7 @@
 * network protocol versioning
 */

-static const int PROTOCOL_VERSION = 70002;
+static const int PROTOCOL_VERSION = 70011;


Why the jump from 70002 to 70011?

Add NODE_BLOOM service bit Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6579 - Zcash equivalent of BIP 111 - bitcoin/bitcoin#6652 - Docs for BIP 111 - bitcoin/bitcoin#7087 - bitcoin/bitcoin#7174 - bitcoin/bitcoin#8709 Part of #2074. Closes #2738.

evGUzQIEQaL4 mentioned this pull request Aug 21, 2015

Implement NODE_BLOOM, ability for nodes to disable serving BIP37 SPV clients. #6578

Closed

petertodd reviewed Aug 21, 2015
View reviewed changes

laanwj added the P2P label Aug 21, 2015

laanwj reviewed Sep 4, 2015
View reviewed changes

TheBlueMatt force-pushed the bloom branch 2 times, most recently from d27778f to abf1dbd Compare September 6, 2015 06:28

TheBlueMatt force-pushed the bloom branch from abf1dbd to afb0cca Compare September 6, 2015 06:29

TheBlueMatt mentioned this pull request Sep 6, 2015

De-neuter NODE_BLOOM #6641

Closed

laanwj merged commit afb0cca into bitcoin:master Sep 8, 2015

laanwj added a commit that referenced this pull request Sep 8, 2015

Merge pull request #6579

0c27795

afb0cca Add NODE_BLOOM service bit and bump protocol version (Matt Corallo)

laanwj mentioned this pull request Sep 8, 2015

doc: update release-notes and bips.md for BIP111 #6652

Merged

maflcko mentioned this pull request Sep 20, 2015

[doc] Update doc/bips.md #6705

Closed

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Dec 28, 2015

Recognise NODE_BLOOM service bit in protocol.h (not used in this vers…

11299bf

…ion) Github-Pull: bitcoin#6579 Partial-Rebased-From: afb0cca

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Dec 28, 2015

Recognise NODE_BLOOM service bit in protocol.h (not used in this vers…

bfa763a

…ion) Github-Pull: bitcoin#6579 Partial-Rebased-From: afb0cca

jonasschnelli mentioned this pull request Mar 18, 2016

De-neuter NODE_BLOOM #7708

Merged

rebroad reviewed Aug 25, 2016
View reviewed changes

rebroad reviewed Sep 12, 2016
View reviewed changes

bitcartel mentioned this pull request Oct 12, 2016

Merge upstream anti DoS patches zcash/zcash#1251

Open

defuse mentioned this pull request Oct 20, 2016

Manually backport upstream's NODE_BLOOM bit zcash/zcash#1574

Closed

1 task

tulip0 mentioned this pull request Jan 13, 2017

Bitcoin Core should throw a warning if peerbloomfilters=1 #9540

Closed

str4d mentioned this pull request Jul 14, 2017

Bitcoin 0.12 P2P/Net PRs 1 zcash/zcash#2534

Draft

str4d mentioned this pull request Dec 19, 2017

Add NODE_BLOOM service bit zcash/zcash#2814

Merged

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

		@@ -49,12 +49,13 @@ UniValue getinfo(const UniValue& params, bool fHelp)
		"{\n"

Add NODE_BLOOM service bit and bump protocol version #6579

Add NODE_BLOOM service bit and bump protocol version #6579

Conversation

TheBlueMatt commented Aug 21, 2015

dcousens commented Aug 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa Sep 3, 2015 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasschnelli commented Aug 21, 2015

pstratem commented Aug 21, 2015

laanwj commented Aug 21, 2015

evGUzQIEQaL4 commented Aug 21, 2015

evoskuil commented Aug 21, 2015

davecgh commented Aug 21, 2015

petertodd commented Aug 24, 2015

jgarzik commented Aug 24, 2015

jgarzik commented Aug 24, 2015

evGUzQIEQaL4 commented Aug 24, 2015

laanwj commented Aug 24, 2015

mikehearn commented Aug 24, 2015

schildbach commented Aug 24, 2015

dcousens commented Aug 24, 2015

jgarzik commented Aug 24, 2015

gavinandresen commented Aug 24, 2015

laanwj commented Aug 24, 2015

jgarzik commented Aug 24, 2015

laanwj commented Aug 24, 2015

mikehearn commented Aug 24, 2015

laanwj commented Aug 24, 2015

jgarzik commented Aug 24, 2015

mikehearn commented Aug 24, 2015

petertodd commented Aug 25, 2015

TheBlueMatt commented Aug 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Sep 4, 2015

TheBlueMatt commented Sep 6, 2015

dcousens commented Sep 6, 2015

petertodd commented Sep 7, 2015

rebroad Aug 25, 2016 • edited Loading

Choose a reason for hiding this comment

sipa commented Aug 25, 2016

Choose a reason for hiding this comment

rebroad Aug 25, 2016 •

edited

Loading