diff options
author | W. J. van der Laan <laanwj@protonmail.com> | 2021-05-27 22:19:05 +0200 |
---|---|---|
committer | W. J. van der Laan <laanwj@protonmail.com> | 2021-05-27 22:40:24 +0200 |
commit | 7257e50dba36328be60f69c998632408802b9a29 (patch) | |
tree | a8c1f31bf1c2707cf2360fcaeb6bdf4b8639fc16 | |
parent | 2e8f3928f1dd4e33a51e5e76b3c254e85097fc90 (diff) | |
parent | 13650fe2e527bf0cf5d977bf5f3f1563b853ecdc (diff) |
Merge bitcoin/bitcoin#20833: rpc/validation: enable packages through testmempoolaccept
13650fe2e527bf0cf5d977bf5f3f1563b853ecdc [policy] detect unsorted packages (glozow)
9ef643e21b44f99f4bce54077788d0ad4d81f7cd [doc] add release note for package testmempoolaccept (glozow)
c4259f4b7ee23ef6e0ec82c5d5b9dfa9cadd5bed [test] functional test for packages in RPCs (glozow)
9ede34a6f20378e86c5289ebd20dd394a5915123 [rpc] allow multiple txns in testmempoolaccept (glozow)
ae8e6df709ff3d52b8e9918e09cacb64f83ae379 [policy] limit package sizes (glozow)
c9e1a26d1f17c8b98632b7796ffa8f8788b5a83c [fuzz] add ProcessNewPackage call in tx_pool fuzzer (glozow)
363e3d916cc036488783bb4bdcfdd3665aecf711 [test] unit tests for ProcessNewPackage (glozow)
cd9a11ac96c01e200d0086b2f011f4a614f5a705 [test] make submit optional in CreateValidMempoolTransaction (glozow)
2ef187941db439c5b3e529f08b6ab153ff061fc5 [validation] package validation for test accepts (glozow)
578148ded62828a9820398165c41670f4dbb523d [validation] explicit Success/Failure ctors for MempoolAcceptResult (glozow)
b88d77aec5e7bef5305a668d15031351c0548b4d [policy] Define packages (glozow)
249f43f3cc52b0ffdf2c47aad95ba9d195f6a45e [refactor] add option to disable RBF (glozow)
897e348f5987eadd8559981a973c045c471b3ad8 [coins/mempool] extend CCoinsViewMemPool to track temporary coins (glozow)
42cf8b25df07c45562b7210e0e15c3fd5edb2c11 [validation] make CheckSequenceLocks context-free (glozow)
Pull request description:
This PR enables validation dry-runs of packages through the `testmempoolaccept` RPC. The expectation is that the results returned from `testmempoolaccept` are what you'd get from test-then-submitting each transaction individually, in that order (this means the package is expected to be sorted in topological order, for now at least). The validation is also atomic: in the case of failure, it immediately halts and may return "unfinished" `MempoolAcceptResult`s for transactions that weren't fully validated. The API for 1 transaction stays the same.
**Motivation:**
- This allows you to test validity for transaction chains (e.g. with multiple spending paths and where you don't want to broadcast yet); closes #18480.
- It's also a first step towards package validation in a minimally invasive way.
- The RPC commit happens to close #21074 by clarifying the "allowed" key.
There are a few added restrictions on the packages, mostly to simplify the logic for areas that aren't critical to main package use cases:
- No package can have conflicts, i.e. none of them can spend the same inputs, even if it would be a valid BIP125 replacement.
- The package cannot conflict with the mempool, i.e. RBF is disabled.
- The total count of the package cannot exceed 25 (the default descendant count limit), and total size cannot exceed 101KvB (the default descendant size limit).
If you're looking for review comments and github isn't loading them, I have a gist compiling some topics of discussion [here](https://gist.github.com/glozow/c3acaf161c95bba491fce31585b2aaf7)
ACKs for top commit:
laanwj:
Code review re-ACK 13650fe2e527bf0cf5d977bf5f3f1563b853ecdc
jnewbery:
Code review ACK 13650fe2e527bf0cf5d977bf5f3f1563b853ecdc
ariard:
ACK 13650fe
Tree-SHA512: 8c5cbfa91a6c714e1c8710bb281d5ff1c5af36741872a7c5df6b24874d6272b4a09f816cb8a4c7de33ef8e1c2a2c252c0df5105b7802f70bc6ff821ed7cc1a2f
-rw-r--r-- | doc/release-notes-20833.md | 12 | ||||
-rw-r--r-- | src/Makefile.am | 1 | ||||
-rw-r--r-- | src/policy/packages.h | 34 | ||||
-rw-r--r-- | src/rpc/rawtransaction.cpp | 130 | ||||
-rw-r--r-- | src/test/fuzz/tx_pool.cpp | 10 | ||||
-rw-r--r-- | src/test/miner_tests.cpp | 3 | ||||
-rw-r--r-- | src/test/txvalidation_tests.cpp | 98 | ||||
-rw-r--r-- | src/test/util/setup_common.cpp | 7 | ||||
-rw-r--r-- | src/test/util/setup_common.h | 4 | ||||
-rw-r--r-- | src/txmempool.cpp | 18 | ||||
-rw-r--r-- | src/txmempool.h | 10 | ||||
-rw-r--r-- | src/validation.cpp | 171 | ||||
-rw-r--r-- | src/validation.h | 64 | ||||
-rwxr-xr-x | test/functional/mempool_accept.py | 3 | ||||
-rwxr-xr-x | test/functional/rpc_packages.py | 362 | ||||
-rwxr-xr-x | test/functional/test_runner.py | 1 |
16 files changed, 841 insertions, 87 deletions
diff --git a/doc/release-notes-20833.md b/doc/release-notes-20833.md new file mode 100644 index 0000000000..9a02bbd275 --- /dev/null +++ b/doc/release-notes-20833.md @@ -0,0 +1,12 @@ +Updated RPCs +------------ + +- 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, even if it would be a valid BIP125 replace-by-fee. 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. (#20833) + diff --git a/src/Makefile.am b/src/Makefile.am index 2c25f04d08..80c142009c 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -193,6 +193,7 @@ BITCOIN_CORE_H = \ outputtype.h \ policy/feerate.h \ policy/fees.h \ + policy/packages.h \ policy/policy.h \ policy/rbf.h \ policy/settings.h \ diff --git a/src/policy/packages.h b/src/policy/packages.h new file mode 100644 index 0000000000..4b1463dcb3 --- /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 <consensus/validation.h> +#include <primitives/transaction.h> + +#include <vector> + +/** 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<CTransactionRef>; + +class PackageValidationState : public ValidationState<PackageValidationResult> {}; + +#endif // BITCOIN_POLICY_PACKAGES_H diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index adb8ac0595..339d711ac9 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -15,6 +15,7 @@ #include <node/context.h> #include <node/psbt.h> #include <node/transaction.h> +#include <policy/packages.h> #include <policy/policy.h> #include <policy/rbf.h> #include <primitives/transaction.h> @@ -885,8 +886,11 @@ static RPCHelpMan sendrawtransaction() static RPCHelpMan testmempoolaccept() { return 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" + "\nReturns result of mempool acceptance tests indicating if raw transaction(s) (serialized, hex-encoded) would be accepted by mempool.\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.\n" @@ -895,17 +899,21 @@ static RPCHelpMan testmempoolaccept() {"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\n"}, + {"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\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::STR_HEX, "wtxid", "The transaction witness hash in hex"}, - {RPCResult::Type::BOOL, "allowed", "If the mempool allows this tx to be inserted"}, + {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. This is different from actual serialized size for witness transactions as witness data is discounted (only present when 'allowed' is true)"}, {RPCResult::Type::OBJ, "fees", "Transaction fees (only present if 'allowed' is true)", { @@ -932,62 +940,86 @@ static RPCHelpMan testmempoolaccept() 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 CFeeRate max_raw_tx_fee_rate = request.params[1].isNull() ? DEFAULT_MAX_RAW_TX_FEE_RATE : CFeeRate(AmountFromValue(request.params[1])); - NodeContext& node = EnsureAnyNodeContext(request.context); + std::vector<CTransactionRef> 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->GetHash().GetHex()); - result_0.pushKV("wtxid", tx->GetWitnessHash().GetHex()); - - ChainstateManager& chainman = EnsureChainman(node); - const MempoolAcceptResult accept_result = WITH_LOCK(cs_main, return AcceptToMemoryPool(chainman.ActiveChainstate(), mempool, std::move(tx), - false /* bypass_limits */, /* test_accept */ true)); - - // 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); + 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]->GetWitnessHash(), + AcceptToMemoryPool(chainstate, mempool, txns[0], /* bypass_limits */ false, /* test_accept*/ true)); + }(); + + 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()); + result_inner.pushKV("wtxid", tx->GetWitnessHash().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->GetWitnessHash()); + if (exit_early || it == package_result.m_tx_results.end()) { + // Validation unfinished. Just return the txid and wtxid. + 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; }, }; } diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index ad11f2c5f2..bab34ea340 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -219,6 +219,16 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool) RegisterSharedValidationInterface(txr); const bool bypass_limits = fuzzed_data_provider.ConsumeBool(); ::fRequireStandard = fuzzed_data_provider.ConsumeBool(); + + // Make sure ProcessNewPackage on one transaction works and always fully validates the transaction. + // 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(node.chainman->ActiveChainstate(), tx_pool, {tx}, true)); + auto it = result_package.m_tx_results.find(tx->GetWitnessHash()); + Assert(it != result_package.m_tx_results.end()); + Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID || + it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID); + const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx_pool, tx, bypass_limits)); const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID; SyncWithValidationInterfaceQueue(); diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 9ba004cc38..c47d0eae1e 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -28,7 +28,8 @@ struct MinerTestingSetup : public TestingSetup { void TestPackageSelection(const CChainParams& chainparams, const CScript& scriptPubKey, const std::vector<CTransactionRef>& 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 8d14071297..95ad85d0f8 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 <consensus/validation.h> +#include <key_io.h> +#include <policy/packages.h> +#include <policy/policy.h> #include <primitives/transaction.h> #include <script/script.h> +#include <script/standard.h> #include <test/util/setup_common.h> #include <validation.h> @@ -47,4 +51,98 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup) BOOST_CHECK(result.m_state.GetResult() == TxValidationResult::TX_CONSENSUS); } +// Create placeholder transactions that have no meaning. +inline CTransactionRef create_placeholder_tx(size_t num_inputs, size_t num_outputs) +{ + CMutableTransaction mtx = CMutableTransaction(); + mtx.vin.resize(num_inputs); + mtx.vout.resize(num_outputs); + auto random_script = CScript() << ToByteVector(InsecureRand256()) << ToByteVector(InsecureRand256()); + for (size_t i{0}; i < num_inputs; ++i) { + mtx.vin[i].prevout.hash = InsecureRand256(); + mtx.vin[i].prevout.n = 0; + mtx.vin[i].scriptSig = random_script; + } + for (size_t o{0}; o < num_outputs; ++o) { + mtx.vout[o].nValue = 1 * CENT; + mtx.vout[o].scriptPubKey = random_script; + } + return MakeTransactionRef(mtx); +} + +BOOST_FIXTURE_TEST_CASE(package_tests, TestChain100Setup) +{ + LOCK(cs_main); + unsigned int initialPoolSize = m_node.mempool->size(); + + // Parent and Child Package + CKey parent_key; + parent_key.MakeNewKey(true); + CScript parent_locking_script = GetScriptForDestination(PKHash(parent_key.GetPubKey())); + auto mtx_parent = CreateValidMempoolTransaction(/* input_transaction */ m_coinbase_txns[0], /* vout */ 0, + /* input_height */ 0, /* input_signing_key */ coinbaseKey, + /* output_destination */ parent_locking_script, + /* output_amount */ CAmount(49 * COIN), /* submit */ false); + CTransactionRef tx_parent = MakeTransactionRef(mtx_parent); + + CKey child_key; + child_key.MakeNewKey(true); + CScript child_locking_script = GetScriptForDestination(PKHash(child_key.GetPubKey())); + auto mtx_child = CreateValidMempoolTransaction(/* input_transaction */ tx_parent, /* vout */ 0, + /* input_height */ 101, /* input_signing_key */ parent_key, + /* output_destination */ child_locking_script, + /* output_amount */ CAmount(48 * COIN), /* submit */ false); + CTransactionRef tx_child = MakeTransactionRef(mtx_child); + 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()); + 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_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()); + + // Packages can't have more than 25 transactions. + Package package_too_many; + package_too_many.reserve(MAX_PACKAGE_COUNT + 1); + for (size_t i{0}; i < MAX_PACKAGE_COUNT + 1; ++i) { + package_too_many.emplace_back(create_placeholder_tx(1, 1)); + } + auto result_too_many = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_too_many, /* test_accept */ true); + BOOST_CHECK(result_too_many.m_state.IsInvalid()); + BOOST_CHECK_EQUAL(result_too_many.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); + BOOST_CHECK_EQUAL(result_too_many.m_state.GetRejectReason(), "package-too-many-transactions"); + + // Packages can't have a total size of more than 101KvB. + CTransactionRef large_ptx = create_placeholder_tx(150, 150); + Package package_too_large; + auto size_large = GetVirtualTransactionSize(*large_ptx); + size_t total_size{0}; + while (total_size <= MAX_PACKAGE_SIZE * 1000) { + package_too_large.push_back(large_ptx); + total_size += size_large; + } + BOOST_CHECK(package_too_large.size() <= MAX_PACKAGE_COUNT); + auto result_too_large = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_too_large, /* test_accept */ true); + BOOST_CHECK(result_too_large.m_state.IsInvalid()); + BOOST_CHECK_EQUAL(result_too_large.m_state.GetResult(), PackageValidationResult::PCKG_POLICY); + BOOST_CHECK_EQUAL(result_too_large.m_state.GetRejectReason(), "package-too-large"); + + // A single, giant transaction submitted through ProcessNewPackage fails on single tx policy. + CTransactionRef giant_ptx = create_placeholder_tx(999, 999); + BOOST_CHECK(GetVirtualTransactionSize(*giant_ptx) > MAX_PACKAGE_SIZE * 1000); + auto result_single_large = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, {giant_ptx}, /* test_accept */ true); + 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"); + 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"); + + // Check that mempool size hasn't changed. + BOOST_CHECK_EQUAL(m_node.mempool->size(), initialPoolSize); +} BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 7bf7f9e0ba..0ac178443a 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -263,7 +263,8 @@ CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactio int input_height, CKey input_signing_key, CScript output_destination, - CAmount output_amount) + CAmount output_amount, + bool submit) { // Transaction we will submit to the mempool CMutableTransaction mempool_txn; @@ -296,8 +297,8 @@ CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactio std::map<int, std::string> input_errors; assert(SignTransaction(mempool_txn, &keystore, input_coins, nHashType, input_errors)); - // Add transaction to the mempool - { + // If submit=true, add transaction to the mempool. + if (submit) { LOCK(cs_main); const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), *m_node.mempool.get(), MakeTransactionRef(mempool_txn), /* bypass_limits */ false); assert(result.m_result_type == MempoolAcceptResult::ResultType::VALID); diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h index b19dd75765..5d12dc2323 100644 --- a/src/test/util/setup_common.h +++ b/src/test/util/setup_common.h @@ -135,13 +135,15 @@ struct TestChain100Setup : public RegTestingSetup { * @param input_signing_key The key to spend the input_transaction * @param output_destination Where to send the output * @param output_amount How much to send + * @param submit Whether or not to submit to mempool */ CMutableTransaction CreateValidMempoolTransaction(CTransactionRef input_transaction, int input_vout, int input_height, CKey input_signing_key, CScript output_destination, - CAmount output_amount = CAmount(1 * COIN)); + CAmount output_amount = CAmount(1 * COIN), + bool submit = true); ~TestChain100Setup(); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 5957637e81..4413da7ea7 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -515,7 +515,9 @@ void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) LockPoints lp = it->GetLockPoints(); assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); bool validLP = TestLockPointValidity(active_chainstate.m_chain, &lp); - if (!CheckFinalTx(active_chainstate.m_chain.Tip(), tx, flags) || !CheckSequenceLocks(active_chainstate, *this, tx, flags, &lp, validLP)) { + CCoinsViewMemPool viewMempool(&active_chainstate.CoinsTip(), *this); + if (!CheckFinalTx(active_chainstate.m_chain.Tip(), tx, flags) + || !CheckSequenceLocks(active_chainstate.m_chain.Tip(), viewMempool, tx, flags, &lp, validLP)) { // Note if CheckSequenceLocks fails the LockPoints may still be invalid // So it's critical that we remove the tx and not depend on the LockPoints. txToRemove.insert(it); @@ -920,6 +922,13 @@ bool CTxMemPool::HasNoInputsOf(const CTransaction &tx) const CCoinsViewMemPool::CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn) : CCoinsViewBacked(baseIn), mempool(mempoolIn) { } bool CCoinsViewMemPool::GetCoin(const COutPoint &outpoint, Coin &coin) const { + // Check to see if the inputs are made available by another tx in the package. + // These Coins would not be available in the underlying CoinsView. + if (auto it = m_temp_added.find(outpoint); it != m_temp_added.end()) { + coin = it->second; + return true; + } + // If an entry in the mempool exists, always return that one, as it's guaranteed to never // conflict with the underlying cache, and it cannot have pruned entries (as it contains full) // transactions. First checking the underlying cache risks returning a pruned entry instead. @@ -935,6 +944,13 @@ bool CCoinsViewMemPool::GetCoin(const COutPoint &outpoint, Coin &coin) const { return base->GetCoin(outpoint, coin); } +void CCoinsViewMemPool::PackageAddTransaction(const CTransactionRef& tx) +{ + for (unsigned int n = 0; n < tx->vout.size(); ++n) { + m_temp_added.emplace(COutPoint(tx->GetHash(), n), Coin(tx->vout[n], MEMPOOL_HEIGHT, false)); + } +} + size_t CTxMemPool::DynamicMemoryUsage() const { LOCK(cs); // Estimate the overhead of mapTx to be 15 pointers + an allocation, as no exact formula for boost::multi_index_contained is implemented. diff --git a/src/txmempool.h b/src/txmempool.h index 594b4981f6..46b89049bb 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -852,7 +852,8 @@ public: * CCoinsView that brings transactions from a mempool into view. * It does not check for spendings by memory pool transactions. * Instead, it provides access to all Coins which are either unspent in the - * base CCoinsView, or are outputs from any mempool transaction! + * base CCoinsView, are outputs from any mempool transaction, or are + * tracked temporarily to allow transaction dependencies in package validation. * This allows transaction replacement to work as expected, as you want to * have all inputs "available" to check signatures, and any cycles in the * dependency graph are checked directly in AcceptToMemoryPool. @@ -862,12 +863,19 @@ public: */ class CCoinsViewMemPool : public CCoinsViewBacked { + /** + * Coins made available by transactions being validated. Tracking these allows for package + * validation, since we can access transaction outputs without submitting them to mempool. + */ + std::unordered_map<COutPoint, Coin, SaltedOutpointHasher> m_temp_added; protected: const CTxMemPool& mempool; public: CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn); bool GetCoin(const COutPoint &outpoint, Coin &coin) const override; + /** Add the coins created by this transaction. */ + void PackageAddTransaction(const CTransactionRef& tx); }; /** diff --git a/src/validation.cpp b/src/validation.cpp index f591e64fd4..86539ab01f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -42,6 +42,7 @@ #include <uint256.h> #include <undo.h> #include <util/check.h> // For NDEBUG compile time check +#include <util/hasher.h> #include <util/moneystr.h> #include <util/rbf.h> #include <util/strencodings.h> @@ -50,6 +51,7 @@ #include <validationinterface.h> #include <warnings.h> +#include <numeric> #include <optional> #include <string> @@ -245,18 +247,13 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) return true; } -bool CheckSequenceLocks(CChainState& active_chainstate, - const CTxMemPool& pool, +bool CheckSequenceLocks(CBlockIndex* tip, + const CCoinsView& coins_view, const CTransaction& tx, int flags, LockPoints* lp, bool useExistingLockPoints) { - AssertLockHeld(cs_main); - AssertLockHeld(pool.cs); - assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); - - CBlockIndex* tip = active_chainstate.m_chain.Tip(); assert(tip != nullptr); CBlockIndex index; @@ -276,14 +273,12 @@ bool CheckSequenceLocks(CChainState& active_chainstate, lockPair.second = lp->time; } else { - // CoinsTip() contains the UTXO set for active_chainstate.m_chain.Tip() - CCoinsViewMemPool viewMemPool(&active_chainstate.CoinsTip(), pool); std::vector<int> prevheights; prevheights.resize(tx.vin.size()); for (size_t txinIndex = 0; txinIndex < tx.vin.size(); txinIndex++) { const CTxIn& txin = tx.vin[txinIndex]; Coin coin; - if (!viewMemPool.GetCoin(txin.prevout, coin)) { + if (!coins_view.GetCoin(txin.prevout, coin)) { return error("%s: Missing input", __func__); } if (coin.nHeight == MEMPOOL_HEIGHT) { @@ -477,11 +472,20 @@ public: */ std::vector<COutPoint>& m_coins_to_uncache; const bool m_test_accept; + /** Disable BIP125 RBFing; disallow all conflicts with mempool transactions. */ + const bool disallow_mempool_conflicts; }; // Single transaction acceptance MempoolAcceptResult AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + /** + * Multiple transaction acceptance. Transactions may or may not be interdependent, + * but must not conflict with each other. Parents must come before children if any + * dependencies exist, otherwise a TX_MISSING_INPUTS error will be returned. + */ + PackageMempoolAcceptResult AcceptMultipleTransactions(const std::vector<CTransactionRef>& txns, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + private: // All the intermediate state that gets passed between the various levels // of checking a given transaction. @@ -638,7 +642,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) break; } } - if (fReplacementOptOut) { + if (fReplacementOptOut || args.disallow_mempool_conflicts) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict"); } @@ -686,10 +690,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Only accept BIP68 sequence locked transactions that can be mined in the next // block; we don't want our mempool filled up with transactions that can't // be mined yet. - // Must keep pool.cs for this unless we change CheckSequenceLocks to take a - // CoinsViewCache instead of create its own + // Pass in m_view which has all of the relevant inputs cached. Note that, since m_view's + // backend was removed, it no longer pulls coins from the mempool. assert(std::addressof(::ChainstateActive()) == std::addressof(m_active_chainstate)); - if (!CheckSequenceLocks(m_active_chainstate, m_pool, tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp)) + if (!CheckSequenceLocks(m_active_chainstate.m_chain.Tip(), m_view, tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp)) return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "non-BIP68-final"); assert(std::addressof(g_chainman.m_blockman) == std::addressof(m_active_chainstate.m_blockman)); @@ -1045,7 +1049,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef Workspace ws(ptx); - if (!PreChecks(args, ws)) return MempoolAcceptResult(ws.m_state); + if (!PreChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); // Only compute the precomputed transaction data if we need to verify // scripts (ie, other policy checks pass). We perform the inexpensive @@ -1053,20 +1057,121 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef // checks pass, to mitigate CPU exhaustion denial-of-service attacks. PrecomputedTransactionData txdata; - if (!PolicyScriptChecks(args, ws, txdata)) return MempoolAcceptResult(ws.m_state); + if (!PolicyScriptChecks(args, ws, txdata)) return MempoolAcceptResult::Failure(ws.m_state); - if (!ConsensusScriptChecks(args, ws, txdata)) return MempoolAcceptResult(ws.m_state); + if (!ConsensusScriptChecks(args, ws, txdata)) return MempoolAcceptResult::Failure(ws.m_state); // Tx was accepted, but not added if (args.m_test_accept) { - return MempoolAcceptResult(std::move(ws.m_replaced_transactions), ws.m_base_fees); + return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_base_fees); } - if (!Finalize(args, ws)) return MempoolAcceptResult(ws.m_state); + if (!Finalize(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); GetMainSignals().TransactionAddedToMempool(ptx, m_pool.GetAndIncrementSequence()); - return MempoolAcceptResult(std::move(ws.m_replaced_transactions), ws.m_base_fees); + return MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_base_fees); +} + +PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::vector<CTransactionRef>& txns, ATMPArgs& args) +{ + AssertLockHeld(cs_main); + + PackageValidationState package_state; + const unsigned int package_count = txns.size(); + + // These context-free package limits can be checked before taking the mempool lock. + if (package_count > MAX_PACKAGE_COUNT) { + package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-many-transactions"); + return PackageMempoolAcceptResult(package_state, {}); + } + + const int64_t total_size = std::accumulate(txns.cbegin(), txns.cend(), 0, + [](int64_t sum, const auto& tx) { return sum + GetVirtualTransactionSize(*tx); }); + // If the package only contains 1 tx, it's better to report the policy violation on individual tx size. + if (package_count > 1 && total_size > MAX_PACKAGE_SIZE * 1000) { + package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-large"); + return PackageMempoolAcceptResult(package_state, {}); + } + + // Construct workspaces and check package policies. + std::vector<Workspace> workspaces{}; + workspaces.reserve(package_count); + { + std::unordered_set<uint256, SaltedTxidHasher> later_txids; + std::transform(txns.cbegin(), txns.cend(), std::inserter(later_txids, later_txids.end()), + [](const auto& tx) { return tx->GetHash(); }); + // Require the package to be sorted in order of dependency, i.e. parents appear before children. + // An unsorted package will fail anyway on missing-inputs, but it's better to quit earlier and + // fail on something less ambiguous (missing-inputs could also be an orphan or trying to + // spend nonexistent coins). + for (const auto& tx : txns) { + for (const auto& input : tx->vin) { + if (later_txids.find(input.prevout.hash) != later_txids.end()) { + // The parent is a subsequent transaction in the package. + package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted"); + return PackageMempoolAcceptResult(package_state, {}); + } + } + later_txids.erase(tx->GetHash()); + workspaces.emplace_back(Workspace(tx)); + } + } + std::map<const uint256, const MempoolAcceptResult> results; + { + // Don't allow any conflicting transactions, i.e. spending the same inputs, in a package. + std::unordered_set<COutPoint, SaltedOutpointHasher> inputs_seen; + for (const auto& tx : txns) { + for (const auto& input : tx->vin) { + if (inputs_seen.find(input.prevout) != inputs_seen.end()) { + // This input is also present in another tx in the package. + package_state.Invalid(PackageValidationResult::PCKG_POLICY, "conflict-in-package"); + return PackageMempoolAcceptResult(package_state, {}); + } + } + // Batch-add all the inputs for a tx at a time. If we added them 1 at a time, we could + // catch duplicate inputs within a single tx. This is a more severe, consensus error, + // and we want to report that from CheckTransaction instead. + std::transform(tx->vin.cbegin(), tx->vin.cend(), std::inserter(inputs_seen, inputs_seen.end()), + [](const auto& input) { return input.prevout; }); + } + } + + LOCK(m_pool.cs); + + // Do all PreChecks first and fail fast to avoid running expensive script checks when unnecessary. + for (Workspace& ws : workspaces) { + if (!PreChecks(args, ws)) { + package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); + // 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 RBFs are + // impossible, we don't need to track the coins spent. Note that this logic will need to be + // updated if RBFs in packages are allowed in the future. + assert(args.disallow_mempool_conflicts); + m_viewmempool.PackageAddTransaction(ws.m_ptx); + } + + for (Workspace& ws : workspaces) { + PrecomputedTransactionData txdata; + if (!PolicyScriptChecks(args, ws, txdata)) { + // 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, 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). + results.emplace(ws.m_ptx->GetWitnessHash(), + MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_base_fees)); + } + } + + return PackageMempoolAcceptResult(package_state, std::move(results)); } } // anon namespace @@ -1079,7 +1184,8 @@ static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainp EXCLUSIVE_LOCKS_REQUIRED(cs_main) { std::vector<COutPoint> coins_to_uncache; - MemPoolAccept::ATMPArgs args { chainparams, nAcceptTime, bypass_limits, coins_to_uncache, test_accept }; + MemPoolAccept::ATMPArgs args { chainparams, nAcceptTime, bypass_limits, coins_to_uncache, + test_accept, /* disallow_mempool_conflicts */ false }; assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args); @@ -1105,6 +1211,29 @@ MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPoo return AcceptToMemoryPoolWithTime(Params(), pool, active_chainstate, tx, GetTime(), bypass_limits, test_accept); } +PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTxMemPool& pool, + const Package& package, bool test_accept) +{ + AssertLockHeld(cs_main); + assert(test_accept); // Only allow package accept dry-runs (testmempoolaccept RPC). + assert(!package.empty()); + assert(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;})); + + std::vector<COutPoint> coins_to_uncache; + const CChainParams& chainparams = Params(); + MemPoolAccept::ATMPArgs args { chainparams, GetTime(), /* bypass_limits */ false, coins_to_uncache, + test_accept, /* disallow_mempool_conflicts */ true }; + assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate)); + const PackageMempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptMultipleTransactions(package, args); + + // Uncache coins pertaining to transactions that were not submitted to the mempool. + // Ensure the cache is still within its size limits. + for (const COutPoint& hashTx : coins_to_uncache) { + active_chainstate.CoinsTip().Uncache(hashTx); + } + return result; +} + CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock) { LOCK(cs_main); diff --git a/src/validation.h b/src/validation.h index adc3d282b6..43e1c5c22e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -18,6 +18,7 @@ #include <fs.h> #include <node/utxo_snapshot.h> #include <policy/feerate.h> +#include <policy/packages.h> #include <protocol.h> // For CMessageHeader::MessageStartChars #include <script/script_error.h> #include <sync.h> @@ -167,9 +168,7 @@ void PruneBlockFilesManual(CChainState& active_chainstate, int nManualPruneHeigh * Validation result for a single transaction mempool acceptance. */ struct MempoolAcceptResult { - /** Used to indicate the results of mempool validation, - * including the possibility of unfinished validation. - */ + /** Used to indicate the results of mempool validation. */ enum class ResultType { VALID, //!> Fully validated, valid. INVALID, //!> Invalid. @@ -182,7 +181,16 @@ struct MempoolAcceptResult { const std::optional<std::list<CTransactionRef>> m_replaced_transactions; /** Raw base fees in satoshis. */ const std::optional<CAmount> m_base_fees; + static MempoolAcceptResult Failure(TxValidationState state) { + return MempoolAcceptResult(state); + } + static MempoolAcceptResult Success(std::list<CTransactionRef>&& replaced_txns, CAmount fees) { + return MempoolAcceptResult(std::move(replaced_txns), fees); + } + +// Private constructors. Use static methods MempoolAcceptResult::Success, etc. to construct. +private: /** Constructor for failure case */ explicit MempoolAcceptResult(TxValidationState state) : m_result_type(ResultType::INVALID), m_state(state) { @@ -196,6 +204,28 @@ struct MempoolAcceptResult { }; /** +* Validation result for package mempool acceptance. +*/ +struct PackageMempoolAcceptResult +{ + const PackageValidationState m_state; + /** + * Map from wtxid to finished MempoolAcceptResults. The client is responsible + * for keeping track of the transaction objects themselves. If a result is not + * present, it means validation was unfinished for that transaction. + */ + std::map<const uint256, const MempoolAcceptResult> m_tx_results; + + explicit PackageMempoolAcceptResult(PackageValidationState state, + std::map<const uint256, const MempoolAcceptResult>&& results) + : 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) + : m_tx_results{ {wtxid, result} } {} +}; + +/** * (Try to) add a transaction to the memory pool. * @param[in] bypass_limits When true, don't enforce mempool fee limits. * @param[in] test_accept When true, run validation checks but don't submit to mempool. @@ -203,6 +233,18 @@ struct MempoolAcceptResult { MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx, bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +/** +* Atomically test acceptance of a package. If the package only contains one tx, package rules still apply. +* @param[in] txns Group of transactions which may be independent or contain +* parent-child dependencies. The transactions must not conflict, i.e. +* must not spend the same inputs, even if it would be a valid BIP125 +* replace-by-fee. Parents must appear before children. +* @returns a PackageMempoolAcceptResult which includes a MempoolAcceptResult for each transaction. +* If a transaction fails, validation will exit early and some results may be missing. +*/ +PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTxMemPool& pool, + const Package& txns, bool test_accept) + EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** Apply the effects of this transaction on the UTXO set represented by view */ void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, int nHeight); @@ -224,9 +266,13 @@ bool CheckFinalTx(const CBlockIndex* active_chain_tip, const CTransaction &tx, i bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** - * Check if transaction will be BIP 68 final in the next block to be created. - * - * Simulates calling SequenceLocks() with data from the tip of the current active chain. + * Check if transaction will be BIP68 final in the next block to be created on top of tip. + * @param[in] tip Chain tip to check tx sequence locks against. For example, + * the tip of the current active chain. + * @param[in] coins_view Any CCoinsView that provides access to the relevant coins + * for checking sequence locks. Any CCoinsView can be passed in; + * it is assumed to be consistent with the tip. + * Simulates calling SequenceLocks() with data from the tip passed in. * Optionally stores in LockPoints the resulting height and time calculated and the hash * of the block needed for calculation or skips the calculation and uses the LockPoints * passed in for evaluation. @@ -234,12 +280,12 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) EXCLUSIVE * * See consensus/consensus.h for flag definitions. */ -bool CheckSequenceLocks(CChainState& active_chainstate, - const CTxMemPool& pool, +bool CheckSequenceLocks(CBlockIndex* tip, + const CCoinsView& coins_view, const CTransaction& tx, int flags, LockPoints* lp = nullptr, - bool useExistingLockPoints = false) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs); + bool useExistingLockPoints = false); /** * Closure representing one script verification diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index c4002f524a..12aac3ab65 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -67,7 +67,8 @@ class MempoolAcceptanceTest(BitcoinTestFramework): self.log.info('Should not accept garbage to testmempoolaccept') assert_raises_rpc_error(-3, 'Expected type array, got string', lambda: node.testmempoolaccept(rawtxs='ff00baar')) - assert_raises_rpc_error(-8, 'Array must contain exactly one raw transaction for now', lambda: node.testmempoolaccept(rawtxs=['ff00baar', 'ff22'])) + assert_raises_rpc_error(-8, 'Array must contain between 1 and 25 transactions.', lambda: node.testmempoolaccept(rawtxs=['ff22']*26)) + assert_raises_rpc_error(-8, 'Array must contain between 1 and 25 transactions.', lambda: node.testmempoolaccept(rawtxs=[])) assert_raises_rpc_error(-22, 'TX decode failed', lambda: node.testmempoolaccept(rawtxs=['ff00baar'])) self.log.info('A transaction already in the blockchain') diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py new file mode 100755 index 0000000000..3d8d81d6b8 --- /dev/null +++ b/test/functional/rpc_packages.py @@ -0,0 +1,362 @@ +#!/usr/bin/env python3 +# 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. +"""RPCs that handle raw transaction packages.""" + +from decimal import Decimal +from io import BytesIO +import random + +from test_framework.address import ADDRESS_BCRT1_P2WSH_OP_TRUE +from test_framework.test_framework import BitcoinTestFramework +from test_framework.messages import ( + BIP125_SEQUENCE_NUMBER, + COIN, + CTransaction, + CTxInWitness, +) +from test_framework.script import ( + CScript, + OP_TRUE, +) +from test_framework.util import ( + assert_equal, + hex_str_to_bytes, +) + +class RPCPackagesTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 1 + self.setup_clean_chain = True + + def assert_testres_equal(self, package_hex, testres_expected): + """Shuffle package_hex and assert that the testmempoolaccept result matches testres_expected. This should only + be used to test packages where the order does not matter. The ordering of transactions in package_hex and + testres_expected must match. + """ + shuffled_indeces = list(range(len(package_hex))) + random.shuffle(shuffled_indeces) + shuffled_package = [package_hex[i] for i in shuffled_indeces] + shuffled_testres = [testres_expected[i] for i in shuffled_indeces] + assert_equal(shuffled_testres, self.nodes[0].testmempoolaccept(shuffled_package)) + + def run_test(self): + self.log.info("Generate blocks to create UTXOs") + node = self.nodes[0] + self.privkeys = [node.get_deterministic_priv_key().key] + self.address = node.get_deterministic_priv_key().address + self.coins = [] + # The last 100 coinbase transactions are premature + for b in node.generatetoaddress(200, self.address)[:100]: + coinbase = node.getblock(blockhash=b, verbosity=2)["tx"][0] + self.coins.append({ + "txid": coinbase["txid"], + "amount": coinbase["vout"][0]["value"], + "scriptPubKey": coinbase["vout"][0]["scriptPubKey"], + }) + + # Create some transactions that can be reused throughout the test. Never submit these to mempool. + self.independent_txns_hex = [] + self.independent_txns_testres = [] + for _ in range(3): + coin = self.coins.pop() + rawtx = node.createrawtransaction([{"txid": coin["txid"], "vout": 0}], + {self.address : coin["amount"] - Decimal("0.0001")}) + signedtx = node.signrawtransactionwithkey(hexstring=rawtx, privkeys=self.privkeys) + assert signedtx["complete"] + testres = node.testmempoolaccept([signedtx["hex"]]) + assert testres[0]["allowed"] + self.independent_txns_hex.append(signedtx["hex"]) + # testmempoolaccept returns a list of length one, avoid creating a 2D list + self.independent_txns_testres.append(testres[0]) + self.independent_txns_testres_blank = [{ + "txid": res["txid"], "wtxid": res["wtxid"]} for res in self.independent_txns_testres] + + self.test_independent() + self.test_chain() + self.test_multiple_children() + self.test_multiple_parents() + self.test_conflicting() + self.test_rbf() + + def chain_transaction(self, parent_txid, parent_value, n=0, parent_locking_script=None): + """Build a transaction that spends parent_txid.vout[n] and produces one output with + amount = parent_value with a fee deducted. + Return tuple (CTransaction object, raw hex, nValue, scriptPubKey of the output created). + """ + node = self.nodes[0] + inputs = [{"txid": parent_txid, "vout": n}] + my_value = parent_value - Decimal("0.0001") + outputs = {self.address : my_value} + rawtx = node.createrawtransaction(inputs, outputs) + prevtxs = [{ + "txid": parent_txid, + "vout": n, + "scriptPubKey": parent_locking_script, + "amount": parent_value, + }] if parent_locking_script else None + signedtx = node.signrawtransactionwithkey(hexstring=rawtx, privkeys=self.privkeys, prevtxs=prevtxs) + tx = CTransaction() + assert signedtx["complete"] + tx.deserialize(BytesIO(hex_str_to_bytes(signedtx["hex"]))) + return (tx, signedtx["hex"], my_value, tx.vout[0].scriptPubKey.hex()) + + def test_independent(self): + self.log.info("Test multiple independent transactions in a package") + node = self.nodes[0] + # For independent transactions, order doesn't matter. + self.assert_testres_equal(self.independent_txns_hex, self.independent_txns_testres) + + self.log.info("Test an otherwise valid package with an extra garbage tx appended") + garbage_tx = node.createrawtransaction([{"txid": "00" * 32, "vout": 5}], {self.address: 1}) + tx = CTransaction() + tx.deserialize(BytesIO(hex_str_to_bytes(garbage_tx))) + # Only the txid and wtxids are returned because validation is incomplete for the independent txns. + # Package validation is atomic: if the node cannot find a UTXO for any single tx in the package, + # it terminates immediately to avoid unnecessary, expensive signature verification. + package_bad = self.independent_txns_hex + [garbage_tx] + testres_bad = self.independent_txns_testres_blank + [{"txid": tx.rehash(), "wtxid": tx.getwtxid(), "allowed": False, "reject-reason": "missing-inputs"}] + self.assert_testres_equal(package_bad, testres_bad) + + self.log.info("Check testmempoolaccept tells us when some transactions completed validation successfully") + coin = self.coins.pop() + tx_bad_sig_hex = node.createrawtransaction([{"txid": coin["txid"], "vout": 0}], + {self.address : coin["amount"] - Decimal("0.0001")}) + tx_bad_sig = CTransaction() + tx_bad_sig.deserialize(BytesIO(hex_str_to_bytes(tx_bad_sig_hex))) + testres_bad_sig = node.testmempoolaccept(self.independent_txns_hex + [tx_bad_sig_hex]) + # By the time the signature for the last transaction is checked, all the other transactions + # have been fully validated, which is why the node returns full validation results for all + # transactions here but empty results in other cases. + assert_equal(testres_bad_sig, self.independent_txns_testres + [{ + "txid": tx_bad_sig.rehash(), + "wtxid": tx_bad_sig.getwtxid(), "allowed": False, + "reject-reason": "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)" + }]) + + self.log.info("Check testmempoolaccept reports txns in packages that exceed max feerate") + coin = self.coins.pop() + tx_high_fee_raw = node.createrawtransaction([{"txid": coin["txid"], "vout": 0}], + {self.address : coin["amount"] - Decimal("0.999")}) + tx_high_fee_signed = node.signrawtransactionwithkey(hexstring=tx_high_fee_raw, privkeys=self.privkeys) + assert tx_high_fee_signed["complete"] + tx_high_fee = CTransaction() + tx_high_fee.deserialize(BytesIO(hex_str_to_bytes(tx_high_fee_signed["hex"]))) + testres_high_fee = node.testmempoolaccept([tx_high_fee_signed["hex"]]) + assert_equal(testres_high_fee, [ + {"txid": tx_high_fee.rehash(), "wtxid": tx_high_fee.getwtxid(), "allowed": False, "reject-reason": "max-fee-exceeded"} + ]) + package_high_fee = [tx_high_fee_signed["hex"]] + self.independent_txns_hex + testres_package_high_fee = node.testmempoolaccept(package_high_fee) + assert_equal(testres_package_high_fee, testres_high_fee + self.independent_txns_testres_blank) + + def test_chain(self): + node = self.nodes[0] + first_coin = self.coins.pop() + + # Chain of 25 transactions + parent_locking_script = None + txid = first_coin["txid"] + chain_hex = [] + chain_txns = [] + value = first_coin["amount"] + + for _ in range(25): + (tx, txhex, value, parent_locking_script) = self.chain_transaction(txid, value, 0, parent_locking_script) + txid = tx.rehash() + chain_hex.append(txhex) + chain_txns.append(tx) + + self.log.info("Check that testmempoolaccept requires packages to be sorted by dependency") + assert_equal(node.testmempoolaccept(rawtxs=chain_hex[::-1]), + [{"txid": tx.rehash(), "wtxid": tx.getwtxid(), "package-error": "package-not-sorted"} for tx in chain_txns[::-1]]) + + self.log.info("Testmempoolaccept a chain of 25 transactions") + testres_multiple = node.testmempoolaccept(rawtxs=chain_hex) + + testres_single = [] + # Test accept and then submit each one individually, which should be identical to package test accept + for rawtx in chain_hex: + testres = node.testmempoolaccept([rawtx]) + testres_single.append(testres[0]) + # Submit the transaction now so its child should have no problem validating + node.sendrawtransaction(rawtx) + assert_equal(testres_single, testres_multiple) + + # Clean up by clearing the mempool + node.generate(1) + + def test_multiple_children(self): + node = self.nodes[0] + + self.log.info("Testmempoolaccept a package in which a transaction has two children within the package") + first_coin = self.coins.pop() + value = (first_coin["amount"] - Decimal("0.0002")) / 2 # Deduct reasonable fee and make 2 outputs + inputs = [{"txid": first_coin["txid"], "vout": 0}] + outputs = [{self.address : value}, {ADDRESS_BCRT1_P2WSH_OP_TRUE : value}] + rawtx = node.createrawtransaction(inputs, outputs) + + parent_signed = node.signrawtransactionwithkey(hexstring=rawtx, privkeys=self.privkeys) + parent_tx = CTransaction() + assert parent_signed["complete"] + parent_tx.deserialize(BytesIO(hex_str_to_bytes(parent_signed["hex"]))) + parent_txid = parent_tx.rehash() + assert node.testmempoolaccept([parent_signed["hex"]])[0]["allowed"] + + parent_locking_script_a = parent_tx.vout[0].scriptPubKey.hex() + child_value = value - Decimal("0.0001") + + # Child A + (_, tx_child_a_hex, _, _) = self.chain_transaction(parent_txid, child_value, 0, parent_locking_script_a) + assert not node.testmempoolaccept([tx_child_a_hex])[0]["allowed"] + + # Child B + rawtx_b = node.createrawtransaction([{"txid": parent_txid, "vout": 1}], {self.address : child_value}) + tx_child_b = CTransaction() + tx_child_b.deserialize(BytesIO(hex_str_to_bytes(rawtx_b))) + tx_child_b.wit.vtxinwit = [CTxInWitness()] + tx_child_b.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])] + tx_child_b_hex = tx_child_b.serialize().hex() + assert not node.testmempoolaccept([tx_child_b_hex])[0]["allowed"] + + self.log.info("Testmempoolaccept with entire package, should work with children in either order") + testres_multiple_ab = node.testmempoolaccept(rawtxs=[parent_signed["hex"], tx_child_a_hex, tx_child_b_hex]) + testres_multiple_ba = node.testmempoolaccept(rawtxs=[parent_signed["hex"], tx_child_b_hex, tx_child_a_hex]) + assert all([testres["allowed"] for testres in testres_multiple_ab + testres_multiple_ba]) + + testres_single = [] + # Test accept and then submit each one individually, which should be identical to package testaccept + for rawtx in [parent_signed["hex"], tx_child_a_hex, tx_child_b_hex]: + testres = node.testmempoolaccept([rawtx]) + testres_single.append(testres[0]) + # Submit the transaction now so its child should have no problem validating + node.sendrawtransaction(rawtx) + assert_equal(testres_single, testres_multiple_ab) + + def create_child_with_parents(self, parents_tx, values, locking_scripts): + """Creates a transaction that spends the first output of each parent in parents_tx.""" + num_parents = len(parents_tx) + total_value = sum(values) + inputs = [{"txid": tx.rehash(), "vout": 0} for tx in parents_tx] + outputs = {self.address : total_value - num_parents * Decimal("0.0001")} + rawtx_child = self.nodes[0].createrawtransaction(inputs, outputs) + prevtxs = [] + for i in range(num_parents): + prevtxs.append({"txid": parents_tx[i].rehash(), "vout": 0, "scriptPubKey": locking_scripts[i], "amount": values[i]}) + signedtx_child = self.nodes[0].signrawtransactionwithkey(hexstring=rawtx_child, privkeys=self.privkeys, prevtxs=prevtxs) + assert signedtx_child["complete"] + return signedtx_child["hex"] + + def test_multiple_parents(self): + node = self.nodes[0] + + self.log.info("Testmempoolaccept a package in which a transaction has multiple parents within the package") + for num_parents in [2, 10, 24]: + # Test a package with num_parents parents and 1 child transaction. + package_hex = [] + parents_tx = [] + values = [] + parent_locking_scripts = [] + for _ in range(num_parents): + parent_coin = self.coins.pop() + value = parent_coin["amount"] + (tx, txhex, value, parent_locking_script) = self.chain_transaction(parent_coin["txid"], value) + package_hex.append(txhex) + parents_tx.append(tx) + values.append(value) + parent_locking_scripts.append(parent_locking_script) + child_hex = self.create_child_with_parents(parents_tx, values, parent_locking_scripts) + # Package accept should work with the parents in any order (as long as parents come before child) + for _ in range(10): + random.shuffle(package_hex) + testres_multiple = node.testmempoolaccept(rawtxs=package_hex + [child_hex]) + assert all([testres["allowed"] for testres in testres_multiple]) + + testres_single = [] + # Test accept and then submit each one individually, which should be identical to package testaccept + for rawtx in package_hex + [child_hex]: + testres_single.append(node.testmempoolaccept([rawtx])[0]) + # Submit the transaction now so its child should have no problem validating + node.sendrawtransaction(rawtx) + assert_equal(testres_single, testres_multiple) + + def test_conflicting(self): + node = self.nodes[0] + prevtx = self.coins.pop() + inputs = [{"txid": prevtx["txid"], "vout": 0}] + output1 = {node.get_deterministic_priv_key().address: 50 - 0.00125} + output2 = {ADDRESS_BCRT1_P2WSH_OP_TRUE: 50 - 0.00125} + + # tx1 and tx2 share the same inputs + rawtx1 = node.createrawtransaction(inputs, output1) + rawtx2 = node.createrawtransaction(inputs, output2) + signedtx1 = node.signrawtransactionwithkey(hexstring=rawtx1, privkeys=self.privkeys) + signedtx2 = node.signrawtransactionwithkey(hexstring=rawtx2, privkeys=self.privkeys) + tx1 = CTransaction() + tx1.deserialize(BytesIO(hex_str_to_bytes(signedtx1["hex"]))) + tx2 = CTransaction() + tx2.deserialize(BytesIO(hex_str_to_bytes(signedtx2["hex"]))) + assert signedtx1["complete"] + assert signedtx2["complete"] + + # Ensure tx1 and tx2 are valid by themselves + assert node.testmempoolaccept([signedtx1["hex"]])[0]["allowed"] + assert node.testmempoolaccept([signedtx2["hex"]])[0]["allowed"] + + self.log.info("Test duplicate transactions in the same package") + testres = node.testmempoolaccept([signedtx1["hex"], signedtx1["hex"]]) + assert_equal(testres, [ + {"txid": tx1.rehash(), "wtxid": tx1.getwtxid(), "package-error": "conflict-in-package"}, + {"txid": tx1.rehash(), "wtxid": tx1.getwtxid(), "package-error": "conflict-in-package"} + ]) + + self.log.info("Test conflicting transactions in the same package") + testres = node.testmempoolaccept([signedtx1["hex"], signedtx2["hex"]]) + assert_equal(testres, [ + {"txid": tx1.rehash(), "wtxid": tx1.getwtxid(), "package-error": "conflict-in-package"}, + {"txid": tx2.rehash(), "wtxid": tx2.getwtxid(), "package-error": "conflict-in-package"} + ]) + + def test_rbf(self): + node = self.nodes[0] + coin = self.coins.pop() + inputs = [{"txid": coin["txid"], "vout": 0, "sequence": BIP125_SEQUENCE_NUMBER}] + fee = Decimal('0.00125000') + output = {node.get_deterministic_priv_key().address: 50 - fee} + raw_replaceable_tx = node.createrawtransaction(inputs, output) + signed_replaceable_tx = node.signrawtransactionwithkey(hexstring=raw_replaceable_tx, privkeys=self.privkeys) + testres_replaceable = node.testmempoolaccept([signed_replaceable_tx["hex"]]) + replaceable_tx = CTransaction() + replaceable_tx.deserialize(BytesIO(hex_str_to_bytes(signed_replaceable_tx["hex"]))) + assert_equal(testres_replaceable, [ + {"txid": replaceable_tx.rehash(), "wtxid": replaceable_tx.getwtxid(), + "allowed": True, "vsize": replaceable_tx.get_vsize(), "fees": { "base": fee }} + ]) + + # Replacement transaction is identical except has double the fee + replacement_tx = CTransaction() + replacement_tx.deserialize(BytesIO(hex_str_to_bytes(signed_replaceable_tx["hex"]))) + replacement_tx.vout[0].nValue -= int(fee * COIN) # Doubled fee + signed_replacement_tx = node.signrawtransactionwithkey(replacement_tx.serialize().hex(), self.privkeys) + replacement_tx.deserialize(BytesIO(hex_str_to_bytes(signed_replacement_tx["hex"]))) + + self.log.info("Test that transactions within a package cannot replace each other") + testres_rbf_conflicting = node.testmempoolaccept([signed_replaceable_tx["hex"], signed_replacement_tx["hex"]]) + assert_equal(testres_rbf_conflicting, [ + {"txid": replaceable_tx.rehash(), "wtxid": replaceable_tx.getwtxid(), "package-error": "conflict-in-package"}, + {"txid": replacement_tx.rehash(), "wtxid": replacement_tx.getwtxid(), "package-error": "conflict-in-package"} + ]) + + self.log.info("Test that packages cannot conflict with mempool transactions, even if a valid BIP125 RBF") + node.sendrawtransaction(signed_replaceable_tx["hex"]) + testres_rbf_single = node.testmempoolaccept([signed_replacement_tx["hex"]]) + # This transaction is a valid BIP125 replace-by-fee + assert testres_rbf_single[0]["allowed"] + testres_rbf_package = self.independent_txns_testres_blank + [{ + "txid": replacement_tx.rehash(), "wtxid": replacement_tx.getwtxid(), "allowed": False, "reject-reason": "txn-mempool-conflict" + }] + self.assert_testres_equal(self.independent_txns_hex + [signed_replacement_tx["hex"]], testres_rbf_package) + +if __name__ == "__main__": + RPCPackagesTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 00527e78f1..49f269f8b4 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -211,6 +211,7 @@ BASE_SCRIPTS = [ 'mempool_package_onemore.py', 'rpc_createmultisig.py --legacy-wallet', 'rpc_createmultisig.py --descriptors', + 'rpc_packages.py', 'feature_versionbits_warning.py', 'rpc_preciousblock.py', 'wallet_importprunedfunds.py --legacy-wallet', |