From cf3ab8e1d0a2f2bdf72e61e2c2dcb35987e5b9bd Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Tue, 3 Oct 2023 13:56:22 -0400 Subject: [PATCH] Stop enforcing descendant size/count limits Cluster size limits should be enough. --- src/txmempool.cpp | 15 ------- test/functional/feature_rbf.py | 9 ---- test/functional/mempool_package_limits.py | 5 ++- test/functional/mempool_packages.py | 51 +++++----------------- test/functional/mempool_sigoplimit.py | 8 ++-- test/functional/mempool_truc.py | 13 +++--- test/functional/mempool_updatefromblock.py | 3 +- 7 files changed, 26 insertions(+), 78 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index ce9218f9111..ab6d3ed229f 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -177,12 +177,6 @@ util::Result CTxMemPool::CalculateAncestorsAndCheckLimit ancestors.insert(stageit); staged_ancestors.erase(stage); - if (stageit->GetSizeWithDescendants() + entry_size > limits.descendant_size_vbytes) { - return util::Error{Untranslated(strprintf("exceeds descendant size limit for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_size_vbytes))}; - } else if (stageit->GetCountWithDescendants() + entry_count > static_cast(limits.descendant_count)) { - return util::Error{Untranslated(strprintf("too many descendants for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_count))}; - } - const CTxMemPoolEntry::Parents& parents = stageit->GetMemPoolParentsConst(); for (const CTxMemPoolEntry& parent : parents) { txiter parent_it = mapTx.iterator_to(parent); @@ -200,15 +194,6 @@ util::Result CTxMemPool::CalculateAncestorsAndCheckLimit util::Result CTxMemPool::CheckPackageLimits(const Package& package, const int64_t total_vsize) 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_opts.limits.descendant_count)) { - return util::Error{Untranslated(strprintf("package count %u exceeds descendant count limit [limit: %u]", pack_count, m_opts.limits.descendant_count))}; - } else if (total_vsize > m_opts.limits.descendant_size_vbytes) { - return util::Error{Untranslated(strprintf("package size %u exceeds descendant size limit [limit: %u]", total_vsize, m_opts.limits.descendant_size_vbytes))}; - } - CTxMemPoolEntry::Parents staged_ancestors; for (const auto& tx : package) { for (const auto& input : tx->vin) { diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py index 81ecef540ef..0590af129dd 100755 --- a/test/functional/feature_rbf.py +++ b/test/functional/feature_rbf.py @@ -27,15 +27,6 @@ MAX_CLUSTER_LIMIT = 64 class ReplaceByFeeTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 2 - self.extra_args = [ - [ - "-limitancestorcount=64", - "-limitdescendantcount=64", - ], - # second node has default mempool parameters - [ - ], - ] self.uses_wallet = None def run_test(self): diff --git a/test/functional/mempool_package_limits.py b/test/functional/mempool_package_limits.py index 5e402ede4fb..3f26777941d 100755 --- a/test/functional/mempool_package_limits.py +++ b/test/functional/mempool_package_limits.py @@ -15,7 +15,7 @@ from test_framework.wallet import MiniWallet # 2) run the subtest, which may submit some transaction(s) to the mempool and # create a list of hex transactions # 3) testmempoolaccept the package hex and check that it fails with the error -# "package-mempool-limits" for each tx +# "too-large-cluster" for each tx # 4) after mining a block, clearing the pre-submitted transactions from mempool, # check that submitting the created package succeeds def check_package_limits(func): @@ -26,7 +26,7 @@ def check_package_limits(func): testres_error_expected = node.testmempoolaccept(rawtxs=package_hex) assert_equal(len(testres_error_expected), len(package_hex)) for txres in testres_error_expected: - assert "package-mempool-limits" in txres["package-error"] + assert "too-large-cluster" in txres["package-error"] # Clear mempool and check that the package passes now self.generate(node, 1) @@ -39,6 +39,7 @@ class MempoolPackageLimitsTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 self.setup_clean_chain = True + self.extra_args = [["-limitclustercount=25"]] def run_test(self): self.wallet = MiniWallet(self.nodes[0]) diff --git a/test/functional/mempool_packages.py b/test/functional/mempool_packages.py index 74d04f077f9..9cf1aa01f56 100755 --- a/test/functional/mempool_packages.py +++ b/test/functional/mempool_packages.py @@ -7,19 +7,18 @@ from decimal import Decimal from test_framework.messages import ( - DEFAULT_DESCENDANT_LIMIT, DEFAULT_CLUSTER_LIMIT, ) from test_framework.p2p import P2PTxInvStore from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, - assert_raises_rpc_error, ) from test_framework.wallet import MiniWallet # custom limits for node1 -CUSTOM_DESCENDANT_LIMIT = 11 +CUSTOM_CLUSTER_LIMIT = 10 +assert CUSTOM_CLUSTER_LIMIT < DEFAULT_CLUSTER_LIMIT class MempoolPackagesTest(BitcoinTestFramework): def set_test_params(self): @@ -30,7 +29,7 @@ class MempoolPackagesTest(BitcoinTestFramework): [ ], [ - "-limitdescendantcount={}".format(CUSTOM_DESCENDANT_LIMIT), + "-limitclustercount={}".format(CUSTOM_CLUSTER_LIMIT), ], ] @@ -40,8 +39,8 @@ class MempoolPackagesTest(BitcoinTestFramework): peer_inv_store = self.nodes[0].add_p2p_connection(P2PTxInvStore()) # keep track of invs - # DEFAULT_DESCENDANT_LIMIT transactions off a confirmed tx should be fine - chain = self.wallet.create_self_transfer_chain(chain_length=DEFAULT_DESCENDANT_LIMIT) + # DEFAULT_CLUSTER_LIMIT transactions off a confirmed tx should be fine for default node + chain = self.wallet.create_self_transfer_chain(chain_length=DEFAULT_CLUSTER_LIMIT) witness_chain = [t["wtxid"] for t in chain] ancestor_vsize = 0 ancestor_fees = Decimal(0) @@ -55,22 +54,18 @@ class MempoolPackagesTest(BitcoinTestFramework): # Otherwise, getrawmempool may be inconsistent with getmempoolentry if unbroadcast changes in between peer_inv_store.wait_for_broadcast(witness_chain) - # Check mempool has DEFAULT_DESCENDANT_LIMIT transactions in it, and descendant and ancestor + # Check mempool has DEFAULT_CLUSTER_LIMIT transactions in it, and descendant and ancestor # count and fees should look correct mempool = self.nodes[0].getrawmempool(True) - assert_equal(len(mempool), DEFAULT_DESCENDANT_LIMIT) + assert_equal(len(mempool), DEFAULT_CLUSTER_LIMIT) descendant_count = 1 descendant_fees = 0 descendant_vsize = 0 assert_equal(ancestor_vsize, sum([mempool[tx]['vsize'] for tx in mempool])) - ancestor_count = DEFAULT_DESCENDANT_LIMIT + ancestor_count = DEFAULT_CLUSTER_LIMIT assert_equal(ancestor_fees, sum([mempool[tx]['fees']['base'] for tx in mempool])) - # Adding one more transaction on to the chain should fail. - next_hop = self.wallet.create_self_transfer(utxo_to_spend=chain[-1]["new_utxo"])["hex"] - assert_raises_rpc_error(-26, "too-long-mempool-chain", lambda: self.nodes[0].sendrawtransaction(next_hop)) - descendants = [] ancestors = [t["txid"] for t in chain] chain = [t["txid"] for t in chain] @@ -186,23 +181,7 @@ class MempoolPackagesTest(BitcoinTestFramework): assert_equal(entry['fees']['modified'], entry['fees']['base'] + Decimal("0.00002")) assert_equal(entry['fees']['descendant'], descendant_fees + Decimal("0.00002")) - # Check that node1's mempool is as expected (-> custom ancestor limit) - mempool0 = self.nodes[0].getrawmempool(False) - mempool1 = self.nodes[1].getrawmempool(False) - assert_equal(len(mempool1), CUSTOM_DESCENDANT_LIMIT) - assert set(mempool1).issubset(set(mempool0)) - for tx in chain[:CUSTOM_DESCENDANT_LIMIT]: - assert tx in mempool1 - entry0 = self.nodes[0].getmempoolentry(tx) - entry1 = self.nodes[1].getmempoolentry(tx) - assert not entry0['unbroadcast'] - assert not entry1['unbroadcast'] - assert_equal(entry1['fees']['base'], entry0['fees']['base']) - assert_equal(entry1['vsize'], entry0['vsize']) - assert_equal(entry1['depends'], entry0['depends']) - # Now test descendant chain limits - tx_children = [] # First create one parent tx with 10 children tx_with_children = self.wallet.send_self_transfer_multi(from_node=self.nodes[0], num_outputs=10) @@ -211,7 +190,7 @@ class MempoolPackagesTest(BitcoinTestFramework): # Sign and send up to MAX_DESCENDANT transactions chained off the parent tx chain = [] # save sent txs for the purpose of checking node1's mempool later (see below) - for _ in range(DEFAULT_DESCENDANT_LIMIT - 1): + for _ in range(DEFAULT_CLUSTER_LIMIT - 1): utxo = transaction_package.pop(0) new_tx = self.wallet.send_self_transfer_multi(from_node=self.nodes[0], num_outputs=10, utxos_to_spend=[utxo]) txid = new_tx["txid"] @@ -221,22 +200,16 @@ class MempoolPackagesTest(BitcoinTestFramework): transaction_package.extend(new_tx["new_utxos"]) mempool = self.nodes[0].getrawmempool(True) - assert_equal(mempool[parent_transaction]['descendantcount'], DEFAULT_DESCENDANT_LIMIT) + assert_equal(mempool[parent_transaction]['descendantcount'], DEFAULT_CLUSTER_LIMIT) assert_equal(sorted(mempool[parent_transaction]['spentby']), sorted(tx_children)) for child in tx_children: assert_equal(mempool[child]['depends'], [parent_transaction]) - # Sending one more chained transaction will fail - next_hop = self.wallet.create_self_transfer(utxo_to_spend=transaction_package.pop(0))["hex"] - assert_raises_rpc_error(-26, "too-long-mempool-chain", lambda: self.nodes[0].sendrawtransaction(next_hop)) - # Check that node1's mempool is as expected, containing: - # - txs from previous ancestor test (-> custom ancestor limit) # - parent tx for descendant test # - txs chained off parent tx (-> custom descendant limit) - self.wait_until(lambda: len(self.nodes[1].getrawmempool()) == - 2*CUSTOM_DESCENDANT_LIMIT, timeout=10) + self.wait_until(lambda: len(self.nodes[1].getrawmempool()) == 2*CUSTOM_CLUSTER_LIMIT, timeout=10) mempool0 = self.nodes[0].getrawmempool(False) mempool1 = self.nodes[1].getrawmempool(False) assert set(mempool1).issubset(set(mempool0)) @@ -247,7 +220,7 @@ class MempoolPackagesTest(BitcoinTestFramework): entry1 = self.nodes[1].getmempoolentry(tx) assert not entry0['unbroadcast'] assert not entry1['unbroadcast'] - assert entry1["descendantcount"] <= CUSTOM_DESCENDANT_LIMIT + assert entry1["descendantcount"] <= CUSTOM_CLUSTER_LIMIT assert_equal(entry1['fees']['base'], entry0['fees']['base']) assert_equal(entry1['vsize'], entry0['vsize']) assert_equal(entry1['depends'], entry0['depends']) diff --git a/test/functional/mempool_sigoplimit.py b/test/functional/mempool_sigoplimit.py index 965f9a5c56d..3b2d737302e 100755 --- a/test/functional/mempool_sigoplimit.py +++ b/test/functional/mempool_sigoplimit.py @@ -169,14 +169,14 @@ class BytesPerSigOpTest(BitcoinTestFramework): assert_equal(parent_individual_testres["vsize"], max_multisig_vsize) # 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. + # here, it would get further in validation and give too-large-cluster error instead. packet_test = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex(), tx_child.serialize().hex()]) - expected_package_error = f"package-mempool-limits, package size {2*max_multisig_vsize} exceeds descendant size limit [limit: 101000]" + expected_package_error = "too-large-cluster" assert_equal([x["package-error"] for x in packet_test], [expected_package_error] * 2) - # When we actually try to submit, the parent makes it into the mempool, but the child would exceed ancestor vsize limits + # When we actually try to submit, the parent makes it into the mempool, but the child would exceed cluster vsize limits res = self.nodes[0].submitpackage([tx_parent.serialize().hex(), tx_child.serialize().hex()]) - assert "too-long-mempool-chain" in res["tx-results"][tx_child.wtxid_hex]["error"] + assert "too-large-cluster" in res["tx-results"][tx_child.wtxid_hex]["error"] assert tx_parent.txid_hex in self.nodes[0].getrawmempool() # Transactions are tiny in weight diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py index 687b707297f..6ba8cbffef0 100755 --- a/test/functional/mempool_truc.py +++ b/test/functional/mempool_truc.py @@ -196,15 +196,15 @@ class MempoolTRUC(BitcoinTestFramework): node.invalidateblock(block["hash"]) self.check_mempool([tx_v3_block["txid"], tx_v2_block["txid"], tx_v3_block2["txid"], tx_v2_from_v3["txid"], tx_v3_from_v2["txid"], tx_v3_child_large["txid"], tx_chain_1["txid"], tx_chain_2["txid"], tx_chain_3["txid"], tx_chain_4["txid"]]) - @cleanup(extra_args=["-limitdescendantsize=10"]) + @cleanup(extra_args=["-limitclustercount=1"]) def test_nondefault_package_limits(self): """ - Max standard tx size + TRUC rules imply the ancestor/descendant rules (at their default + Max standard tx size + TRUC rules imply the cluster rules (at their default values), but those checks must not be skipped. Ensure both sets of checks are done by - changing the ancestor/descendant limit configurations. + changing the cluster limit configurations. """ node = self.nodes[0] - self.log.info("Test that a decreased limitdescendantsize also applies to TRUC child") + self.log.info("Test that a decreased cluster count limit also applies to TRUC child") parent_target_vsize = 9990 child_target_vsize = 500 tx_v3_parent_large1 = self.wallet.send_self_transfer( @@ -218,12 +218,11 @@ class MempoolTRUC(BitcoinTestFramework): version=3 ) - # Parent and child are within v3 limits, but parent's 10kvB descendant limit is exceeded + # Parent and child are within v3 limits, but cluster count limit is exceeded. assert_greater_than_or_equal(TRUC_MAX_VSIZE, tx_v3_parent_large1["tx"].get_vsize()) assert_greater_than_or_equal(TRUC_CHILD_MAX_VSIZE, 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"]) + assert_raises_rpc_error(-26, "too-large-cluster", 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) self.generate(node, 1) diff --git a/test/functional/mempool_updatefromblock.py b/test/functional/mempool_updatefromblock.py index 6ee31ffa650..bf0b8778b64 100755 --- a/test/functional/mempool_updatefromblock.py +++ b/test/functional/mempool_updatefromblock.py @@ -27,8 +27,7 @@ CUSTOM_DESCENDANT_COUNT = CUSTOM_ANCESTOR_COUNT class MempoolUpdateFromBlockTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 - # Ancestor and descendant limits depend on transaction_graph_test requirements - self.extra_args = [['-limitdescendantsize=1000', '-limitancestorsize=1000', f'-limitancestorcount={CUSTOM_ANCESTOR_COUNT}', f'-limitdescendantcount={CUSTOM_DESCENDANT_COUNT}', "-limitclustersize=1000"]] + self.extra_args = [['-limitclustersize=1000']] def create_empty_fork(self, fork_length): '''