Fix virtual size limit enforcement in transaction package context

doc/policy/packages.md

@@ -18,16 +18,19 @@ tip or some preceding transaction in the package.
 
 The following rules are enforced for all packages:
 
-* Packages cannot exceed `MAX_PACKAGE_COUNT=25` count and `MAX_PACKAGE_SIZE=101KvB` total size
+* Packages cannot exceed `MAX_PACKAGE_COUNT=25` count and `MAX_PACKAGE_WEIGHT=404000` total weight
    (#20833)
 
-   - *Rationale*: This is already enforced as mempool ancestor/descendant limits. If
-     transactions in a package are all related, exceeding this limit would mean that the package
-     can either be split up or it wouldn't pass individual mempool policy.
+   - *Rationale*: We want package size to be as small as possible to mitigate DoS via package
+     validation. However, we want to make sure that the limit does not restrict ancestor
+     packages that would be allowed if submitted individually.
 
    - Note that, if these mempool limits change, package limits should be reconsidered. Users may
      also configure their mempool limits differently.
 
+   - Note that the this is transaction weight, not "virtual" size as with other limits to allow
+     simpler context-less checks.
+
 * Packages must be topologically sorted. (#20833)
 
 * Packages cannot have conflicting transactions, i.e. no two transactions in a package can spend

src/policy/packages.cpp

@@ -23,10 +23,10 @@ bool CheckPackage(const Package& txns, PackageValidationState& state)
         return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-many-transactions");
     }
 
-    const int64_t total_size = std::accumulate(txns.cbegin(), txns.cend(), 0,
-                               [](int64_t sum, const auto& tx) { return sum + GetVirtualTransactionSize(*tx); });
-    // If the package only contains 1 tx, it's better to report the policy violation on individual tx size.
-    if (package_count > 1 && total_size > MAX_PACKAGE_SIZE * 1000) {
+    const int64_t total_weight = std::accumulate(txns.cbegin(), txns.cend(), 0,
+                               [](int64_t sum, const auto& tx) { return sum + GetTransactionWeight(*tx); });
+    // If the package only contains 1 tx, it's better to report the policy violation on individual tx weight.
+    if (package_count > 1 && total_weight > MAX_PACKAGE_WEIGHT) {
         return state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-large");
     }
 

src/policy/packages.h

@@ -15,18 +15,22 @@
 
 /** Default maximum number of transactions in a package. */
 static constexpr uint32_t MAX_PACKAGE_COUNT{25};
-/** Default maximum total virtual size of transactions in a package in KvB. */
-static constexpr uint32_t MAX_PACKAGE_SIZE{101};
-static_assert(MAX_PACKAGE_SIZE * WITNESS_SCALE_FACTOR * 1000 >= MAX_STANDARD_TX_WEIGHT);
+/** Default maximum total weight of transactions in a package in weight
+    to allow for context-less checks. This must allow a superset of sigops
+    weighted vsize limited transactions to not disallow transactions we would
+    have otherwise accepted individually. */
+static constexpr uint32_t MAX_PACKAGE_WEIGHT = 404'000;
+static_assert(MAX_PACKAGE_WEIGHT >= MAX_STANDARD_TX_WEIGHT);
 
-// If a package is submitted, it must be within the mempool's ancestor/descendant limits. Since a
-// submitted package must be child-with-unconfirmed-parents (all of the transactions are an ancestor
+// If a package is to be evaluated, it must be at least as large as the mempool's ancestor/descendant limits,
+// otherwise transactions that would be individually accepted may be rejected in a package erroneously.
+// Since a submitted package must be child-with-unconfirmed-parents (all of the transactions are an ancestor
 // of the child), package limits are ultimately bounded by mempool package limits. Ensure that the
 // defaults reflect this constraint.
 static_assert(DEFAULT_DESCENDANT_LIMIT >= MAX_PACKAGE_COUNT);
 static_assert(DEFAULT_ANCESTOR_LIMIT >= MAX_PACKAGE_COUNT);
-static_assert(DEFAULT_ANCESTOR_SIZE_LIMIT_KVB >= MAX_PACKAGE_SIZE);
-static_assert(DEFAULT_DESCENDANT_SIZE_LIMIT_KVB >= MAX_PACKAGE_SIZE);
+static_assert(MAX_PACKAGE_WEIGHT >= DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * WITNESS_SCALE_FACTOR * 1000);
+static_assert(MAX_PACKAGE_WEIGHT >= DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * WITNESS_SCALE_FACTOR * 1000);
 
 /** A "reason" why a package was invalid. It may be that one or more of the included
  * transactions is invalid or the package itself violates our rules.
@@ -47,7 +51,7 @@ class PackageValidationState : public ValidationState<PackageValidationResult> {
 
 /** Context-free package policy checks:
  * 1. The number of transactions cannot exceed MAX_PACKAGE_COUNT.
- * 2. The total virtual size cannot exceed MAX_PACKAGE_SIZE.
+ * 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.
  */

src/test/txpackage_tests.cpp

@@ -51,14 +51,14 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup)
     BOOST_CHECK_EQUAL(state_too_many.GetResult(), PackageValidationResult::PCKG_POLICY);
     BOOST_CHECK_EQUAL(state_too_many.GetRejectReason(), "package-too-many-transactions");
 
-    // Packages can't have a total size of more than 101KvB.
+    // Packages can't have a total weight of more than 404'000WU.
     CTransactionRef large_ptx = create_placeholder_tx(150, 150);
     Package package_too_large;
-    auto size_large = GetVirtualTransactionSize(*large_ptx);
-    size_t total_size{0};
-    while (total_size <= MAX_PACKAGE_SIZE * 1000) {
+    auto size_large = GetTransactionWeight(*large_ptx);
+    size_t total_weight{0};
+    while (total_weight <= MAX_PACKAGE_WEIGHT) {
         package_too_large.push_back(large_ptx);
-        total_size += size_large;
+        total_weight += size_large;
     }
     BOOST_CHECK(package_too_large.size() <= MAX_PACKAGE_COUNT);
     PackageValidationState state_too_large;
@@ -122,7 +122,7 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup)
 
     // A single, giant transaction submitted through ProcessNewPackage fails on single tx policy.
     CTransactionRef giant_ptx = create_placeholder_tx(999, 999);
-    BOOST_CHECK(GetVirtualTransactionSize(*giant_ptx) > MAX_PACKAGE_SIZE * 1000);
+    BOOST_CHECK(GetVirtualTransactionSize(*giant_ptx) > DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1000);
     auto result_single_large = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, {giant_ptx}, /*test_accept=*/true);
     BOOST_CHECK(result_single_large.m_state.IsInvalid());
     BOOST_CHECK_EQUAL(result_single_large.m_state.GetResult(), PackageValidationResult::PCKG_TX);

src/txmempool.cpp

@@ -197,12 +197,28 @@ util::Result<CTxMemPool::setEntries> CTxMemPool::CalculateAncestorsAndCheckLimit
 }
 
 bool CTxMemPool::CheckPackageLimits(const Package& package,
+                                    const int64_t total_vsize,
                                     std::string &errString) const
 {
+    size_t pack_count = package.size();
+
+    // Package itself is busting mempool limits; should be rejected even if no staged_ancestors exist
+    if (pack_count > static_cast<uint64_t>(m_limits.ancestor_count)) {
+        errString = strprintf("package count %u exceeds ancestor count limit [limit: %u]", pack_count, m_limits.ancestor_count);
+        return false;
+    } else if (pack_count > static_cast<uint64_t>(m_limits.descendant_count)) {
+        errString = strprintf("package count %u exceeds descendant count limit [limit: %u]", pack_count, m_limits.descendant_count);
+        return false;
+    } else if (total_vsize > m_limits.ancestor_size_vbytes) {
+        errString = strprintf("package size %u exceeds ancestor size limit [limit: %u]", total_vsize, m_limits.ancestor_size_vbytes);
+        return false;
+    } else if (total_vsize > m_limits.descendant_size_vbytes) {
+        errString = strprintf("package size %u exceeds descendant size limit [limit: %u]", total_vsize, m_limits.descendant_size_vbytes);
+        return false;
+    }
+
     CTxMemPoolEntry::Parents staged_ancestors;
-    int64_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) {
@@ -217,7 +233,7 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
     // 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.
-    const auto ancestors{CalculateAncestorsAndCheckLimits(total_size, package.size(),
+    const auto ancestors{CalculateAncestorsAndCheckLimits(total_vsize, package.size(),
                                                           staged_ancestors, m_limits)};
     // It's possible to overestimate the ancestor/descendant totals.
     if (!ancestors.has_value()) errString = "possibly " + util::ErrorString(ancestors).original;

src/txmempool.h

@@ -606,9 +606,11 @@ class CTxMemPool
      * @param[in]       package                 Transaction package being evaluated for acceptance
      *                                          to mempool. The transactions need not be direct
      *                                          ancestors/descendants of each other.
+     * @param[in]       total_vsize             Sum of virtual sizes for all transactions in package.
      * @param[out]      errString               Populated with error reason if a limit is hit.
      */
     bool CheckPackageLimits(const Package& package,
+                            int64_t total_vsize,
                             std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     /** Populate setDescendants with all in-mempool descendants of hash.

src/validation.cpp

@@ -634,6 +634,7 @@ class MemPoolAccept
     // Enforce package mempool ancestor/descendant limits (distinct from individual
     // ancestor/descendant limits done in PreChecks).
     bool PackageMempoolChecks(const std::vector<CTransactionRef>& txns,
+                              int64_t total_vsize,
                               PackageValidationState& package_state) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs);
 
     // Run the script checks using our policy flags. As this can be slow, we should
@@ -1003,6 +1004,7 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws)
 }
 
 bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txns,
+                                         const int64_t total_vsize,
                                          PackageValidationState& package_state)
 {
     AssertLockHeld(cs_main);
@@ -1013,7 +1015,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
                        { return !m_pool.exists(GenTxid::Txid(tx->GetHash()));}));
 
     std::string err_string;
-    if (!m_pool.CheckPackageLimits(txns, err_string)) {
+    if (!m_pool.CheckPackageLimits(txns, total_vsize, err_string)) {
         // This is a package-wide error, separate from an individual transaction error.
         return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", err_string);
     }
@@ -1298,7 +1300,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
     // 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 && !PackageMempoolChecks(txns, package_state)) {
+    if (txns.size() > 1 && !PackageMempoolChecks(txns, m_total_vsize, package_state)) {
         return PackageMempoolAcceptResult(package_state, std::move(results));
     }
 

test/functional/mempool_sigoplimit.py

@@ -3,6 +3,7 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test sigop limit mempool policy (`-bytespersigop` parameter)"""
+from decimal import Decimal
 from math import ceil
 
 from test_framework.messages import (
@@ -25,16 +26,18 @@
     OP_TRUE,
 )
 from test_framework.script_util import (
+    keys_to_multisig_script,
     script_to_p2wsh_script,
 )
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
     assert_greater_than,
     assert_greater_than_or_equal,
+    assert_raises_rpc_error,
 )
 from test_framework.wallet import MiniWallet
-
+from test_framework.wallet_util import generate_keypair
 
 DEFAULT_BYTES_PER_SIGOP = 20  # default setting
 
@@ -133,6 +136,45 @@ def test_sigops_limit(self, bytes_per_sigop, num_sigops):
         assert_equal(entry_parent['descendantcount'], 2)
         assert_equal(entry_parent['descendantsize'], parent_tx.get_vsize() + sigop_equivalent_vsize)
 
+    def test_sigops_package(self):
+        self.log.info("Test a overly-large sigops-vbyte hits package limits")
+        # Make a 2-transaction package which fails vbyte checks even though
+        # separately they would work.
+        self.restart_node(0, extra_args=["-bytespersigop=5000"] + self.extra_args[0])
+
+        def create_bare_multisig_tx(utxo_to_spend=None):
+            _, pubkey = generate_keypair()
+            amount_for_bare = 50000
+            tx_dict = self.wallet.create_self_transfer(fee=Decimal("3"), utxo_to_spend=utxo_to_spend)
+            tx_utxo = tx_dict["new_utxo"]
+            tx = tx_dict["tx"]
+            tx.vout.append(CTxOut(amount_for_bare, keys_to_multisig_script([pubkey], k=1)))
+            tx.vout[0].nValue -= amount_for_bare
+            tx_utxo["txid"] = tx.rehash()
+            tx_utxo["value"] -= Decimal("0.00005000")
+            return (tx_utxo, tx)
+
+        tx_parent_utxo, tx_parent = create_bare_multisig_tx()
+        tx_child_utxo, tx_child = create_bare_multisig_tx(tx_parent_utxo)
+
+        # Separately, the parent tx is ok
+        parent_individual_testres = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex()])[0]
+        assert parent_individual_testres["allowed"]
+        # Multisig is counted as MAX_PUBKEYS_PER_MULTISIG = 20 sigops
+        assert_equal(parent_individual_testres["vsize"], 5000 * 20)
+
+        # But together, it's exceeding limits in the *package* context. If sigops adjusted vsize wasn't being checked
+        # here, it would get further in validation and give too-long-mempool-chain error instead.
+        packet_test = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex(), tx_child.serialize().hex()])
+        assert_equal([x["package-error"] for x in packet_test], ["package-mempool-limits", "package-mempool-limits"])
+
+        # When we actually try to submit, the parent makes it into the mempool, but the child would exceed ancestor vsize limits
+        assert_raises_rpc_error(-26, "too-long-mempool-chain", self.nodes[0].submitpackage, [tx_parent.serialize().hex(), tx_child.serialize().hex()])
+        assert tx_parent.rehash() in self.nodes[0].getrawmempool()
+
+        # Transactions are tiny in weight
+        assert_greater_than(2000, tx_parent.get_weight() + tx_child.get_weight())
+
     def run_test(self):
         self.wallet = MiniWallet(self.nodes[0])
 
@@ -149,6 +191,8 @@ def run_test(self):
 
             self.generate(self.wallet, 1)
 
+        self.test_sigops_package()
+
 
 if __name__ == '__main__':
     BytesPerSigOpTest().main()