Skip to content

Conversation

pstratem
Copy link
Contributor

Disconnect nodes which request bloom filtering in violation of the protocol.

@jonasschnelli
Copy link
Contributor

See also #6579 for discussions.

utACK.

@@ -4377,7 +4377,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
if (pfrom->nVersion >= NO_BLOOM_VERSION) {
Misbehaving(pfrom->GetId(), 100);
return false;
} else if (GetBoolArg("-enforcenodebloom", DEFAULT_ENFORCENODEBLOOM)) {
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEFAULT_ENFORCENODEBLOOM constant in main.h needs to go too :)

@gmaxwell
Copy link
Contributor

Has someone checked what impact this would have now? (Wouldn't preclude a merge in master, since there is still a fair amount of time to respond before release-- but it would be good to know).

@laanwj
Copy link
Member

laanwj commented Mar 18, 2016

@gmaxwell Very little, I expect. the intersection of nodes that::

  • Will immediately upgrade to code that includes this
  • Set -peerbloomfilters false (it defaults to true, and will continue to do so)
  • Are not yet using -enforcenodebloom on 0.12

Is likely to be very, very small.

This is just continuing the planning, in 0.12 NODE_BLOOM was introduced but not yet enforced (by default) for old versions, and the plan is that in 0.13 it will be enforced for all versions.

What kind of measurement do you think should change that planning?

In no case this is going to be backported to 0.12.

@jonasschnelli
Copy link
Contributor

Added a counter to my SPV stats node to see how many SPV nodes have not adapted the new rule.

@gmaxwell
Copy link
Contributor

Ah. Indeed, I was forgetting that with peerbloomfilters defaulting to true this would have no effect. Still, interesting to know if there has been any progress or not.

@pstratem pstratem force-pushed the 2016-03-17-nodebloom branch from d2aee0b to c90036f Compare March 19, 2016 04:25
@pstratem
Copy link
Contributor Author

@laanwj nit picked

@laanwj
Copy link
Member

laanwj commented Mar 21, 2016

ACK c90036f

@laanwj laanwj merged commit c90036f into bitcoin:master Mar 21, 2016
laanwj added a commit that referenced this pull request Mar 21, 2016
c90036f Always disconnect old nodes which request filtered connections. (Patrick Strateman)
@rebroad
Copy link
Contributor

rebroad commented Aug 25, 2016

@pstratem What is the impact of not disconnecting these nodes?

@jonasschnelli
Copy link
Contributor

@rebroad: check https://github.com/bitcoin/bips/blob/master/bip-0111.mediawiki.

Not all nodes want to provide the BLOOM FILTER service. SPV bloom filtering will consume lots of CPU and disk usage. The produced and consumed data will very likely be worthless for the p2p health after generating/transmitting (where a BLOCK or TX structure will be relay further by other full nodes to other full nodes).

BIP-0111 will help SPV clients to detect, which peers do and don't support the NODE_BLOOM service. Otherwise they need to try and disconnect in case they won't.

@sipa
Copy link
Member

sipa commented Aug 25, 2016 via email

@rebroad
Copy link
Contributor

rebroad commented Aug 25, 2016

@jonasnick @sipa thanks for explanaing

codablock pushed a commit to codablock/dash that referenced this pull request Dec 19, 2017
c90036f Always disconnect old nodes which request filtered connections. (Patrick Strateman)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants