From 533660c58ad5a218671a9bc1537299b1d67bb55d Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Wed, 13 Sep 2023 13:13:57 -0400 Subject: [PATCH 1/4] Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion While allowing submitted packages to be slightly larger than what may be allowed in the mempool to allow simpler reasoning about contextual-less checks vs chain limits. --- doc/policy/packages.md | 11 +++++++---- src/policy/packages.cpp | 8 ++++---- src/policy/packages.h | 20 ++++++++++++-------- src/test/txpackage_tests.cpp | 12 ++++++------ 4 files changed, 29 insertions(+), 22 deletions(-) diff --git a/doc/policy/packages.md b/doc/policy/packages.md index 2a5758318a9..399ae945d52 100644 --- a/doc/policy/packages.md +++ b/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 diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp index a901ef8f38c..fd272a2642e 100644 --- a/src/policy/packages.cpp +++ b/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"); } diff --git a/src/policy/packages.h b/src/policy/packages.h index 0a0e7cf6bb8..702667b8ade 100644 --- a/src/policy/packages.h +++ b/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 { /** 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. */ diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 10ab656d388..571b58156f7 100644 --- a/src/test/txpackage_tests.cpp +++ b/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); From bc013fe8e3d0bae2ab766a8248219aa3c9e344df Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 26 Aug 2023 00:09:16 +0000 Subject: [PATCH 2/4] Bugfix: Pass correct virtual size to CheckPackageLimits --- src/txmempool.cpp | 5 ++--- src/txmempool.h | 2 ++ src/validation.cpp | 6 ++++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index bedb57c13cc..960be428c65 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -197,12 +197,11 @@ util::Result CTxMemPool::CalculateAncestorsAndCheckLimit } bool CTxMemPool::CheckPackageLimits(const Package& package, + const int64_t total_vsize, std::string &errString) const { 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 piter = GetIter(input.prevout.hash); if (piter) { @@ -217,7 +216,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; diff --git a/src/txmempool.h b/src/txmempool.h index b26cd4efa6f..8ea3373cb47 100644 --- a/src/txmempool.h +++ b/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. diff --git a/src/validation.cpp b/src/validation.cpp index 8b5acf9ad17..db3310c2c36 100644 --- a/src/validation.cpp +++ b/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& 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& txns, + const int64_t total_vsize, PackageValidationState& package_state) { AssertLockHeld(cs_main); @@ -1013,7 +1015,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& 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)); } From 1a579f9d01256b0b2570168496d129a8b2009b35 Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Mon, 11 Sep 2023 09:55:39 -0400 Subject: [PATCH 3/4] Handle over-sized (in virtual bytes) packages with no in-mempool ancestors --- src/txmempool.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 960be428c65..e021cfb06e3 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -200,6 +200,23 @@ 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(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(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; for (const auto& tx : package) { for (const auto& input : tx->vin) { From eb8f58f5e4a249d55338304099e8238896d2316d Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Thu, 14 Sep 2023 16:29:55 -0400 Subject: [PATCH 4/4] Add functional test to catch too large vsize packages --- test/functional/mempool_sigoplimit.py | 46 ++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/test/functional/mempool_sigoplimit.py b/test/functional/mempool_sigoplimit.py index 962b2b19bd8..fbec6d0dc8b 100755 --- a/test/functional/mempool_sigoplimit.py +++ b/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 @@ 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 @@ 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()