Merge bitcoin#20833: rpc/validation: enable packages through testmemp…

…oolaccept

13650fe [policy] detect unsorted packages (glozow)
9ef643e [doc] add release note for package testmempoolaccept (glozow)
c4259f4 [test] functional test for packages in RPCs (glozow)
9ede34a [rpc] allow multiple txns in testmempoolaccept (glozow)
ae8e6df [policy] limit package sizes (glozow)
c9e1a26 [fuzz] add ProcessNewPackage call in tx_pool fuzzer (glozow)
363e3d9 [test] unit tests for ProcessNewPackage (glozow)
cd9a11a [test] make submit optional in CreateValidMempoolTransaction (glozow)
2ef1879 [validation] package validation for test accepts (glozow)
578148d [validation] explicit Success/Failure ctors for MempoolAcceptResult (glozow)
b88d77a [policy] Define packages (glozow)
249f43f [refactor] add option to disable RBF (glozow)
897e348 [coins/mempool] extend CCoinsViewMemPool to track temporary coins (glozow)
42cf8b2 [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 bitcoin#18480.
  - It's also a first step towards package validation in a minimally invasive way.
  - The RPC commit happens to close bitcoin#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 13650fe
  jnewbery:
    Code review ACK 13650fe
  ariard:
    ACK 13650fe

Tree-SHA512: 8c5cbfa91a6c714e1c8710bb281d5ff1c5af36741872a7c5df6b24874d6272b4a09f816cb8a4c7de33ef8e1c2a2c252c0df5105b7802f70bc6ff821ed7cc1a2f

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)
+

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 \

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

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;
 },
     };
 }

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();

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);
 };