From be2e4d94e59510e0a98408313feb27e97321b16e Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 3 Nov 2022 15:01:17 +0000 Subject: [PATCH 01/10] [validation] when quitting early in AcceptPackage, set package_state and tx result Bug: not setting package_state means package_state.IsValid() == true and the caller does not know that this failed. We won't be validating this transaction again, so it makes sense to return this failure to the caller. Rename package_state to package_state_quit_early to make it more clear what this variable is used for and what its scope is. Co-authored-by: Greg Sanders --- src/validation.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index e24d39170e..9845e8ddc9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1273,19 +1273,20 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, ATMPArgs& args) { AssertLockHeld(cs_main); - PackageValidationState package_state; + // Used if returning a PackageMempoolAcceptResult directly from this function. + PackageValidationState package_state_quit_early; // Check that the package is well-formed. If it isn't, we won't try to validate any of the // transactions and thus won't return any MempoolAcceptResults, just a package-wide error. // Context-free package checks. - if (!CheckPackage(package, package_state)) return PackageMempoolAcceptResult(package_state, {}); + if (!CheckPackage(package, package_state_quit_early)) return PackageMempoolAcceptResult(package_state_quit_early, {}); // All transactions in the package must be a parent of the last transaction. This is just an // opportunity for us to fail fast on a context-free check without taking the mempool lock. if (!IsChildWithParents(package)) { - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents"); - return PackageMempoolAcceptResult(package_state, {}); + package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents"); + return PackageMempoolAcceptResult(package_state_quit_early, {}); } // IsChildWithParents() guarantees the package is > 1 transactions. @@ -1317,8 +1318,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, return unconfirmed_parent_txids.count(input.prevout.hash) > 0 || m_view.HaveCoin(input.prevout); }; if (!std::all_of(child->vin.cbegin(), child->vin.cend(), package_or_confirmed)) { - package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-unconfirmed-parents"); - return PackageMempoolAcceptResult(package_state, {}); + package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-unconfirmed-parents"); + return PackageMempoolAcceptResult(package_state_quit_early, {}); } // Protect against bugs where we pull more inputs from disk that miss being added to // coins_to_uncache. The backend will be connected again when needed in PreChecks. @@ -1381,6 +1382,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // future. Continue individually validating the rest of the transactions, because // some of them may still be valid. quit_early = true; + package_state_quit_early.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); + results.emplace(wtxid, single_res); } else { txns_new.push_back(tx); } @@ -1390,7 +1393,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // Nothing to do if the entire package has already been submitted. if (quit_early || txns_new.empty()) { // No package feerate when no package validation was done. - return PackageMempoolAcceptResult(package_state, std::move(results)); + return PackageMempoolAcceptResult(package_state_quit_early, std::move(results)); } // Validate the (deduplicated) transactions as a package. auto submission_result = AcceptMultipleTransactions(txns_new, args); From 5d35b4a7dee95ae70bfdcbe79bc39fe488641b23 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 5 Dec 2022 14:32:35 +0000 Subject: [PATCH 02/10] [test] package validation quits early due to non-policy, non-missing-inputs failure --- src/test/txpackage_tests.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index b290a664f9..e77fcecf37 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -278,6 +278,24 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK(result_3gen_submit.m_package_feerate == std::nullopt); } + // Parent and child package where transactions are invalid for reasons other than fee and + // missing inputs, so the package validation isn't expected to happen. + { + CScriptWitness bad_witness; + bad_witness.stack.push_back(std::vector(1)); + CMutableTransaction mtx_parent_invalid{mtx_parent}; + mtx_parent_invalid.vin[0].scriptWitness = bad_witness; + CTransactionRef tx_parent_invalid = MakeTransactionRef(mtx_parent_invalid); + auto result_quit_early = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, + {tx_parent_invalid, tx_child}, /*test_accept=*/ false); + BOOST_CHECK(result_quit_early.m_state.IsInvalid()); + BOOST_CHECK_EQUAL(result_quit_early.m_state.GetResult(), PackageValidationResult::PCKG_TX); + BOOST_CHECK(!result_quit_early.m_tx_results.empty()); + auto it_parent = result_quit_early.m_tx_results.find(tx_parent_invalid->GetWitnessHash()); + BOOST_CHECK(it_parent != result_quit_early.m_tx_results.end()); + BOOST_CHECK_EQUAL(it_parent->second.m_state.GetResult(), TxValidationResult::TX_WITNESS_MUTATED); + } + // Child with missing parent. mtx_child.vin.push_back(CTxIn(COutPoint(package_unrelated[0]->GetHash(), 0))); Package package_missing_parent; From 1605886380e4d3ff2e1236739fb800fa07322c49 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 28 Sep 2022 11:23:55 +0100 Subject: [PATCH 03/10] [validation] return effective feerate from mempool validation --- src/test/txpackage_tests.cpp | 19 +++++++++++++++++++ src/validation.cpp | 19 +++++++++++++++---- src/validation.h | 22 ++++++++++++++++++---- 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index e77fcecf37..244afb9161 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -95,12 +95,14 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) BOOST_CHECK(it_parent != result_parent_child.m_tx_results.end()); BOOST_CHECK_MESSAGE(it_parent->second.m_state.IsValid(), "Package validation unexpectedly failed: " << it_parent->second.m_state.GetRejectReason()); + BOOST_CHECK(it_parent->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_parent)) == COIN); BOOST_CHECK(it_child != result_parent_child.m_tx_results.end()); BOOST_CHECK_MESSAGE(it_child->second.m_state.IsValid(), "Package validation unexpectedly failed: " << it_child->second.m_state.GetRejectReason()); BOOST_CHECK(result_parent_child.m_package_feerate.has_value()); BOOST_CHECK(result_parent_child.m_package_feerate.value() == CFeeRate(2 * COIN, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child))); + BOOST_CHECK(it_child->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_child)) == COIN); // A single, giant transaction submitted through ProcessNewPackage fails on single tx policy. CTransactionRef giant_ptx = create_placeholder_tx(999, 999); @@ -323,8 +325,10 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) auto it_child = submit_parent_child.m_tx_results.find(tx_child->GetWitnessHash()); BOOST_CHECK(it_parent != submit_parent_child.m_tx_results.end()); BOOST_CHECK(it_parent->second.m_state.IsValid()); + BOOST_CHECK(it_parent->second.m_effective_feerate == CFeeRate(1 * COIN, GetVirtualTransactionSize(*tx_parent))); BOOST_CHECK(it_child != submit_parent_child.m_tx_results.end()); BOOST_CHECK(it_child->second.m_state.IsValid()); + BOOST_CHECK(it_child->second.m_effective_feerate == CFeeRate(1 * COIN, GetVirtualTransactionSize(*tx_child))); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash()))); @@ -613,6 +617,8 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK_MESSAGE(mixed_result.m_package_feerate.value() == expected_feerate, strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), mixed_result.m_package_feerate.value().ToString())); + BOOST_CHECK(it_parent3->second.m_effective_feerate.value() == expected_feerate); + BOOST_CHECK(it_child->second.m_effective_feerate.value() == expected_feerate); } } @@ -695,6 +701,8 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) const CFeeRate expected_feerate(coinbase_value - child_value, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child)); + BOOST_CHECK(it_parent->second.m_effective_feerate.value() == expected_feerate); + BOOST_CHECK(it_child->second.m_effective_feerate.value() == expected_feerate); BOOST_CHECK(expected_feerate.GetFeePerK() > 1000); BOOST_CHECK(submit_cpfp.m_package_feerate.has_value()); BOOST_CHECK_MESSAGE(submit_cpfp.m_package_feerate.value() == expected_feerate, @@ -756,6 +764,16 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK_MESSAGE(submit_prioritised_package.m_package_feerate.value() == expected_feerate, strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), submit_prioritised_package.m_package_feerate.value().ToString())); + auto it_parent = submit_prioritised_package.m_tx_results.find(tx_parent_cheap->GetWitnessHash()); + auto it_child = submit_prioritised_package.m_tx_results.find(tx_child_cheap->GetWitnessHash()); + BOOST_CHECK(it_parent != submit_prioritised_package.m_tx_results.end()); + BOOST_CHECK(it_parent->second.m_result_type == MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK(it_parent->second.m_base_fees.value() == 0); + BOOST_CHECK(it_parent->second.m_effective_feerate.value() == expected_feerate); + BOOST_CHECK(it_child != submit_prioritised_package.m_tx_results.end()); + BOOST_CHECK(it_child->second.m_result_type == MempoolAcceptResult::ResultType::VALID); + BOOST_CHECK(it_child->second.m_base_fees.value() == 200); + BOOST_CHECK(it_child->second.m_effective_feerate.value() == expected_feerate); } // Package feerate is calculated without topology in mind; it's just aggregating fees and sizes. @@ -801,6 +819,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK(it_parent->second.m_state.GetRejectReason() == ""); BOOST_CHECK_MESSAGE(it_parent->second.m_base_fees.value() == high_parent_fee, strprintf("rich parent: expected fee %s, got %s", high_parent_fee, it_parent->second.m_base_fees.value())); + BOOST_CHECK(it_parent->second.m_effective_feerate == CFeeRate(high_parent_fee, GetVirtualTransactionSize(*tx_parent_rich))); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_rich->GetHash()))); diff --git a/src/validation.cpp b/src/validation.cpp index 9845e8ddc9..8ae4a3ecb8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -583,6 +583,11 @@ private: /** Total virtual size of all transactions being replaced. */ size_t m_conflicting_size{0}; + /** If we're doing package validation (i.e. m_package_feerates=true), the "effective" + * package feerate of this transaction is the total fees divided by the total size of + * transactions (which may include its ancestors and/or descendants). */ + CFeeRate m_package_feerate{0}; + const CTransactionRef& m_ptx; /** Txid. */ const uint256& m_hash; @@ -1147,9 +1152,11 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& // Find the wtxids of the transactions that made it into the mempool. Allow partial submission, // but don't report success unless they all made it into the mempool. for (Workspace& ws : workspaces) { + const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : + CFeeRate{ws.m_modified_fees, static_cast(ws.m_vsize)}; if (m_pool.exists(GenTxid::Wtxid(ws.m_ptx->GetWitnessHash()))) { results.emplace(ws.m_ptx->GetWitnessHash(), - MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees)); + MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate)); GetMainSignals().TransactionAddedToMempool(ws.m_ptx, m_pool.GetAndIncrementSequence()); } else { all_submitted = false; @@ -1177,16 +1184,17 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef if (!ConsensusScriptChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); + const CFeeRate effective_feerate{ws.m_modified_fees, static_cast(ws.m_vsize)}; // Tx was accepted, but not added if (args.m_test_accept) { - return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees); + return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate); } if (!Finalize(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); GetMainSignals().TransactionAddedToMempool(ptx, m_pool.GetAndIncrementSequence()); - return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees); + return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate); } PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::vector& txns, ATMPArgs& args) @@ -1245,6 +1253,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: } for (Workspace& ws : workspaces) { + ws.m_package_feerate = package_feerate; if (!PolicyScriptChecks(args, ws)) { // Exit early to avoid doing pointless work. Update the failed tx result; the rest are unfinished. package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); @@ -1252,11 +1261,13 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); } if (args.m_test_accept) { + const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : + CFeeRate{ws.m_modified_fees, static_cast(ws.m_vsize)}; // When test_accept=true, transactions that pass PolicyScriptChecks are valid because there are // no further mempool checks (passing PolicyScriptChecks implies passing ConsensusScriptChecks). results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), - ws.m_vsize, ws.m_base_fees)); + ws.m_vsize, ws.m_base_fees, effective_feerate)); } } diff --git a/src/validation.h b/src/validation.h index 63ff9247be..8701326277 100644 --- a/src/validation.h +++ b/src/validation.h @@ -134,6 +134,11 @@ struct MempoolAcceptResult { const std::optional m_vsize; /** Raw base fees in satoshis. */ const std::optional m_base_fees; + /** The feerate at which this transaction was considered. This includes any fee delta added + * 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. Only present when m_result_type = ResultType::VALID. */ + const std::optional m_effective_feerate; // 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. */ @@ -143,8 +148,11 @@ struct MempoolAcceptResult { return MempoolAcceptResult(state); } - static MempoolAcceptResult Success(std::list&& replaced_txns, int64_t vsize, CAmount fees) { - return MempoolAcceptResult(std::move(replaced_txns), vsize, fees); + static MempoolAcceptResult Success(std::list&& replaced_txns, + int64_t vsize, + CAmount fees, + CFeeRate effective_feerate) { + return MempoolAcceptResult(std::move(replaced_txns), vsize, fees, effective_feerate); } static MempoolAcceptResult MempoolTx(int64_t vsize, CAmount fees) { @@ -164,9 +172,15 @@ private: } /** Constructor for success case */ - explicit MempoolAcceptResult(std::list&& replaced_txns, int64_t vsize, CAmount fees) + explicit MempoolAcceptResult(std::list&& replaced_txns, + int64_t vsize, + CAmount fees, + CFeeRate effective_feerate) : m_result_type(ResultType::VALID), - m_replaced_transactions(std::move(replaced_txns)), m_vsize{vsize}, m_base_fees(fees) {} + m_replaced_transactions(std::move(replaced_txns)), + m_vsize{vsize}, + m_base_fees(fees), + m_effective_feerate(effective_feerate) {} /** Constructor for already-in-mempool case. It wouldn't replace any transactions. */ explicit MempoolAcceptResult(int64_t vsize, CAmount fees) From d6c7b78ef2924af72f677ce2a7472c2447141e18 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 9 Dec 2022 13:13:49 +0000 Subject: [PATCH 04/10] [validation] return wtxids of other transactions whose fees were used --- src/test/txpackage_tests.cpp | 17 +++++++++++++++++ src/validation.cpp | 27 +++++++++++++++++++++------ src/validation.h | 22 +++++++++++++++++----- 3 files changed, 55 insertions(+), 11 deletions(-) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 244afb9161..0be843d5d9 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -96,6 +96,8 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) BOOST_CHECK_MESSAGE(it_parent->second.m_state.IsValid(), "Package validation unexpectedly failed: " << it_parent->second.m_state.GetRejectReason()); BOOST_CHECK(it_parent->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_parent)) == COIN); + BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().size(), 1); + BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().front(), tx_parent->GetWitnessHash()); BOOST_CHECK(it_child != result_parent_child.m_tx_results.end()); BOOST_CHECK_MESSAGE(it_child->second.m_state.IsValid(), "Package validation unexpectedly failed: " << it_child->second.m_state.GetRejectReason()); @@ -103,6 +105,8 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) BOOST_CHECK(result_parent_child.m_package_feerate.value() == CFeeRate(2 * COIN, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child))); BOOST_CHECK(it_child->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_child)) == COIN); + BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().size(), 1); + BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().front(), tx_child->GetWitnessHash()); // A single, giant transaction submitted through ProcessNewPackage fails on single tx policy. CTransactionRef giant_ptx = create_placeholder_tx(999, 999); @@ -326,9 +330,13 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK(it_parent != submit_parent_child.m_tx_results.end()); BOOST_CHECK(it_parent->second.m_state.IsValid()); BOOST_CHECK(it_parent->second.m_effective_feerate == CFeeRate(1 * COIN, GetVirtualTransactionSize(*tx_parent))); + BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().size(), 1); + BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().front(), tx_parent->GetWitnessHash()); BOOST_CHECK(it_child != submit_parent_child.m_tx_results.end()); BOOST_CHECK(it_child->second.m_state.IsValid()); BOOST_CHECK(it_child->second.m_effective_feerate == CFeeRate(1 * COIN, GetVirtualTransactionSize(*tx_child))); + BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().size(), 1); + BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().front(), tx_child->GetWitnessHash()); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash()))); @@ -619,6 +627,9 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) mixed_result.m_package_feerate.value().ToString())); BOOST_CHECK(it_parent3->second.m_effective_feerate.value() == expected_feerate); BOOST_CHECK(it_child->second.m_effective_feerate.value() == expected_feerate); + std::vector expected_wtxids({ptx_parent3->GetWitnessHash(), ptx_mixed_child->GetWitnessHash()}); + BOOST_CHECK(it_parent3->second.m_wtxids_fee_calculations.value() == expected_wtxids); + BOOST_CHECK(it_child->second.m_wtxids_fee_calculations.value() == expected_wtxids); } } @@ -703,6 +714,9 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child)); BOOST_CHECK(it_parent->second.m_effective_feerate.value() == expected_feerate); BOOST_CHECK(it_child->second.m_effective_feerate.value() == expected_feerate); + std::vector expected_wtxids({tx_parent->GetWitnessHash(), tx_child->GetWitnessHash()}); + BOOST_CHECK(it_parent->second.m_wtxids_fee_calculations.value() == expected_wtxids); + BOOST_CHECK(it_child->second.m_wtxids_fee_calculations.value() == expected_wtxids); BOOST_CHECK(expected_feerate.GetFeePerK() > 1000); BOOST_CHECK(submit_cpfp.m_package_feerate.has_value()); BOOST_CHECK_MESSAGE(submit_cpfp.m_package_feerate.value() == expected_feerate, @@ -774,6 +788,9 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK(it_child->second.m_result_type == MempoolAcceptResult::ResultType::VALID); BOOST_CHECK(it_child->second.m_base_fees.value() == 200); BOOST_CHECK(it_child->second.m_effective_feerate.value() == expected_feerate); + std::vector expected_wtxids({tx_parent_cheap->GetWitnessHash(), tx_child_cheap->GetWitnessHash()}); + BOOST_CHECK(it_parent->second.m_wtxids_fee_calculations.value() == expected_wtxids); + BOOST_CHECK(it_child->second.m_wtxids_fee_calculations.value() == expected_wtxids); } // Package feerate is calculated without topology in mind; it's just aggregating fees and sizes. diff --git a/src/validation.cpp b/src/validation.cpp index 8ae4a3ecb8..5d0bd0bee1 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1149,14 +1149,21 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& // make sure we haven't exceeded max mempool size. LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); + 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(); }); // Find the wtxids of the transactions that made it into the mempool. Allow partial submission, // but don't report success unless they all made it into the mempool. for (Workspace& ws : workspaces) { const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : CFeeRate{ws.m_modified_fees, static_cast(ws.m_vsize)}; + const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids : + std::vector({ws.m_ptx->GetWitnessHash()}); if (m_pool.exists(GenTxid::Wtxid(ws.m_ptx->GetWitnessHash()))) { results.emplace(ws.m_ptx->GetWitnessHash(), - MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate)); + MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, + ws.m_base_fees, effective_feerate, effective_feerate_wtxids)); GetMainSignals().TransactionAddedToMempool(ws.m_ptx, m_pool.GetAndIncrementSequence()); } else { all_submitted = false; @@ -1185,16 +1192,19 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef if (!ConsensusScriptChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); const CFeeRate effective_feerate{ws.m_modified_fees, static_cast(ws.m_vsize)}; + const std::vector single_wtxid{ws.m_ptx->GetWitnessHash()}; // Tx was accepted, but not added if (args.m_test_accept) { - return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate); + return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, + ws.m_base_fees, effective_feerate, single_wtxid); } if (!Finalize(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); GetMainSignals().TransactionAddedToMempool(ptx, m_pool.GetAndIncrementSequence()); - return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, effective_feerate); + return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_vsize, ws.m_base_fees, + effective_feerate, single_wtxid); } PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::vector& txns, ATMPArgs& args) @@ -1252,6 +1262,10 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: return PackageMempoolAcceptResult(package_state, package_feerate, 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)) { @@ -1263,11 +1277,12 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: if (args.m_test_accept) { const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : CFeeRate{ws.m_modified_fees, static_cast(ws.m_vsize)}; - // When test_accept=true, transactions that pass PolicyScriptChecks are valid because there are - // no further mempool checks (passing PolicyScriptChecks implies passing ConsensusScriptChecks). + const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids : + std::vector{ws.m_ptx->GetWitnessHash()}; results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), - ws.m_vsize, ws.m_base_fees, effective_feerate)); + ws.m_vsize, ws.m_base_fees, effective_feerate, + effective_feerate_wtxids)); } } diff --git a/src/validation.h b/src/validation.h index 8701326277..6746ca1c5e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -137,8 +137,16 @@ struct MempoolAcceptResult { /** The feerate at which this transaction was considered. This includes any fee delta added * 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. Only present when m_result_type = ResultType::VALID. */ + * 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 + * transaction's wtxid and may include others if this transaction was validated as part of a + * package. This is not necessarily equivalent to the list of transactions passed to + * ProcessNewPackage(). + * 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. */ @@ -151,8 +159,10 @@ struct MempoolAcceptResult { static MempoolAcceptResult Success(std::list&& replaced_txns, int64_t vsize, CAmount fees, - CFeeRate effective_feerate) { - return MempoolAcceptResult(std::move(replaced_txns), vsize, fees, effective_feerate); + CFeeRate effective_feerate, + const std::vector& wtxids_fee_calculations) { + return MempoolAcceptResult(std::move(replaced_txns), vsize, fees, + effective_feerate, wtxids_fee_calculations); } static MempoolAcceptResult MempoolTx(int64_t vsize, CAmount fees) { @@ -175,12 +185,14 @@ private: explicit MempoolAcceptResult(std::list&& replaced_txns, int64_t vsize, CAmount fees, - CFeeRate effective_feerate) + CFeeRate effective_feerate, + const std::vector& wtxids_fee_calculations) : m_result_type(ResultType::VALID), m_replaced_transactions(std::move(replaced_txns)), m_vsize{vsize}, m_base_fees(fees), - m_effective_feerate(effective_feerate) {} + 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) From 1691eaa818f7a7b22907f756490b842d80a9a21d Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 28 Sep 2022 14:07:56 +0100 Subject: [PATCH 05/10] [rpc] return effective-feerate in testmempoolaccept and submitpackage --- src/rpc/mempool.cpp | 10 +++++++ test/functional/mempool_accept.py | 5 +++- test/functional/p2p_segwit.py | 10 ++++--- test/functional/rpc_packages.py | 45 ++++++++++++++++++++----------- 4 files changed, 50 insertions(+), 20 deletions(-) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 58e71a0604..5a8724196a 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -126,6 +126,7 @@ static RPCHelpMan testmempoolaccept() {RPCResult::Type::OBJ, "fees", /*optional=*/true, "Transaction fees (only present if 'allowed' is true)", { {RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT}, + {RPCResult::Type::STR_AMOUNT, "effective-feerate", /*optional=*/false, "the effective feerate in " + CURRENCY_UNIT + " per KvB. May differ from the base feerate if, for example, there are modified fees from prioritisetransaction or a package feerate was used."}, }}, {RPCResult::Type::STR, "reject-reason", /*optional=*/true, "Rejection string (only present when 'allowed' is false)"}, }}, @@ -217,6 +218,7 @@ static RPCHelpMan testmempoolaccept() result_inner.pushKV("vsize", virtual_size); UniValue fees(UniValue::VOBJ); fees.pushKV("base", ValueFromAmount(fee)); + fees.pushKV("effective-feerate", ValueFromAmount(tx_result.m_effective_feerate.value().GetFeePerK())); result_inner.pushKV("fees", fees); } } else { @@ -768,6 +770,7 @@ static RPCHelpMan submitpackage() {RPCResult::Type::NUM, "vsize", "Virtual transaction size as defined in BIP 141."}, {RPCResult::Type::OBJ, "fees", "Transaction fees", { {RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT}, + {RPCResult::Type::STR_AMOUNT, "effective-feerate", /*optional=*/true, "if the transaction was not already in the mempool, the effective feerate in " + CURRENCY_UNIT + " per KvB. For example, the package feerate and/or feerate with modified fees from prioritisetransaction."}, }}, }} }}, @@ -856,6 +859,7 @@ static RPCHelpMan submitpackage() CHECK_NONFATAL(it != package_result.m_tx_results.end()); UniValue result_inner{UniValue::VOBJ}; result_inner.pushKV("txid", tx->GetHash().GetHex()); + const auto& tx_result = it->second; if (it->second.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS) { result_inner.pushKV("other-wtxid", it->second.m_other_wtxid.value().GetHex()); } @@ -864,6 +868,12 @@ static RPCHelpMan submitpackage() result_inner.pushKV("vsize", int64_t{it->second.m_vsize.value()}); UniValue fees(UniValue::VOBJ); fees.pushKV("base", ValueFromAmount(it->second.m_base_fees.value())); + if (tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID) { + // Effective feerate is not provided for MEMPOOL_ENTRY transactions even + // though modified fees is known, because it is unknown whether package + // feerate was used when it was originally submitted. + fees.pushKV("effective-feerate", ValueFromAmount(tx_result.m_effective_feerate.value().GetFeePerK())); + } result_inner.pushKV("fees", fees); if (it->second.m_replaced_transactions.has_value()) { for (const auto& ptx : it->second.m_replaced_transactions.value()) { diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index abbca15bed..12ea7a00b1 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -58,7 +58,10 @@ class MempoolAcceptanceTest(BitcoinTestFramework): """Wrapper to check result of testmempoolaccept on node_0's mempool""" result_test = self.nodes[0].testmempoolaccept(*args, **kwargs) for r in result_test: - r.pop('wtxid') # Skip check for now + # Skip these checks for now + r.pop('wtxid') + if "fees" in r: + r["fees"].pop("effective-feerate") assert_equal(result_expected, result_test) assert_equal(self.nodes[0].getmempoolinfo()['size'], self.mempool_size) # Must not change mempool state diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 2ddc09b13b..0072478f1c 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -622,8 +622,9 @@ class SegWitTest(BitcoinTestFramework): if not self.segwit_active: # Just check mempool acceptance, but don't add the transaction to the mempool, since witness is disallowed # in blocks and the tx is impossible to mine right now. - assert_equal( - self.nodes[0].testmempoolaccept([tx3.serialize_with_witness().hex()]), + testres3 = self.nodes[0].testmempoolaccept([tx3.serialize_with_witness().hex()]) + testres3[0]["fees"].pop("effective-feerate") + assert_equal(testres3, [{ 'txid': tx3.hash, 'wtxid': tx3.getwtxid(), @@ -639,8 +640,9 @@ class SegWitTest(BitcoinTestFramework): tx3 = tx tx3.vout = [tx3_out] tx3.rehash() - assert_equal( - self.nodes[0].testmempoolaccept([tx3.serialize_with_witness().hex()]), + testres3_replaced = self.nodes[0].testmempoolaccept([tx3.serialize_with_witness().hex()]) + testres3_replaced[0]["fees"].pop("effective-feerate") + assert_equal(testres3_replaced, [{ 'txid': tx3.hash, 'wtxid': tx3.getwtxid(), diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index f1352f88d8..f3b5f4d2dc 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -20,6 +20,7 @@ from test_framework.util import ( assert_raises_rpc_error, ) from test_framework.wallet import ( + COIN, DEFAULT_FEE, MiniWallet, ) @@ -239,11 +240,13 @@ class RPCPackagesTest(BitcoinTestFramework): coin = self.wallet.get_utxo() fee = Decimal("0.00125000") replaceable_tx = self.wallet.create_self_transfer(utxo_to_spend=coin, sequence=MAX_BIP125_RBF_SEQUENCE, fee = fee) - testres_replaceable = node.testmempoolaccept([replaceable_tx["hex"]]) - assert_equal(testres_replaceable, [ - {"txid": replaceable_tx["txid"], "wtxid": replaceable_tx["wtxid"], - "allowed": True, "vsize": replaceable_tx["tx"].get_vsize(), "fees": { "base": fee }} - ]) + testres_replaceable = node.testmempoolaccept([replaceable_tx["hex"]])[0] + assert_equal(testres_replaceable["txid"], replaceable_tx["txid"]) + assert_equal(testres_replaceable["wtxid"], replaceable_tx["wtxid"]) + assert testres_replaceable["allowed"] + assert_equal(testres_replaceable["vsize"], replaceable_tx["tx"].get_vsize()) + assert_equal(testres_replaceable["fees"]["base"], fee) + assert_fee_amount(fee, replaceable_tx["tx"].get_vsize(), testres_replaceable["fees"]["effective-feerate"]) # Replacement transaction is identical except has double the fee replacement_tx = self.wallet.create_self_transfer(utxo_to_spend=coin, sequence=MAX_BIP125_RBF_SEQUENCE, fee = 2 * fee) @@ -287,11 +290,13 @@ class RPCPackagesTest(BitcoinTestFramework): peer = node.add_p2p_connection(P2PTxInvStore()) package_txns = [] + presubmitted_wtxids = set() for _ in range(num_parents): parent_tx = self.wallet.create_self_transfer(fee=DEFAULT_FEE) package_txns.append(parent_tx) if partial_submit and random.choice([True, False]): node.sendrawtransaction(parent_tx["hex"]) + presubmitted_wtxids.add(parent_tx["wtxid"]) child_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=[tx["new_utxo"] for tx in package_txns], fee_per_output=10000) #DEFAULT_FEE package_txns.append(child_tx) @@ -302,14 +307,14 @@ class RPCPackagesTest(BitcoinTestFramework): for package_txn in package_txns: tx = package_txn["tx"] assert tx.getwtxid() in submitpackage_result["tx-results"] - tx_result = submitpackage_result["tx-results"][tx.getwtxid()] - assert_equal(tx_result, { - "txid": package_txn["txid"], - "vsize": tx.get_vsize(), - "fees": { - "base": DEFAULT_FEE, - } - }) + wtxid = tx.getwtxid() + assert wtxid in submitpackage_result["tx-results"] + tx_result = submitpackage_result["tx-results"][wtxid] + assert_equal(tx_result["txid"], tx.rehash()) + assert_equal(tx_result["vsize"], tx.get_vsize()) + assert_equal(tx_result["fees"]["base"], DEFAULT_FEE) + if wtxid not in presubmitted_wtxids: + assert_fee_amount(DEFAULT_FEE, tx.get_vsize(), tx_result["fees"]["effective-feerate"]) # submitpackage result should be consistent with testmempoolaccept and getmempoolentry self.assert_equal_package_results(node, testmempoolaccept_result, submitpackage_result) @@ -328,8 +333,11 @@ class RPCPackagesTest(BitcoinTestFramework): node = self.nodes[0] peer = node.add_p2p_connection(P2PTxInvStore()) + # Package with 2 parents and 1 child. One parent pays for itself using modified fees, and + # another has 0 fees but is bumped by child. tx_poor = self.wallet.create_self_transfer(fee=0, fee_rate=0) - tx_rich = self.wallet.create_self_transfer(fee=DEFAULT_FEE) + tx_rich = self.wallet.create_self_transfer(fee=0, fee_rate=0) + node.prioritisetransaction(tx_rich["txid"], 0, int(DEFAULT_FEE * COIN)) package_txns = [tx_rich, tx_poor] coins = [tx["new_utxo"] for tx in package_txns] tx_child = self.wallet.create_self_transfer_multi(utxos_to_spend=coins, fee_per_output=10000) #DEFAULT_FEE @@ -340,9 +348,16 @@ class RPCPackagesTest(BitcoinTestFramework): rich_parent_result = submitpackage_result["tx-results"][tx_rich["wtxid"]] poor_parent_result = submitpackage_result["tx-results"][tx_poor["wtxid"]] child_result = submitpackage_result["tx-results"][tx_child["tx"].getwtxid()] - assert_equal(rich_parent_result["fees"]["base"], DEFAULT_FEE) + assert_equal(rich_parent_result["fees"]["base"], 0) assert_equal(poor_parent_result["fees"]["base"], 0) assert_equal(child_result["fees"]["base"], DEFAULT_FEE) + # The "rich" parent does not require CPFP so its effective feerate. + assert_fee_amount(DEFAULT_FEE, tx_rich["tx"].get_vsize(), rich_parent_result["fees"]["effective-feerate"]) + # The "poor" parent and child's effective feerates are the same, composed of the child's fee + # divided by their combined vsize. + assert_fee_amount(DEFAULT_FEE, tx_poor["tx"].get_vsize() + tx_child["tx"].get_vsize(), poor_parent_result["fees"]["effective-feerate"]) + assert_fee_amount(DEFAULT_FEE, tx_poor["tx"].get_vsize() + tx_child["tx"].get_vsize(), child_result["fees"]["effective-feerate"]) + # Package feerate is calculated for the remaining transactions after deduplication and # individual submission. Since this package had a 0-fee parent, package feerate must have # been used and returned. From 601bac88cb95404e7d38ac6348d959c0e06bd922 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 28 Sep 2022 14:07:56 +0100 Subject: [PATCH 06/10] [rpc] return effective-includes in testmempoolaccept and submitpackage --- src/rpc/mempool.cpp | 16 ++++++++++++++++ test/functional/mempool_accept.py | 1 + test/functional/p2p_segwit.py | 2 ++ test/functional/rpc_packages.py | 5 +++++ 4 files changed, 24 insertions(+) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 5a8724196a..e69088b76e 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -127,6 +127,9 @@ static RPCHelpMan testmempoolaccept() { {RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT}, {RPCResult::Type::STR_AMOUNT, "effective-feerate", /*optional=*/false, "the effective feerate in " + CURRENCY_UNIT + " per KvB. May differ from the base feerate if, for example, there are modified fees from prioritisetransaction or a package feerate was used."}, + {RPCResult::Type::ARR, "effective-includes", /*optional=*/false, "transactions whose fees and vsizes are included in effective-feerate.", + {RPCResult{RPCResult::Type::STR_HEX, "", "transaction wtxid in hex"}, + }}, }}, {RPCResult::Type::STR, "reject-reason", /*optional=*/true, "Rejection string (only present when 'allowed' is false)"}, }}, @@ -219,6 +222,11 @@ static RPCHelpMan testmempoolaccept() UniValue fees(UniValue::VOBJ); fees.pushKV("base", ValueFromAmount(fee)); fees.pushKV("effective-feerate", ValueFromAmount(tx_result.m_effective_feerate.value().GetFeePerK())); + UniValue effective_includes_res(UniValue::VARR); + for (const auto& wtxid : tx_result.m_wtxids_fee_calculations.value()) { + effective_includes_res.push_back(wtxid.ToString()); + } + fees.pushKV("effective-includes", effective_includes_res); result_inner.pushKV("fees", fees); } } else { @@ -771,6 +779,9 @@ static RPCHelpMan submitpackage() {RPCResult::Type::OBJ, "fees", "Transaction fees", { {RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT}, {RPCResult::Type::STR_AMOUNT, "effective-feerate", /*optional=*/true, "if the transaction was not already in the mempool, the effective feerate in " + CURRENCY_UNIT + " per KvB. For example, the package feerate and/or feerate with modified fees from prioritisetransaction."}, + {RPCResult::Type::ARR, "effective-includes", /*optional=*/true, "if effective-feerate is provided, the wtxids of the transactions whose fees and vsizes are included in effective-feerate.", + {{RPCResult::Type::STR_HEX, "", "transaction wtxid in hex"}, + }}, }}, }} }}, @@ -873,6 +884,11 @@ static RPCHelpMan submitpackage() // though modified fees is known, because it is unknown whether package // feerate was used when it was originally submitted. fees.pushKV("effective-feerate", ValueFromAmount(tx_result.m_effective_feerate.value().GetFeePerK())); + UniValue effective_includes_res(UniValue::VARR); + for (const auto& wtxid : tx_result.m_wtxids_fee_calculations.value()) { + effective_includes_res.push_back(wtxid.ToString()); + } + fees.pushKV("effective-includes", effective_includes_res); } result_inner.pushKV("fees", fees); if (it->second.m_replaced_transactions.has_value()) { diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index 12ea7a00b1..19cb65be36 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -62,6 +62,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework): r.pop('wtxid') if "fees" in r: r["fees"].pop("effective-feerate") + r["fees"].pop("effective-includes") assert_equal(result_expected, result_test) assert_equal(self.nodes[0].getmempoolinfo()['size'], self.mempool_size) # Must not change mempool state diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 0072478f1c..b0900e49b8 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -624,6 +624,7 @@ class SegWitTest(BitcoinTestFramework): # in blocks and the tx is impossible to mine right now. testres3 = self.nodes[0].testmempoolaccept([tx3.serialize_with_witness().hex()]) testres3[0]["fees"].pop("effective-feerate") + testres3[0]["fees"].pop("effective-includes") assert_equal(testres3, [{ 'txid': tx3.hash, @@ -642,6 +643,7 @@ class SegWitTest(BitcoinTestFramework): tx3.rehash() testres3_replaced = self.nodes[0].testmempoolaccept([tx3.serialize_with_witness().hex()]) testres3_replaced[0]["fees"].pop("effective-feerate") + testres3_replaced[0]["fees"].pop("effective-includes") assert_equal(testres3_replaced, [{ 'txid': tx3.hash, diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index f3b5f4d2dc..ba782c5bb9 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -247,6 +247,7 @@ class RPCPackagesTest(BitcoinTestFramework): assert_equal(testres_replaceable["vsize"], replaceable_tx["tx"].get_vsize()) assert_equal(testres_replaceable["fees"]["base"], fee) assert_fee_amount(fee, replaceable_tx["tx"].get_vsize(), testres_replaceable["fees"]["effective-feerate"]) + assert_equal(testres_replaceable["fees"]["effective-includes"], [replaceable_tx["wtxid"]]) # Replacement transaction is identical except has double the fee replacement_tx = self.wallet.create_self_transfer(utxo_to_spend=coin, sequence=MAX_BIP125_RBF_SEQUENCE, fee = 2 * fee) @@ -315,6 +316,7 @@ class RPCPackagesTest(BitcoinTestFramework): assert_equal(tx_result["fees"]["base"], DEFAULT_FEE) if wtxid not in presubmitted_wtxids: assert_fee_amount(DEFAULT_FEE, tx.get_vsize(), tx_result["fees"]["effective-feerate"]) + assert_equal(tx_result["fees"]["effective-includes"], [wtxid]) # submitpackage result should be consistent with testmempoolaccept and getmempoolentry self.assert_equal_package_results(node, testmempoolaccept_result, submitpackage_result) @@ -353,10 +355,13 @@ class RPCPackagesTest(BitcoinTestFramework): assert_equal(child_result["fees"]["base"], DEFAULT_FEE) # The "rich" parent does not require CPFP so its effective feerate. assert_fee_amount(DEFAULT_FEE, tx_rich["tx"].get_vsize(), rich_parent_result["fees"]["effective-feerate"]) + assert_equal(rich_parent_result["fees"]["effective-includes"], [tx_rich["wtxid"]]) # The "poor" parent and child's effective feerates are the same, composed of the child's fee # divided by their combined vsize. assert_fee_amount(DEFAULT_FEE, tx_poor["tx"].get_vsize() + tx_child["tx"].get_vsize(), poor_parent_result["fees"]["effective-feerate"]) assert_fee_amount(DEFAULT_FEE, tx_poor["tx"].get_vsize() + tx_child["tx"].get_vsize(), child_result["fees"]["effective-feerate"]) + assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], poor_parent_result["fees"]["effective-includes"]) + assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], child_result["fees"]["effective-includes"]) # Package feerate is calculated for the remaining transactions after deduplication and # individual submission. Since this package had a 0-fee parent, package feerate must have From 5eab397b9840de5a4729bea723794b529e5fcbb4 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 28 Sep 2022 14:31:20 +0100 Subject: [PATCH 07/10] [validation] remove PackageMempoolAcceptResult::m_package_feerate This value creates an extremely confusing interface as its existence is dependent upon implementation details (whether something was submitted on its own, etc). MempoolAcceptResult::m_effective_feerate is much more helpful, as it always exists for submitted transactions. --- src/rpc/mempool.cpp | 4 --- src/test/txpackage_tests.cpp | 46 --------------------------------- src/validation.cpp | 14 +++++----- src/validation.h | 6 +---- test/functional/rpc_packages.py | 12 --------- 5 files changed, 7 insertions(+), 75 deletions(-) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index e69088b76e..44bff55f96 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -785,7 +785,6 @@ static RPCHelpMan submitpackage() }}, }} }}, - {RPCResult::Type::STR_AMOUNT, "package-feerate", /*optional=*/true, "package feerate used for feerate checks in " + CURRENCY_UNIT + " per KvB. Excludes transactions which were deduplicated or accepted individually."}, {RPCResult::Type::ARR, "replaced-transactions", /*optional=*/true, "List of txids of replaced transactions", { {RPCResult::Type::STR_HEX, "", "The transaction id"}, @@ -900,9 +899,6 @@ static RPCHelpMan submitpackage() tx_result_map.pushKV(tx->GetWitnessHash().GetHex(), result_inner); } rpc_result.pushKV("tx-results", tx_result_map); - if (package_result.m_package_feerate.has_value()) { - rpc_result.pushKV("package-feerate", ValueFromAmount(package_result.m_package_feerate.value().GetFeePerK())); - } UniValue replaced_list(UniValue::VARR); for (const uint256& hash : replaced_txids) replaced_list.push_back(hash.ToString()); rpc_result.pushKV("replaced-transactions", replaced_list); diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 0be843d5d9..6a7d2c122d 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -101,9 +101,6 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) BOOST_CHECK(it_child != result_parent_child.m_tx_results.end()); BOOST_CHECK_MESSAGE(it_child->second.m_state.IsValid(), "Package validation unexpectedly failed: " << it_child->second.m_state.GetRejectReason()); - BOOST_CHECK(result_parent_child.m_package_feerate.has_value()); - BOOST_CHECK(result_parent_child.m_package_feerate.value() == - CFeeRate(2 * COIN, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child))); BOOST_CHECK(it_child->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_child)) == COIN); BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().size(), 1); BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().front(), tx_child->GetWitnessHash()); @@ -118,7 +115,6 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) auto it_giant_tx = result_single_large.m_tx_results.find(giant_ptx->GetWitnessHash()); BOOST_CHECK(it_giant_tx != result_single_large.m_tx_results.end()); BOOST_CHECK_EQUAL(it_giant_tx->second.m_state.GetRejectReason(), "tx-size"); - BOOST_CHECK(result_single_large.m_package_feerate == std::nullopt); // Check that mempool size hasn't changed. BOOST_CHECK_EQUAL(m_node.mempool->size(), initialPoolSize); @@ -239,7 +235,6 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetRejectReason(), "package-not-child-with-parents"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); - BOOST_CHECK(result_unrelated_submit.m_package_feerate == std::nullopt); // Parent and Child (and Grandchild) Package Package package_parent_child; @@ -281,7 +276,6 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK_EQUAL(result_3gen_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(result_3gen_submit.m_state.GetRejectReason(), "package-not-child-with-parents"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); - BOOST_CHECK(result_3gen_submit.m_package_feerate == std::nullopt); } // Parent and child package where transactions are invalid for reasons other than fee and @@ -314,8 +308,6 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetRejectReason(), "package-not-child-with-unconfirmed-parents"); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); - - BOOST_CHECK(result_missing_parent.m_package_feerate == std::nullopt); } // Submit package with parent + child. @@ -341,10 +333,6 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash()))); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child->GetHash()))); - - // Since both transactions have high feerates, they each passed validation individually. - // Package validation was unnecessary, so there is no package feerate. - BOOST_CHECK(submit_parent_child.m_package_feerate == std::nullopt); } // Already-in-mempool transactions should be detected and de-duplicated. @@ -365,8 +353,6 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent->GetHash()))); BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_child->GetHash()))); - - BOOST_CHECK(submit_deduped.m_package_feerate == std::nullopt); } } @@ -442,11 +428,8 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_child1->GetHash()))); // Child2 would have been validated individually. - BOOST_CHECK(submit_witness1.m_package_feerate == std::nullopt); - const auto submit_witness2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, {ptx_parent, ptx_child2}, /*test_accept=*/false); - BOOST_CHECK(submit_witness2.m_package_feerate == std::nullopt); BOOST_CHECK_MESSAGE(submit_witness2.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_witness2.m_state.GetRejectReason()); auto it_parent2_deduped = submit_witness2.m_tx_results.find(ptx_parent->GetWitnessHash()); @@ -470,7 +453,6 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) auto it_child_dup = submit_segwit_dedup.m_tx_results.find(ptx_child1->GetWitnessHash()); BOOST_CHECK(it_parent_dup->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY); BOOST_CHECK(it_child_dup->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY); - BOOST_CHECK(submit_witness2.m_package_feerate == std::nullopt); } // Try submitting Package1{child2, grandchild} where child2 is same-txid-different-witness as @@ -505,9 +487,6 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_child2->GetHash()))); BOOST_CHECK(!m_node.mempool->exists(GenTxid::Wtxid(ptx_child2->GetWitnessHash()))); BOOST_CHECK(m_node.mempool->exists(GenTxid::Wtxid(ptx_grandchild->GetWitnessHash()))); - - // Since child2 is ignored, grandchild would be validated individually. - BOOST_CHECK(submit_spend_ignored.m_package_feerate == std::nullopt); } // A package Package{parent1, parent2, parent3, child} where the parents are a mixture of @@ -620,11 +599,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_mixed_child->GetHash()))); // package feerate should include parent3 and child. It should not include parent1 or parent2_v1. - BOOST_CHECK(mixed_result.m_package_feerate.has_value()); const CFeeRate expected_feerate(1 * COIN, GetVirtualTransactionSize(*ptx_parent3) + GetVirtualTransactionSize(*ptx_mixed_child)); - BOOST_CHECK_MESSAGE(mixed_result.m_package_feerate.value() == expected_feerate, - strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), - mixed_result.m_package_feerate.value().ToString())); BOOST_CHECK(it_parent3->second.m_effective_feerate.value() == expected_feerate); BOOST_CHECK(it_child->second.m_effective_feerate.value() == expected_feerate); std::vector expected_wtxids({ptx_parent3->GetWitnessHash(), ptx_mixed_child->GetWitnessHash()}); @@ -678,11 +653,6 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK(submit_cpfp_deprio.m_tx_results.empty()); BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); const CFeeRate expected_feerate(0, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child)); - BOOST_CHECK(submit_cpfp_deprio.m_package_feerate.has_value()); - BOOST_CHECK(submit_cpfp_deprio.m_package_feerate.value() == CFeeRate{0}); - BOOST_CHECK_MESSAGE(submit_cpfp_deprio.m_package_feerate.value() == expected_feerate, - strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), - submit_cpfp_deprio.m_package_feerate.value().ToString())); } // Clear the prioritisation of the parent transaction. @@ -718,10 +688,6 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK(it_parent->second.m_wtxids_fee_calculations.value() == expected_wtxids); BOOST_CHECK(it_child->second.m_wtxids_fee_calculations.value() == expected_wtxids); BOOST_CHECK(expected_feerate.GetFeePerK() > 1000); - BOOST_CHECK(submit_cpfp.m_package_feerate.has_value()); - BOOST_CHECK_MESSAGE(submit_cpfp.m_package_feerate.value() == expected_feerate, - strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), - submit_cpfp.m_package_feerate.value().ToString())); } // Just because we allow low-fee parents doesn't mean we allow low-feerate packages. @@ -756,10 +722,6 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) const CFeeRate expected_feerate(200, GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)); BOOST_CHECK(expected_feerate.GetFeePerK() < 1000); - BOOST_CHECK(submit_package_too_low.m_package_feerate.has_value()); - BOOST_CHECK_MESSAGE(submit_package_too_low.m_package_feerate.value() == expected_feerate, - strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), - submit_package_too_low.m_package_feerate.value().ToString())); } // Package feerate includes the modified fees of the transactions. @@ -774,10 +736,6 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) "Package validation unexpectedly failed" << submit_prioritised_package.m_state.GetRejectReason()); const CFeeRate expected_feerate(1 * COIN + 200, GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)); - BOOST_CHECK(submit_prioritised_package.m_package_feerate.has_value()); - BOOST_CHECK_MESSAGE(submit_prioritised_package.m_package_feerate.value() == expected_feerate, - strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(), - submit_prioritised_package.m_package_feerate.value().ToString())); auto it_parent = submit_prioritised_package.m_tx_results.find(tx_parent_cheap->GetWitnessHash()); auto it_child = submit_prioritised_package.m_tx_results.find(tx_child_cheap->GetWitnessHash()); BOOST_CHECK(it_parent != submit_prioritised_package.m_tx_results.end()); @@ -823,10 +781,6 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK_MESSAGE(submit_rich_parent.m_state.IsInvalid(), "Package validation unexpectedly succeeded"); // The child would have been validated on its own and failed, then submitted as a "package" of 1. - // The package feerate is just the child's feerate, which is 0sat/vb. - BOOST_CHECK(submit_rich_parent.m_package_feerate.has_value()); - BOOST_CHECK_MESSAGE(submit_rich_parent.m_package_feerate.value() == CFeeRate(), - "expected 0, got " << submit_rich_parent.m_package_feerate.value().ToString()); BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetRejectReason(), "package-fee-too-low"); diff --git a/src/validation.cpp b/src/validation.cpp index 5d0bd0bee1..b8e29ce2c8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1251,7 +1251,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: 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_feerate, {}); + return PackageMempoolAcceptResult(package_state, {}); } // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction, @@ -1259,7 +1259,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // transactions, but this exemption is not extended to packages in CheckPackageLimits(). std::string err_string; if (txns.size() > 1 && !PackageMempoolChecks(txns, package_state)) { - return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); + return PackageMempoolAcceptResult(package_state, std::move(results)); } std::vector all_package_wtxids; @@ -1272,7 +1272,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // Exit early to avoid doing pointless work. Update the failed tx result; the rest are unfinished. package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); - return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); + return PackageMempoolAcceptResult(package_state, std::move(results)); } if (args.m_test_accept) { const auto effective_feerate = args.m_package_feerates ? ws.m_package_feerate : @@ -1286,14 +1286,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: } } - if (args.m_test_accept) return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); + if (args.m_test_accept) return PackageMempoolAcceptResult(package_state, std::move(results)); if (!SubmitPackage(args, workspaces, package_state, results)) { // PackageValidationState filled in by SubmitPackage(). - return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); + return PackageMempoolAcceptResult(package_state, std::move(results)); } - return PackageMempoolAcceptResult(package_state, package_feerate, std::move(results)); + return PackageMempoolAcceptResult(package_state, std::move(results)); } PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, ATMPArgs& args) @@ -1418,7 +1418,6 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // Nothing to do if the entire package has already been submitted. if (quit_early || txns_new.empty()) { - // No package feerate when no package validation was done. return PackageMempoolAcceptResult(package_state_quit_early, std::move(results)); } // Validate the (deduplicated) transactions as a package. @@ -1427,7 +1426,6 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, for (const auto& [wtxid, mempoolaccept_res] : results) { submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res); } - if (submission_result.m_state.IsValid()) assert(submission_result.m_package_feerate.has_value()); return submission_result; } diff --git a/src/validation.h b/src/validation.h index 6746ca1c5e..900f380563 100644 --- a/src/validation.h +++ b/src/validation.h @@ -216,10 +216,6 @@ struct PackageMempoolAcceptResult * was a package-wide error (see result in m_state), m_tx_results will be empty. */ std::map m_tx_results; - /** Package feerate, defined as the aggregated modified fees divided by the total virtual size - * of all transactions in the package. May be unavailable if some inputs were not available or - * a transaction failure caused validation to terminate early. */ - std::optional m_package_feerate; explicit PackageMempoolAcceptResult(PackageValidationState state, std::map&& results) @@ -227,7 +223,7 @@ struct PackageMempoolAcceptResult explicit PackageMempoolAcceptResult(PackageValidationState state, CFeeRate feerate, std::map&& results) - : m_state{state}, m_tx_results(std::move(results)), m_package_feerate{feerate} {} + : m_state{state}, m_tx_results(std::move(results)) {} /** Constructor to create a PackageMempoolAcceptResult from a single MempoolAcceptResult */ explicit PackageMempoolAcceptResult(const uint256& wtxid, const MempoolAcceptResult& result) diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index ba782c5bb9..10388399ad 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -321,12 +321,6 @@ class RPCPackagesTest(BitcoinTestFramework): # submitpackage result should be consistent with testmempoolaccept and getmempoolentry self.assert_equal_package_results(node, testmempoolaccept_result, submitpackage_result) - # Package feerate is calculated for the remaining transactions after deduplication and - # individual submission. If only 0 or 1 transaction is left, e.g. because all transactions - # had high-feerates or were already in the mempool, no package feerate is provided. - # In this case, since all of the parents have high fees, each is accepted individually. - assert "package-feerate" not in submitpackage_result - # The node should announce each transaction. No guarantees for propagation. peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns]) self.generate(node, 1) @@ -363,12 +357,6 @@ class RPCPackagesTest(BitcoinTestFramework): assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], poor_parent_result["fees"]["effective-includes"]) assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], child_result["fees"]["effective-includes"]) - # Package feerate is calculated for the remaining transactions after deduplication and - # individual submission. Since this package had a 0-fee parent, package feerate must have - # been used and returned. - assert "package-feerate" in submitpackage_result - assert_fee_amount(DEFAULT_FEE, rich_parent_result["vsize"] + child_result["vsize"], submitpackage_result["package-feerate"]) - # The node will broadcast each transaction, still abiding by its peer's fee filter peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns]) self.generate(node, 1) From da484bc738eca47936a31a2e5ad663417e19f311 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 23 Dec 2022 14:07:33 +0000 Subject: [PATCH 08/10] [doc] release note effective-feerate and effective-includes RPC results No release note for submitpackage because it is regtest-only. --- release-notes-26646.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 release-notes-26646.md diff --git a/release-notes-26646.md b/release-notes-26646.md new file mode 100644 index 0000000000..7f94505a01 --- /dev/null +++ b/release-notes-26646.md @@ -0,0 +1,8 @@ +JSON-RPC +-------- + +The `testmempoolaccept` RPC now returns 2 additional results within the "fees" result: +"effective-feerate" is the feerate including fees and sizes of transactions validated together if +package validation was used, and also includes any modified fees from prioritisetransaction. The +"effective-includes" result lists the wtxids of transactions whose modified fees and sizes were used +in the effective-feerate (#26646). From dae81e01e8698e04afb0db4d1442659c3b48fcf5 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 6 Jan 2023 17:45:45 +0000 Subject: [PATCH 09/10] [refactor] rename variables in AcceptPackage for clarity --- src/validation.cpp | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index b8e29ce2c8..6d324efccf 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1352,7 +1352,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, m_view.SetBackend(m_dummy); LOCK(m_pool.cs); - std::map results; + // Stores final results that won't change + std::map results_final; // Node operators are free to set their mempool policies however they please, nodes may receive // transactions in different orders, and malicious counterparties may try to take advantage of // policy differences to pin or delay propagation of transactions. As such, it's possible for @@ -1363,7 +1364,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // checking ancestor/descendant limits, or double-count transaction fees for fee-related policy. ATMPArgs single_args = ATMPArgs::SingleInPackageAccept(args); bool quit_early{false}; - std::vector txns_new; + std::vector txns_package_eval; for (const auto& tx : package) { const auto& wtxid = tx->GetWitnessHash(); const auto& txid = tx->GetHash(); @@ -1374,7 +1375,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // Exact transaction already exists in the mempool. auto iter = m_pool.GetIter(txid); assert(iter != std::nullopt); - results.emplace(wtxid, MempoolAcceptResult::MempoolTx(iter.value()->GetTxSize(), iter.value()->GetFee())); + results_final.emplace(wtxid, MempoolAcceptResult::MempoolTx(iter.value()->GetTxSize(), iter.value()->GetFee())); } else if (m_pool.exists(GenTxid::Txid(txid))) { // Transaction with the same non-witness data but different witness (same txid, // different wtxid) already exists in the mempool. @@ -1386,7 +1387,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, auto iter = m_pool.GetIter(txid); assert(iter != std::nullopt); // Provide the wtxid of the mempool tx so that the caller can look it up in the mempool. - results.emplace(wtxid, MempoolAcceptResult::MempoolTxDifferentWitness(iter.value()->GetTx().GetWitnessHash())); + results_final.emplace(wtxid, MempoolAcceptResult::MempoolTxDifferentWitness(iter.value()->GetTx().GetWitnessHash())); } else { // Transaction does not already exist in the mempool. // Try submitting the transaction on its own. @@ -1395,7 +1396,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // The transaction succeeded on its own and is now in the mempool. Don't include it // in package validation, because its fees should only be "used" once. assert(m_pool.exists(GenTxid::Wtxid(wtxid))); - results.emplace(wtxid, single_res); + results_final.emplace(wtxid, single_res); } else if (single_res.m_state.GetResult() != TxValidationResult::TX_MEMPOOL_POLICY && single_res.m_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { // Package validation policy only differs from individual policy in its evaluation @@ -1409,21 +1410,21 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // some of them may still be valid. quit_early = true; package_state_quit_early.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); - results.emplace(wtxid, single_res); + results_final.emplace(wtxid, single_res); } else { - txns_new.push_back(tx); + txns_package_eval.push_back(tx); } } } // Nothing to do if the entire package has already been submitted. - if (quit_early || txns_new.empty()) { - return PackageMempoolAcceptResult(package_state_quit_early, std::move(results)); + if (quit_early || txns_package_eval.empty()) { + return PackageMempoolAcceptResult(package_state_quit_early, std::move(results_final)); } // Validate the (deduplicated) transactions as a package. - auto submission_result = AcceptMultipleTransactions(txns_new, args); + auto submission_result = AcceptMultipleTransactions(txns_package_eval, args); // Include already-in-mempool transaction results in the final result. - for (const auto& [wtxid, mempoolaccept_res] : results) { + for (const auto& [wtxid, mempoolaccept_res] : results_final) { submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res); } return submission_result; From 264f9ef17f650035882d24083fb419845942a3ac Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 5 Dec 2022 14:55:52 +0000 Subject: [PATCH 10/10] [validation] return MempoolAcceptResult for every tx on PCKG_TX failure This makes the interface more predictable and useful. The caller understands one or more transactions failed, and can learn what happened with each transaction. We already have this information, so we might as well return it. It doesn't make sense to do this for other PackageValidationResult values because: - PCKG_RESULT_UNSET: this means everything succeeded, so the individual failures are no longer accurate. - PCKG_MEMPOOL_ERROR: something went wrong with the mempool logic; transaction failures might not be meaningful. - PCKG_POLICY: this means something was wrong with the package as a whole. The caller should use the PackageValidationState to find the error, rather than looking at individual MempoolAcceptResults. --- src/test/txpackage_tests.cpp | 18 ++++++++++++++++++ src/validation.cpp | 29 +++++++++++++++++++++++++---- 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 6a7d2c122d..e438867d15 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -90,6 +90,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) const auto result_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, {tx_parent, tx_child}, /*test_accept=*/true); BOOST_CHECK_MESSAGE(result_parent_child.m_state.IsValid(), "Package validation unexpectedly failed: " << result_parent_child.m_state.GetRejectReason()); + BOOST_CHECK(result_parent_child.m_tx_results.size() == 2); auto it_parent = result_parent_child.m_tx_results.find(tx_parent->GetWitnessHash()); auto it_child = result_parent_child.m_tx_results.find(tx_child->GetWitnessHash()); BOOST_CHECK(it_parent != result_parent_child.m_tx_results.end()); @@ -112,6 +113,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) BOOST_CHECK(result_single_large.m_state.IsInvalid()); BOOST_CHECK_EQUAL(result_single_large.m_state.GetResult(), PackageValidationResult::PCKG_TX); BOOST_CHECK_EQUAL(result_single_large.m_state.GetRejectReason(), "transaction failed"); + BOOST_CHECK(result_single_large.m_tx_results.size() == 1); auto it_giant_tx = result_single_large.m_tx_results.find(giant_ptx->GetWitnessHash()); BOOST_CHECK(it_giant_tx != result_single_large.m_tx_results.end()); BOOST_CHECK_EQUAL(it_giant_tx->second.m_state.GetRejectReason(), "tx-size"); @@ -291,9 +293,15 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) BOOST_CHECK(result_quit_early.m_state.IsInvalid()); BOOST_CHECK_EQUAL(result_quit_early.m_state.GetResult(), PackageValidationResult::PCKG_TX); BOOST_CHECK(!result_quit_early.m_tx_results.empty()); + BOOST_CHECK_EQUAL(result_quit_early.m_tx_results.size(), 2); auto it_parent = result_quit_early.m_tx_results.find(tx_parent_invalid->GetWitnessHash()); + auto it_child = result_quit_early.m_tx_results.find(tx_child->GetWitnessHash()); BOOST_CHECK(it_parent != result_quit_early.m_tx_results.end()); + BOOST_CHECK(it_child != result_quit_early.m_tx_results.end()); BOOST_CHECK_EQUAL(it_parent->second.m_state.GetResult(), TxValidationResult::TX_WITNESS_MUTATED); + BOOST_CHECK_EQUAL(it_parent->second.m_state.GetRejectReason(), "bad-witness-nonstandard"); + BOOST_CHECK_EQUAL(it_child->second.m_state.GetResult(), TxValidationResult::TX_MISSING_INPUTS); + BOOST_CHECK_EQUAL(it_child->second.m_state.GetRejectReason(), "bad-txns-inputs-missingorspent"); } // Child with missing parent. @@ -317,6 +325,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) expected_pool_size += 2; BOOST_CHECK_MESSAGE(submit_parent_child.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_parent_child.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_parent_child.m_tx_results.size(), package_parent_child.size()); auto it_parent = submit_parent_child.m_tx_results.find(tx_parent->GetWitnessHash()); auto it_child = submit_parent_child.m_tx_results.find(tx_child->GetWitnessHash()); BOOST_CHECK(it_parent != submit_parent_child.m_tx_results.end()); @@ -341,6 +350,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) package_parent_child, /*test_accept=*/false); BOOST_CHECK_MESSAGE(submit_deduped.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_deduped.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_deduped.m_tx_results.size(), package_parent_child.size()); auto it_parent_deduped = submit_deduped.m_tx_results.find(tx_parent->GetWitnessHash()); auto it_child_deduped = submit_deduped.m_tx_results.find(tx_child->GetWitnessHash()); BOOST_CHECK(it_parent_deduped != submit_deduped.m_tx_results.end()); @@ -415,6 +425,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) {ptx_parent, ptx_child1}, /*test_accept=*/false); BOOST_CHECK_MESSAGE(submit_witness1.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_witness1.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_witness1.m_tx_results.size(), 2); auto it_parent1 = submit_witness1.m_tx_results.find(ptx_parent->GetWitnessHash()); auto it_child1 = submit_witness1.m_tx_results.find(ptx_child1->GetWitnessHash()); BOOST_CHECK(it_parent1 != submit_witness1.m_tx_results.end()); @@ -432,6 +443,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) {ptx_parent, ptx_child2}, /*test_accept=*/false); BOOST_CHECK_MESSAGE(submit_witness2.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_witness2.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_witness2.m_tx_results.size(), 2); auto it_parent2_deduped = submit_witness2.m_tx_results.find(ptx_parent->GetWitnessHash()); auto it_child2 = submit_witness2.m_tx_results.find(ptx_child2->GetWitnessHash()); BOOST_CHECK(it_parent2_deduped != submit_witness2.m_tx_results.end()); @@ -449,6 +461,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) {ptx_parent, ptx_child1}, /*test_accept=*/false); BOOST_CHECK_MESSAGE(submit_segwit_dedup.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_segwit_dedup.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_segwit_dedup.m_tx_results.size(), 2); auto it_parent_dup = submit_segwit_dedup.m_tx_results.find(ptx_parent->GetWitnessHash()); auto it_child_dup = submit_segwit_dedup.m_tx_results.find(ptx_child1->GetWitnessHash()); BOOST_CHECK(it_parent_dup->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY); @@ -477,6 +490,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) {ptx_child2, ptx_grandchild}, /*test_accept=*/false); BOOST_CHECK_MESSAGE(submit_spend_ignored.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_spend_ignored.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_spend_ignored.m_tx_results.size(), 2); auto it_child2_ignored = submit_spend_ignored.m_tx_results.find(ptx_child2->GetWitnessHash()); auto it_grandchild = submit_spend_ignored.m_tx_results.find(ptx_grandchild->GetWitnessHash()); BOOST_CHECK(it_child2_ignored != submit_spend_ignored.m_tx_results.end()); @@ -577,6 +591,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) { const auto mixed_result = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_mixed, false); BOOST_CHECK_MESSAGE(mixed_result.m_state.IsValid(), mixed_result.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(mixed_result.m_tx_results.size(), package_mixed.size()); auto it_parent1 = mixed_result.m_tx_results.find(ptx_parent1->GetWitnessHash()); auto it_parent2 = mixed_result.m_tx_results.find(ptx_parent2_v1->GetWitnessHash()); auto it_parent3 = mixed_result.m_tx_results.find(ptx_parent3->GetWitnessHash()); @@ -648,6 +663,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); const auto submit_cpfp_deprio = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_cpfp, /*test_accept=*/ false); + BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); BOOST_CHECK_MESSAGE(submit_cpfp_deprio.m_state.IsInvalid(), "Package validation unexpectedly succeeded: " << submit_cpfp_deprio.m_state.GetRejectReason()); BOOST_CHECK(submit_cpfp_deprio.m_tx_results.empty()); @@ -667,6 +683,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) expected_pool_size += 2; BOOST_CHECK_MESSAGE(submit_cpfp.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_cpfp.m_state.GetRejectReason()); + BOOST_CHECK_EQUAL(submit_cpfp.m_tx_results.size(), package_cpfp.size()); auto it_parent = submit_cpfp.m_tx_results.find(tx_parent->GetWitnessHash()); auto it_child = submit_cpfp.m_tx_results.find(tx_child->GetWitnessHash()); BOOST_CHECK(it_parent != submit_cpfp.m_tx_results.end()); @@ -736,6 +753,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) "Package validation unexpectedly failed" << submit_prioritised_package.m_state.GetRejectReason()); const CFeeRate expected_feerate(1 * COIN + 200, GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)); + BOOST_CHECK_EQUAL(submit_prioritised_package.m_tx_results.size(), package_still_too_low.size()); auto it_parent = submit_prioritised_package.m_tx_results.find(tx_parent_cheap->GetWitnessHash()); auto it_child = submit_prioritised_package.m_tx_results.find(tx_child_cheap->GetWitnessHash()); BOOST_CHECK(it_parent != submit_prioritised_package.m_tx_results.end()); diff --git a/src/validation.cpp b/src/validation.cpp index 6d324efccf..cc89c92805 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1363,6 +1363,11 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // the new transactions. This ensures we don't double-count transaction counts and sizes when // checking ancestor/descendant limits, or double-count transaction fees for fee-related policy. ATMPArgs single_args = ATMPArgs::SingleInPackageAccept(args); + // Results from individual validation. "Nonfinal" because if a transaction fails by itself but + // succeeds later (i.e. when evaluated with a fee-bumping child), the result changes (though not + // reflected in this map). If a transaction fails more than once, we want to return the first + // result, when it was considered on its own. So changes will only be from invalid -> valid. + std::map individual_results_nonfinal; bool quit_early{false}; std::vector txns_package_eval; for (const auto& tx : package) { @@ -1410,22 +1415,38 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // some of them may still be valid. quit_early = true; package_state_quit_early.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); - results_final.emplace(wtxid, single_res); + individual_results_nonfinal.emplace(wtxid, single_res); } else { + individual_results_nonfinal.emplace(wtxid, single_res); txns_package_eval.push_back(tx); } } } - // Nothing to do if the entire package has already been submitted. + // Quit early because package validation won't change the result or the entire package has + // already been submitted. if (quit_early || txns_package_eval.empty()) { + for (const auto& [wtxid, mempoolaccept_res] : individual_results_nonfinal) { + Assume(results_final.emplace(wtxid, mempoolaccept_res).second); + Assume(mempoolaccept_res.m_result_type == MempoolAcceptResult::ResultType::INVALID); + } return PackageMempoolAcceptResult(package_state_quit_early, std::move(results_final)); } - // Validate the (deduplicated) transactions as a package. + // Validate the (deduplicated) transactions as a package. Note that submission_result has its + // own PackageValidationState; package_state_quit_early is unused past this point. auto submission_result = AcceptMultipleTransactions(txns_package_eval, args); // Include already-in-mempool transaction results in the final result. for (const auto& [wtxid, mempoolaccept_res] : results_final) { - submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res); + Assume(submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res).second); + Assume(mempoolaccept_res.m_result_type != MempoolAcceptResult::ResultType::INVALID); + } + if (submission_result.m_state.GetResult() == PackageValidationResult::PCKG_TX) { + // Package validation failed because one or more transactions failed. Provide a result for + // each transaction; if AcceptMultipleTransactions() didn't return a result for a tx, + // include the previous individual failure reason. + submission_result.m_tx_results.insert(individual_results_nonfinal.cbegin(), + individual_results_nonfinal.cend()); + Assume(submission_result.m_tx_results.size() == package.size()); } return submission_result; }