Merge bitcoin/bitcoin#28758: refactors for subpackage evaluation

b5a60abe87 MOVEONLY: CleanupTemporaryCoins into its own function (glozow)
10c0a8678c [test util] CreateValidTransaction multi-in/out, configurable feerate, signal BIP125 (glozow)
6ff647a7e0 scripted-diff: rename CheckPackage to IsWellFormedPackage (glozow)
da9aceba21 [refactor] move package checks into helper functions (glozow)

Pull request description:

  This is part of #27463. It splits off the more trivial changes from #26711 for ease of review, as requested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253.

  - Split package sanitization in policy/packages.h into helper functions
    - Add some tests for its quirks (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1340521597)
  - Rename `CheckPackage` to `IsPackageWellFormed`
  - Improve the `CreateValidTransaction` unit test utility to:
    - Configure the target feerate and return the fee paid
    - Signal BIP125 on transactions to enable RBF tests
    - Allow the specification of multiple inputs and outputs
  - Move `CleanupTemporaryCoins` into its own function to be reused later without duplication

ACKs for top commit:
  dergoegge:
    Code review ACK b5a60abe87
  instagibbs:
    ACK b5a60abe87

Tree-SHA512: 39d67a5f0041e381f0d0f802a98ccffbff11e44daa3a49611189d6306b03f18613d5ff16c618898d490c97a216753e99e0db231ff14d327f92c17ae4d269cfec

src/policy/packages.cpp

@@ -6,16 +6,77 @@
 #include <policy/policy.h>
 #include <primitives/transaction.h>
 #include <uint256.h>
-#include <util/hasher.h>
+#include <util/check.h>
 
 #include <algorithm>
 #include <cassert>
 #include <iterator>
 #include <memory>
 #include <numeric>
-#include <unordered_set>
 
-bool CheckPackage(const Package& txns, PackageValidationState& state)
+/** IsTopoSortedPackage where a set of txids has been pre-populated. The set is assumed to be correct and
+ * is mutated within this function (even if return value is false). */
+bool IsTopoSortedPackage(const Package& txns, std::unordered_set<uint256, SaltedTxidHasher>& later_txids)
+{
+    // Avoid misusing this function: later_txids should contain the txids of txns.
+    Assume(txns.size() == later_txids.size());
+
+    // later_txids always contains the txids of this transaction and the ones that come later in
+    // txns. If any transaction's input spends a tx in that set, we've found a parent placed later
+    // than its child.
+    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 false;
+            }
+        }
+        // Avoid misusing this function: later_txids must contain every tx.
+        Assume(later_txids.erase(tx->GetHash()) == 1);
+    }
+
+    // Avoid misusing this function: later_txids should have contained the txids of txns.
+    Assume(later_txids.empty());
+    return true;
+}
+
+bool IsTopoSortedPackage(const Package& txns)
+{
+    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(); });
+
+    return IsTopoSortedPackage(txns, later_txids);
+}
+
+bool IsConsistentPackage(const Package& txns)
+{
+    // 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) {
+        if (tx->vin.empty()) {
+            // This function checks consistency based on inputs, and we can't do that if there are
+            // no inputs. Duplicate empty transactions are also not consistent with one another.
+            // This doesn't create false negatives, as unconfirmed transactions are not allowed to
+            // have no inputs.
+            return false;
+        }
+        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 false;
+            }
+        }
+        // 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;
+}
+
+bool IsWellFormedPackage(const Package& txns, PackageValidationState& state, bool require_sorted)
 {
     const unsigned int package_count = txns.size();
 
@@ -30,10 +91,6 @@ bool CheckPackage(const Package& txns, PackageValidationState& state)
         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(); });
@@ -44,31 +101,18 @@ bool CheckPackage(const Package& txns, PackageValidationState& state)
         return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-contains-duplicates");
     }
 
-    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.
+    // 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).
+    if (require_sorted && !IsTopoSortedPackage(txns, later_txids)) {
         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.
+    if (!IsConsistentPackage(txns)) {
         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;
 }
 

src/policy/packages.h

@@ -9,8 +9,10 @@
 #include <consensus/validation.h>
 #include <policy/policy.h>
 #include <primitives/transaction.h>
+#include <util/hasher.h>
 
 #include <cstdint>
+#include <unordered_set>
 #include <vector>
 
 /** Default maximum number of transactions in a package. */
@@ -49,13 +51,32 @@ using Package = std::vector<CTransactionRef>;
 
 class PackageValidationState : public ValidationState<PackageValidationResult> {};
 
+/** If any direct dependencies exist between transactions (i.e. a child spending the output of a
+ * parent), checks that all parents appear somewhere in the list before their respective children.
+ * No other ordering is enforced. This function cannot detect indirect dependencies (e.g. a
+ * transaction's grandparent if its parent is not present).
+ * @returns true if sorted. False if any tx spends the output of a tx that appears later in txns.
+ */
+bool IsTopoSortedPackage(const Package& txns);
+
+/** Checks that these transactions don't conflict, i.e., spend the same prevout. This includes
+ * checking that there are no duplicate transactions. Since these checks require looking at the inputs
+ * of a transaction, returns false immediately if any transactions have empty vin.
+ *
+ * Does not check consistency of a transaction with oneself; does not check if a transaction spends
+ * the same prevout multiple times (see bad-txns-inputs-duplicate in CheckTransaction()).
+ *
+ * @returns true if there are no conflicts. False if any two transactions spend the same prevout.
+ * */
+bool IsConsistentPackage(const Package& txns);
+
 /** Context-free package policy checks:
  * 1. The number of transactions cannot exceed MAX_PACKAGE_COUNT.
  * 2. The total weight cannot exceed MAX_PACKAGE_WEIGHT.
  * 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);
+bool IsWellFormedPackage(const Package& txns, PackageValidationState& state, bool require_sorted);
 
 /** Context-free check that a package is exactly one child and its parents; not all parents need to
  * be present, but the package must not contain any transactions that are not the child's parents.

src/test/txpackage_tests.cpp

@@ -9,6 +9,7 @@
 #include <primitives/transaction.h>
 #include <script/script.h>
 #include <test/util/random.h>
+#include <test/util/script.h>
 #include <test/util/setup_common.h>
 #include <validation.h>
 
@@ -47,7 +48,7 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup)
         package_too_many.emplace_back(create_placeholder_tx(1, 1));
     }
     PackageValidationState state_too_many;
-    BOOST_CHECK(!CheckPackage(package_too_many, state_too_many));
+    BOOST_CHECK(!IsWellFormedPackage(package_too_many, state_too_many, /*require_sorted=*/true));
     BOOST_CHECK_EQUAL(state_too_many.GetResult(), PackageValidationResult::PCKG_POLICY);
     BOOST_CHECK_EQUAL(state_too_many.GetRejectReason(), "package-too-many-transactions");
 
@@ -62,7 +63,7 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup)
     }
     BOOST_CHECK(package_too_large.size() <= MAX_PACKAGE_COUNT);
     PackageValidationState state_too_large;
-    BOOST_CHECK(!CheckPackage(package_too_large, state_too_large));
+    BOOST_CHECK(!IsWellFormedPackage(package_too_large, state_too_large, /*require_sorted=*/true));
     BOOST_CHECK_EQUAL(state_too_large.GetResult(), PackageValidationResult::PCKG_POLICY);
     BOOST_CHECK_EQUAL(state_too_large.GetRejectReason(), "package-too-large");
 
@@ -73,9 +74,39 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup)
         package_duplicate_txids_empty.emplace_back(MakeTransactionRef(empty_tx));
     }
     PackageValidationState state_duplicates;
-    BOOST_CHECK(!CheckPackage(package_duplicate_txids_empty, state_duplicates));
+    BOOST_CHECK(!IsWellFormedPackage(package_duplicate_txids_empty, state_duplicates, /*require_sorted=*/true));
     BOOST_CHECK_EQUAL(state_duplicates.GetResult(), PackageValidationResult::PCKG_POLICY);
     BOOST_CHECK_EQUAL(state_duplicates.GetRejectReason(), "package-contains-duplicates");
+    BOOST_CHECK(!IsConsistentPackage(package_duplicate_txids_empty));
+
+    // Packages can't have transactions spending the same prevout
+    CMutableTransaction tx_zero_1;
+    CMutableTransaction tx_zero_2;
+    COutPoint same_prevout{InsecureRand256(), 0};
+    tx_zero_1.vin.emplace_back(same_prevout);
+    tx_zero_2.vin.emplace_back(same_prevout);
+    // Different vouts (not the same tx)
+    tx_zero_1.vout.emplace_back(CENT, P2WSH_OP_TRUE);
+    tx_zero_2.vout.emplace_back(2 * CENT, P2WSH_OP_TRUE);
+    Package package_conflicts{MakeTransactionRef(tx_zero_1), MakeTransactionRef(tx_zero_2)};
+    BOOST_CHECK(!IsConsistentPackage(package_conflicts));
+    // Transactions are considered sorted when they have no dependencies.
+    BOOST_CHECK(IsTopoSortedPackage(package_conflicts));
+    PackageValidationState state_conflicts;
+    BOOST_CHECK(!IsWellFormedPackage(package_conflicts, state_conflicts, /*require_sorted=*/true));
+    BOOST_CHECK_EQUAL(state_conflicts.GetResult(), PackageValidationResult::PCKG_POLICY);
+    BOOST_CHECK_EQUAL(state_conflicts.GetRejectReason(), "conflict-in-package");
+
+    // IsConsistentPackage only cares about conflicts between transactions, not about a transaction
+    // conflicting with itself (i.e. duplicate prevouts in vin).
+    CMutableTransaction dup_tx;
+    const COutPoint rand_prevout{InsecureRand256(), 0};
+    dup_tx.vin.emplace_back(rand_prevout);
+    dup_tx.vin.emplace_back(rand_prevout);
+    Package package_with_dup_tx{MakeTransactionRef(dup_tx)};
+    BOOST_CHECK(IsConsistentPackage(package_with_dup_tx));
+    package_with_dup_tx.emplace_back(create_placeholder_tx(1, 1));
+    BOOST_CHECK(IsConsistentPackage(package_with_dup_tx));
 }
 
 BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup)
@@ -157,8 +188,8 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup)
         CTransactionRef tx_child = MakeTransactionRef(mtx_child);
 
         PackageValidationState state;
-        BOOST_CHECK(CheckPackage({tx_parent, tx_child}, state));
-        BOOST_CHECK(!CheckPackage({tx_child, tx_parent}, state));
+        BOOST_CHECK(IsWellFormedPackage({tx_parent, tx_child}, state, /*require_sorted=*/true));
+        BOOST_CHECK(!IsWellFormedPackage({tx_child, tx_parent}, state, /*require_sorted=*/true));
         BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY);
         BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted");
         BOOST_CHECK(IsChildWithParents({tx_parent, tx_child}));
@@ -186,7 +217,7 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup)
         package.push_back(MakeTransactionRef(child));
 
         PackageValidationState state;
-        BOOST_CHECK(CheckPackage(package, state));
+        BOOST_CHECK(IsWellFormedPackage(package, state, /*require_sorted=*/true));
         BOOST_CHECK(IsChildWithParents(package));
         BOOST_CHECK(IsChildWithParentsTree(package));
 
@@ -224,8 +255,8 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup)
         BOOST_CHECK(!IsChildWithParentsTree({tx_parent, tx_parent_also_child, tx_child}));
         // IsChildWithParents does not detect unsorted parents.
         BOOST_CHECK(IsChildWithParents({tx_parent_also_child, tx_parent, tx_child}));
-        BOOST_CHECK(CheckPackage({tx_parent, tx_parent_also_child, tx_child}, state));
-        BOOST_CHECK(!CheckPackage({tx_parent_also_child, tx_parent, tx_child}, state));
+        BOOST_CHECK(IsWellFormedPackage({tx_parent, tx_parent_also_child, tx_child}, state, /*require_sorted=*/true));
+        BOOST_CHECK(!IsWellFormedPackage({tx_parent_also_child, tx_parent, tx_child}, state, /*require_sorted=*/true));
         BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY);
         BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted");
     }

src/test/util/setup_common.cpp

@@ -49,6 +49,7 @@
 #include <txdb.h>
 #include <txmempool.h>
 #include <util/chaintype.h>
+#include <util/rbf.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/thread.h>
@@ -336,56 +337,106 @@ CBlock TestChain100Setup::CreateAndProcessBlock(
     return block;
 }
 
-
-CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactionRef input_transaction,
-                                                                     int input_vout,
+std::pair<CMutableTransaction, CAmount> TestChain100Setup::CreateValidTransaction(const std::vector<CTransactionRef>& input_transactions,
+                                                                                  const std::vector<COutPoint>& inputs,
                                                                                   int input_height,
-                                                                     CKey input_signing_key,
-                                                                     CScript output_destination,
-                                                                     CAmount output_amount,
-                                                                     bool submit)
+                                                                                  const std::vector<CKey>& input_signing_keys,
+                                                                                  const std::vector<CTxOut>& outputs,
+                                                                                  const std::optional<CFeeRate>& feerate,
+                                                                                  const std::optional<uint32_t>& fee_output)
 {
-    // Transaction we will submit to the mempool
     CMutableTransaction mempool_txn;
+    mempool_txn.vin.reserve(inputs.size());
+    mempool_txn.vout.reserve(outputs.size());
 
-    // Create an input
-    COutPoint outpoint_to_spend(input_transaction->GetHash(), input_vout);
-    CTxIn input(outpoint_to_spend);
-    mempool_txn.vin.push_back(input);
+    for (const auto& outpoint : inputs) {
+        mempool_txn.vin.emplace_back(outpoint, CScript(), MAX_BIP125_RBF_SEQUENCE);
+    }
+    mempool_txn.vout = outputs;
 
-    // Create an output
-    CTxOut output(output_amount, output_destination);
-    mempool_txn.vout.push_back(output);
-
-    // Sign the transaction
     // - Add the signing key to a keystore
     FillableSigningProvider keystore;
+    for (const auto& input_signing_key : input_signing_keys) {
         keystore.AddKey(input_signing_key);
+    }
     // - Populate a CoinsViewCache with the unspent output
     CCoinsView coins_view;
     CCoinsViewCache coins_cache(&coins_view);
+    for (const auto& input_transaction : input_transactions) {
         AddCoins(coins_cache, *input_transaction.get(), input_height);
+    }
+    // Build Outpoint to Coin map for SignTransaction
+    std::map<COutPoint, Coin> input_coins;
+    CAmount inputs_amount{0};
+    for (const auto& outpoint_to_spend : inputs) {
         // - Use GetCoin to properly populate utxo_to_spend,
         Coin utxo_to_spend;
         assert(coins_cache.GetCoin(outpoint_to_spend, utxo_to_spend));
-    // - Then add it to a map to pass in to SignTransaction
-    std::map<COutPoint, Coin> input_coins;
         input_coins.insert({outpoint_to_spend, utxo_to_spend});
+        inputs_amount += utxo_to_spend.out.nValue;
+    }
     // - Default signature hashing type
     int nHashType = SIGHASH_ALL;
     std::map<int, bilingual_str> input_errors;
     assert(SignTransaction(mempool_txn, &keystore, input_coins, nHashType, input_errors));
+    CAmount current_fee = inputs_amount - std::accumulate(outputs.begin(), outputs.end(), CAmount(0),
+        [](const CAmount& acc, const CTxOut& out) {
+        return acc + out.nValue;
+    });
+    // Deduct fees from fee_output to meet feerate if set
+    if (feerate.has_value()) {
+        assert(fee_output.has_value());
+        assert(fee_output.value() < mempool_txn.vout.size());
+        CAmount target_fee = feerate.value().GetFee(GetVirtualTransactionSize(CTransaction{mempool_txn}));
+        CAmount deduction = target_fee - current_fee;
+        if (deduction > 0) {
+            // Only deduct fee if there's anything to deduct. If the caller has put more fees than
+            // the target feerate, don't change the fee.
+            mempool_txn.vout[fee_output.value()].nValue -= deduction;
+            // Re-sign since an output has changed
+            input_errors.clear();
+            assert(SignTransaction(mempool_txn, &keystore, input_coins, nHashType, input_errors));
+            current_fee = target_fee;
+        }
+    }
+    return {mempool_txn, current_fee};
+}
 
+CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(const std::vector<CTransactionRef>& input_transactions,
+                                                                     const std::vector<COutPoint>& inputs,
+                                                                     int input_height,
+                                                                     const std::vector<CKey>& input_signing_keys,
+                                                                     const std::vector<CTxOut>& outputs,
+                                                                     bool submit)
+{
+    CMutableTransaction mempool_txn = CreateValidTransaction(input_transactions, inputs, input_height, input_signing_keys, outputs, std::nullopt, std::nullopt).first;
     // If submit=true, add transaction to the mempool.
     if (submit) {
         LOCK(cs_main);
         const MempoolAcceptResult result = m_node.chainman->ProcessTransaction(MakeTransactionRef(mempool_txn));
         assert(result.m_result_type == MempoolAcceptResult::ResultType::VALID);
     }
-
     return mempool_txn;
 }
 
+CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactionRef input_transaction,
+                                                                     uint32_t input_vout,
+                                                                     int input_height,
+                                                                     CKey input_signing_key,
+                                                                     CScript output_destination,
+                                                                     CAmount output_amount,
+                                                                     bool submit)
+{
+    COutPoint input{input_transaction->GetHash(), input_vout};
+    CTxOut output{output_amount, output_destination};
+    return CreateValidMempoolTransaction(/*input_transactions=*/{input_transaction},
+                                         /*inputs=*/{input},
+                                         /*input_height=*/input_height,
+                                         /*input_signing_keys=*/{input_signing_key},
+                                         /*outputs=*/{output},
+                                         /*submit=*/submit);
+}
+
 std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContext& det_rand, size_t num_transactions, bool submit)
 {
     std::vector<CTransactionRef> mempool_transactions;

src/test/util/setup_common.h

@@ -124,7 +124,44 @@ struct TestChain100Setup : public TestingSetup {
     void mineBlocks(int num_blocks);
 
     /**
-     * Create a transaction and submit to the mempool.
+    * Create a transaction, optionally setting the fee based on the feerate.
+    * Note: The feerate may not be met exactly depending on whether the signatures can have different sizes.
+    *
+    * @param input_transactions   The transactions to spend
+    * @param inputs               Outpoints with which to construct transaction vin.
+    * @param input_height         The height of the block that included the input transactions.
+    * @param input_signing_keys   The keys to spend the input transactions.
+    * @param outputs              Transaction vout.
+    * @param feerate              The feerate the transaction should pay.
+    * @param fee_output           The index of the output to take the fee from.
+    * @return The transaction and the fee it pays
+    */
+    std::pair<CMutableTransaction, CAmount> CreateValidTransaction(const std::vector<CTransactionRef>& input_transactions,
+                                                                   const std::vector<COutPoint>& inputs,
+                                                                   int input_height,
+                                                                   const std::vector<CKey>& input_signing_keys,
+                                                                   const std::vector<CTxOut>& outputs,
+                                                                   const std::optional<CFeeRate>& feerate,
+                                                                   const std::optional<uint32_t>& fee_output);
+    /**
+     * Create a transaction and, optionally, submit to the mempool.
+     *
+     * @param input_transactions   The transactions to spend
+     * @param inputs               Outpoints with which to construct transaction vin.
+     * @param input_height         The height of the block that included the input transaction(s).
+     * @param input_signing_keys   The keys to spend inputs.
+     * @param outputs              Transaction vout.
+     * @param submit               Whether or not to submit to mempool
+     */
+    CMutableTransaction CreateValidMempoolTransaction(const std::vector<CTransactionRef>& input_transactions,
+                                                      const std::vector<COutPoint>& inputs,
+                                                      int input_height,
+                                                      const std::vector<CKey>& input_signing_keys,
+                                                      const std::vector<CTxOut>& outputs,
+                                                      bool submit = true);
+
+    /**
+     * Create a 1-in-1-out transaction and, optionally, submit to the mempool.
      *
      * @param input_transaction  The transaction to spend
      * @param input_vout         The vout to spend from the input_transaction
@@ -135,7 +172,7 @@ struct TestChain100Setup : public TestingSetup {
      * @param submit             Whether or not to submit to mempool
      */
     CMutableTransaction CreateValidMempoolTransaction(CTransactionRef input_transaction,
-                                                      int input_vout,
+                                                      uint32_t input_vout,
                                                       int input_height,
                                                       CKey input_signing_key,
                                                       CScript output_destination,

src/validation.cpp

@@ -546,6 +546,9 @@ public:
         }
     };
 
+    /** Clean up all non-chainstate coins from m_view and m_viewmempool. */
+    void CleanupTemporaryCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs);
+
     // Single transaction acceptance
     MempoolAcceptResult AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
@@ -1256,7 +1259,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
 
     // These context-free package limits can be done before taking the mempool lock.
     PackageValidationState package_state;
-    if (!CheckPackage(txns, package_state)) return PackageMempoolAcceptResult(package_state, {});
+    if (!IsWellFormedPackage(txns, package_state, /*require_sorted=*/true)) return PackageMempoolAcceptResult(package_state, {});
 
     std::vector<Workspace> workspaces{};
     workspaces.reserve(txns.size());
@@ -1345,26 +1348,8 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
     return PackageMempoolAcceptResult(package_state, std::move(results));
 }
 
-PackageMempoolAcceptResult MemPoolAccept::AcceptSubPackage(const std::vector<CTransactionRef>& subpackage, ATMPArgs& args)
+void MemPoolAccept::CleanupTemporaryCoins()
 {
-    AssertLockHeld(::cs_main);
-    AssertLockHeld(m_pool.cs);
-    auto result = [&]() EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_pool.cs) {
-        if (subpackage.size() > 1) {
-            return AcceptMultipleTransactions(subpackage, args);
-        }
-        const auto& tx = subpackage.front();
-        ATMPArgs single_args = ATMPArgs::SingleInPackageAccept(args);
-        const auto single_res = AcceptSingleTransaction(tx, single_args);
-        PackageValidationState package_state_wrapped;
-        if (single_res.m_result_type != MempoolAcceptResult::ResultType::VALID) {
-            package_state_wrapped.Invalid(PackageValidationResult::PCKG_TX, "transaction failed");
-        }
-        return PackageMempoolAcceptResult(package_state_wrapped, {{tx->GetWitnessHash(), single_res}});
-    }();
-    // Clean up m_view and m_viewmempool so that other subpackage evaluations don't have access to
-    // coins they shouldn't. Keep some coins in order to minimize re-fetching coins from the UTXO set.
-    //
     // There are 3 kinds of coins in m_view:
     // (1) Temporary coins from the transactions in subpackage, constructed by m_viewmempool.
     // (2) Mempool coins from transactions in the mempool, constructed by m_viewmempool.
@@ -1390,6 +1375,30 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptSubPackage(const std::vector<CTr
     }
     // This deletes the temporary and mempool coins.
     m_viewmempool.Reset();
+}
+
+PackageMempoolAcceptResult MemPoolAccept::AcceptSubPackage(const std::vector<CTransactionRef>& subpackage, ATMPArgs& args)
+{
+    AssertLockHeld(::cs_main);
+    AssertLockHeld(m_pool.cs);
+    auto result = [&]() EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_pool.cs) {
+        if (subpackage.size() > 1) {
+            return AcceptMultipleTransactions(subpackage, args);
+        }
+        const auto& tx = subpackage.front();
+        ATMPArgs single_args = ATMPArgs::SingleInPackageAccept(args);
+        const auto single_res = AcceptSingleTransaction(tx, single_args);
+        PackageValidationState package_state_wrapped;
+        if (single_res.m_result_type != MempoolAcceptResult::ResultType::VALID) {
+            package_state_wrapped.Invalid(PackageValidationResult::PCKG_TX, "transaction failed");
+        }
+        return PackageMempoolAcceptResult(package_state_wrapped, {{tx->GetWitnessHash(), single_res}});
+    }();
+
+    // Clean up m_view and m_viewmempool so that other subpackage evaluations don't have access to
+    // coins they shouldn't. Keep some coins in order to minimize re-fetching coins from the UTXO set.
+    CleanupTemporaryCoins();
+
     return result;
 }
 
@@ -1403,7 +1412,9 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
     // transactions and thus won't return any MempoolAcceptResults, just a package-wide error.
 
     // Context-free package checks.
-    if (!CheckPackage(package, package_state_quit_early)) return PackageMempoolAcceptResult(package_state_quit_early, {});
+    if (!IsWellFormedPackage(package, package_state_quit_early, /*require_sorted=*/true)) {
+        return PackageMempoolAcceptResult(package_state_quit_early, {});
+    }
 
     // All transactions in the package must be a parent of the last transaction. This is just an
     // opportunity for us to fail fast on a context-free check without taking the mempool lock.