From 34b6c5833d11ea84fbd4b891e06408f6f4ca6fac Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 24 Aug 2024 07:26:09 -0400 Subject: [PATCH] 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 40ea8387202..fd6bfb4a872 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 c78a66dab52..aba170769ef 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))) {