laantungir
/
bitcoin
mirror of https://github.com/bitcoin/bitcoin.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#33504: Mempool: Do not enforce TRUC checks on reorg 

06df14ba75 test: add more TRUC reorg coverge (Greg Sanders)
26e71c237d Mempool: Do not enforce TRUC checks on reorg (Greg Sanders)
bbe8e9063c fuzz: don't bypass_limits for most mempool harnesses (Greg Sanders)

Pull request description:

  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

ACKs for top commit:
  sdaftuar:
    ACK 06df14ba75
  glozow:
    ACK 06df14ba75
  ismaelsadeeq:
    Code review ACK 06df14ba75

Tree-SHA512: bdb6e4dd622ed8b0b11866263fff559fcca6e0ca1c56a884cca9ac4572f0026528a63a9f4c8a0660df2f5efe0766310a30e5df1d6c560f31e4324ea5d4b3c1a8
This commit is contained in:
merge-script 2025-10-02 13:22:22 +01:00
parent 75353a0163 06df14ba75
commit 1ed00a0d39
No known key found for this signature in database
GPG Key ID: 2EEB9F5CC09526C1
4 changed files with 55 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/test/fuzz/package_eval.cpp
View File

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

14
src/test/fuzz/tx_pool.cpp
View File

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

40
src/validation.cpp
View File

@ -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);
if (!args.m_bypass_limits) {
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);
// 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);
}
}
}

33
test/functional/mempool_truc.py
View File

@ -165,23 +165,36 @@ class MempoolTRUC(BitcoinTestFramework):
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)
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
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([])
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):