Remove BIP35 mempool p2p message

[willcl-ark]

The BIP35 mempool P2P message can be used to share the txids in a node's mempool (motivation in BIP35). However it is no longer used (?), is bad for privacy, and we can simplify our P2P code by removing it.

It was originally introduced with a protocol version bump from 60001 to 60002 and identification of a node willing to provide this service was based on this new protocol version and the advertisment of NODE_BLOOM.

Subsequently the service was gated behind the NetPermissionFlags::Mempool flag, meaning that the original BIP identitifcation method is no longer sufficient to determine that a node will offer this service to you (the node operator must whitelist or otherwise elevate your net permissions before they will respond to it). Therefore I think that it is safe to remove without any change in protocol version.

At this stage looking for concept ACKs or anybody still actively using this message to find out more about their use-cases.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	ghost, 0xB10C
Concept ACK	dergoegge, glozow, jnewbery, instagibbs, earuak, darosior, michaelfolkson

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #27114 (p2p: Allow whitelisting outgoing connections by brunoerg)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[dergoegge]

Concept ACK

(I would like to see us do the same for BIP37 at some point)

[mzumsande]

or anybody still actively using this message to find out more about their use-cases.

how about a mailing list post to reach a wider audience?

[glozow]

Concept ACK. I agree there should be a mailing list post.

[ghost]

The BIP35 mempool P2P message can be used to share the txids in a node's mempool (motivation in BIP35). However it is no longer used (?), is bad for privacy, and we can simplify our P2P code by removing it.

It is still possible to guess mempool transactions using getdata by maintaining mempool with no restrictions. I dont think sharing mempool affects privacy particularly in this case as its behind NetPermissionFlags::Mempool flag. If privacy needs to be improved at mempool level it should be possible to make all transactions look similar until included in block except fee rate however that is out of scope for discussion here.

Subsequently the service was gated behind the NetPermissionFlags::Mempool flag, meaning that the original BIP identitifcation method is no longer sufficient to determine that a node will offer this service to you (the node operator must whitelist or otherwise elevate your net permissions before they will respond to it). Therefore I think that it is safe to remove without any change in protocol version.

It could still be helpful in testing. Since we have lot of inconsistencies in mempool policies recently for political reasons, I think we should keep this P2P message available at least for testing if NetPermissionFlags::Mempool flag cannot be removed.

[mzumsande]

It is still possible to guess mempool transactions using getdata by maintaining mempool with no restrictions.

This doesn't work for new transactions, a node answers a getdata request for a tx it didn't announce to a peer via an inv if that transaction is older than UNCONDITIONAL_RELAY_DELAY (2 minutes), see here. This is meant to protect against transaction origin detection.

[sdaftuar]

Personally, I don't use this too much, but I have used it in the past (for a while I maintained a patch for myself to allow me to sync one node's mempool from another by using an RPC command). Supporting a mempool sync between two nodes that you control, to bootstrap newly started nodes, seems like a useful thing conceptually.

I don't feel strongly though, and if this is a mostly rarely or never used feature, then I'm not opposed to removing it.

I'd be curious to know how people feel about a "mempool-sync" functionality -- let's say we came up with a way to use Erlay's set reconciliation (or some other method, like weak blocks) to try to sync the top of node mempools from time to time. Is there a reason that would be a bad idea (assuming we can prevent DoS)? And if not, would that imply that the mempool command (for whitelisted peers, to avoid DoS or privacy attacks) isn't so bad to have either?

[maflcko]

In the pull description NODE_NETWORK should say NODE_BLOOM?

Supporting a mempool sync between two nodes that you control, to bootstrap newly started nodes, seems like a useful thing conceptually.

If you control both nodes, wouldn't it be easier to run the RPC on the sending side directly to fan out all transactions instead of going an extra hop over P2P? If the receiving node is offline, you can copy the mempool dat (see #25018 (comment)), but I guess that replaces the mempool, instead of only appending transactions. Maybe there is also a use case for syncing from a publicly accessible 3rd party node via a P2P message?

[ajtowns]

"Deprecate" means "we still support this, but we recommend you don't use it". cf https://en.wikipedia.org/wiki/Deprecation

[ajtowns]

However it is no longer used (?), is bad for privacy, and we can simplify our P2P code by removing it.

Not knowing whether it's used seems a bad starting point? And it's only an extra boolean and 20 something lines of code, which doesn't seem like a big deal? I would have expected it to be kept around until BIP 37 (bloom filtering) support was dropped entirely, but don't really have a justification for that.

let's say we came up with a way [...] to try to sync the top of node mempools from time to time. Is there a reason that would be a bad idea

I think that's a different way of describing "rebroadcast", which seems like a good idea: it improves miner revenue, improves tx propagation, improves block propagation, makes eclipse and pinning attacks harder... At least assuming it can be implemented without too many terrible tradeoffs.

And if not, would that imply that the mempool command (for whitelisted peers, to avoid DoS or privacy attacks) isn't so bad to have either?

I'd like to have a better way to sync mempools between my own nodes (eg, if my signet inquisition miner goes down and misses some transactions, they generally won't just reappear automatically). Dumping the entire mempool and then running sendrawtransaction a bunch of times is a bit annoying...

If you control both nodes, wouldn't it be easier to run the RPC on the sending side directly to fan out all transactions

Syncing core nodes by sending INV messages and doing regular tx relay effectively rebroadcasts the transactions, potentially resetting their expiry time. Having either a p2p command or an rpc command that lets you just sync mempools without necessarily also rebroadcasting the different transactions would be nice.

So I guess I'm -0.1 on this; but would be +0.5 on keeping it and adding a "sendmempoolcmdtopeer" rpc so that it can be used?

[maflcko]

Syncing core nodes by sending INV messages and doing regular tx relay effectively rebroadcasts the transactions, potentially resetting their expiry time. Having either a p2p command or an rpc command that lets you just sync mempools without necessarily also rebroadcasting the different transactions would be nice.

Maybe I am missing something, but I fail to see how the expiry time can be synced over p2p. My understanding is that the receiving node sets the expiry for newly accepted transactions and leaves it as-is for existing mempool transactions. Also, the reply to a mempool msg is an inv msg, so I also fail to see how an inv message is handled differently on the receiving side whether or not a mempool msg was sent prior?

[sdaftuar]

If you control both nodes, wouldn't it be easier to run the RPC on the sending side directly to fan out all transactions instead of going an extra hop over P2P?

@MarcoFalke Yeah I think this is a fair point... And I guess really if there were a use case, it would be for users of other software interacting with Bitcoin Core and wanting some functionality like this without having to recompile their Bitcoin Core node to run a custom patch (a situation that does not apply to me!).

[willcl-ark]

Thanks for the comments all. I am currently editing a ML post to get feedback from a wider audience too, as sensibly suggested.

Replies to some comments below:

It could still be helpful in testing. Since we have lot of inconsistencies in mempool policies recently for political reasons, I think we should keep this P2P message available at least for testing if NetPermissionFlags::Mempool flag cannot be removed.

AFAIK even for use in "testing" this would require custom code or a patched node (to send and handle the messages) as Bitcoin Core cannot send them. Even if we did remove this message anyone wanting the same functionality for testing could either again run a custom-patched node and/or change their test code to dump the entire mempool from the host node using savemempool and then parse/feed transactions to the client node.

I'd be curious to know how people feel about a "mempool-sync" functionality -- let's say we came up with a way to use Erlay's set reconciliation (or some other method, like weak blocks) to try to sync the top of node mempools from time to time. Is there a reason that would be a bad idea (assuming we can prevent DoS)? And if not, would that imply that the mempool command (for whitelisted peers, to avoid DoS or privacy attacks) isn't so bad to have either?

This is a very nice thought experiment which I would like to think about more, thanks. My gut tells me that querying arbitrary mempools via P2P is likely to damage tx origin privacy if not very carfeully implemented, as nodes may not end up with as much control over when they broadcast transactions -- something we currently rely on for network privacy.

Not knowing whether it's used seems a bad starting point? And it's only an extra boolean and 20 something lines of code, which doesn't seem like a big deal? I would have expected it to be kept around until BIP 37 (bloom filtering) support was dropped entirely, but don't really have a justification for that.

To be clear here, I know it's not used by Bitcoin Core, but the idea of this request for comments is to find out if it's used by any end-users/services which is something we can't find out any other way? Whilst I agree wrt BIP37, I think that would be more controversial as it likely still sees (ill-advised) use, but again, this is un-measured by myself and my long-running node has not had bloom filters enabled for some time.

I'd like to have a better way to sync mempools between my own nodes (eg, if my signet inquisition miner goes down and misses some transactions, they generally won't just reappear automatically). Dumping the entire mempool and then running sendrawtransaction a bunch of times is a bit annoying...

How do you do this currently? I agree that a mempool sync would be nice to have but I don't think P2P messages are the best way for it to work...

So I guess I'm -0.1 on this; but would be +0.5 on keeping it and adding a "sendmempoolcmdtopeer" rpc so that it can be used?|

Agree that if people want this functionality that this UX would be better even than dumping and loading the mempool, even if slightly less efficient and incomplete (see next reply).

Maybe I am missing something, but I fail to see how the expiry time can be synced over p2p. My understanding is that the receiving node sets the expiry for newly accepted transactions and leaves it as-is for existing mempool transactions. Also, the reply to a mempool msg is an inv msg, so I also fail to see how an inv message is handled differently on the receiving side whether or not a mempool msg was sent prior?

This is where savemempool paired with complementary loadmempool would be superior, as the TxMempoolInfo's m_timeis part of the dump.

[jnewbery]

Concept ACK, as long as there aren't responses to the ML post that reveal that this is a widely used feature.

[jnewbery]

This can be combined with the conditional above now:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 2a223a2adf..050563ea95 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2252,12 +2252,10 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
 CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const GenTxid& gtxid, const std::chrono::seconds now)
 {
     auto txinfo = m_mempool.info(gtxid);
-    if (txinfo.tx) {
-        // If a TX is older than UNCONDITIONAL_RELAY_DELAY, permit the request
-        // unconditionally.
-        if (txinfo.m_time <= now - UNCONDITIONAL_RELAY_DELAY) {
-            return std::move(txinfo.tx);
-        }
+    // If a TX is older than UNCONDITIONAL_RELAY_DELAY, permit the request
+    // unconditionally.
+    if (txinfo.tx && txinfo.m_time <= now - UNCONDITIONAL_RELAY_DELAY) {
+        return std::move(txinfo.tx);
     }

[jnewbery]

I think it makes sense to combine the two remaining if (fSendTrickle) blocks now, since fSendTrickle can't be modified between them:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 2a223a2adf..386783a46c 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5568,14 +5568,14 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
                     }
                 }
 
-                // Time to send but the peer has requested we not relay transactions.
                 if (fSendTrickle) {
-                    LOCK(tx_relay->m_bloom_filter_mutex);
-                    if (!tx_relay->m_relay_txs) tx_relay->m_tx_inventory_to_send.clear();
-                }
+                    {
+                        // Check whether the peer has requested that we don't relay transactions.
+                        LOCK(tx_relay->m_bloom_filter_mutex);
+                        if (!tx_relay->m_relay_txs) tx_relay->m_tx_inventory_to_send.clear();
+                    }
 
-                // Determine transactions to relay
-                if (fSendTrickle) {
+                    // Determine transactions to relay
                     // Produce a vector with all candidates for sending
                     std::vector<std::set<uint256>::iterator> vInvTx;
                     vInvTx.reserve(tx_relay->m_tx_inventory_to_send.size());

[jnewbery]

There seems to precedence for updating the document to say when support was removed, rather than deleting the line entirely (see BIP 61 below).

[glozow]

let's say we came up with a way [...] to try to sync the top of node mempools from time to time. Is there a reason that would be a bad idea

I think that's a different way of describing "rebroadcast", which seems like a good idea: it improves miner revenue, improves tx propagation, improves block propagation, makes eclipse and pinning attacks harder... At least assuming it can be implemented without too many terrible tradeoffs.

+1. I don't think it implies BIP35 support is good, though. Revealing the top transactions is pretty different from revealing the entire mempool privacy-wise. The only one that benefits here is the node that sends the mempool request. So these seem like pretty different use cases to me.

Dumping the entire mempool and then running sendrawtransaction a bunch of times is a bit annoying...

This is where savemempool paired with complementary loadmempool would be superior, as the TxMempoolInfo's m_timeis part of the dump.

Adding a loadmempool RPC seems to be exactly what we want here? The timestamps sync, it's relatively convenient (a few rpc commands and no restarts), the syncing node can keep the transactions that are already there (assuming no conflicts), and we don't need to go over p2p.

[ghost]

NACK

It is still possible to guess mempool transactions using getdata by maintaining mempool with no restrictions.

This doesn't work for new transactions, a node answers a getdata request for a tx it didn't announce to a peer via an inv if that transaction is older than UNCONDITIONAL_RELAY_DELAY (2 minutes), see here. This is meant to protect against transaction origin detection.

I was able to know if a transaction exists in mempool of peers using getdata in libbtc with some false positives.

AFAIK even for use in "testing" this would require custom code or a patched node (to send and handle the messages) as Bitcoin Core cannot send them. Even if we did remove this message anyone wanting the same functionality for testing could either again run a custom-patched node and/or change their test code to dump the entire mempool from the host node using savemempool and then parse/feed transactions to the client node.

Everyone testing bitcoin core does not have enough time and incentives to write patches for everything. Removing this p2p message obviously makes testing difficult.

I am still not sure what exactly are we achieving with this pull request? What is the rationale?

Quoting another reviewer: "Not knowing whether it's used seems a bad starting point? And it's only an extra boolean and 20 something lines of code, which doesn't seem like a big deal? I would have expected it to be kept around until BIP 37 (bloom filtering) support was dropped entirely, but don't really have a justification for that."

[instagibbs]

Adding a loadmempool RPC seems to be exactly what we want here?

concept ACK on this, if that's the only use-case. We should probably offer a similar service over RPC first, in addition to information gathering that it's not actually being used in meaningful amounts in production. I've never come across usage myself in my production work.

I'm not sure where "20 lines of code" came from as it looks like at least a few multiples of that, but in general, removing a service no one uses to make code more readable will be a win, especially in net_processing.

[glozow]

Adding a loadmempool RPC seems to be exactly what we want here?

concept ACK on this, if that's the only use-case. We should probably offer a similar service over RPC first, in addition to information gathering that it's not actually being used in meaningful amounts in production. I've never come across usage myself in my production work.

#27460

[ghost]

I retract my NACK

#27426 (review)

ACK for removing this useless p2p message

[darosior]

Concept ACK but i think BTCPay is using it. cc @NicolasDorier

[jonatack]

Linking here this response to the ML request for feedback.

https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-April/021563.html

[HenrikJannsen]

@schildbach - this would be problematic for BitcoinJ, no?

[schildbach]

The mempool message is used by all wallets based on bitcoinj to learn about pending transactions, in both full blocks mode and also in filtered mode. The estimated install base is 10M+.

[HenrikJannsen]

The mempool message is used by all wallets based on bitcoinj to learn about pending transactions, in both full blocks mode and also in filtered mode. The estimated install base is 10M+.

I guess the large install base should be reason enough to drop that proposal.

Bisq is using BitcoinJ and would be affected by that proposed change as well.

[0xB10C]

Its original intention was to be publicly callable, but it is now (in Bitcoin Core) gated behind stricter Net Permissions which make it accessible to trusted peers only.

Subsequently the service was gated behind the NetPermissionFlags::Mempool flag, meaning that the original BIP identitifcation method is no longer sufficient to determine that a node will offer this service to you (the node operator must whitelist or otherwise elevate your net permissions before they will respond to it). Therefore I think that it is safe to remove without any change in protocol version.

This is incorrect. A Bitcoin Core node with bloom filters enabled (off by default; enable with -peerbloomfilters=1) does respond to mempool messages with INV messages containing mempool transactions. Clients do not need special permissions. However, I too found this if-statement to be hard to reason about.

According to DSN KIT data about 22% of the reachable nodes (they only connect to IPv4 and IPv6) set the NODE_BLOOM service flag and bitnodes data (also includes onion nodes) shows about 6100 nodes out of 17554 (35%) having the NODE_BLOOM service flag set. For example, Umbrel and raspiblitz nodes do. Given these numbers, a wallet/light-client probably doesn't have a hard time finding a peer that responds to mempool messages. Removing the handling of mempool messages from Bitcoin Core would probably impact these clients.

However it is no longer used (?), is bad for privacy, and we can simplify our P2P code by removing it.

Not knowing whether it's used seems a bad starting point?

I configured one of my p2p monitoring nodes to capture some mempool-message related statistics a few days ago. The Bitcoin Core node runs a recent master commit and is and has been advertising the bloom filter service bit (via -peerbloomfilters=1) for >30 days. I assume it is well-known to other nodes and light-clients on the network. The inbound connection slots are usually full.

This node has been receiving on average 18.6 mempool-messages per hour (σ = 10.9). With fewer messages on the weekend. The client IP addresses seem to mostly belong to residential ISPs. However, there's also a noticeable amount of requests from Linode servers (VPN? something else?) and some from known VPN providers.

The most common user agents (in descending frequency) are /BitcoinKit:0.1.0/ from bitcoin-kit-ios, /bitcoinj:0.*.*/Bitcoin Wallet:*/ (different versions, but seemingly more recent than the LinkingLion bitcoinj user-agents) from the @schildbach wallet, /WalletKit:0.1.0/ by unstoppable-wallet-android, and /bread:2.1/ by breadwallet-core. I've also received a single mempool message from the user agents /libbitcoin:3.7.0/, /BQS:0.0.1/, and /Bither1.6.1/.

I'm seeing INV responses with up to 50k (MAX_INV_SZ) entries. There are also seem to be INVs to clients which loaded a filter before. The difference in entries per INV too a node without bloom filters is clearly visible.

The mempool message is used by all wallets based on bitcoinj to learn about pending transactions, in both full blocks mode and also in filtered mode. The estimated install base is 10M+.

I think "10M+" is the number of installations over the last 10 years or so? I this doesn't necessarily comes close to 10M active installations and isn't really a compelling statistic for this PR. I think, for Google Play Store installations, you could actually get a somewhat accurate number of active installations from the Google developer console.

Also, I think it would be useful to hear for what the mempool message is being use for in bitcoinj-based wallets. Listing unconfirmed transactions? Would it break the wallet if there wouldn't be any NODE_BLOOM nodes or nodes which respond to mempool messages?

---

I'm generally in favor of slimming down and removing the message (and also the BIP-37 implementation) at some point. Given that the mempool message can and is being used by clients/wallets I don't think it's a good idea to remove it at the moment. However, removing it probably shouldn't render the clients and wallets completely useless.

Still, this leaves me with a tendency towards a Concept NACK for now because it can and is being used.

[michaelfolkson]

@0xB10C: Thanks for this, great analysis.

I'm generally in favor of slimming down and removing the message (and also the BIP-37 implementation) at some point.

I read that as a longer term Concept ACK for removal but you'd like a deprecation release schedule rather than removing it immediately in say the next major version release. Presumably something like a deprecation warning in the next major version release before actual deprecation removal in the following major version release could work for the removal of P2P messages. I previously tried to document the thinking on how the project does/should treat deprecation generally here but not sure whether P2P messages fit into a generalized deprecation process or whether they should be treated uniquely.

[willcl-ark]

Thanks for your statistics on the usage of this message @0xB10C, they show a compelling amount of use for the BIP35 protocol still.

As David Harding suggested on the mailing list, which I agree with, removing an actively-used feature in absence of an alternative would be undesirable, and therefore will close this PR for now.