Mempool: Do not enforce TRUC checks on reorg #33504

pull instagibbs wants to merge 3 commits into bitcoin:master from instagibbs:2025-09-truc-reorg-fix changing 4 files +56 −35
  1. 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

  2. DrahtBot added the label Mempool on Sep 29, 2025
  3. 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.

  4. 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.
  5. 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:

    0Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/tx_pool/93f59991917f0dc980deabf80d553cf4573ebdd5"
    1
    2test/util/txmempool.cpp:191 CheckMempoolTRUCInvariants: Assertion `entry.GetCountWithDescendants() <= TRUC_DESCENDANT_LIMIT' failed.
    3Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/tx_pool/93f59991917f0dc980deabf80d553cf4573ebdd5"
    4
    5⚠️ 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')]
    
  6. instagibbs force-pushed on Sep 29, 2025
  7. 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.
  8. 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
  9. 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
  10. test: add more TRUC reorg coverge 06df14ba75
  11. instagibbs force-pushed on Sep 29, 2025
  12. 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.

  13. 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)

     0diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py
     1index 098631b5c41..58c69f07819 100755
     2--- a/test/functional/mempool_truc.py
     3+++ b/test/functional/mempool_truc.py
     4@@ -167,34 +167,76 @@ class MempoolTRUC(BitcoinTestFramework):
     5         self.log.info("Test that, during a reorg, TRUC rules are not enforced")
     6         self.check_mempool([])
     7 
     8+        # TRUC violations across the block + mempool
     9         # Testing 2<-3 versions allowed
    10         tx_v2_block = self.wallet.create_self_transfer(version=2)
    11-
    12         # Testing 3<-2 versions allowed
    13         tx_v3_block = self.wallet.create_self_transfer(version=3)
    14-
    15         # Testing overly-large child size
    16-        tx_v3_block2 = self.wallet.create_self_transfer(version=3)
    17+        tx_v3_large_parent = self.wallet.create_self_transfer(version=3)
    18+
    19+        # TRUC violations within the block
    20+        # 2<-3 versions
    21+        tx_v2_parent = self.wallet.create_self_transfer(version=2)
    22+        tx_v3_child = self.wallet.create_self_transfer(utxo_to_spend=tx_v2_parent["new_utxo"], version=3)
    23+        # 3<-2 versions
    24+        tx_v3_parent = self.wallet.create_self_transfer(version=3)
    25+        tx_v2_child = self.wallet.create_self_transfer(utxo_to_spend=tx_v3_parent["new_utxo"], version=2)
    26 
    27         # Also create a linear chain of 3 TRUC transactions that will be directly mined, followed by one v2 in-mempool after block is made
    28         tx_chain_1 = self.wallet.create_self_transfer(version=3)
    29         tx_chain_2 = self.wallet.create_self_transfer(utxo_to_spend=tx_chain_1["new_utxo"], version=3)
    30         tx_chain_3 = self.wallet.create_self_transfer(utxo_to_spend=tx_chain_2["new_utxo"], version=3)
    31 
    32-        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"]]
    33+        # 1-parent-2-child group of TRUC transactions that will be directly mined.
    34+        tx_1p2c_parent = self.wallet.create_self_transfer_multi(num_outputs=2, version=3)
    35+        tx_1p2c_child1 = self.wallet.create_self_transfer(utxo_to_spend=tx_1p2c_parent["new_utxos"][0], version=3)
    36+        tx_1p2c_child2 = self.wallet.create_self_transfer(utxo_to_spend=tx_1p2c_parent["new_utxos"][1], version=3)
    37+
    38+        # 2-parent-1-child group of TRUC transactions that will be directly mined.
    39+        tx_2p1c_parent1 = self.wallet.create_self_transfer(version=3)
    40+        tx_2p1c_parent2 = self.wallet.create_self_transfer(version=3)
    41+        tx_2p1c_child = self.wallet.create_self_transfer_multi(utxos_to_spend=[tx_2p1c_parent1["new_utxo"], tx_2p1c_parent2["new_utxo"]], version=3)
    42+
    43+        tx_to_mine = [
    44+            tx_v3_block["hex"], tx_v2_block["hex"], tx_v3_large_parent["hex"],
    45+            tx_v2_parent["hex"], tx_v3_child["hex"],
    46+            tx_v3_parent["hex"], tx_v2_child["hex"],
    47+            tx_chain_1["hex"], tx_chain_2["hex"], tx_chain_3["hex"],
    48+            tx_1p2c_parent["hex"], tx_1p2c_child1["hex"], tx_1p2c_child2["hex"],
    49+            tx_2p1c_parent1["hex"], tx_2p1c_parent2["hex"], tx_2p1c_child["hex"]
    50+        ]
    51         block = self.generateblock(node, output="raw(42)", transactions=tx_to_mine)
    52 
    53         self.check_mempool([])
    54         tx_v2_from_v3 = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=tx_v3_block["new_utxo"], version=2)
    55         tx_v3_from_v2 = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=tx_v2_block["new_utxo"], version=3)
    56-        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)
    57-        assert_greater_than(node.getmempoolentry(tx_v3_child_large["txid"])["vsize"], TRUC_CHILD_MAX_VSIZE)
    58+        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)
    59+        assert_greater_than(node.getmempoolentry(tx_v3_large_child["txid"])["vsize"], TRUC_CHILD_MAX_VSIZE)
    60+
    61         tx_chain_4 = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=tx_chain_3["new_utxo"], version=2)
    62-        self.check_mempool([tx_v2_from_v3["txid"], tx_v3_from_v2["txid"], tx_v3_child_large["txid"], tx_chain_4["txid"]])
    63+        self.check_mempool([tx_v2_from_v3["txid"], tx_v3_from_v2["txid"], tx_v3_large_child["txid"], tx_chain_4["txid"]])
    64 
    65         # Reorg should have all block transactions re-accepted, ignoring TRUC enforcement
    66         node.invalidateblock(block["hash"])
    67-        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"]])
    68+        self.check_mempool([
    69+            # 3<-2 block + mempool
    70+            tx_v3_block["txid"], tx_v2_from_v3["txid"],
    71+            # 2<-3 block + mempool
    72+            tx_v2_block["txid"], tx_v3_from_v2["txid"],
    73+            # oversized child block + mempool
    74+            tx_v3_large_parent["txid"], tx_v3_large_child["txid"],
    75+            # 2<-3 within the block
    76+            tx_v2_parent["txid"], tx_v3_child["txid"],
    77+            # 3<-2 within the block
    78+            tx_v3_parent["txid"], tx_v2_child["txid"],
    79+            # chain of 3 in block + 1 in mempool
    80+            tx_chain_1["txid"], tx_chain_2["txid"], tx_chain_3["txid"], tx_chain_4["txid"],
    81+            # 1-parent-2-child in block
    82+            tx_1p2c_parent["txid"], tx_1p2c_child1["txid"], tx_1p2c_child2["txid"],
    83+            # 2-parent-1-child in block
    84+            tx_2p1c_parent1["txid"], tx_2p1c_parent2["txid"], tx_2p1c_child["txid"],
    85+        ]) [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)(extra_args=["-limitdescendantsize=10"])
    86     def test_nondefault_package_limits(self):
    
  14. luke-jr referenced this in commit 3e1788404f on Sep 30, 2025
  15. luke-jr referenced this in commit b141191cd0 on Sep 30, 2025
  16. luke-jr referenced this in commit 30c435b312 on Sep 30, 2025
  17. bitcoin deleted a comment on Sep 30, 2025
  18. bitcoin deleted a comment on Sep 30, 2025
  19. bitcoin deleted a comment on Sep 30, 2025
  20. bitcoin deleted a comment on Sep 30, 2025
  21. bitcoin deleted a comment on Sep 30, 2025
  22. bitcoin deleted a comment on Sep 30, 2025
  23. bitcoin deleted a comment on Sep 30, 2025
  24. sdaftuar commented at 1:21 pm on September 30, 2025: member
    ACK 06df14ba75be5f48cf9c417424900ace17d1cf4d
  25. fanquake added the label Needs backport (30.x) on Sep 30, 2025
  26. 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
  27. 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.

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

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

  30. ismaelsadeeq approved
  31. 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.

  32. fanquake merged this on Oct 2, 2025
  33. fanquake closed this on Oct 2, 2025

  34. fanquake referenced this in commit a3a1dcb589 on Oct 2, 2025
  35. fanquake referenced this in commit 3485252584 on Oct 2, 2025
  36. fanquake referenced this in commit e4f9ec2f05 on Oct 2, 2025
  37. fanquake removed the label Needs backport (30.x) on Oct 2, 2025
  38. fanquake commented at 1:04 pm on October 2, 2025: member
    Backported to 30.x in #33473.
  39. fanquake referenced this in commit 6f23ead4a2 on Oct 2, 2025
  40. fanquake referenced this in commit 666aec7d49 on Oct 2, 2025
  41. fanquake referenced this in commit a8bb76b61f on Oct 2, 2025
  42. fanquake commented at 2:19 pm on October 2, 2025: member
    Backported to 29.x in #33474.
  43. 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
  44. fanquake referenced this in commit d1b5d4e9ca on Oct 3, 2025
  45. fanquake referenced this in commit ffffdc4e97 on Oct 3, 2025
  46. fanquake referenced this in commit 05f4aa7662 on Oct 3, 2025
  47. fanquake referenced this in commit 11da80fe6a on Oct 3, 2025
  48. fanquake commented at 3:19 pm on October 3, 2025: member
    Backported to 28.x in #33535.

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-10-10 15:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me