From 15d982f91e6b0f145c9dd4edf29827cfabb37a3f Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Wed, 23 Oct 2024 12:24:41 -0400 Subject: [PATCH 01/18] Add package hash to package-rbf log message --- src/validation.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 62cae2cfb5..bc2245f053 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1205,9 +1205,10 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn "package RBF failed: " + err_tup.value().second, ""); } - LogDebug(BCLog::TXPACKAGES, "package RBF checks passed: parent %s (wtxid=%s), child %s (wtxid=%s)\n", + LogDebug(BCLog::TXPACKAGES, "package RBF checks passed: parent %s (wtxid=%s), child %s (wtxid=%s), package hash (%s)\n", txns.front()->GetHash().ToString(), txns.front()->GetWitnessHash().ToString(), - txns.back()->GetHash().ToString(), txns.back()->GetWitnessHash().ToString()); + txns.back()->GetHash().ToString(), txns.back()->GetWitnessHash().ToString(), + GetPackageHash(txns).ToString()); return true; From 87d92fa340195d9c87be3d023ca133b90b3b7d4e Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 24 Oct 2024 08:24:00 -0400 Subject: [PATCH 02/18] test: Add unit test coverage of package rbf + prioritisetransaction --- src/test/txpackage_tests.cpp | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index ea211aedf3..ecee82c48a 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -1078,7 +1078,25 @@ BOOST_AUTO_TEST_CASE(package_rbf_tests) BOOST_CHECK_EQUAL(it_child_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 199 + 1300); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); - } + // Finally, check that we can prioritise tx_child_1 to get package1 into the mempool. + // It should not be possible to resubmit package1 and get it in without prioritisation. + const auto submit4 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false, std::nullopt); + if (auto err_4{CheckPackageMempoolAcceptResult(package1, submit4, /*expect_valid=*/false, m_node.mempool.get())}) { + BOOST_ERROR(err_4.value()); + } + m_node.mempool->PrioritiseTransaction(tx_child_1->GetHash(), 1363); + const auto submit5 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false, std::nullopt); + if (auto err_5{CheckPackageMempoolAcceptResult(package1, submit5, /*expect_valid=*/true, m_node.mempool.get())}) { + BOOST_ERROR(err_5.value()); + } + it_parent_1 = submit5.m_tx_results.find(tx_parent_1->GetWitnessHash()); + it_child_1 = submit5.m_tx_results.find(tx_child_1->GetWitnessHash()); + BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID); + LOCK(m_node.mempool->cs); + BOOST_CHECK(m_node.mempool->GetIter(tx_parent_1->GetHash()).has_value()); + BOOST_CHECK(m_node.mempool->GetIter(tx_child_1->GetHash()).has_value()); + } } BOOST_AUTO_TEST_SUITE_END() From 802214c0832de00f24268183f7763fa984ba7903 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Fri, 23 Aug 2024 18:45:49 -0400 Subject: [PATCH 03/18] Introduce mempool changesets Introduce the CTxMemPool::ChangeSet, a wrapper for creating (potential) new mempool entries and removing conflicts. --- src/txmempool.cpp | 20 +++++++++++++++++++ src/txmempool.h | 31 ++++++++++++++++++++++++++++ src/validation.cpp | 50 +++++++++++++++++++++++++++------------------- 3 files changed, 80 insertions(+), 21 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 8b6f993843..ed0b3913d7 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1369,3 +1369,23 @@ util::Result, std::vector>> CTxMemPool:: std::sort(new_chunks.begin(), new_chunks.end(), std::greater()); return std::make_pair(old_chunks, new_chunks); } + +CTxMemPool::ChangeSet::TxHandle CTxMemPool::ChangeSet::StageAddition(const CTransactionRef& tx, const CAmount fee, int64_t time, unsigned int entry_height, uint64_t entry_sequence, bool spends_coinbase, int64_t sigops_cost, LockPoints lp) +{ + auto newit = m_to_add.emplace(tx, fee, time, entry_height, entry_sequence, spends_coinbase, sigops_cost, lp).first; + m_entry_vec.push_back(newit); + return newit; +} + +void CTxMemPool::ChangeSet::Apply() +{ + LOCK(m_pool->cs); + m_pool->RemoveStaged(m_to_remove, false, MemPoolRemovalReason::REPLACED); + for (size_t i=0; iaddUnchecked(*tx_entry); + } + m_to_add.clear(); + m_to_remove.clear(); + m_entry_vec.clear(); +} diff --git a/src/txmempool.h b/src/txmempool.h index f914cbd729..1025082dc8 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -816,6 +816,37 @@ public: assert(m_epoch.guarded()); // verify guard even when it==nullopt return !it || visited(*it); } + + class ChangeSet { + public: + explicit ChangeSet(CTxMemPool* pool) : m_pool(pool) {} + ~ChangeSet() = default; + + ChangeSet(const ChangeSet&) = delete; + ChangeSet& operator=(const ChangeSet&) = delete; + + using TxHandle = CTxMemPool::txiter; + + TxHandle StageAddition(const CTransactionRef& tx, const CAmount fee, int64_t time, unsigned int entry_height, uint64_t entry_sequence, bool spends_coinbase, int64_t sigops_cost, LockPoints lp); + void StageRemoval(CTxMemPool::txiter it) { m_to_remove.insert(it); } + + util::Result CalculateMemPoolAncestors(TxHandle tx, const Limits& limits) + { + LOCK(m_pool->cs); + auto ret{m_pool->CalculateMemPoolAncestors(*tx, limits)}; + return ret; + } + + void Apply() EXCLUSIVE_LOCKS_REQUIRED(cs_main); + + private: + CTxMemPool* m_pool; + CTxMemPool::indexed_transaction_set m_to_add; + std::vector m_entry_vec; // track the added transactions' insertion order + CTxMemPool::setEntries m_to_remove; + }; + + std::unique_ptr GetChangeSet() EXCLUSIVE_LOCKS_REQUIRED(cs) { return std::make_unique(this); } }; /** diff --git a/src/validation.cpp b/src/validation.cpp index bc2245f053..7445eb6b8a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -633,9 +633,9 @@ private: CTxMemPool::setEntries m_iters_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; + /* Changeset representing adding a transaction and removing its conflicts. */ + std::unique_ptr m_changeset; + CTxMemPool::ChangeSet::TxHandle m_tx_handle; /** 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}; @@ -780,7 +780,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Alias what we need out of ws TxValidationState& state = ws.m_state; - std::unique_ptr& entry = ws.m_entry; if (!CheckTransaction(tx, state)) { return false; // state filled in by CheckTransaction @@ -909,9 +908,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Set entry_sequence to 0 when bypass_limits is used; this allows txs from a block // reorg to be marked earlier than any child txs that were already in the mempool. const uint64_t entry_sequence = bypass_limits ? 0 : m_pool.GetSequence(); - entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence, - fSpendsCoinbase, nSigOpsCost, lock_points.value())); - ws.m_vsize = entry->GetTxSize(); + ws.m_changeset = m_pool.GetChangeSet(); + ws.m_tx_handle = ws.m_changeset->StageAddition(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence, fSpendsCoinbase, nSigOpsCost, lock_points.value()); + + ws.m_vsize = ws.m_tx_handle->GetTxSize(); // Enforces 0-fee for dust transactions, no incentive to be mined alone if (m_pool.m_opts.require_standard) { @@ -983,7 +983,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) maybe_rbf_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants(); } - if (auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, maybe_rbf_limits)}) { + if (auto ancestors{ws.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, maybe_rbf_limits)}) { ws.m_ancestors = std::move(*ancestors); } else { // If CalculateMemPoolAncestors fails second time, we want the original error string. @@ -1015,7 +1015,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || ws.m_ptx->version == TRUC_VERSION) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); } - if (auto ancestors_retry{m_pool.CalculateMemPoolAncestors(*entry, cpfp_carve_out_limits)}) { + if (auto ancestors_retry{ws.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, cpfp_carve_out_limits)}) { ws.m_ancestors = std::move(*ancestors_retry); } else { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); @@ -1114,6 +1114,11 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) return state.Invalid(TxValidationResult::TX_RECONSIDERABLE, strprintf("insufficient fee%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } + + // Add all the to-be-removed transactions to the changeset. + for (auto it : m_subpackage.m_all_conflicts) { + ws.m_changeset->StageRemoval(it); + } return true; } @@ -1173,7 +1178,9 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn "package RBF failed: too many potential replacements", *err_string); } + for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts) { + parent_ws.m_changeset->StageRemoval(it); m_subpackage.m_conflicting_fees += it->GetModifiedFee(); m_subpackage.m_conflicting_size += it->GetTxSize(); } @@ -1283,7 +1290,6 @@ 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; if (!m_subpackage.m_all_conflicts.empty()) Assume(args.m_allow_replacement); // Remove conflicting transactions from the mempool @@ -1296,25 +1302,23 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) it->GetTxSize(), hash.ToString(), tx.GetWitnessHash().ToString(), - entry->GetFee(), - entry->GetTxSize()); + ws.m_tx_handle->GetFee(), + ws.m_tx_handle->GetTxSize()); TRACEPOINT(mempool, replaced, it->GetTx().GetHash().data(), it->GetTxSize(), it->GetFee(), std::chrono::duration_cast>(it->GetTime()).count(), hash.data(), - entry->GetTxSize(), - entry->GetFee() + ws.m_tx_handle->GetTxSize(), + ws.m_tx_handle->GetFee() ); m_subpackage.m_replaced_transactions.push_back(it->GetSharedTx()); } - m_pool.RemoveStaged(m_subpackage.m_all_conflicts, false, MemPoolRemovalReason::REPLACED); + ws.m_changeset->Apply(); // Don't attempt to process the same conflicts repeatedly during subpackage evaluation: - // they no longer exist on subsequent calls to Finalize() post-RemoveStaged + // they no longer exist on subsequent calls to Finalize() post-Apply() m_subpackage.m_all_conflicts.clear(); - // Store transaction in memory - m_pool.addUnchecked(*entry, ws.m_ancestors); // trim mempool and check if tx was trimmed // If we are validating a package, don't trim here because we could evict a previous transaction @@ -1359,7 +1363,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& // Re-calculate mempool ancestors to call addUnchecked(). They may have changed since the // last calculation done in PreChecks, since package ancestors have already been submitted. { - auto ancestors{m_pool.CalculateMemPoolAncestors(*ws.m_entry, m_pool.m_opts.limits)}; + auto ancestors{ws.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, m_pool.m_opts.limits)}; if(!ancestors) { results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); // Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail. @@ -1400,6 +1404,8 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& // Add successful results. The returned results may change later if LimitMempoolSize() evicts them. for (Workspace& ws : workspaces) { + auto iter = m_pool.GetIter(ws.m_ptx->GetHash()); + Assume(iter.has_value()); const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : CFeeRate{ws.m_modified_fees, static_cast(ws.m_vsize)}; const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids : @@ -1410,7 +1416,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& if (!m_pool.m_opts.signals) continue; const CTransaction& tx = *ws.m_ptx; const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, - ws.m_vsize, ws.m_entry->GetHeight(), + ws.m_vsize, (*iter)->GetHeight(), args.m_bypass_limits, args.m_package_submission, IsCurrentForFeeEstimation(m_active_chainstate), m_pool.HasNoInputsOf(tx)); @@ -1481,8 +1487,10 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef if (m_pool.m_opts.signals) { const CTransaction& tx = *ws.m_ptx; + auto iter = m_pool.GetIter(tx.GetHash()); + Assume(iter.has_value()); const auto tx_info = NewMempoolTransactionInfo(ws.m_ptx, ws.m_base_fees, - ws.m_vsize, ws.m_entry->GetHeight(), + ws.m_vsize, (*iter)->GetHeight(), args.m_bypass_limits, args.m_package_submission, IsCurrentForFeeEstimation(m_active_chainstate), m_pool.HasNoInputsOf(tx)); From 01e145b9758f1df14a7ea18058ba9577bf88e459 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 24 Aug 2024 06:53:43 -0400 Subject: [PATCH 04/18] Move changeset from workspace to subpackage Removes a redundant check that mempool limits will not be violated during package acceptance. --- src/txmempool.cpp | 9 ++++- src/txmempool.h | 9 +++++ src/validation.cpp | 87 ++++++++++++++++++++-------------------------- 3 files changed, 55 insertions(+), 50 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index ed0b3913d7..8aa33a46af 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1383,9 +1383,16 @@ void CTxMemPool::ChangeSet::Apply() m_pool->RemoveStaged(m_to_remove, false, MemPoolRemovalReason::REPLACED); for (size_t i=0; iaddUnchecked(*tx_entry); + if (i == 0 && m_ancestors.count(tx_entry)) { + m_pool->addUnchecked(*tx_entry, m_ancestors[tx_entry]); + } else { + // We always recalculate ancestors from scratch if we're dealing + // with transactions which may have parents in the same package. + m_pool->addUnchecked(*tx_entry); + } } m_to_add.clear(); m_to_remove.clear(); m_entry_vec.clear(); + m_ancestors.clear(); } diff --git a/src/txmempool.h b/src/txmempool.h index 1025082dc8..40ea838720 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -832,8 +832,15 @@ public: util::Result CalculateMemPoolAncestors(TxHandle tx, const Limits& limits) { + // Look up transaction in our cache first + auto it = m_ancestors.find(tx); + if (it != m_ancestors.end()) return it->second; + + // If not found, try to have the mempool calculate it, and cache + // for later. LOCK(m_pool->cs); auto ret{m_pool->CalculateMemPoolAncestors(*tx, limits)}; + if (ret) m_ancestors.try_emplace(tx, *ret); return ret; } @@ -843,6 +850,8 @@ public: CTxMemPool* m_pool; CTxMemPool::indexed_transaction_set m_to_add; std::vector m_entry_vec; // track the added transactions' insertion order + // map from the m_to_add index to the ancestors for the transaction + std::map m_ancestors; CTxMemPool::setEntries m_to_remove; }; diff --git a/src/validation.cpp b/src/validation.cpp index 7445eb6b8a..7ba870a10a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -633,8 +633,7 @@ private: CTxMemPool::setEntries m_iters_conflicting; /** All mempool ancestors of this transaction. */ CTxMemPool::setEntries m_ancestors; - /* Changeset representing adding a transaction and removing its conflicts. */ - std::unique_ptr m_changeset; + /* Handle to the tx in the changeset */ CTxMemPool::ChangeSet::TxHandle m_tx_handle; /** 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. */ @@ -691,7 +690,7 @@ private: // Try to add the transaction to the mempool, removing any conflicts first. // Returns true if the transaction is in the mempool after any size // limiting is performed, false otherwise. - bool Finalize(const ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + bool FinalizeSubpackage(const ATMPArgs& args, std::vector& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Submit all transactions to the mempool and call ConsensusScriptChecks to add to the script // cache - should only be called after successful validation of all transactions in the package. @@ -746,6 +745,8 @@ private: CTxMemPool::setEntries m_all_conflicts; /** Mempool transactions that were replaced. */ std::list m_replaced_transactions; + /* Changeset representing adding transactions and removing their conflicts. */ + std::unique_ptr m_changeset; /** Total modified fees of mempool transactions being replaced. */ CAmount m_conflicting_fees{0}; @@ -908,8 +909,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Set entry_sequence to 0 when bypass_limits is used; this allows txs from a block // reorg to be marked earlier than any child txs that were already in the mempool. const uint64_t entry_sequence = bypass_limits ? 0 : m_pool.GetSequence(); - ws.m_changeset = m_pool.GetChangeSet(); - ws.m_tx_handle = ws.m_changeset->StageAddition(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence, fSpendsCoinbase, nSigOpsCost, lock_points.value()); + if (!m_subpackage.m_changeset) { + m_subpackage.m_changeset = m_pool.GetChangeSet(); + } + ws.m_tx_handle = m_subpackage.m_changeset->StageAddition(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence, fSpendsCoinbase, nSigOpsCost, lock_points.value()); ws.m_vsize = ws.m_tx_handle->GetTxSize(); @@ -983,7 +986,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) maybe_rbf_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants(); } - if (auto ancestors{ws.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, maybe_rbf_limits)}) { + if (auto ancestors{m_subpackage.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, maybe_rbf_limits)}) { ws.m_ancestors = std::move(*ancestors); } else { // If CalculateMemPoolAncestors fails second time, we want the original error string. @@ -1015,7 +1018,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || ws.m_ptx->version == TRUC_VERSION) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); } - if (auto ancestors_retry{ws.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, cpfp_carve_out_limits)}) { + if (auto ancestors_retry{m_subpackage.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, cpfp_carve_out_limits)}) { ws.m_ancestors = std::move(*ancestors_retry); } else { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); @@ -1117,7 +1120,7 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) // Add all the to-be-removed transactions to the changeset. for (auto it : m_subpackage.m_all_conflicts) { - ws.m_changeset->StageRemoval(it); + m_subpackage.m_changeset->StageRemoval(it); } return true; } @@ -1180,7 +1183,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts) { - parent_ws.m_changeset->StageRemoval(it); + m_subpackage.m_changeset->StageRemoval(it); m_subpackage.m_conflicting_fees += it->GetModifiedFee(); m_subpackage.m_conflicting_size += it->GetTxSize(); } @@ -1282,13 +1285,13 @@ bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws) return true; } -bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) +bool MemPoolAccept::FinalizeSubpackage(const ATMPArgs& args, std::vector& workspaces) { AssertLockHeld(cs_main); AssertLockHeld(m_pool.cs); - const CTransaction& tx = *ws.m_ptx; - const uint256& hash = ws.m_hash; - TxValidationState& state = ws.m_state; + const CTransaction& tx = *workspaces.front().m_ptx; + const uint256& hash = workspaces.front().m_hash; + TxValidationState& state = workspaces.front().m_state; const bool bypass_limits = args.m_bypass_limits; if (!m_subpackage.m_all_conflicts.empty()) Assume(args.m_allow_replacement); @@ -1302,20 +1305,21 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) it->GetTxSize(), hash.ToString(), tx.GetWitnessHash().ToString(), - ws.m_tx_handle->GetFee(), - ws.m_tx_handle->GetTxSize()); + workspaces[0].m_tx_handle->GetFee(), + workspaces[0].m_tx_handle->GetTxSize()); TRACEPOINT(mempool, replaced, it->GetTx().GetHash().data(), it->GetTxSize(), it->GetFee(), std::chrono::duration_cast>(it->GetTime()).count(), hash.data(), - ws.m_tx_handle->GetTxSize(), - ws.m_tx_handle->GetFee() + workspaces[0].m_tx_handle->GetTxSize(), + workspaces[0].m_tx_handle->GetFee() ); m_subpackage.m_replaced_transactions.push_back(it->GetSharedTx()); } - ws.m_changeset->Apply(); + m_subpackage.m_changeset->Apply(); + m_subpackage.m_changeset.reset(); // Don't attempt to process the same conflicts repeatedly during subpackage evaluation: // they no longer exist on subsequent calls to Finalize() post-Apply() m_subpackage.m_all_conflicts.clear(); @@ -1345,6 +1349,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& return !m_pool.exists(GenTxid::Txid(ws.m_ptx->GetHash())); })); bool all_submitted = true; + FinalizeSubpackage(args, workspaces); // ConsensusScriptChecks adds to the script cache and is therefore consensus-critical; // CheckInputsFromMempoolAndCache asserts that transactions only spend coins available from the // mempool or UTXO set. Submit each transaction to the mempool immediately after calling @@ -1358,36 +1363,19 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR, strprintf("BUG! PolicyScriptChecks succeeded but ConsensusScriptChecks failed: %s", ws.m_ptx->GetHash().ToString())); + // Remove the transaction from the mempool. + if (!m_subpackage.m_changeset) m_subpackage.m_changeset = m_pool.GetChangeSet(); + m_subpackage.m_changeset->StageRemoval(m_pool.GetIter(ws.m_ptx->GetHash()).value()); } - - // Re-calculate mempool ancestors to call addUnchecked(). They may have changed since the - // last calculation done in PreChecks, since package ancestors have already been submitted. - { - auto ancestors{ws.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, m_pool.m_opts.limits)}; - if(!ancestors) { - results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); - // Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail. - Assume(false); - all_submitted = false; - package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR, - strprintf("BUG! Mempool ancestors or descendants were underestimated: %s", - ws.m_ptx->GetHash().ToString())); - } - ws.m_ancestors = std::move(ancestors).value_or(ws.m_ancestors); - } - // If we call LimitMempoolSize() for each individual Finalize(), the mempool will not take - // the transaction's descendant feerate into account because it hasn't seen them yet. Also, - // we risk evicting a transaction that a subsequent package transaction depends on. Instead, - // allow the mempool to temporarily bypass limits, the maximum package size) while - // submitting transactions individually and then trim at the very end. - if (!Finalize(args, ws)) { - results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); - // Since LimitMempoolSize() won't be called, this should never fail. - Assume(false); - all_submitted = false; - package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR, - strprintf("BUG! Adding to mempool failed: %s", ws.m_ptx->GetHash().ToString())); - } + } + if (!all_submitted) { + Assume(m_subpackage.m_changeset); + // This code should be unreachable; it's here as belt-and-suspenders + // to try to ensure we have no consensus-invalid transactions in the + // mempool. + m_subpackage.m_changeset->Apply(); + m_subpackage.m_changeset.reset(); + return false; } std::vector all_package_wtxids; @@ -1430,7 +1418,8 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef AssertLockHeld(cs_main); LOCK(m_pool.cs); // mempool "read lock" (held through m_pool.m_opts.signals->TransactionAddedToMempool()) - Workspace ws(ptx); + std::vector workspaces{Workspace(ptx)}; + Workspace &ws = workspaces.front(); const std::vector single_wtxid{ws.m_ptx->GetWitnessHash()}; if (!PreChecks(args, ws)) { @@ -1478,7 +1467,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef ws.m_base_fees, effective_feerate, single_wtxid); } - if (!Finalize(args, ws)) { + if (!FinalizeSubpackage(args, workspaces)) { // The only possible failure reason is fee-related (mempool full). // Failed for fee reasons. Provide the effective feerate and which txns were included. Assume(ws.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE); From 57983b8add72a04721d3f2050c063a3c4d8683ed Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 24 Aug 2024 07:26:09 -0400 Subject: [PATCH 05/18] Move LimitMempoolSize to take place outside FinalizeSubpackage --- src/validation.cpp | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 7ba870a10a..c78a66dab5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -688,9 +688,7 @@ private: 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 - // limiting is performed, false otherwise. - bool FinalizeSubpackage(const ATMPArgs& args, std::vector& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + void FinalizeSubpackage(const ATMPArgs& args, std::vector& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Submit all transactions to the mempool and call ConsensusScriptChecks to add to the script // cache - should only be called after successful validation of all transactions in the package. @@ -1285,14 +1283,12 @@ bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws) return true; } -bool MemPoolAccept::FinalizeSubpackage(const ATMPArgs& args, std::vector& workspaces) +void MemPoolAccept::FinalizeSubpackage(const ATMPArgs& args, std::vector& workspaces) { AssertLockHeld(cs_main); AssertLockHeld(m_pool.cs); const CTransaction& tx = *workspaces.front().m_ptx; const uint256& hash = workspaces.front().m_hash; - TxValidationState& state = workspaces.front().m_state; - const bool bypass_limits = args.m_bypass_limits; if (!m_subpackage.m_all_conflicts.empty()) Assume(args.m_allow_replacement); // Remove conflicting transactions from the mempool @@ -1323,18 +1319,6 @@ bool MemPoolAccept::FinalizeSubpackage(const ATMPArgs& args, std::vector& workspaces, @@ -1467,11 +1451,19 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef ws.m_base_fees, effective_feerate, single_wtxid); } - if (!FinalizeSubpackage(args, workspaces)) { - // The only possible failure reason is fee-related (mempool full). - // Failed for fee reasons. Provide the effective feerate and which txns were included. - Assume(ws.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE); - return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), {ws.m_ptx->GetWitnessHash()}); + FinalizeSubpackage(args, workspaces); + + // trim mempool and check if tx was trimmed + // If we are validating a package, don't trim here because we could evict a previous transaction + // in the package. LimitMempoolSize() should be called at the very end to make sure the mempool + // is still within limits and package submission happens atomically. + if (!args.m_package_submission && !args.m_bypass_limits) { + LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); + if (!m_pool.exists(GenTxid::Txid(ws.m_hash))) { + // The tx no longer meets our (new) mempool minimum feerate but could be reconsidered in a package. + ws.m_state.Invalid(TxValidationResult::TX_RECONSIDERABLE, "mempool full"); + return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), {ws.m_ptx->GetWitnessHash()}); + } } if (m_pool.m_opts.signals) { From 34b6c5833d11ea84fbd4b891e06408f6f4ca6fac Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 24 Aug 2024 07:26:09 -0400 Subject: [PATCH 06/18] Clean up FinalizeSubpackage to avoid workspace-specific information Also, use the "package hash" for logging replacements in the package rbf setting. --- src/txmempool.h | 12 +++++++++ src/validation.cpp | 61 ++++++++++++++++++++++++++++------------------ 2 files changed, 49 insertions(+), 24 deletions(-) diff --git a/src/txmempool.h b/src/txmempool.h index 40ea838720..fd6bfb4a87 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -844,6 +844,18 @@ public: return ret; } + std::vector GetAddedTxns() const { + std::vector ret; + ret.reserve(m_entry_vec.size()); + for (const auto& entry : m_entry_vec) { + ret.emplace_back(entry->GetSharedTx()); + } + return ret; + } + + size_t GetTxCount() const { return m_entry_vec.size(); } + const CTransaction& GetAddedTxn(size_t index) const { return m_entry_vec.at(index)->GetTx(); } + void Apply() EXCLUSIVE_LOCKS_REQUIRED(cs_main); private: diff --git a/src/validation.cpp b/src/validation.cpp index c78a66dab5..aba170769e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -688,7 +688,7 @@ private: 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. - void FinalizeSubpackage(const ATMPArgs& args, std::vector& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); + void FinalizeSubpackage(const ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); // Submit all transactions to the mempool and call ConsensusScriptChecks to add to the script // cache - should only be called after successful validation of all transactions in the package. @@ -1283,34 +1283,48 @@ bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws) return true; } -void MemPoolAccept::FinalizeSubpackage(const ATMPArgs& args, std::vector& workspaces) +void MemPoolAccept::FinalizeSubpackage(const ATMPArgs& args) { AssertLockHeld(cs_main); AssertLockHeld(m_pool.cs); - const CTransaction& tx = *workspaces.front().m_ptx; - const uint256& hash = workspaces.front().m_hash; if (!m_subpackage.m_all_conflicts.empty()) Assume(args.m_allow_replacement); // Remove conflicting transactions from the mempool for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts) { - LogDebug(BCLog::MEMPOOL, "replacing mempool tx %s (wtxid=%s, fees=%s, vsize=%s). New tx %s (wtxid=%s, fees=%s, vsize=%s)\n", - it->GetTx().GetHash().ToString(), - it->GetTx().GetWitnessHash().ToString(), - it->GetFee(), - it->GetTxSize(), - hash.ToString(), - tx.GetWitnessHash().ToString(), - workspaces[0].m_tx_handle->GetFee(), - workspaces[0].m_tx_handle->GetTxSize()); + std::string log_string = strprintf("replacing mempool tx %s (wtxid=%s, fees=%s, vsize=%s). ", + it->GetTx().GetHash().ToString(), + it->GetTx().GetWitnessHash().ToString(), + it->GetFee(), + it->GetTxSize()); + FeeFrac feerate{m_subpackage.m_total_modified_fees, int32_t(m_subpackage.m_total_vsize)}; + uint256 tx_or_package_hash{}; + if (m_subpackage.m_changeset->GetTxCount() == 1) { + const CTransaction& tx = m_subpackage.m_changeset->GetAddedTxn(0); + tx_or_package_hash = tx.GetHash(); + log_string += strprintf("New tx %s (wtxid=%s, fees=%s, vsize=%s)", + tx.GetHash().ToString(), + tx.GetWitnessHash().ToString(), + feerate.fee, + feerate.size); + } else { + tx_or_package_hash = GetPackageHash(m_subpackage.m_changeset->GetAddedTxns()); + log_string += strprintf("New package %s with %lu txs, fees=%s, vsize=%s", + tx_or_package_hash.ToString(), + m_subpackage.m_changeset->GetTxCount(), + feerate.fee, + feerate.size); + + } + LogDebug(BCLog::MEMPOOL, "%s\n", log_string); TRACEPOINT(mempool, replaced, it->GetTx().GetHash().data(), it->GetTxSize(), it->GetFee(), std::chrono::duration_cast>(it->GetTime()).count(), - hash.data(), - workspaces[0].m_tx_handle->GetTxSize(), - workspaces[0].m_tx_handle->GetFee() + tx_or_package_hash.data(), + feerate.size, + feerate.fee ); m_subpackage.m_replaced_transactions.push_back(it->GetSharedTx()); } @@ -1333,7 +1347,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& return !m_pool.exists(GenTxid::Txid(ws.m_ptx->GetHash())); })); bool all_submitted = true; - FinalizeSubpackage(args, workspaces); + FinalizeSubpackage(args); // ConsensusScriptChecks adds to the script cache and is therefore consensus-critical; // CheckInputsFromMempoolAndCache asserts that transactions only spend coins available from the // mempool or UTXO set. Submit each transaction to the mempool immediately after calling @@ -1402,8 +1416,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef AssertLockHeld(cs_main); LOCK(m_pool.cs); // mempool "read lock" (held through m_pool.m_opts.signals->TransactionAddedToMempool()) - std::vector workspaces{Workspace(ptx)}; - Workspace &ws = workspaces.front(); + Workspace ws(ptx); const std::vector single_wtxid{ws.m_ptx->GetWitnessHash()}; if (!PreChecks(args, ws)) { @@ -1414,6 +1427,9 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef return MempoolAcceptResult::Failure(ws.m_state); } + m_subpackage.m_total_vsize = ws.m_vsize; + m_subpackage.m_total_modified_fees = ws.m_modified_fees; + // Individual modified feerate exceeded caller-defined max; abort if (args.m_client_maxfeerate && CFeeRate(ws.m_modified_fees, ws.m_vsize) > args.m_client_maxfeerate.value()) { ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "max feerate exceeded", ""); @@ -1451,12 +1467,9 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef ws.m_base_fees, effective_feerate, single_wtxid); } - FinalizeSubpackage(args, workspaces); + FinalizeSubpackage(args); - // trim mempool and check if tx was trimmed - // If we are validating a package, don't trim here because we could evict a previous transaction - // in the package. LimitMempoolSize() should be called at the very end to make sure the mempool - // is still within limits and package submission happens atomically. + // Limit the mempool, if appropriate. if (!args.m_package_submission && !args.m_bypass_limits) { LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); if (!m_pool.exists(GenTxid::Txid(ws.m_hash))) { From 7fb62f7db60c7d793828ae45f87bc3f5c63cc989 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 10 Oct 2024 14:59:23 -0400 Subject: [PATCH 07/18] Apply mempool changeset transactions directly into the mempool Rather than individually calling addUnchecked for each transaction added in a changeset (after removing all the to-be-removed transactions), instead we can take advantage of boost::multi_index's splicing features to extract and insert entries directly from the staging multi_index into mapTx. This has the immediate advantage of saving allocation overhead for mempool entries which have already been allocated once. This also means that the memory locations of mempool entries will not change when transactions go from staging to the main mempool. Additionally, eliminate addUnchecked and require all new transactions to enter the mempool via a CTxMemPoolChangeSet. --- src/bench/mempool_ephemeral_spends.cpp | 3 +- src/bench/mempool_eviction.cpp | 3 +- src/bench/mempool_stress.cpp | 3 +- src/bench/rpc_mempool.cpp | 3 +- src/test/blockencodings_tests.cpp | 8 +- src/test/fuzz/mini_miner.cpp | 4 +- src/test/fuzz/partially_downloaded_block.cpp | 2 +- src/test/fuzz/rbf.cpp | 8 +- src/test/mempool_tests.cpp | 92 ++++++++++---------- src/test/miner_tests.cpp | 62 ++++++------- src/test/miniminer_tests.cpp | 42 ++++----- src/test/policyestimator_tests.cpp | 12 +-- src/test/rbf_tests.cpp | 60 ++++++------- src/test/txvalidation_tests.cpp | 24 ++--- src/test/util/setup_common.cpp | 18 ++-- src/test/util/txmempool.cpp | 10 +++ src/test/util/txmempool.h | 4 + src/txmempool.cpp | 76 ++++++++++------ src/txmempool.h | 41 +++++---- src/validation.cpp | 2 +- 20 files changed, 268 insertions(+), 209 deletions(-) diff --git a/src/bench/mempool_ephemeral_spends.cpp b/src/bench/mempool_ephemeral_spends.cpp index e867c61752..f34664a736 100644 --- a/src/bench/mempool_ephemeral_spends.cpp +++ b/src/bench/mempool_ephemeral_spends.cpp @@ -11,6 +11,7 @@ #include