From 0a79eaba729e60a83b0e604e6a18e9ba1ca1bc88 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 20 Jul 2021 10:41:00 +0100 Subject: [PATCH 01/10] [validation] case-based constructors for ATMPArgs No change in behavior. ATMPArgs can continue to have granular rules like switching BIP125 on/off while we create an interface for the different sets of rules for single transactions vs multiple-testmempoolaccept vs package validation. This is a cleaner interface than manually constructing the args, which makes it easy to mix up ordering, use the wrong default, etc. It also means we don't need to edit ATMP/single transaction validation code every time we update ATMPArgs for package validation. --- src/validation.cpp | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 207cdc8233c..66b65c320d1 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -450,7 +450,36 @@ public: /** Whether we allow transactions to replace mempool transactions by BIP125 rules. If false, * any transaction spending the same inputs as a transaction in the mempool is considered * a conflict. */ - const bool m_allow_bip125_replacement{true}; + const bool m_allow_bip125_replacement; + + /** Parameters for single transaction mempool validation. */ + static ATMPArgs SingleAccept(const CChainParams& chainparams, int64_t accept_time, + bool bypass_limits, std::vector& coins_to_uncache, + bool test_accept) { + return ATMPArgs{/* m_chainparams */ chainparams, + /* m_accept_time */ accept_time, + /* m_bypass_limits */ bypass_limits, + /* m_coins_to_uncache */ coins_to_uncache, + /* m_test_accept */ test_accept, + /* m_allow_bip125_replacement */ true, + }; + } + + /** Parameters for test package mempool validation through testmempoolaccept. */ + static ATMPArgs PackageTestAccept(const CChainParams& chainparams, int64_t accept_time, + std::vector& coins_to_uncache) { + return ATMPArgs{/* m_chainparams */ chainparams, + /* m_accept_time */ accept_time, + /* m_bypass_limits */ false, + /* m_coins_to_uncache */ coins_to_uncache, + /* m_test_accept */ true, + /* m_allow_bip125_replacement */ false, + }; + } + + // No default ctor to avoid exposing details to clients and allowing the possibility of + // mixing up the order of the arguments. Use static functions above instead. + ATMPArgs() = delete; }; // Single transaction acceptance @@ -1019,9 +1048,7 @@ static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainp EXCLUSIVE_LOCKS_REQUIRED(cs_main) { std::vector coins_to_uncache; - MemPoolAccept::ATMPArgs args { chainparams, nAcceptTime, bypass_limits, coins_to_uncache, - test_accept, /* m_allow_bip125_replacement */ true }; - + auto args = MemPoolAccept::ATMPArgs::SingleAccept(chainparams, nAcceptTime, bypass_limits, coins_to_uncache, test_accept); const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args); if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { // Remove coins that were not present in the coins cache before calling @@ -1054,8 +1081,7 @@ PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTx std::vector coins_to_uncache; const CChainParams& chainparams = Params(); - MemPoolAccept::ATMPArgs args { chainparams, GetTime(), /* bypass_limits */ false, coins_to_uncache, - test_accept, /* m_allow_bip125_replacement */ false }; + auto args = MemPoolAccept::ATMPArgs::PackageTestAccept(chainparams, GetTime(), coins_to_uncache); const PackageMempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptMultipleTransactions(package, args); // Uncache coins pertaining to transactions that were not submitted to the mempool. From cbb3598b5ce2bea58a8cb1ad2167d7d1d079acf7 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 20 Jul 2021 11:38:44 +0100 Subject: [PATCH 02/10] [validation/refactor] store precomputed txdata in workspace We want to be able to re-use the precomputed transaction data between PolicyScriptChecks and ConsensusScriptChecks in AcceptMultipleTransactions. --- src/validation.cpp | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 66b65c320d1..841aaebe0e0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -513,6 +513,9 @@ private: const CTransactionRef& m_ptx; const uint256& m_hash; TxValidationState m_state; + /** A temporary cache containing serialized transaction data for signature verification. + * Reused across PolicyScriptChecks and ConsensusScriptChecks. */ + PrecomputedTransactionData m_precomputed_txdata; }; // Run the policy checks on a given transaction, excluding any script checks. @@ -523,13 +526,13 @@ private: // Run the script checks using our policy flags. As this can be slow, we should // only invoke this on transactions that have otherwise passed policy checks. - bool PolicyScriptChecks(const ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + bool PolicyScriptChecks(const ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Re-run the script checks, using consensus flags, and try to cache the // result in the scriptcache. This should be done after // PolicyScriptChecks(). This requires that all inputs either be in our // utxo set or in the mempool. - bool ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws, PrecomputedTransactionData &txdata) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + bool ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Try to add the transaction to the mempool, removing any conflicts first. // Returns true if the transaction is in the mempool after any size @@ -842,7 +845,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return true; } -bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata) +bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws) { const CTransaction& tx = *ws.m_ptx; TxValidationState& state = ws.m_state; @@ -851,13 +854,13 @@ bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws, Prec // Check input scripts and signatures. // This is done last to help prevent CPU exhaustion denial-of-service attacks. - if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, true, false, txdata)) { + if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, true, false, ws.m_precomputed_txdata)) { // SCRIPT_VERIFY_CLEANSTACK requires SCRIPT_VERIFY_WITNESS, so we // need to turn both off, and compare against just turning off CLEANSTACK // to see if the failure is specifically due to witness validation. TxValidationState state_dummy; // Want reported failures to be from first CheckInputScripts - if (!tx.HasWitness() && CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, txdata) && - !CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, txdata)) { + if (!tx.HasWitness() && CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, ws.m_precomputed_txdata) && + !CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, ws.m_precomputed_txdata)) { // Only the witness is missing, so the transaction itself may be fine. state.Invalid(TxValidationResult::TX_WITNESS_STRIPPED, state.GetRejectReason(), state.GetDebugMessage()); @@ -868,7 +871,7 @@ bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws, Prec return true; } -bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata) +bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws) { const CTransaction& tx = *ws.m_ptx; const uint256& hash = ws.m_hash; @@ -891,7 +894,8 @@ bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws, P // invalid blocks (using TestBlockValidity), however allowing such // transactions into the mempool can be exploited as a DoS attack. unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(m_active_chainstate.m_chain.Tip(), chainparams.GetConsensus()); - if (!CheckInputsFromMempoolAndCache(tx, state, m_view, m_pool, currentBlockScriptVerifyFlags, txdata, m_active_chainstate.CoinsTip())) { + if (!CheckInputsFromMempoolAndCache(tx, state, m_view, m_pool, currentBlockScriptVerifyFlags, + ws.m_precomputed_txdata, m_active_chainstate.CoinsTip())) { return error("%s: BUG! PLEASE REPORT THIS! CheckInputScripts failed against latest-block but not STANDARD flags %s, %s", __func__, hash.ToString(), state.ToString()); } @@ -952,15 +956,11 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef if (!PreChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); - // Only compute the precomputed transaction data if we need to verify - // scripts (ie, other policy checks pass). We perform the inexpensive - // checks first and avoid hashing and signature verification unless those - // checks pass, to mitigate CPU exhaustion denial-of-service attacks. - PrecomputedTransactionData txdata; + // Perform the inexpensive checks first and avoid hashing and signature verification unless + // those checks pass, to mitigate CPU exhaustion denial-of-service attacks. + if (!PolicyScriptChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); - if (!PolicyScriptChecks(args, ws, txdata)) return MempoolAcceptResult::Failure(ws.m_state); - - if (!ConsensusScriptChecks(args, ws, txdata)) return MempoolAcceptResult::Failure(ws.m_state); + if (!ConsensusScriptChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); // Tx was accepted, but not added if (args.m_test_accept) { @@ -1020,8 +1020,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: } for (Workspace& ws : workspaces) { - PrecomputedTransactionData txdata; - if (!PolicyScriptChecks(args, ws, txdata)) { + if (!PolicyScriptChecks(args, ws)) { // Exit early to avoid doing pointless work. Update the failed tx result; the rest are unfinished. package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); From 8fa2936b34fda9c0bea963311fa80a04b4bf5867 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 28 Oct 2021 13:25:23 +0100 Subject: [PATCH 03/10] [validation] re-introduce bool for whether a transaction is RBF This bool was originally part of Workspace and was removed in #22539 when it was no longer needed in Finalize(). Re-introducing it because, once again, multiple functions will need to know whether we're doing an RBF. Member of MemPoolAccept so that we can use this to inform package RBF in the future. --- src/validation.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 841aaebe0e0..c32309b2149 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -568,6 +568,9 @@ private: // in-mempool conflicts; see below). size_t m_limit_descendants; size_t m_limit_descendant_size; + + /** Whether the transaction(s) would replace any mempool transactions. If so, RBF rules apply. */ + bool m_rbf{false}; }; bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) @@ -808,8 +811,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", *err_string); } - - if (!setConflicts.empty()) { + m_rbf = !setConflicts.empty(); + if (m_rbf) { CFeeRate newFeeRate(nModifiedFees, nSize); // It's possible that the replacement pays more fees than its direct conflicts but not more // than all conflicts (i.e. the direct conflicts have high-fee descendants). However, if the From 36a8441912bf84b4da9c74826dcd42533d8abaaa Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 2 Sep 2021 13:09:11 +0100 Subject: [PATCH 04/10] [validation/rpc] cache + use vsize calculated in PreChecks This is not only cleaner but also helps make sure we are always using the virtual size measure that includes the sigop weight heuristic (which is the vsize the mempool would return). --- src/rpc/rawtransaction.cpp | 2 +- src/validation.cpp | 21 +++++++++++++-------- src/validation.h | 10 ++++++---- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 483717aa7a6..fd18d4c96d7 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -977,7 +977,7 @@ static RPCHelpMan testmempoolaccept() if (tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID) { const CAmount fee = tx_result.m_base_fees.value(); // Check that fee does not exceed maximum fee - const int64_t virtual_size = GetVirtualTransactionSize(*tx); + const int64_t virtual_size = tx_result.m_vsize.value(); const CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size); if (max_raw_tx_fee && fee > max_raw_tx_fee) { result_inner.pushKV("allowed", false); diff --git a/src/validation.cpp b/src/validation.cpp index c32309b2149..266a0ac9e60 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -503,6 +503,9 @@ private: std::unique_ptr m_entry; std::list m_replaced_transactions; + /** Virtual size of the transaction as used by the mempool, calculated using serialized size + * of the transaction and sigops. */ + int64_t m_vsize; CAmount m_base_fees; CAmount m_modified_fees; /** Total modified fees of all transactions being replaced. */ @@ -732,7 +735,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), fSpendsCoinbase, nSigOpsCost, lp)); - unsigned int nSize = entry->GetTxSize(); + ws.m_vsize = entry->GetTxSize(); if (nSigOpsCost > MAX_STANDARD_TX_SIGOPS_COST) return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "bad-txns-too-many-sigops", @@ -740,7 +743,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // No transactions are allowed below minRelayTxFee except from disconnected // blocks - if (!bypass_limits && !CheckFeeRate(nSize, nModifiedFees, state)) return false; + if (!bypass_limits && !CheckFeeRate(ws.m_vsize, nModifiedFees, state)) return false; const CTxMemPool::setEntries setIterConflicting = m_pool.GetIterSet(setConflicts); // Calculate in-mempool ancestors, up to a limit. @@ -795,7 +798,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // to be secure by simply only having two immediately-spendable // outputs - one for each counterparty. For more info on the uses for // this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html - if (nSize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || + if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || !m_pool.CalculateMemPoolAncestors(*entry, setAncestors, 2, m_limit_ancestor_size, m_limit_descendants + 1, m_limit_descendant_size + EXTRA_DESCENDANT_TX_SIZE_LIMIT, dummy_err_string)) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", errString); } @@ -813,7 +816,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) m_rbf = !setConflicts.empty(); if (m_rbf) { - CFeeRate newFeeRate(nModifiedFees, nSize); + CFeeRate newFeeRate(nModifiedFees, ws.m_vsize); // It's possible that the replacement pays more fees than its direct conflicts but not more // than all conflicts (i.e. the direct conflicts have high-fee descendants). However, if the // replacement doesn't pay more fees than its direct conflicts, then we can be sure it's not @@ -841,7 +844,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) nConflictingFees += it->GetModifiedFee(); nConflictingSize += it->GetTxSize(); } - if (const auto err_string{PaysForRBF(nConflictingFees, nModifiedFees, nSize, ::incrementalRelayFee, hash)}) { + if (const auto err_string{PaysForRBF(nConflictingFees, nModifiedFees, ws.m_vsize, + ::incrementalRelayFee, hash)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); } } @@ -967,14 +971,14 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef // Tx was accepted, but not added if (args.m_test_accept) { - return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_base_fees); + return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees); } if (!Finalize(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); GetMainSignals().TransactionAddedToMempool(ptx, m_pool.GetAndIncrementSequence()); - return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_base_fees); + return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees); } PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::vector& txns, ATMPArgs& args) @@ -1033,7 +1037,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // When test_accept=true, transactions that pass PolicyScriptChecks are valid because there are // no further mempool checks (passing PolicyScriptChecks implies passing ConsensusScriptChecks). results.emplace(ws.m_ptx->GetWitnessHash(), - MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_base_fees)); + MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), + ws.m_vsize, ws.m_base_fees)); } } diff --git a/src/validation.h b/src/validation.h index 4da8ec8d24b..256981224ac 100644 --- a/src/validation.h +++ b/src/validation.h @@ -158,14 +158,16 @@ struct MempoolAcceptResult { // The following fields are only present when m_result_type = ResultType::VALID /** Mempool transactions replaced by the tx per BIP 125 rules. */ const std::optional> m_replaced_transactions; + /** Virtual size as used by the mempool, calculated using serialized size and sigops. */ + const std::optional m_vsize; /** Raw base fees in satoshis. */ const std::optional m_base_fees; static MempoolAcceptResult Failure(TxValidationState state) { return MempoolAcceptResult(state); } - static MempoolAcceptResult Success(std::list&& replaced_txns, CAmount fees) { - return MempoolAcceptResult(std::move(replaced_txns), fees); + static MempoolAcceptResult Success(std::list&& replaced_txns, int64_t vsize, CAmount fees) { + return MempoolAcceptResult(std::move(replaced_txns), vsize, fees); } // Private constructors. Use static methods MempoolAcceptResult::Success, etc. to construct. @@ -177,9 +179,9 @@ private: } /** Constructor for success case */ - explicit MempoolAcceptResult(std::list&& replaced_txns, CAmount fees) + explicit MempoolAcceptResult(std::list&& replaced_txns, int64_t vsize, CAmount fees) : m_result_type(ResultType::VALID), - m_replaced_transactions(std::move(replaced_txns)), m_base_fees(fees) {} + m_replaced_transactions(std::move(replaced_txns)), m_vsize{vsize}, m_base_fees(fees) {} }; /** From 3d3e4598b6e570b1f8248b1ee43ec59165a3ff5c Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 28 Oct 2021 16:09:33 +0100 Subject: [PATCH 05/10] [validation] cache iterators to mempool conflicts --- src/validation.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 266a0ac9e60..00a2b034115 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -498,6 +498,7 @@ private: struct Workspace { explicit Workspace(const CTransactionRef& ptx) : m_ptx(ptx), m_hash(ptx->GetHash()) {} std::set m_conflicts; + CTxMemPool::setEntries m_iters_conflicting; CTxMemPool::setEntries m_all_conflicting; CTxMemPool::setEntries m_ancestors; std::unique_ptr m_entry; @@ -745,7 +746,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // blocks if (!bypass_limits && !CheckFeeRate(ws.m_vsize, nModifiedFees, state)) return false; - const CTxMemPool::setEntries setIterConflicting = m_pool.GetIterSet(setConflicts); + ws.m_iters_conflicting = m_pool.GetIterSet(setConflicts); // Calculate in-mempool ancestors, up to a limit. if (setConflicts.size() == 1) { // In general, when we receive an RBF transaction with mempool conflicts, we want to know whether we @@ -775,8 +776,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // the ancestor limits should be the same for both our new transaction and any conflicts). // We don't bother incrementing m_limit_descendants by the full removal count as that limit never comes // into force here (as we're only adding a single transaction). - assert(setIterConflicting.size() == 1); - CTxMemPool::txiter conflict = *setIterConflicting.begin(); + assert(ws.m_iters_conflicting.size() == 1); + CTxMemPool::txiter conflict = *ws.m_iters_conflicting.begin(); m_limit_descendants += 1; m_limit_descendant_size += conflict->GetSizeWithDescendants(); @@ -823,17 +824,17 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // more economically rational to mine. Before we go digging through the mempool for all // transactions that would need to be removed (direct conflicts and all descendants), check // that the replacement transaction pays more than its direct conflicts. - if (const auto err_string{PaysMoreThanConflicts(setIterConflicting, newFeeRate, hash)}) { + if (const auto err_string{PaysMoreThanConflicts(ws.m_iters_conflicting, newFeeRate, hash)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); } // Calculate all conflicting entries and enforce BIP125 Rule #5. - if (const auto err_string{GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting)}) { + if (const auto err_string{GetEntriesForConflicts(tx, m_pool, ws.m_iters_conflicting, allConflicting)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too many potential replacements", *err_string); } // Enforce BIP125 Rule #2. - if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, setIterConflicting)}) { + if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, ws.m_iters_conflicting)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "replacement-adds-unconfirmed", *err_string); } From fd92b0c3986b9eb41ce28eb602f56d405bdd3cd7 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 4 Nov 2021 12:23:32 -0400 Subject: [PATCH 06/10] document workspace members --- src/validation.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 00a2b034115..c444b65f27c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -497,17 +497,29 @@ private: // of checking a given transaction. struct Workspace { explicit Workspace(const CTransactionRef& ptx) : m_ptx(ptx), m_hash(ptx->GetHash()) {} + /** Txids of mempool transactions that this transaction directly conflicts with. */ std::set m_conflicts; + /** Iterators to mempool entries that this transaction directly conflicts with. */ CTxMemPool::setEntries m_iters_conflicting; + /** Iterators to all mempool entries that would be replaced by this transaction, including + * those it directly conflicts with and their descendants. */ CTxMemPool::setEntries m_all_conflicting; + /** All mempool ancestors of this transaction. */ CTxMemPool::setEntries m_ancestors; + /** Mempool entry constructed for this transaction. Constructed in PreChecks() but not + * inserted into the mempool until Finalize(). */ std::unique_ptr m_entry; + /** Pointers to the transactions that have been removed from the mempool and replaced by + * this transaction, used to return to the MemPoolAccept caller. Only populated if + * validation is successful and the original transactions are removed. */ std::list m_replaced_transactions; /** Virtual size of the transaction as used by the mempool, calculated using serialized size * of the transaction and sigops. */ int64_t m_vsize; + /** Fees paid by this transaction: total input amounts subtracted by total output amounts. */ CAmount m_base_fees; + /** Base fees + any fee delta set by the user with prioritisetransaction. */ CAmount m_modified_fees; /** Total modified fees of all transactions being replaced. */ CAmount m_conflicting_fees{0}; @@ -515,6 +527,7 @@ private: size_t m_conflicting_size{0}; const CTransactionRef& m_ptx; + /** Txid. */ const uint256& m_hash; TxValidationState m_state; /** A temporary cache containing serialized transaction data for signature verification. From 9e910d8152e08d26ecce6592870adbe5dabd159e Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 3 Aug 2021 11:54:42 +0100 Subject: [PATCH 07/10] scripted-diff: clean up MemPoolAccept aliases The aliases are leftover from a previous MOVEONLY refactor - they are unnecessary and removing them reduces the diff for splitting out mempool Checks from PreChecks, making RBF variables MemPoolAccept-wide, etc. -BEGIN VERIFY SCRIPT- unalias() { sed -i "s:\<$1\>:$2:g" src/validation.cpp; sed -i "/$2 = $2/d" src/validation.cpp; } unalias nModifiedFees ws.m_modified_fees unalias nConflictingFees ws.m_conflicting_fees unalias nConflictingSize ws.m_conflicting_size unalias setConflicts ws.m_conflicts unalias allConflicting ws.m_all_conflicting unalias setAncestors ws.m_ancestors -END VERIFY SCRIPT- --- src/validation.cpp | 61 +++++++++++++++++++--------------------------- 1 file changed, 25 insertions(+), 36 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index c444b65f27c..138306a193a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -603,13 +603,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Alias what we need out of ws TxValidationState& state = ws.m_state; - std::set& setConflicts = ws.m_conflicts; - CTxMemPool::setEntries& allConflicting = ws.m_all_conflicting; - CTxMemPool::setEntries& setAncestors = ws.m_ancestors; std::unique_ptr& entry = ws.m_entry; - CAmount& nModifiedFees = ws.m_modified_fees; - CAmount& nConflictingFees = ws.m_conflicting_fees; - size_t& nConflictingSize = ws.m_conflicting_size; if (!CheckTransaction(tx, state)) { return false; // state filled in by CheckTransaction @@ -655,7 +649,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Transaction conflicts with a mempool tx, but we're not allowing replacements. return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "bip125-replacement-disallowed"); } - if (!setConflicts.count(ptxConflicting->GetHash())) + if (!ws.m_conflicts.count(ptxConflicting->GetHash())) { // Transactions that don't explicitly signal replaceability are // *not* replaceable with the current logic, even if one of their @@ -668,7 +662,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict"); } - setConflicts.insert(ptxConflicting->GetHash()); + ws.m_conflicts.insert(ptxConflicting->GetHash()); } } } @@ -732,9 +726,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) int64_t nSigOpsCost = GetTransactionSigOpCost(tx, m_view, STANDARD_SCRIPT_VERIFY_FLAGS); - // nModifiedFees includes any fee deltas from PrioritiseTransaction - nModifiedFees = ws.m_base_fees; - m_pool.ApplyDelta(hash, nModifiedFees); + // ws.m_modified_fees includes any fee deltas from PrioritiseTransaction + ws.m_modified_fees = ws.m_base_fees; + m_pool.ApplyDelta(hash, ws.m_modified_fees); // Keep track of transactions that spend a coinbase, which we re-scan // during reorgs to ensure COINBASE_MATURITY is still met. @@ -757,11 +751,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // No transactions are allowed below minRelayTxFee except from disconnected // blocks - if (!bypass_limits && !CheckFeeRate(ws.m_vsize, nModifiedFees, state)) return false; + if (!bypass_limits && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state)) return false; - ws.m_iters_conflicting = m_pool.GetIterSet(setConflicts); + ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts); // Calculate in-mempool ancestors, up to a limit. - if (setConflicts.size() == 1) { + if (ws.m_conflicts.size() == 1) { // In general, when we receive an RBF transaction with mempool conflicts, we want to know whether we // would meet the chain limits after the conflicts have been removed. However, there isn't a practical // way to do this short of calculating the ancestor and descendant sets with an overlay cache of @@ -797,8 +791,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } std::string errString; - if (!m_pool.CalculateMemPoolAncestors(*entry, setAncestors, m_limit_ancestors, m_limit_ancestor_size, m_limit_descendants, m_limit_descendant_size, errString)) { - setAncestors.clear(); + if (!m_pool.CalculateMemPoolAncestors(*entry, ws.m_ancestors, m_limit_ancestors, m_limit_ancestor_size, m_limit_descendants, m_limit_descendant_size, errString)) { + ws.m_ancestors.clear(); // If CalculateMemPoolAncestors fails second time, we want the original error string. std::string dummy_err_string; // Contracting/payment channels CPFP carve-out: @@ -813,24 +807,24 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // outputs - one for each counterparty. For more info on the uses for // this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || - !m_pool.CalculateMemPoolAncestors(*entry, setAncestors, 2, m_limit_ancestor_size, m_limit_descendants + 1, m_limit_descendant_size + EXTRA_DESCENDANT_TX_SIZE_LIMIT, dummy_err_string)) { + !m_pool.CalculateMemPoolAncestors(*entry, ws.m_ancestors, 2, m_limit_ancestor_size, m_limit_descendants + 1, m_limit_descendant_size + EXTRA_DESCENDANT_TX_SIZE_LIMIT, dummy_err_string)) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", errString); } } // A transaction that spends outputs that would be replaced by it is invalid. Now // that we have the set of all ancestors we can detect this - // pathological case by making sure setConflicts and setAncestors don't + // pathological case by making sure ws.m_conflicts and ws.m_ancestors don't // intersect. - if (const auto err_string{EntriesAndTxidsDisjoint(setAncestors, setConflicts, hash)}) { + if (const auto err_string{EntriesAndTxidsDisjoint(ws.m_ancestors, ws.m_conflicts, hash)}) { // We classify this as a consensus error because a transaction depending on something it // conflicts with would be inconsistent. return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", *err_string); } - m_rbf = !setConflicts.empty(); + m_rbf = !ws.m_conflicts.empty(); if (m_rbf) { - CFeeRate newFeeRate(nModifiedFees, ws.m_vsize); + CFeeRate newFeeRate(ws.m_modified_fees, ws.m_vsize); // It's possible that the replacement pays more fees than its direct conflicts but not more // than all conflicts (i.e. the direct conflicts have high-fee descendants). However, if the // replacement doesn't pay more fees than its direct conflicts, then we can be sure it's not @@ -842,7 +836,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } // Calculate all conflicting entries and enforce BIP125 Rule #5. - if (const auto err_string{GetEntriesForConflicts(tx, m_pool, ws.m_iters_conflicting, allConflicting)}) { + if (const auto err_string{GetEntriesForConflicts(tx, m_pool, ws.m_iters_conflicting, ws.m_all_conflicting)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too many potential replacements", *err_string); } @@ -854,11 +848,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Check if it's economically rational to mine this transaction rather than the ones it // replaces and pays for its own relay fees. Enforce BIP125 Rules #3 and #4. - for (CTxMemPool::txiter it : allConflicting) { - nConflictingFees += it->GetModifiedFee(); - nConflictingSize += it->GetTxSize(); + for (CTxMemPool::txiter it : ws.m_all_conflicting) { + ws.m_conflicting_fees += it->GetModifiedFee(); + ws.m_conflicting_size += it->GetTxSize(); } - if (const auto err_string{PaysForRBF(nConflictingFees, nModifiedFees, ws.m_vsize, + if (const auto err_string{PaysForRBF(ws.m_conflicting_fees, ws.m_modified_fees, ws.m_vsize, ::incrementalRelayFee, hash)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); } @@ -931,24 +925,19 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) TxValidationState& state = ws.m_state; const bool bypass_limits = args.m_bypass_limits; - CTxMemPool::setEntries& allConflicting = ws.m_all_conflicting; - CTxMemPool::setEntries& setAncestors = ws.m_ancestors; - const CAmount& nModifiedFees = ws.m_modified_fees; - const CAmount& nConflictingFees = ws.m_conflicting_fees; - const size_t& nConflictingSize = ws.m_conflicting_size; std::unique_ptr& entry = ws.m_entry; // Remove conflicting transactions from the mempool - for (CTxMemPool::txiter it : allConflicting) + for (CTxMemPool::txiter it : ws.m_all_conflicting) { LogPrint(BCLog::MEMPOOL, "replacing tx %s with %s for %s additional fees, %d delta bytes\n", it->GetTx().GetHash().ToString(), hash.ToString(), - FormatMoney(nModifiedFees - nConflictingFees), - (int)entry->GetTxSize() - (int)nConflictingSize); + FormatMoney(ws.m_modified_fees - ws.m_conflicting_fees), + (int)entry->GetTxSize() - (int)ws.m_conflicting_size); ws.m_replaced_transactions.push_back(it->GetSharedTx()); } - m_pool.RemoveStaged(allConflicting, false, MemPoolRemovalReason::REPLACED); + m_pool.RemoveStaged(ws.m_all_conflicting, false, MemPoolRemovalReason::REPLACED); // This transaction should only count for fee estimation if: // - it's not being re-added during a reorg which bypasses typical mempool fee limits @@ -957,7 +946,7 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) bool validForFeeEstimation = !bypass_limits && IsCurrentForFeeEstimation(m_active_chainstate) && m_pool.HasNoInputsOf(tx); // Store transaction in memory - m_pool.addUnchecked(*entry, setAncestors, validForFeeEstimation); + m_pool.addUnchecked(*entry, ws.m_ancestors, validForFeeEstimation); // trim mempool and check if tx was trimmed if (!bypass_limits) { From c9b1439ca9ab691f4672d2cbf33d9381f2985466 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 9 Aug 2021 11:56:57 +0100 Subject: [PATCH 08/10] MOVEONLY: mempool checks to their own functions No change in behavior, because package transactions would not be going through the rbf logic in PreChecks anyway (BIP125 is currently disabled for package acceptance, see ATMPArgs). We draw the line here because each individual transaction in package validation still goes through all PreChecks. For example, checking that one's own conflicts and dependencies are disjoint (a consensus check) and individual transaction mempool ancestor/descendant limits. --- src/validation.cpp | 103 +++++++++++++++++++++++++++++---------------- 1 file changed, 66 insertions(+), 37 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 138306a193a..9a3375fea95 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -541,6 +541,14 @@ private: // only tests that are fast should be done here (to avoid CPU DoS). bool PreChecks(ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + // Run checks for mempool replace-by-fee. + bool ReplacementChecks(Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + + // Enforce package mempool ancestor/descendant limits (distinct from individual + // ancestor/descendant limits done in PreChecks). + bool PackageMempoolChecks(const std::vector& txns, + PackageValidationState& package_state) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + // Run the script checks using our policy flags. As this can be slow, we should // only invoke this on transactions that have otherwise passed policy checks. bool PolicyScriptChecks(const ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); @@ -823,43 +831,67 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } m_rbf = !ws.m_conflicts.empty(); - if (m_rbf) { - CFeeRate newFeeRate(ws.m_modified_fees, ws.m_vsize); - // It's possible that the replacement pays more fees than its direct conflicts but not more - // than all conflicts (i.e. the direct conflicts have high-fee descendants). However, if the - // replacement doesn't pay more fees than its direct conflicts, then we can be sure it's not - // more economically rational to mine. Before we go digging through the mempool for all - // transactions that would need to be removed (direct conflicts and all descendants), check - // that the replacement transaction pays more than its direct conflicts. - if (const auto err_string{PaysMoreThanConflicts(ws.m_iters_conflicting, newFeeRate, hash)}) { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); - } + return true; +} - // Calculate all conflicting entries and enforce BIP125 Rule #5. - if (const auto err_string{GetEntriesForConflicts(tx, m_pool, ws.m_iters_conflicting, ws.m_all_conflicting)}) { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, - "too many potential replacements", *err_string); - } - // Enforce BIP125 Rule #2. - if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, ws.m_iters_conflicting)}) { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, - "replacement-adds-unconfirmed", *err_string); - } +bool MemPoolAccept::ReplacementChecks(Workspace& ws) +{ + AssertLockHeld(cs_main); + AssertLockHeld(m_pool.cs); - // Check if it's economically rational to mine this transaction rather than the ones it - // replaces and pays for its own relay fees. Enforce BIP125 Rules #3 and #4. - for (CTxMemPool::txiter it : ws.m_all_conflicting) { - ws.m_conflicting_fees += it->GetModifiedFee(); - ws.m_conflicting_size += it->GetTxSize(); - } - if (const auto err_string{PaysForRBF(ws.m_conflicting_fees, ws.m_modified_fees, ws.m_vsize, - ::incrementalRelayFee, hash)}) { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); - } + const CTransaction& tx = *ws.m_ptx; + const uint256& hash = ws.m_hash; + TxValidationState& state = ws.m_state; + + CFeeRate newFeeRate(ws.m_modified_fees, ws.m_vsize); + // It's possible that the replacement pays more fees than its direct conflicts but not more + // than all conflicts (i.e. the direct conflicts have high-fee descendants). However, if the + // replacement doesn't pay more fees than its direct conflicts, then we can be sure it's not + // more economically rational to mine. Before we go digging through the mempool for all + // transactions that would need to be removed (direct conflicts and all descendants), check + // that the replacement transaction pays more than its direct conflicts. + if (const auto err_string{PaysMoreThanConflicts(ws.m_iters_conflicting, newFeeRate, hash)}) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); + } + + // Calculate all conflicting entries and enforce BIP125 Rule #5. + if (const auto err_string{GetEntriesForConflicts(tx, m_pool, ws.m_iters_conflicting, ws.m_all_conflicting)}) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, + "too many potential replacements", *err_string); + } + // Enforce BIP125 Rule #2. + if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, ws.m_iters_conflicting)}) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, + "replacement-adds-unconfirmed", *err_string); + } + // Check if it's economically rational to mine this transaction rather than the ones it + // replaces and pays for its own relay fees. Enforce BIP125 Rules #3 and #4. + for (CTxMemPool::txiter it : ws.m_all_conflicting) { + ws.m_conflicting_fees += it->GetModifiedFee(); + ws.m_conflicting_size += it->GetTxSize(); + } + if (const auto err_string{PaysForRBF(ws.m_conflicting_fees, ws.m_modified_fees, ws.m_vsize, + ::incrementalRelayFee, hash)}) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); } return true; } +bool MemPoolAccept::PackageMempoolChecks(const std::vector& txns, + PackageValidationState& package_state) +{ + AssertLockHeld(cs_main); + AssertLockHeld(m_pool.cs); + + std::string err_string; + if (!m_pool.CheckPackageLimits(txns, m_limit_ancestors, m_limit_ancestor_size, m_limit_descendants, + m_limit_descendant_size, err_string)) { + // This is a package-wide error, separate from an individual transaction error. + return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", err_string); + } + return true; +} + bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws) { const CTransaction& tx = *ws.m_ptx; @@ -966,6 +998,8 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef if (!PreChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); + if (m_rbf && !ReplacementChecks(ws)) return MempoolAcceptResult::Failure(ws.m_state); + // Perform the inexpensive checks first and avoid hashing and signature verification unless // those checks pass, to mitigate CPU exhaustion denial-of-service attacks. if (!PolicyScriptChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); @@ -1020,12 +1054,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // because it's unnecessary. Also, CPFP carve out can increase the limit for individual // transactions, but this exemption is not extended to packages in CheckPackageLimits(). std::string err_string; - if (txns.size() > 1 && - !m_pool.CheckPackageLimits(txns, m_limit_ancestors, m_limit_ancestor_size, m_limit_descendants, - m_limit_descendant_size, err_string)) { - // All transactions must have individually passed mempool ancestor and descendant limits - // inside of PreChecks(), so this is separate from an individual transaction error. - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", err_string); + if (txns.size() > 1 && !PackageMempoolChecks(txns, package_state)) { return PackageMempoolAcceptResult(package_state, std::move(results)); } From 68763783658f004efd9117fa7a69b0e271c4eaaa Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 28 Oct 2021 12:54:19 +0100 Subject: [PATCH 09/10] MOVEONLY: move package unit tests to their own file --- src/Makefile.test.include | 1 + src/test/txpackage_tests.cpp | 113 ++++++++++++++++++++++++++++++++ src/test/txvalidation_tests.cpp | 95 --------------------------- 3 files changed, 114 insertions(+), 95 deletions(-) create mode 100644 src/test/txpackage_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 27f93826313..2b82a7428ef 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -139,6 +139,7 @@ BITCOIN_TESTS =\ test/transaction_tests.cpp \ test/txindex_tests.cpp \ test/txrequest_tests.cpp \ + test/txpackage_tests.cpp \ test/txvalidation_tests.cpp \ test/txvalidationcache_tests.cpp \ test/uint256_tests.cpp \ diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp new file mode 100644 index 00000000000..3d19e2d5d1d --- /dev/null +++ b/src/test/txpackage_tests.cpp @@ -0,0 +1,113 @@ +// Copyright (c) 2021 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