Remove BIP35 mempool p2p message

willcl-ark commented at 8:10 pm on April 5, 2023: member

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 commented at 8:10 pm on April 5, 2023: contributor

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 commented at 8:31 pm on April 5, 2023: member

Concept ACK

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

net: remove BIP35 mempool p2p message

This message can be used to share 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.

c4dc593cfc

doc: Release note for `mempool` p2p msg removal

* Remove from the list of supported bips
* Add a release note for the change

05b2945f03

willcl-ark force-pushed on Apr 5, 2023

mzumsande commented at 9:24 pm on April 5, 2023: contributor

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 commented at 12:41 pm on April 6, 2023: member

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

glozow added the label Needs Conceptual Review on Apr 6, 2023

glozow added the label P2P on Apr 6, 2023

ghost commented at 6:34 am on April 7, 2023: none

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 commented at 9:02 pm on April 10, 2023: contributor

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 commented at 12:42 pm on April 11, 2023: member

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 commented at 1:05 pm on April 11, 2023: member

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?

in doc/release-notes-bip35.md:6 in 05b2945f03

0@@ -0,0 +1,14 @@
1+Notable changes
2+===============
3+
4+P2P and network changes
5+-----------------------
6+* Deprecate the BIP35 `mempool` message due to disuse and poor privacy.

ajtowns commented at 4:21 am on April 12, 2023:

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

ajtowns commented at 4:37 am on April 12, 2023: contributor

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 commented at 7:19 am on April 12, 2023: member

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 commented at 8:23 am on April 12, 2023: member

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 commented at 10:02 am on April 12, 2023: member

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.

willcl-ark renamed this:
~~Deprecate and remove BIP35 mempool p2p message~~
Remove BIP35 mempool p2p message
on Apr 12, 2023

in src/net_processing.cpp:2258 in 05b2945f03

2257-        // If a TX could have been INVed in reply to a MEMPOOL request,
2258-        // or is older than UNCONDITIONAL_RELAY_DELAY, permit the request
2259+        // If a TX is older than UNCONDITIONAL_RELAY_DELAY, permit the request
2260         // unconditionally.
2261-        if ((mempool_req.count() && txinfo.m_time <= mempool_req) || txinfo.m_time <= now - UNCONDITIONAL_RELAY_DELAY) {
2262+        if (txinfo.m_time <= now - UNCONDITIONAL_RELAY_DELAY) {

jnewbery commented at 10:49 am on April 12, 2023:

This can be combined with the conditional above now:

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

in src/net_processing.cpp:5614 in 05b2945f03

5573@@ -5610,36 +5574,6 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
5574                     if (!tx_relay->m_relay_txs) tx_relay->m_tx_inventory_to_send.clear();
5575                 }
5576 
5577-                // Respond to BIP35 mempool requests
5578-                if (fSendTrickle && tx_relay->m_send_mempool) {

jnewbery commented at 10:57 am on April 12, 2023:

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

 0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index 2a223a2adf..386783a46c 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -5568,14 +5568,14 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
 5                     }
 6                 }
 7 
 8-                // Time to send but the peer has requested we not relay transactions.
 9                 if (fSendTrickle) {
10-                    LOCK(tx_relay->m_bloom_filter_mutex);
11-                    if (!tx_relay->m_relay_txs) tx_relay->m_tx_inventory_to_send.clear();
12-                }
13+                    {
14+                        // Check whether the peer has requested that we don't relay transactions.
15+                        LOCK(tx_relay->m_bloom_filter_mutex);
16+                        if (!tx_relay->m_relay_txs) tx_relay->m_tx_inventory_to_send.clear();
17+                    }
18 
19-                // Determine transactions to relay
20-                if (fSendTrickle) {
21+                    // Determine transactions to relay
22                     // Produce a vector with all candidates for sending
23                     std::vector<std::set<uint256>::iterator> vInvTx;
24                     vInvTx.reserve(tx_relay->m_tx_inventory_to_send.size());

in doc/bips.md:15 in 05b2945f03

11@@ -12,7 +12,6 @@ BIPs that are implemented by Bitcoin Core (up-to-date up to **v24.0**):
12 * [`BIP 31`](https://github.com/bitcoin/bips/blob/master/bip-0031.mediawiki): The 'pong' protocol message (and the protocol version bump to 60001) has been implemented since **v0.6.1** ([PR #1081](https://github.com/bitcoin/bitcoin/pull/1081)).
13 * [`BIP 32`](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki): Hierarchical Deterministic Wallets has been implemented since **v0.13.0** ([PR #8035](https://github.com/bitcoin/bitcoin/pull/8035)).
14 * [`BIP 34`](https://github.com/bitcoin/bips/blob/master/bip-0034.mediawiki): The rule that requires blocks to contain their height (number) in the coinbase input, and the introduction of version 2 blocks has been implemented since **v0.7.0**. The rule took effect for version 2 blocks as of *block 224413* (March 5th 2013), and version 1 blocks are no longer allowed since *block 227931* (March 25th 2013) ([PR #1526](https://github.com/bitcoin/bitcoin/pull/1526)).
15-* [`BIP 35`](https://github.com/bitcoin/bips/blob/master/bip-0035.mediawiki): The 'mempool' protocol message (and the protocol version bump to 60002) has been implemented since **v0.7.0** ([PR #1641](https://github.com/bitcoin/bitcoin/pull/1641)). As of **v0.13.0**, this is only available for `NODE_BLOOM` (BIP 111) peers.

jnewbery commented at 11:00 am on April 12, 2023:

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

jnewbery commented at 11:01 am on April 12, 2023: contributor

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

glozow commented at 2:32 pm on April 12, 2023: member

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.

unknown changes_requested

unknown commented at 4:29 pm on April 12, 2023: none

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 commented at 4:38 pm on April 12, 2023: member

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 commented at 11:24 am on April 13, 2023: member

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 commented at 6:37 pm on April 13, 2023: none

I retract my NACK

#27426#pullrequestreview-1381688305

ACK for removing this useless p2p message

darosior commented at 11:34 am on April 18, 2023: member

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

jonatack commented at 3:38 pm on April 24, 2023: contributor

Linking here this response to the ML request for feedback.

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

HenrikJannsen commented at 1:56 am on April 28, 2023: none

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

schildbach commented at 1:08 pm on April 28, 2023: contributor

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 commented at 3:38 am on April 29, 2023: none

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 commented at 12:48 pm on May 1, 2023: contributor

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 commented at 1:26 pm on May 1, 2023: contributor

@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 commented at 2:18 pm on May 1, 2023: member

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.

willcl-ark closed this on May 1, 2023

0xB10C referenced this in commit 4581a682d2 on May 2, 2023

bitcoin deleted a comment on May 3, 2023

glozow referenced this in commit 8f5da89625 on May 3, 2023

RandyMcMillan referenced this in commit fb0a4c46b4 on May 27, 2023

bitcoin locked this on May 2, 2024

Remove BIP35 mempool p2p message #27426

Reviews

Conflicts