Merge bitcoin/bitcoin#22084: package testmempoolaccept followups

ee862d6efb4c3c01e55f0d5d7a82cce75323cf40 MOVEONLY: context-free package policies (glozow)
5cac95cd15da04b83afa1d31a43be9f5b30a1827 disallow_mempool_conflicts -> allow_bip125_replacement and check earlier (glozow)
e8ecc621be6afd3252c0f8147e42c3b4918f7f46 [refactor] comment/naming improvements (glozow)
7d91442461776e2ef240d7885f768b624de341a7 [rpc] reserve space in txns (glozow)
6c5f19d9c4d267c54f4dbc4f9d65370ff1e0625b [package] static_assert max package size >= max tx size (glozow)

Pull request description:

  various followups from #20833

ACKs for top commit:
  jnewbery:
    utACK ee862d6efb4c3c01e55f0d5d7a82cce75323cf40
  ariard:
    Code Review ACK ee862d6

Tree-SHA512: 96ecb41f7bbced84d4253070f5274b7267607bfe4033e2bb0d2f55ec778cc41e811130b6321131e0418b5835894e510a4be8a0f822bc9d68d9224418359ac837

10 files changed, 120 insertions, 84 deletions

diff --git a/src/Makefile.am b/src/Makefile.am
index 66cb7cec2a..e2ed70556d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -351,6 +351,7 @@ libbitcoin_server_a_SOURCES = \
   node/ui_interface.cpp \
   noui.cpp \
   policy/fees.cpp \
+  policy/packages.cpp \
   policy/rbf.cpp \
   policy/settings.cpp \
   pow.cpp \
diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp
new file mode 100644
index 0000000000..cfd0539965
--- /dev/null
+++ b/src/policy/packages.cpp
@@ -0,0 +1,62 @@
+// 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.
+
+#include <consensus/validation.h>
+#include <policy/packages.h>
+#include <primitives/transaction.h>
+#include <uint256.h>
+#include <util/hasher.h>
+
+#include <numeric>
+#include <unordered_set>
+
+bool CheckPackage(const Package& txns, PackageValidationState& state)
+{
+    const unsigned int package_count = txns.size();
+
+    if (package_count > MAX_PACKAGE_COUNT) {
+        return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-many-transactions");
+    }
+
+    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) {
+        return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-large");
+    }
+
+    // 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).
+    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(); });
+    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.
+                return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted");
+            }
+        }
+        later_txids.erase(tx->GetHash());
+    }
+
+    // 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.
+                return state.Invalid(PackageValidationResult::PCKG_POLICY, "conflict-in-package");
+            }
+        }
+        // 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; });
+    }
+    return true;
+}
diff --git a/src/policy/packages.h b/src/policy/packages.h
index 4b1463dcb3..6b7ac3e450 100644
--- a/src/policy/packages.h
+++ b/src/policy/packages.h
@@ -6,6 +6,7 @@
 #define BITCOIN_POLICY_PACKAGES_H
 
 #include <consensus/validation.h>
+#include <policy/policy.h>
 #include <primitives/transaction.h>
 
 #include <vector>
@@ -14,6 +15,7 @@
 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};
+static_assert(MAX_PACKAGE_SIZE * WITNESS_SCALE_FACTOR * 1000 >= MAX_STANDARD_TX_WEIGHT);
 
 /** 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.
@@ -31,4 +33,12 @@ using Package = std::vector<CTransactionRef>;
 
 class PackageValidationState : public ValidationState<PackageValidationResult> {};
 
+/** Context-free package policy checks:
+ * 1. The number of transactions cannot exceed MAX_PACKAGE_COUNT.
+ * 2. The total virtual size cannot exceed MAX_PACKAGE_SIZE.
+ * 3. If any dependencies exist between transactions, parents must appear before children.
+ * 4. Transactions cannot conflict, i.e., spend the same inputs.
+ */
+bool CheckPackage(const Package& txns, PackageValidationState& state);
+
 #endif // BITCOIN_POLICY_PACKAGES_H
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 339d711ac9..414c6637a5 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -889,7 +889,7 @@ static RPCHelpMan testmempoolaccept()
                 "\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"
+                "\nThe maximum number of transactions allowed is " + ToString(MAX_PACKAGE_COUNT) + ".\n"
                 "\nThis checks if transactions violate the consensus or policy rules.\n"
                 "\nSee sendrawtransaction call.\n",
                 {
@@ -905,7 +905,7 @@ static RPCHelpMan testmempoolaccept()
                 RPCResult{
                     RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n"
                         "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",
+                        "It is possible for transactions to not be fully validated ('allowed' unset) if another transaction failed.\n",
                     {
                         {RPCResult::Type::OBJ, "", "",
                         {
@@ -939,7 +939,6 @@ static RPCHelpMan testmempoolaccept()
         UniValue::VARR,
         UniValueType(), // VNUM or VSTR, checked inside AmountFromValue()
     });
-
     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,
@@ -951,6 +950,7 @@ static RPCHelpMan testmempoolaccept()
                                              CFeeRate(AmountFromValue(request.params[1]));
 
     std::vector<CTransactionRef> txns;
+    txns.reserve(raw_transactions.size());
     for (const auto& rawtx : raw_transactions.getValues()) {
         CMutableTransaction mtx;
         if (!DecodeHexTx(mtx, rawtx.get_str())) {
@@ -971,8 +971,8 @@ static RPCHelpMan testmempoolaccept()
     }();
 
     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
+    // We will check transaction fees while we iterate through txns in order. If any transaction fee
+    // exceeds maxfeerate, we will leave 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};
diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
index c47d0eae1e..e54948a840 100644
--- a/src/test/miner_tests.cpp
+++ b/src/test/miner_tests.cpp
@@ -28,8 +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)
     {
-        CCoinsViewMemPool viewMempool(&m_node.chainman->ActiveChainstate().CoinsTip(), *m_node.mempool);
-        return CheckSequenceLocks(m_node.chainman->ActiveChain().Tip(), viewMempool, tx, flags);
+        CCoinsViewMemPool view_mempool(&m_node.chainman->ActiveChainstate().CoinsTip(), *m_node.mempool);
+        return CheckSequenceLocks(m_node.chainman->ActiveChain().Tip(), view_mempool, tx, flags);
     }
     BlockAssembler AssemblerForTest(const CChainParams& params);
 };
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 4413da7ea7..7c73a2d78d 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -515,9 +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);
-        CCoinsViewMemPool viewMempool(&active_chainstate.CoinsTip(), *this);
+        CCoinsViewMemPool view_mempool(&active_chainstate.CoinsTip(), *this);
         if (!CheckFinalTx(active_chainstate.m_chain.Tip(), tx, flags)
-            || !CheckSequenceLocks(active_chainstate.m_chain.Tip(), viewMempool, tx, flags, &lp, validLP)) {
+            || !CheckSequenceLocks(active_chainstate.m_chain.Tip(), view_mempool, 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);
diff --git a/src/txmempool.h b/src/txmempool.h
index 46b89049bb..ae4b16d377 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -874,7 +874,8 @@ protected:
 public:
     CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn);
     bool GetCoin(const COutPoint &outpoint, Coin &coin) const override;
-    /** Add the coins created by this transaction. */
+    /** Add the coins created by this transaction. These coins are only temporarily stored in
+     * m_temp_added and cannot be flushed to the back end. Only used for package validation. */
     void PackageAddTransaction(const CTransactionRef& tx);
 };
 
diff --git a/src/validation.cpp b/src/validation.cpp
index 5e3d429c2e..d5c4be6811 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -472,8 +472,10 @@ 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;
+        /** Whether we allow transactions to replace mempool transactions by BIP125 rules. If false,
+         * any transaction spending the same inputs as a transaction in the mempool is considered
+         * a conflict. */
+        const bool m_allow_bip125_replacement{true};
     };
 
     // Single transaction acceptance
@@ -482,7 +484,7 @@ public:
     /**
     * 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.
+    * dependencies exist.
     */
     PackageMempoolAcceptResult AcceptMultipleTransactions(const std::vector<CTransactionRef>& txns, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
@@ -619,6 +621,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     {
         const CTransaction* ptxConflicting = m_pool.GetConflictTx(txin.prevout);
         if (ptxConflicting) {
+            if (!args.m_allow_bip125_replacement) {
+                // Transaction conflicts with a mempool tx, but we're not allowing replacements.
+                return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "bip125-replacement-disallowed");
+            }
             if (!setConflicts.count(ptxConflicting->GetHash()))
             {
                 // Allow opt-out of transaction replacement by setting
@@ -645,7 +651,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
                         break;
                     }
                 }
-                if (fReplacementOptOut || args.disallow_mempool_conflicts) {
+                if (fReplacementOptOut) {
                     return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict");
                 }
 
@@ -1080,65 +1086,15 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
 {
     AssertLockHeld(cs_main);
 
+    // These context-free package limits can be done before taking the mempool lock.
     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, {});
-    }
+    if (!CheckPackage(txns, package_state)) 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));
-        }
-    }
+    workspaces.reserve(txns.size());
+    std::transform(txns.cbegin(), txns.cend(), std::back_inserter(workspaces),
+                   [](const auto& tx) { return 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);
 
@@ -1151,10 +1107,10 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
             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);
+        // package to spend. Since we already checked conflicts in the package and we don't allow
+        // replacements, we don't need to track the coins spent. Note that this logic will need to be
+        // updated if package replace-by-fee is allowed in the future.
+        assert(!args.m_allow_bip125_replacement);
         m_viewmempool.PackageAddTransaction(ws.m_ptx);
     }
 
@@ -1188,7 +1144,7 @@ static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainp
 {
     std::vector<COutPoint> coins_to_uncache;
     MemPoolAccept::ATMPArgs args { chainparams, nAcceptTime, bypass_limits, coins_to_uncache,
-                                   test_accept, /* disallow_mempool_conflicts */ false };
+                                   test_accept, /* m_allow_bip125_replacement */ true };
 
     assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate));
     const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args);
@@ -1225,12 +1181,11 @@ PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTx
     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 };
+                                   test_accept, /* m_allow_bip125_replacement */ false };
     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);
     }
diff --git a/src/validation.h b/src/validation.h
index 359a6c779f..0c396c92ee 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -234,11 +234,13 @@ MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPoo
                                        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.
+* Atomically test acceptance of a package. If the package only contains one tx, package rules still
+* apply. Package validation does not allow BIP125 replacements, so the transaction(s) cannot spend
+* the same inputs as any transaction in the mempool.
 * @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.
+*                                   parent-child dependencies. The transactions must not conflict
+*                                   with each other, i.e., must not spend the same inputs. If any
+*                                   dependencies exist, 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.
 */
@@ -269,9 +271,13 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) EXCLUSIVE
  * 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.
+ * @param[in]   coins_view      Any CCoinsView that provides access to the relevant coins for
+ *                              checking sequence locks. For example, it can be a CCoinsViewCache
+ *                              that isn't connected to anything but contains all the relevant
+ *                              coins, or a CCoinsViewMemPool that is connected to the
+ *                              mempool and chainstate UTXO set. In the latter case, the caller is
+ *                              responsible for holding the appropriate locks to ensure that
+ *                              calls to GetCoin() return correct coins.
  * 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
diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py
index 3d8d81d6b8..d0848d9bd6 100755
--- a/test/functional/rpc_packages.py
+++ b/test/functional/rpc_packages.py
@@ -354,7 +354,8 @@ class RPCPackagesTest(BitcoinTestFramework):
         # 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"
+            "txid": replacement_tx.rehash(), "wtxid": replacement_tx.getwtxid(), "allowed": False,
+            "reject-reason": "bip125-replacement-disallowed"
         }]
         self.assert_testres_equal(self.independent_txns_hex + [signed_replacement_tx["hex"]], testres_rbf_package)