mirror of
https://github.com/bitcoin/bitcoin.git
synced 2025-02-04 10:07:27 -05:00
80fc1af096
47c4b1f52a
mempool: log/halt when CalculateMemPoolAncestors fails unexpectedly (stickies-v)5481f65849
mempool: add AssumeCalculateMemPoolAncestors helper function (stickies-v)f911bdfff9
mempool: use util::Result for CalculateMemPoolAncestors (stickies-v)66e028f739
mempool: use util::Result for CalculateAncestorsAndCheckLimits (stickies-v) Pull request description: Upon reviewing the documentation for `CTxMemPool::CalculateMemPoolAncestors`, I noticed `setAncestors` was meant to be an `out` parameter but actually is an `in,out` parameter, as can be observed by adding `assert(setAncestors.empty());` as the first line in the function and running `make check`. This PR fixes this unexpected behaviour and introduces refactoring improvements to make intents and effects of the code more clear. ## Unexpected behaviour This behaviour occurs only in the package acceptance path, currently only triggered by `testmempoolaccept` and `submitpackage` RPCs. In `MemPoolAccept::AcceptMultipleTransactions()`, we first call `PreChecks()` and then `SubmitPackage()` with the same `Workspace ws` reference. `PreChecks` leaves `ws.m_ancestors` in a potentially non-empty state, before it is passed on to `MemPoolAccept::SubmitPackage`. `SubmitPackage` is the only place where `setAncestors` isn't guaranteed to be empty before calling `CalculateMemPoolAncestors`. The most straightforward fix is to just forcefully clear `setAncestors` at the beginning of CalculateMemPoolAncestors, which is done in the first bugfix commit. ## Improvements ### Return value instead of out-parameters This PR updates the function signatures for `CTxMemPool::CalculateMemPoolAncestors` and `CTxMemPool::CalculateAncestorsAndCheckLimits` to use a `util::Result` return type and eliminate both the `setAncestors` `in,out`-parameter as well as the error string. It simplifies the code and makes the intent and effects more explicit. ### Observability There are 7 instances where we currently call `CalculateMemPoolAncestors` without actually checking if the function succeeded because we assume that it can't fail, such as in [miner.cpp](69b10212ea/src/node/miner.cpp (L399)
). This PR adds a new wrapper `AssumeCalculateMemPoolAncestors` function that logs such unexpected failures, or in case of debug builds even halts the program. It's not crucial to the objective, more of an observability improvement that seems sensible to add on here. ACKs for top commit: achow101: ACK47c4b1f52a
w0xlt: ACK47c4b1f52a
glozow: ACK47c4b1f52a
furszy: light code review ACK47c4b1f5
aureleoules: ACK47c4b1f52a
Tree-SHA512: d908dad00d1a5645eb865c4877cc0bae74b9cd3332a3641eb4a285431aef119f9fc78172d38b55c592168a73dae83242e6af3348815f7b37cbe2d448a3a58648
182 lines
8.2 KiB
C++
182 lines
8.2 KiB
C++
// Copyright (c) 2016-2022 The Bitcoin Core developers
|
|
// Distributed under the MIT software license, see the accompanying
|
|
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
|
|
|
|
#include <policy/rbf.h>
|
|
|
|
#include <consensus/amount.h>
|
|
#include <kernel/mempool_entry.h>
|
|
#include <policy/feerate.h>
|
|
#include <primitives/transaction.h>
|
|
#include <sync.h>
|
|
#include <tinyformat.h>
|
|
#include <txmempool.h>
|
|
#include <uint256.h>
|
|
#include <util/moneystr.h>
|
|
#include <util/rbf.h>
|
|
|
|
#include <limits>
|
|
#include <vector>
|
|
|
|
RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
|
|
{
|
|
AssertLockHeld(pool.cs);
|
|
|
|
// First check the transaction itself.
|
|
if (SignalsOptInRBF(tx)) {
|
|
return RBFTransactionState::REPLACEABLE_BIP125;
|
|
}
|
|
|
|
// If this transaction is not in our mempool, then we can't be sure
|
|
// we will know about all its inputs.
|
|
if (!pool.exists(GenTxid::Txid(tx.GetHash()))) {
|
|
return RBFTransactionState::UNKNOWN;
|
|
}
|
|
|
|
// If all the inputs have nSequence >= maxint-1, it still might be
|
|
// signaled for RBF if any unconfirmed parents have signaled.
|
|
const CTxMemPoolEntry entry{*pool.mapTx.find(tx.GetHash())};
|
|
auto ancestors{pool.AssumeCalculateMemPoolAncestors(__func__, entry, CTxMemPool::Limits::NoLimits(),
|
|
/*fSearchForParents=*/false)};
|
|
|
|
for (CTxMemPool::txiter it : ancestors) {
|
|
if (SignalsOptInRBF(it->GetTx())) {
|
|
return RBFTransactionState::REPLACEABLE_BIP125;
|
|
}
|
|
}
|
|
return RBFTransactionState::FINAL;
|
|
}
|
|
|
|
RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx)
|
|
{
|
|
// If we don't have a local mempool we can only check the transaction itself.
|
|
return SignalsOptInRBF(tx) ? RBFTransactionState::REPLACEABLE_BIP125 : RBFTransactionState::UNKNOWN;
|
|
}
|
|
|
|
std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
|
|
CTxMemPool& pool,
|
|
const CTxMemPool::setEntries& iters_conflicting,
|
|
CTxMemPool::setEntries& all_conflicts)
|
|
{
|
|
AssertLockHeld(pool.cs);
|
|
const uint256 txid = tx.GetHash();
|
|
uint64_t nConflictingCount = 0;
|
|
for (const auto& mi : iters_conflicting) {
|
|
nConflictingCount += mi->GetCountWithDescendants();
|
|
// Rule #5: don't consider replacing more than MAX_REPLACEMENT_CANDIDATES
|
|
// entries from the mempool. This potentially overestimates the number of actual
|
|
// descendants (i.e. if multiple conflicts share a descendant, it will be counted multiple
|
|
// times), but we just want to be conservative to avoid doing too much work.
|
|
if (nConflictingCount > MAX_REPLACEMENT_CANDIDATES) {
|
|
return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n",
|
|
txid.ToString(),
|
|
nConflictingCount,
|
|
MAX_REPLACEMENT_CANDIDATES);
|
|
}
|
|
}
|
|
// Calculate the set of all transactions that would have to be evicted.
|
|
for (CTxMemPool::txiter it : iters_conflicting) {
|
|
pool.CalculateDescendants(it, all_conflicts);
|
|
}
|
|
return std::nullopt;
|
|
}
|
|
|
|
std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx,
|
|
const CTxMemPool& pool,
|
|
const CTxMemPool::setEntries& iters_conflicting)
|
|
{
|
|
AssertLockHeld(pool.cs);
|
|
std::set<uint256> parents_of_conflicts;
|
|
for (const auto& mi : iters_conflicting) {
|
|
for (const CTxIn& txin : mi->GetTx().vin) {
|
|
parents_of_conflicts.insert(txin.prevout.hash);
|
|
}
|
|
}
|
|
|
|
for (unsigned int j = 0; j < tx.vin.size(); j++) {
|
|
// Rule #2: We don't want to accept replacements that require low feerate junk to be
|
|
// mined first. Ideally we'd keep track of the ancestor feerates and make the decision
|
|
// based on that, but for now requiring all new inputs to be confirmed works.
|
|
//
|
|
// Note that if you relax this to make RBF a little more useful, this may break the
|
|
// CalculateMempoolAncestors RBF relaxation which subtracts the conflict count/size from the
|
|
// descendant limit.
|
|
if (!parents_of_conflicts.count(tx.vin[j].prevout.hash)) {
|
|
// Rather than check the UTXO set - potentially expensive - it's cheaper to just check
|
|
// if the new input refers to a tx that's in the mempool.
|
|
if (pool.exists(GenTxid::Txid(tx.vin[j].prevout.hash))) {
|
|
return strprintf("replacement %s adds unconfirmed input, idx %d",
|
|
tx.GetHash().ToString(), j);
|
|
}
|
|
}
|
|
}
|
|
return std::nullopt;
|
|
}
|
|
|
|
std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors,
|
|
const std::set<uint256>& direct_conflicts,
|
|
const uint256& txid)
|
|
{
|
|
for (CTxMemPool::txiter ancestorIt : ancestors) {
|
|
const uint256& hashAncestor = ancestorIt->GetTx().GetHash();
|
|
if (direct_conflicts.count(hashAncestor)) {
|
|
return strprintf("%s spends conflicting transaction %s",
|
|
txid.ToString(),
|
|
hashAncestor.ToString());
|
|
}
|
|
}
|
|
return std::nullopt;
|
|
}
|
|
|
|
std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting,
|
|
CFeeRate replacement_feerate,
|
|
const uint256& txid)
|
|
{
|
|
for (const auto& mi : iters_conflicting) {
|
|
// Don't allow the replacement to reduce the feerate of the mempool.
|
|
//
|
|
// We usually don't want to accept replacements with lower feerates than what they replaced
|
|
// as that would lower the feerate of the next block. Requiring that the feerate always be
|
|
// increased is also an easy-to-reason about way to prevent DoS attacks via replacements.
|
|
//
|
|
// We only consider the feerates of transactions being directly replaced, not their indirect
|
|
// descendants. While that does mean high feerate children are ignored when deciding whether
|
|
// or not to replace, we do require the replacement to pay more overall fees too, mitigating
|
|
// most cases.
|
|
CFeeRate original_feerate(mi->GetModifiedFee(), mi->GetTxSize());
|
|
if (replacement_feerate <= original_feerate) {
|
|
return strprintf("rejecting replacement %s; new feerate %s <= old feerate %s",
|
|
txid.ToString(),
|
|
replacement_feerate.ToString(),
|
|
original_feerate.ToString());
|
|
}
|
|
}
|
|
return std::nullopt;
|
|
}
|
|
|
|
std::optional<std::string> PaysForRBF(CAmount original_fees,
|
|
CAmount replacement_fees,
|
|
size_t replacement_vsize,
|
|
CFeeRate relay_fee,
|
|
const uint256& txid)
|
|
{
|
|
// Rule #3: The replacement fees must be greater than or equal to fees of the
|
|
// transactions it replaces, otherwise the bandwidth used by those conflicting transactions
|
|
// would not be paid for.
|
|
if (replacement_fees < original_fees) {
|
|
return strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s",
|
|
txid.ToString(), FormatMoney(replacement_fees), FormatMoney(original_fees));
|
|
}
|
|
|
|
// Rule #4: The new transaction must pay for its own bandwidth. Otherwise, we have a DoS
|
|
// vector where attackers can cause a transaction to be replaced (and relayed) repeatedly by
|
|
// increasing the fee by tiny amounts.
|
|
CAmount additional_fees = replacement_fees - original_fees;
|
|
if (additional_fees < relay_fee.GetFee(replacement_vsize)) {
|
|
return strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s",
|
|
txid.ToString(),
|
|
FormatMoney(additional_fees),
|
|
FormatMoney(relay_fee.GetFee(replacement_vsize)));
|
|
}
|
|
return std::nullopt;
|
|
}
|