New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

De-neuter NODE_BLOOM #7708

Merged
merged 1 commit into from Mar 21, 2016

Conversation

Projects
None yet
6 participants
@pstratem
Contributor

pstratem commented Mar 18, 2016

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

@pstratem pstratem referenced this pull request Mar 18, 2016

Closed

De-neuter NODE_BLOOM #6641

@jonasschnelli jonasschnelli added the P2P label Mar 18, 2016

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 18, 2016

Member

See also #6579 for discussions.

utACK.

Member

jonasschnelli commented Mar 18, 2016

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 {

This comment has been minimized.

@laanwj

laanwj Mar 18, 2016

Member

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

@laanwj

laanwj Mar 18, 2016

Member

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

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Mar 18, 2016

Member

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

Member

gmaxwell commented Mar 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 18, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 18, 2016

Member

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

Member

jonasschnelli commented Mar 18, 2016

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

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Mar 18, 2016

Member

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.

Member

gmaxwell commented Mar 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Mar 19, 2016

Contributor

@laanwj nit picked

Contributor

pstratem commented Mar 19, 2016

@laanwj nit picked

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj
Member

laanwj commented Mar 21, 2016

ACK c90036f

@laanwj laanwj merged commit c90036f into bitcoin:master Mar 21, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Mar 21, 2016

Merge #7708: De-neuter NODE_BLOOM
c90036f Always disconnect old nodes which request filtered connections. (Patrick Strateman)
@rebroad

This comment has been minimized.

Show comment
Hide comment
@rebroad

rebroad Aug 25, 2016

Contributor

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

Contributor

rebroad commented Aug 25, 2016

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Aug 25, 2016

Member

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

Member

jonasschnelli commented Aug 25, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 25, 2016

Member
Member

sipa commented Aug 25, 2016

@rebroad

This comment has been minimized.

Show comment
Hide comment
@rebroad

rebroad Aug 25, 2016

Contributor

@jonasnick @sipa thanks for explanaing

Contributor

rebroad commented Aug 25, 2016

@jonasnick @sipa thanks for explanaing

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7708: De-neuter NODE_BLOOM
c90036f Always disconnect old nodes which request filtered connections. (Patrick Strateman)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7708: De-neuter NODE_BLOOM
c90036f Always disconnect old nodes which request filtered connections. (Patrick Strateman)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge #7708: De-neuter NODE_BLOOM
c90036f Always disconnect old nodes which request filtered connections. (Patrick Strateman)

codablock added a commit to codablock/dash that referenced this pull request Dec 19, 2017

Merge #7708: De-neuter NODE_BLOOM
c90036f Always disconnect old nodes which request filtered connections. (Patrick Strateman)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment