Ignore BIP35 mempool command by default #9238

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2016/11/dis_mempool changing 8 files +24 −2

jonasschnelli commented at 4:19 PM on November 29, 2016: contributor

The mempool command was repetitively misused for surveillance and DOS attacks and the current use case (SPV scanning of unconfirmed transactions) do not justify the risks.

The command can optionally be enabled with -peermempool.
Ignore BIP35 mempool command by default 2590acfa82
jonasschnelli added the label P2P on Nov 29, 2016
sipa commented at 7:21 PM on November 29, 2016: member

I think this is a bit radical. As long as no alternative for mempool sync exists (like mempool reconciliation), I think it's a useful feature. The resource usage and privacy concerns can be addressed by for example only responding incrementally (a second mempool request only gets invs for txn that were added since the previous mempool request) or further rate limiting.
paveljanik commented at 9:37 PM on November 29, 2016: contributor

Shouldn't this be allowed only for whitelisted nodes?

in src/version.h:None in 2590acfa82

  44 | @@ -45,4 +45,7 @@ static const int SHORT_IDS_BLOCKS_VERSION = 70014;
  45 |  //! not banning for invalid compact blocks starts with this version
  46 |  static const int INVALID_CB_NO_BAN_VERSION = 70015;
  47 |  
  48 | +//! BIP35 mempool requests are ignored when the NODE_MEMPOOLCMD service bit is not set with this version
  49 | +static const int DISABLE_MEMPOOl_CMD_VERSION = 70016;

paveljanik commented at 9:39 PM on November 29, 2016:

l -> L

in doc/release-notes.md:None in 2590acfa82

  61 | @@ -62,6 +62,14 @@ Removal of Priority Estimation
  62 |    major version. To prepare for this, the default for the rate limit of priority
  63 |    transactions (`-limitfreerelay`) has been set to `0` kB/minute.
  64 |  
  65 | +
  66 | +Disabling the p2p mempool command (BIP35)
  67 | +------------------------------------------
  68 | +
  69 | +- The mempool command was repetitively misused for surveillance and DOS attacks and the current use case (SPV scanning of unconfirmed transactions) do not justify the risks.

paveljanik commented at 9:40 PM on November 29, 2016:

DOS -> DoS

paveljanik commented at 9:40 PM on November 29, 2016: contributor

p2p-mempool test failing.

in doc/release-notes.md:None in 2590acfa82

  66 | +Disabling the p2p mempool command (BIP35)
  67 | +------------------------------------------
  68 | +
  69 | +- The mempool command was repetitively misused for surveillance and DOS attacks and the current use case (SPV scanning of unconfirmed transactions) do not justify the risks.
  70 | +
  71 | +- The command can be optionally be enabled with `-peermempool`.

kallewoof commented at 11:57 PM on November 29, 2016:

The command can be optionally be enabled with -peermempool.

gmaxwell commented at 1:46 AM on November 30, 2016: contributor

I think this is far too disruptive right now. There are many things we can do to further close the attack surface without breaking SPV wallets which strongly expect this functionality.
jonasschnelli commented at 7:36 AM on November 30, 2016: contributor

What about setting the default to true (== mempool command enabled)? Right now, all you can do is -peerbloomfilter=0 to disabled the mempool command which goes even further.

The only use-case for BIP35 I'm aware of is the SPV mempool filtering to detect unconfirmed transaction after a restart. Since we have the mempool persistence (#8448) miners probably shouldn't/won't use this anymore.

This PR would introduce a service bit, SPV nodes could make use of the DNS-seeder filter (https://github.com/sipa/bitcoin-seeder/commit/c9679dc98e53124f86ebee5f62ea6fe58cddd3d5) and I expect plenty of nodes supporting this feature. They could optionally try to connect to a peer supporting mempool filtering (while they connect to other nodes with NODE_BLOOM for filtering blocks). During that state (trying to find a node that accept mempool requests), they could show an idle state for "unconfirmed balance".

Filtering the mempool for SPV is also an relatively week concept. Nodes can trivially hide or fake transactions. IMO this feature is already broken or at least trivial to break. I think SPV nodes don't want to filter the mempool of untrusted nodes (but I guess there are no other options thats why they doing it).
gmaxwell commented at 3:17 PM on November 30, 2016: contributor

Right now, all you can do is -peerbloomfilter=0 to disabled the mempool command which goes even further

Right, but why is that not sufficient for parties that just don't care to serve lite wallets at all? I don't really think it is a good use of a service bit for this. We already closed off the use of the mempool requests for timing analysis (as much as relay is closed off, at least).
jonasschnelli commented at 10:13 AM on December 6, 2016: contributor

Closing as this tend to break SPV 0-conf.
jonasschnelli closed this on Dec 6, 2016
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#9540 Bitcoin Core should throw a warning if peerbloomfilters=1