diff --git a/doc/policy/mempool-replacements.md b/doc/policy/mempool-replacements.md index b3c4239b73..96ab4112e2 100644 --- a/doc/policy/mempool-replacements.md +++ b/doc/policy/mempool-replacements.md @@ -11,7 +11,8 @@ their in-mempool descendants (together, "original transactions") if, in addition other consensus and policy rules, each of the following conditions are met: 1. The directly conflicting transactions all signal replaceability explicitly. A transaction is - signaling replaceability if any of its inputs have an nSequence number less than (0xffffffff - 1). + signaling BIP125 replaceability if any of its inputs have an nSequence number less than (0xffffffff - 1). + A transaction also signals replaceibility if its nVersion field is set to 3. *Rationale*: See [BIP125 explanation](https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki#motivation). diff --git a/src/Makefile.am b/src/Makefile.am index e452b42533..3e8870c828 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -242,6 +242,7 @@ BITCOIN_CORE_H = \ node/validation_cache_args.h \ noui.h \ outputtype.h \ + policy/v3_policy.h \ policy/feerate.h \ policy/fees.h \ policy/fees_args.h \ @@ -441,6 +442,7 @@ libbitcoin_node_a_SOURCES = \ node/utxo_snapshot.cpp \ node/validation_cache_args.cpp \ noui.cpp \ + policy/v3_policy.cpp \ policy/fees.cpp \ policy/fees_args.cpp \ policy/packages.cpp \ @@ -702,6 +704,7 @@ libbitcoin_common_a_SOURCES = \ netbase.cpp \ net_permissions.cpp \ outputtype.cpp \ + policy/v3_policy.cpp \ policy/feerate.cpp \ policy/policy.cpp \ protocol.cpp \ @@ -960,6 +963,7 @@ libbitcoinkernel_la_SOURCES = \ node/blockstorage.cpp \ node/chainstate.cpp \ node/utxo_snapshot.cpp \ + policy/v3_policy.cpp \ policy/feerate.cpp \ policy/packages.cpp \ policy/policy.cpp \ diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 76ab2b1a96..f0830d8f22 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -115,11 +115,11 @@ std::optional HasNoNewUnconfirmed(const CTransaction& tx, } std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors, - const std::set& direct_conflicts, + const std::set& direct_conflicts, const uint256& txid) { for (CTxMemPool::txiter ancestorIt : ancestors) { - const uint256& hashAncestor = ancestorIt->GetTx().GetHash(); + const Txid& hashAncestor = ancestorIt->GetTx().GetHash(); if (direct_conflicts.count(hashAncestor)) { return strprintf("%s spends conflicting transaction %s", txid.ToString(), diff --git a/src/policy/rbf.h b/src/policy/rbf.h index fff9828482..5a33ed64a3 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -80,7 +80,7 @@ std::optional HasNoNewUnconfirmed(const CTransaction& tx, const CTx * @returns error message if the sets intersect, std::nullopt if they are disjoint. */ std::optional EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors, - const std::set& direct_conflicts, + const std::set& direct_conflicts, const uint256& txid); /** Check that the feerate of the replacement transaction(s) is higher than the feerate of each diff --git a/src/policy/v3_policy.cpp b/src/policy/v3_policy.cpp new file mode 100644 index 0000000000..158881aeb9 --- /dev/null +++ b/src/policy/v3_policy.cpp @@ -0,0 +1,220 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include +#include +#include +#include +#include + +#include +#include +#include + +/** Helper for PackageV3Checks: Returns a vector containing the indices of transactions (within + * package) that are direct parents of ptx. */ +std::vector FindInPackageParents(const Package& package, const CTransactionRef& ptx) +{ + std::vector in_package_parents; + + std::set possible_parents; + for (auto &input : ptx->vin) { + possible_parents.insert(input.prevout.hash); + } + + for (size_t i{0}; i < package.size(); ++i) { + const auto& tx = package.at(i); + // We assume the package is sorted, so that we don't need to continue + // looking past the transaction itself. + if (&(*tx) == &(*ptx)) break; + if (possible_parents.count(tx->GetHash())) { + in_package_parents.push_back(i); + } + } + return in_package_parents; +} + +/** Helper for PackageV3Checks, storing info for a mempool or package parent. */ +struct ParentInfo { + /** Txid used to identify this parent by prevout */ + const Txid& m_txid; + /** Wtxid used for debug string */ + const Wtxid& m_wtxid; + /** nVersion used to check inheritance of v3 and non-v3 */ + decltype(CTransaction::nVersion) m_version; + /** If parent is in mempool, whether it has any descendants in mempool. */ + bool m_has_mempool_descendant; + + ParentInfo() = delete; + ParentInfo(const Txid& txid, const Wtxid& wtxid, decltype(CTransaction::nVersion) version, bool has_mempool_descendant) : + m_txid{txid}, m_wtxid{wtxid}, m_version{version}, + m_has_mempool_descendant{has_mempool_descendant} + {} +}; + +std::optional PackageV3Checks(const CTransactionRef& ptx, int64_t vsize, + const Package& package, + const CTxMemPool::setEntries& mempool_ancestors) +{ + // This function is specialized for these limits, and must be reimplemented if they ever change. + static_assert(V3_ANCESTOR_LIMIT == 2); + static_assert(V3_DESCENDANT_LIMIT == 2); + + const auto in_package_parents{FindInPackageParents(package, ptx)}; + + // Now we have all ancestors, so we can start checking v3 rules. + if (ptx->nVersion == 3) { + if (mempool_ancestors.size() + in_package_parents.size() + 1 > V3_ANCESTOR_LIMIT) { + return strprintf("tx %s (wtxid=%s) would have too many ancestors", + ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); + } + + const bool has_parent{mempool_ancestors.size() + in_package_parents.size() > 0}; + if (has_parent) { + // A v3 child cannot be too large. + if (vsize > V3_CHILD_MAX_VSIZE) { + return strprintf("v3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes", + ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), + vsize, V3_CHILD_MAX_VSIZE); + } + + const auto parent_info = [&] { + if (mempool_ancestors.size() > 0) { + // There's a parent in the mempool. + auto& mempool_parent = *mempool_ancestors.begin(); + Assume(mempool_parent->GetCountWithDescendants() == 1); + return ParentInfo{mempool_parent->GetTx().GetHash(), + mempool_parent->GetTx().GetWitnessHash(), + mempool_parent->GetTx().nVersion, + /*has_mempool_descendant=*/mempool_parent->GetCountWithDescendants() > 1}; + } else { + // Ancestor must be in the package. Find it. + auto& parent_index = in_package_parents.front(); + auto& package_parent = package.at(parent_index); + return ParentInfo{package_parent->GetHash(), + package_parent->GetWitnessHash(), + package_parent->nVersion, + /*has_mempool_descendant=*/false}; + } + }(); + + // If there is a parent, it must have the right version. + if (parent_info.m_version != 3) { + return strprintf("v3 tx %s (wtxid=%s) cannot spend from non-v3 tx %s (wtxid=%s)", + ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), + parent_info.m_txid.ToString(), parent_info.m_wtxid.ToString()); + } + + for (const auto& package_tx : package) { + // Skip same tx. + if (&(*package_tx) == &(*ptx)) continue; + + for (auto& input : package_tx->vin) { + // Fail if we find another tx with the same parent. We don't check whether the + // sibling is to-be-replaced (done in SingleV3Checks) because these transactions + // are within the same package. + if (input.prevout.hash == parent_info.m_txid) { + return strprintf("tx %s (wtxid=%s) would exceed descendant count limit", + parent_info.m_txid.ToString(), + parent_info.m_wtxid.ToString()); + } + + // This tx can't have both a parent and an in-package child. + if (input.prevout.hash == ptx->GetHash()) { + return strprintf("tx %s (wtxid=%s) would have too many ancestors", + package_tx->GetHash().ToString(), package_tx->GetWitnessHash().ToString()); + } + } + } + + // It shouldn't be possible to have any mempool siblings at this point. SingleV3Checks + // catches mempool siblings. Also, if the package consists of connected transactions, + // any tx having a mempool ancestor would mean the package exceeds ancestor limits. + if (!Assume(!parent_info.m_has_mempool_descendant)) { + return strprintf("tx %u would exceed descendant count limit", parent_info.m_wtxid.ToString()); + } + } + } else { + // Non-v3 transactions cannot have v3 parents. + for (auto it : mempool_ancestors) { + if (it->GetTx().nVersion == 3) { + return strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)", + ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), + it->GetSharedTx()->GetHash().ToString(), it->GetSharedTx()->GetWitnessHash().ToString()); + } + } + for (const auto& index: in_package_parents) { + if (package.at(index)->nVersion == 3) { + return strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)", + ptx->GetHash().ToString(), + ptx->GetWitnessHash().ToString(), + package.at(index)->GetHash().ToString(), + package.at(index)->GetWitnessHash().ToString()); + } + } + } + return std::nullopt; +} + +std::optional SingleV3Checks(const CTransactionRef& ptx, + const CTxMemPool::setEntries& mempool_ancestors, + const std::set& direct_conflicts, + int64_t vsize) +{ + // Check v3 and non-v3 inheritance. + for (const auto& entry : mempool_ancestors) { + if (ptx->nVersion != 3 && entry->GetTx().nVersion == 3) { + return strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)", + ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), + entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString()); + } else if (ptx->nVersion == 3 && entry->GetTx().nVersion != 3) { + return strprintf("v3 tx %s (wtxid=%s) cannot spend from non-v3 tx %s (wtxid=%s)", + ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), + entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString()); + } + } + + // This function is specialized for these limits, and must be reimplemented if they ever change. + static_assert(V3_ANCESTOR_LIMIT == 2); + static_assert(V3_DESCENDANT_LIMIT == 2); + + // The rest of the rules only apply to transactions with nVersion=3. + if (ptx->nVersion != 3) return std::nullopt; + + // Check that V3_ANCESTOR_LIMIT would not be violated, including both in-package and in-mempool. + if (mempool_ancestors.size() + 1 > V3_ANCESTOR_LIMIT) { + return strprintf("tx %s (wtxid=%s) would have too many ancestors", + ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); + } + + // Remaining checks only pertain to transactions with unconfirmed ancestors. + if (mempool_ancestors.size() > 0) { + // If this transaction spends V3 parents, it cannot be too large. + if (vsize > V3_CHILD_MAX_VSIZE) { + return strprintf("v3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes", + ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, V3_CHILD_MAX_VSIZE); + } + + // Check the descendant counts of in-mempool ancestors. + const auto& parent_entry = *mempool_ancestors.begin(); + // If there are any ancestors, this is the only child allowed. The parent cannot have any + // other descendants. We handle the possibility of multiple children as that case is + // possible through a reorg. + const auto& children = parent_entry->GetMemPoolChildrenConst(); + // Don't double-count a transaction that is going to be replaced. This logic assumes that + // any descendant of the V3 transaction is a direct child, which makes sense because a V3 + // transaction can only have 1 descendant. + const bool child_will_be_replaced = !children.empty() && + std::any_of(children.cbegin(), children.cend(), + [&direct_conflicts](const CTxMemPoolEntry& child){return direct_conflicts.count(child.GetTx().GetHash()) > 0;}); + if (parent_entry->GetCountWithDescendants() + 1 > V3_DESCENDANT_LIMIT && !child_will_be_replaced) { + return strprintf("tx %u (wtxid=%s) would exceed descendant count limit", + parent_entry->GetSharedTx()->GetHash().ToString(), + parent_entry->GetSharedTx()->GetWitnessHash().ToString()); + } + } + return std::nullopt; +} diff --git a/src/policy/v3_policy.h b/src/policy/v3_policy.h new file mode 100644 index 0000000000..9e871915e5 --- /dev/null +++ b/src/policy/v3_policy.h @@ -0,0 +1,83 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_POLICY_V3_POLICY_H +#define BITCOIN_POLICY_V3_POLICY_H + +#include +#include +#include +#include +#include +#include + +#include +#include + +// This module enforces rules for transactions with nVersion=3 ("v3 transactions") which help make +// RBF abilities more robust. + +// v3 only allows 1 parent and 1 child when unconfirmed. +/** Maximum number of transactions including an unconfirmed tx and its descendants. */ +static constexpr unsigned int V3_DESCENDANT_LIMIT{2}; +/** Maximum number of transactions including a V3 tx and all its mempool ancestors. */ +static constexpr unsigned int V3_ANCESTOR_LIMIT{2}; + +/** Maximum sigop-adjusted virtual size of a tx which spends from an unconfirmed v3 transaction. */ +static constexpr int64_t V3_CHILD_MAX_VSIZE{1000}; +// These limits are within the default ancestor/descendant limits. +static_assert(V3_CHILD_MAX_VSIZE + MAX_STANDARD_TX_WEIGHT / WITNESS_SCALE_FACTOR <= DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1000); +static_assert(V3_CHILD_MAX_VSIZE + MAX_STANDARD_TX_WEIGHT / WITNESS_SCALE_FACTOR <= DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * 1000); + +/** Must be called for every transaction, even if not v3. Not strictly necessary for transactions + * accepted through AcceptMultipleTransactions. + * + * Checks the following rules: + * 1. A v3 tx must only have v3 unconfirmed ancestors. + * 2. A non-v3 tx must only have non-v3 unconfirmed ancestors. + * 3. A v3's ancestor set, including itself, must be within V3_ANCESTOR_LIMIT. + * 4. A v3's descendant set, including itself, must be within V3_DESCENDANT_LIMIT. + * 5. If a v3 tx has any unconfirmed ancestors, the tx's sigop-adjusted vsize must be within + * V3_CHILD_MAX_VSIZE. + * + * + * @param[in] mempool_ancestors The in-mempool ancestors of ptx. + * @param[in] direct_conflicts In-mempool transactions this tx conflicts with. These conflicts + * are used to more accurately calculate the resulting descendant + * count of in-mempool ancestors. + * @param[in] vsize The sigop-adjusted virtual size of ptx. + * + * @returns debug string if an error occurs, std::nullopt otherwise. + */ +std::optional SingleV3Checks(const CTransactionRef& ptx, + const CTxMemPool::setEntries& mempool_ancestors, + const std::set& direct_conflicts, + int64_t vsize); + +/** Must be called for every transaction that is submitted within a package, even if not v3. + * + * For each transaction in a package: + * If it's not a v3 transaction, verify it has no direct v3 parents in the mempool or the package. + + * If it is a v3 transaction, verify that any direct parents in the mempool or the package are v3. + * If such a parent exists, verify that parent has no other children in the package or the mempool, + * and that the transaction itself has no children in the package. + * + * If any v3 violations in the package exist, this test will fail for one of them: + * - if a v3 transaction T has a parent in the mempool and a child in the package, then PV3C(T) will fail + * - if a v3 transaction T has a parent in the package and a child in the package, then PV3C(T) will fail + * - if a v3 transaction T and a v3 (sibling) transaction U have some parent in the mempool, + * then PV3C(T) and PV3C(U) will fail + * - if a v3 transaction T and a v3 (sibling) transaction U have some parent in the package, + * then PV3C(T) and PV3C(U) will fail + * - if a v3 transaction T has a parent P and a grandparent G in the package, then + * PV3C(P) will fail (though PV3C(G) and PV3C(T) might succeed). + * + * @returns debug string if an error occurs, std::nullopt otherwise. + * */ +std::optional PackageV3Checks(const CTransactionRef& ptx, int64_t vsize, + const Package& package, + const CTxMemPool::setEntries& mempool_ancestors); + +#endif // BITCOIN_POLICY_V3_POLICY_H diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 04d2e68939..c1753a1f6e 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -199,7 +199,7 @@ static RPCHelpMan testmempoolaccept() result_inner.pushKV("txid", tx->GetHash().GetHex()); result_inner.pushKV("wtxid", tx->GetWitnessHash().GetHex()); if (package_result.m_state.GetResult() == PackageValidationResult::PCKG_POLICY) { - result_inner.pushKV("package-error", package_result.m_state.GetRejectReason()); + result_inner.pushKV("package-error", package_result.m_state.ToString()); } auto it = package_result.m_tx_results.find(tx->GetWitnessHash()); if (exit_early || it == package_result.m_tx_results.end()) { @@ -907,7 +907,7 @@ static RPCHelpMan submitpackage() case PackageValidationResult::PCKG_TX: { // Package-wide error we want to return, but we also want to return individual responses - package_msg = package_result.m_state.GetRejectReason(); + package_msg = package_result.m_state.ToString(); CHECK_NONFATAL(package_result.m_tx_results.size() == txns.size() || package_result.m_tx_results.empty()); break; diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 5a08d0ff44..9e658e0ced 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -119,7 +120,8 @@ CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeConte mempool_opts.limits.descendant_size_vbytes = fuzzed_data_provider.ConsumeIntegralInRange(0, 202) * 1'000; mempool_opts.max_size_bytes = fuzzed_data_provider.ConsumeIntegralInRange(0, 200) * 1'000'000; mempool_opts.expiry = std::chrono::hours{fuzzed_data_provider.ConsumeIntegralInRange(0, 999)}; - nBytesPerSigOp = fuzzed_data_provider.ConsumeIntegralInRange(1, 999); + // Only interested in 2 cases: sigop cost 0 or when single legacy sigop cost is >> 1KvB + nBytesPerSigOp = fuzzed_data_provider.ConsumeIntegralInRange(0, 1) * 10'000; mempool_opts.check_ratio = 1; mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool(); @@ -171,11 +173,11 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) // Create transaction to add to the mempool const CTransactionRef tx = [&] { CMutableTransaction tx_mut; - tx_mut.nVersion = CTransaction::CURRENT_VERSION; + tx_mut.nVersion = fuzzed_data_provider.ConsumeBool() ? 3 : CTransaction::CURRENT_VERSION; tx_mut.nLockTime = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral(); // Last tx will sweep all outpoints in package const auto num_in = last_tx ? package_outpoints.size() : fuzzed_data_provider.ConsumeIntegralInRange(1, mempool_outpoints.size()); - const auto num_out = fuzzed_data_provider.ConsumeIntegralInRange(1, mempool_outpoints.size() * 2); + auto num_out = fuzzed_data_provider.ConsumeIntegralInRange(1, mempool_outpoints.size() * 2); auto& outpoints = last_tx ? package_outpoints : mempool_outpoints; @@ -211,17 +213,24 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) tx_mut.vin.push_back(tx_mut.vin.back()); } - // Refer to a non-existant input + // Refer to a non-existent input if (fuzzed_data_provider.ConsumeBool()) { tx_mut.vin.emplace_back(); } + // Make a p2pk output to make sigops adjusted vsize to violate v3, potentially, which is never spent + if (last_tx && amount_in > 1000 && fuzzed_data_provider.ConsumeBool()) { + tx_mut.vout.emplace_back(1000, CScript() << std::vector(33, 0x02) << OP_CHECKSIG); + // Don't add any other outputs. + num_out = 1; + amount_in -= 1000; + } + const auto amount_fee = fuzzed_data_provider.ConsumeIntegralInRange(0, amount_in); const auto amount_out = (amount_in - amount_fee) / num_out; for (int i = 0; i < num_out; ++i) { tx_mut.vout.emplace_back(amount_out, P2WSH_EMPTY); } - // TODO vary transaction sizes to catch size-related issues auto tx = MakeTransactionRef(tx_mut); // Restore previously removed outpoints, except in-package outpoints if (!last_tx) { @@ -261,7 +270,6 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) std::set added; auto txr = std::make_shared(added); RegisterSharedValidationInterface(txr); - const bool bypass_limits = fuzzed_data_provider.ConsumeBool(); // When there are multiple transactions in the package, we call ProcessNewPackage(txs, test_accept=false) // and AcceptToMemoryPool(txs.back(), test_accept=true). When there is only 1 transaction, we might flip it @@ -271,17 +279,20 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) const auto result_package = WITH_LOCK(::cs_main, return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/single_submit)); - const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, txs.back(), GetTime(), bypass_limits, /*test_accept=*/!single_submit)); - const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID; + // Always set bypass_limits to false because it is not supported in ProcessNewPackage and + // can be a source of divergence. + const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, txs.back(), GetTime(), + /*bypass_limits=*/false, /*test_accept=*/!single_submit)); + const bool passed = res.m_result_type == MempoolAcceptResult::ResultType::VALID; SyncWithValidationInterfaceQueue(); UnregisterSharedValidationInterface(txr); // There is only 1 transaction in the package. We did a test-package-accept and a ATMP if (single_submit) { - Assert(accepted != added.empty()); - Assert(accepted == res.m_state.IsValid()); - if (accepted) { + Assert(passed != added.empty()); + Assert(passed == res.m_state.IsValid()); + if (passed) { Assert(added.size() == 1); Assert(txs.back() == *added.begin()); } @@ -295,6 +306,8 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) // This is empty if it fails early checks, or "full" if transactions are looked at deeper Assert(result_package.m_tx_results.size() == txs.size() || result_package.m_tx_results.empty()); } + + CheckMempoolV3Invariants(tx_pool); } UnregisterSharedValidationInterface(outpoints_updater); diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 8c0b0d7029..b44b528b6f 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -227,7 +228,7 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool) // Create transaction to add to the mempool const CTransactionRef tx = [&] { CMutableTransaction tx_mut; - tx_mut.nVersion = CTransaction::CURRENT_VERSION; + tx_mut.nVersion = fuzzed_data_provider.ConsumeBool() ? 3 : CTransaction::CURRENT_VERSION; tx_mut.nLockTime = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral(); const auto num_in = fuzzed_data_provider.ConsumeIntegralInRange(1, outpoints_rbf.size()); const auto num_out = fuzzed_data_provider.ConsumeIntegralInRange(1, outpoints_rbf.size() * 2); @@ -313,6 +314,7 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool) if (accepted) { Assert(added.size() == 1); // For now, no package acceptance Assert(tx == *added.begin()); + CheckMempoolV3Invariants(tx_pool); } else { // Do not consider rejected transaction removed removed.erase(tx); @@ -405,6 +407,9 @@ FUZZ_TARGET(tx_pool, .init = initialize_tx_pool) const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID; if (accepted) { txids.push_back(tx->GetHash()); + // Only check fees if accepted and not bypass_limits, otherwise it's not guaranteed that + // trimming has happened for this tx and previous iterations. + CheckMempoolV3Invariants(tx_pool); } } Finish(fuzzed_data_provider, tx_pool, chainstate); diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp index fb6a3614c0..e6c135eed9 100644 --- a/src/test/rbf_tests.cpp +++ b/src/test/rbf_tests.cpp @@ -135,8 +135,6 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup) // Tests for EntriesAndTxidsDisjoint BOOST_CHECK(EntriesAndTxidsDisjoint(empty_set, {tx1->GetHash()}, unused_txid) == std::nullopt); BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx3->GetHash()}, unused_txid) == std::nullopt); - // EntriesAndTxidsDisjoint uses txids, not wtxids. - BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx2->GetWitnessHash()}, unused_txid) == std::nullopt); BOOST_CHECK(EntriesAndTxidsDisjoint({entry2}, {tx2->GetHash()}, unused_txid).has_value()); BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx1->GetHash()}, unused_txid).has_value()); BOOST_CHECK(EntriesAndTxidsDisjoint(set_12_normal, {tx2->GetHash()}, unused_txid).has_value()); diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index d1cb2531aa..e6cf64611e 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -786,7 +786,7 @@ BOOST_AUTO_TEST_CASE(test_IsStandard) t.nVersion = 0; CheckIsNotStandard(t, "version"); - t.nVersion = 3; + t.nVersion = TX_MAX_STANDARD_VERSION + 1; CheckIsNotStandard(t, "version"); // Allowed nVersion diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index ecf0889711..98d5e892f9 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -4,11 +4,14 @@ #include #include +#include #include #include #include +#include #include