Merge bitcoin/bitcoin#30162: test: MiniWallet: respect passed feerate for padded txs (using target_weight)

39d135e test: MiniWallet: respect fee_rate for target_weight, use in mempool_limit.py (Sebastian Falbesoner)
b2f0a9f test: add framework functional test for MiniWallet's tx padding (Sebastian Falbesoner)
c17550b test: MiniWallet: fix tx padding (`target_weight`) for large sizes, improve accuracy (Sebastian Falbesoner)

Pull request description:

  MiniWallet allows to create padded transactions that are equal or slightly above a certain `target_weight` (first introduced in PR #25379, commit 1d6b438), which can be useful especially for mempool-related tests, e.g. for policy limit checks or scenarios to trigger mempool eviction. Currently the `target_weight` parameter doesn't play together with `fee_rate` though, as the fee calculation is incorrectly based on the tx vsize before the padding output is added, so the fee-rate is consequently far off. This means users are forced to pass an absolute fee, which can be quite inconvenient and leads to lots of duplicated "calculate absolute fee from fee-rate and vsize" code with the pattern `fee = (feerate / 1000) * (weight // 4)` on the call-sites.

  This PR first improves the tx padding itself to be more accurate, adds a functional test for it, and fixes the `fee_rate` treatment for the `{create,send}_self_transfer` methods. (Next step would be to enable this also for the `_self_transfer_multi` methods, but those currently don't even offer a `fee_rate` parameter). Finally, the ability to pass both `target_weight` and `fee_rate` is used in the `mempool_limit.py` functional test. There might be more use-cases in other tests, that could be done in a follow-up.

ACKs for top commit:
  rkrux:
    tACK [39d135e](bitcoin/bitcoin@39d135e)
  ismaelsadeeq:
    Code Review ACK 39d135e  🚀
  glozow:
    light review ACK 39d135e

Tree-SHA512: 6bf8e853a921576d463291d619cdfd6a7e74cf92f61933a563800ac0b3c023a06569b581243166906f56b3c5e8858fec2d8a6910d55899e904221f847eb0953d

Author: glozow

test/functional/feature_framework_miniwallet.py

@@ -0,0 +1,49 @@
+#!/usr/bin/env python3
+# Copyright (c) 2024 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test MiniWallet."""
+from test_framework.blocktools import COINBASE_MATURITY
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_greater_than_or_equal,
+)
+from test_framework.wallet import (
+    MiniWallet,
+    MiniWalletMode,
+)
+
+
+class FeatureFrameworkMiniWalletTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 1
+
+    def test_tx_padding(self):
+        """Verify that MiniWallet's transaction padding (`target_weight` parameter)
+           works accurately enough (i.e. at most 3 WUs higher) with all modes."""
+        for mode_name, wallet in self.wallets:
+            self.log.info(f"Test tx padding with MiniWallet mode {mode_name}...")
+            utxo = wallet.get_utxo(mark_as_spent=False)
+            for target_weight in [1000, 2000, 5000, 10000, 20000, 50000, 100000, 200000, 4000000,
+                                  989,  2001, 4337, 13371, 23219, 49153, 102035, 223419, 3999989]:
+                tx = wallet.create_self_transfer(utxo_to_spend=utxo, target_weight=target_weight)["tx"]
+                self.log.debug(f"-> target weight: {target_weight}, actual weight: {tx.get_weight()}")
+                assert_greater_than_or_equal(tx.get_weight(), target_weight)
+                assert_greater_than_or_equal(target_weight + 3, tx.get_weight())
+
+    def run_test(self):
+        node = self.nodes[0]
+        self.wallets = [
+            ("ADDRESS_OP_TRUE", MiniWallet(node, mode=MiniWalletMode.ADDRESS_OP_TRUE)),
+            ("RAW_OP_TRUE",     MiniWallet(node, mode=MiniWalletMode.RAW_OP_TRUE)),
+            ("RAW_P2PK",        MiniWallet(node, mode=MiniWalletMode.RAW_P2PK)),
+        ]
+        for _, wallet in self.wallets:
+            self.generate(wallet, 10)
+        self.generate(wallet, COINBASE_MATURITY)
+
+        self.test_tx_padding()
+
+
+if __name__ == '__main__':
+    FeatureFrameworkMiniWalletTest().main()

test/functional/mempool_limit.py

@@ -59,7 +59,7 @@ def test_rbf_carveout_disallowed(self):
         mempoolmin_feerate = node.getmempoolinfo()["mempoolminfee"]
         tx_A = self.wallet.send_self_transfer(
             from_node=node,
-            fee=(mempoolmin_feerate / 1000) * (A_weight // 4) + Decimal('0.000001'),
+            fee_rate=mempoolmin_feerate,
             target_weight=A_weight,
             utxo_to_spend=rbf_utxo,
             confirmed_only=True
@@ -77,7 +77,7 @@ def test_rbf_carveout_disallowed(self):
         non_cpfp_carveout_weight = 40001 # EXTRA_DESCENDANT_TX_SIZE_LIMIT + 1
         tx_C = self.wallet.create_self_transfer(
             target_weight=non_cpfp_carveout_weight,
-            fee = (mempoolmin_feerate / 1000) * (non_cpfp_carveout_weight // 4) + Decimal('0.000001'),
+            fee_rate=mempoolmin_feerate,
             utxo_to_spend=tx_B["new_utxo"],
             confirmed_only=True
         )
@@ -109,7 +109,7 @@ def test_mid_package_eviction(self):
         # happen in the middle of package evaluation, as it can invalidate the coins cache.
         mempool_evicted_tx = self.wallet.send_self_transfer(
             from_node=node,
-            fee=(mempoolmin_feerate / 1000) * (evicted_weight // 4) + Decimal('0.000001'),
+            fee_rate=mempoolmin_feerate,
             target_weight=evicted_weight,
             confirmed_only=True
         )
@@ -135,11 +135,11 @@ def test_mid_package_eviction(self):
         parent_weight = 100000
         num_big_parents = 3
         assert_greater_than(parent_weight * num_big_parents, current_info["maxmempool"] - current_info["bytes"])
-        parent_fee = (100 * mempoolmin_feerate / 1000) * (parent_weight // 4)
+        parent_feerate = 100 * mempoolmin_feerate
 
         big_parent_txids = []
         for i in range(num_big_parents):
-            parent = self.wallet.create_self_transfer(fee=parent_fee, target_weight=parent_weight, confirmed_only=True)
+            parent = self.wallet.create_self_transfer(fee_rate=parent_feerate, target_weight=parent_weight, confirmed_only=True)
             parent_utxos.append(parent["new_utxo"])
             package_hex.append(parent["hex"])
             big_parent_txids.append(parent["txid"])
@@ -314,18 +314,20 @@ def run_test(self):
         target_weight_each = 200000
         assert_greater_than(target_weight_each * 2, node.getmempoolinfo()["maxmempool"] - node.getmempoolinfo()["bytes"])
         # Should be a true CPFP: parent's feerate is just below mempool min feerate
-        parent_fee = (mempoolmin_feerate / 1000) * (target_weight_each // 4) - Decimal("0.00001")
+        parent_feerate = mempoolmin_feerate - Decimal("0.000001")  # 0.1 sats/vbyte below min feerate
         # Parent + child is above mempool minimum feerate
-        child_fee = (worst_feerate_btcvb) * (target_weight_each // 4) - Decimal("0.00001")
+        child_feerate = (worst_feerate_btcvb * 1000) - Decimal("0.000001")  # 0.1 sats/vbyte below worst feerate
         # However, when eviction is triggered, these transactions should be at the bottom.
         # This assertion assumes parent and child are the same size.
         miniwallet.rescan_utxos()
-        tx_parent_just_below = miniwallet.create_self_transfer(fee=parent_fee, target_weight=target_weight_each)
-        tx_child_just_above = miniwallet.create_self_transfer(utxo_to_spend=tx_parent_just_below["new_utxo"], fee=child_fee, target_weight=target_weight_each)
+        tx_parent_just_below = miniwallet.create_self_transfer(fee_rate=parent_feerate, target_weight=target_weight_each)
+        tx_child_just_above = miniwallet.create_self_transfer(utxo_to_spend=tx_parent_just_below["new_utxo"], fee_rate=child_feerate, target_weight=target_weight_each)
         # This package ranks below the lowest descendant package in the mempool
-        assert_greater_than(worst_feerate_btcvb, (parent_fee + child_fee) / (tx_parent_just_below["tx"].get_vsize() + tx_child_just_above["tx"].get_vsize()))
-        assert_greater_than(mempoolmin_feerate, (parent_fee) / (tx_parent_just_below["tx"].get_vsize()))
-        assert_greater_than((parent_fee + child_fee) / (tx_parent_just_below["tx"].get_vsize() + tx_child_just_above["tx"].get_vsize()), mempoolmin_feerate / 1000)
+        package_fee = tx_parent_just_below["fee"] + tx_child_just_above["fee"]
+        package_vsize = tx_parent_just_below["tx"].get_vsize() + tx_child_just_above["tx"].get_vsize()
+        assert_greater_than(worst_feerate_btcvb, package_fee / package_vsize)
+        assert_greater_than(mempoolmin_feerate, tx_parent_just_below["fee"] / (tx_parent_just_below["tx"].get_vsize()))
+        assert_greater_than(package_fee / package_vsize, mempoolmin_feerate / 1000)
         res = node.submitpackage([tx_parent_just_below["hex"], tx_child_just_above["hex"]])
         for wtxid in [tx_parent_just_below["wtxid"], tx_child_just_above["wtxid"]]:
             assert_equal(res["tx-results"][wtxid]["error"], "mempool full")

test/functional/test_framework/wallet.py

@@ -7,6 +7,7 @@
 from copy import deepcopy
 from decimal import Decimal
 from enum import Enum
+import math
 from typing import (
     Any,
     Optional,
@@ -33,10 +34,13 @@
     CTxInWitness,
     CTxOut,
     hash256,
+    ser_compact_size,
+    WITNESS_SCALE_FACTOR,
 )
 from test_framework.script import (
     CScript,
     LEAF_VERSION_TAPSCRIPT,
+    OP_1,
     OP_NOP,
     OP_RETURN,
     OP_TRUE,
@@ -52,6 +56,7 @@
 from test_framework.util import (
     assert_equal,
     assert_greater_than_or_equal,
+    get_fee,
 )
 from test_framework.wallet_util import generate_keypair
 
@@ -119,13 +124,16 @@ def _bulk_tx(self, tx, target_weight):
         """Pad a transaction with extra outputs until it reaches a target weight (or higher).
         returns the tx
         """
-        tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN, b'a'])))
+        tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN])))
+        # determine number of needed padding bytes by converting weight difference to vbytes
         dummy_vbytes = (target_weight - tx.get_weight() + 3) // 4
-        tx.vout[-1].scriptPubKey = CScript([OP_RETURN, b'a' * dummy_vbytes])
-        # Lower bound should always be off by at most 3
+        # compensate for the increase of the compact-size encoded script length
+        # (note that the length encoding of the unpadded output script needs one byte)
+        dummy_vbytes -= len(ser_compact_size(dummy_vbytes)) - 1
+        tx.vout[-1].scriptPubKey = CScript([OP_RETURN] + [OP_1] * dummy_vbytes)
+        # Actual weight should be at most 3 higher than target weight
         assert_greater_than_or_equal(tx.get_weight(), target_weight)
-        # Higher bound should always be off by at most 3 + 12 weight (for encoding the length)
-        assert_greater_than_or_equal(target_weight + 15, tx.get_weight())
+        assert_greater_than_or_equal(target_weight + 3, tx.get_weight())
 
     def get_balance(self):
         return sum(u['value'] for u in self._utxos)
@@ -367,6 +375,10 @@ def create_self_transfer(
             vsize = Decimal(168)  # P2PK (73 bytes scriptSig + 35 bytes scriptPubKey + 60 bytes other)
         else:
             assert False
+        if target_weight and not fee:  # respect fee_rate if target weight is passed
+            # the actual weight might be off by 3 WUs, so calculate based on that (see self._bulk_tx)
+            max_actual_weight = target_weight + 3
+            fee = get_fee(math.ceil(max_actual_weight / WITNESS_SCALE_FACTOR), fee_rate)
         send_value = utxo_to_spend["value"] - (fee or (fee_rate * vsize / 1000))
 
         # create tx

test/functional/test_runner.py

@@ -361,6 +361,7 @@
     'feature_addrman.py',
     'feature_asmap.py',
     'feature_fastprune.py',
+    'feature_framework_miniwallet.py',
     'mempool_unbroadcast.py',
     'mempool_compatibility.py',
     'mempool_accept_wtxid.py',