Merge bitcoin/bitcoin#21800: mempool/validation: mempool ancestor/descendant limits for packages

accf3d5868460b4b14ab607fd66ac985b086fbb3 [test] mempool package ancestor/descendant limits (glozow)
2b6b26e57c24d2f0abd442c1c33098e3121572ce [test] parameterizable fee for make_chain and create_child_with_parents (glozow)
313c09f7b7beddfdb74c284720d209c81dfdb94f [test] helper function to increase transaction weight (glozow)
f8253d69d6f02850995a11eeb71fedc22e6f6575 extract/rename helper functions from rpc_packages.py (glozow)
3cd663a5d33aa7ef87994e452bced7f192d021a0 [policy] ancestor/descendant limits for packages (glozow)
c6e016aa139c8363e9b38bbc1ba0dca55700b8a7 [mempool] check ancestor/descendant limits for packages (glozow)
f551841d3ec080a2d7a7988c7b35088dff6c5830 [refactor] pass size/count instead of entry to CalculateAncestorsAndCheckLimits (glozow)
97dd1c729d2bbedf9527b914c0cc8267b8a7c21b MOVEONLY: add helper function for calculating ancestors and checking limits (glozow)
f95bbf58aaf72aab8a9c5827b1f162f3b8ac38f4 misc package validation doc improvements (glozow)

Pull request description:

  This PR implements a function to calculate mempool ancestors for a package and enforces ancestor/descendant limits on them as a whole. It reuses a portion of `CalculateMemPoolAncestors()`; there's also a small refactor to move the reused code into a generic helper function. Instead of calculating ancestors and descendants on every single transaction in the package and their ancestors, we use a "worst case" heuristic, treating every transaction in the package as each other's ancestor and descendant. This may overestimate everyone's counts, but is still pretty accurate in the our main package use cases, in which at least one of the transactions in the package is directly related to all the others (e.g. 1 parent + 1 child, multiple parents with 1 child, or chains).

  Note on Terminology: While "package" is often used to describe groups of related transactions _within_ the mempool, here, I only use package to mean the group of not-in-mempool transactions we are currently validating.

  #### Motivation

  It would be a potential DoS vector to allow submission of packages to mempool without a proper guard for mempool ancestors/descendants. In general, the purpose of mempool ancestor/descendant limits is to limit the computational complexity of dealing with families during removals and additions. We want to be able to validate multiple transactions on top of the mempool, but also avoid these scenarios:

  - We underestimate the ancestors/descendants during package validation and end up with extremely complex families in our mempool (potentially a DoS vector).
  - We expend an unreasonable amount of resources calculating everyone's ancestors and descendants during package validation.

ACKs for top commit:
  JeremyRubin:
    utACK accf3d5
  ariard:
    ACK accf3d5.

Tree-SHA512: 0d18ce4b77398fe872e0b7c2cc66d3aac2135e561b64029584339e1f4de2a6a16ebab3dd5784f376e119cbafc4d50168b28d3bd95d0b3d01158714ade2e3624d
Signed-off-by: Vijay <vijaydas.mp@gmail.com>

src/rpc/rawtransaction.cpp

@@ -1200,7 +1200,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 another transaction failed.\n",
+                        "Transactions that cannot be fully validated due to failures in other transactions will not contain an 'allowed' result.\n",
                     {
                         {RPCResult::Type::OBJ, "", "",
                         {

src/txmempool.cpp

@@ -159,7 +159,98 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256> &vHashes
     }
 }
 
-bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntries &setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string &errString, bool fSearchForParents /* = true */) const
+bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size,
+                                                  size_t entry_count,
+                                                  setEntries& setAncestors,
+                                                  CTxMemPoolEntry::Parents& staged_ancestors,
+                                                  uint64_t limitAncestorCount,
+                                                  uint64_t limitAncestorSize,
+                                                  uint64_t limitDescendantCount,
+                                                  uint64_t limitDescendantSize,
+                                                  std::string &errString) const
+{
+    size_t totalSizeWithAncestors = entry_size;
+
+    while (!staged_ancestors.empty()) {
+        const CTxMemPoolEntry& stage = staged_ancestors.begin()->get();
+        txiter stageit = mapTx.iterator_to(stage);
+
+        setAncestors.insert(stageit);
+        staged_ancestors.erase(stage);
+        totalSizeWithAncestors += stageit->GetTxSize();
+
+        if (stageit->GetSizeWithDescendants() + entry_size > limitDescendantSize) {
+            errString = strprintf("exceeds descendant size limit for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limitDescendantSize);
+            return false;
+        } else if (stageit->GetCountWithDescendants() + entry_count > limitDescendantCount) {
+            errString = strprintf("too many descendants for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limitDescendantCount);
+            return false;
+        } else if (totalSizeWithAncestors > limitAncestorSize) {
+            errString = strprintf("exceeds ancestor size limit [limit: %u]", limitAncestorSize);
+            return false;
+        }
+
+        const CTxMemPoolEntry::Parents& parents = stageit->GetMemPoolParentsConst();
+        for (const CTxMemPoolEntry& parent : parents) {
+            txiter parent_it = mapTx.iterator_to(parent);
+
+            // If this is a new ancestor, add it.
+            if (setAncestors.count(parent_it) == 0) {
+                staged_ancestors.insert(parent);
+            }
+            if (staged_ancestors.size() + setAncestors.size() + entry_count > limitAncestorCount) {
+                errString = strprintf("too many unconfirmed ancestors [limit: %u]", limitAncestorCount);
+                return false;
+            }
+        }
+    }
+
+    return true;
+}
+
+bool CTxMemPool::CheckPackageLimits(const Package& package,
+                                    uint64_t limitAncestorCount,
+                                    uint64_t limitAncestorSize,
+                                    uint64_t limitDescendantCount,
+                                    uint64_t limitDescendantSize,
+                                    std::string &errString) const
+{
+    CTxMemPoolEntry::Parents staged_ancestors;
+    size_t total_size = 0;
+    for (const auto& tx : package) {
+        total_size += GetVirtualTransactionSize(*tx);
+        for (const auto& input : tx->vin) {
+            std::optional<txiter> piter = GetIter(input.prevout.hash);
+            if (piter) {
+                staged_ancestors.insert(**piter);
+                if (staged_ancestors.size() + package.size() > limitAncestorCount) {
+                    errString = strprintf("too many unconfirmed parents [limit: %u]", limitAncestorCount);
+                    return false;
+                }
+            }
+        }
+    }
+    // When multiple transactions are passed in, the ancestors and descendants of all transactions
+    // considered together must be within limits even if they are not interdependent. This may be
+    // stricter than the limits for each individual transaction.
+    setEntries setAncestors;
+    const auto ret = CalculateAncestorsAndCheckLimits(total_size, package.size(),
+                                                      setAncestors, staged_ancestors,
+                                                      limitAncestorCount, limitAncestorSize,
+                                                      limitDescendantCount, limitDescendantSize, errString);
+    // It's possible to overestimate the ancestor/descendant totals.
+    if (!ret) errString.insert(0, "possibly ");
+    return ret;
+}
+
+bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry,
+                                           setEntries &setAncestors,
+                                           uint64_t limitAncestorCount,
+                                           uint64_t limitAncestorSize,
+                                           uint64_t limitDescendantCount,
+                                           uint64_t limitDescendantSize,
+                                           std::string &errString,
+                                           bool fSearchForParents /* = true */) const
 {
     CTxMemPoolEntry::Parents staged_ancestors;
     const CTransaction &tx = entry.GetTx();
@@ -179,49 +270,16 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntr
             }
         }
     } else {
-        // If we're not searching for parents, we require this to be an
+        // If we're not searching for parents, we require this to already be an
-        // entry in the mempool already.
+        // entry in the mempool and use the entry's cached parents.
         txiter it = mapTx.iterator_to(entry);
         staged_ancestors = it->GetMemPoolParentsConst();
     }
 
-    size_t totalSizeWithAncestors = entry.GetTxSize();
+    return CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /* entry_count */ 1,
-
+                                            setAncestors, staged_ancestors,
-    while (!staged_ancestors.empty()) {
+                                            limitAncestorCount, limitAncestorSize,
-        const CTxMemPoolEntry& stage = staged_ancestors.begin()->get();
+                                            limitDescendantCount, limitDescendantSize, errString);
-        txiter stageit = mapTx.iterator_to(stage);
-
-        setAncestors.insert(stageit);
-        staged_ancestors.erase(stage);
-        totalSizeWithAncestors += stageit->GetTxSize();
-
-        if (stageit->GetSizeWithDescendants() + entry.GetTxSize() > limitDescendantSize) {
-            errString = strprintf("exceeds descendant size limit for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limitDescendantSize);
-            return false;
-        } else if (stageit->GetCountWithDescendants() + 1 > limitDescendantCount) {
-            errString = strprintf("too many descendants for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limitDescendantCount);
-            return false;
-        } else if (totalSizeWithAncestors > limitAncestorSize) {
-            errString = strprintf("exceeds ancestor size limit [limit: %u]", limitAncestorSize);
-            return false;
-        }
-
-        const CTxMemPoolEntry::Parents& parents = stageit->GetMemPoolParentsConst();
-        for (const CTxMemPoolEntry& parent : parents) {
-            txiter parent_it = mapTx.iterator_to(parent);
-
-            // If this is a new ancestor, add it.
-            if (setAncestors.count(parent_it) == 0) {
-                staged_ancestors.insert(parent);
-            }
-            if (staged_ancestors.size() + setAncestors.size() + 1 > limitAncestorCount) {
-                errString = strprintf("too many unconfirmed ancestors [limit: %u]", limitAncestorCount);
-                return false;
-            }
-        }
-    }
-
-    return true;
 }
 
 void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors)

src/txmempool.h

@@ -21,6 +21,7 @@
 #include <gsl/pointers.h>
 #include <indirectmap.h>
 #include <policy/feerate.h>
+#include <policy/packages.h>
 #include <primitives/transaction.h>
 #include <random.h>
 #include <netaddress.h>
@@ -588,6 +589,25 @@ private:
      */
     std::set<uint256> m_unbroadcast_txids GUARDED_BY(cs);
 
+
+    /**
+     * Helper function to calculate all in-mempool ancestors of staged_ancestors and apply ancestor
+     * and descendant limits (including staged_ancestors thsemselves, entry_size and entry_count).
+     * param@[in]   entry_size          Virtual size to include in the limits.
+     * param@[in]   entry_count         How many entries to include in the limits.
+     * param@[in]   staged_ancestors    Should contain entries in the mempool.
+     * param@[out]  setAncestors        Will be populated with all mempool ancestors.
+     */
+    bool CalculateAncestorsAndCheckLimits(size_t entry_size,
+                                          size_t entry_count,
+                                          setEntries& setAncestors,
+                                          CTxMemPoolEntry::Parents &staged_ancestors,
+                                          uint64_t limitAncestorCount,
+                                          uint64_t limitAncestorSize,
+                                          uint64_t limitDescendantCount,
+                                          uint64_t limitDescendantSize,
+                                          std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs);
+
 public:
     indirectmap<COutPoint, const CTransaction*> mapNextTx GUARDED_BY(cs);
     std::map<uint256, CAmount> mapDeltas GUARDED_BY(cs);
@@ -715,6 +735,28 @@ public:
      */
     bool CalculateMemPoolAncestors(const CTxMemPoolEntry& entry, setEntries& setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string& errString, bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
+    /** Calculate all in-mempool ancestors of a set of transactions not already in the mempool and
+     * check ancestor and descendant limits. Heuristics are used to estimate the ancestor and
+     * descendant count of all entries if the package were to be added to the mempool.  The limits
+     * are applied to the union of all package transactions. For example, if the package has 3
+     * transactions and limitAncestorCount = 25, the union of all 3 sets of ancestors (including the
+     * transactions themselves) must be <= 22.
+     * @param[in]       package                 Transaction package being evaluated for acceptance
+     *                                          to mempool. The transactions need not be direct
+     *                                          ancestors/descendants of each other.
+     * @param[in]       limitAncestorCount      Max number of txns including ancestors.
+     * @param[in]       limitAncestorSize       Max virtual size including ancestors.
+     * @param[in]       limitDescendantCount    Max number of txns including descendants.
+     * @param[in]       limitDescendantSize     Max virtual size including descendants.
+     * @param[out]      errString               Populated with error reason if a limit is hit.
+     */
+    bool CheckPackageLimits(const Package& package,
+                            uint64_t limitAncestorCount,
+                            uint64_t limitAncestorSize,
+                            uint64_t limitDescendantCount,
+                            uint64_t limitDescendantSize,
+                            std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs);
+
     /** Populate setDescendants with all in-mempool descendants of hash.
      *  Assumes that setDescendants includes all in-mempool descendants of anything
      *  already in it.  */

src/validation.cpp

@@ -929,6 +929,19 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
         m_viewmempool.PackageAddTransaction(ws.m_ptx);
     }
 
+    // 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;
+    if (txns.size() > 1 &&
+        !m_pool.CheckPackageLimits(txns, m_limit_ancestors, m_limit_ancestor_size, m_limit_descendants,
+                                   m_limit_descendant_size, err_string)) {
+        // All transactions must have individually passed mempool ancestor and descendant limits
+        // inside of PreChecks(), so this is separate from an individual transaction error.
+        package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", err_string);
+        return PackageMempoolAcceptResult(package_state, std::move(results));
+    }
+
     for (Workspace& ws : workspaces) {
         PrecomputedTransactionData txdata;
         if (!PolicyScriptChecks(args, ws, txdata)) {

src/validation.h

@@ -224,7 +224,8 @@ struct PackageMempoolAcceptResult
     /**
     * Map from txid to finished MempoolAcceptResults. The client is responsible
     * for keeping track of the transaction objects themselves. If a result is not
-    * present, it means validation was unfinished for that transaction.
+    * present, it means validation was unfinished for that transaction. If there
+    * was a package-wide error (see result in m_state), m_tx_results will be empty.
     */
     std::map<const uint256, const MempoolAcceptResult> m_tx_results;
 
@@ -251,7 +252,8 @@ MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPoo
 * @param[in]    txns                Group of transactions which may be independent or contain
 *                                   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.
+*                                   dependencies exist, parents must appear anywhere in the list
+*                                   before their 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.
 */

test/functional/mempool_package_limits.py

@@ -0,0 +1,472 @@
+#!/usr/bin/env python3
+# 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.
+"""Test logic for limiting mempool and package ancestors/descendants."""
+
+from decimal import Decimal
+
+from test_framework.address import ADDRESS_BCRT1_P2SH_OP_TRUE
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.messages import (
+    COIN,
+    CTransaction,
+    tx_from_hex,
+)
+from test_framework.script import (
+    CScript,
+    OP_TRUE,
+)
+from test_framework.util import (
+    assert_equal,
+)
+from test_framework.wallet import (
+    bulk_transaction,
+    create_child_with_parents,
+    make_chain,
+)
+
+class MempoolPackageLimitsTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 1
+        self.setup_clean_chain = True
+
+    def run_test(self):
+        self.log.info("Generate blocks to create UTXOs")
+        node = self.nodes[0]
+        self.privkeys = [node.get_deterministic_priv_key().key]
+        self.address = node.get_deterministic_priv_key().address
+        self.coins = []
+        # The last 100 coinbase transactions are premature
+        for b in node.generatetoaddress(200, self.address)[:100]:
+            coinbase = node.getblock(blockhash=b, verbosity=2)["tx"][0]
+            self.coins.append({
+                "txid": coinbase["txid"],
+                "amount": coinbase["vout"][0]["value"],
+                "scriptPubKey": coinbase["vout"][0]["scriptPubKey"],
+            })
+
+        self.test_chain_limits()
+        self.test_desc_count_limits()
+        self.test_anc_count_limits()
+        self.test_anc_count_limits_2()
+        self.test_anc_count_limits_bushy()
+
+        # The node will accept our (nonstandard) extra large OP_RETURN outputs
+        self.restart_node(0, extra_args=["-acceptnonstdtxn=1"])
+        self.test_anc_size_limits()
+        self.test_desc_size_limits()
+
+    def test_chain_limits_helper(self, mempool_count, package_count):
+        node = self.nodes[0]
+        assert_equal(0, node.getmempoolinfo()["size"])
+        first_coin = self.coins.pop()
+        spk = None
+        txid = first_coin["txid"]
+        chain_hex = []
+        chain_txns = []
+        value = first_coin["amount"]
+
+        for i in range(mempool_count + package_count):
+            (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk)
+            txid = tx.rehash()
+            if i < mempool_count:
+                node.sendrawtransaction(txhex)
+                assert_equal(node.getrawmempool(verbose=True)[txid]["ancestorcount"], i + 1)
+            else:
+                chain_hex.append(txhex)
+                chain_txns.append(tx)
+        testres_too_long = node.testmempoolaccept(rawtxs=chain_hex)
+        for txres in testres_too_long:
+            assert_equal(txres["package-error"], "package-mempool-limits")
+
+        # Clear mempool and check that the package passes now
+        node.generate(1)
+        assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=chain_hex)])
+
+    def test_chain_limits(self):
+        """Create chains from mempool and package transactions that are longer than 25,
+        but only if both in-mempool and in-package transactions are considered together.
+        This checks that both mempool and in-package transactions are taken into account when
+        calculating ancestors/descendant limits.
+        """
+        self.log.info("Check that in-package ancestors count for mempool ancestor limits")
+
+        # 24 transactions in the mempool and 2 in the package. The parent in the package has
+        # 24 in-mempool ancestors and 1 in-package descendant. The child has 0 direct parents
+        # in the mempool, but 25 in-mempool and in-package ancestors in total.
+        self.test_chain_limits_helper(24, 2)
+        # 2 transactions in the mempool and 24 in the package.
+        self.test_chain_limits_helper(2, 24)
+        # 13 transactions in the mempool and 13 in the package.
+        self.test_chain_limits_helper(13, 13)
+
+    def test_desc_count_limits(self):
+        """Create an 'A' shaped package with 24 transactions in the mempool and 2 in the package:
+                    M1
+                   ^  ^
+                 M2a  M2b
+                .       .
+               .         .
+              .           .
+             M12a          ^
+            ^              M13b
+           ^                 ^
+          Pa                  Pb
+        The top ancestor in the package exceeds descendant limits but only if the in-mempool and in-package
+        descendants are all considered together (24 including in-mempool descendants and 26 including both
+        package transactions).
+        """
+        node = self.nodes[0]
+        assert_equal(0, node.getmempoolinfo()["size"])
+        self.log.info("Check that in-mempool and in-package descendants are calculated properly in packages")
+        # Top parent in mempool, M1
+        first_coin = self.coins.pop()
+        parent_value = (first_coin["amount"] - Decimal("0.0002")) / 2 # Deduct reasonable fee and make 2 outputs
+        inputs = [{"txid": first_coin["txid"], "vout": 0}]
+        outputs = [{self.address : parent_value}, {ADDRESS_BCRT1_P2SH_OP_TRUE : parent_value}]
+        rawtx = node.createrawtransaction(inputs, outputs)
+
+        parent_signed = node.signrawtransactionwithkey(hexstring=rawtx, privkeys=self.privkeys)
+        assert parent_signed["complete"]
+        parent_tx = tx_from_hex(parent_signed["hex"])
+        parent_txid = parent_tx.rehash()
+        node.sendrawtransaction(parent_signed["hex"])
+
+        package_hex = []
+
+        # Chain A
+        spk = parent_tx.vout[0].scriptPubKey.hex()
+        value = parent_value
+        txid = parent_txid
+        for i in range(12):
+            (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk)
+            txid = tx.rehash()
+            if i < 11: # M2a... M12a
+                node.sendrawtransaction(txhex)
+            else: # Pa
+                package_hex.append(txhex)
+
+        # Chain B
+        value = parent_value - Decimal("0.0001")
+        rawtx_b = node.createrawtransaction([{"txid": parent_txid, "vout": 1}], {self.address : value})
+        tx_child_b = tx_from_hex(rawtx_b) # M2b
+        tx_child_b.vin[0].scriptSig = CScript([CScript([OP_TRUE])])
+        tx_child_b_hex = tx_child_b.serialize().hex()
+        node.sendrawtransaction(tx_child_b_hex)
+        spk = tx_child_b.vout[0].scriptPubKey.hex()
+        txid = tx_child_b.rehash()
+        for i in range(12):
+            (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk)
+            txid = tx.rehash()
+            if i < 11: # M3b... M13b
+                node.sendrawtransaction(txhex)
+            else: # Pb
+                package_hex.append(txhex)
+
+        assert_equal(24, node.getmempoolinfo()["size"])
+        assert_equal(2, len(package_hex))
+        testres_too_long = node.testmempoolaccept(rawtxs=package_hex)
+        for txres in testres_too_long:
+            assert_equal(txres["package-error"], "package-mempool-limits")
+
+        # Clear mempool and check that the package passes now
+        node.generate(1)
+        assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=package_hex)])
+
+    def test_anc_count_limits(self):
+        """Create a 'V' shaped chain with 24 transactions in the mempool and 3 in the package:
+        M1a                    M1b
+         ^                     ^
+          M2a                M2b
+           .                 .
+            .               .
+             .             .
+             M12a        M12b
+               ^         ^
+                Pa     Pb
+                 ^    ^
+                   Pc
+        The lowest descendant, Pc, exceeds ancestor limits, but only if the in-mempool
+        and in-package ancestors are all considered together.
+        """
+        node = self.nodes[0]
+        assert_equal(0, node.getmempoolinfo()["size"])
+        package_hex = []
+        parents_tx = []
+        values = []
+        scripts = []
+
+        self.log.info("Check that in-mempool and in-package ancestors are calculated properly in packages")
+
+        # Two chains of 13 transactions each
+        for _ in range(2):
+            spk = None
+            top_coin = self.coins.pop()
+            txid = top_coin["txid"]
+            value = top_coin["amount"]
+            for i in range(13):
+                (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk)
+                txid = tx.rehash()
+                if i < 12:
+                    node.sendrawtransaction(txhex)
+                else: # Save the 13th transaction for the package
+                    package_hex.append(txhex)
+                    parents_tx.append(tx)
+                    scripts.append(spk)
+                    values.append(value)
+
+        # Child Pc
+        child_hex = create_child_with_parents(node, self.address, self.privkeys, parents_tx, values, scripts)
+        package_hex.append(child_hex)
+
+        assert_equal(24, node.getmempoolinfo()["size"])
+        assert_equal(3, len(package_hex))
+        testres_too_long = node.testmempoolaccept(rawtxs=package_hex)
+        for txres in testres_too_long:
+            assert_equal(txres["package-error"], "package-mempool-limits")
+
+        # Clear mempool and check that the package passes now
+        node.generate(1)
+        assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=package_hex)])
+
+    def test_anc_count_limits_2(self):
+        """Create a 'Y' shaped chain with 24 transactions in the mempool and 2 in the package:
+        M1a                M1b
+         ^                ^
+          M2a            M2b
+           .            .
+            .          .
+             .        .
+            M12a    M12b
+               ^    ^
+                 Pc
+                 ^
+                 Pd
+        The lowest descendant, Pd, exceeds ancestor limits, but only if the in-mempool
+        and in-package ancestors are all considered together.
+        """
+        node = self.nodes[0]
+        assert_equal(0, node.getmempoolinfo()["size"])
+        parents_tx = []
+        values = []
+        scripts = []
+
+        self.log.info("Check that in-mempool and in-package ancestors are calculated properly in packages")
+        # Two chains of 12 transactions each
+        for _ in range(2):
+            spk = None
+            top_coin = self.coins.pop()
+            txid = top_coin["txid"]
+            value = top_coin["amount"]
+            for i in range(12):
+                (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk)
+                txid = tx.rehash()
+                value -= Decimal("0.0001")
+                node.sendrawtransaction(txhex)
+                if i == 11:
+                    # last 2 transactions will be the parents of Pc
+                    parents_tx.append(tx)
+                    values.append(value)
+                    scripts.append(spk)
+
+        # Child Pc
+        pc_hex = create_child_with_parents(node, self.address, self.privkeys, parents_tx, values, scripts)
+        pc_tx = tx_from_hex(pc_hex)
+        pc_value = sum(values) - Decimal("0.0002")
+        pc_spk = pc_tx.vout[0].scriptPubKey.hex()
+
+        # Child Pd
+        (_, pd_hex, _, _) = make_chain(node, self.address, self.privkeys, pc_tx.rehash(), pc_value, 0, pc_spk)
+
+        assert_equal(24, node.getmempoolinfo()["size"])
+        testres_too_long = node.testmempoolaccept(rawtxs=[pc_hex, pd_hex])
+        for txres in testres_too_long:
+            assert_equal(txres["package-error"], "package-mempool-limits")
+
+        # Clear mempool and check that the package passes now
+        node.generate(1)
+        assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=[pc_hex, pd_hex])])
+
+    def test_anc_count_limits_bushy(self):
+        """Create a tree with 20 transactions in the mempool and 6 in the package:
+        M1...M4 M5...M8 M9...M12 M13...M16 M17...M20
+            ^      ^       ^        ^         ^             (each with 4 parents)
+            P0     P1      P2      P3        P4
+             ^     ^       ^       ^         ^              (5 parents)
+                           PC
+        Where M(4i+1)...M+(4i+4) are the parents of Pi and P0, P1, P2, P3, and P4 are the parents of PC.
+        P0... P4 individually only have 4 parents each, and PC has no in-mempool parents. But
+        combined, PC has 25 in-mempool and in-package parents.
+        """
+        node = self.nodes[0]
+        assert_equal(0, node.getmempoolinfo()["size"])
+        package_hex = []
+        parent_txns = []
+        parent_values = []
+        scripts = []
+        for _ in range(5): # Make package transactions P0 ... P4
+            gp_tx = []
+            gp_values = []
+            gp_scripts = []
+            for _ in range(4): # Make mempool transactions M(4i+1)...M(4i+4)
+                parent_coin = self.coins.pop()
+                value = parent_coin["amount"]
+                txid = parent_coin["txid"]
+                (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value)
+                gp_tx.append(tx)
+                gp_values.append(value)
+                gp_scripts.append(spk)
+                node.sendrawtransaction(txhex)
+            # Package transaction Pi
+            pi_hex = create_child_with_parents(node, self.address, self.privkeys, gp_tx, gp_values, gp_scripts)
+            package_hex.append(pi_hex)
+            pi_tx = tx_from_hex(pi_hex)
+            parent_txns.append(pi_tx)
+            parent_values.append(Decimal(pi_tx.vout[0].nValue) / COIN)
+            scripts.append(pi_tx.vout[0].scriptPubKey.hex())
+        # Package transaction PC
+        package_hex.append(create_child_with_parents(node, self.address, self.privkeys, parent_txns, parent_values, scripts))
+
+        assert_equal(20, node.getmempoolinfo()["size"])
+        assert_equal(6, len(package_hex))
+        testres = node.testmempoolaccept(rawtxs=package_hex)
+        for txres in testres:
+            assert_equal(txres["package-error"], "package-mempool-limits")
+
+        # Clear mempool and check that the package passes now
+        node.generate(1)
+        assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=package_hex)])
+
+    def test_anc_size_limits(self):
+        """Test Case with 2 independent transactions in the mempool and a parent + child in the
+        package, where the package parent is the child of both mempool transactions (30KvB each):
+              A     B
+               ^   ^
+                 C
+                 ^
+                 D
+        The lowest descendant, D, exceeds ancestor size limits, but only if the in-mempool
+        and in-package ancestors are all considered together.
+        """
+        node = self.nodes[0]
+        assert_equal(0, node.getmempoolinfo()["size"])
+        parents_tx = []
+        values = []
+        scripts = []
+        target_weight = 1000 * 30 # 30KvB
+        high_fee = Decimal("0.003") # 10 sats/vB
+        self.log.info("Check that in-mempool and in-package ancestor size limits are calculated properly in packages")
+        # Mempool transactions A and B
+        for _ in range(2):
+            spk = None
+            top_coin = self.coins.pop()
+            txid = top_coin["txid"]
+            value = top_coin["amount"]
+            (tx, _, _, _) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk, high_fee)
+            bulked_tx = bulk_transaction(tx, node, target_weight, self.privkeys)
+            node.sendrawtransaction(bulked_tx.serialize().hex())
+            parents_tx.append(bulked_tx)
+            values.append(Decimal(bulked_tx.vout[0].nValue) / COIN)
+            scripts.append(bulked_tx.vout[0].scriptPubKey.hex())
+
+        # Package transaction C
+        small_pc_hex = create_child_with_parents(node, self.address, self.privkeys, parents_tx, values, scripts, high_fee)
+        pc_tx = bulk_transaction(tx_from_hex(small_pc_hex), node, target_weight, self.privkeys)
+        pc_value = Decimal(pc_tx.vout[0].nValue) / COIN
+        pc_spk = pc_tx.vout[0].scriptPubKey.hex()
+        pc_hex = pc_tx.serialize().hex()
+
+        # Package transaction D
+        (small_pd, _, val, spk) = make_chain(node, self.address, self.privkeys, pc_tx.rehash(), pc_value, 0, pc_spk, high_fee)
+        prevtxs = [{
+            "txid": pc_tx.rehash(),
+            "vout": 0,
+            "scriptPubKey": spk,
+            "amount": val,
+        }]
+        pd_tx = bulk_transaction(small_pd, node, target_weight, self.privkeys, prevtxs)
+        pd_hex = pd_tx.serialize().hex()
+
+        assert_equal(2, node.getmempoolinfo()["size"])
+        testres_too_heavy = node.testmempoolaccept(rawtxs=[pc_hex, pd_hex])
+        for txres in testres_too_heavy:
+            assert_equal(txres["package-error"], "package-mempool-limits")
+
+        # Clear mempool and check that the package passes now
+        node.generate(1)
+        assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=[pc_hex, pd_hex])])
+
+    def test_desc_size_limits(self):
+        """Create 3 mempool transactions and 2 package transactions (25KvB each):
+              Ma
+             ^ ^
+            Mb  Mc
+           ^     ^
+          Pd      Pe
+        The top ancestor in the package exceeds descendant size limits but only if the in-mempool
+        and in-package descendants are all considered together.
+        """
+        node = self.nodes[0]
+        assert_equal(0, node.getmempoolinfo()["size"])
+        target_weight = 21 * 1000
+        high_fee = Decimal("0.0021") # 10 sats/vB
+        self.log.info("Check that in-mempool and in-package descendant sizes are calculated properly in packages")
+        # Top parent in mempool, Ma
+        first_coin = self.coins.pop()
+        parent_value = (first_coin["amount"] - high_fee) / 2 # Deduct fee and make 2 outputs
+        inputs = [{"txid": first_coin["txid"], "vout": 0}]
+        outputs = [{self.address : parent_value}, {ADDRESS_BCRT1_P2SH_OP_TRUE:  parent_value}]
+        rawtx = node.createrawtransaction(inputs, outputs)
+        parent_tx = bulk_transaction(tx_from_hex(rawtx), node, target_weight, self.privkeys)
+        node.sendrawtransaction(parent_tx.serialize().hex())
+
+        package_hex = []
+        for j in range(2): # Two legs (left and right)
+            # Mempool transaction (Mb and Mc)
+            mempool_tx = CTransaction()
+            spk = parent_tx.vout[j].scriptPubKey.hex()
+            value = Decimal(parent_tx.vout[j].nValue) / COIN
+            txid = parent_tx.rehash()
+            prevtxs = [{
+                "txid": txid,
+                "vout": j,
+                "scriptPubKey": spk,
+                "amount": value,
+            }]
+            if j == 0: # normal key
+                (tx_small, _, _, _) = make_chain(node, self.address, self.privkeys, txid, value, j, spk, high_fee)
+                mempool_tx = bulk_transaction(tx_small, node, target_weight, self.privkeys, prevtxs)
+            else: # OP_TRUE
+                inputs = [{"txid": txid, "vout": 1}]
+                outputs = {self.address: value - high_fee}
+                small_tx = tx_from_hex(node.createrawtransaction(inputs, outputs))
+                mempool_tx = bulk_transaction(small_tx, node, target_weight, None, prevtxs)
+            node.sendrawtransaction(mempool_tx.serialize().hex())
+
+            # Package transaction (Pd and Pe)
+            spk = mempool_tx.vout[0].scriptPubKey.hex()
+            value = Decimal(mempool_tx.vout[0].nValue) / COIN
+            txid = mempool_tx.rehash()
+            (tx_small, _, _, _) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk, high_fee)
+            prevtxs = [{
+                "txid": txid,
+                "vout": 0,
+                "scriptPubKey": spk,
+                "amount": value,
+            }]
+            package_tx = bulk_transaction(tx_small, node, target_weight, self.privkeys, prevtxs)
+            package_hex.append(package_tx.serialize().hex())
+
+        assert_equal(3, node.getmempoolinfo()["size"])
+        assert_equal(2, len(package_hex))
+        testres_too_heavy = node.testmempoolaccept(rawtxs=package_hex)
+        for txres in testres_too_heavy:
+            assert_equal(txres["package-error"], "package-mempool-limits")
+
+        # Clear mempool and check that the package passes now
+        node.generate(1)
+        assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=package_hex)])
+
+if __name__ == "__main__":
+    MempoolPackageLimitsTest().main()

test/functional/rpc_packages.py

@@ -19,6 +19,11 @@ from test_framework.script import (
 from test_framework.util import (
     assert_equal,
 )
+from test_framework.wallet import (
+    create_child_with_parents,
+    create_raw_chain,
+    make_chain,
+)
 
 class RPCPackagesTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -74,27 +79,6 @@ class RPCPackagesTest(BitcoinTestFramework):
         self.test_multiple_parents()
         self.test_conflicting()
 
-    def chain_transaction(self, parent_txid, parent_value, n=0, parent_locking_script=None):
-        """Build a transaction that spends parent_txid.vout[n] and produces one output with
-        amount = parent_value with a fee deducted.
-        Return tuple (CTransaction object, raw hex, nValue, scriptPubKey of the output created).
-        """
-        node = self.nodes[0]
-        inputs = [{"txid": parent_txid, "vout": n}]
-        my_value = parent_value - Decimal("0.0001")
-        outputs = {self.address : my_value}
-        rawtx = node.createrawtransaction(inputs, outputs)
-        prevtxs = [{
-            "txid": parent_txid,
-            "vout": n,
-            "scriptPubKey": parent_locking_script,
-            "amount": parent_value,
-        }] if parent_locking_script else None
-        signedtx = node.signrawtransactionwithkey(hexstring=rawtx, privkeys=self.privkeys, prevtxs=prevtxs)
-        tx = tx_from_hex(signedtx["hex"])
-        tx.rehash()
-        assert signedtx["complete"]
-        return (tx, signedtx["hex"], my_value, tx.vout[0].scriptPubKey.hex())
 
     def test_independent(self):
         self.log.info("Test multiple independent transactions in a package")
@@ -146,20 +130,7 @@ class RPCPackagesTest(BitcoinTestFramework):
     def test_chain(self):
         node = self.nodes[0]
         first_coin = self.coins.pop()
-
+        (chain_hex, chain_txns) = create_raw_chain(node, first_coin, self.address, self.privkeys)
-        # Chain of 25 transactions
-        parent_locking_script = None
-        txid = first_coin["txid"]
-        chain_hex = []
-        chain_txns = []
-        value = first_coin["amount"]
-
-        for _ in range(25):
-            (tx, txhex, value, parent_locking_script) = self.chain_transaction(txid, value, 0, parent_locking_script)
-            txid = tx.rehash()
-            chain_hex.append(txhex)
-            chain_txns.append(tx)
-
         self.log.info("Check that testmempoolaccept requires packages to be sorted by dependency")
         assert_equal(node.testmempoolaccept(rawtxs=chain_hex[::-1]),
                 [{"txid": tx.rehash(), "package-error": "package-not-sorted"} for tx in chain_txns[::-1]])
@@ -199,7 +170,7 @@ class RPCPackagesTest(BitcoinTestFramework):
         child_value = value - Decimal("0.0001")
 
         # Child A
-        (_, tx_child_a_hex, _, _) = self.chain_transaction(parent_txid, child_value, 0, parent_locking_script_a)
+        (_, tx_child_a_hex, _, _) = make_chain(node, self.address, self.privkeys, parent_txid, child_value, 0, parent_locking_script_a)
         assert not node.testmempoolaccept([tx_child_a_hex])[0]["allowed"]
 
         # Child B
@@ -223,19 +194,6 @@ class RPCPackagesTest(BitcoinTestFramework):
             node.sendrawtransaction(rawtx)
         assert_equal(testres_single, testres_multiple_ab)
 
-    def create_child_with_parents(self, parents_tx, values, locking_scripts):
-        """Creates a transaction that spends the first output of each parent in parents_tx."""
-        num_parents = len(parents_tx)
-        total_value = sum(values)
-        inputs = [{"txid": tx.rehash(), "vout": 0} for tx in parents_tx]
-        outputs = {self.address : total_value - num_parents * Decimal("0.0001")}
-        rawtx_child = self.nodes[0].createrawtransaction(inputs, outputs)
-        prevtxs = []
-        for i in range(num_parents):
-            prevtxs.append({"txid": parents_tx[i].rehash(), "vout": 0, "scriptPubKey": locking_scripts[i], "amount": values[i]})
-        signedtx_child = self.nodes[0].signrawtransactionwithkey(hexstring=rawtx_child, privkeys=self.privkeys, prevtxs=prevtxs)
-        assert signedtx_child["complete"]
-        return signedtx_child["hex"]
 
     def test_multiple_parents(self):
         node = self.nodes[0]
@@ -250,12 +208,12 @@ class RPCPackagesTest(BitcoinTestFramework):
             for _ in range(num_parents):
                 parent_coin = self.coins.pop()
                 value = parent_coin["amount"]
-                (tx, txhex, value, parent_locking_script) = self.chain_transaction(parent_coin["txid"], value)
+                (tx, txhex, value, parent_locking_script) = make_chain(node, self.address, self.privkeys, parent_coin["txid"], value)
                 package_hex.append(txhex)
                 parents_tx.append(tx)
                 values.append(value)
                 parent_locking_scripts.append(parent_locking_script)
-            child_hex = self.create_child_with_parents(parents_tx, values, parent_locking_scripts)
+            child_hex = create_child_with_parents(node, self.address, self.privkeys, parents_tx, values, parent_locking_scripts)
             # Package accept should work with the parents in any order (as long as parents come before child)
             for _ in range(10):
                 random.shuffle(package_hex)

test/functional/test_framework/messages.py

@@ -544,6 +544,9 @@ class CTransaction:
     def get_vsize(self):
         return len(self.serialize())
 
+    def get_weight(self):
+        return self.get_vsize()
+
     def __repr__(self):
         return "CTransaction(nVersion=%i vin=%s vout=%s nLockTime=%i)" \
                % (self.nVersion, repr(self.vin), repr(self.vout), self.nLockTime)

test/functional/test_framework/wallet.py

@@ -4,10 +4,12 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """A limited-functionality wallet, which may replace a real wallet in tests"""
 
+from copy import deepcopy
 from decimal import Decimal
 from enum import Enum
 from test_framework.address import ADDRESS_BCRT1_P2SH_OP_TRUE
 from test_framework.key import ECKey
+from random import choice
 from typing import Optional
 from test_framework.messages import (
     COIN,
@@ -15,6 +17,7 @@ from test_framework.messages import (
     CTransaction,
     CTxIn,
     CTxOut,
+    tx_from_hex,
 )
 from test_framework.script import (
     CScript,
@@ -26,9 +29,11 @@ from test_framework.script import (
 )
 from test_framework.util import (
     assert_equal,
+    assert_greater_than_or_equal,
     satoshi_round,
 )
 
+DEFAULT_FEE = Decimal("0.0001")
 
 class MiniWalletMode(Enum):
     """Determines the transaction type the MiniWallet is creating and spending.
@@ -165,3 +170,74 @@ class MiniWallet:
     def sendrawtransaction(self, *, from_node, tx_hex):
         from_node.sendrawtransaction(tx_hex)
         self.scan_tx(from_node.decoderawtransaction(tx_hex))
+
+def make_chain(node, address, privkeys, parent_txid, parent_value, n=0, parent_locking_script=None, fee=DEFAULT_FEE):
+    """Build a transaction that spends parent_txid.vout[n] and produces one output with
+    amount = parent_value with a fee deducted.
+    Return tuple (CTransaction object, raw hex, nValue, scriptPubKey of the output created).
+    """
+    inputs = [{"txid": parent_txid, "vout": n}]
+    my_value = parent_value - fee
+    outputs = {address : my_value}
+    rawtx = node.createrawtransaction(inputs, outputs)
+    prevtxs = [{
+        "txid": parent_txid,
+        "vout": n,
+        "scriptPubKey": parent_locking_script,
+        "amount": parent_value,
+    }] if parent_locking_script else None
+    signedtx = node.signrawtransactionwithkey(hexstring=rawtx, privkeys=privkeys, prevtxs=prevtxs)
+    assert signedtx["complete"]
+    tx = tx_from_hex(signedtx["hex"])
+    return (tx, signedtx["hex"], my_value, tx.vout[0].scriptPubKey.hex())
+
+def create_child_with_parents(node, address, privkeys, parents_tx, values, locking_scripts, fee=DEFAULT_FEE):
+    """Creates a transaction that spends the first output of each parent in parents_tx."""
+    num_parents = len(parents_tx)
+    total_value = sum(values)
+    inputs = [{"txid": tx.rehash(), "vout": 0} for tx in parents_tx]
+    outputs = {address : total_value - fee}
+    rawtx_child = node.createrawtransaction(inputs, outputs)
+    prevtxs = []
+    for i in range(num_parents):
+        prevtxs.append({"txid": parents_tx[i].rehash(), "vout": 0, "scriptPubKey": locking_scripts[i], "amount": values[i]})
+    signedtx_child = node.signrawtransactionwithkey(hexstring=rawtx_child, privkeys=privkeys, prevtxs=prevtxs)
+    assert signedtx_child["complete"]
+    return signedtx_child["hex"]
+
+def create_raw_chain(node, first_coin, address, privkeys, chain_length=25):
+    """Helper function: create a "chain" of chain_length transactions. The nth transaction in the
+    chain is a child of the n-1th transaction and parent of the n+1th transaction.
+    """
+    parent_locking_script = None
+    txid = first_coin["txid"]
+    chain_hex = []
+    chain_txns = []
+    value = first_coin["amount"]
+
+    for _ in range(chain_length):
+        (tx, txhex, value, parent_locking_script) = make_chain(node, address, privkeys, txid, value, 0, parent_locking_script)
+        txid = tx.rehash()
+        chain_hex.append(txhex)
+        chain_txns.append(tx)
+
+    return (chain_hex, chain_txns)
+
+def bulk_transaction(tx, node, target_weight, privkeys, prevtxs=None):
+    """Pad a transaction with extra outputs until it reaches a target weight (or higher).
+    returns CTransaction object
+    """
+    tx_heavy = deepcopy(tx)
+    assert_greater_than_or_equal(target_weight, tx_heavy.get_weight())
+    while tx_heavy.get_weight() < target_weight:
+        random_spk = "6a4d0200"  # OP_RETURN OP_PUSH2 512 bytes
+        for _ in range(512*2):
+            random_spk += choice("0123456789ABCDEF")
+        tx_heavy.vout.append(CTxOut(0, bytes.fromhex(random_spk)))
+    # Re-sign the transaction
+    if privkeys:
+        signed = node.signrawtransactionwithkey(tx_heavy.serialize().hex(), privkeys, prevtxs)
+        return tx_from_hex(signed["hex"])
+    # OP_TRUE
+    tx_heavy.vin[0].scriptSig = CScript([CScript([OP_TRUE])])
+    return tx_heavy

test/functional/test_runner.py

@@ -244,6 +244,7 @@ BASE_SCRIPTS = [
     'rpc_createmultisig.py --legacy-wallet',
     'rpc_createmultisig.py --descriptors',
     'rpc_packages.py',
+    'mempool_package_limits.py',
     'feature_versionbits_warning.py',
     'rpc_preciousblock.py',
     'wallet_importprunedfunds.py --legacy-wallet',