Skip to content

Mempool: Do not enforce TRUC checks on reorg #33504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fanquake merged 3 commits into from
Oct 2, 2025
Merged

Mempool: Do not enforce TRUC checks on reorg #33504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bbe8e90
fuzz: don't bypass_limits for most mempool harnesses
instagibbs Sep 29, 2025
26e71c2
Mempool: Do not enforce TRUC checks on reorg
instagibbs Sep 26, 2025
06df14b
test: add more TRUC reorg coverge
instagibbs Sep 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/test/fuzz/package_eval.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ FUZZ_TARGET(ephemeral_package_eval, .init = initialize_tx_pool)
return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/single_submit, /*client_maxfeerate=*/{}));

const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, txs.back(), GetTime(),
/*bypass_limits=*/fuzzed_data_provider.ConsumeBool(), /*test_accept=*/!single_submit));
/*bypass_limits=*/false, /*test_accept=*/!single_submit));

if (!single_submit && result_package.m_state.GetResult() != PackageValidationResult::PCKG_POLICY) {
// We don't know anything about the validity since transactions were randomly generated, so
Expand Down
14 changes: 10 additions & 4 deletions src/test/fuzz/tx_pool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,6 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
std::set<CTransactionRef> added;
auto txr = std::make_shared<TransactionsDelta>(removed, added);
node.validation_signals->RegisterSharedValidationInterface(txr);
const bool bypass_limits = fuzzed_data_provider.ConsumeBool();

// Make sure ProcessNewPackage on one transaction works.
// The result is not guaranteed to be the same as what is returned by ATMP.
Expand All @@ -311,7 +310,7 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID);
}

const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx, GetTime(), bypass_limits, /*test_accept=*/false));
const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx, GetTime(), /*bypass_limits=*/false, /*test_accept=*/false));
const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID;
node.validation_signals->SyncWithValidationInterfaceQueue();
node.validation_signals->UnregisterSharedValidationInterface(txr);
Expand Down Expand Up @@ -394,6 +393,9 @@ FUZZ_TARGET(tx_pool, .init = initialize_tx_pool)

chainstate.SetMempool(&tx_pool);

// If we ever bypass limits, do not do TRUC invariants checks
bool ever_bypassed_limits{false};

LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300)
{
const auto mut_tx = ConsumeTransaction(fuzzed_data_provider, txids);
Expand All @@ -412,13 +414,17 @@ FUZZ_TARGET(tx_pool, .init = initialize_tx_pool)
tx_pool.PrioritiseTransaction(txid, delta);
}

const bool bypass_limits{fuzzed_data_provider.ConsumeBool()};
ever_bypassed_limits |= bypass_limits;

const auto tx = MakeTransactionRef(mut_tx);
const bool bypass_limits = fuzzed_data_provider.ConsumeBool();
const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx, GetTime(), bypass_limits, /*test_accept=*/false));
const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID;
if (accepted) {
txids.push_back(tx->GetHash());
CheckMempoolTRUCInvariants(tx_pool);
if (!ever_bypassed_limits) {
CheckMempoolTRUCInvariants(tx_pool);
}
}
}
Finish(fuzzed_data_provider, tx_pool, chainstate);
Expand Down
42 changes: 22 additions & 20 deletions src/validation.cpp
View file
Open in desktop

@glozow glozow Sep 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -1044,26 +1044,28 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
// Even though just checking direct mempool parents for inheritance would be sufficient, we
// check using the full ancestor set here because it's more convenient to use what we have
// already calculated.
if (const auto err{SingleTRUCChecks(ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) {
// Single transaction contexts only.
if (args.m_allow_sibling_eviction && err->second != nullptr) {
// We should only be considering where replacement is considered valid as well.
Assume(args.m_allow_replacement);

// Potential sibling eviction. Add the sibling to our list of mempool conflicts to be
// included in RBF checks.
ws.m_conflicts.insert(err->second->GetHash());
// Adding the sibling to m_iters_conflicting here means that it doesn't count towards
// RBF Carve Out above. This is correct, since removing to-be-replaced transactions from
// the descendant count is done separately in SingleTRUCChecks for TRUC transactions.
ws.m_iters_conflicting.insert(m_pool.GetIter(err->second->GetHash()).value());
ws.m_sibling_eviction = true;
// The sibling will be treated as part of the to-be-replaced set in ReplacementChecks.
// Note that we are not checking whether it opts in to replaceability via BIP125 or TRUC
// (which is normally done in PreChecks). However, the only way a TRUC transaction can
// have a non-TRUC and non-BIP125 descendant is due to a reorg.
} else {
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "TRUC-violation", err->first);
if (!args.m_bypass_limits) {

@glozow glozow Sep 29, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (const auto err{SingleTRUCChecks(ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) {
// Single transaction contexts only.
if (args.m_allow_sibling_eviction && err->second != nullptr) {
// We should only be considering where replacement is considered valid as well.
Assume(args.m_allow_replacement);

// Potential sibling eviction. Add the sibling to our list of mempool conflicts to be
// included in RBF checks.
ws.m_conflicts.insert(err->second->GetHash());
// Adding the sibling to m_iters_conflicting here means that it doesn't count towards
// RBF Carve Out above. This is correct, since removing to-be-replaced transactions from
// the descendant count is done separately in SingleTRUCChecks for TRUC transactions.
ws.m_iters_conflicting.insert(m_pool.GetIter(err->second->GetHash()).value());
ws.m_sibling_eviction = true;
// The sibling will be treated as part of the to-be-replaced set in ReplacementChecks.
// Note that we are not checking whether it opts in to replaceability via BIP125 or TRUC
// (which is normally done in PreChecks). However, the only way a TRUC transaction can
// have a non-TRUC and non-BIP125 descendant is due to a reorg.
} else {
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "TRUC-violation", err->first);
}
}
}

Expand Down
33 changes: 23 additions & 10 deletions test/functional/mempool_truc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,23 +165,36 @@ def test_truc_replacement(self):
def test_truc_reorg(self):
node = self.nodes[0]
self.log.info("Test that, during a reorg, TRUC rules are not enforced")
tx_v2_block = self.wallet.send_self_transfer(from_node=node, version=2)

@glozow glozow Sep 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tx_v3_block = self.wallet.send_self_transfer(from_node=node, version=3)
tx_v3_block2 = self.wallet.send_self_transfer(from_node=node, version=3)
self.check_mempool([tx_v3_block["txid"], tx_v2_block["txid"], tx_v3_block2["txid"]])
self.check_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

@ismaelsadeeq ismaelsadeeq Oct 2, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tx_v3_block2 = self.wallet.create_self_transfer(version=3)

# 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"]]
block = self.generateblock(node, output="raw(42)", transactions=tx_to_mine)

block = self.generate(node, 1)
self.check_mempool([])

@ismaelsadeeq ismaelsadeeq Oct 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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)
self.check_mempool([tx_v2_from_v3["txid"], tx_v3_from_v2["txid"], tx_v3_child_large["txid"]])
node.invalidateblock(block[0])
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"]])
# This is needed because generate() will create the exact same block again.
node.reconsiderblock(block[0])
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"]])

# 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"]])

@cleanup(extra_args=["-limitdescendantsize=10"])
def test_nondefault_package_limits(self):
Expand Down
Loading