diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 23ea9d1f15a..50f4c416d79 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -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 diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 90155d389c0..c728485d041 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -296,7 +296,6 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool) std::set added; auto txr = std::make_shared(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); diff --git a/src/validation.cpp b/src/validation.cpp index 8fcc719a684..3f77955da62 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -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); + } } } diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py index 562afcd34dd..098631b5c41 100755 --- a/test/functional/mempool_truc.py +++ b/test/functional/mempool_truc.py @@ -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):