diff options
author | Andrew Chow <github@achow101.com> | 2023-09-20 07:43:01 -0400 |
---|---|---|
committer | Andrew Chow <github@achow101.com> | 2023-09-20 07:49:13 -0400 |
commit | 3966b0a0b6b5e6110ba8106c04af1067fc6219bc (patch) | |
tree | 35e6ad2aefe97c17985d6507909c80046063cfd0 /src/validation.cpp | |
parent | e9a4793b82e40631f0b8ce435375a7690bca19d0 (diff) | |
parent | ee589d4466bb0548a6f2215afe8abd0735768dab (diff) | |
download | bitcoin-3966b0a0b6b5e6110ba8106c04af1067fc6219bc.tar.xz |
Merge bitcoin/bitcoin#28472: Remove MemPoolAccept::m_limits to avoid mutating it in package evaluation
ee589d4466bb0548a6f2215afe8abd0735768dab Add regression test for m_limit mutation (Greg Sanders)
275579d8c133c066212a26423639956e2576e97a 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 ee589d4466bb0548a6f2215afe8abd0735768dab
glozow:
ACK ee589d4466bb0548a6f2215afe8abd0735768dab, nits can be ignored
ariard:
Code Review ACK ee589d446
Tree-SHA512: 14cf8edc73e014220def82563f5fb4192d1c2c111829712abf16340bfbfd9a85e2148d723af6fd4995d503dd67232b48dcf8b1711668d25b5aee5eab1bdb578c
Diffstat (limited to 'src/validation.cpp')
-rw-r--r-- | src/validation.cpp | 26 |
1 files changed, 14 insertions, 12 deletions
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. |