From 73b68bd8b4f9447e30091c7f8c3dc91a086bd93b Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Mon, 8 Apr 2024 15:05:07 +0200 Subject: [PATCH 1/5] fill_mempool: remove subtest-specific comment --- test/functional/mempool_limit.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index 6215610c3152b..3e6856700e9c0 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -47,8 +47,6 @@ def fill_mempool(self): # Generate UTXOs to flood the mempool # 1 to create a tx initially that will be evicted from the mempool later # 75 transactions each with a fee rate higher than the previous one - # And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee - # And 2 more for the package cpfp test self.generate(miniwallet, 1 + (num_of_batches * tx_batch_size)) # Mine 99 blocks so that the UTXOs are allowed to be spent From a3da63e8febe475f2250f6432bca237d31fa9107 Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Mon, 8 Apr 2024 15:07:38 +0200 Subject: [PATCH 2/5] Move fill_mempool to util function --- test/functional/mempool_limit.py | 52 ++------------------------ test/functional/test_framework/util.py | 39 +++++++++++++++++++ 2 files changed, 43 insertions(+), 48 deletions(-) diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py index 3e6856700e9c0..e8a568f7aba14 100755 --- a/test/functional/mempool_limit.py +++ b/test/functional/mempool_limit.py @@ -6,7 +6,6 @@ from decimal import Decimal -from test_framework.blocktools import COINBASE_MATURITY from test_framework.p2p import P2PTxInvStore from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -14,8 +13,7 @@ assert_fee_amount, assert_greater_than, assert_raises_rpc_error, - create_lots_of_big_transactions, - gen_return_txouts, + fill_mempool, ) from test_framework.wallet import ( COIN, @@ -34,48 +32,6 @@ def set_test_params(self): ]] self.supports_cli = False - def fill_mempool(self): - """Fill mempool until eviction.""" - self.log.info("Fill the mempool until eviction is triggered and the mempoolminfee rises") - txouts = gen_return_txouts() - node = self.nodes[0] - miniwallet = self.wallet - relayfee = node.getnetworkinfo()['relayfee'] - - tx_batch_size = 1 - num_of_batches = 75 - # Generate UTXOs to flood the mempool - # 1 to create a tx initially that will be evicted from the mempool later - # 75 transactions each with a fee rate higher than the previous one - self.generate(miniwallet, 1 + (num_of_batches * tx_batch_size)) - - # Mine 99 blocks so that the UTXOs are allowed to be spent - self.generate(node, COINBASE_MATURITY - 1) - - self.log.debug("Create a mempool tx that will be evicted") - tx_to_be_evicted_id = miniwallet.send_self_transfer(from_node=node, fee_rate=relayfee)["txid"] - - # Increase the tx fee rate to give the subsequent transactions a higher priority in the mempool - # The tx has an approx. vsize of 65k, i.e. multiplying the previous fee rate (in sats/kvB) - # by 130 should result in a fee that corresponds to 2x of that fee rate - base_fee = relayfee * 130 - - self.log.debug("Fill up the mempool with txs with higher fee rate") - with node.assert_debug_log(["rolling minimum fee bumped"]): - for batch_of_txid in range(num_of_batches): - fee = (batch_of_txid + 1) * base_fee - create_lots_of_big_transactions(miniwallet, node, fee, tx_batch_size, txouts) - - self.log.debug("The tx should be evicted by now") - # The number of transactions created should be greater than the ones present in the mempool - assert_greater_than(tx_batch_size * num_of_batches, len(node.getrawmempool())) - # Initial tx created should not be present in the mempool anymore as it had a lower fee rate - assert tx_to_be_evicted_id not in node.getrawmempool() - - self.log.debug("Check that mempoolminfee is larger than minrelaytxfee") - assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) - assert_greater_than(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) - def test_rbf_carveout_disallowed(self): node = self.nodes[0] @@ -137,7 +93,7 @@ def test_mid_package_eviction(self): assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) assert_equal(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) - self.fill_mempool() + fill_mempool(self, node, self.wallet) current_info = node.getmempoolinfo() mempoolmin_feerate = current_info["mempoolminfee"] @@ -227,7 +183,7 @@ def test_mid_package_replacement(self): assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) assert_equal(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) - self.fill_mempool() + fill_mempool(self, node, self.wallet) current_info = node.getmempoolinfo() mempoolmin_feerate = current_info["mempoolminfee"] @@ -301,7 +257,7 @@ def run_test(self): assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) assert_equal(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) - self.fill_mempool() + fill_mempool(self, node, self.wallet) # Deliberately try to create a tx with a fee less than the minimum mempool fee to assert that it does not get added to the mempool self.log.info('Create a mempool tx that will not pass mempoolminfee') diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index c5b69a39549ac..79aebd2d7a8c6 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -496,6 +496,45 @@ def check_node_connections(*, node, num_in, num_out): assert_equal(info["connections_in"], num_in) assert_equal(info["connections_out"], num_out) +def fill_mempool(test_framework, node, miniwallet): + """Fill mempool until eviction.""" + test_framework.log.info("Fill the mempool until eviction is triggered and the mempoolminfee rises") + txouts = gen_return_txouts() + relayfee = node.getnetworkinfo()['relayfee'] + + tx_batch_size = 1 + num_of_batches = 75 + # Generate UTXOs to flood the mempool + # 1 to create a tx initially that will be evicted from the mempool later + # 75 transactions each with a fee rate higher than the previous one + test_framework.generate(miniwallet, 1 + (num_of_batches * tx_batch_size)) + + # Mine COINBASE_MATURITY - 1 blocks so that the UTXOs are allowed to be spent + test_framework.generate(node, 100 - 1) + + test_framework.log.debug("Create a mempool tx that will be evicted") + tx_to_be_evicted_id = miniwallet.send_self_transfer(from_node=node, fee_rate=relayfee)["txid"] + + # Increase the tx fee rate to give the subsequent transactions a higher priority in the mempool + # The tx has an approx. vsize of 65k, i.e. multiplying the previous fee rate (in sats/kvB) + # by 130 should result in a fee that corresponds to 2x of that fee rate + base_fee = relayfee * 130 + + test_framework.log.debug("Fill up the mempool with txs with higher fee rate") + with node.assert_debug_log(["rolling minimum fee bumped"]): + for batch_of_txid in range(num_of_batches): + fee = (batch_of_txid + 1) * base_fee + create_lots_of_big_transactions(miniwallet, node, fee, tx_batch_size, txouts) + + test_framework.log.debug("The tx should be evicted by now") + # The number of transactions created should be greater than the ones present in the mempool + assert_greater_than(tx_batch_size * num_of_batches, len(node.getrawmempool())) + # Initial tx created should not be present in the mempool anymore as it had a lower fee rate + assert tx_to_be_evicted_id not in node.getrawmempool() + + test_framework.log.debug("Check that mempoolminfee is larger than minrelaytxfee") + assert_equal(node.getmempoolinfo()['minrelaytxfee'], Decimal('0.00001000')) + assert_greater_than(node.getmempoolinfo()['mempoolminfee'], Decimal('0.00001000')) # Transaction/Block functions ############################# From f3aa5bd5eb6d1088f98a4dc7daaab0e17a7d5529 Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Mon, 8 Apr 2024 15:10:16 +0200 Subject: [PATCH 3/5] fill_mempool: assertions and docsctring update --- test/functional/test_framework/util.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 79aebd2d7a8c6..dbaf42fdaa278 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -497,11 +497,21 @@ def check_node_connections(*, node, num_in, num_out): assert_equal(info["connections_out"], num_out) def fill_mempool(test_framework, node, miniwallet): - """Fill mempool until eviction.""" + """Fill mempool until eviction. + + Allows for simpler testing of scenarios with floating mempoolminfee > minrelay + Requires -datacarriersize=100000 and + -maxmempool=5. + It will not ensure mempools become synced as it + is based on a single node and assumes -minrelaytxfee + is 1 sat/vbyte. + """ test_framework.log.info("Fill the mempool until eviction is triggered and the mempoolminfee rises") txouts = gen_return_txouts() relayfee = node.getnetworkinfo()['relayfee'] + assert_equal(relayfee, Decimal('0.00001000')) + tx_batch_size = 1 num_of_batches = 75 # Generate UTXOs to flood the mempool From 91d7d8f22a1c528db14fa743c66cd861ea00e84b Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Tue, 26 Mar 2024 10:35:44 -0400 Subject: [PATCH 4/5] AcceptMultipleTransactions: Fix workspace client_maxfeerate If we do not set the Failure for the workspace when there is a client_maxfeerate related error, we hit an Assume() to the contrary. Properly set it. --- src/validation.cpp | 4 +- test/functional/rpc_packages.py | 67 +++++++++++++++++++++++++++++---- 2 files changed, 62 insertions(+), 9 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 5c585438d14da..bee382bd1fa3d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1366,7 +1366,9 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: // Individual modified feerate exceeded caller-defined max; abort // N.B. this doesn't take into account CPFPs. Chunk-aware validation may be more robust. if (args.m_client_maxfeerate && CFeeRate(ws.m_modified_fees, ws.m_vsize) > args.m_client_maxfeerate.value()) { - package_state.Invalid(PackageValidationResult::PCKG_TX, "max feerate exceeded"); + // Need to set failure here both individually and at package level + ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "max feerate exceeded", ""); + package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); // Exit early to avoid doing pointless work. Update the failed tx result; the rest are unfinished. results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state)); return PackageMempoolAcceptResult(package_state, std::move(results)); diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index 029e36816654e..e061030da3180 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -18,6 +18,7 @@ assert_equal, assert_fee_amount, assert_raises_rpc_error, + fill_mempool, ) from test_framework.wallet import ( DEFAULT_FEE, @@ -82,7 +83,8 @@ def run_test(self): self.test_conflicting() self.test_rbf() self.test_submitpackage() - self.test_maxfeerate_maxburn_submitpackage() + self.test_maxfeerate_submitpackage() + self.test_maxburn_submitpackage() def test_independent(self, coin): self.log.info("Test multiple independent transactions in a package") @@ -358,7 +360,7 @@ def test_submitpackage(self): assert_equal(res["tx-results"][sec_wtxid]["error"], "version") peer.wait_for_broadcast([first_wtxid]) - def test_maxfeerate_maxburn_submitpackage(self): + def test_maxfeerate_submitpackage(self): node = self.nodes[0] # clear mempool deterministic_address = node.get_deterministic_priv_key().address @@ -369,23 +371,72 @@ def test_maxfeerate_maxburn_submitpackage(self): minrate_btc_kvb = min([chained_txn["fee"] / chained_txn["tx"].get_vsize() * 1000 for chained_txn in chained_txns]) chain_hex = [t["hex"] for t in chained_txns] pkg_result = node.submitpackage(chain_hex, maxfeerate=minrate_btc_kvb - Decimal("0.00000001")) + + # First tx failed in single transaction evaluation, so package message is generic + assert_equal(pkg_result["package_msg"], "transaction failed") assert_equal(pkg_result["tx-results"][chained_txns[0]["wtxid"]]["error"], "max feerate exceeded") assert_equal(pkg_result["tx-results"][chained_txns[1]["wtxid"]]["error"], "bad-txns-inputs-missingorspent") assert_equal(node.getrawmempool(), []) + # Make chain of two transactions where parent doesn't make minfee threshold + # but child is too high fee + # Lower mempool limit to make it easier to fill_mempool + self.restart_node(0, extra_args=[ + "-datacarriersize=100000", + "-maxmempool=5", + "-persistmempool=0", + ]) + + fill_mempool(self, node, self.wallet) + + minrelay = node.getmempoolinfo()["minrelaytxfee"] + parent = self.wallet.create_self_transfer( + fee_rate=minrelay, + ) + + child = self.wallet.create_self_transfer( + fee_rate=DEFAULT_FEE, + utxo_to_spend=parent["new_utxo"], + ) + + pkg_result = node.submitpackage([parent["hex"], child["hex"]], maxfeerate=DEFAULT_FEE - Decimal("0.00000001")) + + # Child is connected even though parent is invalid and still reports fee exceeded + # this implies sub-package evaluation of both entries together. + assert_equal(pkg_result["package_msg"], "transaction failed") + assert "mempool min fee not met" in pkg_result["tx-results"][parent["wtxid"]]["error"] + assert_equal(pkg_result["tx-results"][child["wtxid"]]["error"], "max feerate exceeded") + assert parent["txid"] not in node.getrawmempool() + assert child["txid"] not in node.getrawmempool() + + # Reset maxmempool, datacarriersize, reset dynamic mempool minimum feerate, and empty mempool. + self.restart_node(0) + + assert_equal(node.getrawmempool(), []) + + def test_maxburn_submitpackage(self): + node = self.nodes[0] + + assert_equal(node.getrawmempool(), []) + self.log.info("Submitpackage maxburnamount arg testing") - tx = tx_from_hex(chain_hex[1]) + chained_txns_burn = self.wallet.create_self_transfer_chain(chain_length=2) + chained_burn_hex = [t["hex"] for t in chained_txns_burn] + + tx = tx_from_hex(chained_burn_hex[1]) tx.vout[-1].scriptPubKey = b'a' * 10001 # scriptPubKey bigger than 10k IsUnspendable - chain_hex = [chain_hex[0], tx.serialize().hex()] + chained_burn_hex = [chained_burn_hex[0], tx.serialize().hex()] # burn test is run before any package evaluation; nothing makes it in and we get broader exception - assert_raises_rpc_error(-25, "Unspendable output exceeds maximum configured by user", node.submitpackage, chain_hex, 0, chained_txns[1]["new_utxo"]["value"] - Decimal("0.00000001")) + assert_raises_rpc_error(-25, "Unspendable output exceeds maximum configured by user", node.submitpackage, chained_burn_hex, 0, chained_txns_burn[1]["new_utxo"]["value"] - Decimal("0.00000001")) assert_equal(node.getrawmempool(), []) + minrate_btc_kvb_burn = min([chained_txn_burn["fee"] / chained_txn_burn["tx"].get_vsize() * 1000 for chained_txn_burn in chained_txns_burn]) + # Relax the restrictions for both and send it; parent gets through as own subpackage - pkg_result = node.submitpackage(chain_hex, maxfeerate=minrate_btc_kvb, maxburnamount=chained_txns[1]["new_utxo"]["value"]) - assert "error" not in pkg_result["tx-results"][chained_txns[0]["wtxid"]] + pkg_result = node.submitpackage(chained_burn_hex, maxfeerate=minrate_btc_kvb_burn, maxburnamount=chained_txns_burn[1]["new_utxo"]["value"]) + assert "error" not in pkg_result["tx-results"][chained_txns_burn[0]["wtxid"]] assert_equal(pkg_result["tx-results"][tx.getwtxid()]["error"], "scriptpubkey") - assert_equal(node.getrawmempool(), [chained_txns[0]["txid"]]) + assert_equal(node.getrawmempool(), [chained_txns_burn[0]["txid"]]) if __name__ == "__main__": RPCPackagesTest().main() From 4ba1d0b55339c3ea90e2bcd64662a06f0f90dd46 Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Wed, 27 Mar 2024 09:30:05 -0400 Subject: [PATCH 5/5] fuzz: Add coverage for client_maxfeerate --- src/test/fuzz/package_eval.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index c801c2e325fb0..c201118bceea5 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -276,8 +276,14 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) // (the package is a test accept and ATMP is a submission). auto single_submit = txs.size() == 1 && fuzzed_data_provider.ConsumeBool(); + // Exercise client_maxfeerate logic + std::optional client_maxfeerate{}; + if (fuzzed_data_provider.ConsumeBool()) { + client_maxfeerate = CFeeRate(fuzzed_data_provider.ConsumeIntegralInRange(-1, 50 * COIN), 100); + } + const auto result_package = WITH_LOCK(::cs_main, - return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/single_submit, /*client_maxfeerate=*/{})); + return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/single_submit, client_maxfeerate)); // Always set bypass_limits to false because it is not supported in ProcessNewPackage and // can be a source of divergence.