From d578e2e3540e085942001350ff3aeb047bdac973 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 17 May 2024 11:04:18 +0100 Subject: [PATCH 1/3] [policy] explicitly require non-v3 for CPFP carve out This carve out is intended to allow a second child under restricted circumstances, but this topology is not allowed for v3 transactions. As CPFP carve out does not explicitly require a second child to actually exist, it has the effect of granting a free +10KvB descendant size limit when a single child is enough to bust the descendant limit. --- src/validation.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 048c97529a2..3ea25db36cb 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -953,7 +953,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // If the new transaction is relatively small (up to 40k weight) // and has at most one ancestor (ie ancestor limit of 2, including // the new transaction), allow it if its parent has exactly the - // descendant limit descendants. + // descendant limit descendants. The transaction also cannot be v3, + // as its topology restrictions do not allow a second child. // // This allows protocols which rely on distrusting counterparties // being able to broadcast descendants of an unconfirmed transaction @@ -967,7 +968,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) .descendant_size_vbytes = maybe_rbf_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT, }; const auto error_message{util::ErrorString(ancestors).original}; - if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT) { + if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || ws.m_ptx->nVersion == 3) { return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message); } ancestors = m_pool.CalculateMemPoolAncestors(*entry, cpfp_carve_out_limits); From a29f1df289cf27c6cbd565448548b3dc1392a9b0 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 15 Apr 2024 11:45:15 +0100 Subject: [PATCH 2/3] [policy] restrict all v3 transactions to 10kvB --- src/policy/v3_policy.cpp | 12 ++++++ src/policy/v3_policy.h | 7 +++- test/functional/mempool_accept_v3.py | 57 ++++++++++++++++++++++++---- 3 files changed, 67 insertions(+), 9 deletions(-) diff --git a/src/policy/v3_policy.cpp b/src/policy/v3_policy.cpp index 3c3942d7073..d44832fceb0 100644 --- a/src/policy/v3_policy.cpp +++ b/src/policy/v3_policy.cpp @@ -67,6 +67,12 @@ std::optional PackageV3Checks(const CTransactionRef& ptx, int64_t v // Now we have all ancestors, so we can start checking v3 rules. if (ptx->nVersion == 3) { + // SingleV3Checks should have checked this already. + if (!Assume(vsize <= V3_MAX_VSIZE)) { + return strprintf("v3 tx %s (wtxid=%s) is too big: %u > %u virtual bytes", + ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, V3_MAX_VSIZE); + } + if (mempool_ancestors.size() + in_package_parents.size() + 1 > V3_ANCESTOR_LIMIT) { return strprintf("tx %s (wtxid=%s) would have too many ancestors", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); @@ -186,6 +192,12 @@ std::optional> SingleV3Checks(const CTra // The rest of the rules only apply to transactions with nVersion=3. if (ptx->nVersion != 3) return std::nullopt; + if (vsize > V3_MAX_VSIZE) { + return std::make_pair(strprintf("v3 tx %s (wtxid=%s) is too big: %u > %u virtual bytes", + ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, V3_MAX_VSIZE), + nullptr); + } + // Check that V3_ANCESTOR_LIMIT would not be violated. if (mempool_ancestors.size() + 1 > V3_ANCESTOR_LIMIT) { return std::make_pair(strprintf("tx %s (wtxid=%s) would have too many ancestors", diff --git a/src/policy/v3_policy.h b/src/policy/v3_policy.h index 2e56f8822bb..25aff37a1b1 100644 --- a/src/policy/v3_policy.h +++ b/src/policy/v3_policy.h @@ -24,11 +24,13 @@ static constexpr unsigned int V3_DESCENDANT_LIMIT{2}; /** Maximum number of transactions including a V3 tx and all its mempool ancestors. */ static constexpr unsigned int V3_ANCESTOR_LIMIT{2}; +/** Maximum sigop-adjusted virtual size of all v3 transactions. */ +static constexpr int64_t V3_MAX_VSIZE{10000}; /** Maximum sigop-adjusted virtual size of a tx which spends from an unconfirmed v3 transaction. */ static constexpr int64_t V3_CHILD_MAX_VSIZE{1000}; // These limits are within the default ancestor/descendant limits. -static_assert(V3_CHILD_MAX_VSIZE + MAX_STANDARD_TX_WEIGHT / WITNESS_SCALE_FACTOR <= DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1000); -static_assert(V3_CHILD_MAX_VSIZE + MAX_STANDARD_TX_WEIGHT / WITNESS_SCALE_FACTOR <= DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * 1000); +static_assert(V3_MAX_VSIZE + V3_CHILD_MAX_VSIZE <= DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1000); +static_assert(V3_MAX_VSIZE + V3_CHILD_MAX_VSIZE <= DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * 1000); /** Must be called for every transaction, even if not v3. Not strictly necessary for transactions * accepted through AcceptMultipleTransactions. @@ -40,6 +42,7 @@ static_assert(V3_CHILD_MAX_VSIZE + MAX_STANDARD_TX_WEIGHT / WITNESS_SCALE_FACTOR * 4. A v3's descendant set, including itself, must be within V3_DESCENDANT_LIMIT. * 5. If a v3 tx has any unconfirmed ancestors, the tx's sigop-adjusted vsize must be within * V3_CHILD_MAX_VSIZE. + * 6. A v3 tx must be within V3_MAX_VSIZE. * * * @param[in] mempool_ancestors The in-mempool ancestors of ptx. diff --git a/test/functional/mempool_accept_v3.py b/test/functional/mempool_accept_v3.py index 1b55cd0a0db..c8423ad8de9 100755 --- a/test/functional/mempool_accept_v3.py +++ b/test/functional/mempool_accept_v3.py @@ -6,6 +6,7 @@ from decimal import Decimal from test_framework.messages import ( MAX_BIP125_RBF_SEQUENCE, + WITNESS_SCALE_FACTOR, ) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -21,6 +22,7 @@ from test_framework.wallet import ( ) MAX_REPLACEMENT_CANDIDATES = 100 +V3_MAX_VSIZE = 10000 def cleanup(extra_args=None): def decorator(func): @@ -49,6 +51,20 @@ class MempoolAcceptV3(BitcoinTestFramework): assert_equal(len(txids), len(mempool_contents)) assert all([txid in txids for txid in mempool_contents]) + @cleanup(extra_args=["-datacarriersize=20000", "-acceptnonstdtxn=1"]) + def test_v3_max_vsize(self): + node = self.nodes[0] + self.log.info("Test v3-specific maximum transaction vsize") + tx_v3_heavy = self.wallet.create_self_transfer(target_weight=(V3_MAX_VSIZE + 1) * WITNESS_SCALE_FACTOR, version=3) + assert_greater_than_or_equal(tx_v3_heavy["tx"].get_vsize(), V3_MAX_VSIZE) + expected_error_heavy = f"v3-rule-violation, v3 tx {tx_v3_heavy['txid']} (wtxid={tx_v3_heavy['wtxid']}) is too big" + assert_raises_rpc_error(-26, expected_error_heavy, node.sendrawtransaction, tx_v3_heavy["hex"]) + self.check_mempool([]) + + # Ensure we are hitting the v3-specific limit and not something else + tx_v2_heavy = self.wallet.send_self_transfer(from_node=node, target_weight=(V3_MAX_VSIZE + 1) * WITNESS_SCALE_FACTOR, version=2) + self.check_mempool([tx_v2_heavy["txid"]]) + @cleanup(extra_args=["-datacarriersize=1000", "-acceptnonstdtxn=1"]) def test_v3_acceptance(self): node = self.nodes[0] @@ -201,10 +217,24 @@ class MempoolAcceptV3(BitcoinTestFramework): """ node = self.nodes[0] self.log.info("Test that a decreased limitdescendantsize also applies to v3 child") - tx_v3_parent_large1 = self.wallet.send_self_transfer(from_node=node, target_weight=99900, version=3) - tx_v3_child_large1 = self.wallet.create_self_transfer(utxo_to_spend=tx_v3_parent_large1["new_utxo"], version=3) - # Child is within v3 limits, but parent's descendant limit is exceeded - assert_greater_than(1000, tx_v3_child_large1["tx"].get_vsize()) + parent_target_weight = 9990 * WITNESS_SCALE_FACTOR + child_target_weight = 500 * WITNESS_SCALE_FACTOR + tx_v3_parent_large1 = self.wallet.send_self_transfer( + from_node=node, + target_weight=parent_target_weight, + version=3 + ) + tx_v3_child_large1 = self.wallet.create_self_transfer( + utxo_to_spend=tx_v3_parent_large1["new_utxo"], + target_weight=child_target_weight, + version=3 + ) + + # Parent and child are within v3 limits, but parent's 10kvB descendant limit is exceeded + assert_greater_than_or_equal(V3_MAX_VSIZE, tx_v3_parent_large1["tx"].get_vsize()) + assert_greater_than_or_equal(1000, tx_v3_child_large1["tx"].get_vsize()) + assert_greater_than(tx_v3_parent_large1["tx"].get_vsize() + tx_v3_child_large1["tx"].get_vsize(), 10000) + assert_raises_rpc_error(-26, f"too-long-mempool-chain, exceeds descendant size limit for tx {tx_v3_parent_large1['txid']}", node.sendrawtransaction, tx_v3_child_large1["hex"]) self.check_mempool([tx_v3_parent_large1["txid"]]) assert_equal(node.getmempoolentry(tx_v3_parent_large1["txid"])["descendantcount"], 1) @@ -212,10 +242,22 @@ class MempoolAcceptV3(BitcoinTestFramework): self.log.info("Test that a decreased limitancestorsize also applies to v3 parent") self.restart_node(0, extra_args=["-limitancestorsize=10", "-datacarriersize=40000", "-acceptnonstdtxn=1"]) - tx_v3_parent_large2 = self.wallet.send_self_transfer(from_node=node, target_weight=99900, version=3) - tx_v3_child_large2 = self.wallet.create_self_transfer(utxo_to_spend=tx_v3_parent_large2["new_utxo"], version=3) - # Child is within v3 limits + tx_v3_parent_large2 = self.wallet.send_self_transfer( + from_node=node, + target_weight=parent_target_weight, + version=3 + ) + tx_v3_child_large2 = self.wallet.create_self_transfer( + utxo_to_spend=tx_v3_parent_large2["new_utxo"], + target_weight=child_target_weight, + version=3 + ) + + # Parent and child are within v3 limits + assert_greater_than_or_equal(V3_MAX_VSIZE, tx_v3_parent_large2["tx"].get_vsize()) assert_greater_than_or_equal(1000, tx_v3_child_large2["tx"].get_vsize()) + assert_greater_than(tx_v3_parent_large2["tx"].get_vsize() + tx_v3_child_large2["tx"].get_vsize(), 10000) + assert_raises_rpc_error(-26, f"too-long-mempool-chain, exceeds ancestor size limit", node.sendrawtransaction, tx_v3_child_large2["hex"]) self.check_mempool([tx_v3_parent_large2["txid"]]) @@ -585,6 +627,7 @@ class MempoolAcceptV3(BitcoinTestFramework): node = self.nodes[0] self.wallet = MiniWallet(node) self.generate(self.wallet, 120) + self.test_v3_max_vsize() self.test_v3_acceptance() self.test_v3_replacement() self.test_v3_bip125() From 154b2b2296edccb5ed24e829798dacb6195edc11 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 20 May 2024 10:30:18 +0100 Subject: [PATCH 3/3] [fuzz] V3_MAX_VSIZE and effective ancestor/descendant size limits --- src/test/util/txmempool.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index 71cf8aca605..585fc39bf21 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -125,9 +125,15 @@ void CheckMempoolV3Invariants(const CTxMemPool& tx_pool) for (const auto& tx_info : tx_pool.infoAll()) { const auto& entry = *Assert(tx_pool.GetEntry(tx_info.tx->GetHash())); if (tx_info.tx->nVersion == 3) { + // Check that special maximum virtual size is respected + Assert(entry.GetTxSize() <= V3_MAX_VSIZE); + // Check that special v3 ancestor/descendant limits and rules are always respected Assert(entry.GetCountWithDescendants() <= V3_DESCENDANT_LIMIT); Assert(entry.GetCountWithAncestors() <= V3_ANCESTOR_LIMIT); + Assert(entry.GetSizeWithDescendants() <= V3_MAX_VSIZE + V3_CHILD_MAX_VSIZE); + Assert(entry.GetSizeWithAncestors() <= V3_MAX_VSIZE + V3_CHILD_MAX_VSIZE); + // If this transaction has at least 1 ancestor, it's a "child" and has restricted weight. if (entry.GetCountWithAncestors() > 1) { Assert(entry.GetTxSize() <= V3_CHILD_MAX_VSIZE);