diff --git a/src/bench/block_assemble.cpp b/src/bench/block_assemble.cpp --- a/src/bench/block_assemble.cpp +++ b/src/bench/block_assemble.cpp @@ -52,12 +52,10 @@ LOCK(::cs_main); for (const auto &txr : txs) { - TxValidationState vstate; - bool ret{::AcceptToMemoryPool( + const MempoolAcceptResult res = ::AcceptToMemoryPool( test_setup.m_node.chainman->ActiveChainstate(), config, - *test_setup.m_node.mempool, vstate, txr, - false /* bypass_limits */)}; - assert(ret); + *test_setup.m_node.mempool, txr, false /* bypass_limits */); + assert(res.m_result_type == MempoolAcceptResult::ResultType::VALID); } } diff --git a/src/net_processing.cpp b/src/net_processing.cpp --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3082,9 +3082,11 @@ continue; } - TxValidationState state; - if (AcceptToMemoryPool(m_chainman.ActiveChainstate(), config, m_mempool, - state, porphanTx, false /* bypass_limits */)) { + const MempoolAcceptResult result = + AcceptToMemoryPool(m_chainman.ActiveChainstate(), config, m_mempool, + porphanTx, false /* bypass_limits */); + const TxValidationState &state = result.m_state; + if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanTxId.ToString()); RelayTransaction(orphanTxId, m_connman); @@ -4322,10 +4324,11 @@ return; } - TxValidationState state; - - if (AcceptToMemoryPool(m_chainman.ActiveChainstate(), config, m_mempool, - state, ptx, false /* bypass_limits */)) { + const MempoolAcceptResult result = + AcceptToMemoryPool(m_chainman.ActiveChainstate(), config, m_mempool, + ptx, false /* bypass_limits */); + const TxValidationState &state = result.m_state; + if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { m_mempool.check(m_chainman.ActiveChainstate()); // As this version of the transaction was acceptable, we can forget // about any requests for it. diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -63,25 +63,27 @@ if (!node.mempool->exists(txid)) { // Transaction is not already in the mempool. - TxValidationState state; if (max_tx_fee > Amount::zero()) { // First, call ATMP with test_accept and check the fee. If ATMP // fails here, return error immediately. - Amount fee = Amount::zero(); - if (!AcceptToMemoryPool(node.chainman->ActiveChainstate(), - config, *node.mempool, state, tx, - false /* bypass_limits */, - /* test_accept */ true, &fee)) { - return HandleATMPError(state, err_string); - } else if (fee > max_tx_fee) { + const MempoolAcceptResult result = AcceptToMemoryPool( + node.chainman->ActiveChainstate(), config, *node.mempool, + tx, false /* bypass_limits */, + /* 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) { return TransactionError::MAX_FEE_EXCEEDED; } } // Try to submit the transaction to the mempool. - if (!AcceptToMemoryPool(node.chainman->ActiveChainstate(), config, - *node.mempool, state, tx, - false /* bypass_limits */)) { - return HandleATMPError(state, err_string); + const MempoolAcceptResult result = AcceptToMemoryPool( + node.chainman->ActiveChainstate(), config, *node.mempool, tx, + false /* bypass_limits */); + if (result.m_result_type != + MempoolAcceptResult::ResultType::VALID) { + return HandleATMPError(result.m_state, err_string); } // Transaction was accepted to the mempool. diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1156,20 +1156,19 @@ UniValue result_0(UniValue::VOBJ); result_0.pushKV("txid", txid.GetHex()); - TxValidationState state; - bool test_accept_res; - Amount fee = Amount::zero(); ChainstateManager &chainman = EnsureChainman(node); - test_accept_res = WITH_LOCK( - cs_main, return AcceptToMemoryPool( - chainman.ActiveChainstate(), config, mempool, - state, std::move(tx), false /* bypass_limits */, - true /* test_accept */, &fee)); + const MempoolAcceptResult accept_result = WITH_LOCK( + cs_main, + return AcceptToMemoryPool( + chainman.ActiveChainstate(), config, mempool, std::move(tx), + false /* bypass_limits */, true /* test_accept */)); // Only return the fee and size if the transaction would pass ATMP. // These can be used to calculate the feerate. - if (test_accept_res) { + if (accept_result.m_result_type == + MempoolAcceptResult::ResultType::VALID) { + const Amount fee = accept_result.m_base_fees.value(); // Check that fee does not exceed maximum fee if (max_raw_tx_fee != Amount::zero() && fee > max_raw_tx_fee) { result_0.pushKV("allowed", false); @@ -1184,6 +1183,7 @@ result.push_back(std::move(result_0)); } else { result_0.pushKV("allowed", false); + const TxValidationState state = accept_result.m_state; if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { result_0.pushKV("reject-reason", "missing-inputs"); diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -32,25 +32,23 @@ BOOST_CHECK(CTransaction(coinbaseTx).IsCoinBase()); - TxValidationState state; - LOCK(cs_main); unsigned int initialPoolSize = m_node.mempool->size(); + const MempoolAcceptResult result = AcceptToMemoryPool( + m_node.chainman->ActiveChainstate(), GetConfig(), *m_node.mempool, + MakeTransactionRef(coinbaseTx), true /* bypass_limits */); - BOOST_CHECK_EQUAL(false, - AcceptToMemoryPool(m_node.chainman->ActiveChainstate(), - GetConfig(), *m_node.mempool, state, - MakeTransactionRef(coinbaseTx), - true /* bypass_limits */)); + BOOST_CHECK(result.m_result_type == + MempoolAcceptResult::ResultType::INVALID); // Check that the transaction hasn't been added to mempool. BOOST_CHECK_EQUAL(m_node.mempool->size(), initialPoolSize); // Check that the validation state reflects the unsuccesful attempt. - BOOST_CHECK(state.IsInvalid()); - BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-tx-coinbase"); - BOOST_CHECK(state.GetResult() == TxValidationResult::TX_CONSENSUS); + BOOST_CHECK(result.m_state.IsInvalid()); + BOOST_CHECK_EQUAL(result.m_state.GetRejectReason(), "bad-tx-coinbase"); + BOOST_CHECK(result.m_state.GetResult() == TxValidationResult::TX_CONSENSUS); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -32,10 +32,10 @@ const auto ToMemPool = [this](const CMutableTransaction &tx) { LOCK(cs_main); - TxValidationState state; - return AcceptToMemoryPool( + const MempoolAcceptResult result = AcceptToMemoryPool( m_node.chainman->ActiveChainstate(), GetConfig(), *m_node.mempool, - state, MakeTransactionRef(tx), true /* bypass_limits */); + MakeTransactionRef(tx), true /* bypass_limits */); + return result.m_result_type == MempoolAcceptResult::ResultType::VALID; }; // Create a double-spend of mature coinbase txn: diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -329,13 +329,15 @@ // Add the txs to the tx pool { LOCK(cs_main); - TxValidationState state; for (const auto &tx : txs) { - BOOST_REQUIRE_MESSAGE( + const MempoolAcceptResult result = AcceptToMemoryPool(m_node.chainman->ActiveChainstate(), - config, *m_node.mempool, state, tx, - /* bypass_limits */ false), - state.GetRejectReason()); + config, *m_node.mempool, tx, + /* bypass_limits */ false); + BOOST_REQUIRE_MESSAGE( + result.m_result_type == + MempoolAcceptResult::ResultType::VALID, + result.m_state.GetRejectReason()); } } diff --git a/src/txmempool.cpp b/src/txmempool.cpp --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1413,10 +1413,10 @@ for (const CTransactionRef &tx : reverse_iterate(queuedTx.get())) { // ignore validation errors in resurrected transactions - TxValidationState stateDummy; if (!fAddToMempool || tx->IsCoinBase() || - !AcceptToMemoryPool(active_chainstate, config, pool, stateDummy, tx, - true /* bypass_limits */)) { + AcceptToMemoryPool(active_chainstate, config, pool, tx, + true /* bypass_limits */) + .m_result_type != MempoolAcceptResult::ResultType::VALID) { // If the transaction doesn't make it in to the mempool, remove any // transactions that depend on it (which would now be orphans). pool.removeRecursive(*tx, MemPoolRemovalReason::REORG); diff --git a/src/validation.h b/src/validation.h --- a/src/validation.h +++ b/src/validation.h @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -27,6 +28,7 @@ #include #include #include // For CTxMemPool::cs +#include #include #include @@ -55,7 +57,6 @@ class CTxMemPool; class CTxUndo; class DisconnectedBlockTransactions; -class TxValidationState; struct CCheckpointData; struct ChainTxData; @@ -274,14 +275,53 @@ void PruneBlockFilesManual(CChainState &active_chainstate, int nManualPruneHeight); +/** + * Validation result for a single transaction mempool acceptance. + */ +struct MempoolAcceptResult { + /** + * Used to indicate the results of mempool validation, + * including the possibility of unfinished validation. + */ + enum class ResultType { + //! Fully validated, valid. + VALID, + //! Invalid. + INVALID, + }; + ResultType m_result_type; + TxValidationState m_state; + + // The following fields are only present when m_result_type = + // ResultType::VALID + /** Raw base fees. */ + std::optional m_base_fees; + + /** Constructor for failure case */ + explicit MempoolAcceptResult(TxValidationState state) + : m_result_type(ResultType::INVALID), m_state(state), + m_base_fees(std::nullopt) { + // Can be invalid or error + Assume(!state.IsValid()); + } + + /** Constructor for success case */ + explicit MempoolAcceptResult(Amount fees) + : m_result_type(ResultType::VALID), m_state(TxValidationState{}), + m_base_fees(fees) {} +}; + /** * (try to) add transaction to memory pool - * @param[out] fee_out optional argument to return tx fee to the caller + * + * @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. */ -bool AcceptToMemoryPool(CChainState &active_chainstate, const Config &config, - CTxMemPool &pool, TxValidationState &state, - const CTransactionRef &tx, bool bypass_limits, - bool test_accept = false, Amount *fee_out = nullptr) +MempoolAcceptResult +AcceptToMemoryPool(CChainState &active_chainstate, const Config &config, + CTxMemPool &pool, const CTransactionRef &tx, + bool bypass_limits, bool test_accept = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** diff --git a/src/validation.cpp b/src/validation.cpp --- a/src/validation.cpp +++ b/src/validation.cpp @@ -359,11 +359,12 @@ */ std::vector &m_coins_to_uncache; const bool m_test_accept; - Amount *m_fee_out; + Amount m_fee_out; }; // Single transaction acceptance - bool AcceptSingleTransaction(const CTransactionRef &ptx, ATMPArgs &args) + MempoolAcceptResult AcceptSingleTransaction(const CTransactionRef &ptx, + ATMPArgs &args) EXCLUSIVE_LOCKS_REQUIRED(cs_main); private: @@ -551,10 +552,7 @@ return false; } - // If fee_out is passed, return the fee to the caller - if (args.m_fee_out) { - *args.m_fee_out = nFees; - } + args.m_fee_out = nFees; // Check for non-standard pay-to-script-hash in inputs if (fRequireStandard && @@ -697,8 +695,9 @@ return true; } -bool MemPoolAccept::AcceptSingleTransaction(const CTransactionRef &ptx, - ATMPArgs &args) { +MempoolAcceptResult +MemPoolAccept::AcceptSingleTransaction(const CTransactionRef &ptx, + ATMPArgs &args) { AssertLockHeld(cs_main); // mempool "read lock" (held through // GetMainSignals().TransactionAddedToMempool()) @@ -709,7 +708,7 @@ m_active_chainstate.m_chain.Tip())); if (!PreChecks(args, workspace)) { - return false; + return MempoolAcceptResult(args.m_state); } // Only compute the precomputed transaction data if we need to verify @@ -719,22 +718,22 @@ PrecomputedTransactionData txdata(*ptx); if (!ConsensusScriptChecks(args, workspace, txdata)) { - return false; + return MempoolAcceptResult(args.m_state); } // Tx was accepted, but not added if (args.m_test_accept) { - return true; + return MempoolAcceptResult(args.m_fee_out); } if (!Finalize(args, workspace)) { - return false; + return MempoolAcceptResult(args.m_state); } GetMainSignals().TransactionAddedToMempool( ptx, m_pool.GetAndIncrementSequence()); - return true; + return MempoolAcceptResult(args.m_fee_out); } } // namespace @@ -742,19 +741,19 @@ /** * (try to) add transaction to memory pool with a specified acceptance time. */ -static bool AcceptToMemoryPoolWithTime( +static MempoolAcceptResult AcceptToMemoryPoolWithTime( const Config &config, CTxMemPool &pool, CChainState &active_chainstate, - TxValidationState &state, const CTransactionRef &tx, int64_t nAcceptTime, - bool bypass_limits, bool test_accept, Amount *fee_out = nullptr) - EXCLUSIVE_LOCKS_REQUIRED(cs_main) { + const CTransactionRef &tx, int64_t nAcceptTime, bool bypass_limits, + bool test_accept) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { AssertLockHeld(cs_main); + TxValidationState state; std::vector coins_to_uncache; MemPoolAccept::ATMPArgs args{ config, state, nAcceptTime, bypass_limits, - coins_to_uncache, test_accept, fee_out}; - bool res = MemPoolAccept(pool, active_chainstate) - .AcceptSingleTransaction(tx, args); - if (!res) { + coins_to_uncache, test_accept, {}}; + const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate) + .AcceptSingleTransaction(tx, args); + if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { // Remove coins that were not present in the coins cache before calling // ATMPW; this is to prevent memory DoS in case we receive a large // number of invalid transactions that attempt to overrun the in-memory @@ -771,16 +770,15 @@ BlockValidationState stateDummy; active_chainstate.FlushStateToDisk(config.GetChainParams(), stateDummy, FlushStateMode::PERIODIC); - return res; + return result; } -bool AcceptToMemoryPool(CChainState &active_chainstate, const Config &config, - CTxMemPool &pool, TxValidationState &state, - const CTransactionRef &tx, bool bypass_limits, - bool test_accept, Amount *fee_out) { - return AcceptToMemoryPoolWithTime(config, pool, active_chainstate, state, - tx, GetTime(), bypass_limits, test_accept, - fee_out); +MempoolAcceptResult AcceptToMemoryPool(CChainState &active_chainstate, + const Config &config, CTxMemPool &pool, + const CTransactionRef &tx, + bool bypass_limits, bool test_accept) { + return AcceptToMemoryPoolWithTime(config, pool, active_chainstate, tx, + GetTime(), bypass_limits, test_accept); } CTransactionRef GetTransaction(const CBlockIndex *const block_index, @@ -5760,13 +5758,13 @@ if (amountdelta != Amount::zero()) { pool.PrioritiseTransaction(tx->GetId(), amountdelta); } - TxValidationState state; if (nTime > nNow - nExpiryTimeout) { LOCK(cs_main); - AcceptToMemoryPoolWithTime( - config, pool, active_chainstate, state, tx, nTime, - false /* bypass_limits */, false /* test_accept */); - if (state.IsValid()) { + if (AcceptToMemoryPoolWithTime( + config, pool, active_chainstate, tx, nTime, + false /* bypass_limits */, false /* test_accept */) + .m_result_type == + MempoolAcceptResult::ResultType::VALID) { ++count; } else { // mempool may contain the transaction already, e.g. from