diff --git a/src/policy/v3_policy.cpp b/src/policy/v3_policy.cpp index f838dc6c0fd..3c3942d7073 100644 --- a/src/policy/v3_policy.cpp +++ b/src/policy/v3_policy.cpp @@ -130,10 +130,11 @@ std::optional PackageV3Checks(const CTransactionRef& ptx, int64_t v } // It shouldn't be possible to have any mempool siblings at this point. SingleV3Checks - // catches mempool siblings. Also, if the package consists of connected transactions, + // catches mempool siblings and sibling eviction is not extended to packages. Also, if the package consists of connected transactions, // any tx having a mempool ancestor would mean the package exceeds ancestor limits. if (!Assume(!parent_info.m_has_mempool_descendant)) { - return strprintf("tx %u would exceed descendant count limit", parent_info.m_wtxid.ToString()); + return strprintf("tx %s (wtxid=%s) would exceed descendant count limit", + parent_info.m_txid.ToString(), parent_info.m_wtxid.ToString()); } } } else { @@ -158,7 +159,7 @@ std::optional PackageV3Checks(const CTransactionRef& ptx, int64_t v return std::nullopt; } -std::optional SingleV3Checks(const CTransactionRef& ptx, +std::optional> SingleV3Checks(const CTransactionRef& ptx, const CTxMemPool::setEntries& mempool_ancestors, const std::set& direct_conflicts, int64_t vsize) @@ -166,13 +167,15 @@ std::optional SingleV3Checks(const CTransactionRef& ptx, // Check v3 and non-v3 inheritance. for (const auto& entry : mempool_ancestors) { if (ptx->nVersion != 3 && entry->GetTx().nVersion == 3) { - return strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)", + return std::make_pair(strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), - entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString()); + entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString()), + nullptr); } else if (ptx->nVersion == 3 && entry->GetTx().nVersion != 3) { - return strprintf("v3 tx %s (wtxid=%s) cannot spend from non-v3 tx %s (wtxid=%s)", + return std::make_pair(strprintf("v3 tx %s (wtxid=%s) cannot spend from non-v3 tx %s (wtxid=%s)", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), - entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString()); + entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString()), + nullptr); } } @@ -185,16 +188,18 @@ std::optional SingleV3Checks(const CTransactionRef& ptx, // Check that V3_ANCESTOR_LIMIT would not be violated. if (mempool_ancestors.size() + 1 > V3_ANCESTOR_LIMIT) { - return strprintf("tx %s (wtxid=%s) would have too many ancestors", - ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); + 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 this transaction spends V3 parents, it cannot be too large. if (vsize > V3_CHILD_MAX_VSIZE) { - return strprintf("v3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes", - ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, V3_CHILD_MAX_VSIZE); + return std::make_pair(strprintf("v3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes", + ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, V3_CHILD_MAX_VSIZE), + nullptr); } // Check the descendant counts of in-mempool ancestors. @@ -210,9 +215,20 @@ std::optional SingleV3Checks(const CTransactionRef& ptx, std::any_of(children.cbegin(), children.cend(), [&direct_conflicts](const CTxMemPoolEntry& child){return direct_conflicts.count(child.GetTx().GetHash()) > 0;}); if (parent_entry->GetCountWithDescendants() + 1 > V3_DESCENDANT_LIMIT && !child_will_be_replaced) { - return strprintf("tx %u (wtxid=%s) would exceed descendant count limit", - parent_entry->GetSharedTx()->GetHash().ToString(), - parent_entry->GetSharedTx()->GetWitnessHash().ToString()); + // Allow sibling eviction for v3 transaction: if another child already exists, even if + // we don't conflict inputs with it, consider evicting it under RBF rules. We rely on v3 rules + // only permitting 1 descendant, as otherwise we would need to have logic for deciding + // which descendant to evict. Skip if this isn't true, e.g. if the transaction has + // multiple children or the sibling also has descendants due to a reorg. + const bool consider_sibling_eviction{parent_entry->GetCountWithDescendants() == 2 && + children.begin()->get().GetCountWithAncestors() == 2}; + + // 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()), + consider_sibling_eviction ? children.begin()->get().GetSharedTx() : nullptr); } } return std::nullopt; diff --git a/src/policy/v3_policy.h b/src/policy/v3_policy.h index 9e871915e51..2e56f8822bb 100644 --- a/src/policy/v3_policy.h +++ b/src/policy/v3_policy.h @@ -48,9 +48,15 @@ static_assert(V3_CHILD_MAX_VSIZE + MAX_STANDARD_TX_WEIGHT / WITNESS_SCALE_FACTOR * count of in-mempool ancestors. * @param[in] vsize The sigop-adjusted virtual size of ptx. * - * @returns debug string if an error occurs, std::nullopt otherwise. + * @returns 3 possibilities: + * - std::nullopt if all v3 checks were applied successfully + * - debug string + pointer to a mempool sibling if this transaction would be the second child in a + * 1-parent-1-child cluster; the caller may consider evicting the specified sibling or return an + * error with the debug string. + * - debug string + nullptr if this transaction violates some v3 rule and sibling eviction is not + * applicable. */ -std::optional SingleV3Checks(const CTransactionRef& ptx, +std::optional> SingleV3Checks(const CTransactionRef& ptx, const CTxMemPool::setEntries& mempool_ancestors, const std::set& direct_conflicts, int64_t vsize); diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index e045949b437..95583b53bf7 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -115,7 +115,9 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) const auto expected_error_str{strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 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())}; - BOOST_CHECK(*SingleV3Checks(tx_v2_from_v3, *ancestors_v2_from_v3, empty_conflicts_set, GetVirtualTransactionSize(*tx_v2_from_v3)) == expected_error_str); + auto result_v2_from_v3{SingleV3Checks(tx_v2_from_v3, *ancestors_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(*PackageV3Checks(tx_v2_from_v3, GetVirtualTransactionSize(*tx_v2_from_v3), package_v3_v2, empty_ancestors), expected_error_str); @@ -130,8 +132,9 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) const auto expected_error_str_2{strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 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())}; - BOOST_CHECK(*SingleV3Checks(tx_v2_from_v2_and_v3, *ancestors_v2_from_both, empty_conflicts_set, GetVirtualTransactionSize(*tx_v2_from_v2_and_v3)) - == expected_error_str_2); + auto result_v2_from_both{SingleV3Checks(tx_v2_from_v2_and_v3, *ancestors_v2_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(*PackageV3Checks(tx_v2_from_v2_and_v3, GetVirtualTransactionSize(*tx_v2_from_v2_and_v3), package_v3_v2_v2, empty_ancestors), expected_error_str_2); @@ -147,7 +150,9 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) const auto expected_error_str{strprintf("v3 tx %s (wtxid=%s) cannot spend from non-v3 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())}; - BOOST_CHECK(*SingleV3Checks(tx_v3_from_v2, *ancestors_v3_from_v2, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_from_v2)) == expected_error_str); + auto result_v3_from_v2{SingleV3Checks(tx_v3_from_v2, *ancestors_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(*PackageV3Checks(tx_v3_from_v2, GetVirtualTransactionSize(*tx_v3_from_v2), package_v2_v3, empty_ancestors), expected_error_str); @@ -162,8 +167,9 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) const auto expected_error_str_2{strprintf("v3 tx %s (wtxid=%s) cannot spend from non-v3 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())}; - BOOST_CHECK(*SingleV3Checks(tx_v3_from_v2_and_v3, *ancestors_v3_from_both, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_from_v2_and_v3)) - == expected_error_str_2); + auto result_v3_from_both{SingleV3Checks(tx_v3_from_v2_and_v3, *ancestors_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); // tx_v3_from_v2_and_v3 also violates V3_ANCESTOR_LIMIT. const auto expected_error_str_3{strprintf("tx %s (wtxid=%s) would have too many ancestors", @@ -215,8 +221,9 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) 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())}; - BOOST_CHECK_EQUAL(*SingleV3Checks(tx_v3_multi_parent, *ancestors, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_multi_parent)), - expected_error_str); + auto result{SingleV3Checks(tx_v3_multi_parent, *ancestors, 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(*PackageV3Checks(tx_v3_multi_parent, GetVirtualTransactionSize(*tx_v3_multi_parent), package_multi_parents, empty_ancestors), expected_error_str); @@ -239,8 +246,9 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_multi_gen), m_limits)}; 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())}; - BOOST_CHECK_EQUAL(*SingleV3Checks(tx_v3_multi_gen, *ancestors, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_multi_gen)), - expected_error_str); + auto result{SingleV3Checks(tx_v3_multi_gen, *ancestors, 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(*PackageV3Checks(middle_tx, GetVirtualTransactionSize(*middle_tx), package_multi_gen, empty_ancestors), expected_error_str); @@ -256,8 +264,9 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child_big), m_limits)}; const auto expected_error_str{strprintf("v3 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, V3_CHILD_MAX_VSIZE)}; - BOOST_CHECK_EQUAL(*SingleV3Checks(tx_v3_child_big, *ancestors, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_child_big)), - expected_error_str); + auto result{SingleV3Checks(tx_v3_child_big, *ancestors, 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(*PackageV3Checks(tx_v3_child_big, GetVirtualTransactionSize(*tx_v3_child_big), package_child_big, empty_ancestors), @@ -298,9 +307,10 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) const auto expected_error_str{strprintf("v3 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, V3_CHILD_MAX_VSIZE)}; - BOOST_CHECK_EQUAL(*SingleV3Checks(tx_many_sigops, *ancestors, empty_conflicts_set, - GetVirtualTransactionSize(*tx_many_sigops, /*nSigOpCost=*/total_sigops, /*bytes_per_sigop=*/ DEFAULT_BYTES_PER_SIGOP)), - expected_error_str); + auto result{SingleV3Checks(tx_many_sigops, *ancestors, 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(*PackageV3Checks(tx_many_sigops, total_sigops * DEFAULT_BYTES_PER_SIGOP / WITNESS_SCALE_FACTOR, package_child_sigops, empty_ancestors), @@ -319,22 +329,58 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) BOOST_CHECK(PackageV3Checks(tx_mempool_v3_child, GetVirtualTransactionSize(*tx_mempool_v3_child), package_v3_1p1c, empty_ancestors) == std::nullopt); } - // A v3 transaction cannot have more than 1 descendant. - // Configuration where tx has multiple direct children. + // A v3 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); - auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child2), m_limits)}; + + // Configuration where parent already has 1 other child in mempool + auto ancestors_1sibling{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child2), m_limits)}; 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())}; - BOOST_CHECK_EQUAL(*SingleV3Checks(tx_v3_child2, *ancestors, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_child2)), - expected_error_str); - // If replacing the child, make sure there is no double-counting. - BOOST_CHECK(SingleV3Checks(tx_v3_child2, *ancestors, {tx_mempool_v3_child->GetHash()}, GetVirtualTransactionSize(*tx_v3_child2)) + auto result_with_sibling_eviction{SingleV3Checks(tx_v3_child2, *ancestors_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(SingleV3Checks(tx_v3_child2, *ancestors_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(*PackageV3Checks(tx_v3_child2, GetVirtualTransactionSize(*tx_v3_child2), package_v3_1p2c, empty_ancestors), 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. + pool.addUnchecked(entry.FromTx(tx_v3_child2)); + auto tx_v3_child3 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 24}}, /*version=*/3); + auto entry_mempool_parent = pool.GetIter(mempool_tx_v3->GetHash().ToUint256()).value(); + BOOST_CHECK_EQUAL(entry_mempool_parent->GetCountWithDescendants(), 3); + auto ancestors_2siblings{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child3), m_limits)}; + + auto result_2children{SingleV3Checks(tx_v3_child3, *ancestors_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); + } + + // Sibling eviction: parent already has 1 other child, which also has its own child (no sibling eviction allowed). This may happen as the result of a reorg. + { + auto tx_mempool_grandparent = make_tx(random_outpoints(1), /*version=*/3); + auto tx_mempool_sibling = make_tx({COutPoint{tx_mempool_grandparent->GetHash(), 0}}, /*version=*/3); + auto tx_mempool_nibling = make_tx({COutPoint{tx_mempool_sibling->GetHash(), 0}}, /*version=*/3); + auto tx_to_submit = make_tx({COutPoint{tx_mempool_grandparent->GetHash(), 1}}, /*version=*/3); + + pool.addUnchecked(entry.FromTx(tx_mempool_grandparent)); + pool.addUnchecked(entry.FromTx(tx_mempool_sibling)); + pool.addUnchecked(entry.FromTx(tx_mempool_nibling)); + + auto ancestors_3gen{pool.CalculateMemPoolAncestors(entry.FromTx(tx_to_submit), m_limits)}; + 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{SingleV3Checks(tx_to_submit, *ancestors_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); } // Configuration where tx has multiple generations of descendants is not tested because that is diff --git a/src/validation.cpp b/src/validation.cpp index e3ec208379c..94d2680db74 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -589,12 +589,14 @@ private: // of checking a given transaction. struct Workspace { explicit Workspace(const CTransactionRef& ptx) : m_ptx(ptx), m_hash(ptx->GetHash()) {} - /** Txids of mempool transactions that this transaction directly conflicts with. */ + /** Txids of mempool transactions that this transaction directly conflicts with or may + * replace via sibling eviction. */ std::set m_conflicts; - /** Iterators to mempool entries that this transaction directly conflicts with. */ + /** Iterators to mempool entries that this transaction directly conflicts with or may + * replace via sibling eviction. */ CTxMemPool::setEntries m_iters_conflicting; /** Iterators to all mempool entries that would be replaced by this transaction, including - * those it directly conflicts with and their descendants. */ + * m_conflicts and their descendants. */ CTxMemPool::setEntries m_all_conflicting; /** All mempool ancestors of this transaction. */ CTxMemPool::setEntries m_ancestors; @@ -602,9 +604,12 @@ private: * inserted into the mempool until Finalize(). */ std::unique_ptr m_entry; /** Pointers to the transactions that have been removed from the mempool and replaced by - * this transaction, used to return to the MemPoolAccept caller. Only populated if + * this transaction (everything in m_all_conflicting), used to return to the MemPoolAccept caller. Only populated if * validation is successful and the original transactions are removed. */ std::list m_replaced_transactions; + /** Whether RBF-related data structures (m_conflicts, m_iters_conflicting, m_all_conflicting, + * m_replaced_transactions) include a sibling in addition to txns with conflicting inputs. */ + bool m_sibling_eviction{false}; /** Virtual size of the transaction as used by the mempool, calculated using serialized size * of the transaction and sigops. */ @@ -694,7 +699,8 @@ private: Chainstate& m_active_chainstate; - /** Whether the transaction(s) would replace any mempool transactions. If so, RBF rules apply. */ + /** Whether the transaction(s) would replace any mempool transactions and/or evict any siblings. + * If so, RBF rules apply. */ bool m_rbf{false}; }; @@ -958,8 +964,27 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } ws.m_ancestors = *ancestors; - if (const auto err_string{SingleV3Checks(ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "v3-rule-violation", *err_string); + // 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 (const auto err{SingleV3Checks(ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) { + // Disabled within package validation. + if (err->second != nullptr && 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()); + // Adding the sibling to m_iters_conflicting here means that it doesn't count towards + // RBF Carve Out above. This is correct, since removing to-be-replaced transactions from + // the descendant count is done separately in SingleV3Checks for v3 transactions. + ws.m_iters_conflicting.insert(m_pool.GetIter(err->second->GetHash()).value()); + ws.m_sibling_eviction = true; + // The sibling will be treated as part of the to-be-replaced set in ReplacementChecks. + // Note that we are not checking whether it opts in to replaceability via BIP125 or v3 + // (which is normally done in PreChecks). However, the only way a v3 transaction can + // have a non-v3 and non-BIP125 descendant is due to a reorg. + } else { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "v3-rule-violation", err->first); + } } // A transaction that spends outputs that would be replaced by it is invalid. Now @@ -999,18 +1024,21 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) // Even though this is a fee-related failure, this result is TX_MEMPOOL_POLICY, not // TX_RECONSIDERABLE, because it cannot be bypassed using package validation. // This must be changed if package RBF is enabled. - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, + strprintf("insufficient fee%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } // Calculate all conflicting entries and enforce Rule #5. if (const auto err_string{GetEntriesForConflicts(tx, m_pool, ws.m_iters_conflicting, ws.m_all_conflicting)}) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, - "too many potential replacements", *err_string); + strprintf("too many potential replacements%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } // Enforce Rule #2. if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, ws.m_iters_conflicting)}) { + // Sibling eviction is only done for v3 transactions, which cannot have multiple ancestors. + Assume(!ws.m_sibling_eviction); return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, - "replacement-adds-unconfirmed", *err_string); + strprintf("replacement-adds-unconfirmed%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } // Check if it's economically rational to mine this transaction rather than the ones it // replaces and pays for its own relay fees. Enforce Rules #3 and #4. @@ -1023,7 +1051,8 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws) // Even though this is a fee-related failure, this result is TX_MEMPOOL_POLICY, not // TX_RECONSIDERABLE, because it cannot be bypassed using package validation. // This must be changed if package RBF is enabled. - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, + strprintf("insufficient fee%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string); } return true; } diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py index 7ac3c97c4b4..1b55cd0a0db 100755 --- a/test/functional/mempool_accept_v3.py +++ b/test/functional/mempool_accept_v3.py @@ -15,10 +15,13 @@ from test_framework.util import ( assert_raises_rpc_error, ) from test_framework.wallet import ( + COIN, DEFAULT_FEE, MiniWallet, ) +MAX_REPLACEMENT_CANDIDATES = 100 + def cleanup(extra_args=None): def decorator(func): def wrapper(self): @@ -290,8 +293,13 @@ class MempoolAcceptV3(BitcoinTestFramework): self.check_mempool([tx_in_mempool["txid"]]) @cleanup(extra_args=["-acceptnonstdtxn=1"]) - def test_mempool_sibling(self): - self.log.info("Test that v3 transaction cannot have mempool siblings") + def test_sibling_eviction_package(self): + """ + When a transaction has a mempool sibling, it may be eligible for sibling eviction. + However, this option is only available in single transaction acceptance. It doesn't work in + a multi-testmempoolaccept (where RBF is disabled) or when doing package CPFP. + """ + self.log.info("Test v3 sibling eviction in submitpackage and multi-testmempoolaccept") node = self.nodes[0] # Add a parent + child to mempool tx_mempool_parent = self.wallet.send_self_transfer_multi( @@ -307,26 +315,57 @@ class MempoolAcceptV3(BitcoinTestFramework): ) self.check_mempool([tx_mempool_parent["txid"], tx_mempool_sibling["txid"]]) - tx_has_mempool_sibling = self.wallet.create_self_transfer( + tx_sibling_1 = self.wallet.create_self_transfer( utxo_to_spend=tx_mempool_parent["new_utxos"][1], - version=3 + version=3, + fee_rate=DEFAULT_FEE*100, ) - expected_error_mempool_sibling = f"v3-rule-violation, tx {tx_mempool_parent['txid']} (wtxid={tx_mempool_parent['wtxid']}) would exceed descendant count limit" - assert_raises_rpc_error(-26, expected_error_mempool_sibling, node.sendrawtransaction, tx_has_mempool_sibling["hex"]) + tx_has_mempool_uncle = self.wallet.create_self_transfer(utxo_to_spend=tx_sibling_1["new_utxo"], version=3) - tx_has_mempool_uncle = self.wallet.create_self_transfer(utxo_to_spend=tx_has_mempool_sibling["new_utxo"], version=3) + tx_sibling_2 = self.wallet.create_self_transfer( + utxo_to_spend=tx_mempool_parent["new_utxos"][0], + version=3, + fee_rate=DEFAULT_FEE*200, + ) - # Also fails with another non-related transaction via testmempoolaccept + tx_sibling_3 = self.wallet.create_self_transfer( + utxo_to_spend=tx_mempool_parent["new_utxos"][1], + version=3, + fee_rate=0, + ) + tx_bumps_parent_with_sibling = self.wallet.create_self_transfer( + utxo_to_spend=tx_sibling_3["new_utxo"], + version=3, + fee_rate=DEFAULT_FEE*300, + ) + + # Fails with another non-related transaction via testmempoolaccept tx_unrelated = self.wallet.create_self_transfer(version=3) - result_test_unrelated = node.testmempoolaccept([tx_has_mempool_sibling["hex"], tx_unrelated["hex"]]) + result_test_unrelated = node.testmempoolaccept([tx_sibling_1["hex"], tx_unrelated["hex"]]) assert_equal(result_test_unrelated[0]["reject-reason"], "v3-rule-violation") - result_test_1p1c = node.testmempoolaccept([tx_has_mempool_sibling["hex"], tx_has_mempool_uncle["hex"]]) + # Fails in a package via testmempoolaccept + result_test_1p1c = node.testmempoolaccept([tx_sibling_1["hex"], tx_has_mempool_uncle["hex"]]) assert_equal(result_test_1p1c[0]["reject-reason"], "v3-rule-violation") - # Also fails with a child via submitpackage - result_submitpackage = node.submitpackage([tx_has_mempool_sibling["hex"], tx_has_mempool_uncle["hex"]]) - assert_equal(result_submitpackage["tx-results"][tx_has_mempool_sibling['wtxid']]['error'], expected_error_mempool_sibling) + # Allowed when tx is submitted in a package and evaluated individually. + # Note that the child failed since it would be the 3rd generation. + result_package_indiv = node.submitpackage([tx_sibling_1["hex"], tx_has_mempool_uncle["hex"]]) + self.check_mempool([tx_mempool_parent["txid"], tx_sibling_1["txid"]]) + expected_error_gen3 = f"v3-rule-violation, tx {tx_has_mempool_uncle['txid']} (wtxid={tx_has_mempool_uncle['wtxid']}) would have too many ancestors" + + assert_equal(result_package_indiv["tx-results"][tx_has_mempool_uncle['wtxid']]['error'], expected_error_gen3) + + # Allowed when tx is submitted in a package with in-mempool parent (which is deduplicated). + node.submitpackage([tx_mempool_parent["hex"], tx_sibling_2["hex"]]) + self.check_mempool([tx_mempool_parent["txid"], tx_sibling_2["txid"]]) + + # Child cannot pay for sibling eviction for parent, as it violates v3 topology limits + result_package_cpfp = node.submitpackage([tx_sibling_3["hex"], tx_bumps_parent_with_sibling["hex"]]) + self.check_mempool([tx_mempool_parent["txid"], tx_sibling_2["txid"]]) + expected_error_cpfp = f"v3-rule-violation, tx {tx_mempool_parent['txid']} (wtxid={tx_mempool_parent['wtxid']}) would exceed descendant count limit" + + assert_equal(result_package_cpfp["tx-results"][tx_sibling_3['wtxid']]['error'], expected_error_cpfp) @cleanup(extra_args=["-datacarriersize=1000", "-acceptnonstdtxn=1"]) @@ -429,11 +468,123 @@ class MempoolAcceptV3(BitcoinTestFramework): self.check_mempool([ancestor_tx["txid"], child_1_conflict["txid"], child_2["txid"]]) assert_equal(node.getmempoolentry(ancestor_tx["txid"])["descendantcount"], 3) + @cleanup(extra_args=["-acceptnonstdtxn=1"]) + def test_v3_sibling_eviction(self): + self.log.info("Test sibling eviction for v3") + node = self.nodes[0] + tx_v3_parent = self.wallet.send_self_transfer_multi(from_node=node, num_outputs=2, version=3) + # This is the sibling to replace + tx_v3_child_1 = self.wallet.send_self_transfer( + from_node=node, utxo_to_spend=tx_v3_parent["new_utxos"][0], fee_rate=DEFAULT_FEE * 2, version=3 + ) + assert tx_v3_child_1["txid"] in node.getrawmempool() + + self.log.info("Test tx must be higher feerate than sibling to evict it") + tx_v3_child_2_rule6 = self.wallet.create_self_transfer( + utxo_to_spend=tx_v3_parent["new_utxos"][1], fee_rate=DEFAULT_FEE, version=3 + ) + rule6_str = f"insufficient fee (including sibling eviction), rejecting replacement {tx_v3_child_2_rule6['txid']}; new feerate" + assert_raises_rpc_error(-26, rule6_str, node.sendrawtransaction, tx_v3_child_2_rule6["hex"]) + self.check_mempool([tx_v3_parent['txid'], tx_v3_child_1['txid']]) + + self.log.info("Test tx must meet absolute fee rules to evict sibling") + tx_v3_child_2_rule4 = self.wallet.create_self_transfer( + utxo_to_spend=tx_v3_parent["new_utxos"][1], fee_rate=2 * DEFAULT_FEE + Decimal("0.00000001"), version=3 + ) + rule4_str = f"insufficient fee (including sibling eviction), rejecting replacement {tx_v3_child_2_rule4['txid']}, not enough additional fees to relay" + assert_raises_rpc_error(-26, rule4_str, node.sendrawtransaction, tx_v3_child_2_rule4["hex"]) + self.check_mempool([tx_v3_parent['txid'], tx_v3_child_1['txid']]) + + self.log.info("Test tx cannot cause more than 100 evictions including RBF and sibling eviction") + # First add 4 groups of 25 transactions. + utxos_for_conflict = [] + txids_v2_100 = [] + for _ in range(4): + confirmed_utxo = self.wallet.get_utxo(confirmed_only=True) + utxos_for_conflict.append(confirmed_utxo) + # 25 is within descendant limits + chain_length = int(MAX_REPLACEMENT_CANDIDATES / 4) + chain = self.wallet.create_self_transfer_chain(chain_length=chain_length, utxo_to_spend=confirmed_utxo) + for item in chain: + txids_v2_100.append(item["txid"]) + node.sendrawtransaction(item["hex"]) + self.check_mempool(txids_v2_100 + [tx_v3_parent["txid"], tx_v3_child_1["txid"]]) + + # Replacing 100 transactions is fine + tx_v3_replacement_only = self.wallet.create_self_transfer_multi(utxos_to_spend=utxos_for_conflict, fee_per_output=4000000) + # Override maxfeerate - it costs a lot to replace these 100 transactions. + assert node.testmempoolaccept([tx_v3_replacement_only["hex"]], maxfeerate=0)[0]["allowed"] + # Adding another one exceeds the limit. + utxos_for_conflict.append(tx_v3_parent["new_utxos"][1]) + tx_v3_child_2_rule5 = self.wallet.create_self_transfer_multi(utxos_to_spend=utxos_for_conflict, fee_per_output=4000000, version=3) + rule5_str = f"too many potential replacements (including sibling eviction), rejecting replacement {tx_v3_child_2_rule5['txid']}; too many potential replacements (101 > 100)" + assert_raises_rpc_error(-26, rule5_str, node.sendrawtransaction, tx_v3_child_2_rule5["hex"]) + self.check_mempool(txids_v2_100 + [tx_v3_parent["txid"], tx_v3_child_1["txid"]]) + + self.log.info("Test sibling eviction is successful if it meets all RBF rules") + tx_v3_child_2 = self.wallet.create_self_transfer( + utxo_to_spend=tx_v3_parent["new_utxos"][1], fee_rate=DEFAULT_FEE*10, version=3 + ) + node.sendrawtransaction(tx_v3_child_2["hex"]) + self.check_mempool(txids_v2_100 + [tx_v3_parent["txid"], tx_v3_child_2["txid"]]) + + self.log.info("Test that it's possible to do a sibling eviction and RBF at the same time") + utxo_unrelated_conflict = self.wallet.get_utxo(confirmed_only=True) + tx_unrelated_replacee = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_unrelated_conflict) + assert tx_unrelated_replacee["txid"] in node.getrawmempool() + + fee_to_beat_child2 = int(tx_v3_child_2["fee"] * COIN) + + tx_v3_child_3 = self.wallet.create_self_transfer_multi( + utxos_to_spend=[tx_v3_parent["new_utxos"][0], utxo_unrelated_conflict], fee_per_output=fee_to_beat_child2*5, version=3 + ) + node.sendrawtransaction(tx_v3_child_3["hex"]) + self.check_mempool(txids_v2_100 + [tx_v3_parent["txid"], tx_v3_child_3["txid"]]) + + @cleanup(extra_args=["-acceptnonstdtxn=1"]) + def test_reorg_sibling_eviction_1p2c(self): + node = self.nodes[0] + self.log.info("Test that sibling eviction is not allowed when multiple siblings exist") + + tx_with_multi_children = self.wallet.send_self_transfer_multi(from_node=node, num_outputs=3, version=3, confirmed_only=True) + self.check_mempool([tx_with_multi_children["txid"]]) + + block_to_disconnect = self.generate(node, 1)[0] + self.check_mempool([]) + + tx_with_sibling1 = self.wallet.send_self_transfer(from_node=node, version=3, utxo_to_spend=tx_with_multi_children["new_utxos"][0]) + tx_with_sibling2 = self.wallet.send_self_transfer(from_node=node, version=3, utxo_to_spend=tx_with_multi_children["new_utxos"][1]) + self.check_mempool([tx_with_sibling1["txid"], tx_with_sibling2["txid"]]) + + # Create a reorg, bringing tx_with_multi_children back into the mempool with a descendant count of 3. + node.invalidateblock(block_to_disconnect) + self.check_mempool([tx_with_multi_children["txid"], tx_with_sibling1["txid"], tx_with_sibling2["txid"]]) + assert_equal(node.getmempoolentry(tx_with_multi_children["txid"])["descendantcount"], 3) + + # Sibling eviction is not allowed because there are two siblings + tx_with_sibling3 = self.wallet.create_self_transfer( + version=3, + utxo_to_spend=tx_with_multi_children["new_utxos"][2], + fee_rate=DEFAULT_FEE*50 + ) + expected_error_2siblings = f"v3-rule-violation, tx {tx_with_multi_children['txid']} (wtxid={tx_with_multi_children['wtxid']}) would exceed descendant count limit" + assert_raises_rpc_error(-26, expected_error_2siblings, node.sendrawtransaction, tx_with_sibling3["hex"]) + + # However, an RBF (with conflicting inputs) is possible even if the resulting cluster size exceeds 2 + tx_with_sibling3_rbf = self.wallet.send_self_transfer( + from_node=node, + version=3, + utxo_to_spend=tx_with_multi_children["new_utxos"][0], + fee_rate=DEFAULT_FEE*50 + ) + self.check_mempool([tx_with_multi_children["txid"], tx_with_sibling3_rbf["txid"], tx_with_sibling2["txid"]]) + + def run_test(self): self.log.info("Generate blocks to create UTXOs") node = self.nodes[0] self.wallet = MiniWallet(node) - self.generate(self.wallet, 110) + self.generate(self.wallet, 120) self.test_v3_acceptance() self.test_v3_replacement() self.test_v3_bip125() @@ -441,10 +592,12 @@ class MempoolAcceptV3(BitcoinTestFramework): self.test_nondefault_package_limits() self.test_v3_ancestors_package() self.test_v3_ancestors_package_and_mempool() - self.test_mempool_sibling() + self.test_sibling_eviction_package() self.test_v3_package_inheritance() self.test_v3_in_testmempoolaccept() self.test_reorg_2child_rbf() + self.test_v3_sibling_eviction() + self.test_reorg_sibling_eviction_1p2c() if __name__ == "__main__":