[Net]Add -enforcenodebloom option #7087

Merged
merged 3 commits into from Nov 26, 2015

Conversation

Projects
None yet
10 participants
@pstratem
Contributor

pstratem commented Nov 24, 2015

No description provided.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 24, 2015

Member

It seems this conflicts with #6641

Member

MarcoFalke commented Nov 24, 2015

It seems this conflicts with #6641

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Nov 24, 2015

Contributor

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

Contributor

pstratem commented Nov 24, 2015

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

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 24, 2015

Member

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?

Member

MarcoFalke commented Nov 24, 2015

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?

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Nov 24, 2015

Contributor

@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

Contributor

pstratem commented Nov 24, 2015

@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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 24, 2015

Member

Concept ACK.
Needs help message in init.cpp

Member

jonasschnelli commented Nov 24, 2015

Concept ACK.
Needs help message in init.cpp

@pstratem

This comment has been minimized.

Show comment
Hide comment
Contributor

pstratem commented Nov 24, 2015

@laanwj

View changes

src/main.cpp
@@ -3988,8 +3988,17 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
return true;
}
-
-
+ if ((strCommand == "filterload" ||

This comment has been minimized.

@laanwj

laanwj Nov 24, 2015

Member

Why is this block of code moved?

@laanwj

laanwj Nov 24, 2015

Member

Why is this block of code moved?

This comment has been minimized.

@pstratem

pstratem Nov 24, 2015

Contributor

It simplifies the logic to move it.

@pstratem

pstratem Nov 24, 2015

Contributor

It simplifies the logic to move it.

This comment has been minimized.

@laanwj

laanwj Nov 24, 2015

Member

How?

Please explain things like this in your commit messages. Why you did it, why it is correct, and so on.
Say - this causes a bug at some point in the future, developers need to be able to know why you did it. With an empty commit message (apart from the title) that's hard to figure out.

@laanwj

laanwj Nov 24, 2015

Member

How?

Please explain things like this in your commit messages. Why you did it, why it is correct, and so on.
Say - this causes a bug at some point in the future, developers need to be able to know why you did it. With an empty commit message (apart from the title) that's hard to figure out.

@laanwj laanwj added the P2P label Nov 24, 2015

Move bloom filter filtering logic outside of command "switch" (giant …
…if/else).

Moving this logic outside of the "switch" makes it far simpler to
enable the forced disconnect by a parameter.
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 24, 2015

Member

Thanks @pstratem . utACK.

Member

laanwj commented Nov 24, 2015

Thanks @pstratem . utACK.

@laanwj

View changes

src/init.cpp
@@ -362,6 +362,9 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-onion=<ip:port>", strprintf(_("Use separate SOCKS5 proxy to reach peers via Tor hidden services (default: %s)"), "-proxy"));
strUsage += HelpMessageOpt("-onlynet=<net>", _("Only connect to nodes in network <net> (ipv4, ipv6 or onion)"));
strUsage += HelpMessageOpt("-permitbaremultisig", strprintf(_("Relay non-P2SH multisig (default: %u)"), 1));
+ strUsage += HelpMessageOpt("-peerbloomfilters", strprintf(_("Support filtering of blocks and transaction with bloom filters (default: %u)"), 1));
+ if (showDebug)
+ strUsage += HelpMessageOpt("-enforcenodebloom", strprintf(_("Enforce minimum protocol version to limit use of bloom filters (default: %u)"), 0));

This comment has been minimized.

@laanwj

laanwj Nov 24, 2015

Member

Nit: don't use translation _() for debug option messages.

@laanwj

laanwj Nov 24, 2015

Member

Nit: don't use translation _() for debug option messages.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 24, 2015

Member

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

Member

laanwj commented Nov 24, 2015

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

pstratem added some commits Nov 24, 2015

Add enforcenodebloom option.
Previously peers which implement a protocol version less than NO_BLOOM_VERSION
would not be disconnected for sending a filter command, regardless of the
peerbloomfilter option.

Many node operators do not wish to provide expensive bloom filtering for SPV
clients, previously they had to cherry pick the commit which enabled the
disconnect logic.

The default should remain false until a sufficient percent of SPV clients
have updated.

@pstratem pstratem changed the title from [Net][WIP]Add -enforcenodebloom option to [Net]Add -enforcenodebloom option Nov 24, 2015

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 24, 2015

Contributor

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.

Contributor

petertodd commented Nov 24, 2015

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.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 24, 2015

Member

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

Member

gmaxwell commented Nov 24, 2015

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

@jameshilliard

This comment has been minimized.

Show comment
Hide comment
@jameshilliard

jameshilliard Nov 24, 2015

Contributor

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

Contributor

jameshilliard commented Nov 24, 2015

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

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Nov 25, 2015

Contributor

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

Contributor

pstratem commented Nov 25, 2015

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 25, 2015

Member

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

Member

laanwj commented Nov 25, 2015

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

@laanwj laanwj added this to the 0.12.0 milestone Nov 25, 2015

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 25, 2015

Contributor

concept ACK / utACK

Contributor

dcousens commented Nov 25, 2015

concept ACK / utACK

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 25, 2015

Member

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
Member

jonasschnelli commented Nov 25, 2015

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

This comment has been minimized.

Show comment
Hide comment
Contributor

pstratem commented Nov 25, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 25, 2015

Member

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

Member

laanwj commented Nov 25, 2015

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 25, 2015

Member

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

Member

jonasschnelli commented Nov 25, 2015

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 25, 2015

Member

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

Member

laanwj commented Nov 25, 2015

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 25, 2015

Member

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

Member

jonasschnelli commented Nov 25, 2015

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

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 26, 2015

Contributor

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)

Contributor

TheBlueMatt commented Nov 26, 2015

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)

@jameshilliard

This comment has been minimized.

Show comment
Hide comment
@jameshilliard

jameshilliard Nov 26, 2015

Contributor

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

Contributor

jameshilliard commented Nov 26, 2015

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

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 26, 2015

Contributor

Agreed with @jameshilliard, easier than merging #6641 on my own node

Contributor

dcousens commented Nov 26, 2015

Agreed with @jameshilliard, easier than merging #6641 on my own node

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 26, 2015

Contributor

I can absolutely see a bunch of people here responding to any number of support queries with "disable the bloom filtering stuff" and end up getting connected to in a tight loop (I don't think breadwallet is the only one that does this). Certainly encouraging miners and exchanges to all start turning this on before any spv clients have even written code to handle it (have any? Im not aware of any) could potentially cause a lot of problems. And, indeed, many of these services aren't willing to carry many patches to Bitcoin core unless they have to, which should been you'd end up with way less broken crap.

On November 25, 2015 4:07:23 PM PST, jameshilliard notifications@github.com wrote:

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


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

Contributor

TheBlueMatt commented Nov 26, 2015

I can absolutely see a bunch of people here responding to any number of support queries with "disable the bloom filtering stuff" and end up getting connected to in a tight loop (I don't think breadwallet is the only one that does this). Certainly encouraging miners and exchanges to all start turning this on before any spv clients have even written code to handle it (have any? Im not aware of any) could potentially cause a lot of problems. And, indeed, many of these services aren't willing to carry many patches to Bitcoin core unless they have to, which should been you'd end up with way less broken crap.

On November 25, 2015 4:07:23 PM PST, jameshilliard notifications@github.com wrote:

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


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

@jameshilliard

This comment has been minimized.

Show comment
Hide comment
@jameshilliard

jameshilliard Nov 26, 2015

Contributor

@TheBlueMatt What's the best way to force disconnect SPV clients so that they don't keep trying to reconnect? I think f2pool bans any non-white-listed clients by subver but I'm not sure how different that disconnect implementation is. Another pool operator just iptables bans SPV clients when they start using resources. We will likely always have some non-upgraded SPV clients regardless and this would be useful for testing how best to disconnect them.

Contributor

jameshilliard commented Nov 26, 2015

@TheBlueMatt What's the best way to force disconnect SPV clients so that they don't keep trying to reconnect? I think f2pool bans any non-white-listed clients by subver but I'm not sure how different that disconnect implementation is. Another pool operator just iptables bans SPV clients when they start using resources. We will likely always have some non-upgraded SPV clients regardless and this would be useful for testing how best to disconnect them.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 26, 2015

Contributor

@jameshilliard no, we should just be disconnecting them, that is the correct behavior, but it isnt something we just try to do often. My point is more, if this patch is merged, people who are not experiencing issues with bloom filter-based load will start enabling it, potentially causing serious issues to virtually all deployed SPV clients. If I thought no one was going to go around telling people to use this option (it not be listed even with -debug, IMO) then I'd be mostly OK with it, but I'm rather confident that is not the case.

Contributor

TheBlueMatt commented Nov 26, 2015

@jameshilliard no, we should just be disconnecting them, that is the correct behavior, but it isnt something we just try to do often. My point is more, if this patch is merged, people who are not experiencing issues with bloom filter-based load will start enabling it, potentially causing serious issues to virtually all deployed SPV clients. If I thought no one was going to go around telling people to use this option (it not be listed even with -debug, IMO) then I'd be mostly OK with it, but I'm rather confident that is not the case.

@jameshilliard

This comment has been minimized.

Show comment
Hide comment
@jameshilliard

jameshilliard Nov 26, 2015

Contributor

@TheBlueMatt what real world problems would just disconnecting them cause, wouldn't they just try to connect to a different node?

Contributor

jameshilliard commented Nov 26, 2015

@TheBlueMatt what real world problems would just disconnecting them cause, wouldn't they just try to connect to a different node?

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 26, 2015

Contributor

@TheBlueMatt I tested SPV behavior ages ago when I originally implemented NODE_BLOOM, and the only time you get a tight reconnection loop is when the user has specifically asked the SPV wallet to be a peer. bitcoinj does exponential backoff for failed peers, so peers discovered via normal methods won't be reconnected too in a tight loop: https://github.com/bitcoinj/bitcoinj/blob/master/core/src/main/java/org/bitcoinj/core/PeerGroup.java#L214

Anyway, in the unlikely event that this became a problem it'd be quick and easy to modify the DNS seeds to only return nodes with the NODE_BLOOM bit set, a fix that doesn't need any changes to already deployed wallet software. (bitcoinj still only connects to peers provided by the seeds: https://github.com/bitcoinj/bitcoinj/blob/master/core/src/main/java/org/bitcoinj/core/Peer.java#L411)

Contributor

petertodd commented Nov 26, 2015

@TheBlueMatt I tested SPV behavior ages ago when I originally implemented NODE_BLOOM, and the only time you get a tight reconnection loop is when the user has specifically asked the SPV wallet to be a peer. bitcoinj does exponential backoff for failed peers, so peers discovered via normal methods won't be reconnected too in a tight loop: https://github.com/bitcoinj/bitcoinj/blob/master/core/src/main/java/org/bitcoinj/core/PeerGroup.java#L214

Anyway, in the unlikely event that this became a problem it'd be quick and easy to modify the DNS seeds to only return nodes with the NODE_BLOOM bit set, a fix that doesn't need any changes to already deployed wallet software. (bitcoinj still only connects to peers provided by the seeds: https://github.com/bitcoinj/bitcoinj/blob/master/core/src/main/java/org/bitcoinj/core/Peer.java#L411)

@jameshilliard

This comment has been minimized.

Show comment
Hide comment
@jameshilliard

jameshilliard Nov 26, 2015

Contributor

@petertodd can the DNS seeds be modified to take version number and the service bit into account and simply exclude any 0.12 clients that have NODE_BLOOM disabled? That way SPV clients can either connect to older nodes or 0.12 nodes that specifically allow bloom filters.

Contributor

jameshilliard commented Nov 26, 2015

@petertodd can the DNS seeds be modified to take version number and the service bit into account and simply exclude any 0.12 clients that have NODE_BLOOM disabled? That way SPV clients can either connect to older nodes or 0.12 nodes that specifically allow bloom filters.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 26, 2015

Contributor

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

Contributor

TheBlueMatt commented Nov 26, 2015

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

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 26, 2015

Contributor

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

Contributor

petertodd commented Nov 26, 2015

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

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 26, 2015

Contributor

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

Contributor

petertodd commented Nov 26, 2015

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

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 26, 2015

Contributor

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

Contributor

TheBlueMatt commented Nov 26, 2015

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

@jameshilliard

This comment has been minimized.

Show comment
Hide comment
@jameshilliard

jameshilliard Nov 26, 2015

Contributor

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?

Contributor

jameshilliard commented Nov 26, 2015

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?

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 26, 2015

Contributor

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

Contributor

TheBlueMatt commented Nov 26, 2015

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

@jameshilliard

This comment has been minimized.

Show comment
Hide comment
@jameshilliard

jameshilliard Nov 26, 2015

Contributor

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?

Contributor

jameshilliard commented Nov 26, 2015

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?

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 26, 2015

Contributor

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

Contributor

petertodd commented Nov 26, 2015

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

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 26, 2015

Contributor

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)

Contributor

TheBlueMatt commented Nov 26, 2015

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)

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Nov 26, 2015

Contributor

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

Contributor

pstratem commented Nov 26, 2015

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

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 26, 2015

Contributor

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)

Contributor

TheBlueMatt commented Nov 26, 2015

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)

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 26, 2015

Contributor

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

Contributor

petertodd commented Nov 26, 2015

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

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 26, 2015

Contributor

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)

Contributor

TheBlueMatt commented Nov 26, 2015

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)

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 26, 2015

Contributor

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

Contributor

petertodd commented Nov 26, 2015

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

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 26, 2015

Contributor

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)

Contributor

TheBlueMatt commented Nov 26, 2015

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)

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 26, 2015

Contributor

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

Contributor

dcousens commented Nov 26, 2015

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

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 26, 2015

Contributor

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

Contributor

petertodd commented Nov 26, 2015

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

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Nov 26, 2015

Contributor

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

Contributor

pstratem commented Nov 26, 2015

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 26, 2015

Member

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.

Member

laanwj commented Nov 26, 2015

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.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 26, 2015

Member

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.

Member

gmaxwell commented Nov 26, 2015

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.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 26, 2015

Member

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.

Member

gmaxwell commented Nov 26, 2015

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.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 26, 2015

Member

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.

Member

laanwj commented Nov 26, 2015

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.

@laanwj laanwj merged commit 9cf6688 into bitcoin:master Nov 26, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 26, 2015

Merge pull request #7087
9cf6688 Document both the peerbloomfilters and enforcenodebloom options. (Patick Strateman)
0f4dc53 Add enforcenodebloom option. (Patick Strateman)
b3caa9b Move bloom filter filtering logic outside of command "switch" (giant if/else). (Patick Strateman)
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 26, 2015

Member

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

Member

jonasschnelli commented Nov 26, 2015

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

@voisine

This comment has been minimized.

Show comment
Hide comment
@voisine

voisine Dec 22, 2015

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.

voisine commented Dec 22, 2015

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.

@str4d str4d referenced this pull request in zcash/zcash Jul 14, 2017

Open

Bitcoin 0.12 P2P/Net PRs 1 #2534

@str4d str4d referenced this pull request in zcash/zcash Dec 19, 2017

Merged

Add NODE_BLOOM service bit #2814

zkbot added a commit to zcash/zcash that referenced this pull request Apr 5, 2018

Auto merge of #2814 - str4d:2074-node-bloom, r=str4d
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