From 845690761a48b9a2e2934af6fbfb8885f51e202d Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 23 Jan 2024 18:16:48 +0000 Subject: [PATCH] partial bitcoin#20833: enable packages through testmempoolaccept excludes: - c9e1a26d1f17c8b98632b7796ffa8f8788b5a83c (will be added in future fuzzing PR) inapplicable: - 249f43f3cc52b0ffdf2c47aad95ba9d195f6a45e (we don't have RBF) --- doc/release-notes-5834.md | 8 + src/Makefile.am | 1 + src/policy/packages.h | 34 ++++ src/rpc/rawtransaction.cpp | 127 +++++++----- src/test/miner_tests.cpp | 3 +- src/test/txvalidation_tests.cpp | 98 ++++++++++ src/test/util/setup_common.cpp | 7 +- src/test/util/setup_common.h | 4 +- src/txmempool.cpp | 18 +- src/txmempool.h | 10 +- src/validation.cpp | 158 +++++++++++++-- src/validation.h | 64 +++++- test/functional/mempool_accept.py | 3 +- test/functional/rpc_packages.py | 311 ++++++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 15 files changed, 763 insertions(+), 84 deletions(-) create mode 100644 src/policy/packages.h create mode 100755 test/functional/rpc_packages.py diff --git a/doc/release-notes-5834.md b/doc/release-notes-5834.md index fb5f652de683cb..b405071e803950 100644 --- a/doc/release-notes-5834.md +++ b/doc/release-notes-5834.md @@ -3,3 +3,11 @@ Updated RPCs - The `testmempoolaccept` RPC returns `vsize` and a `fee` object with the `base` fee if the transaction passes validation. +- The `testmempoolaccept` RPC now accepts multiple transactions (still experimental at the moment, + API may be unstable). This is intended for testing transaction packages with dependency + relationships; it is not recommended for batch-validating independent transactions. In addition to + mempool policy, package policies apply: the list cannot contain more than 25 transactions or have a + total size exceeding 101K virtual bytes, and cannot conflict with (spend the same inputs as) each other or + the mempool. There are some known limitations to the accuracy of the test accept: it's possible for + `testmempoolaccept` to return "allowed"=True for a group of transactions, but "too-long-mempool-chain" + if they are actually submitted. diff --git a/src/Makefile.am b/src/Makefile.am index 97abc0ec906661..8f0a279423e9bd 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -275,6 +275,7 @@ BITCOIN_CORE_H = \ outputtype.h \ policy/feerate.h \ policy/fees.h \ + policy/packages.h \ policy/policy.h \ policy/settings.h \ pow.h \ diff --git a/src/policy/packages.h b/src/policy/packages.h new file mode 100644 index 00000000000000..4b1463dcb34ebc --- /dev/null +++ b/src/policy/packages.h @@ -0,0 +1,34 @@ +// Copyright (c) 2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_POLICY_PACKAGES_H +#define BITCOIN_POLICY_PACKAGES_H + +#include +#include + +#include + +/** Default maximum number of transactions in a package. */ +static constexpr uint32_t MAX_PACKAGE_COUNT{25}; +/** Default maximum total virtual size of transactions in a package in KvB. */ +static constexpr uint32_t MAX_PACKAGE_SIZE{101}; + +/** A "reason" why a package was invalid. It may be that one or more of the included + * transactions is invalid or the package itself violates our rules. + * We don't distinguish between consensus and policy violations right now. + */ +enum class PackageValidationResult { + PCKG_RESULT_UNSET = 0, //!< Initial value. The package has not yet been rejected. + PCKG_POLICY, //!< The package itself is invalid (e.g. too many transactions). + PCKG_TX, //!< At least one tx is invalid. +}; + +/** A package is an ordered list of transactions. The transactions cannot conflict with (spend the + * same inputs as) one another. */ +using Package = std::vector; + +class PackageValidationState : public ValidationState {}; + +#endif // BITCOIN_POLICY_PACKAGES_H diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index b4a44f55412eb9..f04ad8ed9449b5 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -1054,7 +1055,10 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) { RPCHelpMan{"testmempoolaccept", "\nReturns result of mempool acceptance tests indicating if raw transaction (serialized, hex-encoded) would be accepted by mempool.\n" - "\nThis checks if the transaction violates the consensus or policy rules.\n" + "\nIf multiple transactions are passed in, parents must come before children and package policies apply: the transactions cannot conflict with any mempool transactions or each other.\n" + "\nIf one transaction fails, other transactions may not be fully validated (the 'allowed' key will be blank).\n" + "\nThe maximum number of transactions allowed is 25 (MAX_PACKAGE_COUNT)\n" + "\nThis checks if transactions violate the consensus or policy rules.\n" "\nSee sendrawtransaction call.\n", { {"rawtxs", RPCArg::Type::ARR, RPCArg::Optional::NO, "An array of hex strings of raw transactions.", @@ -1062,17 +1066,21 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) {"rawtx", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, ""}, }, }, - {"maxfeerate", RPCArg::Type::AMOUNT, /* default */ FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK()), "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + "/kB\n"}, + {"maxfeerate", RPCArg::Type::AMOUNT, /* default */ FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK()), + "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + "/kB\n"}, }, RPCResult{ RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n" - "Length is exactly one for now.", + "Returns results for each transaction in the same order they were passed in.\n" + "It is possible for transactions to not be fully validated ('allowed' unset) if an earlier transaction failed.\n", { {RPCResult::Type::OBJ, "", "", { {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"}, - {RPCResult::Type::BOOL, "allowed", "If the mempool allows this tx to be inserted"}, - {RPCResult::Type::NUM, "vsize", "Virtual transaction size as defined in BIP 141"}, + {RPCResult::Type::STR, "package-error", "Package validation error, if any (only possible if rawtxs had more than 1 transaction)."}, + {RPCResult::Type::BOOL, "allowed", "Whether this tx would be accepted to the mempool and pass client-specified maxfeerate." + "If not present, the tx was not fully validated due to a failure in another tx in the list."}, + {RPCResult::Type::NUM, "vsize", "Virtual transaction size as defined in BIP 141."}, {RPCResult::Type::OBJ, "fees", "Transaction fees (only present if 'allowed' is true)", { {RPCResult::Type::STR_AMOUNT, "base", "transaction fee in " + CURRENCY_UNIT}, @@ -1098,62 +1106,85 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request) UniValueType(), // VNUM or VSTR, checked inside AmountFromValue() }); - if (request.params[0].get_array().size() != 1) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Array must contain exactly one raw transaction for now"); - } - - CMutableTransaction mtx; - if (!DecodeHexTx(mtx, request.params[0].get_array()[0].get_str())) { - throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); + const UniValue raw_transactions = request.params[0].get_array(); + if (raw_transactions.size() < 1 || raw_transactions.size() > MAX_PACKAGE_COUNT) { + throw JSONRPCError(RPC_INVALID_PARAMETER, + "Array must contain between 1 and " + ToString(MAX_PACKAGE_COUNT) + " transactions."); } - CTransactionRef tx(MakeTransactionRef(std::move(mtx))); - const uint256& tx_hash = tx->GetHash(); const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull() ? DEFAULT_MAX_RAW_TX_FEE_RATE : CFeeRate(AmountFromValue(request.params[1])); - const NodeContext& node = EnsureAnyNodeContext(request.context); + std::vector txns; + for (const auto& rawtx : raw_transactions.getValues()) { + CMutableTransaction mtx; + if (!DecodeHexTx(mtx, rawtx.get_str())) { + throw JSONRPCError(RPC_DESERIALIZATION_ERROR, + "TX decode failed: " + rawtx.get_str() + " Make sure the tx has at least one input."); + } + txns.emplace_back(MakeTransactionRef(std::move(mtx))); + } + NodeContext& node = EnsureAnyNodeContext(request.context); CTxMemPool& mempool = EnsureMemPool(node); - int64_t virtual_size = GetVirtualTransactionSize(*tx); - CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size); - - UniValue result(UniValue::VARR); - UniValue result_0(UniValue::VOBJ); - result_0.pushKV("txid", tx_hash.GetHex()); + CChainState& chainstate = EnsureChainman(node).ActiveChainstate(); + const PackageMempoolAcceptResult package_result = [&] { + LOCK(::cs_main); + if (txns.size() > 1) return ProcessNewPackage(chainstate, mempool, txns, /* test_accept */ true); + return PackageMempoolAcceptResult(txns[0]->GetHash(), + AcceptToMemoryPool(chainstate, mempool, txns[0], /* bypass_limits */ false, /* test_accept*/ true)); + }(); - ChainstateManager& chainman = EnsureChainman(node); - const MempoolAcceptResult accept_result = WITH_LOCK(cs_main, return AcceptToMemoryPool(chainman.ActiveChainstate(), mempool, std::move(tx), - false /* bypass_limits */, true /* test_accept */)); - - // Only return the fee and vsize if the transaction would pass ATMP. - // These can be used to calculate the feerate. - if (accept_result.m_result_type == MempoolAcceptResult::ResultType::VALID) { - const CAmount fee = accept_result.m_base_fees.value(); - // Check that fee does not exceed maximum fee - if (max_raw_tx_fee && fee > max_raw_tx_fee) { - result_0.pushKV("allowed", false); - result_0.pushKV("reject-reason", "max-fee-exceeded"); - } else { - result_0.pushKV("allowed", true); - result_0.pushKV("vsize", virtual_size); - UniValue fees(UniValue::VOBJ); - fees.pushKV("base", ValueFromAmount(fee)); - result_0.pushKV("fees", fees); + UniValue rpc_result(UniValue::VARR); + // We will check transaction fees we iterate through txns in order. If any transaction fee + // exceeds maxfeerate, we will keave the rest of the validation results blank, because it + // doesn't make sense to return a validation result for a transaction if its ancestor(s) would + // not be submitted. + bool exit_early{false}; + for (const auto& tx : txns) { + UniValue result_inner(UniValue::VOBJ); + result_inner.pushKV("txid", tx->GetHash().GetHex()); + if (package_result.m_state.GetResult() == PackageValidationResult::PCKG_POLICY) { + result_inner.pushKV("package-error", package_result.m_state.GetRejectReason()); } - result.push_back(std::move(result_0)); - } else { - result_0.pushKV("allowed", false); - const TxValidationState state = accept_result.m_state; - if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { - result_0.pushKV("reject-reason", "missing-inputs"); + auto it = package_result.m_tx_results.find(tx->GetHash()); + if (exit_early || it == package_result.m_tx_results.end()) { + // Validation unfinished. Just return the txid. + rpc_result.push_back(result_inner); + continue; + } + const auto& tx_result = it->second; + if (tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID) { + const CAmount fee = tx_result.m_base_fees.value(); + // Check that fee does not exceed maximum fee + const int64_t virtual_size = GetVirtualTransactionSize(*tx); + const CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size); + if (max_raw_tx_fee && fee > max_raw_tx_fee) { + result_inner.pushKV("allowed", false); + result_inner.pushKV("reject-reason", "max-fee-exceeded"); + exit_early = true; + } else { + // Only return the fee and vsize if the transaction would pass ATMP. + // These can be used to calculate the feerate. + result_inner.pushKV("allowed", true); + result_inner.pushKV("vsize", virtual_size); + UniValue fees(UniValue::VOBJ); + fees.pushKV("base", ValueFromAmount(fee)); + result_inner.pushKV("fees", fees); + } } else { - result_0.pushKV("reject-reason", state.GetRejectReason()); + result_inner.pushKV("allowed", false); + const TxValidationState state = tx_result.m_state; + if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { + result_inner.pushKV("reject-reason", "missing-inputs"); + } else { + result_inner.pushKV("reject-reason", state.GetRejectReason()); + } } - result.push_back(std::move(result_0)); + rpc_result.push_back(result_inner); } - return result; + return rpc_result; } UniValue decodepsbt(const JSONRPCRequest& request) diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 0c8af463a27df3..630c5dbf27abfc 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -35,7 +35,8 @@ struct MinerTestingSetup : public TestingSetup { void TestPackageSelection(const CChainParams& chainparams, const CScript& scriptPubKey, const std::vector& txFirst) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_node.mempool->cs); bool TestSequenceLocks(const CTransaction& tx, int flags) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_node.mempool->cs) { - return CheckSequenceLocks(::ChainstateActive(), *m_node.mempool, tx, flags); + CCoinsViewMemPool viewMempool(&m_node.chainman->ActiveChainstate().CoinsTip(), *m_node.mempool); + return CheckSequenceLocks(m_node.chainman->ActiveChain().Tip(), viewMempool, tx, flags); } BlockAssembler AssemblerForTest(const CChainParams& params); }; diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index 326ec4466e1bb2..ac6f8b91a7d73b 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -3,8 +3,12 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include +#include +#include #include #include