From 497c9e29640858bb3beb20089c2d4f9e133c7e42 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 27 Sep 2021 16:40:25 +0100 Subject: [PATCH 1/7] [test] Don't set bypass_limits to true in txvalidationcache_tests.cpp AcceptToMemoryPool() is called for transactions with fees above minRelayTxFee and with the mempool not full, so setting bypass_limits to true or false has no impact on the test. The only way that changing bypass_limits from true to false could change the result would be to change the outcome to INVALID(TX_MEMPOOL_POLICY). Since all the ATMP calls in this test result in VALID both before and after this change, there is no change in behavior. --- src/test/txvalidationcache_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index afb3ad0cfd0..f799b8c9aab 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -37,7 +37,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, Dersig100Setup) LOCK(cs_main); const MempoolAcceptResult result = AcceptToMemoryPool(m_node.chainman->ActiveChainstate(), *m_node.mempool, MakeTransactionRef(tx), - true /* bypass_limits */); + false /* bypass_limits */); return result.m_result_type == MempoolAcceptResult::ResultType::VALID; }; From 5759fd12b8d5937e9187fa33489a95b1d8e6d1e5 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 27 Sep 2021 16:40:25 +0100 Subject: [PATCH 2/7] [test] Don't set bypass_limits to true in txvalidation_tests.cpp AcceptToMemoryPool() is called for an invalid coinbase transaction, so setting bypass_limits to true or false has no impact on the test. The only way that changing bypass_limits from true to false could change the result would be to change the outcome to INVALID(TX_MEMPOOL_POLICY). Since the ATMP call in this test results in INVALID(TX_CONSENSUS) both before and after this change, there is no change in behavior. --- src/test/txvalidation_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index ade9e210f22..a966c307204 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -38,7 +38,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup) unsigned int initialPoolSize = m_node.mempool->size(); const MempoolAcceptResult result = AcceptToMemoryPool(m_node.chainman->ActiveChainstate(), *m_node.mempool, MakeTransactionRef(coinbaseTx), - true /* bypass_limits */); + false /* bypass_limits */); BOOST_CHECK(result.m_result_type == MempoolAcceptResult::ResultType::INVALID); From 4c24142b1ec121623f81ba644d77341bc1bd88dd Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 19 Oct 2021 12:12:56 +0100 Subject: [PATCH 3/7] [validation] Remove comment about AcceptToMemoryPool() "This logic is not necessary for memory pool transactions, as AcceptToMemoryPool already refuses previously-known transaction ids entirely." refers to the logic at https://github.com/bitcoin/bitcoin/blob/a206b0ea12eb4606b93323268fc81a4f1f952531/src/main.cpp#L484-L486, which was later removed in commit 450cbb0944cd20a06ce806e6679a1f4c83c50db2. --- src/validation.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 207cdc8233c..f1c44d61ae1 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1658,8 +1658,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 From 36167faea92c97ddea7403280a5074073c8e5f90 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 3 Nov 2021 12:06:40 +0000 Subject: [PATCH 4/7] [logging/documentation] Remove reference to AcceptToMemoryPool from error string User-facing error messages should not leak internal implementation details like function names. Update the MEMPOOL_REJECTED error string from "Transaction rejected by AcceptToMemoryPool" to the more generic "Transaction rejected by mempool". Also update the MEMPOOL_ERROR error message from "AcceptToMemoryPool failed" to the more precise "Mempool internal error" since this error indicates and internal (e.g. logic/hardware/etc) failure, and not a transaction rejection. --- src/util/error.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/error.cpp b/src/util/error.cpp index 48c81693f36..d019ba018d0 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: From 92a3aeecf6a82e9cbc9fda11022b0548efd24d05 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 27 Sep 2021 16:55:42 +0100 Subject: [PATCH 5/7] [validation] Add CChainState::ProcessTransaction() This just calls through to AcceptToMemoryPool() internally, and is currently unused. Also add a new transaction validation failure reason TX_NO_MEMPOOL to indicate that there is no mempool. --- src/consensus/validation.h | 1 + src/net_processing.cpp | 1 + src/validation.cpp | 11 +++++++++++ src/validation.h | 9 +++++++++ 4 files changed, 22 insertions(+) diff --git a/src/consensus/validation.h b/src/consensus/validation.h index c4d305434a0..05416d0aca4 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 9f3aa5b4a3e..3ab716dc3b7 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1409,6 +1409,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 != "") { diff --git a/src/validation.cpp b/src/validation.cpp index f1c44d61ae1..1947f5d9c33 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3421,6 +3421,17 @@ 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); + } + return AcceptToMemoryPool(active_chainstate, *active_chainstate.m_mempool, tx, /*bypass_limits=*/ false, test_accept); +} + bool TestBlockValidity(BlockValidationState& state, const CChainParams& chainparams, CChainState& chainstate, diff --git a/src/validation.h b/src/validation.h index 4da8ec8d24b..096e609abe4 100644 --- a/src/validation.h +++ b/src/validation.h @@ -994,6 +994,15 @@ 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. + */ + [[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); From 2c64270bbe523ef87e7225c351464e7c716f0b3e Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 27 Sep 2021 17:47:21 +0100 Subject: [PATCH 6/7] [refactor] Don't call AcceptToMemoryPool() from outside validation.cpp --- src/bench/block_assemble.cpp | 4 ++-- src/net_processing.cpp | 14 +++++++------- src/node/transaction.cpp | 6 ++---- src/rpc/rawtransaction.cpp | 5 +++-- src/test/txvalidation_tests.cpp | 3 +-- src/test/txvalidationcache_tests.cpp | 3 +-- src/test/util/setup_common.cpp | 2 +- src/test/validation_block_tests.cpp | 2 +- src/validation.h | 13 ++++++++++--- 9 files changed, 28 insertions(+), 24 deletions(-) diff --git a/src/bench/block_assemble.cpp b/src/bench/block_assemble.cpp index b4b33d115f3..0577ab80e34 100644 --- a/src/bench/block_assemble.cpp +++ b/src/bench/block_assemble.cpp @@ -34,10 +34,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/net_processing.cpp b/src/net_processing.cpp index 3ab716dc3b7..7e35f1f9e69 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 @@ -2243,7 +2243,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) { @@ -3260,7 +3260,7 @@ 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) { @@ -3384,8 +3384,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 2a7bcc057fa..33b8e9351c2 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 483717aa7a6..dd9f78da0d5 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -946,12 +946,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 a966c307204..2e21c666ab0 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), - false /* 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 f799b8c9aab..8be64531c42 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -36,8 +36,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), - false /* 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 a3c7564d769..5a0c8e152af 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -315,7 +315,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 8f4ff6815bd..8a48d539f86 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -273,7 +273,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/validation.h b/src/validation.h index 096e609abe4..71bbcbf0d4b 100644 --- a/src/validation.h +++ b/src/validation.h @@ -206,9 +206,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); From 0fdb619aaf1d62598263361a6082d182be1af792 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 27 Sep 2021 18:11:08 +0100 Subject: [PATCH 7/7] [validation] Always call mempool.check() after processing a new transaction CTxMemPool::check() will carry out internal consistency checks 1/n times, where n is set by the `-checkmempool` configuration option. By default, mempool consistency checks are disabled entirely on mainnet. Therefore, this change has no effect on mainnet nodes running with default configuration. It simply removes the responsibility to trigger mempool consistency checks from net_processing. --- src/net_processing.cpp | 4 ---- src/validation.cpp | 4 +++- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 7e35f1f9e69..2185ccc7001 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2300,8 +2300,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& peer, @@ -3264,8 +3262,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 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.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()); diff --git a/src/validation.cpp b/src/validation.cpp index 1947f5d9c33..79ef6a6b19e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3429,7 +3429,9 @@ MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef& state.Invalid(TxValidationResult::TX_NO_MEMPOOL, "no-mempool"); return MempoolAcceptResult::Failure(state); } - return AcceptToMemoryPool(active_chainstate, *active_chainstate.m_mempool, tx, /*bypass_limits=*/ false, test_accept); + auto result = AcceptToMemoryPool(active_chainstate, *active_chainstate.m_mempool, tx, /*bypass_limits=*/ false, test_accept); + active_chainstate.m_mempool->check(active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1); + return result; } bool TestBlockValidity(BlockValidationState& state,