diff options
| -rw-r--r-- | release-notes-26646.md | 8 | ||||
| -rw-r--r-- | src/rpc/mempool.cpp | 30 | ||||
| -rw-r--r-- | src/test/txpackage_tests.cpp | 118 | ||||
| -rw-r--r-- | src/validation.cpp | 115 | ||||
| -rw-r--r-- | src/validation.h | 40 | ||||
| -rwxr-xr-x | test/functional/mempool_accept.py | 6 | ||||
| -rwxr-xr-x | test/functional/p2p_segwit.py | 12 | ||||
| -rwxr-xr-x | test/functional/rpc_packages.py | 60 |
8 files changed, 266 insertions, 123 deletions
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). diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 58e71a0604..44bff55f96 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -126,6 +126,10 @@ 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::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)"}, }}, @@ -217,6 +221,12 @@ 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())); + 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 { @@ -768,10 +778,13 @@ 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."}, + {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"}, + }}, }}, }} }}, - {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"}, @@ -856,6 +869,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 +878,17 @@ 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())); + 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()) { for (const auto& ptx : it->second.m_replaced_transactions.value()) { @@ -874,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 b290a664f9..e438867d15 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -90,17 +90,21 @@ 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()); 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()); - 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()); // A single, giant transaction submitted through ProcessNewPackage fails on single tx policy. CTransactionRef giant_ptx = create_placeholder_tx(999, 999); @@ -109,10 +113,10 @@ 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"); - 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); @@ -233,7 +237,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; @@ -275,7 +278,30 @@ 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 + // missing inputs, so the package validation isn't expected to happen. + { + CScriptWitness bad_witness; + bad_witness.stack.push_back(std::vector<unsigned char>(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()); + 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. @@ -290,8 +316,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. @@ -301,20 +325,23 @@ 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()); 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()))); 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. @@ -323,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()); @@ -335,8 +363,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); } } @@ -399,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()); @@ -412,13 +439,11 @@ 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()); + 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()); @@ -436,11 +461,11 @@ 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); 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 @@ -465,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()); @@ -475,9 +501,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 @@ -568,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()); @@ -590,11 +614,12 @@ 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<uint256> 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); } } @@ -638,16 +663,12 @@ 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()); 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. @@ -662,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()); @@ -677,11 +699,12 @@ 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); + std::vector<uint256> 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, - 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. @@ -716,10 +739,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. @@ -734,10 +753,20 @@ 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())); + 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()); + 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); + std::vector<uint256> 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. @@ -770,10 +799,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"); @@ -783,6 +808,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 e24d39170e..cc89c92805 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; @@ -1144,12 +1149,21 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>& // make sure we haven't exceeded max mempool size. LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); + std::vector<uint256> 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<uint32_t>(ws.m_vsize)}; + const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids : + std::vector<uint256>({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)); + 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; @@ -1177,16 +1191,20 @@ 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<uint32_t>(ws.m_vsize)}; + const std::vector<uint256> 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); + 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); + 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<CTransactionRef>& txns, ATMPArgs& args) @@ -1233,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, @@ -1241,51 +1259,60 @@ 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<uint256> 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)) { // 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) { - // 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 = args.m_package_feerates ? ws.m_package_feerate : + CFeeRate{ws.m_modified_fees, static_cast<uint32_t>(ws.m_vsize)}; + const auto effective_feerate_wtxids = args.m_package_feerates ? all_package_wtxids : + std::vector<uint256>{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)); + ws.m_vsize, ws.m_base_fees, effective_feerate, + effective_feerate_wtxids)); } } - 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) { 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,15 +1344,16 @@ 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. m_view.SetBackend(m_dummy); LOCK(m_pool.cs); - std::map<const uint256, const MempoolAcceptResult> results; + // Stores final results that won't change + std::map<const uint256, const MempoolAcceptResult> 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 @@ -1335,8 +1363,13 @@ 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<uint256, MempoolAcceptResult> individual_results_nonfinal; bool quit_early{false}; - std::vector<CTransactionRef> txns_new; + std::vector<CTransactionRef> txns_package_eval; for (const auto& tx : package) { const auto& wtxid = tx->GetWitnessHash(); const auto& txid = tx->GetHash(); @@ -1347,7 +1380,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. @@ -1359,7 +1392,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. @@ -1368,7 +1401,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 @@ -1381,24 +1414,40 @@ 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"); + individual_results_nonfinal.emplace(wtxid, single_res); } else { - txns_new.push_back(tx); + individual_results_nonfinal.emplace(wtxid, single_res); + txns_package_eval.push_back(tx); } } } - // 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)); + // 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. - auto submission_result = AcceptMultipleTransactions(txns_new, args); + // 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) { - submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res); + for (const auto& [wtxid, mempoolaccept_res] : results_final) { + 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()); } - 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 63ff9247be..900f380563 100644 --- a/src/validation.h +++ b/src/validation.h @@ -134,6 +134,19 @@ struct MempoolAcceptResult { const std::optional<int64_t> m_vsize; /** Raw base fees in satoshis. */ const std::optional<CAmount> 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 (see m_wtxids_fee_calculations below). + * Only present when m_result_type = ResultType::VALID. + */ + const std::optional<CFeeRate> 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<std::vector<uint256>> 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. */ @@ -143,8 +156,13 @@ struct MempoolAcceptResult { return MempoolAcceptResult(state); } - static MempoolAcceptResult Success(std::list<CTransactionRef>&& replaced_txns, int64_t vsize, CAmount fees) { - return MempoolAcceptResult(std::move(replaced_txns), vsize, fees); + static MempoolAcceptResult Success(std::list<CTransactionRef>&& replaced_txns, + int64_t vsize, + CAmount fees, + CFeeRate effective_feerate, + const std::vector<uint256>& wtxids_fee_calculations) { + return MempoolAcceptResult(std::move(replaced_txns), vsize, fees, + effective_feerate, wtxids_fee_calculations); } static MempoolAcceptResult MempoolTx(int64_t vsize, CAmount fees) { @@ -164,9 +182,17 @@ private: } /** Constructor for success case */ - explicit MempoolAcceptResult(std::list<CTransactionRef>&& replaced_txns, int64_t vsize, CAmount fees) + explicit MempoolAcceptResult(std::list<CTransactionRef>&& replaced_txns, + int64_t vsize, + CAmount fees, + CFeeRate effective_feerate, + const std::vector<uint256>& wtxids_fee_calculations) : 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), + 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) @@ -190,10 +216,6 @@ struct PackageMempoolAcceptResult * was a package-wide error (see result in m_state), m_tx_results will be empty. */ std::map<const uint256, const MempoolAcceptResult> 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<CFeeRate> m_package_feerate; explicit PackageMempoolAcceptResult(PackageValidationState state, std::map<const uint256, const MempoolAcceptResult>&& results) @@ -201,7 +223,7 @@ struct PackageMempoolAcceptResult explicit PackageMempoolAcceptResult(PackageValidationState state, CFeeRate feerate, std::map<const uint256, const MempoolAcceptResult>&& 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/mempool_accept.py b/test/functional/mempool_accept.py index abbca15bed..19cb65be36 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -58,7 +58,11 @@ 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") + 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 2ddc09b13b..b0900e49b8 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -622,8 +622,10 @@ 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") + testres3[0]["fees"].pop("effective-includes") + assert_equal(testres3, [{ 'txid': tx3.hash, 'wtxid': tx3.getwtxid(), @@ -639,8 +641,10 @@ 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") + testres3_replaced[0]["fees"].pop("effective-includes") + 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..10388399ad 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,14 @@ 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"]) + 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) @@ -287,11 +291,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,24 +308,19 @@ 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"]) + 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) - # 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) @@ -328,8 +329,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,14 +344,18 @@ 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) - # 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 "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"]) # 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]) |