diff --git a/src/bench/block_assemble.cpp b/src/bench/block_assemble.cpp index 0092426315..6672fb2dac 100644 --- a/src/bench/block_assemble.cpp +++ b/src/bench/block_assemble.cpp @@ -35,10 +35,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 95f897d887..1af20cd117 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -53,6 +53,7 @@ enum class TxValidationResult { */ TX_CONFLICT, TX_MEMPOOL_POLICY, //!< violated mempool's fee/size/descendant/RBF/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 863622fbbb..540cb8e30f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -461,9 +461,9 @@ private: bool AlreadyHaveTx(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** - * 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 @@ -1413,6 +1413,7 @@ bool PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationStat case TxValidationResult::TX_WITNESS_STRIPPED: case TxValidationResult::TX_CONFLICT: case TxValidationResult::TX_MEMPOOL_POLICY: + case TxValidationResult::TX_NO_MEMPOOL: break; } if (message != "") { @@ -2293,7 +2294,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) { @@ -2350,8 +2351,6 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) break; } } - CChainState& active_chainstate = m_chainman.ActiveChainstate(); - m_mempool.check(active_chainstate.m_chain.Tip(), active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1); } bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer, @@ -3317,12 +3316,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 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) { - CChainState& active_chainstate = m_chainman.ActiveChainstate(); - m_mempool.check(active_chainstate.m_chain.Tip(), active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1); // As this version of the transaction was acceptable, we can forget about any // requests for it. m_txrequest.ForgetTxHash(tx.GetHash()); @@ -3441,8 +3438,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } // 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 2a7bcc057f..33b8e9351c 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -70,8 +70,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, false /* bypass_limits */, - true /* test_accept */); + const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*test_accept=*/ true); if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { return HandleATMPError(result.m_state, err_string); } else if (result.m_base_fees.value() > max_tx_fee) { @@ -79,8 +78,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, false /* bypass_limits */, - false /* test_accept */); + const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*test_accept=*/ false); if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { return HandleATMPError(result.m_state, err_string); } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index ce62fee0c8..864e9831d6 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1006,12 +1006,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]->GetWitnessHash(), - 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 8d25f5331d..c71ab01af4 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -37,8 +37,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 f2e2d7eaab..f7eb7d8718 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -37,8 +37,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 0d78038569..bd44b9c2be 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -340,7 +340,7 @@ CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactio // If submit=true, add transaction to the mempool. if (submit) { LOCK(cs_main); - const MempoolAcceptResult result = AcceptToMemoryPool(m_node.chainman->ActiveChainstate(), *m_node.mempool.get(), 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 ab448a732f..b7fc14537b 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -276,7 +276,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/txmempool.cpp b/src/txmempool.cpp index 2cb6cb2255..0905c5afc2 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -816,14 +816,12 @@ void CTxMemPool::check(const CBlockIndex* active_chain_tip, const CCoinsViewCach const CTransaction& tx2 = it2->GetTx(); assert(tx2.vout.size() > txin.prevout.n && !tx2.vout[txin.prevout.n].IsNull()); setParentCheck.insert(*it2); - } else { - // peg-in inputs are not sanity-checked to be valid - // assert(txin.m_is_pegin); FIXME } // We are iterating through the mempool entries sorted in order by ancestor count. // All parents must have been checked before their children and their coins added to // the mempoolDuplicate coins cache. - assert(mempoolDuplicate.HaveCoin(txin.prevout)); + // ELEMENTS: peg-in inputs are not sanity-checked to be valid + assert(mempoolDuplicate.HaveCoin(txin.prevout) || txin.m_is_pegin); // Check whether its inputs are marked in mapNextTx. auto it3 = mapNextTx.find(txin.prevout); assert(it3 != mapNextTx.end()); diff --git a/src/util/error.cpp b/src/util/error.cpp index 0aa8e7edc5..c947feb270 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 555848daef..8d1f2b6463 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1909,8 +1909,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 @@ -3922,6 +3920,19 @@ bool ChainstateManager::ProcessNewBlock(const CChainParams& chainparams, const s return true; } +MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef& tx, bool test_accept) +{ + 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=*/ false, test_accept); + active_chainstate.m_mempool->check(active_chainstate.m_chain.Tip(), active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1); + return result; +} + bool TestBlockValidity(BlockValidationState& state, const CChainParams& chainparams, CChainState& chainstate, diff --git a/src/validation.h b/src/validation.h index 1dc70931f4..0d8a066ac2 100644 --- a/src/validation.h +++ b/src/validation.h @@ -218,9 +218,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); @@ -997,6 +1004,15 @@ public: */ bool ProcessNewBlockHeaders(const std::vector& block, BlockValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex = nullptr, bool* all_duplicate = 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. + */ + [[nodiscard]] MempoolAcceptResult ProcessTransaction(const CTransactionRef& tx, bool test_accept=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);