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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #17805 (test: Add test for rpcwhitelistdefault by emilengler)
• #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
• #17693 (rpc: Add generateblock to mine a custom set of transactions by andrewtoth)
• #17484 (wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload by ariard)
• #17303 (p2p: Stop relaying non-mempool txs, improve tx privacy slightly by MarcoFalke)
• #17261 (Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101)
• #16910 (wallet: reduce loading time by using unordered maps by achow101)
• #16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
• #15719 (Drop Chain::requestMempoolTransactions method by ryanofsky)

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.

This code is ready for initial review.

there are still some to dos before it would be ready for merge:

• identify expected rebroadcast traffic & worst case bandwidth usage.
• persist setUnbroadcastTxIDs to mempool.dat
• add functionality to run an automated job to cache min fee rate for txns to be included in block, then apply that filter to exclude txns with fee rate < min from rebroadcast set. this will reduce rebroadcast noise in scenarios where the mempool is emptying out.

there are also some follow-ups that can be addressed in separate PRs:

• m_best_block_time is no longer used & can be removed & the wallet no longer needs to subscribe to UpdatedBlockTip() validation interface method
• functionality to mark a peer (as "local" or such) so the mempool would still enforce initial broadcast for transactions received from one of these peers.

Concept ACK

thanks for the review @mzumsande !

I don’t understand 1) the concept “should have been mined" [...] do you mean the txns should have been mined in a specific block/range of blocks, but weren’t? Should no rebroadcasts happen in an ideal world where miners have an identical mempool to ours and mine rationally?

based on the local mempool, we are attempting to answer the question of what txns we think should have been mined. And saying if it wasn't, maybe there was an issue with relay.
You are correct- in a world where all mempools are consistent there wouldn't be rebroadcasts.

As to 2) from what I understand, BlockAssembler creates a block template with 3/4*MAX_BLOCK_WEIGHT including the top feerate packages of our current mempool. Wouldn’t it always fill this block template with txns if our mempool is large enough, and therefore rather include 75% of the txns that we expect to be mined in the next block, instead of txns that should have been mined in the past?

yes. you will start with txns you expect to be mined in the next block. the recency filter will (likely) remove some of those transactions. however, in the case of a mempool thats emptying out, the recency filter might not have much effect. for that I have this todo before the PR would be ready for merge:

add functionality to run an automated job to cache min fee rate for txns to be included in block, then apply that filter to exclude txns with fee rate < min from rebroadcast set

based on the local mempool, we are attempting to answer the question of what txns we think should have been mined.

What confuses me is how we can answer that without actually looking into recent blocks.
Considering that txns are removed from the mempool once they are included in a valid block, it is possible that the previous blocks removed the respective top of our previous versions of the mempool, so we are left with txns that were not at the top of the mempool earlier but are now, which we wouldn't want to rebroadcast.
Or the miners could have left out several high-fee-rate txns in favor of lower ones, in which case the higher ones are still present in our mempool and we would like to rebroadcast them.
Or there just might have been no new blocks found in the hour since the last rebroadcast, in which case we wouldn't need to rebroadcast.

How could we distinguish between these cases by just looking at our current mempool?

I think the best way to address would be a kill switch to disable rebroadcasting.

There is one, -walletbroadcast=0.

How should this be handling dependencies?

If you are merely filtering on feerate will you end up with a situation where you rebroadcast a child transaction that has a high feerate even when the low feerate of its parent means it has no chance of being included?

Has anyone considered significantly increasing the time between rebroadcasts as an additional change (and one that could go in separately much faster)? Particularly once you've performed one successful broadcast per execution additional rebroadcasts usually do nothing except leak data to attackers that frequently reconnect to you-- you won't resend to a host you've sent to before, and its not usually useful to do so. This wouldn't obviate the need to be more intelligent, but it would reduce the bleeding and also make it more acceptable for the rest of the change to be more approximate.

Miners could have exclusive deals with parties that do not want to broadcast their transactions to the network and pay the miners privately to mine their transactions.

This sort of concern would be substantially mitigated by not rebroadcasting as soon as the transaction is 'missed' but only after it is missed by N blocks (say 3) blocks where, according to your mempool they should have included the transaction. Then you would only rebroadcast inappropriately if all 3 miners should have included it but skipped it for some reason.

The biggest question we all have about these changes is the bandwidth impact.

Bandwidth impact should be somewhat mitigated by the fact that the known filters should block many of the rebroadcasts to long running peers.

FWIW, I missed on the first pass that this change makes rebroadcasting apply to all mempool transactions. I think that is conceptually a very good move but it has considerable implications.

Looking at your average bandwidth usage isn't enough, you have to worry about cases like what if all nodes are running into their mempool limits under high load-- will this change cause the network to start DOS attacking itself?

Would this PR be better if we used a "thin blocks" style relay? We already form a block template, but then we don't take advantage of the compact-blocks relay code which would minimize the data sent, assuming the neighbor node does have most of what we rebroadcast...

Compact blocks would be entirely the wrong mechanism. The transactions are unordered, unlike a block. And they are almost entirely known. The mechanism you want is the erlay reconcillation mechanism because it uses bandwidth equal to the size of the actual set difference, not the size of the mempool.

In fact with that mechanism, in theory there is no need to do any additional mempool feerate tracking, comparisons with blocks or whatever. Just use erlay to send IDs for the entire mempool (filtered only by the peer's current relay minimum feerate) after every block. This takes bandwidth equal to 4 bytes times the number of differences, no bytes per entry in common.

However, in practice it isn't that simple. While designing erlay we specifically consider and discarded the approach of making it work by synchronizing the mempools (which was how I approached the relay efficiency problem in my original writeup). The reason for this is that difference in mempool policy (standardness, minfees, maximum mempool sizes, ancestor depth limits, softforks) or even just simple transaction ordering causes persistent network-wide bandwidth leaks if you relay by syncing the mempool.

All these problems also apply here, because this is essentially a mempool syncing proposal, but even worse because compared to the erlay reconciliation this is an extremely inefficient way of syncing: it spends bandwidth for transactions the peer already knows.

Consider an attacker that monitors the network and finds nodes close to miners. Then he sends the near-miner nodes a bunch of very low feerate transactions which won't get mined soon . He concurrently sends conflicting high feerate transactions to every other node. The high feerate transactions don't get mined, the other nodes have no idea why, and they bleed bandwidth continually uselessly re-advertising transactions. (if the higher feerate txn gets mined by accident he just tries again)

If erlay style relay is used, the bandwidth is really only wasted at the low/high feerate boundary... but unfortunately the attacker can make that boundary arbitrarily large (e.g. give half the nodes the low feerate txn in additional to all the miner-proximal nodes).

Question: Is it a goal to rebroadcast transactions to peers that have already had them sent to them?

If yes: the current PR (and wallet rebroadcasting) fails to do that (reliably) because of the per peer bloom filters on broadcasts.

If no: great that reduces bandwidth, but the current bloomfiltering approach doesn't successfully do that reliably-- because it forgets. The existing bloom filters are slow and use a lot of memory (they're gigantic to avoid false positives that would break txn relay). For other reasons its been previously proposed to replace them with data in each mempool transaction that indicates which peers its been relayed to.

E.g. a possible approach is each mempool txn gets a

NodeId lowest_connected_unsent_peer;
vector<bool> sent_peers_above_lowest_unsent;

Or something analogous but with less allocation overhead. :)

I suspect that for reasonable numbers of peers and transactions this could take less memory than the bloom filter (which I seem to vaguely recall is something like 1MB per peer?).

thank you for your review @gmaxwell !

RE: kill switch & -walletbroadcast=0

• This mechanism previously disabled rebroadcasts, since that was a purely wallet focused responsibility. With these changes (in the current state), setting this flag to false will not disable the node rebroadcasting transactions.
• Conceptually I think this makes sense. with these changes rebroadcasting transactions is more of a p2p/mempool responsibility. Disabling wallet broadcast shouldn’t disable rebroadcasting.
• -blocksonly mode would disable rebroadcast because there would be no txns in the mempool (unless you submit locally)
• I’m planning to add another flag to disable rebroadcast specifically. This can also be used for rollout, and nodes can choose to enable the new logic to verify reasonable behavior.

RE: handling dependencies
for the reasons you mentioned I use addPackageTx for calculating top of mempool.

Has anyone considered significantly increasing the time between rebroadcasts as an additional change (and one that could go in separately much faster)

I have not. That’s a great idea. I'm going to create a smaller PR with the mempool logic that guarantees delivery of wallet transactions & reduce frequency of rebroadcasts in the existing implementation. Thank you!

This sort of concern would be substantially mitigated by not rebroadcasting as soon as the transaction is ‘missed’ but only after it is missed by N blocks (say 3) blocks …

Yeah. For these sorts of reason there are impositions on the rebroadcast set (mainly txn must be >30 mins old. block must have arrived between last cache run and rebroadcast ). Still an open question whether the params chosen are the most reasonable.

RE: bandwidth

Yup and yup. filterInventoryKnown hugely decreases the inv messages sent to peers in the normal cases. But average bandwidth usage is insufficient for accepting these changes, simply just one aspect.

The attack you’ve portrayed here is in line with the open question I’ve been pondering about mempools with different policies - the non-adversarial case you’ve mentioned. The solution I’m thinking about is adding another data structure, max_rebroadcast_count that would maintain txids, expiry times, and count (number of times I have rebroadcasted). Over a certain count, it would serve as a rebroadcast blacklist. If the txn is mined into a block it would be removed from the list, still have to think through the other expiry conditions. I’m still hammering out the details but would be interested in hearing your feedback.

Question: Is it a goal to rebroadcast transactions to peers that have already had them sent to them?

Hm, somewhere in between? The goal of rebroadcasting transactions is to propagate a transaction out to the network in your mempool that you suspect the miners might not have. If you’ve recently sent a txn to a peer, you don’t have reason to believe resending it to them would help solve this issue, and would just waste bandwidth. On the other hand, you would want to rebroadcast to the same peers in eg. a situation where you have a large mempool, your peers have small mempools & a competitive fee rate market caused them to drop a transaction. After the fee pressure reduces, re-announcing the txn to them could help it get mined. In its current form, the bloom filter is conducive to this behavior by preventing repeat sends to a peer within a ~2-6 hr time period (including txns dropped from the mempool).

RE: alternative peer-transaction tracking system

I’m definitely interested in thinking through this approach you’re proposing / other alternatives to the current bloom filter, but think its tangential to this PR? Would be happy to continue the convo on a new issue if you’re interested in opening.

closing this PR until #18038 gets merged.

if you're interested in moving this project forward, #18038 is ready for review :) thanks in advance!

Is this work going to continue now that #18038 is done?

@gmaxwell yes! I'm currently trying to work out a couple more kinks before continuing the review process on github & seeking feedback on approach.

this work is being continued in #21061 :)