rfc: only put replaced txs in extra pool

Sjors commented at 12:17 pm on May 15, 2025: member

We currently store rejected mempool transactions in the extra pool. This includes (some) policy and consensus rejected transactions.

It seems trivial to attack this pool for free with consensus invalid transactions, or almost free with policy violating transactions that are extremely unlikely to ever get mined. This was known at the time it was introduced: #9499 (review)

At the same time the orphanage already handles some cases that were initially dealt with here.

This commit simplifies things by only putting replaced transactions in the extra pool, as these represent things that at least previously passed our mempool DoS protections.

Later on we could introduce similar DoS protection as #31829 does for the orphan pool, and then expand the list of things we store again. Or even revert to the present behavior.

This extra pool was introduced by #9499. It started with just adding replaced transactions in commit 93380c5247526e2248248a7d539233ec48d11bdd, added orphans in 7f8c8cab1e537809848088d3bfaa13ecca7ac8cc and then all the other rejected things in 863edb45b9841c3058e883015c7576e6f69e3cc6.

TODO:

restore some lost test coverage
consider bringing some reject reasons back to this pool

mining: only put replaced txs in extra pool

We currently store rejected mempool transactions in the extra pool. This includes (some) policy and consensus rejected transactions.

It seems trivial to attack this pool for free with consensus invalid transactions, or almost free with policy violating transactions that are extremely unlikely to ever get mined.

At the same time the orphanage already handles some cases that were initially dealt with here.

This commit simplifies things by only putting replaced transactions in the extra pool, as these represent things that at least previously passed our mempool DoS protections.

20294f8ac3

DrahtBot commented at 12:17 pm on May 15, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32510.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32379 (p2p: stop special-casing witness-stripped error for unconfirmed transactions by darosior)

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.

in src/test/fuzz/txdownloadman.cpp:227 in 20294f8ac3

223@@ -224,7 +224,6 @@ FUZZ_TARGET(txdownloadman, .init = initialize)
224                 bool first_time_failure{fuzzed_data_provider.ConsumeBool()};
225 
226                 node::RejectedTxTodo todo = txdownloadman.MempoolRejectedTx(rand_tx, state, rand_peer, first_time_failure);
227-                Assert(first_time_failure || !todo.m_should_add_extra_compact_tx);

Sjors commented at 12:19 pm on May 15, 2025:

Not sure what to replace this check with, but now it doesn’t check anything.

glozow added the label P2P on May 15, 2025

Sjors commented at 12:49 pm on May 15, 2025: member

cc @0xB10C do you have any stats on what rejected stuff typically ends up in the extra pool?

glozow commented at 1:01 pm on May 15, 2025: member

Agree that a FIFO ring buffer isn’t a robust data structure; it’s opportunistic as it says on the can. However, I think deleting most things from vExtraTxnForCompact because it’s trivially DoSable is a bit like giving up on orphan resolution because the orphanage is trivially DoSable… it seems more productive to try to come up with ways to make it less vulnerable to churn. EDIT: yes I realize this isn’t deleting the data structure altogether, but it’s close.

I have to imagine that especially with policy changes happening, this vector is useful (yes, in the absence of attackers). If you don’t support 1p1c yet, you’ll currently be throwing the TX_RECONSIDERABLE parent and TX_MISSING_INPUTS child in there. Same if you don’t support TRUC yet. I think I had a comment somewhere about returning false for TX_CONSENSUS errors. @0xB10C was collecting data on how often we use this for compact block relay reconstruction I think? Correct me if I’m wrong and it’s actually useless 100% of the time. Still, then I’d take that as motivation to add smarter protections.

in src/node/txdownloadman_impl.cpp:361 in 20294f8ac3

356@@ -357,8 +357,6 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction
357 {
358     const CTransaction& tx{*ptx};
359     // Results returned to caller
360-    // Whether we should call AddToCompactExtraTransactions at the end
361-    bool add_extra_compact_tx{first_time_failure};

glozow commented at 1:20 pm on May 15, 2025:

Any thoughts on storing transactions from outbounds only? In this function we’d be able to query who the announcers were (that we still remember). net_processing also knows who sent the transaction.

Sjors commented at 3:12 pm on May 15, 2025: member

Just to add another thought here: it seems that recently fee bumps were a big source of failed compact block propagation. By restricting the extra pool to just those, and perhaps expanding its default size, we might get more value of it, with less DoS downside.

That said, I don’t know what percentage of that fee bump traffic was due to our not supporting -mempoolfullrbf.

szarka commented at 3:26 pm on May 15, 2025: none

I have to imagine that especially with policy changes happening, this vector is useful (yes, in the absence of attackers). If you don’t support 1p1c yet, you’ll currently be throwing the TX_RECONSIDERABLE parent and TX_MISSING_INPUTS child in there. Same if you don’t support TRUC yet. I think I had a comment somewhere about returning false for TX_CONSENSUS errors.

@0xB10C was collecting data on how often we use this for compact block relay reconstruction I think? Correct me if I’m wrong and it’s actually useless 100% of the time. Still, then I’d take that as motivation to add smarter protections.

Given the concerns expressed by many recently about “filtering” slowing down block propagation, this PR sounds like a step backward to me. I am always interested in seeing B10C’s research, of course, if it exists. But FWIW here are the stats from the most recent block (896843, which doesn’t look especially spammy) on a Knots node (same logic for this functionality, AFAIK) with blockreconstructionextratxn=10000 and moderately-aggressive filtering:

02025-05-15T15:20:14Z [cmpctblock] Successfully reconstructed block 000000000000000000021ff7cef215cf12815880bf031fdb44d14636fd7a1272 with 1 txn prefilled, 3594 txn from mempool (incl at least 87 from extra pool) and 32 txn requested

Seems like including things that are rejected for policy reasons is useful.

Sjors commented at 4:02 pm on May 15, 2025: member

@szarka the extra pool is capped at 100kb regardless of the number of transactions, so any filtered content over 100kb would overflow it.

It’s (unfortunately) unclear from your log example what these 87 transactions were. If they were all RBF replacements, then this PR wouldn’t hurt that. If they’re all inscriptions, then it would.

Seems like including things that are rejected for policy reasons is useful.

In principle yes. The problem is that when we decide a transaction is invalid for policy reasons, we don’t recheck it against consensus. We don’t know the difference. So we hold on to consensus invalid transactions, which means anyone can connect to your node, feed it 100kb worth or garbage and then disconnect.

Things would be different if we did check whether something is only policy invalid, but that would eat extra CPU cycles. @glozow wrote:

is a bit like giving up on orphan resolution because the orphanage is trivially DoSable… it seems more productive to try to come up with ways to make it less vulnerable to churn

You’re working on that for orphan resolution, but nobody is currently working on it for the extra pool. If we can apply the same mechanism, that would probably be a better solution. And perhaps that’s really easy.

If we can’t apply that here, then it seems better to allocate the 100kb towards the most effective and hardest to DoS thing, e.g. by restricting it to fee bumps.

In any case I’d love to see @0xB10C’s data. I’ll keep this in draft.

szarka commented at 4:07 pm on May 15, 2025: none

In any case I’d love to see @0xB10C’s data. I’ll keep this in draft.

I think maybe this Delving post is what @glozow was referring to?

Sjors commented at 6:54 am on May 16, 2025: member

Thanks everyone for their feedback. My current thinking is that there’s two parallel ways forward:

Create a separate pool just for fee bumps, move those out of the extra pool
Harden the extra pool using the mechanism introduced in #31829

Fee bumps themselves are more DoS resistant than the other categories in extra pool. By moving them to their own structure (1), they can’t be overcrowded by the other stuff.

In that case there’s no need to limit the extra pool; it’s small, best case it helps, worst case it’s useless. By hardening it (2), we potentially keep it useful even in that worst case.

Additionally perhaps we should improve compact block reconstruction logging to include the rejection categories (e.g. by tracking the reason in vExtraTxnForCompact).

Sjors closed this on May 16, 2025

rfc: only put replaced txs in extra pool #32510

Code Coverage & Benchmarks

Reviews

Conflicts