(Partial) Merge bitcoin#20833: rpc/validation: enable packages throug…

…h testmempoolaccept

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

@@ -264,6 +264,7 @@ BITCOIN_CORE_H = \
   outputtype.h \
   policy/feerate.h \
   policy/fees.h \
+  policy/packages.h \
   policy/policy.h \
   policy/settings.h \
   pow.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

@@ -18,6 +18,7 @@
 #include <node/coin.h>
 #include <node/context.h>
 #include <node/transaction.h>
+#include <policy/packages.h>
 #include <policy/policy.h>
 #include <policy/settings.h>
 #include <primitives/transaction.h>
@@ -927,25 +928,32 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
 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"
+                "\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.",
                         {
                             {"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::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::STR, "reject-reason", "Rejection string (only present when 'allowed' is false)"},
                         }},
                     }
@@ -967,8 +975,10 @@ 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");
+    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.");
     }
 
     CMutableTransaction mtx;

src/test/miner_tests.cpp

@@ -35,7 +35,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);
 };

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>
 
@@ -51,4 +55,98 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup)
     BOOST_CHECK(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()

src/txmempool.cpp

@@ -807,7 +807,9 @@ void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) EXCLU
         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);
@@ -1475,6 +1477,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.
@@ -1490,6 +1499,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 12 pointers + an allocation, as no exact formula for boost::multi_index_contained is implemented.

src/txmempool.h

@@ -869,7 +869,8 @@ class CTxMemPool
  * 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.
@@ -879,12 +880,19 @@ class CTxMemPool
  */
 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);
 };
 
 /**