Mempool: Do not enforce TRUC checks on reorg

[instagibbs]

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]

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]

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

[fanquake]

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]

@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.

[sdaftuar]

ACK 06df14b

[glozow]

ACK 06df14b

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 (comment). 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.

[glozow]

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.

[glozow]

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(extra_args=["-limitdescendantsize=10"])
     def test_nondefault_package_limits(self):

[glozow]

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

[ismaelsadeeq]

Code review ACK 06df14b

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.

[ismaelsadeeq]

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

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)?

[ismaelsadeeq]

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

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