Retropex/bitcoin
1
0
Fork 0
mirror of https://github.com/Retropex/bitcoin.git synced 2025-05-28 13:02:38 +02:00
Code Issues Actions Packages Projects Releases Wiki Activity

Merge 28471 via fix_pkg_eval_sigops_pr28471-25+knots

Browse Source
This commit is contained in:
Luke Dashjr 2023-11-15 23:49:11 +00:00
commit 6e34d1b4d9
9 changed files with 111 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
doc/policy/packages.md
View File

@ -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

8
src/policy/packages.cpp
View File

@ -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");
}

20
src/policy/packages.h
View File

@ -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.
*/

12
src/test/txpackage_tests.cpp
View File

@ -49,14 +49,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;
@ -109,7 +109,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);

22
src/txmempool.cpp
View File

@ -195,13 +195,29 @@ util::Result<CTxMemPool::setEntries> CTxMemPool::CalculateAncestorsAndCheckLimit
}
bool CTxMemPool::CheckPackageLimits(const Package& package,
const int64_t total_vsize,
const Limits& limits,
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;
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) {
@ -216,7 +232,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, limits)};
// It's possible to overestimate the ancestor/descendant totals.
if (!ancestors.has_value()) errString = "possibly " + util::ErrorString(ancestors).original;

2
src/txmempool.h
View File

@ -607,10 +607,12 @@ 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[in] limits Maximum number and size of ancestors and descendants
* @param[out] errString Populated with error reason if a limit is hit.
*/
bool CheckPackageLimits(const Package& package,
int64_t total_vsize,
const Limits& limits,
std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs);

6
src/validation.cpp
View File

@ -623,6 +623,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
@ -978,6 +979,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);
@ -988,7 +990,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, m_limits, err_string)) {
if (!m_pool.CheckPackageLimits(txns, total_vsize, m_limits, 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);
}
@ -1281,7 +1283,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));
}

46
test/functional/mempool_sigoplimit.py
View File

@ -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()

12
test/functional/test_framework/wallet_util.py
View File

@ -119,3 +119,15 @@ def generate_wif_key():
k = ECKey()
k.generate()
return bytes_to_wif(k.get_bytes(), k.is_compressed)
def generate_keypair(compressed=True, wif=False):
"""Generate a new random keypair and return the corresponding ECKey /
bytes objects. The private key can also be provided as WIF (wallet
import format) string instead, which is often useful for wallet RPC
interaction."""
privkey = ECKey()
privkey.generate(compressed)
pubkey = privkey.get_pubkey().get_bytes()
if wif:
privkey = bytes_to_wif(privkey.get_bytes(), compressed)
return privkey, pubkey