From c8571c09568a72640e98de05704ccae1ee8b6844 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 3 Oct 2024 17:29:22 +0000 Subject: [PATCH] merge bitcoin#23173: Add `ChainstateManager::ProcessTransaction` We need to extend `bypass_limits` to `ProcessTransaction()` because Dash allows the `sendrawtransaction` RPC to bypass limits with the optional `bypasslimits` boolean (introduced in dash#2110). The bool arguments are not in alphabetical order to prevent breakage with Bitcoin code that expects `bypass_limits` to always be false. --- src/bench/block_assemble.cpp | 4 ++-- src/consensus/validation.h | 1 + src/net_processing.cpp | 19 ++++++++----------- src/node/transaction.cpp | 6 ++---- src/rpc/rawtransaction.cpp | 5 +++-- src/test/txvalidation_tests.cpp | 2 +- src/test/txvalidationcache_tests.cpp | 2 +- src/test/util/setup_common.cpp | 2 +- src/test/validation_block_tests.cpp | 2 +- src/util/error.cpp | 4 ++-- src/validation.cpp | 15 +++++++++++++-- src/validation.h | 23 ++++++++++++++++++++--- 12 files changed, 55 insertions(+), 30 deletions(-) diff --git a/src/bench/block_assemble.cpp b/src/bench/block_assemble.cpp index a23b9e8ac6..56afa0132f 100644 --- a/src/bench/block_assemble.cpp +++ b/src/bench/block_assemble.cpp @@ -39,10 +39,10 @@ static void AssembleBlock(benchmark::Bench& bench) txs.at(b) = MakeTransactionRef(tx); } { - LOCK(::cs_main); // Required for ::AcceptToMemoryPool. + LOCK(::cs_main); for (const auto& txr : txs) { - const MempoolAcceptResult res = ::AcceptToMemoryPool(test_setup->m_node.chainman->ActiveChainstate(), *test_setup->m_node.mempool, txr, false /* bypass_limits */); + const MempoolAcceptResult res = test_setup->m_node.chainman->ProcessTransaction(txr); assert(res.m_result_type == MempoolAcceptResult::ResultType::VALID); } } diff --git a/src/consensus/validation.h b/src/consensus/validation.h index b06f3c8c83..d4b0c92886 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -31,6 +31,7 @@ enum class TxValidationResult { TX_CONFLICT, TX_CONFLICT_LOCK, //!< conflicts with InstantSend lock TX_MEMPOOL_POLICY, //!< violated mempool's fee/size/descendant/etc limits + TX_NO_MEMPOOL, //!< this node does not have a mempool so can't validate the transaction }; /** A "reason" why a block was invalid, suitable for determining whether the diff --git a/src/net_processing.cpp b/src/net_processing.cpp index d14d4d2a0c..9536b8fd07 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -856,9 +856,9 @@ private: EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_recent_confirmed_transactions_mutex); /** - * Filter for transactions that were recently rejected by - * AcceptToMemoryPool. These are not rerequested until the chain tip - * changes, at which point the entire filter is reset. + * Filter for transactions that were recently rejected by the mempool. + * These are not rerequested until the chain tip changes, at which point + * the entire filter is reset. * * Without this filter we'd be re-requesting txs from each of our peers, * increasing bandwidth consumption considerably. For instance, with 100 @@ -1816,6 +1816,7 @@ bool PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationStat case TxValidationResult::TX_PREMATURE_SPEND: case TxValidationResult::TX_CONFLICT: case TxValidationResult::TX_MEMPOOL_POLICY: + case TxValidationResult::TX_NO_MEMPOOL: // moved from BLOCK case TxValidationResult::TX_BAD_SPECIAL: case TxValidationResult::TX_CONFLICT_LOCK: @@ -3039,7 +3040,7 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) const auto [porphanTx, from_peer] = m_orphanage.GetTx(orphanHash); if (porphanTx == nullptr) continue; - const MempoolAcceptResult result = AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, porphanTx, false /* bypass_limits */); + const MempoolAcceptResult result = m_chainman.ProcessTransaction(porphanTx); const TxValidationState& state = result.m_state; if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { @@ -3065,8 +3066,6 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) break; } } - CChainState& active_chainstate = m_chainman.ActiveChainstate(); - m_mempool.check(active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1); } bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& node, Peer& peer, @@ -4224,7 +4223,7 @@ void PeerManagerImpl::ProcessMessage( return; } - const MempoolAcceptResult result = AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, ptx, false /* bypass_limits */); + const MempoolAcceptResult result = m_chainman.ProcessTransaction(ptx); const TxValidationState& state = result.m_state; if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { @@ -4235,8 +4234,6 @@ void PeerManagerImpl::ProcessMessage( m_cj_ctx->dstxman->AddDSTX(dstx); } - CChainState& active_chainstate = m_chainman.ActiveChainstate(); - m_mempool.check(active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1); RelayTransaction(tx.GetHash()); m_orphanage.AddChildrenToWorkSet(tx, peer->m_orphan_work_set); @@ -4308,8 +4305,8 @@ void PeerManagerImpl::ProcessMessage( } // If a tx has been detected by m_recent_rejects, we will have reached - // this point and the tx will have been ignored. Because we haven't run - // the tx through AcceptToMemoryPool, we won't have computed a DoS + // this point and the tx will have been ignored. Because we haven't + // submitted the tx to our mempool, we won't have computed a DoS // score for it or determined exactly why we consider it invalid. // // This means we won't penalize any peer subsequently relaying a DoSy diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index b36af2c1b5..b7d3e8c251 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -65,8 +65,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t if (max_tx_fee > 0) { // First, call ATMP with test_accept and check the fee. If ATMP // fails here, return error immediately. - const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx, - bypass_limits, true /* test_accept */); + const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*test_accept=*/true, bypass_limits); if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { return HandleATMPError(result.m_state, err_string.original); } else if (result.m_base_fees.value() > max_tx_fee) { @@ -74,8 +73,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t } } // Try to submit the transaction to the mempool. - const MempoolAcceptResult result = AcceptToMemoryPool(node.chainman->ActiveChainstate(), *node.mempool, tx, - bypass_limits, false /* test_accept */); + const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*test_accept=*/false, bypass_limits); if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { return HandleATMPError(result.m_state, err_string.original); } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index fca81b4c7e..f800db45f4 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1246,12 +1246,13 @@ static RPCHelpMan testmempoolaccept() NodeContext& node = EnsureAnyNodeContext(request.context); CTxMemPool& mempool = EnsureMemPool(node); - CChainState& chainstate = EnsureChainman(node).ActiveChainstate(); + ChainstateManager& chainman = EnsureChainman(node); + CChainState& chainstate = chainman.ActiveChainstate(); const PackageMempoolAcceptResult package_result = [&] { LOCK(::cs_main); if (txns.size() > 1) return ProcessNewPackage(chainstate, mempool, txns, /* test_accept */ true); return PackageMempoolAcceptResult(txns[0]->GetHash(), - AcceptToMemoryPool(chainstate, mempool, txns[0], /* bypass_limits */ false, /* test_accept*/ true)); + chainman.ProcessTransaction(txns[0], /*test_accept=*/ true)); }(); UniValue rpc_result(UniValue::VARR); diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index 65afd6a796..e94fd34b39 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -43,7 +43,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup) LOCK(cs_main); unsigned int initialPoolSize = m_node.mempool->size(); - const MempoolAcceptResult result = AcceptToMemoryPool(m_node.chainman->ActiveChainstate(), *m_node.mempool, MakeTransactionRef(coinbaseTx), true /* bypass_limits */); + const MempoolAcceptResult result = m_node.chainman->ProcessTransaction(MakeTransactionRef(coinbaseTx)); BOOST_CHECK(result.m_result_type == MempoolAcceptResult::ResultType::INVALID); diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index f8db20c73e..100220055a 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -32,7 +32,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, Dersig100Setup) const auto ToMemPool = [this](const CMutableTransaction& tx) { LOCK(cs_main); - const MempoolAcceptResult result = AcceptToMemoryPool(m_node.chainman->ActiveChainstate(), *m_node.mempool, MakeTransactionRef(tx), true /* bypass_limits */); + const MempoolAcceptResult result = m_node.chainman->ProcessTransaction(MakeTransactionRef(tx)); return result.m_result_type == MempoolAcceptResult::ResultType::VALID; }; diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index b8fba9dd06..30b1f4adbe 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -519,7 +519,7 @@ CMutableTransaction TestChainSetup::CreateValidMempoolTransaction(CTransactionRe // If submit=true, add transaction to the mempool. if (submit) { LOCK(cs_main); - const MempoolAcceptResult result = AcceptToMemoryPool(m_node.chainman->ActiveChainstate(), *m_node.mempool, MakeTransactionRef(mempool_txn), /* bypass_limits */ false); + const MempoolAcceptResult result = m_node.chainman->ProcessTransaction(MakeTransactionRef(mempool_txn)); assert(result.m_result_type == MempoolAcceptResult::ResultType::VALID); } diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index e532da6289..b58050d15d 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -279,7 +279,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg) { LOCK(cs_main); for (const auto& tx : txs) { - const MempoolAcceptResult result = AcceptToMemoryPool(m_node.chainman->ActiveChainstate(), *m_node.mempool, tx, false /* bypass_limits */); + const MempoolAcceptResult result = m_node.chainman->ProcessTransaction(tx); BOOST_REQUIRE(result.m_result_type == MempoolAcceptResult::ResultType::VALID); } } diff --git a/src/util/error.cpp b/src/util/error.cpp index 76fac4d391..43c4a3efd0 100644 --- a/src/util/error.cpp +++ b/src/util/error.cpp @@ -20,9 +20,9 @@ bilingual_str TransactionErrorString(const TransactionError err) case TransactionError::P2P_DISABLED: return Untranslated("Peer-to-peer functionality missing or disabled"); case TransactionError::MEMPOOL_REJECTED: - return Untranslated("Transaction rejected by AcceptToMemoryPool"); + return Untranslated("Transaction rejected by mempool"); case TransactionError::MEMPOOL_ERROR: - return Untranslated("AcceptToMemoryPool failed"); + return Untranslated("Mempool internal error"); case TransactionError::INVALID_PSBT: return Untranslated("PSBT is not well-formed"); case TransactionError::PSBT_MISMATCH: diff --git a/src/validation.cpp b/src/validation.cpp index 51c5d28fc8..5978c60954 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1942,8 +1942,6 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, // can be duplicated to remove the ability to spend the first instance -- even after // being sent to another address. // See BIP30, CVE-2012-1909, and http://r6.ca/blog/20120206T005236Z.html for more information. - // This logic is not necessary for memory pool transactions, as AcceptToMemoryPool - // already refuses previously-known transaction ids entirely. // This rule was originally applied to all blocks with a timestamp after March 15, 2012, 0:00 UTC. // Now that the whole chain is irreversibly beyond that time it is applied to all blocks except the // two in the chain that violate it. This prevents exploiting the issue against nodes during their @@ -3980,6 +3978,19 @@ bool ChainstateManager::ProcessNewBlock(const CChainParams& chainparams, const s return true; } +MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef& tx, bool test_accept, bool bypass_limits) +{ + CChainState& active_chainstate = ActiveChainstate(); + if (!active_chainstate.m_mempool) { + TxValidationState state; + state.Invalid(TxValidationResult::TX_NO_MEMPOOL, "no-mempool"); + return MempoolAcceptResult::Failure(state); + } + auto result = AcceptToMemoryPool(active_chainstate, *active_chainstate.m_mempool, tx, bypass_limits, test_accept); + active_chainstate.m_mempool->check(active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1); + return result; +} + bool TestBlockValidity(BlockValidationState& state, llmq::CChainLocksHandler& clhandler, CEvoDB& evoDb, diff --git a/src/validation.h b/src/validation.h index 9b27f340da..dc6c4f648d 100644 --- a/src/validation.h +++ b/src/validation.h @@ -240,9 +240,16 @@ struct PackageMempoolAcceptResult }; /** - * (Try to) add a transaction to the memory pool. - * @param[in] bypass_limits When true, don't enforce mempool fee limits. - * @param[in] test_accept When true, run validation checks but don't submit to mempool. + * Try to add a transaction to the mempool. This is an internal function and is + * exposed only for testing. Client code should use ChainstateManager::ProcessTransaction() + * + * @param[in] active_chainstate Reference to the active chainstate. + * @param[in] pool Reference to the node's mempool. + * @param[in] tx The transaction to submit for mempool acceptance. + * @param[in] bypass_limits When true, don't enforce mempool fee and capacity limits. + * @param[in] test_accept When true, run validation checks but don't submit to mempool. + * + * @returns a MempoolAcceptResult indicating whether the transaction was accepted/rejected with reason. */ MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx, bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -1021,6 +1028,16 @@ public: */ bool ProcessNewBlockHeaders(const std::vector& block, BlockValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex = nullptr) LOCKS_EXCLUDED(cs_main); + /** + * Try to add a transaction to the memory pool. + * + * @param[in] tx The transaction to submit for mempool acceptance. + * @param[in] test_accept When true, run validation checks but don't submit to mempool. + * @param[in] bypass_limits When true, don't enforce mempool fee and capacity limits. + */ + [[nodiscard]] MempoolAcceptResult ProcessTransaction(const CTransactionRef& tx, bool test_accept=false, bool bypass_limits=false) + EXCLUSIVE_LOCKS_REQUIRED(cs_main); + //! Load the block tree and coins database from disk, initializing state if we're running with -reindex bool LoadBlockIndex() EXCLUSIVE_LOCKS_REQUIRED(cs_main);