Merge bitcoin/bitcoin#28471: Fix virtual size limit enforcement in transaction package context

eb8f58f5e4 Add functional test to catch too large vsize packages (Greg Sanders)
1a579f9d01 Handle over-sized (in virtual bytes) packages with no in-mempool ancestors (Greg Sanders)
bc013fe8e3 Bugfix: Pass correct virtual size to CheckPackageLimits (Luke Dashjr)
533660c58a Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion (Greg Sanders)

Pull request description:

  (Alternative) Minimal subset of https://github.com/bitcoin/bitcoin/pull/28345 to:

  1) Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT which accounts for additional WU necessary to not exclude default chain limit transactions that would have been accepted individually. Avoids sigops vbyte confusion.
  2) pass correct vsize to chain limit evaluations in package context
  3) stop overly-large packages that have no existing mempool ancestors (also a bugfix by itself if someone sets non-standard chain limits)

  This should fix the known issues while not blocking additional refactoring later.

ACKs for top commit:
  achow101:
    ACK eb8f58f5e4
  ariard:
    Re-Code ACK eb8f58f5e
  glozow:
    reACK eb8f58f5e4

Tree-SHA512: 1b5cca1a526207e25d387fcc29a776a3198c3a013dc2b35c6275b9d5a64db2476c154ebf52e3a1aed0b9924c75613f21a946577aa760de28cadf0c9c7f68dc39

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 @@ public:
      * @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 @@ private:
     // 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,6 +26,7 @@ from test_framework.script import (
     OP_TRUE,
 )
 from test_framework.script_util import (
+    keys_to_multisig_script,
     script_to_p2wsh_script,
 )
 from test_framework.test_framework import BitcoinTestFramework
@@ -32,9 +34,10 @@ 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 @@ class BytesPerSigOpTest(BitcoinTestFramework):
         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 @@ class BytesPerSigOpTest(BitcoinTestFramework):
 
             self.generate(self.wallet, 1)
 
+        self.test_sigops_package()
+
 
 if __name__ == '__main__':
     BytesPerSigOpTest().main()