diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 66e537a57ba..96095539ece 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -154,12 +154,15 @@ void CheckATMPInvariants(const MempoolAcceptResult& res, bool txid_in_mempool, b // It may be already in the mempool since in ATMP cases we don't set MEMPOOL_ENTRY or DIFFERENT_WITNESS Assert(!res.m_state.IsValid()); Assert(res.m_state.IsInvalid()); + + const bool is_reconsiderable{res.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE}; Assert(!res.m_replaced_transactions); Assert(!res.m_vsize); Assert(!res.m_base_fees); - // Unable or unwilling to calculate fees - Assert(!res.m_effective_feerate); - Assert(!res.m_wtxids_fee_calculations); + // Fee information is provided if the failure is TX_RECONSIDERABLE. + // In other cases, validation may be unable or unwilling to calculate the fees. + Assert(res.m_effective_feerate.has_value() == is_reconsiderable); + Assert(res.m_wtxids_fee_calculations.has_value() == is_reconsiderable); Assert(!res.m_other_wtxid); break; } diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 30e16ee365f..84c9ecc3d1e 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -753,15 +753,26 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK(m_node.mempool->GetMinFee().GetFee(GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)) > parent_fee + child_fee); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); - // Cheap package should fail with package-fee-too-low. + // Cheap package should fail for being too low fee. { const auto submit_package_too_low = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_still_too_low, /*test_accept=*/false); - BOOST_CHECK_EQUAL(submit_package_too_low.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); - BOOST_CHECK_EQUAL(submit_package_too_low.m_state.GetRejectReason(), "package-fee-too-low"); if (auto err_package_too_low{CheckPackageMempoolAcceptResult(package_still_too_low, submit_package_too_low, /*expect_valid=*/false, m_node.mempool.get())}) { BOOST_ERROR(err_package_too_low.value()); + } else { + // Individual feerate of parent is too low. + BOOST_CHECK_EQUAL(submit_package_too_low.m_tx_results.at(tx_parent_cheap->GetWitnessHash()).m_state.GetResult(), + TxValidationResult::TX_RECONSIDERABLE); + BOOST_CHECK(submit_package_too_low.m_tx_results.at(tx_parent_cheap->GetWitnessHash()).m_effective_feerate.value() == + CFeeRate(parent_fee, GetVirtualTransactionSize(*tx_parent_cheap))); + // Package feerate of parent + child is too low. + BOOST_CHECK_EQUAL(submit_package_too_low.m_tx_results.at(tx_child_cheap->GetWitnessHash()).m_state.GetResult(), + TxValidationResult::TX_RECONSIDERABLE); + BOOST_CHECK(submit_package_too_low.m_tx_results.at(tx_child_cheap->GetWitnessHash()).m_effective_feerate.value() == + CFeeRate(parent_fee + child_fee, GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap))); } + BOOST_CHECK_EQUAL(submit_package_too_low.m_state.GetResult(), PackageValidationResult::PCKG_TX); + BOOST_CHECK_EQUAL(submit_package_too_low.m_state.GetRejectReason(), "transaction failed"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); } diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index 147e589debf..6d3bb01be86 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -89,11 +89,14 @@ std::optional CheckPackageMempoolAcceptResult(const Package& txns, } // m_effective_feerate and m_wtxids_fee_calculations should exist iff the result was valid - if (atmp_result.m_effective_feerate.has_value() != valid) { + // or if the failure was TX_RECONSIDERABLE + const bool valid_or_reconsiderable{atmp_result.m_result_type == MempoolAcceptResult::ResultType::VALID || + atmp_result.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE}; + if (atmp_result.m_effective_feerate.has_value() != valid_or_reconsiderable) { return strprintf("tx %s result should %shave m_effective_feerate", wtxid.ToString(), valid ? "" : "not "); } - if (atmp_result.m_wtxids_fee_calculations.has_value() != valid) { + if (atmp_result.m_wtxids_fee_calculations.has_value() != valid_or_reconsiderable) { return strprintf("tx %s result should %shave m_effective_feerate", wtxid.ToString(), valid ? "" : "not "); } diff --git a/src/validation.cpp b/src/validation.cpp index c478252f182..018566fa9c3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1237,7 +1237,13 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef Workspace ws(ptx); const std::vector single_wtxid{ws.m_ptx->GetWitnessHash()}; - if (!PreChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); + if (!PreChecks(args, ws)) { + if (ws.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) { + // Failed for fee reasons. Provide the effective feerate and which tx was included. + return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), single_wtxid); + } + return MempoolAcceptResult::Failure(ws.m_state); + } if (m_rbf && !ReplacementChecks(ws)) return MempoolAcceptResult::Failure(ws.m_state); @@ -1254,7 +1260,12 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef ws.m_base_fees, effective_feerate, single_wtxid); } - if (!Finalize(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); + if (!Finalize(args, ws)) { + // The only possible failure reason is fee-related (mempool full). + // Failed for fee reasons. Provide the effective feerate and which txns were included. + Assume(ws.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE); + return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), {ws.m_ptx->GetWitnessHash()}); + } GetMainSignals().TransactionAddedToMempool(ptx, m_pool.GetAndIncrementSequence()); @@ -1308,11 +1319,16 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: const auto m_total_modified_fees = std::accumulate(workspaces.cbegin(), workspaces.cend(), CAmount{0}, [](CAmount sum, auto& ws) { return sum + ws.m_modified_fees; }); const CFeeRate package_feerate(m_total_modified_fees, m_total_vsize); + std::vector all_package_wtxids; + all_package_wtxids.reserve(workspaces.size()); + std::transform(workspaces.cbegin(), workspaces.cend(), std::back_inserter(all_package_wtxids), + [](const auto& ws) { return ws.m_ptx->GetWitnessHash(); }); TxValidationState placeholder_state; if (args.m_package_feerates && !CheckFeeRate(m_total_vsize, m_total_modified_fees, placeholder_state)) { - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-fee-too-low"); - return PackageMempoolAcceptResult(package_state, {}); + package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); + return PackageMempoolAcceptResult(package_state, {{workspaces.back().m_ptx->GetWitnessHash(), + MempoolAcceptResult::FeeFailure(placeholder_state, CFeeRate(m_total_modified_fees, m_total_vsize), all_package_wtxids)}}); } // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, @@ -1323,10 +1339,6 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: return PackageMempoolAcceptResult(package_state, std::move(results)); } - std::vector all_package_wtxids; - all_package_wtxids.reserve(workspaces.size()); - std::transform(workspaces.cbegin(), workspaces.cend(), std::back_inserter(all_package_wtxids), - [](const auto& ws) { return ws.m_ptx->GetWitnessHash(); }); for (Workspace& ws : workspaces) { ws.m_package_feerate = package_feerate; if (!PolicyScriptChecks(args, ws)) { diff --git a/src/validation.h b/src/validation.h index 75deba64934..e669ec46d5d 100644 --- a/src/validation.h +++ b/src/validation.h @@ -114,7 +114,27 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex* pin void PruneBlockFilesManual(Chainstate& active_chainstate, int nManualPruneHeight); /** -* Validation result for a single transaction mempool acceptance. +* Validation result for a transaction evaluated by MemPoolAccept (single or package). +* Here are the expected fields and properties of a result depending on its ResultType, applicable to +* results returned from package evaluation: +*+---------------------------+----------------+-------------------+------------------+----------------+-------------------+ +*| Field or property | VALID | INVALID | MEMPOOL_ENTRY | DIFFERENT_WITNESS | +*| | |--------------------------------------| | | +*| | | TX_RECONSIDERABLE | Other | | | +*+---------------------------+----------------+-------------------+------------------+----------------+-------------------+ +*| txid in mempool? | yes | no | no* | yes | yes | +*| wtxid in mempool? | yes | no | no* | yes | no | +*| m_state | yes, IsValid() | yes, IsInvalid() | yes, IsInvalid() | yes, IsValid() | yes, IsValid() | +*| m_replaced_transactions | yes | no | no | no | no | +*| m_vsize | yes | no | no | yes | no | +*| m_base_fees | yes | no | no | yes | no | +*| m_effective_feerate | yes | yes | no | no | no | +*| m_wtxids_fee_calculations | yes | yes | no | no | no | +*| m_other_wtxid | no | no | no | no | yes | +*+---------------------------+----------------+-------------------+------------------+----------------+-------------------+ +* (*) Individual transaction acceptance doesn't return MEMPOOL_ENTRY and DIFFERENT_WITNESS. It returns +* INVALID, with the errors txn-already-in-mempool and txn-same-nonwitness-data-in-mempool +* respectively. In those cases, the txid or wtxid may be in the mempool for a TX_CONFLICT. */ struct MempoolAcceptResult { /** Used to indicate the results of mempool validation. */ @@ -130,7 +150,6 @@ struct MempoolAcceptResult { /** Contains information about why the transaction failed. */ const TxValidationState m_state; - // The following fields are only present when m_result_type = ResultType::VALID or MEMPOOL_ENTRY /** Mempool transactions replaced by the tx. */ const std::optional> m_replaced_transactions; /** Virtual size as used by the mempool, calculated using serialized size and sigops. */ @@ -141,7 +160,6 @@ struct MempoolAcceptResult { * using prioritisetransaction (i.e. modified fees). If this transaction was submitted as a * package, this is the package feerate, which may also include its descendants and/or * ancestors (see m_wtxids_fee_calculations below). - * Only present when m_result_type = ResultType::VALID. */ const std::optional m_effective_feerate; /** Contains the wtxids of the transactions used for fee-related checks. Includes this @@ -151,7 +169,6 @@ struct MempoolAcceptResult { * Only present when m_result_type = ResultType::VALID. */ const std::optional> m_wtxids_fee_calculations; - // The following field is only present when m_result_type = ResultType::DIFFERENT_WITNESS /** The wtxid of the transaction in the mempool which has the same txid but different witness. */ const std::optional m_other_wtxid; @@ -159,6 +176,12 @@ struct MempoolAcceptResult { return MempoolAcceptResult(state); } + static MempoolAcceptResult FeeFailure(TxValidationState state, + CFeeRate effective_feerate, + const std::vector& wtxids_fee_calculations) { + return MempoolAcceptResult(state, effective_feerate, wtxids_fee_calculations); + } + static MempoolAcceptResult Success(std::list&& replaced_txns, int64_t vsize, CAmount fees, @@ -197,6 +220,15 @@ private: m_effective_feerate(effective_feerate), m_wtxids_fee_calculations(wtxids_fee_calculations) {} + /** Constructor for fee-related failure case */ + explicit MempoolAcceptResult(TxValidationState state, + CFeeRate effective_feerate, + const std::vector& wtxids_fee_calculations) + : m_result_type(ResultType::INVALID), + m_state(state), + m_effective_feerate(effective_feerate), + m_wtxids_fee_calculations(wtxids_fee_calculations) {} + /** Constructor for already-in-mempool case. It wouldn't replace any transactions. */ explicit MempoolAcceptResult(int64_t vsize, CAmount fees) : m_result_type(ResultType::MEMPOOL_ENTRY), m_vsize{vsize}, m_base_fees(fees) {}