From 38f70ba6ac86fb96c60571d2e1f316315c1c73cc Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Mon, 27 Nov 2023 14:50:55 -0500 Subject: [PATCH] RPC: Add maxfeerate and maxburnamount args to submitpackage And thread the feerate value through ProcessNewPackage to reject individual transactions that exceed the given feerate. This allows subpackage processing, and is compatible with future package RBF work. --- src/node/transaction.h | 6 ++++++ src/rpc/client.cpp | 2 ++ src/rpc/mempool.cpp | 33 ++++++++++++++++++++++++++--- src/test/fuzz/package_eval.cpp | 2 +- src/test/fuzz/tx_pool.cpp | 2 +- src/test/txpackage_tests.cpp | 36 ++++++++++++++++---------------- src/validation.cpp | 37 ++++++++++++++++++++++++++++----- src/validation.h | 6 ++++-- test/functional/rpc_packages.py | 30 ++++++++++++++++++++++++++ 9 files changed, 124 insertions(+), 30 deletions(-) diff --git a/src/node/transaction.h b/src/node/transaction.h index 168273594ce50..6782536aceba2 100644 --- a/src/node/transaction.h +++ b/src/node/transaction.h @@ -26,6 +26,12 @@ struct NodeContext; */ static const CFeeRate DEFAULT_MAX_RAW_TX_FEE_RATE{COIN / 10}; +/** Maximum burn value for sendrawtransaction, submitpackage, and testmempoolaccept RPC calls. + * By default, a transaction with a burn value higher than this will be rejected + * by these RPCs and the GUI. This can be overridden with the maxburnamount argument. + */ +static const CAmount DEFAULT_MAX_BURN_AMOUNT{0}; + /** * Submit a transaction to the mempool and (optionally) relay it to all P2P peers. * diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 5825efdf82ea1..eb05f33b42a4b 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -128,6 +128,8 @@ static const CRPCConvertParam vRPCConvertParams[] = { "testmempoolaccept", 0, "rawtxs" }, { "testmempoolaccept", 1, "maxfeerate" }, { "submitpackage", 0, "package" }, + { "submitpackage", 1, "maxfeerate" }, + { "submitpackage", 2, "maxburnamount" }, { "combinerawtransaction", 0, "txs" }, { "fundrawtransaction", 1, "options" }, { "fundrawtransaction", 1, "add_inputs"}, diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 25bfec2d45e21..8539506f2f751 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -28,6 +28,7 @@ using kernel::DumpMempool; +using node::DEFAULT_MAX_BURN_AMOUNT; using node::DEFAULT_MAX_RAW_TX_FEE_RATE; using node::MempoolPath; using node::NodeContext; @@ -46,7 +47,7 @@ static RPCHelpMan sendrawtransaction() {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())}, "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + "/kvB.\nFee rates larger than 1BTC/kvB are rejected.\nSet to 0 to accept any fee rate."}, - {"maxburnamount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(0)}, + {"maxburnamount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_BURN_AMOUNT)}, "Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n" "If burning funds through unspendable outputs is desired, increase this value.\n" "This check is based on heuristics and does not guarantee spendability of outputs.\n"}, @@ -180,7 +181,7 @@ static RPCHelpMan testmempoolaccept() Chainstate& chainstate = chainman.ActiveChainstate(); const PackageMempoolAcceptResult package_result = [&] { LOCK(::cs_main); - if (txns.size() > 1) return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/true); + if (txns.size() > 1) return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/true, /*max_sane_feerate=*/{}); return PackageMempoolAcceptResult(txns[0]->GetWitnessHash(), chainman.ProcessTransaction(txns[0], /*test_accept=*/true)); }(); @@ -823,6 +824,14 @@ static RPCHelpMan submitpackage() {"rawtx", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, ""}, }, }, + {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())}, + "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + + "/kvB.\nFee rates larger than 1BTC/kvB are rejected.\nSet to 0 to accept any fee rate."}, + {"maxburnamount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_BURN_AMOUNT)}, + "Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n" + "If burning funds through unspendable outputs is desired, increase this value.\n" + "This check is based on heuristics and does not guarantee spendability of outputs.\n" + }, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -862,6 +871,17 @@ static RPCHelpMan submitpackage() "Array must contain between 1 and " + ToString(MAX_PACKAGE_COUNT) + " transactions."); } + // Fee check needs to be run with chainstate and package context + const CFeeRate max_raw_tx_fee_rate = ParseFeeRate(self.Arg(1)); + std::optional max_sane_feerate{max_raw_tx_fee_rate}; + // 0-value is special; it's mapped to no sanity check + if (max_raw_tx_fee_rate == CFeeRate(0)) { + max_sane_feerate = std::nullopt; + } + + // Burn sanity check is run with no context + const CAmount max_burn_amount = request.params[2].isNull() ? 0 : AmountFromValue(request.params[2]); + std::vector txns; txns.reserve(raw_transactions.size()); for (const auto& rawtx : raw_transactions.getValues()) { @@ -870,6 +890,13 @@ static RPCHelpMan submitpackage() throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed: " + rawtx.get_str() + " Make sure the tx has at least one input."); } + + for (const auto& out : mtx.vout) { + if((out.scriptPubKey.IsUnspendable() || !out.scriptPubKey.HasValidOps()) && out.nValue > max_burn_amount) { + throw JSONRPCTransactionError(TransactionError::MAX_BURN_EXCEEDED); + } + } + txns.emplace_back(MakeTransactionRef(std::move(mtx))); } if (!IsChildWithParentsTree(txns)) { @@ -879,7 +906,7 @@ static RPCHelpMan submitpackage() NodeContext& node = EnsureAnyNodeContext(request.context); CTxMemPool& mempool = EnsureMemPool(node); Chainstate& chainstate = EnsureChainman(node).ActiveChainstate(); - const auto package_result = WITH_LOCK(::cs_main, return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/ false)); + const auto package_result = WITH_LOCK(::cs_main, return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/ false, max_sane_feerate)); std::string package_msg = "success"; diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 9e658e0ceda9b..a16dbbe8ca6af 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -277,7 +277,7 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) auto single_submit = txs.size() == 1 && fuzzed_data_provider.ConsumeBool(); const auto result_package = WITH_LOCK(::cs_main, - return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/single_submit)); + return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/single_submit, /*max_sane_feerate=*/{})); // Always set bypass_limits to false because it is not supported in ProcessNewPackage and // can be a source of divergence. diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index fcf230642a61c..69f3cc22f6062 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -291,7 +291,7 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool) // Make sure ProcessNewPackage on one transaction works. // The result is not guaranteed to be the same as what is returned by ATMP. const auto result_package = WITH_LOCK(::cs_main, - return ProcessNewPackage(chainstate, tx_pool, {tx}, true)); + return ProcessNewPackage(chainstate, tx_pool, {tx}, true, /*max_sane_feerate=*/{})); // If something went wrong due to a package-specific policy, it might not return a // validation result for the transaction. if (result_package.m_state.GetResult() != PackageValidationResult::PCKG_POLICY) { diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index f6456526bb054..eb131dc6bba8f 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -132,7 +132,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) /*output_amount=*/CAmount(48 * COIN), /*submit=*/false); CTransactionRef tx_child = MakeTransactionRef(mtx_child); Package package_parent_child{tx_parent, tx_child}; - const auto result_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_parent_child, /*test_accept=*/true); + const auto result_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_parent_child, /*test_accept=*/true, /*max_sane_feerate=*/{}); if (auto err_parent_child{CheckPackageMempoolAcceptResult(package_parent_child, result_parent_child, /*expect_valid=*/true, nullptr)}) { BOOST_ERROR(err_parent_child.value()); } else { @@ -151,7 +151,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup) CTransactionRef giant_ptx = create_placeholder_tx(999, 999); BOOST_CHECK(GetVirtualTransactionSize(*giant_ptx) > DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1000); Package package_single_giant{giant_ptx}; - auto result_single_large = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_single_giant, /*test_accept=*/true); + auto result_single_large = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_single_giant, /*test_accept=*/true, /*max_sane_feerate=*/{}); if (auto err_single_large{CheckPackageMempoolAcceptResult(package_single_giant, result_single_large, /*expect_valid=*/false, nullptr)}) { BOOST_ERROR(err_single_large.value()); } else { @@ -275,7 +275,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) package_unrelated.emplace_back(MakeTransactionRef(mtx)); } auto result_unrelated_submit = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_unrelated, /*test_accept=*/false); + package_unrelated, /*test_accept=*/false, /*max_sane_feerate=*/{}); // We don't expect m_tx_results for each transaction when basic sanity checks haven't passed. BOOST_CHECK(result_unrelated_submit.m_state.IsInvalid()); BOOST_CHECK_EQUAL(result_unrelated_submit.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); @@ -315,7 +315,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) // 3 Generations is not allowed. { auto result_3gen_submit = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_3gen, /*test_accept=*/false); + package_3gen, /*test_accept=*/false, /*max_sane_feerate=*/{}); BOOST_CHECK(result_3gen_submit.m_state.IsInvalid()); 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"); @@ -332,7 +332,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) CTransactionRef tx_parent_invalid = MakeTransactionRef(mtx_parent_invalid); Package package_invalid_parent{tx_parent_invalid, tx_child}; auto result_quit_early = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_invalid_parent, /*test_accept=*/ false); + package_invalid_parent, /*test_accept=*/ false, /*max_sane_feerate=*/{}); if (auto err_parent_invalid{CheckPackageMempoolAcceptResult(package_invalid_parent, result_quit_early, /*expect_valid=*/false, m_node.mempool.get())}) { BOOST_ERROR(err_parent_invalid.value()); } else { @@ -353,7 +353,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) package_missing_parent.push_back(MakeTransactionRef(mtx_child)); { const auto result_missing_parent = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_missing_parent, /*test_accept=*/false); + package_missing_parent, /*test_accept=*/false, /*max_sane_feerate=*/{}); BOOST_CHECK(result_missing_parent.m_state.IsInvalid()); 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"); @@ -363,7 +363,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) // Submit package with parent + child. { const auto submit_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_parent_child, /*test_accept=*/false); + package_parent_child, /*test_accept=*/false, /*max_sane_feerate=*/{}); expected_pool_size += 2; BOOST_CHECK_MESSAGE(submit_parent_child.m_state.IsValid(), "Package validation unexpectedly failed: " << submit_parent_child.m_state.GetRejectReason()); @@ -385,7 +385,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup) // Already-in-mempool transactions should be detected and de-duplicated. { const auto submit_deduped = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_parent_child, /*test_accept=*/false); + package_parent_child, /*test_accept=*/false, /*max_sane_feerate=*/{}); if (auto err_deduped{CheckPackageMempoolAcceptResult(package_parent_child, submit_deduped, /*expect_valid=*/true, m_node.mempool.get())}) { BOOST_ERROR(err_deduped.value()); } else { @@ -456,7 +456,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) { Package package_parent_child1{ptx_parent, ptx_child1}; const auto submit_witness1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_parent_child1, /*test_accept=*/false); + package_parent_child1, /*test_accept=*/false, /*max_sane_feerate=*/{}); if (auto err_witness1{CheckPackageMempoolAcceptResult(package_parent_child1, submit_witness1, /*expect_valid=*/true, m_node.mempool.get())}) { BOOST_ERROR(err_witness1.value()); } @@ -464,7 +464,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) // Child2 would have been validated individually. Package package_parent_child2{ptx_parent, ptx_child2}; const auto submit_witness2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_parent_child2, /*test_accept=*/false); + package_parent_child2, /*test_accept=*/false, /*max_sane_feerate=*/{}); if (auto err_witness2{CheckPackageMempoolAcceptResult(package_parent_child2, submit_witness2, /*expect_valid=*/true, m_node.mempool.get())}) { BOOST_ERROR(err_witness2.value()); } else { @@ -478,7 +478,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) // Deduplication should work when wtxid != txid. Submit package with the already-in-mempool // transactions again, which should not fail. const auto submit_segwit_dedup = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_parent_child1, /*test_accept=*/false); + package_parent_child1, /*test_accept=*/false, /*max_sane_feerate=*/{}); if (auto err_segwit_dedup{CheckPackageMempoolAcceptResult(package_parent_child1, submit_segwit_dedup, /*expect_valid=*/true, m_node.mempool.get())}) { BOOST_ERROR(err_segwit_dedup.value()); } else { @@ -508,7 +508,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) { Package package_child2_grandchild{ptx_child2, ptx_grandchild}; const auto submit_spend_ignored = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_child2_grandchild, /*test_accept=*/false); + package_child2_grandchild, /*test_accept=*/false, /*max_sane_feerate=*/{}); if (auto err_spend_ignored{CheckPackageMempoolAcceptResult(package_child2_grandchild, submit_spend_ignored, /*expect_valid=*/true, m_node.mempool.get())}) { BOOST_ERROR(err_spend_ignored.value()); } else { @@ -606,7 +606,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup) // parent3 should be accepted // child should be accepted { - const auto mixed_result = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_mixed, false); + const auto mixed_result = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_mixed, false, /*max_sane_feerate=*/{}); if (auto err_mixed{CheckPackageMempoolAcceptResult(package_mixed, mixed_result, /*expect_valid=*/true, m_node.mempool.get())}) { BOOST_ERROR(err_mixed.value()); } else { @@ -670,7 +670,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); + package_cpfp, /*test_accept=*/ false, /*max_sane_feerate=*/{}); if (auto err_cpfp_deprio{CheckPackageMempoolAcceptResult(package_cpfp, submit_cpfp_deprio, /*expect_valid=*/false, m_node.mempool.get())}) { BOOST_ERROR(err_cpfp_deprio.value()); } else { @@ -692,7 +692,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) { BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); const auto submit_cpfp = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_cpfp, /*test_accept=*/ false); + package_cpfp, /*test_accept=*/ false, /*max_sane_feerate=*/{}); if (auto err_cpfp{CheckPackageMempoolAcceptResult(package_cpfp, submit_cpfp, /*expect_valid=*/true, m_node.mempool.get())}) { BOOST_ERROR(err_cpfp.value()); } else { @@ -744,7 +744,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) // Cheap package should fail for being too low fee. { const auto submit_package_too_low = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_still_too_low, /*test_accept=*/false); + package_still_too_low, /*test_accept=*/false, /*max_sane_feerate=*/{}); if (auto err_package_too_low{CheckPackageMempoolAcceptResult(package_still_too_low, submit_package_too_low, /*expect_valid=*/false, m_node.mempool.get())}) { BOOST_ERROR(err_package_too_low.value()); } else { @@ -770,7 +770,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) // Now that the child's fees have "increased" by 1 BTC, the cheap package should succeed. { const auto submit_prioritised_package = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_still_too_low, /*test_accept=*/false); + package_still_too_low, /*test_accept=*/false, /*max_sane_feerate=*/{}); if (auto err_prioritised{CheckPackageMempoolAcceptResult(package_still_too_low, submit_prioritised_package, /*expect_valid=*/true, m_node.mempool.get())}) { BOOST_ERROR(err_prioritised.value()); } else { @@ -818,7 +818,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup) { BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); const auto submit_rich_parent = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, - package_rich_parent, /*test_accept=*/false); + package_rich_parent, /*test_accept=*/false, /*max_sane_feerate=*/{}); if (auto err_rich_parent{CheckPackageMempoolAcceptResult(package_rich_parent, submit_rich_parent, /*expect_valid=*/false, m_node.mempool.get())}) { BOOST_ERROR(err_rich_parent.value()); } else { diff --git a/src/validation.cpp b/src/validation.cpp index 81a3c35864995..428195663a48a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -472,6 +472,11 @@ class MemPoolAccept * policies such as mempool min fee and min relay fee. */ const bool m_package_feerates; + /** Used for local submission of transactions to catch "absurd" fees + * due to fee miscalculation by wallets. std:nullopt implies unset, allowing any feerates. + * Any individual transaction failing this check causes immediate failure. + */ + const std::optional m_client_maxfeerate; /** Parameters for single transaction mempool validation. */ static ATMPArgs SingleAccept(const CChainParams& chainparams, int64_t accept_time, @@ -485,6 +490,7 @@ class MemPoolAccept /* m_allow_replacement */ true, /* m_package_submission */ false, /* m_package_feerates */ false, + /* m_client_maxfeerate */ {}, // checked by caller }; } @@ -499,12 +505,13 @@ class MemPoolAccept /* m_allow_replacement */ false, /* m_package_submission */ false, // not submitting to mempool /* m_package_feerates */ false, + /* m_client_maxfeerate */ {}, // checked by caller }; } /** Parameters for child-with-unconfirmed-parents package validation. */ static ATMPArgs PackageChildWithParents(const CChainParams& chainparams, int64_t accept_time, - std::vector& coins_to_uncache) { + std::vector& coins_to_uncache, std::optional& client_maxfeerate) { return ATMPArgs{/* m_chainparams */ chainparams, /* m_accept_time */ accept_time, /* m_bypass_limits */ false, @@ -513,6 +520,7 @@ class MemPoolAccept /* m_allow_replacement */ false, /* m_package_submission */ true, /* m_package_feerates */ true, + /* m_client_maxfeerate */ client_maxfeerate, }; } @@ -526,6 +534,7 @@ class MemPoolAccept /* m_allow_replacement */ true, /* m_package_submission */ true, // do not LimitMempoolSize in Finalize() /* m_package_feerates */ false, // only 1 transaction + /* m_client_maxfeerate */ package_args.m_client_maxfeerate, }; } @@ -539,7 +548,8 @@ class MemPoolAccept bool test_accept, bool allow_replacement, bool package_submission, - bool package_feerates) + bool package_feerates, + std::optional client_maxfeerate) : m_chainparams{chainparams}, m_accept_time{accept_time}, m_bypass_limits{bypass_limits}, @@ -547,7 +557,8 @@ class MemPoolAccept m_test_accept{test_accept}, m_allow_replacement{allow_replacement}, m_package_submission{package_submission}, - m_package_feerates{package_feerates} + m_package_feerates{package_feerates}, + m_client_maxfeerate{client_maxfeerate} { } }; @@ -1255,6 +1266,12 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef return MempoolAcceptResult::Failure(ws.m_state); } + // Individual modified feerate exceeded caller-defined max; abort + if (args.m_client_maxfeerate && CFeeRate(ws.m_modified_fees, ws.m_vsize) > args.m_client_maxfeerate.value()) { + ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "max feerate exceeded", ""); + return MempoolAcceptResult::Failure(ws.m_state); + } + if (m_rbf && !ReplacementChecks(ws)) return MempoolAcceptResult::Failure(ws.m_state); // Perform the inexpensive checks first and avoid hashing and signature verification unless @@ -1313,6 +1330,16 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); return PackageMempoolAcceptResult(package_state, std::move(results)); } + + // Individual modified feerate exceeded caller-defined max; abort + // N.B. this doesn't take into account CPFPs. Chunk-aware validation may be more robust. + if (args.m_client_maxfeerate && CFeeRate(ws.m_modified_fees, ws.m_vsize) > args.m_client_maxfeerate.value()) { + package_state.Invalid(PackageValidationResult::PCKG_TX, "max feerate exceeded"); + // Exit early to avoid doing pointless work. Update the failed tx result; the rest are unfinished. + results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); + return PackageMempoolAcceptResult(package_state, std::move(results)); + } + // Make the coins created by this transaction available for subsequent transactions in the // package to spend. Since we already checked conflicts in the package and we don't allow // replacements, we don't need to track the coins spent. Note that this logic will need to be @@ -1657,7 +1684,7 @@ MempoolAcceptResult AcceptToMemoryPool(Chainstate& active_chainstate, const CTra } PackageMempoolAcceptResult ProcessNewPackage(Chainstate& active_chainstate, CTxMemPool& pool, - const Package& package, bool test_accept) + const Package& package, bool test_accept, std::optional client_maxfeerate) { AssertLockHeld(cs_main); assert(!package.empty()); @@ -1671,7 +1698,7 @@ PackageMempoolAcceptResult ProcessNewPackage(Chainstate& active_chainstate, CTxM auto args = MemPoolAccept::ATMPArgs::PackageTestAccept(chainparams, GetTime(), coins_to_uncache); return MemPoolAccept(pool, active_chainstate).AcceptMultipleTransactions(package, args); } else { - auto args = MemPoolAccept::ATMPArgs::PackageChildWithParents(chainparams, GetTime(), coins_to_uncache); + auto args = MemPoolAccept::ATMPArgs::PackageChildWithParents(chainparams, GetTime(), coins_to_uncache, client_maxfeerate); return MemPoolAccept(pool, active_chainstate).AcceptPackage(package, args); } }(); diff --git a/src/validation.h b/src/validation.h index 94765bfbcd8a6..0746319b23f62 100644 --- a/src/validation.h +++ b/src/validation.h @@ -274,13 +274,15 @@ MempoolAcceptResult AcceptToMemoryPool(Chainstate& active_chainstate, const CTra /** * Validate (and maybe submit) a package to the mempool. See doc/policy/packages.md for full details * on package validation rules. -* @param[in] test_accept When true, run validation checks but don't submit to mempool. +* @param[in] test_accept When true, run validation checks but don't submit to mempool. +* @param[in] max_sane_feerate If exceeded by an individual transaction, rest of (sub)package evalution is aborted. +* Only for sanity checks against local submission of transactions. * @returns a PackageMempoolAcceptResult which includes a MempoolAcceptResult for each transaction. * If a transaction fails, validation will exit early and some results may be missing. It is also * possible for the package to be partially submitted. */ PackageMempoolAcceptResult ProcessNewPackage(Chainstate& active_chainstate, CTxMemPool& pool, - const Package& txns, bool test_accept) + const Package& txns, bool test_accept, std::optional max_sane_feerate) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /* Mempool validation helper functions */ diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index 664f2df3f19a0..64cb872faa5bb 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -81,6 +81,7 @@ def run_test(self): self.test_conflicting() self.test_rbf() self.test_submitpackage() + self.test_maxfeerate_maxburn_submitpackage() def test_independent(self, coin): self.log.info("Test multiple independent transactions in a package") @@ -356,5 +357,34 @@ def test_submitpackage(self): assert_equal(res["tx-results"][sec_wtxid]["error"], "version") peer.wait_for_broadcast([first_wtxid]) + def test_maxfeerate_maxburn_submitpackage(self): + node = self.nodes[0] + # clear mempool + deterministic_address = node.get_deterministic_priv_key().address + self.generatetoaddress(node, 1, deterministic_address) + + self.log.info("Submitpackage maxfeerate arg testing") + chained_txns = self.wallet.create_self_transfer_chain(chain_length=2) + minrate_btc_kvb = min([chained_txn["fee"] / chained_txn["tx"].get_vsize() * 1000 for chained_txn in chained_txns]) + chain_hex = [t["hex"] for t in chained_txns] + pkg_result = node.submitpackage(chain_hex, maxfeerate=minrate_btc_kvb - Decimal("0.00000001")) + assert_equal(pkg_result["tx-results"][chained_txns[0]["wtxid"]]["error"], "max feerate exceeded") + assert_equal(pkg_result["tx-results"][chained_txns[1]["wtxid"]]["error"], "bad-txns-inputs-missingorspent") + assert_equal(node.getrawmempool(), []) + + self.log.info("Submitpackage maxburnamount arg testing") + tx = tx_from_hex(chain_hex[1]) + tx.vout[-1].scriptPubKey = b'a' * 10001 # scriptPubKey bigger than 10k IsUnspendable + chain_hex = [chain_hex[0], tx.serialize().hex()] + # burn test is run before any package evaluation; nothing makes it in and we get broader exception + assert_raises_rpc_error(-25, "Unspendable output exceeds maximum configured by user", node.submitpackage, chain_hex, 0, chained_txns[1]["new_utxo"]["value"] - Decimal("0.00000001")) + assert_equal(node.getrawmempool(), []) + + # Relax the restrictions for both and send it; parent gets through as own subpackage + pkg_result = node.submitpackage(chain_hex, maxfeerate=minrate_btc_kvb, maxburnamount=chained_txns[1]["new_utxo"]["value"]) + assert "error" not in pkg_result["tx-results"][chained_txns[0]["wtxid"]] + assert_equal(pkg_result["tx-results"][tx.getwtxid()]["error"], "scriptpubkey") + assert_equal(node.getrawmempool(), [chained_txns[0]["txid"]]) + if __name__ == "__main__": RPCPackagesTest().main()