Skip to content

Commit

Permalink
[refactor] make some members MemPoolAccept-wide
Browse files Browse the repository at this point in the history 
No change in behavior.

For single transaction acceptance, this is a simple refactor:
Workspace::m_all_conflicting         -> MemPoolAccept::m_all_conflicts
Workspace::m_conflicting_fees        -> MemPoolAccept::m_conflicting_fees
Workspace::m_conflicting_size        -> MemPoolAccept::m_conflicting_size
Workspace::m_replaced_transactions   -> MemPoolAccept::m_replaced_transactions

And local variables m_total_vsize and m_total_modified_fees are now
MemPoolAccept members so they can be accessed from PackageMempoolChecks.

We want these to be package-wide variables because
- Transactions could conflict with the same tx (just not the same
prevout), or their conflicts could share descendants.
- We want to compare conflicts with the package fee rather than
individual transaction fee.

We reset these MemPoolAccept-wide fields for each subpackage
evaluation to not cause state leaking, similar to temporary
coins.
  • Loading branch information
@glozow @instagibbs
glozow authored and instagibbs committed May 22, 2024
1 parent b6aec91 commit 6b33732
Showing 1 changed file with 64 additions and 39 deletions.
103 changes: 64 additions & 39 deletions src/validation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -622,18 +622,11 @@ class MemPoolAccept
/** 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
* m_conflicts and their descendants. */
CTxMemPool::setEntries m_all_conflicting;
/** All mempool ancestors of this transaction. */
CTxMemPool::setEntries m_ancestors;
/** Mempool entry constructed for this transaction. Constructed in PreChecks() but not
* inserted into the mempool until Finalize(). */
std::unique_ptr<CTxMemPoolEntry> m_entry;
/** Pointers to the transactions that have been removed from the mempool and replaced by
* 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<CTransactionRef> 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};
Expand All @@ -645,10 +638,6 @@ class MemPoolAccept
CAmount m_base_fees;
/** Base fees + any fee delta set by the user with prioritisetransaction. */
CAmount m_modified_fees;
/** Total modified fees of all transactions being replaced. */
CAmount m_conflicting_fees{0};
/** Total virtual size of all transactions being replaced. */
size_t m_conflicting_size{0};

/** If we're doing package validation (i.e. m_package_feerates=true), the "effective"
* package feerate of this transaction is the total fees divided by the total size of
Expand Down Expand Up @@ -726,9 +715,39 @@ class MemPoolAccept

Chainstate& m_active_chainstate;

/** Whether the transaction(s) would replace any mempool transactions and/or evict any siblings.
* If so, RBF rules apply. */
bool m_rbf{false};
// Fields below are per *sub*package state and must be reset prior to subsequent
// AcceptSingleTransaction and AcceptMultipleTransactions invocations
struct SubPackageState {
/** Aggregated modified fees of all transactions, used to calculate package feerate. */
CAmount m_total_modified_fees{0};
/** Aggregated virtual size of all transactions, used to calculate package feerate. */
int64_t m_total_vsize{0};

// RBF-related members
/** Whether the transaction(s) would replace any mempool transactions and/or evict any siblings.
* If so, RBF rules apply. */
bool m_rbf{false};
/** All directly conflicting mempool transactions and their descendants. */
CTxMemPool::setEntries m_all_conflicts;
/** Mempool transactions that were replaced. */
std::list<CTransactionRef> m_replaced_transactions;

/** Total modified fees of mempool transactions being replaced. */
CAmount m_conflicting_fees{0};
/** Total size (in virtual bytes) of mempool transactions being replaced. */
size_t m_conflicting_size{0};
};

struct SubPackageState m_subpackage;

/** Re-set sub-package state to not leak between evaluations */
void ClearSubPackageState() EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs)
{
m_subpackage = SubPackageState{};

// And clean coins while at it
CleanupTemporaryCoins();
}
};

bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
Expand Down Expand Up @@ -1036,7 +1055,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", *err_string);
}

m_rbf = !ws.m_conflicts.empty();
// 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;
}

Expand Down Expand Up @@ -1068,24 +1088,25 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws)
}

// 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)}) {
if (const auto err_string{GetEntriesForConflicts(tx, m_pool, ws.m_iters_conflicting, m_subpackage.m_all_conflicts)}) {
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
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)}) {
if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, m_subpackage.m_all_conflicts)}) {
// 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,
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.
for (CTxMemPool::txiter it : ws.m_all_conflicting) {
ws.m_conflicting_fees += it->GetModifiedFee();
ws.m_conflicting_size += it->GetTxSize();
for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts) {
m_subpackage.m_conflicting_fees += it->GetModifiedFee();
m_subpackage.m_conflicting_size += it->GetTxSize();
}
if (const auto err_string{PaysForRBF(ws.m_conflicting_fees, ws.m_modified_fees, ws.m_vsize,
if (const auto err_string{PaysForRBF(m_subpackage.m_conflicting_fees, ws.m_modified_fees, ws.m_vsize,
m_pool.m_opts.incremental_relay_feerate, hash)}) {
// 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.
Expand Down Expand Up @@ -1184,19 +1205,18 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
const uint256& hash = ws.m_hash;
TxValidationState& state = ws.m_state;
const bool bypass_limits = args.m_bypass_limits;

std::unique_ptr<CTxMemPoolEntry>& entry = ws.m_entry;

// Remove conflicting transactions from the mempool
for (CTxMemPool::txiter it : ws.m_all_conflicting)
for (CTxMemPool::txiter it : m_subpackage.m_all_conflicts)
{
LogPrint(BCLog::MEMPOOL, "replacing tx %s (wtxid=%s) with %s (wtxid=%s) for %s additional fees, %d delta bytes\n",
it->GetTx().GetHash().ToString(),
it->GetTx().GetWitnessHash().ToString(),
hash.ToString(),
tx.GetWitnessHash().ToString(),
FormatMoney(ws.m_modified_fees - ws.m_conflicting_fees),
(int)entry->GetTxSize() - (int)ws.m_conflicting_size);
FormatMoney(ws.m_modified_fees - m_subpackage.m_conflicting_fees),
(int)entry->GetTxSize() - (int)m_subpackage.m_conflicting_size);
TRACE7(mempool, replaced,
it->GetTx().GetHash().data(),
it->GetTxSize(),
Expand All @@ -1206,9 +1226,12 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
entry->GetTxSize(),
entry->GetFee()
);
ws.m_replaced_transactions.push_back(it->GetSharedTx());
m_subpackage.m_replaced_transactions.push_back(it->GetSharedTx());
}
m_pool.RemoveStaged(ws.m_all_conflicting, false, MemPoolRemovalReason::REPLACED);
m_pool.RemoveStaged(m_subpackage.m_all_conflicts, false, MemPoolRemovalReason::REPLACED);
// Don't attempt to process the same conflicts repeatedly during subpackage evaluation:
// they no longer exist on subsequent calls to Finalize() post-RemoveStaged
m_subpackage.m_all_conflicts.clear();
// Store transaction in memory
m_pool.addUnchecked(*entry, ws.m_ancestors);

Expand Down Expand Up @@ -1294,7 +1317,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids :
std::vector<Wtxid>{ws.m_ptx->GetWitnessHash()};
results.emplace(ws.m_ptx->GetWitnessHash(),
MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize,
MempoolAcceptResult::Success(std::move(m_subpackage.m_replaced_transactions), ws.m_vsize,
ws.m_base_fees, effective_feerate, effective_feerate_wtxids));
if (!m_pool.m_opts.signals) continue;
const CTransaction& tx = *ws.m_ptx;
Expand Down Expand Up @@ -1330,7 +1353,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
return MempoolAcceptResult::Failure(ws.m_state);
}

if (m_rbf && !ReplacementChecks(ws)) return MempoolAcceptResult::Failure(ws.m_state);
if (m_subpackage.m_rbf && !ReplacementChecks(ws)) return MempoolAcceptResult::Failure(ws.m_state);

// Perform the inexpensive checks first and avoid hashing and signature verification unless
// those checks pass, to mitigate CPU exhaustion denial-of-service attacks.
Expand All @@ -1341,7 +1364,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
const CFeeRate effective_feerate{ws.m_modified_fees, static_cast<uint32_t>(ws.m_vsize)};
// Tx was accepted, but not added
if (args.m_test_accept) {
return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize,
return MempoolAcceptResult::Success(std::move(m_subpackage.m_replaced_transactions), ws.m_vsize,
ws.m_base_fees, effective_feerate, single_wtxid);
}

Expand All @@ -1362,7 +1385,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
m_pool.m_opts.signals->TransactionAddedToMempool(tx_info, m_pool.GetAndIncrementSequence());
}

return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees,
return MempoolAcceptResult::Success(std::move(m_subpackage.m_replaced_transactions), ws.m_vsize, ws.m_base_fees,
effective_feerate, single_wtxid);
}

Expand Down Expand Up @@ -1407,6 +1430,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
// replacements, we don't need to track the coins spent. Note that this logic will need to be
// updated if package replace-by-fee is allowed in the future.
assert(!args.m_allow_replacement);
assert(!m_subpackage.m_rbf);
m_viewmempool.PackageAddTransaction(ws.m_ptx);
}

Expand All @@ -1428,26 +1452,26 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
// a child that is below mempool minimum feerate. To avoid these behaviors, callers of
// AcceptMultipleTransactions need to restrict txns topology (e.g. to ancestor sets) and check
// the feerates of individuals and subsets.
const auto m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0},
m_subpackage.m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0},
[](int64_t sum, auto& ws) { return sum + ws.m_vsize; });
const auto m_total_modified_fees = std::accumulate(workspaces.cbegin(), workspaces.cend(), CAmount{0},
m_subpackage.m_total_modified_fees = std::accumulate(workspaces.cbegin(), workspaces.cend(), CAmount{0},
[](CAmount sum, auto& ws) { return sum + ws.m_modified_fees; });
const CFeeRate package_feerate(m_total_modified_fees, m_total_vsize);
const CFeeRate package_feerate(m_subpackage.m_total_modified_fees, m_subpackage.m_total_vsize);
std::vector<Wtxid> all_package_wtxids;
all_package_wtxids.reserve(workspaces.size());
std::transform(workspaces.cbegin(), workspaces.cend(), std::back_inserter(all_package_wtxids),
[](const auto& ws) { return ws.m_ptx->GetWitnessHash(); });
TxValidationState placeholder_state;
if (args.m_package_feerates &&
!CheckFeeRate(m_total_vsize, m_total_modified_fees, placeholder_state)) {
!CheckFeeRate(m_subpackage.m_total_vsize, m_subpackage.m_total_modified_fees, placeholder_state)) {
package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed");
return PackageMempoolAcceptResult(package_state, {{workspaces.back().m_ptx->GetWitnessHash(),
MempoolAcceptResult::FeeFailure(placeholder_state, CFeeRate(m_total_modified_fees, m_total_vsize), all_package_wtxids)}});
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, m_total_vsize, package_state)) {
if (txns.size() > 1 && !PackageMempoolChecks(txns, m_subpackage.m_total_vsize, package_state)) {
return PackageMempoolAcceptResult(package_state, std::move(results));
}

Expand All @@ -1465,7 +1489,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids :
std::vector<Wtxid>{ws.m_ptx->GetWitnessHash()};
results.emplace(ws.m_ptx->GetWitnessHash(),
MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions),
MempoolAcceptResult::Success(std::move(m_subpackage.m_replaced_transactions),
ws.m_vsize, ws.m_base_fees, effective_feerate,
effective_feerate_wtxids));
}
Expand Down Expand Up @@ -1530,7 +1554,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptSubPackage(const std::vector<CTr

// Clean up m_view and m_viewmempool so that other subpackage evaluations don't have access to
// coins they shouldn't. Keep some coins in order to minimize re-fetching coins from the UTXO set.
CleanupTemporaryCoins();
// Clean up package feerate and rbf calculations
ClearSubPackageState();

return result;
}
Expand Down

0 comments on commit 6b33732

Please sign in to comment.