Merge bitcoin/bitcoin#29735: AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure

4ba1d0b553 fuzz: Add coverage for client_maxfeerate (Greg Sanders)
91d7d8f22a AcceptMultipleTransactions: Fix workspace client_maxfeerate (Greg Sanders)
f3aa5bd5eb fill_mempool: assertions and docsctring update (Greg Sanders)
a3da63e8fe Move fill_mempool to util function (Greg Sanders)
73b68bd8b4 fill_mempool: remove subtest-specific comment (Greg Sanders)

Pull request description:

  Bug causes an `Assume()` failure due to the expectation that the individual result should be invalid when done over `submitpackage` via rpc.

  Bug introduced by https://github.com/bitcoin/bitcoin/pull/28950 , and I discovered it rebasing https://github.com/bitcoin/bitcoin/pull/28984 since it's easier to hit in that test scenario.

  Tests in place were only checking `AcceptSingleTransaction`-level checks due to package evaluation only triggering when minfee is too high for the parent transaction.

  Added test along with fix, moving the fill_mempool utility into a common area for re-use.

ACKs for top commit:
  glozow:
    reACK 4ba1d0b553
  theStack:
    ACK 4ba1d0b553
  ismaelsadeeq:
    re-ACK 4ba1d0b553  via [diff](4fe7d150eb..4ba1d0b553)

Tree-SHA512: 3729bdf7f25d04e232f173ccee04ddbb2afdaafa3d04292a01cecf58fb11b3b2bc133e8490277f1a67622b62d17929c242dc980f9bb647896beea4332ee35306

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<CFeeRate> client_maxfeerate{};
+        if (fuzzed_data_provider.ConsumeBool()) {
+            client_maxfeerate = CFeeRate(fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(-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.

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));

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 @@ from test_framework.util import (
     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,50 +32,6 @@ class MempoolLimitTest(BitcoinTestFramework):
         ]]
         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
-        # 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
-        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]
 
@@ -139,7 +93,7 @@ class MempoolLimitTest(BitcoinTestFramework):
         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"]
 
@@ -229,7 +183,7 @@ class MempoolLimitTest(BitcoinTestFramework):
         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"]
 
@@ -303,7 +257,7 @@ class MempoolLimitTest(BitcoinTestFramework):
         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')

test/functional/rpc_packages.py

@@ -18,6 +18,7 @@ from test_framework.util import (
     assert_equal,
     assert_fee_amount,
     assert_raises_rpc_error,
+    fill_mempool,
 )
 from test_framework.wallet import (
     DEFAULT_FEE,
@@ -82,7 +83,8 @@ class RPCPackagesTest(BitcoinTestFramework):
         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 @@ class RPCPackagesTest(BitcoinTestFramework):
         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 @@ class RPCPackagesTest(BitcoinTestFramework):
         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(), [])
 
-        self.log.info("Submitpackage maxburnamount arg testing")
-        tx = tx_from_hex(chain_hex[1])
-        tx.vout[-1].scriptPubKey = b'a' * 10001 # scriptPubKey bigger than 10k IsUnspendable
-        chain_hex = [chain_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"))
+        # 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")
+        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
+        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, 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()

test/functional/test_framework/util.py

@@ -496,6 +496,55 @@ 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.
+
+    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
+    # 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
 #############################