highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-20 15:19:07 +01:00
Code Issues Packages Projects Releases Wiki Activity

Rework RBF and TRUC validation

Browse Source 
Calculating mempool ancestors for a new transaction should not be done until
after cluster size limits have been enforced, to limit CPU DoS potential.

Achieve this by reworking TRUC and RBF validation logic:

- TRUC policy enforcement is now done using only mempool parents of
  new transactions, not all mempool ancestors (note that it's fine to calculate
  ancestors of in-mempool transactions, if the number of such calls is
  reasonably bounded).
- RBF replacement checks are performed earlier (which allows for checking
  cluster size limits earlier, because cluster size checks cannot happen until
  after all conflicts are staged for removal).
- Verifying that a new transaction doesn't conflict with an ancestor now
  happens later, in AcceptSingleTransaction() rather than in PreChecks(). This
  means that the test is not performed at all in AcceptMultipleTransactions(),
  but in package acceptance we already disallow RBF in situations where a
  package transaction has in-mempool parents.

Also to ensure that all RBF validation logic is applied in both the single
transaction and multiple transaction cases, remove the optimization that skips
the PackageMempoolChecks() in the case of a single transaction being validated
in AcceptMultipleTransactions().
This commit is contained in:
Suhas Daftuar
2025-09-21 12:52:05 -04:00
parent 19b8479868
commit 3a646ec462
6 changed files with 137 additions and 111 deletions
Download Patch File Download Diff File Expand all files Collapse all files

76
src/validation.cpp
View File

@@ -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<CTxMemPoolEntry::CTxMemPoolEntryRef> 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<CTransactionRef>& 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<CTransactionRef>& 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));
}