It seems this conflicts with #6641

@MarcoFalke they have the same goal, this one is gated by a cli flag and such can be enabled today

So we get rid of -enforcenodebloom when the other one gets merged? As this is a non documented expert feature, I guess the expert could just cherry-pick #6641 to get the same result today?

@MarcoFalke the other one doesn't get merged, we simply change the default here from false to true

yes you can cherry pick today; infact i do it all the time, but it's hugely annoying

Concept ACK.
Needs help message in init.cpp

@jonasschnelli done

Thanks @pstratem . utACK.

@MarcoFalke the other one doesn't get merged, we simply change the default here from false to true

I think the idea is that at that point (in the future) the temporary -enforcenodebloom option will go away and it will always be enforced, converging to a similar situation as #6641.

utACK

Note that this CLI option is useful for SPV wallet authors who need to test that their NODE_BLOOM support actually works properly in all cases.

I suppose, utACK, given @petertodd's logic.

This also seems like a useful flag for mining pools to use to prevent resource usage from SPV clients.

@gmaxwell admittedly my primary motivation is that i often find myself merging #6641 and it's getting annoying

Want to see some tested ACKs here but I definitely like this to be in 0.12.

concept ACK / utACK

Gave it a testshot together with a modificated version of Breadwallet iOS.
The FILTER: debug output was manually added.
Works, ... but Breadwallet likes to reconnect the whole time (multiple times per second).
Would banning the node for 1-2 minutes make sense?

2015-11-25 09:14:52 receive version message: /breadwallet:0.5.3/: version 70002, blocks=0, us=176.9.45.239:8333, peer=4
2015-11-25 09:14:52 ->FILTER: filter command detected
2015-11-25 09:14:52 FILTER: -enforcenodebloom, disconnecting node
2015-11-25 09:14:52 ProcessMessages(filterload, 823 bytes) FAILED peer=4
2015-11-25 09:14:52 receive version message: /breadwallet:0.5.3/: version 70002, blocks=0, us=176.9.45.239:8333, peer=5
2015-11-25 09:14:52 ->FILTER: filter command detected
2015-11-25 09:14:52 FILTER: -enforcenodebloom, disconnecting node
2015-11-25 09:14:52 ProcessMessages(filterload, 823 bytes) FAILED peer=5
2015-11-25 09:14:52 receive version message: /breadwallet:0.5.3/: version 70002, blocks=0, us=176.9.45.239:8333, peer=6
2015-11-25 09:14:52 ->FILTER: filter command detected
2015-11-25 09:14:52 FILTER: -enforcenodebloom, disconnecting node
2015-11-25 09:14:52 ProcessMessages(filterload, 823 bytes) FAILED peer=6
2015-11-25 09:14:52 tor: Disconnected from Tor control port 127.0.0.1:9051, trying to reconnect
2015-11-25 09:14:52 receive version message: /breadwallet:0.5.3/: version 70002, blocks=0, us=176.9.45.239:8333, peer=7
2015-11-25 09:14:53 ->FILTER: filter command detected
2015-11-25 09:14:53 FILTER: -enforcenodebloom, disconnecting node
2015-11-25 09:14:53 ProcessMessages(filterload, 823 bytes) FAILED peer=7
2015-11-25 09:14:53 receive version message: /breadwallet:0.5.3/: version 70002, blocks=0, us=176.9.45.239:8333, peer=8
2015-11-25 09:14:53 ->FILTER: filter command detected
2015-11-25 09:14:53 FILTER: -enforcenodebloom, disconnecting node
2015-11-25 09:14:53 ProcessMessages(filterload, 823 bytes) FAILED peer=8
2015-11-25 09:14:53 receive version message: /breadwallet:0.5.3/: version 70002, blocks=0, us=176.9.45.239:8333, peer=9
2015-11-25 09:14:53 ->FILTER: filter command detected
2015-11-25 09:14:53 FILTER: -enforcenodebloom, disconnecting node
2015-11-25 09:14:53 ProcessMessages(filterload, 823 bytes) FAILED peer=9
2015-11-25 09:14:53 receive version message: /breadwallet:0.5.3/: version 70002, blocks=0, us=176.9.45.239:8333, peer=10
2015-11-25 09:14:53 ->FILTER: filter command detected
2015-11-25 09:14:53 FILTER: -enforcenodebloom, disconnecting node
2015-11-25 09:14:53 ProcessMessages(filterload, 823 bytes) FAILED peer=10
2015-11-25 09:14:53 receive version message: /breadwallet:0.5.3/: version 70002, blocks=0, us=176.9.45.239:8333, peer=11
2015-11-25 09:14:53 ->FILTER: filter command detected
2015-11-25 09:14:53 FILTER: -enforcenodebloom, disconnecting node
2015-11-25 09:14:53 ProcessMessages(filterload, 823 bytes) FAILED peer=11
2015-11-25 09:14:53 receive version message: /breadwallet:0.5.3/: version 70002, blocks=0, us=176.9.45.239:8333, peer=12
2015-11-25 09:14:54 ->FILTER: filter command detected
2015-11-25 09:14:54 FILTER: -enforcenodebloom, disconnecting node
2015-11-25 09:14:54 ProcessMessages(filterload, 823 bytes) FAILED peer=12
2015-11-25 09:14:54 receive version message: /breadwallet:0.5.3/: version 70002, blocks=0, us=176.9.45.239:8333, peer=13
2015-11-25 09:14:54 ->FILTER: filter command detected

@jonasschnelli :/

@jonasschnelli Isn't that, somewhat, the behavior you expect if you explicitly pointed that wallet at the node? In normal use, I'd hope it will cycle through its list and choose another node to connect to?

@laanwj: Right. The behavior is expected (for a SPV wallet with one designated node). I think a bare fDisconnect=true (without a ban for serval minutes or an increase of `Misbehaving()´ of the disconnected node) can result in endless disconnect/connect loops.

Would a tiny increase of misbehaving points (Misbehaving(<tbd>)) in this case be appropriate so that the node gets banned after serval reconnect loops?

@jonasschnelli Misbehaving doesn't work over reconnects (it can't).

@laanwj: arr... right. Only the ban does.

Really not a big fan of this for 0.12. We shouldn't be deploying this in the same version as NODE_BLOOM. I know many people who will be running around telling everyone to run with this option, but it is way too early to do that. If suddenly a large chunk of the network starts using this option many clients that haven't had a chance to upgrade yet will suddenly break, which I think is an absolutely terrible outcome. Further, this absolutely doesn't help with testing anything... It's pretty disingenuous to claim that anyone wants this to assist in testing anything since the future behavior can be enabled by using the latest version number.

It's not like the various bloom-related DoS issues aren't thoroughly discussed in public already anyway, but those can be fixed directly (ie with the mempool-call-limits Greg was suggesting, maybe in conjunction with a similar limit on block requests).

On November 24, 2015 12:14:42 AM PST, MarcoFalke notifications@github.com wrote:

It seems this conflicts with #6641

---

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

@TheBlueMatt I don't see people all of a sudden trying to turn this off unless they run a high-availability service like a mining pool(where it would be useful on). I think having the flag is a better option than having people run patch sets on core(since they wouldn't be as well tested).

@petertodd Hmm, ok, good to hear that bitcoinj doesnt do tight reconnect loop (not sure who told me it did :( ). Still, that does not mean this will not break things without giving them almost any time to get an upgrade deployed (bitcoinj isnt the only SPV implementation anymore...has anyone done a census of how each behaves?). Further, no one has suggested serious justification for doing this now instead of waiting in this PR (aside from "I dont want to have to merge this patch regularly myself), so I'm kinda confused as to why people are pushing hard to do this.

@TheBlueMatt It's just a obscure command line option - heck we added an option to not relay transactions, a much more problematic flag if used widely. I think part of why people are pushing hard to just merge it precisely because people are pushing hard not too. :)

Anyway, a circumstance where a lot of people are enabling this option suddenly is probably a circumstance where you want it available, e.g. an active bloom-related attack. And like I say, we can fix the problem it might pose for SPV clients very quickly by changing DNS seed behavior.

@TheBlueMatt Speaking of DNS seeds, here's a patch to sipa's seed to only return bloom-supporting peers: sipa/bitcoin-seeder#34

Indeed, but not realying transaction is not likely to be deployed widely because there is not a fear of DoS attacks. My point is not that we shouldn't have it available (I started the whole recent NODE_BLOOM push because we should eventually have it available in case of emergencies) but that I'm afraid people will start deploying it while it still breaks a ton of clients. The fact that half the people commenting on this PR are running a similar patch on their own nodes is sufficient evidence to me that a bunch of people will enable this option even when they aren't facing active DoS issues.

In any case, yes, only returning NODE_BLOOM nodes from the DNS seeds is an option, though I dont particularly want to do that since they arent only used for bloom-based SPV clients. (and, for the record, I use the code at https://github.com/TheBlueMatt/dnsseed-bitcoinj, though I think I have forgotten to push some changes from a few months ago, for my seed to keep some diversity to seeds).

The fact that half the people commenting on this PR are running a similar patch on their own nodes is sufficient evidence to me that a bunch of people will enable this option even when they aren't facing active DoS issues.

I think everyone here wanting to use this has a valid use. In any case most people run default settings and often don't even bother to upgrade their nodes(majority of nodes is still 0.11.0) so I don't think it's the case where people are actually tuning settings like this unnecessarily. Is a disconnection even noticeable for the SPV user in practice?

@jameshilliard I'm not at all convinced. I absolutely know that @pstratem is running it on nodes as a preventative measure (or to cut down CPU load on machines where it doesnt really matter what the CPU load is).

As for disconnection, if a client is spending all of its time reconnecting, I've definitely seen bugs where it would stall chain sync.

As for disconnection, if a client is spending all of its time reconnecting, I've definitely seen bugs where it would stall chain sync.

So SPV wallets are interpreting a disconnect as just a network issue and just keep retrying...that sounds like a pretty bad DoS vulnerability on their side? What happens if we just completely ban the SPV client?

@TheBlueMatt bitcoinj has 5 different mainnet DNS seeds, and all but seed returns about 25 addresses. Every address returned is potentially used, and ones that don't work are skipped, so you'd need all 100 or so potential peers to have turned this option on to be in a situation where you couldn't connect. (five if you're behind a DNS server that returns single results) That's just not very likely to happen even if a significant number of users manually turn bloom filters off, and like I say above, it's very easy and fast to have at least some of the DNS seeds return only bloom-enabled peers if we need too.

I really don't think adding this default-off flag is likely to cause problems. Much more likely is we'd find ourselves in a position where we need to tell users to turn bloom filters off due to an attack, and that's a much more serious problem than inconveniencing SPV users briefly.

Re: other things using DNS seeds, like what? Bitcoin Core uses them, but only when we know of no other addresses - getting just bloom-supporting peers does no harm as we'll soon find other peers.

Are there not other spv clients than Bitcoin? That doesn't seem like the case?

My concern is, do you really know how many of the various nodes on the network are run by miners (and would likely switch to using this flag rather quickly)? My impression is it's actually a ton, given that the approach to relay most miners seem to still use is to spin up a ton of geographically distributed nodes and peer them all with each other.

On November 25, 2015 6:30:35 PM PST, Peter Todd notifications@github.com wrote:

@TheBlueMatt bitcoinj has 5 different mainnet DNS seeds, and all but
seed returns about 25 addresses. Every address returned is potentially
used, and ones that don't work are skipped, so you'd need all 100 or so
potential peers to have turned this option on to be in a situation
where you couldn't connect. (five if you're behind a DNS server that
returns single results) That's just not very likely to happen even if a
significant number of users manually turn bloom filters off, and like I
say above, it's very easy and fast to have at least some of the DNS
seeds return only bloom-enabled peers if we need too.

I really don't think adding this default-off flag is likely to cause
problems. Much more likely is we'd find ourselves in a position where
we need to tell users to turn bloom filters off due to an attack, and
that's a much more serious problem than inconveniencing SPV users
briefly.

Re: other things using DNS seeds, like what? Bitcoin Core uses them,
but only when we know of no other addresses - getting just
bloom-supporting peers does no harm as we'll soon find other peers.

---

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

@TheBlueMatt I'm far less worried about SPV clients being interrupted than I am a massive DoS attack using bloom filters.

Yes, and we should fix such DoS vectors without disturbing legitimate clients if possible. We can merge the mempool-call-limits pull and limit filtered block requests (which we already do to one-per-process-each-node's-pending-messages so it can't completely starve other connections).

On November 25, 2015 6:59:10 PM PST, Patrick Strateman notifications@github.com wrote:

@TheBlueMatt I'm far less worried about SPV clients being interrupted
than I am a massive DoS attack using bloom filters.

---

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

@TheBlueMatt The only SPV implementations I know with significant usage are bitcoinj and the library breadwallet uses.

Anyway, lets do the math on this, assuming the worst case where your local DNS server returns one peer each. What's the probability of getting at least two bloom supporting peers, out of 5 seeds? This is a binomial problem and we want the CDF where X <= 1, which can be calculated with SciPy:

1-scipy.stats.binom.cdf(1, 5, p)

90% success happens so long as >59% of all nodes support bloom filters, with 98.5% success at 75% support. For at least one bloom supporting peer, just 60% of nodes supporting bloom gives a 99% success rate.

That isn't the only concern. If your implementation gets into a reconnect loop and doesn't kill itself after some time, you'll blow through radio uptime/battery life pretty significantly.

On November 25, 2015 7:33:48 PM PST, Peter Todd notifications@github.com wrote:

@TheBlueMatt The only SPV implementations I know with significant usage
are bitcoinj and the library breadwallet uses.

Anyway, lets do the math on this, assuming the worst case where your
local DNS server returns one peer each. What's the probability of
getting at least two bloom supporting peers, out of 5 seeds? This is a
binomial problem and we want the CDF where X <= 1, which can be
calculated with SciPy:

1-scipy.stats.binom.cdf(1, 5, p)

90% success happens so long as >59% of all nodes support bloom filters,
with 98.5% success at 75% support. For at least one bloom supporting
peer, just 60% of nodes supporting bloom gives a 99% success rate.

---

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

@TheBlueMatt Bitcoinj has exponential backoff bottoming out at an attempt every 10 minutes; why would it get into a reconnect loop?

Even if that could happen, bloom filters are far from the only way that could happen.

Yes, you've clearly stated that Bitcoinj likely won't be effected unless all of your peers have this enabled. However, another comment already indicated breadwallet seems to get into a reconnect loop.

On November 25, 2015 7:42:10 PM PST, Peter Todd notifications@github.com wrote:

@TheBlueMatt Bitcoinj has exponential backoff bottoming out at an
attempt every 10 minutes; why would it get into a reconnect loop?

Even if that could happen, bloom filters are far from the only way that
could happen.

---

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

@TheBlueMatt only because only 1 node was specified by @jonasschnelli for connectivity.
It should probably back off however, ping @voisine?

@TheBlueMatt Yeah, I just checked, non-manually-added peers aren't tried again after a disconnect. Also, unlike bitcoinj, Breadwallet processes addr messages and keeps a persistent set of known addresses across reboots.

@TheBlueMatt I really do not believe there is a practical issue here.

Yes, you've clearly stated that Bitcoinj likely won't be effected unless all of your peers have this enabled. However, another comment already indicated breadwallet seems to get into a reconnect loop.

That was cleared up a post later, when it was revealed that he was explicitly telling the software to connect to his node. Retrying is the expected behavior (what if the node is 'fixed' in the meantime?). Sure, exponential back-off would be nice, but it's beside the point.

I'll be frank about the reason I want this in: to be able to tell people to use this option when bloom filter is used in a DoS attack on the network or on them personally. This has been the case, and having them install a patch is impractical (and for promises about mitigation using request limits and such it's too late for 0.12 - feature freeze is in less than a week).

Apart from that it is an obscure, unadvertised, undocumented option, and I don't expect anyone besides a few 'obsessive performance tweakers' to actually enable it.

As far as the concern that miners may turn this on and that maybe their nodes are a substantial fraction of the network: In 0.12 they can set the upload limiter, which will disconnect lite wallets most of the time just as much as this will.

If it becomes the case that people dig up the hidden open and are turning this on and causing problems we can respond by starting up more nodes and setting dnsseeds to filter results; it would be a pain but no great harm; and I think a pretty reasonable risk all considered. Consider instead: what happens if bloom based DOS attacks become much more common? The fix there will be for everyone to upgrade their software. Thats much more of a pain.

FWIW, I've been testing the bandwidth limiting cranked all the way down at home since that patch was first written. I've seen it hang up on zillions of lite wallets and none have gone into a reconnect loop.

I've seen it hang up on zillions of lite wallets and none have gone into a reconnect loop.

Thinking of it, it would be extremely unfriendly behavior to reconnect to a random peer that disconnects you. I've done experiments where I had bitcoind in a restart loop every few minutes, then compared which nodes were reconnecting, I'm fairly sure I've seen that behavior only with spy nodes.

@dcousens @voisine

only because only 1 node was specified by @jonasschnelli for connectivity.

Maybe my manual change (to only connect to one peer) made Breadwallet end up in a reconnect loop. Not sure about it. Anyways: if its an issue, it needs to be solved on the SPV client side.

Just found this discussion and thought I'd chime in. Breadwallet will choose a new random node for each reconnect attempt, and after 20 failed attempts in a row it will stop and display "not connected" in the UI, and wait for the user to manually initiate another 20 reconnect attempts to random nodes. It also keeps track of nodes that don't provide expected responses and marks them as misbehaving (not for network connect issues though). I'm also happy to take suggestions for improvements.