diff options
| -rw-r--r-- | src/txmempool.cpp | 7 | ||||
| -rw-r--r-- | src/txmempool.h | 2 | ||||
| -rw-r--r-- | src/validation.cpp | 26 | ||||
| -rwxr-xr-x | test/functional/mempool_limit.py | 51 |
4 files changed, 68 insertions, 18 deletions
diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 92379484e3..bedb57c13c 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -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; @@ -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; } } @@ -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(); diff --git a/src/txmempool.h b/src/txmempool.h index fcef19e807..b26cd4efa6 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -606,11 +606,9 @@ public: * @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. diff --git a/src/validation.cpp b/src/validation.cpp index 1d4786bb17..8b5acf9ad1 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -433,8 +433,7 @@ public: 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) { } @@ -684,8 +683,6 @@ private: 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}; }; @@ -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 @@ -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: @@ -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) { @@ -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); } @@ -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. diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index 0abebbec02..a1147f70f3 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -78,6 +78,56 @@ class MempoolLimitTest(BitcoinTestFramework): 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") @@ -324,6 +374,7 @@ class MempoolLimitTest(BitcoinTestFramework): self.test_mid_package_replacement() self.test_mid_package_eviction() + self.test_rbf_carveout_disallowed() if __name__ == '__main__': |