From 57ee3029ddadaee5cb48224e0a87f704b7971bd8 Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Mon, 20 May 2024 09:40:03 -0400 Subject: [PATCH 1/5] Add description for m_test_accept --- src/validation.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/validation.cpp b/src/validation.cpp index 1d9d5ae753201..6e06d0c07cd30 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -455,6 +455,7 @@ class MemPoolAccept * the mempool. */ std::vector& m_coins_to_uncache; + /** When true, the transaction or package will not be submitted to the mempool. */ const bool m_test_accept; /** 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 From 69f7ab05bafec1cf06fd7a58351f78e32bbfa2cf Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Mon, 20 May 2024 09:56:25 -0400 Subject: [PATCH 2/5] Add m_allow_sibling_eviction as separate ATMPArgs flag --- src/validation.cpp | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 6e06d0c07cd30..0aa28e279987b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -457,10 +457,12 @@ class MemPoolAccept std::vector& m_coins_to_uncache; /** When true, the transaction or package will not be submitted to the mempool. */ const bool m_test_accept; - /** Whether we allow transactions to replace mempool transactions by BIP125 rules. If false, + /** Whether we allow transactions to replace mempool transactions. If false, * any transaction spending the same inputs as a transaction in the mempool is considered * a conflict. */ const bool m_allow_replacement; + /** When true, allow sibling eviction. This only occurs in single transaction package settings. */ + const bool m_allow_sibling_eviction; /** When true, the mempool will not be trimmed when any transactions are submitted in * Finalize(). Instead, limits should be enforced at the end to ensure the package is not * partially submitted. @@ -486,6 +488,7 @@ class MemPoolAccept /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ test_accept, /* m_allow_replacement */ true, + /* m_allow_sibling_eviction */ true, /* m_package_submission */ false, /* m_package_feerates */ false, /* m_client_maxfeerate */ {}, // checked by caller @@ -501,6 +504,7 @@ class MemPoolAccept /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ true, /* m_allow_replacement */ false, + /* m_allow_sibling_eviction */ false, /* m_package_submission */ false, // not submitting to mempool /* m_package_feerates */ false, /* m_client_maxfeerate */ {}, // checked by caller @@ -516,6 +520,7 @@ class MemPoolAccept /* m_coins_to_uncache */ coins_to_uncache, /* m_test_accept */ false, /* m_allow_replacement */ false, + /* m_allow_sibling_eviction */ false, /* m_package_submission */ true, /* m_package_feerates */ true, /* m_client_maxfeerate */ client_maxfeerate, @@ -530,6 +535,7 @@ class MemPoolAccept /* m_coins_to_uncache */ package_args.m_coins_to_uncache, /* m_test_accept */ package_args.m_test_accept, /* m_allow_replacement */ true, + /* m_allow_sibling_eviction */ true, /* m_package_submission */ true, // do not LimitMempoolSize in Finalize() /* m_package_feerates */ false, // only 1 transaction /* m_client_maxfeerate */ package_args.m_client_maxfeerate, @@ -545,6 +551,7 @@ class MemPoolAccept std::vector& coins_to_uncache, bool test_accept, bool allow_replacement, + bool allow_sibling_eviction, bool package_submission, bool package_feerates, std::optional client_maxfeerate) @@ -554,6 +561,7 @@ class MemPoolAccept m_coins_to_uncache{coins_to_uncache}, m_test_accept{test_accept}, m_allow_replacement{allow_replacement}, + m_allow_sibling_eviction{allow_sibling_eviction}, m_package_submission{package_submission}, m_package_feerates{package_feerates}, m_client_maxfeerate{client_maxfeerate} @@ -981,8 +989,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // check using the full ancestor set here because it's more convenient to use what we have // already calculated. if (const auto err{SingleV3Checks(ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) { - // Disabled within package validation. - if (err->second != nullptr && args.m_allow_replacement) { + // 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()); From cbbfe719b223b9e05398227cef68c99eb97670bd Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 3 Nov 2022 12:36:12 +0000 Subject: [PATCH 3/5] cpfp carveout is excluded in packages The behavior is not new, but this rule exits earlier than before. Previously, a carve out could have been granted in PreChecks() but then nullified in PackageMempoolChecks() when CheckPackageLimits() is called with the default limits. --- doc/policy/packages.md | 9 ++++++-- src/validation.cpp | 27 ++++++++++++++++------ test/functional/mempool_package_onemore.py | 19 +++++++++++---- 3 files changed, 41 insertions(+), 14 deletions(-) diff --git a/doc/policy/packages.md b/doc/policy/packages.md index dba270e494067..7e983221c5f39 100644 --- a/doc/policy/packages.md +++ b/doc/policy/packages.md @@ -48,8 +48,13 @@ The following rules are enforced for all packages: heavily connected, i.e. some transaction in the package is the ancestor or descendant of all the other transactions. -The following rules are only enforced for packages to be submitted to the mempool (not enforced for -test accepts): +* [CPFP Carve Out](./mempool-limits.md#CPFP-Carve-Out) is disabled in packaged contexts. (#21800) + + - *Rationale*: This carve out cannot be accurately applied when there are multiple transactions' + ancestors and descendants being considered at the same time. + +The following rules are only enforced for packages to be submitted to the mempool (not +enforced for test accepts): * Packages must be child-with-unconfirmed-parents packages. This also means packages must contain at least 2 transactions. (#22674) diff --git a/src/validation.cpp b/src/validation.cpp index 0aa28e279987b..3346ca0326383 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -478,6 +478,9 @@ class MemPoolAccept */ const std::optional m_client_maxfeerate; + /** Whether CPFP carveout and RBF carveout are granted. */ + const bool m_allow_carveouts; + /** Parameters for single transaction mempool validation. */ static ATMPArgs SingleAccept(const CChainParams& chainparams, int64_t accept_time, bool bypass_limits, std::vector& coins_to_uncache, @@ -492,6 +495,7 @@ class MemPoolAccept /* m_package_submission */ false, /* m_package_feerates */ false, /* m_client_maxfeerate */ {}, // checked by caller + /* m_allow_carveouts */ true, }; } @@ -508,6 +512,7 @@ class MemPoolAccept /* m_package_submission */ false, // not submitting to mempool /* m_package_feerates */ false, /* m_client_maxfeerate */ {}, // checked by caller + /* m_allow_carveouts */ false, }; } @@ -524,6 +529,7 @@ class MemPoolAccept /* m_package_submission */ true, /* m_package_feerates */ true, /* m_client_maxfeerate */ client_maxfeerate, + /* m_allow_carveouts */ false, }; } @@ -539,6 +545,7 @@ class MemPoolAccept /* m_package_submission */ true, // do not LimitMempoolSize in Finalize() /* m_package_feerates */ false, // only 1 transaction /* m_client_maxfeerate */ package_args.m_client_maxfeerate, + /* m_allow_carveouts */ false, }; } @@ -554,7 +561,8 @@ class MemPoolAccept bool allow_sibling_eviction, bool package_submission, bool package_feerates, - std::optional client_maxfeerate) + std::optional client_maxfeerate, + bool allow_carveouts) : m_chainparams{chainparams}, m_accept_time{accept_time}, m_bypass_limits{bypass_limits}, @@ -564,7 +572,8 @@ class MemPoolAccept m_allow_sibling_eviction{allow_sibling_eviction}, m_package_submission{package_submission}, m_package_feerates{package_feerates}, - m_client_maxfeerate{client_maxfeerate} + m_client_maxfeerate{client_maxfeerate}, + m_allow_carveouts{allow_carveouts} { } }; @@ -917,7 +926,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) CTxMemPool::Limits maybe_rbf_limits = m_pool.m_opts.limits; // Calculate in-mempool ancestors, up to a limit. - if (ws.m_conflicts.size() == 1) { + if (ws.m_conflicts.size() == 1 && args.m_allow_carveouts) { // 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 @@ -956,6 +965,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) ws.m_ancestors = std::move(*ancestors); } else { // If CalculateMemPoolAncestors fails second time, we want the original error string. + const auto error_message{util::ErrorString(ancestors).original}; + + // Carve-out is not allowed in this context; fail + if (!args.m_allow_carveouts) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); + } + // Contracting/payment channels CPFP carve-out: // If the new transaction is relatively small (up to 40k weight) // and has at most one ancestor (ie ancestor limit of 2, including @@ -974,7 +990,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) .descendant_count = maybe_rbf_limits.descendant_count + 1, .descendant_size_vbytes = maybe_rbf_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT, }; - const auto error_message{util::ErrorString(ancestors).original}; if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || ws.m_ptx->nVersion == 3) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); } @@ -1431,9 +1446,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: } // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, - // 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; + // because it's unnecessary. if (txns.size() > 1 && !PackageMempoolChecks(txns, m_total_vsize, package_state)) { return PackageMempoolAcceptResult(package_state, std::move(results)); } diff --git a/test/functional/mempool_package_onemore.py b/test/functional/mempool_package_onemore.py index 98b397e32bb02..8f58b671107c7 100755 --- a/test/functional/mempool_package_onemore.py +++ b/test/functional/mempool_package_onemore.py @@ -47,21 +47,30 @@ def run_test(self): # Adding one more transaction on to the chain should fail. assert_raises_rpc_error(-26, "too-long-mempool-chain, too many unconfirmed ancestors [limit: 25]", self.chain_tx, [utxo]) - # ...even if it chains on from some point in the middle of the chain. + # ... or if it chains on from some point in the middle of the chain. assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[2]]) assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[1]]) # ...even if it chains on to two parent transactions with one in the chain. assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[0], second_chain]) # ...especially if its > 40k weight assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[0]], num_outputs=350) + # ...even if it's submitted with other transactions + replaceable_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=[chain[0]]) + txns = [replaceable_tx["tx"], self.wallet.create_self_transfer_multi(utxos_to_spend=replaceable_tx["new_utxos"])["tx"]] + txns_hex = [tx.serialize().hex() for tx in txns] + assert_equal(self.nodes[0].testmempoolaccept(txns_hex)[0]["reject-reason"], "too-long-mempool-chain") + pkg_result = self.nodes[0].submitpackage(txns_hex) + assert "too-long-mempool-chain" in pkg_result["tx-results"][txns[0].getwtxid()]["error"] + assert_equal(pkg_result["tx-results"][txns[1].getwtxid()]["error"], "bad-txns-inputs-missingorspent") # But not if it chains directly off the first transaction - replacable_tx = self.wallet.send_self_transfer_multi(from_node=self.nodes[0], utxos_to_spend=[chain[0]])['tx'] + self.nodes[0].sendrawtransaction(replaceable_tx["hex"]) # and the second chain should work just fine self.chain_tx([second_chain]) - # Make sure we can RBF the chain which used our carve-out rule - replacable_tx.vout[0].nValue -= 1000000 - self.nodes[0].sendrawtransaction(replacable_tx.serialize().hex()) + # Ensure an individual transaction with single direct conflict can RBF the chain which used our carve-out rule + replacement_tx = replaceable_tx["tx"] + replacement_tx.vout[0].nValue -= 1000000 + self.nodes[0].sendrawtransaction(replacement_tx.serialize().hex()) # Finally, check that we added two transactions assert_equal(len(self.nodes[0].getrawmempool()), DEFAULT_ANCESTOR_LIMIT + 3) From 20d8936d8bb6137a5a3722d34e0d7ae756031009 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 3 Aug 2021 16:39:35 +0100 Subject: [PATCH 4/5] [refactor] make some members MemPoolAccept-wide No change in behavior. For single transaction acceptance, this is a simple refactor: Workspace::m_all_conflicting Workspace::m_conflicting_fees Workspace::m_conflicting_size Workspace::m_replaced_transactions are now grouped under a new SubPackageState struct that is a member of MemPoolAccept. And local variables m_total_vsize and m_total_modified_fees are now SubpackageState members so they can be accessed from PackageMempoolChecks. We want these to be package-wide variables because - Transactions could conflict with the same tx (just not the same prevout), or their conflicts could share descendants. - We want to compare conflicts with the package fee rather than individual transaction fee. We reset these MemPoolAccept-wide fields for each subpackage evaluation to not cause state leaking, similar to temporary coins. --- src/validation.cpp | 103 ++++++++++++++++++++++++++++----------------- 1 file changed, 64 insertions(+), 39 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 3346ca0326383..c712b89bf8213 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -621,18 +621,11 @@ class MemPoolAccept /** Iterators to mempool entries that this transaction directly conflicts with or may * replace via sibling eviction. */ CTxMemPool::setEntries m_iters_conflicting; - /** Iterators to all mempool entries that would be replaced by this transaction, including - * m_conflicts 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 (everything in m_all_conflicting), used to return to the MemPoolAccept caller. Only populated if - * validation is successful and the original transactions are removed. */ - std::list m_replaced_transactions; /** Whether RBF-related data structures (m_conflicts, m_iters_conflicting, m_all_conflicting, * m_replaced_transactions) include a sibling in addition to txns with conflicting inputs. */ bool m_sibling_eviction{false}; @@ -644,10 +637,6 @@ class MemPoolAccept 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}; - /** Total virtual size of all transactions being replaced. */ - size_t m_conflicting_size{0}; /** If we're doing package validation (i.e. m_package_feerates=true), the "effective" * package feerate of this transaction is the total fees divided by the total size of @@ -725,9 +714,39 @@ class MemPoolAccept Chainstate& m_active_chainstate; - /** Whether the transaction(s) would replace any mempool transactions and/or evict any siblings. - * If so, RBF rules apply. */ - bool m_rbf{false}; + // Fields below are per *sub*package state and must be reset prior to subsequent + // AcceptSingleTransaction and AcceptMultipleTransactions invocations + struct SubPackageState { + /** Aggregated modified fees of all transactions, used to calculate package feerate. */ + CAmount m_total_modified_fees{0}; + /** Aggregated virtual size of all transactions, used to calculate package feerate. */ + int64_t m_total_vsize{0}; + + // RBF-related members + /** Whether the transaction(s) would replace any mempool transactions and/or evict any siblings. + * If so, RBF rules apply. */ + bool m_rbf{false}; + /** All directly conflicting mempool transactions and their descendants. */ + CTxMemPool::setEntries m_all_conflicts; + /** Mempool transactions that were replaced. */ + std::list m_replaced_transactions; + + /** Total modified fees of mempool transactions being replaced. */ + CAmount m_conflicting_fees{0}; + /** Total size (in virtual bytes) of mempool transactions being replaced. */ + size_t m_conflicting_size{0}; + }; + + struct SubPackageState m_subpackage; + + /** Re-set sub-package state to not leak between evaluations */ + void ClearSubPackageState() EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs) + { + m_subpackage = SubPackageState{}; + + // And clean coins while at it + CleanupTemporaryCoins(); + } }; bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) @@ -1036,7 +1055,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", *err_string); } - m_rbf = !ws.m_conflicts.empty(); + // We want to detect conflicts in any tx in a package to trigger package RBF logic + m_subpackage.m_rbf |= !ws.m_conflicts.empty(); return true; } @@ -1068,24 +1088,25 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) } // Calculate all conflicting entries and enforce Rule #5. - if (const auto err_string{GetEntriesForConflicts(tx, m_pool, ws.m_iters_conflicting, ws.m_all_conflicting)}) { + if (const auto err_string{GetEntriesForConflicts(tx, m_pool, ws.m_iters_conflicting, m_subpackage.m_all_conflicts)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, strprintf("too many potential replacements%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } // Enforce Rule #2. - if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, ws.m_iters_conflicting)}) { + if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, m_subpackage.m_all_conflicts)}) { // Sibling eviction is only done for v3 transactions, which cannot have multiple ancestors. Assume(!ws.m_sibling_eviction); return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, strprintf("replacement-adds-unconfirmed%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *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 Rules #3 and #4. - for (CTxMemPool::txiter it : ws.m_all_conflicting) { - ws.m_conflicting_fees += it->GetModifiedFee(); - ws.m_conflicting_size += it->GetTxSize(); + for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts) { + m_subpackage.m_conflicting_fees += it->GetModifiedFee(); + m_subpackage.m_conflicting_size += it->GetTxSize(); } - if (const auto err_string{PaysForRBF(ws.m_conflicting_fees, ws.m_modified_fees, ws.m_vsize, + if (const auto err_string{PaysForRBF(m_subpackage.m_conflicting_fees, ws.m_modified_fees, ws.m_vsize, m_pool.m_opts.incremental_relay_feerate, hash)}) { // Even though this is a fee-related failure, this result is TX_MEMPOOL_POLICY, not // TX_RECONSIDERABLE, because it cannot be bypassed using package validation. @@ -1184,19 +1205,18 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) const uint256& hash = ws.m_hash; TxValidationState& state = ws.m_state; const bool bypass_limits = args.m_bypass_limits; - std::unique_ptr& entry = ws.m_entry; // Remove conflicting transactions from the mempool - for (CTxMemPool::txiter it : ws.m_all_conflicting) + for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts) { LogPrint(BCLog::MEMPOOL, "replacing tx %s (wtxid=%s) with %s (wtxid=%s) for %s additional fees, %d delta bytes\n", it->GetTx().GetHash().ToString(), it->GetTx().GetWitnessHash().ToString(), hash.ToString(), tx.GetWitnessHash().ToString(), - FormatMoney(ws.m_modified_fees - ws.m_conflicting_fees), - (int)entry->GetTxSize() - (int)ws.m_conflicting_size); + FormatMoney(ws.m_modified_fees - m_subpackage.m_conflicting_fees), + (int)entry->GetTxSize() - (int)m_subpackage.m_conflicting_size); TRACE7(mempool, replaced, it->GetTx().GetHash().data(), it->GetTxSize(), @@ -1206,9 +1226,12 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) entry->GetTxSize(), entry->GetFee() ); - ws.m_replaced_transactions.push_back(it->GetSharedTx()); + m_subpackage.m_replaced_transactions.push_back(it->GetSharedTx()); } - m_pool.RemoveStaged(ws.m_all_conflicting, false, MemPoolRemovalReason::REPLACED); + m_pool.RemoveStaged(m_subpackage.m_all_conflicts, false, MemPoolRemovalReason::REPLACED); + // Don't attempt to process the same conflicts repeatedly during subpackage evaluation: + // they no longer exist on subsequent calls to Finalize() post-RemoveStaged + m_subpackage.m_all_conflicts.clear(); // Store transaction in memory m_pool.addUnchecked(*entry, ws.m_ancestors); @@ -1294,7 +1317,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids : std::vector{ws.m_ptx->GetWitnessHash()}; results.emplace(ws.m_ptx->GetWitnessHash(), - MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, + MempoolAcceptResult::Success(std::move(m_subpackage.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate, effective_feerate_wtxids)); if (!m_pool.m_opts.signals) continue; const CTransaction& tx = *ws.m_ptx; @@ -1330,7 +1353,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef return MempoolAcceptResult::Failure(ws.m_state); } - if (m_rbf && !ReplacementChecks(ws)) return MempoolAcceptResult::Failure(ws.m_state); + if (m_subpackage.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. @@ -1341,7 +1364,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef const CFeeRate effective_feerate{ws.m_modified_fees, static_cast(ws.m_vsize)}; // Tx was accepted, but not added if (args.m_test_accept) { - return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, + return MempoolAcceptResult::Success(std::move(m_subpackage.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate, single_wtxid); } @@ -1362,7 +1385,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef m_pool.m_opts.signals->TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence()); } - return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, + return MempoolAcceptResult::Success(std::move(m_subpackage.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate, single_wtxid); } @@ -1407,6 +1430,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // replacements, we don't need to track the coins spent. Note that this logic will need to be // updated if package replace-by-fee is allowed in the future. assert(!args.m_allow_replacement); + assert(!m_subpackage.m_rbf); m_viewmempool.PackageAddTransaction(ws.m_ptx); } @@ -1428,26 +1452,26 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // a child that is below mempool minimum feerate. To avoid these behaviors, callers of // AcceptMultipleTransactions need to restrict txns topology (e.g. to ancestor sets) and check // the feerates of individuals and subsets. - const auto m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0}, + m_subpackage.m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0}, [](int64_t sum, auto& ws) { return sum + ws.m_vsize; }); - const auto m_total_modified_fees = std::accumulate(workspaces.cbegin(), workspaces.cend(), CAmount{0}, + m_subpackage.m_total_modified_fees = std::accumulate(workspaces.cbegin(), workspaces.cend(), CAmount{0}, [](CAmount sum, auto& ws) { return sum + ws.m_modified_fees; }); - const CFeeRate package_feerate(m_total_modified_fees, m_total_vsize); + const CFeeRate package_feerate(m_subpackage.m_total_modified_fees, m_subpackage.m_total_vsize); std::vector all_package_wtxids; all_package_wtxids.reserve(workspaces.size()); std::transform(workspaces.cbegin(), workspaces.cend(), std::back_inserter(all_package_wtxids), [](const auto& ws) { return ws.m_ptx->GetWitnessHash(); }); TxValidationState placeholder_state; if (args.m_package_feerates && - !CheckFeeRate(m_total_vsize, m_total_modified_fees, placeholder_state)) { + !CheckFeeRate(m_subpackage.m_total_vsize, m_subpackage.m_total_modified_fees, placeholder_state)) { package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); return PackageMempoolAcceptResult(package_state, {{workspaces.back().m_ptx->GetWitnessHash(), - MempoolAcceptResult::FeeFailure(placeholder_state, CFeeRate(m_total_modified_fees, m_total_vsize), all_package_wtxids)}}); + MempoolAcceptResult::FeeFailure(placeholder_state, CFeeRate(m_subpackage.m_total_modified_fees, m_subpackage.m_total_vsize), all_package_wtxids)}}); } // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, // because it's unnecessary. - if (txns.size() > 1 && !PackageMempoolChecks(txns, m_total_vsize, package_state)) { + if (txns.size() > 1 && !PackageMempoolChecks(txns, m_subpackage.m_total_vsize, package_state)) { return PackageMempoolAcceptResult(package_state, std::move(results)); } @@ -1465,7 +1489,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids : std::vector{ws.m_ptx->GetWitnessHash()}; results.emplace(ws.m_ptx->GetWitnessHash(), - MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), + MempoolAcceptResult::Success(std::move(m_subpackage.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate, effective_feerate_wtxids)); } @@ -1530,7 +1554,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptSubPackage(const std::vector Date: Wed, 22 May 2024 10:11:15 -0400 Subject: [PATCH 5/5] Add sanity checks for various ATMPArgs booleans --- src/validation.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index c712b89bf8213..c50a1c8a6126f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -575,6 +575,14 @@ class MemPoolAccept m_client_maxfeerate{client_maxfeerate}, m_allow_carveouts{allow_carveouts} { + // If we are using package feerates, we must be doing package submission. + // It also means carveouts and sibling eviction are not permitted. + if (m_package_feerates) { + Assume(m_package_submission); + Assume(!m_allow_carveouts); + Assume(!m_allow_sibling_eviction); + } + if (m_allow_sibling_eviction) Assume(m_allow_replacement); } };