Skip to content

Disable the mempool P2P command when bloom filters disabled #8078

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 2 commits into from
Jun 8, 2016
Merged

Disable the mempool P2P command when bloom filters disabled #8078

laanwj merged 2 commits into from
Jun 8, 2016

Conversation

@petertodd
Copy link
Contributor

@petertodd 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
Copy link
Contributor

jonasschnelli commented May 20, 2016

utACK fd432d7bb487e410a5e83f375df018132f183871

@laanwj laanwj added the P2P label May 20, 2016
@laanwj
Copy link
Member

laanwj commented May 20, 2016

utACK/concept ACK fd432d7

@gmaxwell
Copy link
Contributor

gmaxwell commented May 20, 2016 •
edited
Loading

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

@petertodd
Copy link
Contributor Author

petertodd commented May 20, 2016

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

@petertodd @jonasschnelli
Disable the mempool P2P command when bloom filters disabled
beceac9 
Only useful to SPV peers, and attackers... like bloom is a DoS vector as far
more data is sent than received.
@kazcw
Copy link
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
Copy link
Contributor

jonasschnelli commented May 20, 2016

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

@petertodd petertodd force-pushed the 2016-05-mempool-p2p-and-bloom branch from fd432d7 to 3d3602f Compare May 20, 2016 15:09
@petertodd
Copy link
Contributor Author

petertodd commented May 20, 2016

@jonasschnelli Good idea, done.

@luke-jr
Copy link
Member

luke-jr commented May 20, 2016

Also useful to bootstrap a mempool.

@petertodd
Copy link
Contributor Author

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.

maflcko
maflcko reviewed May 21, 2016
View reviewed changes
qa/rpc-tests/p2p-mempool.py

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

@maflcko maflcko May 21, 2016

Choose a reason for hiding this comment

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

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
Copy link
Contributor

arowser commented May 25, 2016

Can one of the admins verify this patch?

@maflcko
Copy link
Member

maflcko commented Jun 3, 2016

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

@laanwj
Copy link
Member

laanwj commented Jun 7, 2016

utACK beceac9

@laanwj
Copy link
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
@laanwj
Merge #8078: Disable the mempool P2P command when bloom filters disabled
2156fa2 
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)
@jonasschnelli jonasschnelli mentioned this pull request Aug 4, 2016
Merged
@defuse defuse mentioned this pull request Aug 16, 2016
Open
@rebroad rebroad mentioned this pull request Aug 26, 2016
Closed
@defuse defuse mentioned this pull request Oct 20, 2016
Closed
1 task
codablock pushed a commit to codablock/dash that referenced this pull request Dec 22, 2017
@laanwj @codablock
Merge bitcoin#8078: Disable the mempool P2P command when bloom filter…
2ee817f 
…s 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)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
@laanwj @andvgal
Merge bitcoin#8078: Disable the mempool P2P command when bloom filter…
4bad9e8 
…s 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)
@furszy furszy mentioned this pull request Feb 6, 2021
Merged
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Feb 13, 2021
@furszy
Merge #2194: [Backport] Disable the mempool P2P command when bloom fi…
297fd18 
…lters disabled

aba64e5 Enable p2p_mempool.py test in the test suite (furszy)
536c251 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. (Peter Todd)

Pull request description:

  Another DoS vector patch coming straight from bitcoin#8078.

  Plus, enabled and connected the `p2p_mempool.py` functional test and reordered the `mempool_persist.py` in the list based on its time, favoring running tests in parallel.

ACKs for top commit:
  random-zebra:
    utACK aba64e5
  Fuzzbawls:
    utACK aba64e5

Tree-SHA512: c5cd1797fa2b7d90db9761a55d8c9851653e7d1915678a281cf8b0748f5ca12bd4a0bfd4fdaabed9af2f0ee27d8171a3a3f7fa0875044946c2e190d66a07b24b
@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.

Reviewers

No reviews

Assignees

No one assigned

Labels

P2P

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@petertodd @jonasschnelli @laanwj @gmaxwell @kazcw @luke-jr @arowser @maflcko