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

Add NODE_BLOOM service bit and bump protocol version #6579

Merged
merged 1 commit into from Sep 8, 2015

Conversation

@TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Aug 21, 2015

Rehashing #2900, BIP to be posted soon.

@dcousens
Copy link
Contributor

@dcousens dcousens commented Aug 21, 2015

concept ACK

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

This comment has been minimized.

@petertodd

petertodd Aug 21, 2015
Contributor

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.

This comment has been minimized.

@pstratem

pstratem Aug 21, 2015
Contributor

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.

This comment has been minimized.

@petertodd

petertodd Aug 21, 2015
Contributor

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.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Aug 21, 2015
Author Contributor

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

This comment has been minimized.

@laanwj

laanwj Sep 3, 2015
Member

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.

This comment has been minimized.

@petertodd

petertodd Sep 3, 2015
Contributor

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

This comment has been minimized.

@sipa

sipa via email Sep 3, 2015
Member

This comment has been minimized.

@petertodd

petertodd Sep 3, 2015
Contributor

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

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Sep 3, 2015
Author Contributor

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

This comment has been minimized.

@laanwj

laanwj Sep 3, 2015
Member

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

@petertodd
petertodd reviewed Aug 21, 2015
View changes
src/rpcmisc.cpp Outdated
" \"walletversion\": xxxxx, (numeric) the wallet version\n"
" \"balance\": xxxxxxx, (numeric) the total bitcoin balance of the wallet\n"
" \"blocks\": xxxxxx, (numeric) the current number of blocks processed in the server\n"
" \"timeoffset\": xxxxx, (numeric) the time offset\n"
" \"connections\": xxxxx, (numeric) the number of connections\n"
" \"proxy\": \"host:port\", (string, optional) the proxy used by the server\n"
" \"proxy\": \"host:port\", (string, optional) the proxy used by the server\n"

This comment has been minimized.

@petertodd

petertodd Aug 21, 2015
Contributor

stray space

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Aug 21, 2015
Author Contributor

Nope, purposeful space. It makes it lign up on RPC output.

@petertodd
petertodd reviewed Aug 21, 2015
View changes
src/rpcmisc.cpp Outdated
@@ -62,7 +63,7 @@ UniValue getinfo(const UniValue& params, bool fHelp)
" \"unlocked_until\": ttt, (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked\n"
" \"paytxfee\": x.xxxx, (numeric) the transaction fee set in " + CURRENCY_UNIT + "/kB\n"
" \"relayfee\": x.xxxx, (numeric) minimum relay fee for non-free transactions in " + CURRENCY_UNIT + "/kB\n"
" \"errors\": \"...\" (string) any error messages\n"
" \"errors\": \"...\" (string) any error messages\n"

This comment has been minimized.

@petertodd

petertodd Aug 21, 2015
Contributor

another stray space

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Aug 21, 2015
Author Contributor

Nope, purposeful space. It makes it lign up on RPC output.

@jonasschnelli
jonasschnelli reviewed Aug 21, 2015
View changes
src/main.cpp Outdated
{
if (pfrom->nVersion >= NO_BLOOM_VERSION)
Misbehaving(pfrom->GetId(), 100);
//TODO: Enable this after reasonable network upgrade

This comment has been minimized.

@jonasschnelli

jonasschnelli Aug 21, 2015
Member

Is the else part required? With a score of 100 it get directly banned (with the default -banscore) anyways?

This comment has been minimized.

@petertodd

petertodd Aug 21, 2015
Contributor

Well, the idea would be that >= NO_BLOOM_VERSION should know not to send filters to non-bloom-supporting peers - it's misbehavior in that case.

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Aug 21, 2015

utACK.

@pstratem
Copy link
Contributor

@pstratem pstratem commented Aug 21, 2015

utACK (testing now)

@laanwj
Copy link
Member

@laanwj laanwj commented Aug 21, 2015

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

@laanwj laanwj added the P2P label Aug 21, 2015
@evGUzQIEQaL4
Copy link

@evGUzQIEQaL4 evGUzQIEQaL4 commented Aug 21, 2015

@evoskuil
Copy link

@evoskuil evoskuil commented Aug 21, 2015

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

@davecgh
Copy link
Contributor

@davecgh davecgh commented Aug 21, 2015

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

@petertodd petertodd commented Aug 24, 2015

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

@jgarzik
Copy link
Contributor

@jgarzik jgarzik commented Aug 24, 2015

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

@jgarzik jgarzik commented Aug 24, 2015

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

@evGUzQIEQaL4 evGUzQIEQaL4 commented Aug 24, 2015

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

@laanwj laanwj commented Aug 24, 2015

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

@mikehearn mikehearn commented Aug 24, 2015

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

@schildbach schildbach commented Aug 24, 2015

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

@dcousens
Copy link
Contributor

@dcousens dcousens commented Aug 24, 2015

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

@jgarzik jgarzik commented Aug 24, 2015

  1. It sounds like this does negatively impact major user segments.

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

@gavinandresen gavinandresen commented Aug 24, 2015

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

@laanwj
Copy link
Member

@laanwj laanwj commented Aug 24, 2015

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

@jgarzik jgarzik commented Aug 24, 2015

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

@laanwj
Copy link
Member

@laanwj laanwj commented Aug 24, 2015

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

@mikehearn mikehearn commented Aug 24, 2015

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

@laanwj laanwj commented Aug 24, 2015

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

@jgarzik jgarzik commented Aug 24, 2015

@mikehearn

  1. Agree on user benefit

  2. Let's not assume about others motivations. It remains true that there is an attack vector.

@mikehearn
Copy link
Contributor

@mikehearn mikehearn commented Aug 24, 2015

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.

@sipa
Copy link
Member

@sipa sipa commented Aug 25, 2015

Given that pruning will likely become more granular, with nodes advertizing
ranges or other subsets of blocks they maintain, I think the meaning of
NODE_BLOOM should be "I support the filter* commands and will apply the
filters to reportee blocks and transactions, regardless of what blocks and
transactions I actually have available", and not "I serve all filtered
blocks".

@petertodd
Copy link
Contributor

@petertodd petertodd commented Aug 25, 2015

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

@TheBlueMatt
Copy link
Contributor Author

@TheBlueMatt TheBlueMatt commented Aug 25, 2015

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

On August 25, 2015 10:21:05 AM PDT, Peter Todd notifications@github.com 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
laanwj reviewed Sep 4, 2015
View changes
src/rpcmisc.cpp Outdated
@@ -49,12 +49,13 @@ UniValue getinfo(const UniValue& params, bool fHelp)
"{\n"

This comment has been minimized.

@laanwj

laanwj Sep 4, 2015
Member

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

This comment has been minimized.

@petertodd

petertodd Sep 4, 2015
Contributor

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

@laanwj Thanks for catching that!

@laanwj
Copy link
Member

@laanwj laanwj commented Sep 4, 2015

ACK, apart from nit about getinfo.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:bloom branch 2 times, most recently Sep 6, 2015
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 TheBlueMatt force-pushed the TheBlueMatt:bloom branch to afb0cca Sep 6, 2015
@TheBlueMatt
Copy link
Contributor Author

@TheBlueMatt TheBlueMatt commented Sep 6, 2015

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

@TheBlueMatt TheBlueMatt mentioned this pull request Sep 6, 2015
@dcousens
Copy link
Contributor

@dcousens dcousens commented Sep 6, 2015

utACK (again)

@petertodd
Copy link
Contributor

@petertodd petertodd commented Sep 7, 2015

ACK (again)

@laanwj laanwj merged commit afb0cca into bitcoin:master Sep 8, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Sep 8, 2015
afb0cca Add NODE_BLOOM service bit and bump protocol version (Matt Corallo)
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Dec 28, 2015
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Dec 28, 2015
pfrom->nVersion >= NO_BLOOM_VERSION)
{
if (pfrom->nVersion >= NO_BLOOM_VERSION)
Misbehaving(pfrom->GetId(), 100);

This comment has been minimized.

@rebroad

rebroad Aug 25, 2016
Contributor

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

@sipa sipa commented Aug 25, 2016

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.

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

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

This comment has been minimized.

@rebroad

rebroad Sep 12, 2016
Contributor

Why the jump from 70002 to 70011?

else if (!(nLocalServices & NODE_BLOOM) &&
(strCommand == "filterload" ||
strCommand == "filteradd" ||
strCommand == "filterclear") &&

This comment has been minimized.

@rebroad

rebroad Sep 13, 2016
Contributor

Ages back, before this was merged, I'd written code that permitted turning off TX relaying during IBD (when relaying TXs is less useful to do most TXs being orphans), and then when we were getting close to the latest block, it would send a "filterclear" message to all connected nodes in order for them to start relaying TXs. If I was to be using this code today, I would be getting banned from every fairly recent Bitcoin Core node. Somehow seems a little extreme considering all I am effectively doing is offering to relay ALL TXs they sent to my node. Presumeably there isn't any DoS vector in getting a node to revert to behaviour that has existed since the very first version of the Satoshi client - and given this pull request was primarily for anti-DoS reasons, then could perhaps "filterclear" be excluded from the list of messages that cause the node to be banned? Or is thre a DoS vector in allowng only "filterclear" messages? Without allowing "filterclear" then I can see no other way to enable TX relaying at a point in time after the version message has been sent.

@defuse defuse mentioned this pull request Oct 20, 2016
0 of 1 task complete
zkbot added a commit to zcash/zcash that referenced this pull request Apr 5, 2018
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.