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

Disable the mempool P2P command when bloom filters disabled #8078

Merged
merged 2 commits into from Jun 8, 2016

Conversation

Projects
None yet
8 participants
@petertodd
Contributor

petertodd commented May 20, 2016

Only useful to SPV peers, and attackers... like bloom is a DoS vector as far more data is sent than received.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 20, 2016

Member

utACK fd432d7bb487e410a5e83f375df018132f183871

Member

jonasschnelli commented May 20, 2016

utACK fd432d7bb487e410a5e83f375df018132f183871

@laanwj laanwj added the P2P label May 20, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 20, 2016

Member

utACK/concept ACK fd432d7

Member

laanwj commented May 20, 2016

utACK/concept ACK fd432d7

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 20, 2016

Member

This should also make getdata only use the relaypool to close off the mempool privacy loss.

Member

gmaxwell commented May 20, 2016

This should also make getdata only use the relaypool to close off the mempool privacy loss.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd May 20, 2016

Contributor

@gmaxwell Sounds reasonable, but I think out of scope of this pull-req.

Contributor

petertodd commented May 20, 2016

@gmaxwell Sounds reasonable, but I think out of scope of this pull-req.

Disable the mempool P2P command when bloom filters disabled
Only useful to SPV peers, and attackers... like bloom is a DoS vector as far
more data is sent than received.
@kazcw

This comment has been minimized.

Show comment
Hide comment
@kazcw

kazcw May 20, 2016

Contributor

Would it make sense also to trigger this when bloom filters are enabled, but pfrom has not set one?

Contributor

kazcw commented May 20, 2016

Would it make sense also to trigger this when bloom filters are enabled, but pfrom has not set one?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 20, 2016

Member

Simple test you might want to pull into this PR:
jonasschnelli@3d3602f

Member

jonasschnelli commented May 20, 2016

Simple test you might want to pull into this PR:
jonasschnelli@3d3602f

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd May 20, 2016

Contributor

@jonasschnelli Good idea, done.

Contributor

petertodd commented May 20, 2016

@jonasschnelli Good idea, done.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr May 20, 2016

Member

Also useful to bootstrap a mempool.

Member

luke-jr commented May 20, 2016

Also useful to bootstrap a mempool.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd May 20, 2016

Contributor

@luke-jr That's why it's still allowed for whitelisted nodes; in general it's a DoS attack and we need a better system. Also we discussed the bootstrap case and think getting all the mempool isn't actually all that useful, for things we expect to need it like block relaying optimizations - need a subset.

Contributor

petertodd commented May 20, 2016

@luke-jr That's why it's still allowed for whitelisted nodes; in general it's a DoS attack and we need a better system. Also we discussed the bootstrap case and think getting all the mempool isn't actually all that useful, for things we expect to need it like block relaying optimizations - need a subset.

class P2PMempoolTests(BitcoinTestFramework):
def setup_chain(self):
initialize_chain_clean(self.options.tmpdir, 2)

This comment has been minimized.

@MarcoFalke

MarcoFalke May 21, 2016

Member

Could you use __init__ for that, instead of overwriting setup_chain?

I.e. something like

    def __init__(self):
        super().__init__()
        self.num_nodes = 2
        self.setup_clean_chain = True # not needed (default should already be true)
@MarcoFalke

MarcoFalke May 21, 2016

Member

Could you use __init__ for that, instead of overwriting setup_chain?

I.e. something like

    def __init__(self):
        super().__init__()
        self.num_nodes = 2
        self.setup_clean_chain = True # not needed (default should already be true)
@arowser

This comment has been minimized.

Show comment
Hide comment
@arowser

arowser May 25, 2016

Contributor

Can one of the admins verify this patch?

Contributor

arowser commented May 25, 2016

Can one of the admins verify this patch?

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 3, 2016

Member

@petertodd @jonasschnelli Interested in fixing the two nits for the rpc test?

Member

MarcoFalke commented Jun 3, 2016

@petertodd @jonasschnelli Interested in fixing the two nits for the rpc test?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 7, 2016

Member

utACK beceac9

Member

laanwj commented Jun 7, 2016

utACK beceac9

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 8, 2016

Member

Going to merge this - the nits can be done later.

Member

laanwj commented Jun 8, 2016

Going to merge this - the nits can be done later.

@laanwj laanwj merged commit 3d3602f into bitcoin:master Jun 8, 2016

laanwj added a commit that referenced this pull request Jun 8, 2016

Merge #8078: Disable the mempool P2P command when bloom filters disabled
3d3602f Add RPC test for the p2p mempool command in conjunction with disabled bloomfilters (Jonas Schnelli)
beceac9 Disable the mempool P2P command when bloom filters disabled (Peter Todd)

@defuse defuse referenced this pull request Oct 20, 2016

Closed

Manually backport upstream's NODE_BLOOM bit #1574

0 of 1 task complete

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

Merge #8078: Disable the mempool P2P command when bloom filters disabled
3d3602f Add RPC test for the p2p mempool command in conjunction with disabled bloomfilters (Jonas Schnelli)
beceac9 Disable the mempool P2P command when bloom filters disabled (Peter Todd)

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

Merge #8078: Disable the mempool P2P command when bloom filters disabled
3d3602f Add RPC test for the p2p mempool command in conjunction with disabled bloomfilters (Jonas Schnelli)
beceac9 Disable the mempool P2P command when bloom filters disabled (Peter Todd)

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

Merge #8078: Disable the mempool P2P command when bloom filters disabled
3d3602f Add RPC test for the p2p mempool command in conjunction with disabled bloomfilters (Jonas Schnelli)
beceac9 Disable the mempool P2P command when bloom filters disabled (Peter Todd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment