From 0402e6c7808017bf5c04edb4b68128ede7d1c1e7 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Sat, 14 Oct 2023 21:15:06 -0400 Subject: [PATCH] Remove unused limits from CalculateMemPoolAncestors --- src/policy/rbf.cpp | 2 +- src/rpc/mempool.cpp | 2 +- src/test/txvalidation_tests.cpp | 31 ++++++++++++++----------------- src/txmempool.cpp | 26 ++++++-------------------- src/txmempool.h | 22 ++++++---------------- src/validation.cpp | 2 +- 6 files changed, 29 insertions(+), 56 deletions(-) diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 6852cd52a33..19823464768 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -39,7 +39,7 @@ 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. const auto& entry{*Assert(pool.GetEntry(tx.GetHash()))}; - auto ancestors{pool.AssumeCalculateMemPoolAncestors(__func__, entry, CTxMemPool::Limits::NoLimits(), + auto ancestors{pool.AssumeCalculateMemPoolAncestors(__func__, entry, /*fSearchForParents=*/false)}; for (CTxMemPool::txiter it : ancestors) { diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index b5373697e7a..d558a8cb58e 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -474,7 +474,7 @@ static RPCHelpMan getmempoolancestors() throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction not in mempool"); } - auto ancestors{mempool.AssumeCalculateMemPoolAncestors(self.m_name, *entry, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)}; + auto ancestors{mempool.AssumeCalculateMemPoolAncestors(self.m_name, *entry, /*fSearchForParents=*/false)}; if (!fVerbose) { UniValue o(UniValue::VARR); diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index 89cf2f85626..27f5d959841 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -285,8 +285,6 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) AddToMempool(pool, entry.FromTx(mempool_tx_v3)); auto mempool_tx_v2 = make_tx(random_outpoints(1), /*version=*/2); AddToMempool(pool, entry.FromTx(mempool_tx_v2)); - // Default values. - CTxMemPool::Limits m_limits{}; // Cannot spend from an unconfirmed TRUC transaction unless this tx is also TRUC. { @@ -294,7 +292,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) // ^ // tx_v2_from_v3 auto tx_v2_from_v3 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 0}}, /*version=*/2); - auto ancestors_v2_from_v3{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v2_from_v3), m_limits)}; + auto ancestors_v2_from_v3{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v2_from_v3))}; const auto expected_error_str{strprintf("non-version=3 tx %s (wtxid=%s) cannot spend from version=3 tx %s (wtxid=%s)", tx_v2_from_v3->GetHash().ToString(), tx_v2_from_v3->GetWitnessHash().ToString(), mempool_tx_v3->GetHash().ToString(), mempool_tx_v3->GetWitnessHash().ToString())}; @@ -311,7 +309,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) // ^ ^ // tx_v2_from_v2_and_v3 auto tx_v2_from_v2_and_v3 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 0}, COutPoint{mempool_tx_v2->GetHash(), 0}}, /*version=*/2); - auto ancestors_v2_from_both{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v2_from_v2_and_v3), m_limits)}; + auto ancestors_v2_from_both{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v2_from_v2_and_v3))}; const auto expected_error_str_2{strprintf("non-version=3 tx %s (wtxid=%s) cannot spend from version=3 tx %s (wtxid=%s)", tx_v2_from_v2_and_v3->GetHash().ToString(), tx_v2_from_v2_and_v3->GetWitnessHash().ToString(), mempool_tx_v3->GetHash().ToString(), mempool_tx_v3->GetWitnessHash().ToString())}; @@ -329,7 +327,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) // ^ // tx_v3_from_v2 auto tx_v3_from_v2 = make_tx({COutPoint{mempool_tx_v2->GetHash(), 0}}, /*version=*/3); - auto ancestors_v3_from_v2{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_from_v2), m_limits)}; + auto ancestors_v3_from_v2{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_from_v2))}; const auto expected_error_str{strprintf("version=3 tx %s (wtxid=%s) cannot spend from non-version=3 tx %s (wtxid=%s)", tx_v3_from_v2->GetHash().ToString(), tx_v3_from_v2->GetWitnessHash().ToString(), mempool_tx_v2->GetHash().ToString(), mempool_tx_v2->GetWitnessHash().ToString())}; @@ -346,7 +344,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) // ^ ^ // tx_v3_from_v2_and_v3 auto tx_v3_from_v2_and_v3 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 0}, COutPoint{mempool_tx_v2->GetHash(), 0}}, /*version=*/3); - auto ancestors_v3_from_both{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_from_v2_and_v3), m_limits)}; + auto ancestors_v3_from_both{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_from_v2_and_v3))}; const auto expected_error_str_2{strprintf("version=3 tx %s (wtxid=%s) cannot spend from non-version=3 tx %s (wtxid=%s)", tx_v3_from_v2_and_v3->GetHash().ToString(), tx_v3_from_v2_and_v3->GetWitnessHash().ToString(), mempool_tx_v2->GetHash().ToString(), mempool_tx_v2->GetWitnessHash().ToString())}; @@ -366,7 +364,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) // ^ // tx_v3_from_v3 auto tx_v3_from_v3 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 0}}, /*version=*/3); - auto ancestors_v3{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_from_v3), m_limits)}; + auto ancestors_v3{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_from_v3))}; BOOST_CHECK(SingleTRUCChecks(pool, tx_v3_from_v3, *ancestors_v3, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_from_v3)) == std::nullopt); @@ -377,7 +375,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) // ^ // tx_v2_from_v2 auto tx_v2_from_v2 = make_tx({COutPoint{mempool_tx_v2->GetHash(), 0}}, /*version=*/2); - auto ancestors_v2{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v2_from_v2), m_limits)}; + auto ancestors_v2{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v2_from_v2))}; BOOST_CHECK(SingleTRUCChecks(pool, tx_v2_from_v2, *ancestors_v2, empty_conflicts_set, GetVirtualTransactionSize(*tx_v2_from_v2)) == std::nullopt); @@ -400,7 +398,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) } auto tx_v3_multi_parent = make_tx(mempool_outpoints, /*version=*/3); package_multi_parents.emplace_back(tx_v3_multi_parent); - auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_multi_parent), m_limits)}; + auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_multi_parent))}; BOOST_CHECK_EQUAL(ancestors->size(), 3); const auto expected_error_str{strprintf("tx %s (wtxid=%s) would have too many ancestors", tx_v3_multi_parent->GetHash().ToString(), tx_v3_multi_parent->GetWitnessHash().ToString())}; @@ -426,7 +424,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) } auto tx_v3_multi_gen = make_tx({last_outpoint}, /*version=*/3); package_multi_gen.emplace_back(tx_v3_multi_gen); - auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_multi_gen), m_limits)}; + auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_multi_gen))}; const auto expected_error_str{strprintf("tx %s (wtxid=%s) would have too many ancestors", tx_v3_multi_gen->GetHash().ToString(), tx_v3_multi_gen->GetWitnessHash().ToString())}; auto result{SingleTRUCChecks(pool, tx_v3_multi_gen, *ancestors, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_multi_gen))}; @@ -444,7 +442,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) { auto tx_v3_child_big = make_tx(many_inputs, /*version=*/3); const auto vsize{GetVirtualTransactionSize(*tx_v3_child_big)}; - auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child_big), m_limits)}; + auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child_big))}; const auto expected_error_str{strprintf("version=3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes", tx_v3_child_big->GetHash().ToString(), tx_v3_child_big->GetWitnessHash().ToString(), vsize, TRUC_CHILD_MAX_VSIZE)}; auto result{SingleTRUCChecks(pool, tx_v3_child_big, *ancestors, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_child_big))}; @@ -479,7 +477,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) mtx_many_sigops.vout.back().nValue = 10000; auto tx_many_sigops{MakeTransactionRef(mtx_many_sigops)}; - auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_many_sigops), m_limits)}; + auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_many_sigops))}; // legacy uses fAccurate = false, and the maximum number of multisig keys is used const int64_t total_sigops{static_cast(tx_many_sigops->vin.size()) * static_cast(script_multisig.GetSigOpCount(/*fAccurate=*/false))}; BOOST_CHECK_EQUAL(total_sigops, tx_many_sigops->vin.size() * MAX_PUBKEYS_PER_MULTISIG); @@ -504,7 +502,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) auto tx_mempool_v3_child = make_tx({COutPoint{mempool_tx_v3->GetHash(), 0}}, /*version=*/3); { BOOST_CHECK(GetTransactionWeight(*tx_mempool_v3_child) <= TRUC_CHILD_MAX_VSIZE * WITNESS_SCALE_FACTOR); - auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_mempool_v3_child), m_limits)}; + auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_mempool_v3_child))}; BOOST_CHECK(SingleTRUCChecks(pool, tx_mempool_v3_child, *ancestors, empty_conflicts_set, GetVirtualTransactionSize(*tx_mempool_v3_child)) == std::nullopt); AddToMempool(pool, entry.FromTx(tx_mempool_v3_child)); @@ -515,9 +513,8 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) // A TRUC transaction cannot have more than 1 descendant. Sibling is returned when exactly 1 exists. { auto tx_v3_child2 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 1}}, /*version=*/3); - // Configuration where parent already has 1 other child in mempool - auto ancestors_1sibling{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child2), m_limits)}; + auto ancestors_1sibling{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child2))}; const auto expected_error_str{strprintf("tx %s (wtxid=%s) would exceed descendant count limit", mempool_tx_v3->GetHash().ToString(), mempool_tx_v3->GetWitnessHash().ToString())}; auto result_with_sibling_eviction{SingleTRUCChecks(pool, tx_v3_child2, *ancestors_1sibling, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_child2))}; @@ -538,7 +535,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) auto tx_v3_child3 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 24}}, /*version=*/3); auto entry_mempool_parent = pool.GetIter(mempool_tx_v3->GetHash()).value(); BOOST_CHECK_EQUAL(pool.GetDescendantCount(entry_mempool_parent), 3); - auto ancestors_2siblings{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child3), m_limits)}; + auto ancestors_2siblings{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child3))}; auto result_2children{SingleTRUCChecks(pool, tx_v3_child3, *ancestors_2siblings, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_child3))}; BOOST_CHECK_EQUAL(result_2children->first, expected_error_str); @@ -557,7 +554,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) AddToMempool(pool, entry.FromTx(tx_mempool_sibling)); AddToMempool(pool, entry.FromTx(tx_mempool_nibling)); - auto ancestors_3gen{pool.CalculateMemPoolAncestors(entry.FromTx(tx_to_submit), m_limits)}; + auto ancestors_3gen{pool.CalculateMemPoolAncestors(entry.FromTx(tx_to_submit))}; const auto expected_error_str{strprintf("tx %s (wtxid=%s) would exceed descendant count limit", tx_mempool_grandparent->GetHash().ToString(), tx_mempool_grandparent->GetWitnessHash().ToString())}; auto result_3gen{SingleTRUCChecks(pool, tx_to_submit, *ancestors_3gen, empty_conflicts_set, GetVirtualTransactionSize(*tx_to_submit))}; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 233e34210c7..d16a5efc4d6 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -99,11 +99,7 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector& vHashesToU } } -util::Result CTxMemPool::CalculateAncestorsAndCheckLimits( - int64_t entry_size, - size_t entry_count, - CTxMemPoolEntry::Parents& staged_ancestors, - const Limits& limits) const +util::Result CTxMemPool::CalculateAncestors(CTxMemPoolEntry::Parents& staged_ancestors) const { setEntries ancestors; @@ -141,11 +137,7 @@ util::Result 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. - const auto ancestors{CalculateAncestorsAndCheckLimits(total_vsize, package.size(), - staged_ancestors, m_opts.limits)}; + const auto ancestors{CalculateAncestors(staged_ancestors)}; // It's possible to overestimate the ancestor/descendant totals. if (!ancestors.has_value()) return util::Error{Untranslated("possibly " + util::ErrorString(ancestors).original)}; return {}; @@ -161,7 +153,6 @@ bool CTxMemPool::HasDescendants(const Txid& txid) const util::Result CTxMemPool::CalculateMemPoolAncestors( const CTxMemPoolEntry &entry, - const Limits& limits, bool fSearchForParents /* = true */) const { CTxMemPoolEntry::Parents staged_ancestors; @@ -184,17 +175,15 @@ util::Result CTxMemPool::CalculateMemPoolAncestors( staged_ancestors = it->GetMemPoolParentsConst(); } - return CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /*entry_count=*/1, staged_ancestors, - limits); + return CalculateAncestors(staged_ancestors); } CTxMemPool::setEntries CTxMemPool::AssumeCalculateMemPoolAncestors( std::string_view calling_fn_name, const CTxMemPoolEntry &entry, - const Limits& limits, bool fSearchForParents /* = true */) const { - auto result{CalculateMemPoolAncestors(entry, limits, fSearchForParents)}; + auto result{CalculateMemPoolAncestors(entry, fSearchForParents)}; if (!Assume(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); @@ -283,10 +272,7 @@ void CTxMemPool::Apply(ChangeSet* changeset) // We can only use a cached ancestor calculation for the first // transaction in a package, because in-package parents won't be // present in the cached ancestor sets of in-package children. - // We pass in Limits::NoLimits() to ensure that this function won't fail - // (we're going to be applying this set of transactions whether or - // not the mempool policy limits are being respected). - ancestors = *Assume(changeset->CalculateMemPoolAncestors(tx_entry, Limits::NoLimits())); + ancestors = *Assume(changeset->CalculateMemPoolAncestors(tx_entry)); } // First splice this entry into mapTx. auto node_handle = changeset->m_to_add.extract(tx_entry); @@ -306,7 +292,7 @@ void CTxMemPool::Apply(ChangeSet* changeset) void CTxMemPool::addNewTransaction(CTxMemPool::txiter it) { - auto ancestors{AssumeCalculateMemPoolAncestors(__func__, *it, Limits::NoLimits())}; + auto ancestors{AssumeCalculateMemPoolAncestors(__func__, *it)}; return addNewTransaction(it, ancestors); } diff --git a/src/txmempool.h b/src/txmempool.h index b5da8c4cd37..40f5ffeba8f 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -306,21 +306,14 @@ private: /** - * Helper function to calculate all in-mempool ancestors of staged_ancestors and apply ancestor - * and descendant limits (including staged_ancestors themselves, entry_size and entry_count). + * Helper function to calculate all in-mempool ancestors of staged_ancestors * - * @param[in] entry_size Virtual size to include in the limits. - * @param[in] entry_count How many entries to include in the limits. * @param[in] staged_ancestors Should contain entries in the mempool. - * @param[in] limits Maximum number and size of ancestors and descendants * * @return all in-mempool ancestors, or an error if any ancestor or descendant limits were hit */ - util::Result CalculateAncestorsAndCheckLimits(int64_t entry_size, - size_t entry_count, - CTxMemPoolEntry::Parents &staged_ancestors, - const Limits& limits - ) const EXCLUSIVE_LOCKS_REQUIRED(cs); + util::Result CalculateAncestors(CTxMemPoolEntry::Parents &staged_ancestors) + const EXCLUSIVE_LOCKS_REQUIRED(cs); static TxMempoolInfo GetInfo(CTxMemPool::indexed_transaction_set::const_iterator it) { @@ -437,15 +430,13 @@ public: * (these are all calculated including the tx itself) * * @param[in] entry CTxMemPoolEntry of which all in-mempool ancestors are calculated - * @param[in] limits Maximum number and size of ancestors and descendants * @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 all in-mempool ancestors, or an error if any ancestor or descendant limits were hit + * @return all in-mempool ancestors */ util::Result CalculateMemPoolAncestors(const CTxMemPoolEntry& entry, - const Limits& limits, bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** @@ -465,7 +456,6 @@ public: setEntries AssumeCalculateMemPoolAncestors( std::string_view calling_fn_name, const CTxMemPoolEntry &entry, - const Limits& limits, bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs); bool HasDescendants(const Txid& txid) const; @@ -729,7 +719,7 @@ public: /** Check if any cluster limits are exceeded. Returns true if pass, false if fail. */ bool CheckMemPoolPolicyLimits(); - util::Result CalculateMemPoolAncestors(TxHandle tx, const Limits& limits) + util::Result CalculateMemPoolAncestors(TxHandle tx) { // Look up transaction in our cache first auto it = m_ancestors.find(tx); @@ -738,7 +728,7 @@ public: // 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)}; + auto ret{m_pool->CalculateMemPoolAncestors(*tx)}; if (ret) m_ancestors.try_emplace(tx, *ret); return ret; } diff --git a/src/validation.cpp b/src/validation.cpp index 348e610238e..49f8af3ec94 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -962,7 +962,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts); // Calculate in-mempool ancestors, up to a limit. - if (auto ancestors{m_subpackage.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, m_pool.m_opts.limits)}) { + if (auto ancestors{m_subpackage.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle)}) { ws.m_ancestors = std::move(*ancestors); } else { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", util::ErrorString(ancestors).original);