cpfp carveout is excluded in packages

The behavior is not new, but this rule exits earlier than before.
Previously, a carve out could have been granted in PreChecks() but then
nullified in PackageMempoolChecks() when CheckPackageLimits() is called
with the default limits.

Author: glozow

doc/policy/packages.md

@@ -48,8 +48,13 @@ The following rules are enforced for all packages:
      heavily connected, i.e. some transaction in the package is the ancestor or descendant of all
      the other transactions.
 
-The following rules are only enforced for packages to be submitted to the mempool (not enforced for
-test accepts):
+* [CPFP Carve Out](./mempool-limits.md#CPFP-Carve-Out) is disabled in packaged contexts. (#21800)
+
+   - *Rationale*: This carve out cannot be accurately applied when there are multiple transactions'
+     ancestors and descendants being considered at the same time.
+
+The following rules are only enforced for packages to be submitted to the mempool (not
+enforced for test accepts):
 
 * Packages must be child-with-unconfirmed-parents packages. This also means packages must contain at
   least 2 transactions. (#22674)

src/validation.cpp

@@ -478,6 +478,9 @@ class MemPoolAccept
          */
         const std::optional<CFeeRate> m_client_maxfeerate;
 
+        /** Whether CPFP carveout and RBF carveout are granted. */
+        const bool m_allow_carveouts;
+
         /** Parameters for single transaction mempool validation. */
         static ATMPArgs SingleAccept(const CChainParams& chainparams, int64_t accept_time,
                                      bool bypass_limits, std::vector<COutPoint>& coins_to_uncache,
@@ -492,6 +495,7 @@ class MemPoolAccept
                             /* m_package_submission */ false,
                             /* m_package_feerates */ false,
                             /* m_client_maxfeerate */ {}, // checked by caller
+                            /* m_allow_carveouts */ true,
             };
         }
 
@@ -508,6 +512,7 @@ class MemPoolAccept
                             /* m_package_submission */ false, // not submitting to mempool
                             /* m_package_feerates */ false,
                             /* m_client_maxfeerate */ {}, // checked by caller
+                            /* m_allow_carveouts */ false,
             };
         }
 
@@ -524,6 +529,7 @@ class MemPoolAccept
                             /* m_package_submission */ true,
                             /* m_package_feerates */ true,
                             /* m_client_maxfeerate */ client_maxfeerate,
+                            /* m_allow_carveouts */ false,
             };
         }
 
@@ -539,6 +545,7 @@ class MemPoolAccept
                             /* m_package_submission */ true, // do not LimitMempoolSize in Finalize()
                             /* m_package_feerates */ false, // only 1 transaction
                             /* m_client_maxfeerate */ package_args.m_client_maxfeerate,
+                            /* m_allow_carveouts */ false,
             };
         }
 
@@ -554,7 +561,8 @@ class MemPoolAccept
                  bool allow_sibling_eviction,
                  bool package_submission,
                  bool package_feerates,
-                 std::optional<CFeeRate> client_maxfeerate)
+                 std::optional<CFeeRate> client_maxfeerate,
+                 bool allow_carveouts)
             : m_chainparams{chainparams},
               m_accept_time{accept_time},
               m_bypass_limits{bypass_limits},
@@ -564,7 +572,8 @@ class MemPoolAccept
               m_allow_sibling_eviction{allow_sibling_eviction},
               m_package_submission{package_submission},
               m_package_feerates{package_feerates},
-              m_client_maxfeerate{client_maxfeerate}
+              m_client_maxfeerate{client_maxfeerate},
+              m_allow_carveouts{allow_carveouts}
         {
         }
     };
@@ -917,7 +926,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     CTxMemPool::Limits maybe_rbf_limits = m_pool.m_opts.limits;
 
     // Calculate in-mempool ancestors, up to a limit.
-    if (ws.m_conflicts.size() == 1) {
+    if (ws.m_conflicts.size() == 1 && args.m_allow_carveouts) {
         // In general, when we receive an RBF transaction with mempool conflicts, we want to know whether we
         // would meet the chain limits after the conflicts have been removed. However, there isn't a practical
         // way to do this short of calculating the ancestor and descendant sets with an overlay cache of
@@ -956,6 +965,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
         ws.m_ancestors = std::move(*ancestors);
     } else {
         // If CalculateMemPoolAncestors fails second time, we want the original error string.
+        const auto error_message{util::ErrorString(ancestors).original};
+
+        // Carve-out is not allowed in this context; fail
+        if (!args.m_allow_carveouts) {
+            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message);
+        }
+
         // Contracting/payment channels CPFP carve-out:
         // If the new transaction is relatively small (up to 40k weight)
         // and has at most one ancestor (ie ancestor limit of 2, including
@@ -974,7 +990,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
             .descendant_count = maybe_rbf_limits.descendant_count + 1,
             .descendant_size_vbytes = maybe_rbf_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT,
         };
-        const auto error_message{util::ErrorString(ancestors).original};
         if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || ws.m_ptx->nVersion == 3) {
             return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message);
         }
@@ -1431,9 +1446,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
     }
 
     // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction,
-    // because it's unnecessary. Also, CPFP carve out can increase the limit for individual
-    // transactions, but this exemption is not extended to packages in CheckPackageLimits().
-    std::string err_string;
+    // because it's unnecessary.
     if (txns.size() > 1 && !PackageMempoolChecks(txns, m_total_vsize, package_state)) {
         return PackageMempoolAcceptResult(package_state, std::move(results));
     }

test/functional/mempool_package_onemore.py

@@ -47,21 +47,30 @@ def run_test(self):
 
         # Adding one more transaction on to the chain should fail.
         assert_raises_rpc_error(-26, "too-long-mempool-chain, too many unconfirmed ancestors [limit: 25]", self.chain_tx, [utxo])
-        # ...even if it chains on from some point in the middle of the chain.
+        # ... or if it chains on from some point in the middle of the chain.
         assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[2]])
         assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[1]])
         # ...even if it chains on to two parent transactions with one in the chain.
         assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[0], second_chain])
         # ...especially if its > 40k weight
         assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[0]], num_outputs=350)
+        # ...even if it's submitted with other transactions
+        replaceable_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=[chain[0]])
+        txns = [replaceable_tx["tx"], self.wallet.create_self_transfer_multi(utxos_to_spend=replaceable_tx["new_utxos"])["tx"]]
+        txns_hex = [tx.serialize().hex() for tx in txns]
+        assert_equal(self.nodes[0].testmempoolaccept(txns_hex)[0]["reject-reason"], "too-long-mempool-chain")
+        pkg_result = self.nodes[0].submitpackage(txns_hex)
+        assert "too-long-mempool-chain" in pkg_result["tx-results"][txns[0].getwtxid()]["error"]
+        assert_equal(pkg_result["tx-results"][txns[1].getwtxid()]["error"], "bad-txns-inputs-missingorspent")
         # But not if it chains directly off the first transaction
-        replacable_tx = self.wallet.send_self_transfer_multi(from_node=self.nodes[0], utxos_to_spend=[chain[0]])['tx']
+        self.nodes[0].sendrawtransaction(replaceable_tx["hex"])
         # and the second chain should work just fine
         self.chain_tx([second_chain])
 
-        # Make sure we can RBF the chain which used our carve-out rule
-        replacable_tx.vout[0].nValue -= 1000000
-        self.nodes[0].sendrawtransaction(replacable_tx.serialize().hex())
+        # Ensure an individual transaction with single direct conflict can RBF the chain which used our carve-out rule
+        replacement_tx = replaceable_tx["tx"]
+        replacement_tx.vout[0].nValue -= 1000000
+        self.nodes[0].sendrawtransaction(replacement_tx.serialize().hex())
 
         # Finally, check that we added two transactions
         assert_equal(len(self.nodes[0].getrawmempool()), DEFAULT_ANCESTOR_LIMIT + 3)