diff --git a/src/policy/truc_policy.cpp b/src/policy/truc_policy.cpp index 853ccd58355..c5ba866a5da 100644 --- a/src/policy/truc_policy.cpp +++ b/src/policy/truc_policy.cpp @@ -57,7 +57,7 @@ struct ParentInfo { std::optional PackageTRUCChecks(const CTxMemPool& pool, const CTransactionRef& ptx, int64_t vsize, const Package& package, - const CTxMemPool::setEntries& mempool_ancestors) + const std::vector& mempool_parents) { // This function is specialized for these limits, and must be reimplemented if they ever change. static_assert(TRUC_ANCESTOR_LIMIT == 2); @@ -65,7 +65,7 @@ std::optional PackageTRUCChecks(const CTxMemPool& pool, const CTran const auto in_package_parents{FindInPackageParents(package, ptx)}; - // Now we have all ancestors, so we can start checking TRUC rules. + // Now we have all parents, so we can start checking TRUC rules. if (ptx->version == TRUC_VERSION) { // SingleTRUCChecks should have checked this already. if (!Assume(vsize <= TRUC_MAX_VSIZE)) { @@ -73,12 +73,19 @@ std::optional PackageTRUCChecks(const CTxMemPool& pool, const CTran ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, TRUC_MAX_VSIZE); } - if (mempool_ancestors.size() + in_package_parents.size() + 1 > TRUC_ANCESTOR_LIMIT) { + if (mempool_parents.size() + in_package_parents.size() + 1 > TRUC_ANCESTOR_LIMIT) { return strprintf("tx %s (wtxid=%s) would have too many ancestors", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); } - const bool has_parent{mempool_ancestors.size() + in_package_parents.size() > 0}; + if (mempool_parents.size()) { + if (pool.GetAncestorCount(mempool_parents[0]) + in_package_parents.size() + 1 > TRUC_ANCESTOR_LIMIT) { + return strprintf("tx %s (wtxid=%s) would have too many ancestors", + ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); + } + } + + const bool has_parent{mempool_parents.size() + in_package_parents.size() > 0}; if (has_parent) { // A TRUC child cannot be too large. if (vsize > TRUC_CHILD_MAX_VSIZE) { @@ -89,12 +96,12 @@ std::optional PackageTRUCChecks(const CTxMemPool& pool, const CTran // Exactly 1 parent exists, either in mempool or package. Find it. const auto parent_info = [&] { - if (mempool_ancestors.size() > 0) { - auto& mempool_parent = *mempool_ancestors.begin(); + if (mempool_parents.size() > 0) { + const auto& mempool_parent = &mempool_parents[0].get(); return ParentInfo{mempool_parent->GetTx().GetHash(), mempool_parent->GetTx().GetWitnessHash(), mempool_parent->GetTx().version, - /*has_mempool_descendant=*/pool.GetDescendantCount(mempool_parent) > 1}; + /*has_mempool_descendant=*/pool.GetDescendantCount(*mempool_parent) > 1}; } else { auto& parent_index = in_package_parents.front(); auto& package_parent = package.at(parent_index); @@ -141,11 +148,11 @@ std::optional PackageTRUCChecks(const CTxMemPool& pool, const CTran } } else { // Non-TRUC transactions cannot have TRUC parents. - for (auto it : mempool_ancestors) { - if (it->GetTx().version == TRUC_VERSION) { + for (auto it : mempool_parents) { + if (it.get().GetTx().version == TRUC_VERSION) { return strprintf("non-version=3 tx %s (wtxid=%s) cannot spend from version=3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), - it->GetSharedTx()->GetHash().ToString(), it->GetSharedTx()->GetWitnessHash().ToString()); + it.get().GetSharedTx()->GetHash().ToString(), it.get().GetSharedTx()->GetWitnessHash().ToString()); } } for (const auto& index: in_package_parents) { @@ -162,13 +169,14 @@ std::optional PackageTRUCChecks(const CTxMemPool& pool, const CTran } std::optional> SingleTRUCChecks(const CTxMemPool& pool, const CTransactionRef& ptx, - const CTxMemPool::setEntries& mempool_ancestors, + const std::vector& mempool_parents, const std::set& direct_conflicts, int64_t vsize) { LOCK(pool.cs); // Check TRUC and non-TRUC inheritance. - for (const auto& entry : mempool_ancestors) { + for (const auto& entry_ref : mempool_parents) { + const auto& entry = &entry_ref.get(); if (ptx->version != TRUC_VERSION && entry->GetTx().version == TRUC_VERSION) { return std::make_pair(strprintf("non-version=3 tx %s (wtxid=%s) cannot spend from version=3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), @@ -196,14 +204,21 @@ std::optional> SingleTRUCChecks(const CT } // Check that TRUC_ANCESTOR_LIMIT would not be violated. - if (mempool_ancestors.size() + 1 > TRUC_ANCESTOR_LIMIT) { + if (mempool_parents.size() + 1 > TRUC_ANCESTOR_LIMIT) { return std::make_pair(strprintf("tx %s (wtxid=%s) would have too many ancestors", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()), nullptr); } // Remaining checks only pertain to transactions with unconfirmed ancestors. - if (mempool_ancestors.size() > 0) { + if (mempool_parents.size() > 0) { + // Ensure that the in-mempool parent doesn't have any additional + // ancestors, as that would also be a violation. + if (pool.GetAncestorCount(mempool_parents[0]) + 1 > TRUC_ANCESTOR_LIMIT) { + return std::make_pair(strprintf("tx %s (wtxid=%s) would have too many ancestors", + ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()), + nullptr); + } // If this transaction spends TRUC parents, it cannot be too large. if (vsize > TRUC_CHILD_MAX_VSIZE) { return std::make_pair(strprintf("version=3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes", @@ -211,14 +226,14 @@ std::optional> SingleTRUCChecks(const CT nullptr); } - // Check the descendant counts of in-mempool ancestors. - const auto& parent_entry = *mempool_ancestors.begin(); - // If there are any ancestors, this is the only child allowed. The parent cannot have any + // Check the descendant counts of in-mempool parents. + const auto& parent_entry = mempool_parents[0].get(); + // If there are any parents, this is the only child allowed. The parent cannot have any // other descendants. We handle the possibility of multiple children as that case is // possible through a reorg. CTxMemPool::setEntries descendants; - pool.CalculateDescendants(parent_entry, descendants); - descendants.erase(parent_entry); + auto parent_it = pool.CalculateDescendants(parent_entry, descendants); + descendants.erase(parent_it); // Don't double-count a transaction that is going to be replaced. This logic assumes that // any descendant of the TRUC transaction is a direct child, which makes sense because a // TRUC transaction can only have 1 descendant. @@ -237,8 +252,8 @@ std::optional> SingleTRUCChecks(const CT // Return the sibling if its eviction can be considered. Provide the "descendant count // limit" string either way, as the caller may decide not to do sibling eviction. return std::make_pair(strprintf("tx %u (wtxid=%s) would exceed descendant count limit", - parent_entry->GetSharedTx()->GetHash().ToString(), - parent_entry->GetSharedTx()->GetWitnessHash().ToString()), + parent_entry.GetSharedTx()->GetHash().ToString(), + parent_entry.GetSharedTx()->GetWitnessHash().ToString()), consider_sibling_eviction ? (*descendants.begin())->GetSharedTx() : nullptr); } } diff --git a/src/policy/truc_policy.h b/src/policy/truc_policy.h index abe5295876d..0ffd07511db 100644 --- a/src/policy/truc_policy.h +++ b/src/policy/truc_policy.h @@ -50,7 +50,7 @@ static_assert(TRUC_MAX_VSIZE + TRUC_CHILD_MAX_VSIZE <= DEFAULT_DESCENDANT_SIZE_L * * * @param[in] pool A reference to the mempool. - * @param[in] mempool_ancestors The in-mempool ancestors of ptx. + * @param[in] mempool_parents The in-mempool ancestors of ptx. * @param[in] direct_conflicts In-mempool transactions this tx conflicts with. These conflicts * are used to more accurately calculate the resulting descendant * count of in-mempool ancestors. @@ -65,7 +65,7 @@ static_assert(TRUC_MAX_VSIZE + TRUC_CHILD_MAX_VSIZE <= DEFAULT_DESCENDANT_SIZE_L * applicable. */ std::optional> SingleTRUCChecks(const CTxMemPool& pool, const CTransactionRef& ptx, - const CTxMemPool::setEntries& mempool_ancestors, + const std::vector& mempool_parents, const std::set& direct_conflicts, int64_t vsize); @@ -92,6 +92,6 @@ std::optional> SingleTRUCChecks(const CT * */ std::optional PackageTRUCChecks(const CTxMemPool& pool, const CTransactionRef& ptx, int64_t vsize, const Package& package, - const CTxMemPool::setEntries& mempool_ancestors); + const std::vector& mempool_parents); #endif // BITCOIN_POLICY_TRUC_POLICY_H diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index c0c9cc9c586..3271838e788 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -115,7 +115,6 @@ BOOST_FIXTURE_TEST_CASE(ephemeral_tests, RegTestingSetup) CTxMemPool& pool = *Assert(m_node.mempool); LOCK2(cs_main, pool.cs); TestMemPoolEntryHelper entry; - CTxMemPool::setEntries empty_ancestors; TxValidationState child_state; Wtxid child_wtxid; @@ -279,7 +278,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) LOCK2(cs_main, pool.cs); TestMemPoolEntryHelper entry; std::set empty_conflicts_set; - CTxMemPool::setEntries empty_ancestors; + std::vector empty_parents; auto mempool_tx_v3 = make_tx(random_outpoints(1), /*version=*/3); AddToMempool(pool, entry.FromTx(mempool_tx_v3)); @@ -292,33 +291,32 @@ 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))}; + auto parents_v2_from_v3 = pool.GetParents(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())}; - auto result_v2_from_v3{SingleTRUCChecks(pool, tx_v2_from_v3, ancestors_v2_from_v3, empty_conflicts_set, GetVirtualTransactionSize(*tx_v2_from_v3))}; + auto result_v2_from_v3{SingleTRUCChecks(pool, tx_v2_from_v3, parents_v2_from_v3, empty_conflicts_set, GetVirtualTransactionSize(*tx_v2_from_v3))}; BOOST_CHECK_EQUAL(result_v2_from_v3->first, expected_error_str); BOOST_CHECK_EQUAL(result_v2_from_v3->second, nullptr); Package package_v3_v2{mempool_tx_v3, tx_v2_from_v3}; - BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v2_from_v3, GetVirtualTransactionSize(*tx_v2_from_v3), package_v3_v2, empty_ancestors), expected_error_str); - CTxMemPool::setEntries entries_mempool_v3{pool.GetIter(mempool_tx_v3->GetHash()).value()}; - BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v2_from_v3, GetVirtualTransactionSize(*tx_v2_from_v3), {tx_v2_from_v3}, entries_mempool_v3), expected_error_str); + BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v2_from_v3, GetVirtualTransactionSize(*tx_v2_from_v3), package_v3_v2, empty_parents), expected_error_str); + BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v2_from_v3, GetVirtualTransactionSize(*tx_v2_from_v3), {tx_v2_from_v3}, parents_v2_from_v3), expected_error_str); // mempool_tx_v3 mempool_tx_v2 // ^ ^ // 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))}; + auto parents_2_from_both{pool.GetParents(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())}; - auto result_v2_from_both{SingleTRUCChecks(pool, tx_v2_from_v2_and_v3, ancestors_v2_from_both, empty_conflicts_set, GetVirtualTransactionSize(*tx_v2_from_v2_and_v3))}; + auto result_v2_from_both{SingleTRUCChecks(pool, tx_v2_from_v2_and_v3, parents_2_from_both, empty_conflicts_set, GetVirtualTransactionSize(*tx_v2_from_v2_and_v3))}; BOOST_CHECK_EQUAL(result_v2_from_both->first, expected_error_str_2); BOOST_CHECK_EQUAL(result_v2_from_both->second, nullptr); Package package_v3_v2_v2{mempool_tx_v3, mempool_tx_v2, tx_v2_from_v2_and_v3}; - BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v2_from_v2_and_v3, GetVirtualTransactionSize(*tx_v2_from_v2_and_v3), package_v3_v2_v2, empty_ancestors), expected_error_str_2); + BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v2_from_v2_and_v3, GetVirtualTransactionSize(*tx_v2_from_v2_and_v3), package_v3_v2_v2, empty_parents), expected_error_str_2); } // TRUC cannot spend from an unconfirmed non-TRUC transaction. @@ -327,28 +325,27 @@ 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))}; + auto parents_v3_from_v2{pool.GetParents(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())}; - auto result_v3_from_v2{SingleTRUCChecks(pool, tx_v3_from_v2, ancestors_v3_from_v2, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_from_v2))}; + auto result_v3_from_v2{SingleTRUCChecks(pool, tx_v3_from_v2, parents_v3_from_v2, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_from_v2))}; BOOST_CHECK_EQUAL(result_v3_from_v2->first, expected_error_str); BOOST_CHECK_EQUAL(result_v3_from_v2->second, nullptr); Package package_v2_v3{mempool_tx_v2, tx_v3_from_v2}; - BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v3_from_v2, GetVirtualTransactionSize(*tx_v3_from_v2), package_v2_v3, empty_ancestors), expected_error_str); - CTxMemPool::setEntries entries_mempool_v2{pool.GetIter(mempool_tx_v2->GetHash()).value()}; - BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v3_from_v2, GetVirtualTransactionSize(*tx_v3_from_v2), {tx_v3_from_v2}, entries_mempool_v2), expected_error_str); + BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v3_from_v2, GetVirtualTransactionSize(*tx_v3_from_v2), package_v2_v3, empty_parents), expected_error_str); + BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v3_from_v2, GetVirtualTransactionSize(*tx_v3_from_v2), {tx_v3_from_v2}, parents_v3_from_v2), expected_error_str); // mempool_tx_v3 mempool_tx_v2 // ^ ^ // 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))}; + auto parents_v3_from_both{pool.GetParents(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())}; - auto result_v3_from_both{SingleTRUCChecks(pool, tx_v3_from_v2_and_v3, ancestors_v3_from_both, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_from_v2_and_v3))}; + auto result_v3_from_both{SingleTRUCChecks(pool, tx_v3_from_v2_and_v3, parents_v3_from_both, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_from_v2_and_v3))}; BOOST_CHECK_EQUAL(result_v3_from_both->first, expected_error_str_2); BOOST_CHECK_EQUAL(result_v3_from_both->second, nullptr); @@ -356,7 +353,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) const auto expected_error_str_3{strprintf("tx %s (wtxid=%s) would have too many ancestors", tx_v3_from_v2_and_v3->GetHash().ToString(), tx_v3_from_v2_and_v3->GetWitnessHash().ToString())}; Package package_v3_v2_v3{mempool_tx_v3, mempool_tx_v2, tx_v3_from_v2_and_v3}; - BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v3_from_v2_and_v3, GetVirtualTransactionSize(*tx_v3_from_v2_and_v3), package_v3_v2_v3, empty_ancestors), expected_error_str_3); + BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v3_from_v2_and_v3, GetVirtualTransactionSize(*tx_v3_from_v2_and_v3), package_v3_v2_v3, empty_parents), expected_error_str_3); } // V3 from V3 is ok, and non-V3 from non-V3 is ok. { @@ -364,23 +361,23 @@ 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))}; - BOOST_CHECK(SingleTRUCChecks(pool, tx_v3_from_v3, ancestors_v3, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_from_v3)) + auto parents_v3{pool.GetParents(entry.FromTx(tx_v3_from_v3))}; + BOOST_CHECK(SingleTRUCChecks(pool, tx_v3_from_v3, parents_v3, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_from_v3)) == std::nullopt); Package package_v3_v3{mempool_tx_v3, tx_v3_from_v3}; - BOOST_CHECK(PackageTRUCChecks(pool, tx_v3_from_v3, GetVirtualTransactionSize(*tx_v3_from_v3), package_v3_v3, empty_ancestors) == std::nullopt); + BOOST_CHECK(PackageTRUCChecks(pool, tx_v3_from_v3, GetVirtualTransactionSize(*tx_v3_from_v3), package_v3_v3, empty_parents) == std::nullopt); // mempool_tx_v2 // ^ // 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))}; - BOOST_CHECK(SingleTRUCChecks(pool, tx_v2_from_v2, ancestors_v2, empty_conflicts_set, GetVirtualTransactionSize(*tx_v2_from_v2)) + auto parents_v2{pool.GetParents(entry.FromTx(tx_v2_from_v2))}; + BOOST_CHECK(SingleTRUCChecks(pool, tx_v2_from_v2, parents_v2, empty_conflicts_set, GetVirtualTransactionSize(*tx_v2_from_v2)) == std::nullopt); Package package_v2_v2{mempool_tx_v2, tx_v2_from_v2}; - BOOST_CHECK(PackageTRUCChecks(pool, tx_v2_from_v2, GetVirtualTransactionSize(*tx_v2_from_v2), package_v2_v2, empty_ancestors) == std::nullopt); + BOOST_CHECK(PackageTRUCChecks(pool, tx_v2_from_v2, GetVirtualTransactionSize(*tx_v2_from_v2), package_v2_v2, empty_parents) == std::nullopt); } // Tx spending TRUC cannot have too many mempool ancestors @@ -398,15 +395,15 @@ 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))}; - BOOST_CHECK_EQUAL(ancestors.size(), 3); + auto parents{pool.GetParents(entry.FromTx(tx_v3_multi_parent))}; + BOOST_CHECK_EQUAL(parents.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())}; - auto result{SingleTRUCChecks(pool, tx_v3_multi_parent, ancestors, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_multi_parent))}; + auto result{SingleTRUCChecks(pool, tx_v3_multi_parent, parents, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_multi_parent))}; BOOST_CHECK_EQUAL(result->first, expected_error_str); BOOST_CHECK_EQUAL(result->second, nullptr); - BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v3_multi_parent, GetVirtualTransactionSize(*tx_v3_multi_parent), package_multi_parents, empty_ancestors), + BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v3_multi_parent, GetVirtualTransactionSize(*tx_v3_multi_parent), package_multi_parents, empty_parents), expected_error_str); } @@ -424,16 +421,16 @@ 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))}; + auto parents{pool.GetParents(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))}; + auto result{SingleTRUCChecks(pool, tx_v3_multi_gen, parents, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_multi_gen))}; BOOST_CHECK_EQUAL(result->first, expected_error_str); BOOST_CHECK_EQUAL(result->second, nullptr); // Middle tx is what triggers a failure for the grandchild: - BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, middle_tx, GetVirtualTransactionSize(*middle_tx), package_multi_gen, empty_ancestors), expected_error_str); - BOOST_CHECK(PackageTRUCChecks(pool, tx_v3_multi_gen, GetVirtualTransactionSize(*tx_v3_multi_gen), package_multi_gen, empty_ancestors) == std::nullopt); + BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, middle_tx, GetVirtualTransactionSize(*middle_tx), package_multi_gen, empty_parents), expected_error_str); + BOOST_CHECK(PackageTRUCChecks(pool, tx_v3_multi_gen, GetVirtualTransactionSize(*tx_v3_multi_gen), package_multi_gen, empty_parents) == std::nullopt); } // Tx spending TRUC cannot be too large in virtual size. @@ -442,15 +439,15 @@ 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))}; + auto parents{pool.GetParents(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))}; + auto result{SingleTRUCChecks(pool, tx_v3_child_big, parents, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_child_big))}; BOOST_CHECK_EQUAL(result->first, expected_error_str); BOOST_CHECK_EQUAL(result->second, nullptr); Package package_child_big{mempool_tx_v3, tx_v3_child_big}; - BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v3_child_big, GetVirtualTransactionSize(*tx_v3_child_big), package_child_big, empty_ancestors), + BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v3_child_big, GetVirtualTransactionSize(*tx_v3_child_big), package_child_big, empty_parents), expected_error_str); } @@ -477,24 +474,24 @@ 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))}; + auto parents{pool.GetParents(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); const int64_t bip141_vsize{GetVirtualTransactionSize(*tx_many_sigops)}; // Weight limit is not reached... - BOOST_CHECK(SingleTRUCChecks(pool, tx_many_sigops, ancestors, empty_conflicts_set, bip141_vsize) == std::nullopt); + BOOST_CHECK(SingleTRUCChecks(pool, tx_many_sigops, parents, empty_conflicts_set, bip141_vsize) == std::nullopt); // ...but sigop limit is. const auto expected_error_str{strprintf("version=3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes", tx_many_sigops->GetHash().ToString(), tx_many_sigops->GetWitnessHash().ToString(), total_sigops * DEFAULT_BYTES_PER_SIGOP / WITNESS_SCALE_FACTOR, TRUC_CHILD_MAX_VSIZE)}; - auto result{SingleTRUCChecks(pool, tx_many_sigops, ancestors, empty_conflicts_set, + auto result{SingleTRUCChecks(pool, tx_many_sigops, parents, empty_conflicts_set, GetVirtualTransactionSize(*tx_many_sigops, /*nSigOpCost=*/total_sigops, /*bytes_per_sigop=*/ DEFAULT_BYTES_PER_SIGOP))}; BOOST_CHECK_EQUAL(result->first, expected_error_str); BOOST_CHECK_EQUAL(result->second, nullptr); Package package_child_sigops{mempool_tx_v3, tx_many_sigops}; - BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_many_sigops, total_sigops * DEFAULT_BYTES_PER_SIGOP / WITNESS_SCALE_FACTOR, package_child_sigops, empty_ancestors), + BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_many_sigops, total_sigops * DEFAULT_BYTES_PER_SIGOP / WITNESS_SCALE_FACTOR, package_child_sigops, empty_parents), expected_error_str); } @@ -502,32 +499,32 @@ 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))}; - BOOST_CHECK(SingleTRUCChecks(pool, tx_mempool_v3_child, ancestors, empty_conflicts_set, GetVirtualTransactionSize(*tx_mempool_v3_child)) == std::nullopt); + auto parents{pool.GetParents(entry.FromTx(tx_mempool_v3_child))}; + BOOST_CHECK(SingleTRUCChecks(pool, tx_mempool_v3_child, parents, empty_conflicts_set, GetVirtualTransactionSize(*tx_mempool_v3_child)) == std::nullopt); AddToMempool(pool, entry.FromTx(tx_mempool_v3_child)); Package package_v3_1p1c{mempool_tx_v3, tx_mempool_v3_child}; - BOOST_CHECK(PackageTRUCChecks(pool, tx_mempool_v3_child, GetVirtualTransactionSize(*tx_mempool_v3_child), package_v3_1p1c, empty_ancestors) == std::nullopt); + BOOST_CHECK(PackageTRUCChecks(pool, tx_mempool_v3_child, GetVirtualTransactionSize(*tx_mempool_v3_child), package_v3_1p1c, empty_parents) == std::nullopt); } // 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))}; + auto parents_1sibling{pool.GetParents(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))}; + auto result_with_sibling_eviction{SingleTRUCChecks(pool, tx_v3_child2, parents_1sibling, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_child2))}; BOOST_CHECK_EQUAL(result_with_sibling_eviction->first, expected_error_str); // The other mempool child is returned to allow for sibling eviction. BOOST_CHECK_EQUAL(result_with_sibling_eviction->second, tx_mempool_v3_child); // If directly replacing the child, make sure there is no double-counting. - BOOST_CHECK(SingleTRUCChecks(pool, tx_v3_child2, ancestors_1sibling, {tx_mempool_v3_child->GetHash()}, GetVirtualTransactionSize(*tx_v3_child2)) + BOOST_CHECK(SingleTRUCChecks(pool, tx_v3_child2, parents_1sibling, {tx_mempool_v3_child->GetHash()}, GetVirtualTransactionSize(*tx_v3_child2)) == std::nullopt); Package package_v3_1p2c{mempool_tx_v3, tx_mempool_v3_child, tx_v3_child2}; - BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v3_child2, GetVirtualTransactionSize(*tx_v3_child2), package_v3_1p2c, empty_ancestors), + BOOST_CHECK_EQUAL(*PackageTRUCChecks(pool, tx_v3_child2, GetVirtualTransactionSize(*tx_v3_child2), package_v3_1p2c, empty_parents), expected_error_str); // Configuration where parent already has 2 other children in mempool (no sibling eviction allowed). This may happen as the result of a reorg. @@ -535,9 +532,9 @@ 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))}; + auto parents_2siblings{pool.GetParents(entry.FromTx(tx_v3_child3))}; - auto result_2children{SingleTRUCChecks(pool, tx_v3_child3, ancestors_2siblings, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_child3))}; + auto result_2children{SingleTRUCChecks(pool, tx_v3_child3, parents_2siblings, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_child3))}; BOOST_CHECK_EQUAL(result_2children->first, expected_error_str); // The other mempool child is not returned because sibling eviction is not allowed. BOOST_CHECK_EQUAL(result_2children->second, nullptr); @@ -554,10 +551,10 @@ 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))}; + auto parents_3gen{pool.GetParents(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))}; + auto result_3gen{SingleTRUCChecks(pool, tx_to_submit, parents_3gen, empty_conflicts_set, GetVirtualTransactionSize(*tx_to_submit))}; BOOST_CHECK_EQUAL(result_3gen->first, expected_error_str); // The other mempool child is not returned because sibling eviction is not allowed. BOOST_CHECK_EQUAL(result_3gen->second, nullptr); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index fb835b1f3ad..99b7d3ef516 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -358,9 +358,16 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) // Calculates descendants of given entry and adds to setDescendants. void CTxMemPool::CalculateDescendants(txiter entryit, setEntries& setDescendants) const { - for (auto tx : m_txgraph->GetDescendants(*entryit, TxGraph::Level::MAIN)) { + (void)CalculateDescendants(*entryit, setDescendants); + return; +} + +CTxMemPool::txiter CTxMemPool::CalculateDescendants(const CTxMemPoolEntry& entry, setEntries& setDescendants) const +{ + for (auto tx : m_txgraph->GetDescendants(entry, TxGraph::Level::MAIN)) { setDescendants.insert(mapTx.iterator_to(static_cast(*tx))); } + return mapTx.iterator_to(entry); } void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReason reason) diff --git a/src/txmempool.h b/src/txmempool.h index addf3e137bd..437bdf4ebd3 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -451,10 +451,11 @@ public: util::Result CheckPackageLimits(const Package& package, int64_t total_vsize) const EXCLUSIVE_LOCKS_REQUIRED(cs); - /** Populate setDescendants with all in-mempool descendants of hash. + /** Populate setDescendants with all in-mempool descendants of given transaction. * Assumes that setDescendants includes all in-mempool descendants of anything * already in it. */ void CalculateDescendants(txiter it, setEntries& setDescendants) const EXCLUSIVE_LOCKS_REQUIRED(cs); + CTxMemPool::txiter CalculateDescendants(const CTxMemPoolEntry& entry, setEntries& setDescendants) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** The minimum fee to get into the mempool, which may itself not be enough * for larger-sized transactions. diff --git a/src/validation.cpp b/src/validation.cpp index 6a02acd5861..62b629d4b86 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -632,8 +632,8 @@ private: /** Iterators to mempool entries that this transaction directly conflicts with or may * replace via sibling eviction. */ CTxMemPool::setEntries m_iters_conflicting; - /** All mempool ancestors of this transaction. */ - CTxMemPool::setEntries m_ancestors; + /** All mempool parents of this transaction. */ + std::vector m_parents; /* Handle to the tx in the changeset */ CTxMemPool::ChangeSet::TxHandle m_tx_handle; /** Whether RBF-related data structures (m_conflicts, m_iters_conflicting, @@ -961,18 +961,15 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts); - ws.m_ancestors = m_subpackage.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle); + ws.m_parents = m_pool.GetParents(*ws.m_tx_handle); - // Even though just checking direct mempool parents for inheritance would be sufficient, we - // check using the full ancestor set here because it's more convenient to use what we have - // already calculated. if (!args.m_bypass_limits) { - if (const auto err{SingleTRUCChecks(m_pool, ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) { + // Perform the TRUC checks, using the in-mempool parents. + if (const auto err{SingleTRUCChecks(m_pool, ws.m_ptx, ws.m_parents, ws.m_conflicts, ws.m_vsize)}) { // Single transaction contexts only. if (args.m_allow_sibling_eviction && err->second != nullptr) { // We should only be considering where replacement is considered valid as well. Assume(args.m_allow_replacement); - // Potential sibling eviction. Add the sibling to our list of mempool conflicts to be // included in RBF checks. ws.m_conflicts.insert(err->second->GetHash()); @@ -991,16 +988,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } } - // 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 - // intersect. - if (const auto err_string{EntriesAndTxidsDisjoint(ws.m_ancestors, ws.m_conflicts, hash)}) { - // We classify this as a consensus error because a transaction depending on something it - // conflicts with would be inconsistent. - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", *err_string); - } - // We want to detect conflicts in any tx in a package to trigger package RBF logic m_subpackage.m_rbf |= !ws.m_conflicts.empty(); return true; @@ -1086,12 +1073,16 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: package must be 1-parent-1-child"); } - // If the package has in-mempool ancestors, we won't consider a package RBF + // If the package has in-mempool parents, we won't consider a package RBF // since it would result in a cluster larger than 2. // N.B. To relax this constraint we will need to revisit how CCoinsViewMemPool::PackageAddTransaction // is being used inside AcceptMultipleTransactions to track available inputs while processing a package. + // Specifically we would need to check that the ancestors of the new + // transactions don't intersect with the set of transactions to be removed + // due to RBF, which is not checked at all in the package acceptance + // context. for (const auto& ws : workspaces) { - if (!ws.m_ancestors.empty()) { + if (!ws.m_parents.empty()) { return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: new transaction cannot have mempool ancestors"); } } @@ -1115,7 +1106,6 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn "package RBF failed: too many potential replacements", *err_string); } - for (CTxMemPool::txiter it : all_conflicts) { m_subpackage.m_changeset->StageRemoval(it); m_subpackage.m_conflicting_fees += it->GetModifiedFee(); @@ -1361,15 +1351,6 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransactionInternal(const CTransa 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", ""); - return MempoolAcceptResult::Failure(ws.m_state); - } - if (m_subpackage.m_rbf && !ReplacementChecks(ws)) { if (ws.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) { // Failed for incentives-based fee reasons. Provide the effective feerate and which tx was included. @@ -1384,6 +1365,32 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransactionInternal(const CTransa return MempoolAcceptResult::Failure(ws.m_state); } + // Now that we've verified the cluster limit is respected, we can perform + // calculations involving the full ancestors of the tx. + if (ws.m_conflicts.size()) { + auto ancestors = m_subpackage.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle); + + // 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 this tx's ancestors don't + // intersect. + if (const auto err_string{EntriesAndTxidsDisjoint(ancestors, ws.m_conflicts, ptx->GetHash())}) { + // We classify this as a consensus error because a transaction depending on something it + // conflicts with would be inconsistent. + ws.m_state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", *err_string); + 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", ""); + return MempoolAcceptResult::Failure(ws.m_state); + } + if (m_pool.m_opts.require_standard) { Wtxid dummy_wtxid; if (!CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool, ws.m_state, dummy_wtxid)) { @@ -1490,10 +1497,10 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactionsInternal(con m_viewmempool.PackageAddTransaction(ws.m_ptx); } - // At this point we have all in-mempool ancestors, and we know every transaction's vsize. + // At this point we have all in-mempool parents, and we know every transaction's vsize. // Run the TRUC checks on the package. for (Workspace& ws : workspaces) { - if (auto err{PackageTRUCChecks(m_pool, ws.m_ptx, ws.m_vsize, txns, ws.m_ancestors)}) { + if (auto err{PackageTRUCChecks(m_pool, ws.m_ptx, ws.m_vsize, txns, ws.m_parents)}) { package_state.Invalid(PackageValidationResult::PCKG_POLICY, "TRUC-violation", err.value()); return PackageMempoolAcceptResult(package_state, {}); } @@ -1525,9 +1532,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactionsInternal(con MempoolAcceptResult::FeeFailure(placeholder_state, CFeeRate(m_subpackage.m_total_modified_fees, m_subpackage.m_total_vsize), all_package_wtxids)}}); } - // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, - // because it's unnecessary. - if (txns.size() > 1 && !PackageMempoolChecks(txns, workspaces, m_subpackage.m_total_vsize, package_state)) { + // Apply package mempool RBF checks. + if (!PackageMempoolChecks(txns, workspaces, m_subpackage.m_total_vsize, package_state)) { return PackageMempoolAcceptResult(package_state, std::move(results)); }