From 66e028f7399b6511f9b73b1cef54b6a6ac38a024 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 7 Oct 2022 17:38:04 +0100 Subject: [PATCH 1/4] mempool: use util::Result for CalculateAncestorsAndCheckLimits Avoid using setAncestors outparameter, simplify function signatures and avoid creating unused dummy strings. --- src/txmempool.cpp | 55 ++++++++++++++++++++++++----------------------- src/txmempool.h | 19 +++++++--------- 2 files changed, 36 insertions(+), 38 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 12e2d5f2241..4bdb7e682e4 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -17,8 +17,10 @@ #include #include #include +#include #include #include +#include #include #include @@ -147,32 +149,29 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector& vHashes } } -bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size, - size_t entry_count, - setEntries& setAncestors, - CTxMemPoolEntry::Parents& staged_ancestors, - const Limits& limits, - std::string &errString) const +util::Result CTxMemPool::CalculateAncestorsAndCheckLimits( + size_t entry_size, + size_t entry_count, + CTxMemPoolEntry::Parents& staged_ancestors, + const Limits& limits) const { size_t totalSizeWithAncestors = entry_size; + setEntries ancestors; while (!staged_ancestors.empty()) { const CTxMemPoolEntry& stage = staged_ancestors.begin()->get(); txiter stageit = mapTx.iterator_to(stage); - setAncestors.insert(stageit); + ancestors.insert(stageit); staged_ancestors.erase(stage); totalSizeWithAncestors += stageit->GetTxSize(); if (stageit->GetSizeWithDescendants() + entry_size > static_cast(limits.descendant_size_vbytes)) { - errString = strprintf("exceeds descendant size limit for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_size_vbytes); - return false; + return util::Error{Untranslated(strprintf("exceeds descendant size limit for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_size_vbytes))}; } else if (stageit->GetCountWithDescendants() + entry_count > static_cast(limits.descendant_count)) { - errString = strprintf("too many descendants for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_count); - return false; + return util::Error{Untranslated(strprintf("too many descendants for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_count))}; } else if (totalSizeWithAncestors > static_cast(limits.ancestor_size_vbytes)) { - errString = strprintf("exceeds ancestor size limit [limit: %u]", limits.ancestor_size_vbytes); - return false; + return util::Error{Untranslated(strprintf("exceeds ancestor size limit [limit: %u]", limits.ancestor_size_vbytes))}; } const CTxMemPoolEntry::Parents& parents = stageit->GetMemPoolParentsConst(); @@ -180,17 +179,16 @@ bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size, txiter parent_it = mapTx.iterator_to(parent); // If this is a new ancestor, add it. - if (setAncestors.count(parent_it) == 0) { + if (ancestors.count(parent_it) == 0) { staged_ancestors.insert(parent); } - if (staged_ancestors.size() + setAncestors.size() + entry_count > static_cast(limits.ancestor_count)) { - errString = strprintf("too many unconfirmed ancestors [limit: %u]", limits.ancestor_count); - return false; + if (staged_ancestors.size() + ancestors.size() + entry_count > static_cast(limits.ancestor_count)) { + return util::Error{Untranslated(strprintf("too many unconfirmed ancestors [limit: %u]", limits.ancestor_count))}; } } } - return true; + return ancestors; } bool CTxMemPool::CheckPackageLimits(const Package& package, @@ -215,13 +213,11 @@ bool CTxMemPool::CheckPackageLimits(const Package& package, // When multiple transactions are passed in, the ancestors and descendants of all transactions // considered together must be within limits even if they are not interdependent. This may be // stricter than the limits for each individual transaction. - setEntries setAncestors; - const auto ret = CalculateAncestorsAndCheckLimits(total_size, package.size(), - setAncestors, staged_ancestors, - limits, errString); + const auto ancestors{CalculateAncestorsAndCheckLimits(total_size, package.size(), + staged_ancestors, limits)}; // It's possible to overestimate the ancestor/descendant totals. - if (!ret) errString.insert(0, "possibly "); - return ret; + if (!ancestors.has_value()) errString = "possibly " + util::ErrorString(ancestors).original; + return ancestors.has_value(); } bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, @@ -254,9 +250,14 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, staged_ancestors = it->GetMemPoolParentsConst(); } - return CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /*entry_count=*/1, - setAncestors, staged_ancestors, - limits, errString); + const auto calculated_ancestors{CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /*entry_count=*/1, + staged_ancestors, limits)}; + if (!calculated_ancestors.has_value()) { + errString = util::ErrorString(calculated_ancestors).original; + return false; + } + setAncestors = *calculated_ancestors; + return true; } void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors) diff --git a/src/txmempool.h b/src/txmempool.h index d48327e5dc4..396c84cb197 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -28,6 +28,7 @@ #include #include #include +#include #include #include @@ -428,24 +429,20 @@ private: /** * Helper function to calculate all in-mempool ancestors of staged_ancestors and apply ancestor - * and descendant limits (including staged_ancestors thsemselves, entry_size and entry_count). + * and descendant limits (including staged_ancestors themselves, entry_size and entry_count). * * @param[in] entry_size Virtual size to include in the limits. * @param[in] entry_count How many entries to include in the limits. - * @param[out] setAncestors Will be populated with all mempool ancestors. * @param[in] staged_ancestors Should contain entries in the mempool. * @param[in] limits Maximum number and size of ancestors and descendants - * @param[out] errString Populated with error reason if any limits are hit * - * @return true if no limits were hit and all in-mempool ancestors were calculated, false - * otherwise + * @return all in-mempool ancestors, or an error if any ancestor or descendant limits were hit */ - bool CalculateAncestorsAndCheckLimits(size_t entry_size, - size_t entry_count, - setEntries& setAncestors, - CTxMemPoolEntry::Parents &staged_ancestors, - const Limits& limits, - std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs); + util::Result CalculateAncestorsAndCheckLimits(size_t entry_size, + size_t entry_count, + CTxMemPoolEntry::Parents &staged_ancestors, + const Limits& limits + ) const EXCLUSIVE_LOCKS_REQUIRED(cs); public: indirectmap mapNextTx GUARDED_BY(cs); From f911bdfff95eba3793fffaf71a31cc8bfc6f80c9 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Sun, 9 Oct 2022 17:19:06 +0100 Subject: [PATCH 2/4] mempool: use util::Result for CalculateMemPoolAncestors Avoid using setAncestors outparameter, simplify function signatures and avoid creating unused dummy strings. --- src/node/interfaces.cpp | 5 +--- src/node/miner.cpp | 5 ++-- src/policy/rbf.cpp | 7 +++-- src/rpc/mempool.cpp | 11 ++++---- src/test/mempool_tests.cpp | 13 +++++---- src/txmempool.cpp | 54 ++++++++++++++++---------------------- src/txmempool.h | 9 ++----- src/validation.cpp | 40 +++++++++++++++------------- 8 files changed, 64 insertions(+), 80 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 212780b2599..7b9baeaf3bd 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -672,12 +672,9 @@ public: if (!m_node.mempool) return true; LockPoints lp; CTxMemPoolEntry entry(tx, 0, 0, 0, false, 0, lp); - CTxMemPool::setEntries ancestors; const CTxMemPool::Limits& limits{m_node.mempool->m_limits}; - std::string unused_error_string; LOCK(m_node.mempool->cs); - return m_node.mempool->CalculateMemPoolAncestors( - entry, ancestors, limits, unused_error_string); + return m_node.mempool->CalculateMemPoolAncestors(entry, limits).has_value(); } CFeeRate estimateSmartFee(int num_blocks, bool conservative, FeeCalculation* calc) override { diff --git a/src/node/miner.cpp b/src/node/miner.cpp index e11ec5b0f1f..e507c1381cf 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -394,9 +394,8 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele continue; } - CTxMemPool::setEntries ancestors; - std::string dummy; - mempool.CalculateMemPoolAncestors(*iter, ancestors, CTxMemPool::Limits::NoLimits(), dummy, false); + auto ancestors_result{mempool.CalculateMemPoolAncestors(*iter, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)}; + auto ancestors{std::move(ancestors_result).value_or(CTxMemPool::setEntries{})}; onlyUnconfirmed(ancestors); ancestors.insert(iter); diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 994e13dd566..bdaf2d0a308 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -16,14 +16,13 @@ #include #include +#include #include RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) { AssertLockHeld(pool.cs); - CTxMemPool::setEntries ancestors; - // First check the transaction itself. if (SignalsOptInRBF(tx)) { return RBFTransactionState::REPLACEABLE_BIP125; @@ -37,9 +36,9 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) // If all the inputs have nSequence >= maxint-1, it still might be // signaled for RBF if any unconfirmed parents have signaled. - std::string dummy; CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash()); - pool.CalculateMemPoolAncestors(entry, ancestors, CTxMemPool::Limits::NoLimits(), dummy, false); + auto ancestors_result{pool.CalculateMemPoolAncestors(entry, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)}; + auto ancestors{std::move(ancestors_result).value_or(CTxMemPool::setEntries{})}; for (CTxMemPool::txiter it : ancestors) { if (SignalsOptInRBF(it->GetTx())) { diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 039a4328e38..af980de6b91 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -23,6 +23,8 @@ #include #include +#include + using kernel::DumpMempool; using node::DEFAULT_MAX_RAW_TX_FEE_RATE; @@ -449,19 +451,18 @@ static RPCHelpMan getmempoolancestors() throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction not in mempool"); } - CTxMemPool::setEntries setAncestors; - std::string dummy; - mempool.CalculateMemPoolAncestors(*it, setAncestors, CTxMemPool::Limits::NoLimits(), dummy, false); + auto ancestors_result{mempool.CalculateMemPoolAncestors(*it, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)}; + auto ancestors{std::move(ancestors_result).value_or(CTxMemPool::setEntries{})}; if (!fVerbose) { UniValue o(UniValue::VARR); - for (CTxMemPool::txiter ancestorIt : setAncestors) { + for (CTxMemPool::txiter ancestorIt : ancestors) { o.push_back(ancestorIt->GetTx().GetHash().ToString()); } return o; } else { UniValue o(UniValue::VOBJ); - for (CTxMemPool::txiter ancestorIt : setAncestors) { + for (CTxMemPool::txiter ancestorIt : ancestors) { const CTxMemPoolEntry &e = *ancestorIt; const uint256& _hash = e.GetTx().GetHash(); UniValue info(UniValue::VOBJ); diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index b12aac6299d..29668939276 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -202,10 +202,9 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) tx7.vout[1].scriptPubKey = CScript() << OP_11 << OP_EQUAL; tx7.vout[1].nValue = 1 * COIN; - CTxMemPool::setEntries setAncestorsCalculated; - std::string dummy; - BOOST_CHECK_EQUAL(pool.CalculateMemPoolAncestors(entry.Fee(2'000'000LL).FromTx(tx7), setAncestorsCalculated, CTxMemPool::Limits::NoLimits(), dummy), true); - BOOST_CHECK(setAncestorsCalculated == setAncestors); + auto ancestors_calculated{pool.CalculateMemPoolAncestors(entry.Fee(2000000LL).FromTx(tx7), CTxMemPool::Limits::NoLimits())}; + BOOST_REQUIRE(ancestors_calculated.has_value()); + BOOST_CHECK(*ancestors_calculated == setAncestors); pool.addUnchecked(entry.FromTx(tx7), setAncestors); BOOST_CHECK_EQUAL(pool.size(), 7U); @@ -261,9 +260,9 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) tx10.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; tx10.vout[0].nValue = 10 * COIN; - setAncestorsCalculated.clear(); - BOOST_CHECK_EQUAL(pool.CalculateMemPoolAncestors(entry.Fee(200'000LL).Time(NodeSeconds{4s}).FromTx(tx10), setAncestorsCalculated, CTxMemPool::Limits::NoLimits(), dummy), true); - BOOST_CHECK(setAncestorsCalculated == setAncestors); + ancestors_calculated = pool.CalculateMemPoolAncestors(entry.Fee(200000LL).Time(NodeSeconds{4s}).FromTx(tx10), CTxMemPool::Limits::NoLimits()); + BOOST_REQUIRE(ancestors_calculated); + BOOST_CHECK(*ancestors_calculated == setAncestors); pool.addUnchecked(entry.FromTx(tx10), setAncestors); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 4bdb7e682e4..32f285276be 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -25,6 +25,8 @@ #include #include +#include +#include bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp) { @@ -220,11 +222,10 @@ bool CTxMemPool::CheckPackageLimits(const Package& package, return ancestors.has_value(); } -bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, - setEntries &setAncestors, - const Limits& limits, - std::string &errString, - bool fSearchForParents /* = true */) const +util::Result CTxMemPool::CalculateMemPoolAncestors( + const CTxMemPoolEntry &entry, + const Limits& limits, + bool fSearchForParents /* = true */) const { CTxMemPoolEntry::Parents staged_ancestors; const CTransaction &tx = entry.GetTx(); @@ -238,8 +239,7 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, if (piter) { staged_ancestors.insert(**piter); if (staged_ancestors.size() + 1 > static_cast(limits.ancestor_count)) { - errString = strprintf("too many unconfirmed parents [limit: %u]", limits.ancestor_count); - return false; + return util::Error{Untranslated(strprintf("too many unconfirmed parents [limit: %u]", limits.ancestor_count))}; } } } @@ -250,14 +250,8 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, staged_ancestors = it->GetMemPoolParentsConst(); } - const auto calculated_ancestors{CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /*entry_count=*/1, - staged_ancestors, limits)}; - if (!calculated_ancestors.has_value()) { - errString = util::ErrorString(calculated_ancestors).original; - return false; - } - setAncestors = *calculated_ancestors; - return true; + return CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /*entry_count=*/1, staged_ancestors, + limits); } void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors) @@ -321,9 +315,7 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b } } for (txiter removeIt : entriesToRemove) { - setEntries setAncestors; const CTxMemPoolEntry &entry = *removeIt; - std::string dummy; // Since this is a tx that is already in the mempool, we can call CMPA // with fSearchForParents = false. If the mempool is in a consistent // state, then using true or false should both be correct, though false @@ -343,10 +335,11 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b // mempool parents we'd calculate by searching, and it's important that // we use the cached notion of ancestor transactions as the set of // things to update for removal. - CalculateMemPoolAncestors(entry, setAncestors, Limits::NoLimits(), dummy, false); + auto ancestors_result{CalculateMemPoolAncestors(entry, Limits::NoLimits(), /*fSearchForParents=*/false)}; + auto ancestors{std::move(ancestors_result).value_or(setEntries{})}; // Note that UpdateAncestorsOf severs the child links that point to // removeIt in the entries for the parents of removeIt. - UpdateAncestorsOf(false, removeIt, setAncestors); + UpdateAncestorsOf(false, removeIt, ancestors); } // After updating all the ancestor sizes, we can now sever the link between each // transaction being removed and any mempool children (ie, update CTxMemPoolEntry::m_parents @@ -696,15 +689,14 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei assert(setParentCheck.size() == it->GetMemPoolParentsConst().size()); assert(std::equal(setParentCheck.begin(), setParentCheck.end(), it->GetMemPoolParentsConst().begin(), comp)); // Verify ancestor state is correct. - setEntries setAncestors; - std::string dummy; - CalculateMemPoolAncestors(*it, setAncestors, Limits::NoLimits(), dummy); - uint64_t nCountCheck = setAncestors.size() + 1; + auto ancestors_result{CalculateMemPoolAncestors(*it, Limits::NoLimits())}; + auto ancestors{std::move(ancestors_result).value_or(setEntries{})}; + uint64_t nCountCheck = ancestors.size() + 1; uint64_t nSizeCheck = it->GetTxSize(); CAmount nFeesCheck = it->GetModifiedFee(); int64_t nSigOpCheck = it->GetSigOpCost(); - for (txiter ancestorIt : setAncestors) { + for (txiter ancestorIt : ancestors) { nSizeCheck += ancestorIt->GetTxSize(); nFeesCheck += ancestorIt->GetModifiedFee(); nSigOpCheck += ancestorIt->GetSigOpCost(); @@ -859,10 +851,9 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeD if (it != mapTx.end()) { mapTx.modify(it, [&nFeeDelta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(nFeeDelta); }); // Now update all ancestors' modified fees with descendants - setEntries setAncestors; - std::string dummy; - CalculateMemPoolAncestors(*it, setAncestors, Limits::NoLimits(), dummy, false); - for (txiter ancestorIt : setAncestors) { + auto ancestors_result{CalculateMemPoolAncestors(*it, Limits::NoLimits(), /*fSearchForParents=*/false)}; + auto ancestors{std::move(ancestors_result).value_or(setEntries{})}; + for (txiter ancestorIt : ancestors) { mapTx.modify(ancestorIt, [=](CTxMemPoolEntry& e){ e.UpdateDescendantState(0, nFeeDelta, 0);}); } // Now update all descendants' modified fees with ancestors @@ -999,10 +990,9 @@ int CTxMemPool::Expire(std::chrono::seconds time) void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, bool validFeeEstimate) { - setEntries setAncestors; - std::string dummy; - CalculateMemPoolAncestors(entry, setAncestors, Limits::NoLimits(), dummy); - return addUnchecked(entry, setAncestors, validFeeEstimate); + auto ancestors_result{CalculateMemPoolAncestors(entry, Limits::NoLimits())}; + auto ancestors{std::move(ancestors_result).value_or(CTxMemPool::setEntries{})}; + return addUnchecked(entry, ancestors, validFeeEstimate); } void CTxMemPool::UpdateChild(txiter entry, txiter child, bool add) diff --git a/src/txmempool.h b/src/txmempool.h index 396c84cb197..14b0dc3871d 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -555,20 +555,15 @@ public: * (these are all calculated including the tx itself) * * @param[in] entry CTxMemPoolEntry of which all in-mempool ancestors are calculated - * @param[out] setAncestors Will be populated with all mempool ancestors. * @param[in] limits Maximum number and size of ancestors and descendants - * @param[out] errString Populated with error reason if any limits are hit * @param[in] fSearchForParents Whether to search a tx's vin for in-mempool parents, or look * up parents from mapLinks. Must be true for entries not in * the mempool * - * @return true if no limits were hit and all in-mempool ancestors were calculated, false - * otherwise + * @return all in-mempool ancestors, or an error if any ancestor or descendant limits were hit */ - bool CalculateMemPoolAncestors(const CTxMemPoolEntry& entry, - setEntries& setAncestors, + util::Result CalculateMemPoolAncestors(const CTxMemPoolEntry& entry, const Limits& limits, - std::string& errString, bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** Calculate all in-mempool ancestors of a set of transactions not already in the mempool and diff --git a/src/validation.cpp b/src/validation.cpp index 1cf6fc0675d..f5f3f209a8d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -64,6 +64,7 @@ #include #include #include +#include using kernel::CCoinsStats; using kernel::CoinStatsHashType; @@ -870,11 +871,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) m_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants(); } - std::string errString; - if (!m_pool.CalculateMemPoolAncestors(*entry, ws.m_ancestors, m_limits, errString)) { - ws.m_ancestors.clear(); + auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, m_limits)}; + if (!ancestors) { // If CalculateMemPoolAncestors fails second time, we want the original error string. - std::string dummy_err_string; // 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 @@ -892,14 +891,16 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) .descendant_count = m_limits.descendant_count + 1, .descendant_size_vbytes = m_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT, }; - if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || - !m_pool.CalculateMemPoolAncestors(*entry, ws.m_ancestors, - cpfp_carve_out_limits, - dummy_err_string)) { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", errString); + const auto error_message{util::ErrorString(ancestors).original}; + if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); } + ancestors = m_pool.CalculateMemPoolAncestors(*entry, cpfp_carve_out_limits); + if (!ancestors) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); } + ws.m_ancestors = *ancestors; + // 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 ws.m_conflicts and ws.m_ancestors don't @@ -1114,15 +1115,18 @@ 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. - std::string unused_err_string; - if(!m_pool.CalculateMemPoolAncestors(*ws.m_entry, ws.m_ancestors, m_limits, unused_err_string)) { - 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())); + { + auto ancestors{m_pool.CalculateMemPoolAncestors(*ws.m_entry, m_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, From 5481f65849313ff947f38433b1ac28285a7f7694 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Tue, 25 Oct 2022 12:33:37 +0100 Subject: [PATCH 3/4] mempool: add AssumeCalculateMemPoolAncestors helper function There are quite a few places that assume CalculateMemPoolAncestors will return a value without raising an error. This helper function adds logging (and Assume for debug builds) that ensures robustness but increases visibility in case of unexpected failures --- src/txmempool.cpp | 14 ++++++++++++++ src/txmempool.h | 21 +++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 32f285276be..4232c6d305c 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -254,6 +254,20 @@ util::Result CTxMemPool::CalculateMemPoolAncestors( limits); } +CTxMemPool::setEntries CTxMemPool::AssumeCalculateMemPoolAncestors( + std::string_view calling_fn_name, + const CTxMemPoolEntry &entry, + const Limits& limits, + bool fSearchForParents /* = true */) const +{ + auto result{Assume(CalculateMemPoolAncestors(entry, limits, fSearchForParents))}; + if (!result) { + LogPrintLevel(BCLog::MEMPOOL, BCLog::Level::Error, "%s: CalculateMemPoolAncestors failed unexpectedly, continuing with empty ancestor set (%s)\n", + calling_fn_name, util::ErrorString(result).original); + } + return std::move(result).value_or(CTxMemPool::setEntries{}); +} + void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors) { const CTxMemPoolEntry::Parents& parents = it->GetMemPoolParentsConst(); diff --git a/src/txmempool.h b/src/txmempool.h index 14b0dc3871d..c176b3602a8 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -566,6 +567,26 @@ public: const Limits& limits, bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs); + /** + * Same as CalculateMemPoolAncestors, but always returns a (non-optional) setEntries. + * Should only be used when it is assumed CalculateMemPoolAncestors would not fail. If + * CalculateMemPoolAncestors does unexpectedly fail, an empty setEntries is returned and the + * error is logged to BCLog::MEMPOOL with level BCLog::Level::Error. In debug builds, failure + * of CalculateMemPoolAncestors will lead to shutdown due to assertion failure. + * + * @param[in] calling_fn_name Name of calling function so we can properly log the call site + * + * @return a setEntries corresponding to the result of CalculateMemPoolAncestors or an empty + * setEntries if it failed + * + * @see CTXMemPool::CalculateMemPoolAncestors() + */ + setEntries AssumeCalculateMemPoolAncestors( + std::string_view calling_fn_name, + const CTxMemPoolEntry &entry, + const Limits& limits, + bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs); + /** Calculate all in-mempool ancestors of a set of transactions not already in the mempool and * check ancestor and descendant limits. Heuristics are used to estimate the ancestor and * descendant count of all entries if the package were to be added to the mempool. The limits From 47c4b1f52ab8d95d7deef83050bad49d1e3e5990 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Mon, 24 Oct 2022 18:03:07 +0100 Subject: [PATCH 4/4] mempool: log/halt when CalculateMemPoolAncestors fails unexpectedly When CalculateMemPoolAncestors fails unexpectedly (e.g. it exceeds ancestor/descendant limits even though we expect no limits to be applied), add an error log entry for increased visibility. For debug builds, the application will even halt completely since this is not supposed to happen. --- src/node/miner.cpp | 3 +-- src/policy/rbf.cpp | 7 +++---- src/rpc/mempool.cpp | 3 +-- src/txmempool.cpp | 12 ++++-------- 4 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index e507c1381cf..ddb304f2106 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -394,8 +394,7 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele continue; } - auto ancestors_result{mempool.CalculateMemPoolAncestors(*iter, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)}; - auto ancestors{std::move(ancestors_result).value_or(CTxMemPool::setEntries{})}; + auto ancestors{mempool.AssumeCalculateMemPoolAncestors(__func__, *iter, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)}; onlyUnconfirmed(ancestors); ancestors.insert(iter); diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index bdaf2d0a308..5211c18743d 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -16,7 +16,6 @@ #include #include -#include #include RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) @@ -36,9 +35,9 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) // If all the inputs have nSequence >= maxint-1, it still might be // signaled for RBF if any unconfirmed parents have signaled. - CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash()); - auto ancestors_result{pool.CalculateMemPoolAncestors(entry, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)}; - auto ancestors{std::move(ancestors_result).value_or(CTxMemPool::setEntries{})}; + const CTxMemPoolEntry entry{*pool.mapTx.find(tx.GetHash())}; + auto ancestors{pool.AssumeCalculateMemPoolAncestors(__func__, entry, CTxMemPool::Limits::NoLimits(), + /*fSearchForParents=*/false)}; for (CTxMemPool::txiter it : ancestors) { if (SignalsOptInRBF(it->GetTx())) { diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index af980de6b91..7584c9c772a 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -451,8 +451,7 @@ static RPCHelpMan getmempoolancestors() throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction not in mempool"); } - auto ancestors_result{mempool.CalculateMemPoolAncestors(*it, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)}; - auto ancestors{std::move(ancestors_result).value_or(CTxMemPool::setEntries{})}; + auto ancestors{mempool.AssumeCalculateMemPoolAncestors(__func__, *it, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)}; if (!fVerbose) { UniValue o(UniValue::VARR); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 4232c6d305c..c83f89179bd 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -349,8 +349,7 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b // mempool parents we'd calculate by searching, and it's important that // we use the cached notion of ancestor transactions as the set of // things to update for removal. - auto ancestors_result{CalculateMemPoolAncestors(entry, Limits::NoLimits(), /*fSearchForParents=*/false)}; - auto ancestors{std::move(ancestors_result).value_or(setEntries{})}; + auto ancestors{AssumeCalculateMemPoolAncestors(__func__, entry, Limits::NoLimits(), /*fSearchForParents=*/false)}; // Note that UpdateAncestorsOf severs the child links that point to // removeIt in the entries for the parents of removeIt. UpdateAncestorsOf(false, removeIt, ancestors); @@ -703,8 +702,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei assert(setParentCheck.size() == it->GetMemPoolParentsConst().size()); assert(std::equal(setParentCheck.begin(), setParentCheck.end(), it->GetMemPoolParentsConst().begin(), comp)); // Verify ancestor state is correct. - auto ancestors_result{CalculateMemPoolAncestors(*it, Limits::NoLimits())}; - auto ancestors{std::move(ancestors_result).value_or(setEntries{})}; + auto ancestors{AssumeCalculateMemPoolAncestors(__func__, *it, Limits::NoLimits())}; uint64_t nCountCheck = ancestors.size() + 1; uint64_t nSizeCheck = it->GetTxSize(); CAmount nFeesCheck = it->GetModifiedFee(); @@ -865,8 +863,7 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeD if (it != mapTx.end()) { mapTx.modify(it, [&nFeeDelta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(nFeeDelta); }); // Now update all ancestors' modified fees with descendants - auto ancestors_result{CalculateMemPoolAncestors(*it, Limits::NoLimits(), /*fSearchForParents=*/false)}; - auto ancestors{std::move(ancestors_result).value_or(setEntries{})}; + auto ancestors{AssumeCalculateMemPoolAncestors(__func__, *it, Limits::NoLimits(), /*fSearchForParents=*/false)}; for (txiter ancestorIt : ancestors) { mapTx.modify(ancestorIt, [=](CTxMemPoolEntry& e){ e.UpdateDescendantState(0, nFeeDelta, 0);}); } @@ -1004,8 +1001,7 @@ int CTxMemPool::Expire(std::chrono::seconds time) void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, bool validFeeEstimate) { - auto ancestors_result{CalculateMemPoolAncestors(entry, Limits::NoLimits())}; - auto ancestors{std::move(ancestors_result).value_or(CTxMemPool::setEntries{})}; + auto ancestors{AssumeCalculateMemPoolAncestors(__func__, entry, Limits::NoLimits())}; return addUnchecked(entry, ancestors, validFeeEstimate); }