Mempool: Do not enforce TRUC checks on reorg #33504

instagibbs commented at 7:36 PM on September 29, 2025: member

This was the intended behavior but our tests didn't cover the scenario where in-block transactions themselves violate TRUC topological constraints.

The behavior in master will potentially lead to many erroneous evictions during a reorg, where evicted TRUC packages may be very high feerate and make sense to mine all together in the next block and are well within the normal anti-DoS chain limits.

This issue exists since the merge of https://github.com/bitcoin/bitcoin/pull/28948/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R956

DrahtBot added the label Mempool on Sep 29, 2025

DrahtBot commented at 7:36 PM on September 29, 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/33504.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sdaftuar, glozow, ismaelsadeeq

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:

#32587 (test: Fix reorg patterns in tests to use proper fork-based approach by yuvicc)
#28676 (Cluster mempool implementation by sdaftuar)

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.

instagibbs commented at 7:37 PM on September 29, 2025: member

cc @glozow @sdaftuar candidate for backport(s)? Should be trivial for all effected versions.

fanquake commented at 7:48 PM on September 29, 2025: member

https://github.com/bitcoin/bitcoin/actions/runs/18108538923/job/51529011305?pr=33504#step:8:1778:

Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/tx_pool/93f59991917f0dc980deabf80d553cf4573ebdd5"

test/util/txmempool.cpp:191 CheckMempoolTRUCInvariants: Assertion `entry.GetCountWithDescendants() <= TRUC_DESCENDANT_LIMIT' failed.
Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/tx_pool/93f59991917f0dc980deabf80d553cf4573ebdd5"

⚠️ Failure generated from target with exit code 1: ['/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/bin/fuzz', PosixPath('/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/tx_pool')]

instagibbs force-pushed on Sep 29, 2025

instagibbs commented at 8:12 PM on September 29, 2025: member

@fanquake thanks, TRUC topology is no longer actually being stopped when limits are bypassed(when a reorg happens), so invariants checks for TRUC may fail now. I now removed the bypass_limits flags from all but one harness because they're not actually being used to model reorgs. If we want to fuzz reorgs, we should do that instead.

fuzz: don't bypass_limits for most mempool harnesses

Using bypass_limits=true is essentially fuzzing part of a
reorg only, and results in TRUC invariants unable to be
checked. Remove most instances of bypassing limits, leaving
one harness able to do so.

bbe8e9063c

Mempool: Do not enforce TRUC checks on reorg

Not enforcing TRUC topology on reorg was the intended
behavior, but the appropriate bypass argument was not
checked.

This mistake means we could potentially invalidate a long
chain of perfectly incentive-compatible transactions that
were made historically, including subsequent non-TRUC
transactions, all of which may have been very high feerate.

Lastly, it wastes CPU cycles doing topology checks since
this behavior cannot actually enforce the topology in
general for the reorg setting.

26e71c237d

test: add more TRUC reorg coverge 06df14ba75

instagibbs force-pushed on Sep 29, 2025

in src/validation.cpp:1047 in 06df14ba75

1062 | -            // Note that we are not checking whether it opts in to replaceability via BIP125 or TRUC
1063 | -            // (which is normally done in PreChecks). However, the only way a TRUC transaction can
1064 | -            // have a non-TRUC and non-BIP125 descendant is due to a reorg.
1065 | -        } else {
1066 | -            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "TRUC-violation", err->first);
1067 | +    if (!args.m_bypass_limits) {

glozow commented at 9:59 PM on September 29, 2025:

While PackageTRUCChecks isn't in the reorg calling path, I think it would be cleaner and more future-proof to also gate that using if (!args.m_bypass_limits). There are Assumes in that function relying on the fact that SingleTRUCChecks is called first.

If you'd prefer not to, I'd recommend a comment in the commit message explaining that PackageTRUCChecks are not gated here because reorgs do not ever go through AcceptMultipleTransactions.

in test/functional/mempool_truc.py:168 in 06df14ba75

 164 | @@ -165,23 +165,36 @@ def test_truc_replacement(self):
 165 |      def test_truc_reorg(self):
 166 |          node = self.nodes[0]
 167 |          self.log.info("Test that, during a reorg, TRUC rules are not enforced")
 168 | -        tx_v2_block = self.wallet.send_self_transfer(from_node=node, version=2)

glozow commented at 1:01 AM on September 30, 2025:

Suggestion for a few more within-block test cases, if you're interested (can also just open a followup, have some other test ideas as well)

diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py
index 098631b5c41..58c69f07819 100755
--- a/test/functional/mempool_truc.py
+++ b/test/functional/mempool_truc.py
@@ -167,34 +167,76 @@ class MempoolTRUC(BitcoinTestFramework):
         self.log.info("Test that, during a reorg, TRUC rules are not enforced")
         self.check_mempool([])
 
+        # TRUC violations across the block + mempool
         # Testing 2<-3 versions allowed
         tx_v2_block = self.wallet.create_self_transfer(version=2)
-
         # Testing 3<-2 versions allowed
         tx_v3_block = self.wallet.create_self_transfer(version=3)
-
         # Testing overly-large child size
-        tx_v3_block2 = self.wallet.create_self_transfer(version=3)
+        tx_v3_large_parent = self.wallet.create_self_transfer(version=3)
+
+        # TRUC violations within the block
+        # 2<-3 versions
+        tx_v2_parent = self.wallet.create_self_transfer(version=2)
+        tx_v3_child = self.wallet.create_self_transfer(utxo_to_spend=tx_v2_parent["new_utxo"], version=3)
+        # 3<-2 versions
+        tx_v3_parent = self.wallet.create_self_transfer(version=3)
+        tx_v2_child = self.wallet.create_self_transfer(utxo_to_spend=tx_v3_parent["new_utxo"], version=2)
 
         # Also create a linear chain of 3 TRUC transactions that will be directly mined, followed by one v2 in-mempool after block is made
         tx_chain_1 = self.wallet.create_self_transfer(version=3)
         tx_chain_2 = self.wallet.create_self_transfer(utxo_to_spend=tx_chain_1["new_utxo"], version=3)
         tx_chain_3 = self.wallet.create_self_transfer(utxo_to_spend=tx_chain_2["new_utxo"], version=3)
 
-        tx_to_mine = [tx_v3_block["hex"], tx_v2_block["hex"], tx_v3_block2["hex"], tx_chain_1["hex"], tx_chain_2["hex"], tx_chain_3["hex"]]
+        # 1-parent-2-child group of TRUC transactions that will be directly mined.
+        tx_1p2c_parent = self.wallet.create_self_transfer_multi(num_outputs=2, version=3)
+        tx_1p2c_child1 = self.wallet.create_self_transfer(utxo_to_spend=tx_1p2c_parent["new_utxos"][0], version=3)
+        tx_1p2c_child2 = self.wallet.create_self_transfer(utxo_to_spend=tx_1p2c_parent["new_utxos"][1], version=3)
+
+        # 2-parent-1-child group of TRUC transactions that will be directly mined.
+        tx_2p1c_parent1 = self.wallet.create_self_transfer(version=3)
+        tx_2p1c_parent2 = self.wallet.create_self_transfer(version=3)
+        tx_2p1c_child = self.wallet.create_self_transfer_multi(utxos_to_spend=[tx_2p1c_parent1["new_utxo"], tx_2p1c_parent2["new_utxo"]], version=3)
+
+        tx_to_mine = [
+            tx_v3_block["hex"], tx_v2_block["hex"], tx_v3_large_parent["hex"],
+            tx_v2_parent["hex"], tx_v3_child["hex"],
+            tx_v3_parent["hex"], tx_v2_child["hex"],
+            tx_chain_1["hex"], tx_chain_2["hex"], tx_chain_3["hex"],
+            tx_1p2c_parent["hex"], tx_1p2c_child1["hex"], tx_1p2c_child2["hex"],
+            tx_2p1c_parent1["hex"], tx_2p1c_parent2["hex"], tx_2p1c_child["hex"]
+        ]
         block = self.generateblock(node, output="raw(42)", transactions=tx_to_mine)
 
         self.check_mempool([])
         tx_v2_from_v3 = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=tx_v3_block["new_utxo"], version=2)
         tx_v3_from_v2 = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=tx_v2_block["new_utxo"], version=3)
-        tx_v3_child_large = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=tx_v3_block2["new_utxo"], target_vsize=1250, version=3)
-        assert_greater_than(node.getmempoolentry(tx_v3_child_large["txid"])["vsize"], TRUC_CHILD_MAX_VSIZE)
+        tx_v3_large_child = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=tx_v3_large_parent["new_utxo"], target_vsize=1250, version=3)
+        assert_greater_than(node.getmempoolentry(tx_v3_large_child["txid"])["vsize"], TRUC_CHILD_MAX_VSIZE)
+
         tx_chain_4 = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=tx_chain_3["new_utxo"], version=2)
-        self.check_mempool([tx_v2_from_v3["txid"], tx_v3_from_v2["txid"], tx_v3_child_large["txid"], tx_chain_4["txid"]])
+        self.check_mempool([tx_v2_from_v3["txid"], tx_v3_from_v2["txid"], tx_v3_large_child["txid"], tx_chain_4["txid"]])
 
         # Reorg should have all block transactions re-accepted, ignoring TRUC enforcement
         node.invalidateblock(block["hash"])
-        self.check_mempool([tx_v3_block["txid"], tx_v2_block["txid"], tx_v3_block2["txid"], tx_v2_from_v3["txid"], tx_v3_from_v2["txid"], tx_v3_child_large["txid"], tx_chain_1["txid"], tx_chain_2["txid"], tx_chain_3["txid"], tx_chain_4["txid"]])
+        self.check_mempool([
+            # 3<-2 block + mempool
+            tx_v3_block["txid"], tx_v2_from_v3["txid"],
+            # 2<-3 block + mempool
+            tx_v2_block["txid"], tx_v3_from_v2["txid"],
+            # oversized child block + mempool
+            tx_v3_large_parent["txid"], tx_v3_large_child["txid"],
+            # 2<-3 within the block
+            tx_v2_parent["txid"], tx_v3_child["txid"],
+            # 3<-2 within the block
+            tx_v3_parent["txid"], tx_v2_child["txid"],
+            # chain of 3 in block + 1 in mempool
+            tx_chain_1["txid"], tx_chain_2["txid"], tx_chain_3["txid"], tx_chain_4["txid"],
+            # 1-parent-2-child in block
+            tx_1p2c_parent["txid"], tx_1p2c_child1["txid"], tx_1p2c_child2["txid"],
+            # 2-parent-1-child in block
+            tx_2p1c_parent1["txid"], tx_2p1c_parent2["txid"], tx_2p1c_child["txid"],
+        ]) [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)(extra_args=["-limitdescendantsize=10"])
     def test_nondefault_package_limits(self):

luke-jr referenced this in commit 3e1788404f on Sep 30, 2025

luke-jr referenced this in commit b141191cd0 on Sep 30, 2025

luke-jr referenced this in commit 30c435b312 on Sep 30, 2025

bitcoin deleted a comment on Sep 30, 2025

sdaftuar commented at 1:21 PM on September 30, 2025: member

ACK 06df14ba75be5f48cf9c417424900ace17d1cf4d

fanquake added the label Needs backport (30.x) on Sep 30, 2025

in src/validation.cpp:1 in 06df14ba75

glozow commented at 10:36 PM on September 30, 2025:

nit: documentation for bypass_limits in validation.h could mention that TRUC rules are not enforced and it's intended for reorgs

glozow commented at 10:31 PM on October 1, 2025: member

ACK 06df14ba75b

We don't re-check TRUC rules for descendants of reorged transactions because (1) there would be a performance hit and (2) we'd prefer to keep those fee-paying transactions: #28948 (review). I agree it makes sense to also skip the ones in blocks themselves for similar reasons, especially as there are miners that don't apply the TRUC rules - it'd be good to re-mine their transactions if there are reorgs.

in test/functional/mempool_truc.py:176 in 06df14ba75

 175 | +        tx_v2_block = self.wallet.create_self_transfer(version=2)
 176 | +
 177 | +        # Testing 3<-2 versions allowed
 178 | +        tx_v3_block = self.wallet.create_self_transfer(version=3)
 179 | +
 180 | +        # Testing overly-large child size

ismaelsadeeq commented at 10:57 AM on October 2, 2025:

In "test: add more TRUC reorg coverge" 06df14ba75be5f48cf9c417424900ace17d1cf4d

What does "Testing overly-large child size" mean here? the tx being created has a normal size maybe use the pointer as used above 3<-2 (large size)?

in test/functional/mempool_truc.py:187 in 06df14ba75

 187 | +
 188 | +        tx_to_mine = [tx_v3_block["hex"], tx_v2_block["hex"], tx_v3_block2["hex"], tx_chain_1["hex"], tx_chain_2["hex"], tx_chain_3["hex"]]
 189 | +        block = self.generateblock(node, output="raw(42)", transactions=tx_to_mine)
 190 |  
 191 | -        block = self.generate(node, 1)
 192 |          self.check_mempool([])

ismaelsadeeq commented at 10:57 AM on October 2, 2025:

In "test: add more TRUC reorg coverge" https://github.com/bitcoin/bitcoin/commit/06df14ba75be5f48cf9c417424900ace17d1cf4d

This empty mempool check in between the block generation is redundant, because we have not send any transaction to the mempool.

ismaelsadeeq approved

ismaelsadeeq commented at 11:07 AM on October 2, 2025: member

Code review ACK 06df14ba75be5f48cf9c417424900ace17d1cf4d

I've reviewed the code and verify that after this PR TRUC rules are not enforced when limit is bypassed only for single transactions validation path.

Few nits on tests which can come in follow-up with glozow suggestions as well.

fanquake merged this on Oct 2, 2025

fanquake closed this on Oct 2, 2025

fanquake referenced this in commit a3a1dcb589 on Oct 2, 2025

fanquake referenced this in commit 3485252584 on Oct 2, 2025

fanquake referenced this in commit e4f9ec2f05 on Oct 2, 2025

fanquake removed the label Needs backport (30.x) on Oct 2, 2025

fanquake commented at 1:04 PM on October 2, 2025: member

Backported to 30.x in #33473.

fanquake referenced this in commit 6f23ead4a2 on Oct 2, 2025

fanquake referenced this in commit 666aec7d49 on Oct 2, 2025

fanquake referenced this in commit a8bb76b61f on Oct 2, 2025

fanquake commented at 2:19 PM on October 2, 2025: member

Backported to 29.x in #33474.

instagibbs commented at 1:13 PM on October 3, 2025: member

I'll open a follow-up soon thanks for the in depth review here

fanquake referenced this in commit d1b5d4e9ca on Oct 3, 2025

fanquake referenced this in commit ffffdc4e97 on Oct 3, 2025

fanquake referenced this in commit 05f4aa7662 on Oct 3, 2025

fanquake referenced this in commit 11da80fe6a on Oct 3, 2025

fanquake commented at 3:19 PM on October 3, 2025: member

Backported to 28.x in #33535.