Skip to content

Commit 1703067

Browse files
committed
[policy] sibling eviction for v3 transactions
1 parent b5d15f7 commit 1703067

File tree

5 files changed

+76
-27
lines changed

5 files changed

+76
-27
lines changed

src/policy/v3_policy.cpp

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -130,10 +130,11 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
130130
}
131131

132132
// It shouldn't be possible to have any mempool siblings at this point. SingleV3Checks
133-
// catches mempool siblings. Also, if the package consists of connected transactions,
133+
// catches mempool siblings and sibling eviction is not extended to packages. Also, if the package consists of connected transactions,
134134
// any tx having a mempool ancestor would mean the package exceeds ancestor limits.
135135
if (!Assume(!parent_info.m_has_mempool_descendant)) {
136-
return strprintf("tx %u would exceed descendant count limit", parent_info.m_wtxid.ToString());
136+
return strprintf("tx %s (wtxid=%s) would exceed descendant count limit",
137+
parent_info.m_txid.ToString(), parent_info.m_wtxid.ToString());
137138
}
138139
}
139140
} else {
@@ -214,10 +215,20 @@ std::optional<std::pair<std::string, CTransactionRef>> SingleV3Checks(const CTra
214215
std::any_of(children.cbegin(), children.cend(),
215216
[&direct_conflicts](const CTxMemPoolEntry& child){return direct_conflicts.count(child.GetTx().GetHash()) > 0;});
216217
if (parent_entry->GetCountWithDescendants() + 1 > V3_DESCENDANT_LIMIT && !child_will_be_replaced) {
218+
// Allow sibling eviction for v3 transaction: if another child already exists, even if
219+
// we don't conflict inputs with it, consider evicting it under RBF rules. We rely on v3 rules
220+
// only permitting 1 descendant, as otherwise we would need to have logic for deciding
221+
// which descendant to evict. Skip if this isn't true, e.g. if the transaction has
222+
// multiple children or the sibling also has descendants due to a reorg.
223+
const bool consider_sibling_eviction{parent_entry->GetCountWithDescendants() == 2 &&
224+
children.begin()->get().GetCountWithAncestors() == 2};
225+
226+
// Return the sibling if its eviction can be considered. Provide the "descendant count
227+
// limit" string either way, as the caller may decide not to do sibling eviction.
217228
return std::make_pair(strprintf("tx %u (wtxid=%s) would exceed descendant count limit",
218-
parent_entry->GetSharedTx()->GetHash().ToString(),
219-
parent_entry->GetSharedTx()->GetWitnessHash().ToString()),
220-
nullptr);
229+
parent_entry->GetSharedTx()->GetHash().ToString(),
230+
parent_entry->GetSharedTx()->GetWitnessHash().ToString()),
231+
consider_sibling_eviction ? children.begin()->get().GetSharedTx() : nullptr);
221232
}
222233
}
223234
return std::nullopt;

src/policy/v3_policy.h

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,13 @@ static_assert(V3_CHILD_MAX_VSIZE + MAX_STANDARD_TX_WEIGHT / WITNESS_SCALE_FACTOR
4848
* count of in-mempool ancestors.
4949
* @param[in] vsize The sigop-adjusted virtual size of ptx.
5050
*
51-
* @returns debug string if an error occurs, std::nullopt otherwise.
51+
* @returns 3 possibilities:
52+
* - std::nullopt if all v3 checks were applied successfully
53+
* - debug string + pointer to a mempool sibling if this transaction would be the second child in a
54+
* 1-parent-1-child cluster; the caller may consider evicting the specified sibling or return an
55+
* error with the debug string.
56+
* - debug string + nullptr if this transaction violates some v3 rule and sibling eviction is not
57+
* applicable.
5258
*/
5359
std::optional<std::pair<std::string, CTransactionRef>> SingleV3Checks(const CTransactionRef& ptx,
5460
const CTxMemPool::setEntries& mempool_ancestors,

src/test/txvalidation_tests.cpp

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -329,18 +329,21 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup)
329329
BOOST_CHECK(PackageV3Checks(tx_mempool_v3_child, GetVirtualTransactionSize(*tx_mempool_v3_child), package_v3_1p1c, empty_ancestors) == std::nullopt);
330330
}
331331

332-
// A v3 transaction cannot have more than 1 descendant.
333-
// Configuration where tx has multiple direct children.
332+
// A v3 transaction cannot have more than 1 descendant. Sibling is returned when exactly 1 exists.
334333
{
335334
auto tx_v3_child2 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 1}}, /*version=*/3);
336-
auto ancestors{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child2), m_limits)};
335+
336+
// Configuration where parent already has 1 other child in mempool
337+
auto ancestors_1sibling{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child2), m_limits)};
337338
const auto expected_error_str{strprintf("tx %s (wtxid=%s) would exceed descendant count limit",
338339
mempool_tx_v3->GetHash().ToString(), mempool_tx_v3->GetWitnessHash().ToString())};
339-
auto result{SingleV3Checks(tx_v3_child2, *ancestors, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_child2))};
340-
BOOST_CHECK_EQUAL(result->first, expected_error_str);
341-
BOOST_CHECK_EQUAL(result->second, nullptr);
342-
// If replacing the child, make sure there is no double-counting.
343-
BOOST_CHECK(SingleV3Checks(tx_v3_child2, *ancestors, {tx_mempool_v3_child->GetHash()}, GetVirtualTransactionSize(*tx_v3_child2))
340+
auto result_with_sibling_eviction{SingleV3Checks(tx_v3_child2, *ancestors_1sibling, empty_conflicts_set, GetVirtualTransactionSize(*tx_v3_child2))};
341+
BOOST_CHECK_EQUAL(result_with_sibling_eviction->first, expected_error_str);
342+
// The other mempool child is returned to allow for sibling eviction.
343+
BOOST_CHECK_EQUAL(result_with_sibling_eviction->second, tx_mempool_v3_child);
344+
345+
// If directly replacing the child, make sure there is no double-counting.
346+
BOOST_CHECK(SingleV3Checks(tx_v3_child2, *ancestors_1sibling, {tx_mempool_v3_child->GetHash()}, GetVirtualTransactionSize(*tx_v3_child2))
344347
== std::nullopt);
345348

346349
Package package_v3_1p2c{mempool_tx_v3, tx_mempool_v3_child, tx_v3_child2};

src/validation.cpp

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -585,22 +585,27 @@ class MemPoolAccept
585585
// of checking a given transaction.
586586
struct Workspace {
587587
explicit Workspace(const CTransactionRef& ptx) : m_ptx(ptx), m_hash(ptx->GetHash()) {}
588-
/** Txids of mempool transactions that this transaction directly conflicts with. */
588+
/** Txids of mempool transactions that this transaction directly conflicts with or may
589+
* replace via sibling eviction. */
589590
std::set<Txid> m_conflicts;
590-
/** Iterators to mempool entries that this transaction directly conflicts with. */
591+
/** Iterators to mempool entries that this transaction directly conflicts with or may
592+
* replace via sibling eviction. */
591593
CTxMemPool::setEntries m_iters_conflicting;
592594
/** Iterators to all mempool entries that would be replaced by this transaction, including
593-
* those it directly conflicts with and their descendants. */
595+
* m_conflicts and their descendants. */
594596
CTxMemPool::setEntries m_all_conflicting;
595597
/** All mempool ancestors of this transaction. */
596598
CTxMemPool::setEntries m_ancestors;
597599
/** Mempool entry constructed for this transaction. Constructed in PreChecks() but not
598600
* inserted into the mempool until Finalize(). */
599601
std::unique_ptr<CTxMemPoolEntry> m_entry;
600602
/** Pointers to the transactions that have been removed from the mempool and replaced by
601-
* this transaction, used to return to the MemPoolAccept caller. Only populated if
603+
* this transaction (everything in m_all_conflicting), used to return to the MemPoolAccept caller. Only populated if
602604
* validation is successful and the original transactions are removed. */
603605
std::list<CTransactionRef> m_replaced_transactions;
606+
/** Whether RBF-related data structures (m_conflicts, m_iters_conflicting, m_all_conflicting,
607+
* m_replaced_transactions) include a sibling in addition to txns with conflicting inputs. */
608+
bool m_sibling_eviction{false};
604609

605610
/** Virtual size of the transaction as used by the mempool, calculated using serialized size
606611
* of the transaction and sigops. */
@@ -690,7 +695,8 @@ class MemPoolAccept
690695

691696
Chainstate& m_active_chainstate;
692697

693-
/** Whether the transaction(s) would replace any mempool transactions. If so, RBF rules apply. */
698+
/** Whether the transaction(s) would replace any mempool transactions and/or evict any siblings.
699+
* If so, RBF rules apply. */
694700
bool m_rbf{false};
695701
};
696702

@@ -954,8 +960,27 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
954960
}
955961

956962
ws.m_ancestors = *ancestors;
963+
// Even though just checking direct mempool parents for inheritance would be sufficient, we
964+
// check using the full ancestor set here because it's more convenient to use what we have
965+
// already calculated.
957966
if (const auto err{SingleV3Checks(ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) {
958-
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "v3-rule-violation", err->first);
967+
// Disabled within package validation.
968+
if (err->second != nullptr && args.m_allow_replacement) {
969+
// Potential sibling eviction. Add the sibling to our list of mempool conflicts to be
970+
// included in RBF checks.
971+
ws.m_conflicts.insert(err->second->GetHash());
972+
// Adding the sibling to m_iters_conflicting here means that it doesn't count towards
973+
// RBF Carve Out above. This is correct, since removing to-be-replaced transactions from
974+
// the descendant count is done separately in SingleV3Checks for v3 transactions.
975+
ws.m_iters_conflicting.insert(m_pool.GetIter(err->second->GetHash()).value());
976+
ws.m_sibling_eviction = true;
977+
// The sibling will be treated as part of the to-be-replaced set in ReplacementChecks.
978+
// Note that we are not checking whether it opts in to replaceability via BIP125 or v3
979+
// (which is normally done in PreChecks). However, the only way a v3 transaction can
980+
// have a non-v3 and non-BIP125 descendant is due to a reorg.
981+
} else {
982+
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "v3-rule-violation", err->first);
983+
}
959984
}
960985

961986
// A transaction that spends outputs that would be replaced by it is invalid. Now
@@ -995,18 +1020,21 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws)
9951020
// Even though this is a fee-related failure, this result is TX_MEMPOOL_POLICY, not
9961021
// TX_RECONSIDERABLE, because it cannot be bypassed using package validation.
9971022
// This must be changed if package RBF is enabled.
998-
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string);
1023+
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
1024+
strprintf("insufficient fee%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string);
9991025
}
10001026

10011027
// Calculate all conflicting entries and enforce Rule #5.
10021028
if (const auto err_string{GetEntriesForConflicts(tx, m_pool, ws.m_iters_conflicting, ws.m_all_conflicting)}) {
10031029
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
1004-
"too many potential replacements", *err_string);
1030+
strprintf("too many potential replacements%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string);
10051031
}
10061032
// Enforce Rule #2.
10071033
if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, ws.m_iters_conflicting)}) {
1034+
// Sibling eviction is only done for v3 transactions, which cannot have multiple ancestors.
1035+
Assume(!ws.m_sibling_eviction);
10081036
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
1009-
"replacement-adds-unconfirmed", *err_string);
1037+
strprintf("replacement-adds-unconfirmed%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string);
10101038
}
10111039
// Check if it's economically rational to mine this transaction rather than the ones it
10121040
// replaces and pays for its own relay fees. Enforce Rules #3 and #4.
@@ -1019,7 +1047,8 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws)
10191047
// Even though this is a fee-related failure, this result is TX_MEMPOOL_POLICY, not
10201048
// TX_RECONSIDERABLE, because it cannot be bypassed using package validation.
10211049
// This must be changed if package RBF is enabled.
1022-
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string);
1050+
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
1051+
strprintf("insufficient fee%s", ws.m_sibling_eviction ? " (including sibling eviction)" : ""), *err_string);
10231052
}
10241053
return true;
10251054
}

test/functional/mempool_accept_v3.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -311,8 +311,8 @@ def test_mempool_sibling(self):
311311
utxo_to_spend=tx_mempool_parent["new_utxos"][1],
312312
version=3
313313
)
314-
expected_error_mempool_sibling = f"v3-rule-violation, tx {tx_mempool_parent['txid']} (wtxid={tx_mempool_parent['wtxid']}) would exceed descendant count limit"
315-
assert_raises_rpc_error(-26, expected_error_mempool_sibling, node.sendrawtransaction, tx_has_mempool_sibling["hex"])
314+
expected_error_mempool_sibling_no_eviction = f"insufficient fee (including sibling eviction), rejecting replacement"
315+
assert_raises_rpc_error(-26, expected_error_mempool_sibling_no_eviction, node.sendrawtransaction, tx_has_mempool_sibling["hex"])
316316

317317
tx_has_mempool_uncle = self.wallet.create_self_transfer(utxo_to_spend=tx_has_mempool_sibling["new_utxo"], version=3)
318318

@@ -326,7 +326,7 @@ def test_mempool_sibling(self):
326326

327327
# Also fails with a child via submitpackage
328328
result_submitpackage = node.submitpackage([tx_has_mempool_sibling["hex"], tx_has_mempool_uncle["hex"]])
329-
assert_equal(result_submitpackage["tx-results"][tx_has_mempool_sibling['wtxid']]['error'], expected_error_mempool_sibling)
329+
assert expected_error_mempool_sibling_no_eviction in result_submitpackage["tx-results"][tx_has_mempool_sibling['wtxid']]['error']
330330

331331

332332
@cleanup(extra_args=["-datacarriersize=1000", "-acceptnonstdtxn=1"])

0 commit comments

Comments
 (0)