diff --git a/src/txmempool.cpp b/src/txmempool.cpp index ed0b3913d7b..8aa33a46af3 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 1025082dc88..40ea8387202 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 7445eb6b8a6..7ba870a10a0 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);