diff options
author | stickies-v <stickies-v@protonmail.com> | 2022-10-09 17:19:06 +0100 |
---|---|---|
committer | stickies-v <stickies-v@protonmail.com> | 2022-12-13 15:42:49 +0000 |
commit | f911bdfff95eba3793fffaf71a31cc8bfc6f80c9 (patch) | |
tree | 3ea4d4ae42f038409ccf60af7e7c267bcc1305fa | |
parent | 66e028f7399b6511f9b73b1cef54b6a6ac38a024 (diff) | |
download | bitcoin-f911bdfff95eba3793fffaf71a31cc8bfc6f80c9.tar.xz |
mempool: use util::Result for CalculateMemPoolAncestors
Avoid using setAncestors outparameter, simplify function signatures
and avoid creating unused dummy strings.
-rw-r--r-- | src/node/interfaces.cpp | 5 | ||||
-rw-r--r-- | src/node/miner.cpp | 5 | ||||
-rw-r--r-- | src/policy/rbf.cpp | 7 | ||||
-rw-r--r-- | src/rpc/mempool.cpp | 11 | ||||
-rw-r--r-- | src/test/mempool_tests.cpp | 13 | ||||
-rw-r--r-- | src/txmempool.cpp | 54 | ||||
-rw-r--r-- | src/txmempool.h | 9 | ||||
-rw-r--r-- | src/validation.cpp | 40 |
8 files changed, 64 insertions, 80 deletions
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 212780b259..7b9baeaf3b 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 e11ec5b0f1..e507c1381c 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 994e13dd56..bdaf2d0a30 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -16,14 +16,13 @@ #include <util/rbf.h> #include <limits> +#include <utility> #include <vector> 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 039a4328e3..af980de6b9 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -23,6 +23,8 @@ #include <util/moneystr.h> #include <util/time.h> +#include <utility> + 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 b12aac6299..2966893927 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 4bdb7e682e..32f285276b 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -25,6 +25,8 @@ #include <cmath> #include <optional> +#include <string_view> +#include <utility> 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::setEntries> 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<uint64_t>(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 396c84cb19..14b0dc3871 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<setEntries> 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 1cf6fc0675..f5f3f209a8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -64,6 +64,7 @@ #include <numeric> #include <optional> #include <string> +#include <utility> 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<Workspace>& // 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, |