Disable the mempool P2P command when bloom filters disabled #8078

pull petertodd wants to merge 2 commits into bitcoin:master from petertodd:2016-05-mempool-p2p-and-bloom changing 2 files +106 −0

petertodd commented at 1:48 PM on May 20, 2016: contributor

Only useful to SPV peers, and attackers... like bloom is a DoS vector as far more data is sent than received.
jonasschnelli commented at 1:49 PM on May 20, 2016: contributor

utACK fd432d7bb487e410a5e83f375df018132f183871
laanwj added the label P2P on May 20, 2016
laanwj commented at 1:57 PM on May 20, 2016: member

utACK/concept ACK https://github.com/bitcoin/bitcoin/pull/8078/commits/fd432d7bb487e410a5e83f375df018132f183871
gmaxwell commented at 1:59 PM on May 20, 2016: contributor

This should also make getdata only use the relaypool to close off the mempool privacy loss.
petertodd commented at 2:05 PM on May 20, 2016: contributor

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

beceac9bbf

kazcw commented at 2:50 PM on May 20, 2016: contributor

Would it make sense also to trigger this when bloom filters are enabled, but pfrom has not set one?
Add RPC test for the p2p mempool command in conjunction with disabled bloomfilters 3d3602faf4
jonasschnelli commented at 2:52 PM on May 20, 2016: contributor

Simple test you might want to pull into this PR: https://github.com/jonasschnelli/bitcoin/commit/3d3602faf4a47855be264f05d9d52253e8bd0f9d
petertodd force-pushed on May 20, 2016
petertodd commented at 3:10 PM on May 20, 2016: contributor

@jonasschnelli Good idea, done.
luke-jr commented at 3:10 PM on May 20, 2016: member

Also useful to bootstrap a mempool.
petertodd commented at 3:21 PM on 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.

in qa/rpc-tests/p2p-mempool.py:None in 3d3602faf4

  71 | +        self.lastInv = []
  72 | +        self.send_message(msg_mempool())
  73 | +
  74 | +class P2PMempoolTests(BitcoinTestFramework):
  75 | +    def setup_chain(self):
  76 | +        initialize_chain_clean(self.options.tmpdir, 2)

MarcoFalke commented at 2:18 PM on May 21, 2016:

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 commented at 8:43 AM on May 25, 2016: contributor

Can one of the admins verify this patch?
MarcoFalke commented at 11:46 AM on June 3, 2016: member

@petertodd @jonasschnelli Interested in fixing the two nits for the rpc test?
laanwj commented at 2:25 PM on June 7, 2016: member

utACK https://github.com/bitcoin/bitcoin/pull/8078/commits/beceac9bbf14bf4a81f6f63b9cca2a64157054ae
laanwj commented at 10:56 AM on June 8, 2016: member

Going to merge this - the nits can be done later.
laanwj merged this on Jun 8, 2016
laanwj closed this on Jun 8, 2016
laanwj referenced this in commit 2156fa23b8 on Jun 8, 2016
codablock referenced this in commit 3014543a5e on Sep 16, 2017
codablock referenced this in commit 9845bfe13a on Sep 19, 2017
codablock referenced this in commit 2ee817fb0d on Dec 22, 2017
andvgal referenced this in commit 4bad9e80cc on Jan 6, 2019
furszy referenced this in commit 297fd18fd4 on Feb 13, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels