Skip to content

Commit

Permalink
Merge bitcoin#28472: Remove MemPoolAccept::m_limits to avoid mutating…
Browse files Browse the repository at this point in the history
… it in package evaluation

ee589d4 Add regression test for m_limit mutation (Greg Sanders)
275579d Remove MemPoolAccept::m_limits, only have local copies for carveouts (Greg Sanders)

Pull request description:

  Without remoing it, if we ever call `PreChecks()` multiple times for any reason during any one `MempoolAccept`, subsequent invocations may have incorrect limits, allowing longer/larger chains than should be allowed.

  Currently this is only an issue with `submitpackage`, so this is not exposed on mainnet.

ACKs for top commit:
  achow101:
    ACK ee589d4
  glozow:
    ACK ee589d4, nits can be ignored
  ariard:
    Code Review ACK ee589d4

Tree-SHA512: 14cf8edc73e014220def82563f5fb4192d1c2c111829712abf16340bfbfd9a85e2148d723af6fd4995d503dd67232b48dcf8b1711668d25b5aee5eab1bdb578c
  • Loading branch information
achow101 committed Sep 20, 2023
2 parents e9a4793 + ee589d4 commit 3966b0a
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 18 deletions.
7 changes: 3 additions & 4 deletions src/txmempool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,6 @@ util::Result<CTxMemPool::setEntries> CTxMemPool::CalculateAncestorsAndCheckLimit
}

bool CTxMemPool::CheckPackageLimits(const Package& package,
const Limits& limits,
std::string &errString) const
{
CTxMemPoolEntry::Parents staged_ancestors;
Expand All @@ -208,8 +207,8 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
std::optional<txiter> piter = GetIter(input.prevout.hash);
if (piter) {
staged_ancestors.insert(**piter);
if (staged_ancestors.size() + package.size() > static_cast<uint64_t>(limits.ancestor_count)) {
errString = strprintf("too many unconfirmed parents [limit: %u]", limits.ancestor_count);
if (staged_ancestors.size() + package.size() > static_cast<uint64_t>(m_limits.ancestor_count)) {
errString = strprintf("too many unconfirmed parents [limit: %u]", m_limits.ancestor_count);
return false;
}
}
Expand All @@ -219,7 +218,7 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
// considered together must be within limits even if they are not interdependent. This may be
// stricter than the limits for each individual transaction.
const auto ancestors{CalculateAncestorsAndCheckLimits(total_size, package.size(),
staged_ancestors, limits)};
staged_ancestors, m_limits)};
// It's possible to overestimate the ancestor/descendant totals.
if (!ancestors.has_value()) errString = "possibly " + util::ErrorString(ancestors).original;
return ancestors.has_value();
Expand Down
2 changes: 0 additions & 2 deletions src/txmempool.h
Original file line number Diff line number Diff line change
Expand Up @@ -606,11 +606,9 @@ class CTxMemPool
* @param[in] package Transaction package being evaluated for acceptance
* to mempool. The transactions need not be direct
* ancestors/descendants of each other.
* @param[in] limits Maximum number and size of ancestors and descendants
* @param[out] errString Populated with error reason if a limit is hit.
*/
bool CheckPackageLimits(const Package& package,
const Limits& limits,
std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs);

/** Populate setDescendants with all in-mempool descendants of hash.
Expand Down
26 changes: 14 additions & 12 deletions src/validation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -433,8 +433,7 @@ class MemPoolAccept
m_pool(mempool),
m_view(&m_dummy),
m_viewmempool(&active_chainstate.CoinsTip(), m_pool),
m_active_chainstate(active_chainstate),
m_limits{m_pool.m_limits}
m_active_chainstate(active_chainstate)
{
}

Expand Down Expand Up @@ -684,8 +683,6 @@ class MemPoolAccept

Chainstate& m_active_chainstate;

CTxMemPool::Limits m_limits;

/** Whether the transaction(s) would replace any mempool transactions. If so, RBF rules apply. */
bool m_rbf{false};
};
Expand Down Expand Up @@ -874,6 +871,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
if (!bypass_limits && !args.m_package_feerates && !CheckFeeRate(ws.m_vsize, ws.m_modified_fees, state)) return false;

ws.m_iters_conflicting = m_pool.GetIterSet(ws.m_conflicts);

// Note that these modifications are only applicable to single transaction scenarios;
// carve-outs and package RBF are disabled for multi-transaction evaluations.
CTxMemPool::Limits maybe_rbf_limits = m_pool.m_limits;

// Calculate in-mempool ancestors, up to a limit.
if (ws.m_conflicts.size() == 1) {
// In general, when we receive an RBF transaction with mempool conflicts, we want to know whether we
Expand Down Expand Up @@ -906,11 +908,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
assert(ws.m_iters_conflicting.size() == 1);
CTxMemPool::txiter conflict = *ws.m_iters_conflicting.begin();

m_limits.descendant_count += 1;
m_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants();
maybe_rbf_limits.descendant_count += 1;
maybe_rbf_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants();
}

auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, m_limits)};
auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, maybe_rbf_limits)};
if (!ancestors) {
// If CalculateMemPoolAncestors fails second time, we want the original error string.
// Contracting/payment channels CPFP carve-out:
Expand All @@ -926,9 +928,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
// this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
CTxMemPool::Limits cpfp_carve_out_limits{
.ancestor_count = 2,
.ancestor_size_vbytes = m_limits.ancestor_size_vbytes,
.descendant_count = m_limits.descendant_count + 1,
.descendant_size_vbytes = m_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT,
.ancestor_size_vbytes = maybe_rbf_limits.ancestor_size_vbytes,
.descendant_count = maybe_rbf_limits.descendant_count + 1,
.descendant_size_vbytes = maybe_rbf_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT,
};
const auto error_message{util::ErrorString(ancestors).original};
if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT) {
Expand Down Expand Up @@ -1011,7 +1013,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
{ return !m_pool.exists(GenTxid::Txid(tx->GetHash()));}));

std::string err_string;
if (!m_pool.CheckPackageLimits(txns, m_limits, err_string)) {
if (!m_pool.CheckPackageLimits(txns, err_string)) {
// This is a package-wide error, separate from an individual transaction error.
return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", err_string);
}
Expand Down Expand Up @@ -1166,7 +1168,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
// Re-calculate mempool ancestors to call addUnchecked(). They may have changed since the
// last calculation done in PreChecks, since package ancestors have already been submitted.
{
auto ancestors{m_pool.CalculateMemPoolAncestors(*ws.m_entry, m_limits)};
auto ancestors{m_pool.CalculateMemPoolAncestors(*ws.m_entry, m_pool.m_limits)};
if(!ancestors) {
results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
// Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail.
Expand Down
51 changes: 51 additions & 0 deletions test/functional/mempool_limit.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,56 @@ def fill_mempool(self):
assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000'))
assert_greater_than(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000'))

def test_rbf_carveout_disallowed(self):
node = self.nodes[0]

self.log.info("Check that individually-evaluated transactions in a package don't increase package limits for other subpackage parts")

# We set chain limits to 2 ancestors, 1 descendant, then try to get a parents-and-child chain of 2 in mempool
#
# A: Solo transaction to be RBF'd (to bump descendant limit for package later)
# B: First transaction in package, RBFs A by itself under individual evaluation, which would give it +1 descendant limit
# C: Second transaction in package, spends B. If the +1 descendant limit persisted, would make it into mempool

self.restart_node(0, extra_args=self.extra_args[0] + ["-limitancestorcount=2", "-limitdescendantcount=1"])

# Generate a confirmed utxo we will double-spend
rbf_utxo = self.wallet.send_self_transfer(
from_node=node,
confirmed_only=True
)["new_utxo"]
self.generate(node, 1)

# tx_A needs to be RBF'd, set minfee at set size
A_weight = 1000
mempoolmin_feerate = node.getmempoolinfo()["mempoolminfee"]
tx_A = self.wallet.send_self_transfer(
from_node=node,
fee=(mempoolmin_feerate / 1000) * (A_weight // 4) + Decimal('0.000001'),
target_weight=A_weight,
utxo_to_spend=rbf_utxo,
confirmed_only=True
)

# RBF's tx_A, is not yet submitted
tx_B = self.wallet.create_self_transfer(
fee=tx_A["fee"] * 4,
target_weight=A_weight,
utxo_to_spend=rbf_utxo,
confirmed_only=True
)

# Spends tx_B's output, too big for cpfp carveout (because that would also increase the descendant limit by 1)
non_cpfp_carveout_weight = 40001 # EXTRA_DESCENDANT_TX_SIZE_LIMIT + 1
tx_C = self.wallet.create_self_transfer(
target_weight=non_cpfp_carveout_weight,
fee = (mempoolmin_feerate / 1000) * (non_cpfp_carveout_weight // 4) + Decimal('0.000001'),
utxo_to_spend=tx_B["new_utxo"],
confirmed_only=True
)

assert_raises_rpc_error(-26, "too-long-mempool-chain", node.submitpackage, [tx_B["hex"], tx_C["hex"]])

def test_mid_package_eviction(self):
node = self.nodes[0]
self.log.info("Check a package where each parent passes the current mempoolminfee but would cause eviction before package submission terminates")
Expand Down Expand Up @@ -324,6 +374,7 @@ def run_test(self):

self.test_mid_package_replacement()
self.test_mid_package_eviction()
self.test_rbf_carveout_disallowed()


if __name__ == '__main__':
Expand Down

0 comments on commit 3966b0a

Please sign in to comment.