Add NODE_BLOOM service bit and bump protocol version

TheBlueMatt commented at 4:39 am on August 21, 2015: member

Rehashing #2900, BIP to be posted soon.

dcousens commented at 5:20 am on August 21, 2015: contributor

concept ACK

in src/main.cpp: in 2fec72d37d outdated

4597+    {
4598+        if (pfrom->nVersion >= NO_BLOOM_VERSION)
4599+            Misbehaving(pfrom->GetId(), 100);
4600+        //TODO: Enable this after reasonable network upgrade
4601+        //else
4602+        //    pfrom->fDisconnect = true;

petertodd commented at 5:50 am on August 21, 2015:

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.

pstratem commented at 5:59 am on August 21, 2015:

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.

petertodd commented at 6:11 am on August 21, 2015:

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.

TheBlueMatt commented at 5:08 pm on August 21, 2015:

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

laanwj commented at 3:53 pm on September 3, 2015:

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.

petertodd commented at 4:03 pm on September 3, 2015:

FWIW I did test my original disconnect instantly approach, and (at the time) the wallets I tested it with handled it just fine.

petertodd commented at 4:05 pm on September 3, 2015:

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

sipa commented at 4:05 pm on September 3, 2015:

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

petertodd commented at 4:06 pm on September 3, 2015:

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

TheBlueMatt commented at 5:51 pm on September 3, 2015:

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

laanwj commented at 6:00 pm on September 3, 2015:

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

in src/rpcmisc.cpp: in 2fec72d37d outdated

54             "  \"balance\": xxxxxxx,         (numeric) the total bitcoin balance of the wallet\n"
55             "  \"blocks\": xxxxxx,           (numeric) the current number of blocks processed in the server\n"
56             "  \"timeoffset\": xxxxx,        (numeric) the time offset\n"
57             "  \"connections\": xxxxx,       (numeric) the number of connections\n"
58-            "  \"proxy\": \"host:port\",     (string, optional) the proxy used by the server\n"
59+            "  \"proxy\": \"host:port\",       (string, optional) the proxy used by the server\n"

petertodd commented at 5:50 am on August 21, 2015:

stray space

TheBlueMatt commented at 5:07 pm on August 21, 2015:

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

in src/rpcmisc.cpp: in 2fec72d37d outdated

62@@ -62,7 +63,7 @@ UniValue getinfo(const UniValue& params, bool fHelp)
63             "  \"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"
64             "  \"paytxfee\": x.xxxx,         (numeric) the transaction fee set in " + CURRENCY_UNIT + "/kB\n"
65             "  \"relayfee\": x.xxxx,         (numeric) minimum relay fee for non-free transactions in " + CURRENCY_UNIT + "/kB\n"
66-            "  \"errors\": \"...\"           (string) any error messages\n"
67+            "  \"errors\": \"...\"             (string) any error messages\n"

petertodd commented at 5:50 am on August 21, 2015:

another stray space

TheBlueMatt commented at 5:07 pm on August 21, 2015:

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

in src/main.cpp: in 2fec72d37d outdated

4595+               strCommand == "filteradd" ||
4596+               strCommand == "filterclear"))
4597+    {
4598+        if (pfrom->nVersion >= NO_BLOOM_VERSION)
4599+            Misbehaving(pfrom->GetId(), 100);
4600+        //TODO: Enable this after reasonable network upgrade

jonasschnelli commented at 6:10 am on August 21, 2015:

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

petertodd commented at 6:13 am on August 21, 2015:

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 commented at 6:11 am on August 21, 2015: contributor

utACK.

pstratem commented at 6:14 am on August 21, 2015: contributor

utACK (testing now)

laanwj commented at 8:00 am on August 21, 2015: member

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

laanwj added the label P2P on Aug 21, 2015

evGUzQIEQaL4 commented at 10:25 am on August 21, 2015: none

BIP on the mailing list (still unnumbered).

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

evoskuil commented at 11:18 am on August 21, 2015: none

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

davecgh commented at 5:59 pm on August 21, 2015: contributor

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 commented at 2:09 am on August 24, 2015: contributor

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

jgarzik commented at 3:47 am on August 24, 2015: contributor

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 commented at 4:04 am on August 24, 2015: contributor

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 commented at 4:10 am on August 24, 2015: none

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 commented at 10:31 am on August 24, 2015: member

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 commented at 2:20 pm on August 24, 2015: contributor

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 commented at 2:32 pm on August 24, 2015: contributor

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

dcousens commented at 2:44 pm on August 24, 2015: contributor

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 commented at 2:51 pm on August 24, 2015: contributor

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 commented at 3:08 pm on August 24, 2015: contributor

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

laanwj commented at 3:09 pm on August 24, 2015: member

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)

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

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

jgarzik commented at 3:10 pm on August 24, 2015: contributor

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

laanwj commented at 3:12 pm on August 24, 2015: member

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 commented at 3:14 pm on August 24, 2015: contributor

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 commented at 3:20 pm on August 24, 2015: member

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 commented at 3:31 pm on August 24, 2015: contributor

@mikehearn

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

mikehearn commented at 3:32 pm on August 24, 2015: contributor

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.

laanwj commented at 3:35 pm on August 24, 2015: member

That’s why it is an I/O DoS.

jgarzik commented at 3:37 pm on August 24, 2015: contributor

What @laanwj said…

mikehearn commented at 3:40 pm on August 24, 2015: contributor

It’s not an IO DoS. Please read the email I sent. There is no DoS - no service is denied, which is the essential element of a DoS attack.

bitcoind spends much more time just deserializing the blocks than it does waiting for disk seeks. If it was an IOP DoS then CPU usage would be low. In fact it pegs the core.

sipa commented at 3:40 pm on August 24, 2015: member

Even ignoring the DoS argument, the fact is that there are codebases that do not support this feature…

laanwj commented at 3:45 pm on August 24, 2015: member

The exact impact depends on the speed of the disk and the speed of the CPU. It is a fact that allowing a client to request blocks without sending them over the network opens up the possibility to generate lots of I/O. There is not even a way to optimize or index for it.

Obviously I agree that bloom filtering provides a useful service, that’s why it deserves a service bit. Otherwise it should just be removed. But I don’t see any of this as an argument to disallow disabling it. It is not required for participating in the network as a full node.

mikehearn commented at 3:51 pm on August 24, 2015: contributor

The attack doesn’t let you generate lots of I/O because bitcoind is single threaded and spends most of its time digesting the data it just loaded. IO is simply not a bottleneck in this attack at all. I tested this both on my laptop and the bog-standard VPS I had for the last ~15 years.

Is there any experimental data I can give you that would convince you this theory is wrong? @sipa Any protocol can have half-complete implementations. That doesn’t mean every feature should become optional. Otherwise it’d become really hard to implement anything, you’d have to try and implement sane fallbacks for every case and eventually it just gets nuts. OpenGL had this problem, everything got added as an optional extension and it resulted in a horrible developer and user experience. Direct3D kicked their ass because it was willing to mandate functionality. Eventually the GL guys got a lot stricter about expanding the base functionality set.

Anyway, this change clearly does not have consensus.

dcousens commented at 4:03 pm on August 24, 2015: contributor

Otherwise it’d become really hard to implement anything, you’d have to try and implement sane fallbacks for every case and eventually it just gets nuts

I thought the point of this was to be trust less; in which case, we have to have sane fall backs.

I’d rather ask nodes if they support a service, and if they do, penalize them if they fail to provide that service. Right now, I might penalize nodes that simply don’t want to support one feature of the protocol.

Anyway, this change clearly does not have consensus.

Consensus is only relevant on the NODE_BLOOM service bit protocol specifics. It is not at all relevant on making this an optional feature to support.

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

That sounds like a misconstrued intent, and might be misinterpreted as a connection issue by the client. I think it would be beneficial to make this intent clear to avoid over-penalization of honest nodes.

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.

Which is why the default behaviour remains. If users are configuring to disable SPV support en masse, then perhaps that is the clearest measure of consensus that they don’t want to support that feature.

edit: made some additional comments

laanwj commented at 4:04 pm on August 24, 2015: member

@mikehearn Ok, it doesn’t strictly need to be single-threaded, though. That’s an artifact of how things are handled in BitcoinCore’s current implementation. Probably a cs_main lock that is held all the time. I think that’s a good point, in a way: by artificially slowing down bloom queries the DoS can be avoided: don’t read blocks from disk faster than you would if you had to send them over the network.

But no, this won’t convince me that this change is not “needed”. I’m looking at this from the other way around. Providing this service is not needed, it is something that can be offered altruistically. It should have been a service bit from the beginning.

Full community consensus is important for changes to the consensus code, certainly for hard forks. Not for an optional behavior change in the P2P network code.

Roasbeef commented at 4:06 pm on August 24, 2015: none

For the record: btcd does in fact fully support bloom filtering.

However, there is a patch that was proposed earlier this year which introduces a CLI flag that if enabled, just sends merkleblock messages containing the entire, unfiltered block (lowering CPU utilization, but increasing bandwidth usage for the node).

TheBlueMatt commented at 4:13 pm on August 24, 2015: member

This discussion seems to have been derailed, so let me make this clear: Neither this pull nor the BIP has any stated intention of phasing out bloom filtering support in the protocol. As much as I’d love to, I 100% agree with @mikehearn here, that would break any ability of SPV clients to operate on the P2P network (either as a way to double-check centralized servers, or otherwise), and that is really not a good idea without a replacement in place. This pull/BIP DOES suggest we phase out REQUIRED bloom filtering support in the protocol - thereby fixing the peer selection of SPV clients in the face of btcd with some flags/many patched versions of Core/etc peers, providing a remedy for a potential DoS attack, etc.

TheBlueMatt commented at 4:25 pm on August 24, 2015: member

@jgarzik re: clients being updated: I’d love to hear a better way? Right now, as an SPV client expecting bloom filtering support, you can connect to a number of nodes, and they will disconnect you/not filter/etc. This allows us to keep exactly that same behavior (for nodes which do not disable the enabled-by-default flag), while giving SPV clients a better way to find peers after a relatively small update.

davecgh commented at 4:30 pm on August 24, 2015: contributor

I want to clarify that we don’t like not having a proper option to disable the feature in btcd not because we simply don’t want to implement that part of the protocol (it is already done and has been for over a year), rather because it is a dangerous, low-cost attack vector and we want to provide our users with the choice.

This isn’t some theoretical concern either. Back when we implemented this feature in btcd, we realized just how dangerous it is due to the ease by which malicious actors can use it at almost zero cost to attack virtually every reachable node on the network for very little effort. In order to test our concerns, we wrote a utility (in roughly less than an hour I might add, so it wasn’t even difficult) that makes use of bloom filters to effectively make some nodes unusable and severely hamper others. We then proceeded to setup a private simulation network and test the results. In short, we confirmed our fears. A single node can be used to wreak havoc on every reachable node on the network at almost zero cost the attacker. This experience greatly differs from what Mike is claiming above about it not really affecting anything. Also, the assumption about single threads disregards the fact that is a current implementation detail in Bitcoin Core that might change and btcd is already not single threaded.

Before anyone asks, no the code for the tool is not released and no we won’t release it. We care about Bitcoin and refuse to release things that can trivially be used to harm it. Honestly, I was very hesitant to publicly post that we even created such a tool and performed internal testing as this community has historically liked to spread a lot of FUD, particularly about alternative full node implementations, and should a malicious attacker start to use this attack vector, the first thing people will probably do is point to this post and blame us as the culprits.

It is not my intention to offend anyone, but the fact is that the current implementation of bloom filters are a dangerous feature that is forced onto all implementations with no proper recourse in choosing not to support it. As I mentioned above, currently the alternatives are a protocol break or simply not applying the filters while claiming to do so. Neither of those options is better for the clients than having an optional bit which signals support.

TheBlueMatt commented at 4:33 pm on August 24, 2015: member

@davecgh Sadly, there are other such attack codes already public (dont know about their effectiveness, however), eg https://github.com/petertodd/bloom-io-attack

mikehearn commented at 4:38 pm on August 24, 2015: contributor

Dave, if you send me the code of your attack tool I’ll compare it to mine and see if I can reproduce what you saw with Bitcoin Core/XT.

Regardless, this entire approach is wrong. You don’t fix denial of service attacks by denying service to your users! That’s exactly what the attacker wants. You fix them with better resource scheduling, prioritisation and potentially bans, to ensure your real users are serviced first.

Bitcoin Core, for now at least, seems to survive even when being pelted with tons of getdatas for filtered blocks. Given the CPU profiles this is not surprising: it’s doing much the same thing as it would just serving full blocks.

A hypothetical future version of Core might be multi-threaded or optimised in other ways, moving the bottleneck around. But then a hypothetical future version of Core can also be smarter about how it handles DoS attacks. @TheBlueMatt Glad to hear you don’t think phasing out Bloom filtering is realistic. In that case, given that apparently only libbitcoin doesn’t implement it and that’s not widely used at all, it seems this change is a form of makework. Nodes that don’t want to implement the protocol can just not listen, or (at worst) just disconnect peers that want this service to make them go away, just as they can today.

dcousens commented at 4:51 pm on August 24, 2015: contributor

@TheBlueMatt Nodes that don’t want to implement the protocol can just not listen, or (at worst) just disconnect peers that want this service to make them go away, just as they can today.

Except, that sucks. I’m not going to repeat my comments, but to avoid you just stepping over them, here is a link.

TheBlueMatt commented at 5:07 pm on August 24, 2015: member

@mikehearn I was also referring to some Bitcoin Core nodes which are patched to not support bloom filtering, eg for some miner relaynodes where sleeping for one second responding to some bloom requests is a huge problem.

mikehearn commented at 5:22 pm on August 24, 2015: contributor

Those nodes should not listen at all. If someone finds a new kind of DoS attack that does actually cause DoS against the RPC interface, you’d be vulnerable. If your node is super-critical and very latency sensitive, just not listening solves all kinds of potential problems in one go.

Look - I realise that just having a node bit is not the end of the world. But it also doesn’t solve anything, as the stated rationale’s for the BIP aren’t valid, and the feature is not something that can actually be removed without breaking lots of wallets. If there are problems with it they need to be fixed, not used as an excuse to kill an important feature by the Blockstream employees who already admit they would love to do so. @dcousens I did read your comment. You say “that sucks” but the alternative also sucks for a different set of people.

TheBlueMatt commented at 5:26 pm on August 24, 2015: member

@mikehearn Those nodes absolutely should listen, in fact that is the reason they exist - to listen and get as many connections as possible out to the network.

mikehearn commented at 5:36 pm on August 24, 2015: contributor

Oh, I see where this goes - of course, a node can easily establish lots of outbound connections, but Core doesn’t currently allow that to be configured, does it. So the only way to get lots of connections is to listen.

But that’s a self-imposed constraint. There’s a patch to fix it, it could be merged tomorrow. Then miner nodes could have lots of connections without listening.

TheBlueMatt commented at 5:45 pm on August 24, 2015: member

I think this line of discussion has gone rather tangential to the issue at hand. Sufficient to say, some people do, today, run listening nodes which, for perfectly logical reasons (not just “I dont want to implement this”), do not support filtered connections but nonetheless support the rest of the protocol.

GamerSg commented at 6:03 pm on August 24, 2015: contributor

I fully agree that there should be an option to turn this off.

I run 3 full nodes, 1 on a production server and 2 on my personal machines.

I find it absurd that i cannot turn off bloom filtering on my production server where bitcoind runs on the same machine as my webserver and DB. The site/service should not be disrupted because bitcoind is busy serving some guy on a SPV wallet who wants to buy coffee.

I have no issues leaving Bloom filtering on my personal machines but i should have the ability to disable it if i need to.

Moreover, pruned nodes will require this to indicate to SPV wallets that they are unable to service their requests anyway.

I can see Mike’s point with his OpenGL example. However i do not think providing bloom filtering or the servicing of SPV clients is remotely close to a mandatory protocol requirement, especially when taking into account that provision of this service is a matter of altruism.

mikehearn commented at 6:09 pm on August 24, 2015: contributor

@GamerSg My tests don’t seem to indicate any actual issue with filtering, but I do understand your concern. For a production critical node you want to minimise things that can go wrong and that’s entirely reasonable.

A better way to give you what you want is to let outbound connections be configured and then you can disable listening. That means you are protected against any funny protocol games unless you get really unlucky and connect to an attacking node. However it’d require merging this:

https://github.com/bitcoin/bitcoin/pull/6014/files

I think we’ll probably add that to XT, as the arguments against it weren’t really convincing to me or Gavin, and as we can see above, Matt is arguing that useful features for clients should be disabled because you “must” listen if you have a miner node. But that’s only because the above patch wasn’t merged. If it was, that logic would no longer apply.

Once that’s fixed, I disagree there’s any logical reason to disable this feature: I think we analysed all of the suggested reasons by now.

But hey, I guess you’ll do this anyway, despite the objections of various wallet developers and other protocol experts. So go ahead and define a node bit for it. Handling nodes that don’t support it will go onto wallet devs giant TODO lists, and get prioritised along with other things.

gmaxwell commented at 6:31 pm on August 24, 2015: contributor

@mikehearn No one is suggesting removing the functionality, or even disabling it by default. All that is being discussed is making it optional in a way that doesn’t break users completely– usually a pretty good principle of protocol design for resource intensive functionality. Sometimes the cost/complexity with making something optional isn’t worth it– but for resource intensive features, it often is.

As an aside, the claim that BIP37 filters provide any privacy in practice appears to have been soundly disproved ( https://eprint.iacr.org/2014/763.pdf among others ) and it’s unclear how much privacy can usefully provide even in theory. There are alternative proposals for things which would be private, lack the transaction hiding vulnerability, be less resource intensive on the servers, etc. which have been tendered. Phasing out the feature wouldn’t make sense now, but it may sometime in the future and so that shouldn’t be precluded by making it mandatory in a way that is hard to correctly handle.

Regarding #6014 it is a facility to increase the number of outbound connections. It’s suggested application was improving mining pool relay, but measurements show it has the opposite effect. It also wastes capacity on external peers, and in the future will likely increase the odds of a node being banned by peers for being a mass connector. I do not see why it’s been implicated to address the fairly reasonable request of making an unneeded expensive to offer service optional– disabling listening is sufficient if you’ll really tolerate a nuclear workaround– it doesn’t also require making an ordinary node look like an abusive user monitoring mass-connecting one. (Though I can see why some people would prefer that ordinary nodes and spy nodes would appear indistinguishable)

laanwj commented at 0:15 am on August 25, 2015: member

@mikehearn Your suggestion for #6014 is typical of the kind of shallow half-solutions you’ve been pursuing in your fork.

Sure, disabling listening will make it significantly harder to target you specifically. However it is no robust solution: if the goal is to attack the network then setting up ’trap’ nodes that attack any connecting full node would work as well. Furthermore by combining different exploits (MiTM, BGP, …) it may still be possible to tickle your node to connect to a specific node and attack it.

You are distracting from the problem at hand and proposing something that may have more dangerous consequences for the network. In this case you apparently prefer an option to cause obnoxious node behavior, which #6014 allows by exhausting connection slots, to allowing people to disable a known DoS vector on their own node with as little impact as possible.

dcousens commented at 0:58 am on August 25, 2015: contributor

@mikehearn you mentioned the following:

the alternative also sucks for a different set of people.

But you have failed to address why this is actually a problem? (or address any of my points for that matter).

The arguments you presented have been based around disruption to services and users, yet, this change does not change the protocols current availability in any way.

Your argument for nodes who shouldn’t be allowed to disable SPV are contradicted by your own instructions on doing it in a protocol-hostile way.

Remember, this PR does not change the default behaviour of allowing filter* commands.

It just makes it explicit that the node does support them, to avoid both DoS to users AND nodes a-like.

Therefore, I think this can only increase the overall quality of the network; as it avoids pervasive behaviours of “just disconnect users you don’t like”, which, while valid, is quite hostile to an open network.

evGUzQIEQaL4 commented at 3:01 am on August 25, 2015: none

@Roasbeef Sorry misread other btd commenter, so you support BIP37 but would prefer not to for denial of service reasons? That seems quite reasonable.

However, there is a patch that was proposed earlier this year which introduces a CLI flag that if enabled, just sends merkleblock messages containing the entire, unfiltered block (lowering CPU utilization, but increasing bandwidth usage for the node).

Keep in mind that patch would have a severe effect on people using mobile wallets based on BIP37. If they expect to download 50MB of data and end up downloading 50GB over their 3G connection in some countries this could cost tens of thousands of dollars. As far as I can find no wallet has the ability to detect the overage of data usage and just keeps downloading and processing more blocks blindly. Absent NODE_BLOOM you would be best kicking them, ignoring their requests, or doing a soft denial of service against the client by returning fake-filtered blocks with no contents.

However it is no robust solution: if the goal is to attack the network then setting up ’trap’ nodes that attack any connecting full node would work as well. Furthermore by combining different exploits (MiTM, BGP, …) it may still be possible to tickle your node to connect to a specific node and attack it. @laanwj I’m slightly surprised to learn that BIP37 commands are respected for outgoing connections, that seems to be simultaneously a liability for non listening nodes and completely useless. It means the comment about not listening protects you from it is fairly poor. Chainalysis was quite successful in getting thousands of nodes to connect to them, it seems quite feasible to do that. Probably room for making that a 100+ DoS score, I don’t see any reason for anybody to ever use it like that except for denial of service.

evoskuil commented at 6:34 am on August 25, 2015: none

If everyone disables listening, how exactly do outbound connections work?

evGUzQIEQaL4 commented at 7:12 am on August 25, 2015: none

I think we’ll probably add that to XT, as the arguments against it weren’t really convincing to me or Gavin, and as we can see above, Matt is arguing that useful features for clients should be disabled because you “must” listen if you have a miner node. But that’s only because the above patch wasn’t merged. If it was, that logic would no longer apply. @mikehearn That’s ludicrous, you know that the network has a very small number of listening sockets, a large number which are being burnt by irresponsible services like bitnodes.io (6000+) and blockchain.info (2000+). People will mindlessly increase their number of outgoing connections to the point that we reach saturation and nobody will be able to join the network, that includes BIP37 SPV wallets. In total we have only 700k sockets, and every single non listening node already burns 8 of them without your patch, a maximum of 87k non listening nodes with no room for denial of service absorption or SPV clients. @evoskuil They don’t, listening sockets are a precious resource.

mikehearn commented at 11:18 am on August 25, 2015: contributor

Well, this is drifting off topic - but bitnodes.io doesn’t hold connections open all the time by any means, which is why they appear so often in the logs. Most of the sockets that I can see being used on my nodes are by long term nodes, or transient SPV wallets used by real users.

Regardless, I was responding to the argument “you have to listen to get lots of connections which is why everything that might use resources should be optional”. It sounds like some of you disagree there’s ever any reason to open lots of connections. Alright then - that’s why I said there’s no downside to just disabling listening if you have a node that you cannot risk any kind of disruption at all (i.e. not most nodes). Kills multiple possibly unknown birds with one stone.

WRT optionality. If indeed the code is never disabled, then this whole change has very little impact either way. That’s why I’m asking why bother? It adds complexity that is only wanted by a tiny number of node implementations (just libbitcoin and pseudo-node I think?), but every wallet author will have to deal with.

If the code is disabled, which frankly, I can easily see you guys arguing for in future given recent comments elsewhere (eg this “spv-lite” nonsense), then we just substitute one set of problems with another.

So it seems like a poor cost/benefit tradeoff, to me.

But anyway, whatever, you guys do what you like.

mikehearn commented at 11:19 am on August 25, 2015: contributor

@laanwj By the way, you make it sound like hijacking BGP is something anyone can do from their bog-standard Windows box. All attacks have costs and all you can ever do is raise those costs. The cost difference between “make a TCP connection and send a message” and “do a globally visible BGP hijack” is ….. extremely large. I’ll take it.

evGUzQIEQaL4 commented at 11:32 am on August 25, 2015: none

It adds complexity that is only wanted by a tiny number of node implementations (just libbitcoin and pseudo-node I think?), but every wallet author will have to deal with.

Seeing as the majority of SPV wallets are backed by BitcoinJ, you can deal with them quite handily in a single change. The authors of btcd also commented that they would like it given their concerns of denial of service, which is completely valid.

My tests don’t seem to indicate any actual issue with filtering, but I do understand your concern.

Would you mind publishing your tests and tools to produce them? Your results don’t mesh with other peoples concerns, it would be interesting to see how you came to the conclusion that BIP37 is harmless.

mikehearn commented at 11:55 am on August 25, 2015: contributor

I did publish my tests: see my email to the mailing list. If you mean publish the code then I won’t do that for the same reasons Dave gave. Peter Todd has published a program which is rather similar (and I notice it pretends to be bitcoinj - that’s piss poor behaviour right there).

I have a snapshot of the CPU profiles saved to disk. I can send them to you if you like. They show about 90% of the time spent loading blocks off disk, mostly inside CTransaction::UpdateHash. The issue is that tx hashes are not cached on disk alongside blocks, so loading a block ends up spending a lot of time in SHA256. It looks to me like block loading and filtering could be optimised a lot.

Seeing as the majority of SPV wallets are backed by BitcoinJ, you can deal with them quite handily in a single change

Maybe it will help if I spell out the cost/benefit analysis I’m using.

There are several SPV wallet engines out there that aren’t bitcoinj. And because the DNS protocol doesn’t support service bit queries, all the operators would have to update to exclude !NODE_BLOOM by default as well, or everyone would have to migrate to the Cartographer protocol which does support more advanced queries.

And then bitcoinj is a library, so all the apps that use that library have to have new versions issued, including desktop wallets that may not even be maintained anymore but which people still use, or where users can reject upgrades.

So there’s a fair chunk of work. Of course it’s doable. That’s the cost.

If we weigh it against the benefits, well, I think they’re pretty small. DoS attacks have to be fixed no matter what, as users rely on it. A simple prioritisation scheme can easily mitigate issues if they were to arise, e.g. if you have 10 IPs with a pending filtered getdata request, pick the one that has so far requested the fewest blocks. DoS connections will rapidly get the highest counts and end up being serviced last. If your node uses work scheduling and thread priorities correctly it should not disrupt any other more important tasks and sure enough, in Core, it doesn’t seem to. At least with the obvious/basic form of the attack.

That leaves “I don’t want to implement it because it’s extra work”. But it’s a few pages of code, even in C++. Compared to the rest of the protocol it’s really not a big deal.

So: higher costs than benefits -> not a good idea.

That’s the rationale. It’s debatable. If you think there will be way more node implementations than wallet implementations perhaps the cost/benefit ratio looks different. But so far I think we see the opposite.

However just saying “everyone uses bitcoinj so there’s no cost” is not really true.

evGUzQIEQaL4 commented at 12:34 pm on August 25, 2015: none

@mikehearn So you wouldn’t have a problem with a Bitcoin Core option that disabled BIP37 responses entirely without making any attempt to signal it to its peers? Is dropping them the best, faking filtering and returning blank blocks, or just ignoring those responses entirely? You’re the one who is going to need to make sure BitcoinJ can handle those situations properly so your contributions here are invaluable.

I did publish my tests: see my email to the mailing list. If you mean publish the code then I won’t do that for the same reasons Dave gave.

I guess it’s just a bit weird to have changes denied because someone did all this testing which shows everything is absolutely fine but can’t actually show their process. It’s not like exhaustion attacks against BIP37 are a new thing, the code is already out there. If it is as benign as you have said then there should be absolutely no concern of anybody using it to attack Bitcoin nodes, wouldn’t you say?

Peter Todd has published a program which is rather similar

Peter Todds implementation is actually pretty weak now and could do with an upgrade, as there are much larger blocks in the chain now from the spam attack you can do a much better job denial of service attacking clients if you hand pick blocks which have the highest impact on the node.

If your BIP101 goes through then we will inevitably have 8x bigger blocks with which to perform the attack, is there enough headroom in the “not a problem” levels of denial of service possible? Come January this will be a much more potent vector if you get your way.

(and I notice it pretends to be bitcoinj - that’s piss poor behavior right there).

Welcome to the real world, where BitcoinJ is the new Mozilla/5.0.

mikehearn commented at 3:59 pm on August 25, 2015: contributor

@mikehearn So you wouldn’t have a problem with a Bitcoin Core option that disabled BIP37 responses entirely without making any attempt to signal it to its peers?

How did you arrive at that conclusion? Of course I would. I’m arguing it shouldn’t be an optional part of the protocol.

I guess it’s just a bit weird to have changes denied because someone did all this testing which shows everything is absolutely fine but can’t actually show their process.

Would you like me to send you the code privately? It uses blocks which were selected to be above 800kb, though in fact most of them are max sized (1mb).

I mean, I don’t know what more you want. I gave the data I gathered from the attack I implemented. I have suggested algorithms that would resolve further issues were they to arise, for example because they slowed down syncing of legit mobile phones.

You’re right that releasing the source probably wouldn’t be a big deal, given that the attack doesn’t really seem to work, but it can’t help anyone either.

If your BIP101 goes through then we will inevitably have 8x bigger blocks with which to perform the attack, is there enough headroom in the “not a problem” levels of denial of service possible?

As blocks get bigger, Bloom filtering will get slower for all users. RPCs and other things run in a different thread to network traffic, so I guess it wouldn’t make much difference there. They respond just fine even when the network thread is maxed out.

But the CPU profiles suggest that it should be possible to eliminate most of that 90% block loading time. Bloom filtering has scaled through two orders of magnitude since it was designed and launched, with tuning it can probably do another order of magnitude. That’d take us to 10mb blocks. 10kb to 10mb is not bad!

But at some point the whole search-everything-to-find-what-you-want approach won’t work any more, assuming Bitcoin becomes more than ~20x as popular as it is today. Let’s hope so, eh? In anticipation of this wondrous event, I have described a more scalable alternative scheme on the bitcoinj list at the bottom of this email:

https://groups.google.com/forum/#!searchin/bitcoinj/bloom$20filtering/bitcoinj/Ys13qkTwcNg/9qxnhwnkeoIJ

There will be plenty of time to develop such things in future.

Now I’m trying to figure out why you want this again. From the bug you filed here:

#6578

you talked about miners and high reliability merchant nodes. We already beat this to death: disable listening and then you can avoid all kinds of issues. There have been plenty of DoS attacks that didn’t involve Bloom filtering so it makes sense to take this approach. If you need max reliability, you should not be allowing random potential attackers to connect to your node.

Welcome to the real world, where BitcoinJ is the new Mozilla/5.0.

Wrong. Nothing changes behaviour based on the subver field. Peter did it just to make people angry at me instead of him, it has no other purpose. It’s exactly what I’ve come to expect from a guy like that.

evGUzQIEQaL4 commented at 4:34 pm on August 25, 2015: none

How did you arrive at that conclusion? Of course I would. I’m arguing it shouldn’t be an optional part of the protocol.

So far your arguments have been complaining about protocol modification time, I don’t see why you think all nodes should be serving BIP37. They already don’t, and there’s no requirement for inter full node communication to ever need it.

Now I’m trying to figure out why you want this again. From the bug you filed [ ] you talked about miners and high reliability merchant nodes. We already beat this to death: disable listening and then you can avoid all kinds of issues. There have been plenty of DoS attacks that didn’t involve Bloom filtering so it makes sense to take this approach. If you need max reliability, you should not be allowing random potential attackers to connect to your node.

BIP37 commands appear to be responded to even for outgoing connections so not listening is hardly a bulletproof defense, there’s no strong guarantee your client won’t connect to at least one sybil attacking node.

As far as I’m aware the only other denial of service attacks are going to be things like socket exhaustion, which don’t disrupt my node at all. Most of all I want to lower my attack surface, and disabling a very intensive feature with known risk and no peering improvements for me is a no-brainer. It might not make me impervious, but it reduces risk.

Wrong. Nothing changes behaviour based on the subver field. Peter did it just to make people angry at me instead of him, it has no other purpose. It’s exactly what I’ve come to expect from a guy like that.

That’s quite a jump to make, I clone the Satoshi subversion for all my evil clients because otherwise I stick out in peoples access logs. What else are you going to call it, /MyFirstEvilClient:0.1/?

But at some point the whole search-everything-to-find-what-you-want approach won’t work any more, assuming Bitcoin becomes more than ~20x as popular as it is today. Let’s hope so, eh? In anticipation of this wondrous event, I have described a more scalable alternative scheme on the bitcoinj list at the bottom of this email:

It’s not the subject of this pull request, but those assumptions are utterly ridiculous. People don’t log bloom filter requests because of wiretapping laws? There’s academic papers which are built on the foundation of collected information that was leaked by BIP37! Fixing address reuse provides almost no perceivable privacy in bitcoin due to change addresses leaking incredible amounts of user information. BIP37 leaks are only useful to GCHQ or NSA!? Unbelievable.

mikehearn commented at 4:45 pm on August 25, 2015: contributor

I’m starting to feel this debate will go nowhere.

So far your arguments have been complaining about protocol modification time

I already said in my comment above, that I don’t think this is a good idea for a whole bunch of reasons, one of which is that supporting it would take a big pile of time away from more important things. But that’s not the only reason.

As far as I’m aware the only other denial of service attacks are going to be things like socket exhaustion

Then you would be wrong.

I have been on the bitcoin-security list for many years. DoS attacks are found routinely. Some of them have been able to e.g. ensure you never download a transaction.

Assuming that the only DoS attacks that have been or ever will be are BIP 37 + connection exhaustion related is a very bad assumption indeed. I think every release of Core has had DoS fixes in them.

BIP37 commands appear to be responded to even for outgoing connections so not listening is hardly a bulletproof defense

It’s not bulletproof but it raises the difficulty for an attacker substantially. If you find you got super unlucky and ended up connecting to a malicious node, OK, ban it and you won’t connect to them again. Fixed.

The BF protocol might turn out to be useful for shrinking relay bandwidth in future, as a quickly deployable option. Full-match filters are already short-circuited in the code. So that’s why they aren’t ignored for outbound connections.

I clone the Satoshi subversion for all my evil clients because otherwise I stick out in peoples access logs. What else are you going to call it, /MyFirstEvilClient:0.1/?

Um, yes. If your goal is to be a “security researcher” then obviously your attack programs are only meant to demonstrate an issue, not actually be designed for real attackers :(

It’s not the subject of this pull request, but those assumptions are utterly ridiculous.

We’re talking about scalability, not privacy. But yeah, I’ve yet to find anyone other than academic researchers who actually care about doing analysis on Bloom filters, though it’s a safe bet intelligence agencies do too. Beyond that, the data appears to be fairly useless (read: no profit in it).

But I don’t want to have that debate again right now. You were supposed to read the last section. I guess you have no thoughts on better scaling protocols.

evGUzQIEQaL4 commented at 5:00 pm on August 25, 2015: none

I’ve yet to find anyone other than academic researchers who actually care about doing analysis on Bloom filters,

“Nobody cares” is a pretty poor privacy defense, at the very minimum captured bloom filter leaks would be an extremely potent tool for blackmail.

I guess you have no thoughts on better scaling protocols.

“Scaling” at the detriment of user privacy and node stability is insane. Rather than propose solutions which do this, it is also completely acceptable to say that you know what, this doesn’t scale at all and there’s no solutions I can come up with. If the only option is to run through a mine field, you don’t present it as a sensible option. Many users are very focused on privacy, I’m not sure they would be as quick to rush to BIP37 if they really knew how leaky the system is.

I have been on the bitcoin-security list for many years. DoS attacks are found routinely. Some of them have been able to e.g. ensure you never download a transaction.

I know, the contents of it was leaked a while back, would you like a copy? Some of the responses to issues are fairly discouraging against people bothering to report them in the future. Refusing to let nodes make their own choice to disable code which is a known DoS risk (2 years is hardly a secret) is a completely different matter than responsibly disclosed bugs, for which there’s no feasible way for people to disable.

dexX7 commented at 5:01 pm on August 25, 2015: contributor

It seems useful to signal the supported services, and since NODE_BLOOM appears to be (willingly or unwillingly) not available at any time, I can see how this service bit provides value. The discussion whether it can optionally be disabled seems pretty unrelated though.

I’m wondering, how it plays together with block pruning, and I’d assume if pruning is enabled, then NODE_BLOOM should be disabled, similar to NODE_NETWORK? edit: thanks for the clarification.

evGUzQIEQaL4 commented at 5:04 pm on August 25, 2015: none

As far as I know there’s no bad interaction between a pruned node and NODE_BLOOM, it could still serve BIP37 filters if it chose to and has the blocks available. New BIP37 SPV wallets won’t attempt to download very old blocks so it’s likely that they will be able to completely sync from even a heavily pruned node.

sipa commented at 5:06 pm on August 25, 2015: member

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 commented at 5:20 pm on August 25, 2015: contributor

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

TheBlueMatt commented at 5:23 pm on August 25, 2015: member

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)

in src/rpcmisc.cpp: in 1fcc433a65 outdated

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

laanwj commented at 4:35 pm on September 4, 2015:

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

petertodd commented at 6:03 pm on September 4, 2015:

Lol, how did I miss that? I added localservices in 99ddc6cb706e825e54da244a10d4d6389fc5eaae @laanwj Thanks for catching that!

laanwj commented at 4:36 pm on September 4, 2015: member

ACK, apart from nit about getinfo.

TheBlueMatt force-pushed on Sep 6, 2015

Add NODE_BLOOM service bit and bump protocol version

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

afb0ccaf9c

TheBlueMatt force-pushed on Sep 6, 2015

TheBlueMatt commented at 6:29 am on September 6, 2015: member

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

dcousens commented at 2:26 pm on September 6, 2015: contributor

utACK (again)

petertodd commented at 5:13 pm on September 7, 2015: contributor

ACK (again)

laanwj merged this on Sep 8, 2015

laanwj closed this on Sep 8, 2015

laanwj referenced this in commit 0c27795140 on Sep 8, 2015

luke-jr referenced this in commit 11299bf11c on Dec 28, 2015

luke-jr referenced this in commit bfa763aea0 on Dec 28, 2015

in src/main.cpp: in afb0ccaf9c

4597+               strCommand == "filterclear") &&
4598+              //TODO: Remove this line after reasonable network upgrade
4599+              pfrom->nVersion >= NO_BLOOM_VERSION)
4600+    {
4601+        if (pfrom->nVersion >= NO_BLOOM_VERSION)
4602+            Misbehaving(pfrom->GetId(), 100);

rebroad commented at 6:53 am on August 25, 2016:

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 commented at 6:57 am on August 25, 2016: member

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.

in src/version.h: in afb0ccaf9c

 8@@ -9,7 +9,7 @@
 9  * network protocol versioning
10  */
11 
12-static const int PROTOCOL_VERSION = 70002;
13+static const int PROTOCOL_VERSION = 70011;

rebroad commented at 6:49 pm on September 12, 2016:

Why the jump from 70002 to 70011?

in src/main.cpp: in afb0ccaf9c

4590@@ -4591,6 +4591,21 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
4591     }
4592 
4593 
4594+    else if (!(nLocalServices & NODE_BLOOM) &&
4595+              (strCommand == "filterload" ||
4596+               strCommand == "filteradd" ||
4597+               strCommand == "filterclear") &&

rebroad commented at 2:01 am on September 13, 2016:

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

zkbot referenced this in commit f6d09aa822 on Apr 5, 2018

DrahtBot locked this on Aug 16, 2022

Add NODE_BLOOM service bit and bump protocol version #6579