Merge bitcoin/bitcoin#32973: validation: docs and cleanups for MemPoolAccept coins views

b6d4688 [doc] reword comments in test_mid_package_replacement (glozow)
f3a613a [cleanup] delete brittle test_mid_package_eviction (glozow)
c3cd7fc [doc] remove references to now-nonexistent Finalize() function (glozow)
d8140f5 don't make a copy of m_non_base_coins (glozow)
98ba2b1 [doc] MemPoolAccept coins views (glozow)
ba02c30 [doc] always CleanupTemporaryCoins after a mempool trim (glozow)

Pull request description:

  Deletes `test_mid_package_eviction` that is brittle and already covered in other places. It was introduced in #28251 addressing 2 issues: (1) calling `LimitMempoolSize()` in the middle of package validation and (2) not updating coins view cache when the mempool contents change, leading to "disappearing coins."

  (1) If you let `AcceptSingleTransaction` call `LimitMempoolSize` in the middle of package validation, you should get a failure in `test_mid_package_eviction_success` (the package is rejected):
  ```
  diff --git a/src/validation.cpp b/src/validation.cpp
  index f2f6098..4bd6f059849 100644
  --- a/src/validation.cpp
  +++ b/src/validation.cpp
  @@ -1485,7 +1485,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
       FinalizeSubpackage(args);

       // Limit the mempool, if appropriate.
  -    if (!args.m_package_submission && !args.m_bypass_limits) {
  +    if (!args.m_bypass_limits) {
           LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip());
           // If mempool contents change, then the m_view cache is dirty. Given this isn't a package
           // submission, we won't be using the cache anymore, but clear it anyway for clarity.
  ```
  Mempool modifications have a pretty narrow interface since #31122 and `TrimToSize()` cannot be called while there is an outstanding mempool changeset. So I think there is a low likelihood of accidentally reintroducing this problem and not immediately hitting e.g. a fuzzer crash on this line https://github.com/bitcoin/bitcoin/blob/b53fab1467fde73c40402e2022b25edfff1e4668/src/txmempool.cpp#L1143

  (2) If you remove the `CleanupTemporaryCoins()` call from `ClearSubPackageState()` you should get a failure from `test_mid_package_replacement`:
  ```
  diff --git a/src/validation.cpp b/src/validation.cpp
  index f2f6098..01b904b69ef 100644
  --- a/src/validation.cpp
  +++ b/src/validation.cpp
  @@ -779,7 +779,7 @@ private:
           m_subpackage = SubPackageState{};

           // And clean coins while at it
  -        CleanupTemporaryCoins();
  +        // CleanupTemporaryCoins();
       }
   };
  ```
  I also added/cleaned up the documentation about coins views to hopefully make it extremely clear when people should `CleanupTemporaryCoins`.

ACKs for top commit:
  instagibbs:
    reACK bitcoin/bitcoin@b6d4688
  sdaftuar:
    utACK b6d4688
  marcofleon:
    ACK b6d4688

Tree-SHA512: 79c68e263013b1153520f5453e6b579b8fe7e1d6a9952b1ac2c3c3c017034e6d21d7000a140bba4cc9d2ce50ea3a84cc6f91fd5febc52d7b3fa4f797955d987d

Author: fanquake

src/txmempool.h

@@ -940,7 +940,7 @@ class CCoinsViewMemPool : public CCoinsViewBacked
      * m_temp_added and cannot be flushed to the back end. Only used for package validation. */
     void PackageAddTransaction(const CTransactionRef& tx);
     /** Get all coins in m_non_base_coins. */
-    std::unordered_set<COutPoint, SaltedOutpointHasher> GetNonBaseCoins() const { return m_non_base_coins; }
+    const std::unordered_set<COutPoint, SaltedOutpointHasher>& GetNonBaseCoins() const { return m_non_base_coins; }
     /** Clear m_temp_added and m_non_base_coins. */
     void Reset();
 };

src/validation.cpp

@@ -469,10 +469,8 @@ class MemPoolAccept
         const bool m_allow_replacement;
         /** When true, allow sibling eviction. This only occurs in single transaction package settings. */
         const bool m_allow_sibling_eviction;
-        /** When true, the mempool will not be trimmed when any transactions are submitted in
-         * Finalize(). Instead, limits should be enforced at the end to ensure the package is not
-         * partially submitted.
-         */
+        /** Used to skip the LimitMempoolSize() call within AcceptSingleTransaction(). This should be used when multiple
+         * AcceptSubPackage calls are expected and the mempool will be trimmed at the end of AcceptPackage(). */
         const bool m_package_submission;
         /** When true, use package feerates instead of individual transaction feerates for fee-based
          * policies such as mempool min fee and min relay fee.
@@ -548,7 +546,7 @@ class MemPoolAccept
                             /* m_test_accept */ package_args.m_test_accept,
                             /* m_allow_replacement */ true,
                             /* m_allow_sibling_eviction */ true,
-                            /* m_package_submission */ true, // do not LimitMempoolSize in Finalize()
+                            /* m_package_submission */ true, // trim at the end of AcceptPackage()
                             /* m_package_feerates */ false, // only 1 transaction
                             /* m_client_maxfeerate */ package_args.m_client_maxfeerate,
                             /* m_allow_carveouts */ false,
@@ -725,8 +723,27 @@ class MemPoolAccept
 
 private:
     CTxMemPool& m_pool;
+
+    /** Holds a cached view of available coins from the UTXO set, mempool, and artificial temporary coins (to enable package validation).
+     * The view doesn't track whether a coin previously existed but has now been spent. We detect conflicts in other ways:
+     * - conflicts within a transaction are checked in CheckTransaction (bad-txns-inputs-duplicate)
+     * - conflicts within a package are checked in IsWellFormedPackage (conflict-in-package)
+     * - conflicts with an existing mempool transaction are found in CTxMemPool::GetConflictTx and replacements are allowed
+     * The temporary coins should persist between individual transaction checks so that package validation is possible,
+     * but must be cleaned up when we finish validating a subpackage, whether accepted or rejected. The cache must also
+     * be cleared when mempool contents change (when a changeset is applied or when the mempool trims itself) because it
+     * can return cached coins that no longer exist in the backend. Use CleanupTemporaryCoins() anytime you are finished
+     * with a SubPackageState or call LimitMempoolSize().
+     */
     CCoinsViewCache m_view;
+
+    // These are the two possible backends for m_view.
+    /** When m_view is connected to m_viewmempool as its backend, it can pull coins from the mempool and from the UTXO
+     * set. This is also where temporary coins are stored. */
     CCoinsViewMemPool m_viewmempool;
+    /** When m_view is connected to m_dummy, it can no longer look up coins from the mempool or UTXO set (meaning no disk
+     * operations happen), but can still return coins it accessed previously. Useful for keeping track of which coins
+     * were pulled from disk. */
     CCoinsView m_dummy;
 
     Chainstate& m_active_chainstate;
@@ -1470,6 +1487,10 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
     // Limit the mempool, if appropriate.
     if (!args.m_package_submission && !args.m_bypass_limits) {
         LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip());
+        // If mempool contents change, then the m_view cache is dirty. Given this isn't a package
+        // submission, we won't be using the cache anymore, but clear it anyway for clarity.
+        CleanupTemporaryCoins();
+
         if (!m_pool.exists(ws.m_hash)) {
             // The tx no longer meets our (new) mempool minimum feerate but could be reconsidered in a package.
             ws.m_state.Invalid(TxValidationResult::TX_RECONSIDERABLE, "mempool full");
@@ -1647,7 +1668,8 @@ void MemPoolAccept::CleanupTemporaryCoins()
     // (3) Confirmed coins don't need to be removed. The chainstate has not changed (we are
     // holding cs_main and no blocks have been processed) so the confirmed tx cannot disappear like
     // a mempool tx can. The coin may now be spent after we submitted a tx to mempool, but
-    // we have already checked that the package does not have 2 transactions spending the same coin.
+    // we have already checked that the package does not have 2 transactions spending the same coin
+    // and we check whether a mempool transaction spends conflicting coins (CTxMemPool::GetConflictTx).
     // Keeping them in m_view is an optimization to not re-fetch confirmed coins if we later look up
     // inputs for this transaction again.
     for (const auto& outpoint : m_viewmempool.GetNonBaseCoins()) {
@@ -1831,6 +1853,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
 
     // Make sure we haven't exceeded max mempool size.
     // Package transactions that were submitted to mempool or already in mempool may be evicted.
+    // If mempool contents change, then the m_view cache is dirty. It has already been cleared above.
     LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip());
 
     for (const auto& tx : package) {

test/functional/mempool_limit.py

@@ -177,98 +177,6 @@ def test_mid_package_eviction_success(self):
                     evicted_feerate_btc_per_kvb = entry["fees"]["modified"] / (Decimal(entry["vsize"]) / 1000)
                     assert_greater_than(evicted_feerate_btc_per_kvb, max_parent_feerate)
 
-    def test_mid_package_eviction(self):
-        node = self.nodes[0]
-        self.log.info("Check a package where each parent passes the current mempoolminfee but would cause eviction before package submission terminates")
-
-        self.restart_node(0, extra_args=self.extra_args[0])
-
-        # Restarting the node resets mempool minimum feerate
-        assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000'))
-        assert_equal(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000'))
-
-        fill_mempool(self, node)
-        current_info = node.getmempoolinfo()
-        mempoolmin_feerate = current_info["mempoolminfee"]
-
-        package_hex = []
-        # UTXOs to be spent by the ultimate child transaction
-        parent_utxos = []
-
-        evicted_vsize = 2000
-        # Mempool transaction which is evicted due to being at the "bottom" of the mempool when the
-        # mempool overflows and evicts by descendant score. It's important that the eviction doesn't
-        # happen in the middle of package evaluation, as it can invalidate the coins cache.
-        mempool_evicted_tx = self.wallet.send_self_transfer(
-            from_node=node,
-            fee_rate=mempoolmin_feerate,
-            target_vsize=evicted_vsize,
-            confirmed_only=True
-        )
-        # Already in mempool when package is submitted.
-        assert mempool_evicted_tx["txid"] in node.getrawmempool()
-
-        # This parent spends the above mempool transaction that exists when its inputs are first
-        # looked up, but disappears later. It is rejected for being too low fee (but eligible for
-        # reconsideration), and its inputs are cached. When the mempool transaction is evicted, its
-        # coin is no longer available, but the cache could still contains the tx.
-        cpfp_parent = self.wallet.create_self_transfer(
-            utxo_to_spend=mempool_evicted_tx["new_utxo"],
-            fee_rate=mempoolmin_feerate - Decimal('0.00001'),
-            confirmed_only=True)
-        package_hex.append(cpfp_parent["hex"])
-        parent_utxos.append(cpfp_parent["new_utxo"])
-        assert_equal(node.testmempoolaccept([cpfp_parent["hex"]])[0]["reject-reason"], "mempool min fee not met")
-
-        self.wallet.rescan_utxos()
-
-        # Series of parents that don't need CPFP and are submitted individually. Each one is large and
-        # high feerate, which means they should trigger eviction but not be evicted.
-        parent_vsize = 25000
-        num_big_parents = 3
-        # Need to be large enough to trigger eviction
-        # (note that the mempool usage of a tx is about three times its vsize)
-        assert_greater_than(parent_vsize * num_big_parents * 3, current_info["maxmempool"] - current_info["bytes"])
-        parent_feerate = 100 * mempoolmin_feerate
-
-        big_parent_txids = []
-        for i in range(num_big_parents):
-            parent = self.wallet.create_self_transfer(fee_rate=parent_feerate, target_vsize=parent_vsize, confirmed_only=True)
-            parent_utxos.append(parent["new_utxo"])
-            package_hex.append(parent["hex"])
-            big_parent_txids.append(parent["txid"])
-            # There is room for each of these transactions independently
-            assert node.testmempoolaccept([parent["hex"]])[0]["allowed"]
-
-        # Create a child spending everything, bumping cpfp_parent just above mempool minimum
-        # feerate. It's important not to bump too much as otherwise mempool_evicted_tx would not be
-        # evicted, making this test much less meaningful.
-        approx_child_vsize = self.wallet.create_self_transfer_multi(utxos_to_spend=parent_utxos)["tx"].get_vsize()
-        cpfp_fee = (mempoolmin_feerate / 1000) * (cpfp_parent["tx"].get_vsize() + approx_child_vsize) - cpfp_parent["fee"]
-        # Specific number of satoshis to fit within a small window. The parent_cpfp + child package needs to be
-        # - When there is mid-package eviction, high enough feerate to meet the new mempoolminfee
-        # - When there is no mid-package eviction, low enough feerate to be evicted immediately after submission.
-        magic_satoshis = 1200
-        cpfp_satoshis = int(cpfp_fee * COIN) + magic_satoshis
-
-        child = self.wallet.create_self_transfer_multi(utxos_to_spend=parent_utxos, fee_per_output=cpfp_satoshis)
-        package_hex.append(child["hex"])
-
-        # Package should be submitted, temporarily exceeding maxmempool, and then evicted.
-        with node.assert_debug_log(expected_msgs=["rolling minimum fee bumped"]):
-            assert_equal(node.submitpackage(package_hex)["package_msg"], "transaction failed")
-
-        # Maximum size must never be exceeded.
-        assert_greater_than(node.getmempoolinfo()["maxmempool"], node.getmempoolinfo()["bytes"])
-
-        # Evicted transaction and its descendants must not be in mempool.
-        resulting_mempool_txids = node.getrawmempool()
-        assert mempool_evicted_tx["txid"] not in resulting_mempool_txids
-        assert cpfp_parent["txid"] not in resulting_mempool_txids
-        assert child["txid"] not in resulting_mempool_txids
-        for txid in big_parent_txids:
-            assert txid in resulting_mempool_txids
-
     def test_mid_package_replacement(self):
         node = self.nodes[0]
         self.log.info("Check a package where an early tx depends on a later-replaced mempool tx")
@@ -283,9 +191,7 @@ def test_mid_package_replacement(self):
         current_info = node.getmempoolinfo()
         mempoolmin_feerate = current_info["mempoolminfee"]
 
-        # Mempool transaction which is evicted due to being at the "bottom" of the mempool when the
-        # mempool overflows and evicts by descendant score. It's important that the eviction doesn't
-        # happen in the middle of package evaluation, as it can invalidate the coins cache.
+        # Mempool transaction is replaced by a package transaction.
         double_spent_utxo = self.wallet.get_utxo(confirmed_only=True)
         replaced_tx = self.wallet.send_self_transfer(
             from_node=node,
@@ -297,8 +203,8 @@ def test_mid_package_replacement(self):
         assert replaced_tx["txid"] in node.getrawmempool()
 
         # This parent spends the above mempool transaction that exists when its inputs are first
-        # looked up, but disappears later. It is rejected for being too low fee (but eligible for
-        # reconsideration), and its inputs are cached. When the mempool transaction is evicted, its
+        # looked up, but will disappear if the replacement occurs. It is rejected for being too low fee (but eligible for
+        # reconsideration), and its inputs are cached. When the mempool transaction is replaced, its
         # coin is no longer available, but the cache could still contain the tx.
         cpfp_parent = self.wallet.create_self_transfer(
             utxo_to_spend=replaced_tx["new_utxo"],
@@ -433,7 +339,6 @@ def run_test(self):
 
         self.test_mid_package_eviction_success()
         self.test_mid_package_replacement()
-        self.test_mid_package_eviction()
         self.test_rbf_carveout_disallowed()