Merge bitcoin#30718: test: switch MiniWallet padding unit from weight to vsize

achow101 · achow101 · commit fc642c33ef28 · 2024-09-30T17:07:12.000-04:00
940edd6 test: refactor: introduce and use `TRUC_CHILD_MAX_VSIZE` constant (Sebastian Falbesoner) c16ae71 test: switch MiniWallet padding unit from weight to vsize (Sebastian Falbesoner) Pull request description: This PR is a late follow-up for bitcoin#30162, where I retrospectively consider the padding unit of choice as a mistake. The weight unit is merely a consensus rule detail and is largely irrelevant from a user's perspective w.r.t. fee-rate calculations and mempool policy rules (e.g. for package relay and TRUC limits), so there doesn't seem to be any value of using a granularity that we can't even guarantee to reach exactly anyway. Switch to the more natural unit of vsize instead, which simplifies both the padding implementation (no "round up to the next multiple of 4" anymore) and the current tests that take use of this padding. The rather annoying multiplications by `WITNESS_SCALE_FACTOR` can then be removed and weird-looking magic numbers like `4004` can be replaced by numbers that are more connected to actual policy limit constants from the codebase, e.g. `1001` for exceeding `TRUC_CHILD_MAX_VSIZE` by one. The second commits introduces a constant for that. ACKs for top commit: glozow: reACK 940edd6 via range-diff instagibbs: reACK 940edd6 maflcko: re-ACK 940edd6 🍷 achow101: ACK 940edd6 Tree-SHA512: 35325f22bbe548664273051b705059b8f2f4316215be116c71b8c21dc87d190b3e8fcc4a48f04deaba2f3632a9c809d272b0bae654cf74d7492759554c0f0d14
diff --git a/test/functional/feature_blocksxor.py b/test/functional/feature_blocksxor.py
@@ -31,7 +31,7 @@ def run_test(self):
         node = self.nodes[0]
         wallet = MiniWallet(node)
         for _ in range(5):
-            wallet.send_self_transfer(from_node=node, target_weight=80000)
+            wallet.send_self_transfer(from_node=node, target_vsize=20000)
             self.generate(wallet, 1)
 
         block_files = list(node.blocks_path.glob('blk[0-9][0-9][0-9][0-9][0-9].dat'))
diff --git a/test/functional/feature_framework_miniwallet.py b/test/functional/feature_framework_miniwallet.py
@@ -9,7 +9,7 @@
 from test_framework.blocktools import COINBASE_MATURITY
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
-    assert_greater_than_or_equal,
+    assert_equal,
 )
 from test_framework.wallet import (
     MiniWallet,
@@ -22,17 +22,15 @@ 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."""
+        """Verify that MiniWallet's transaction padding (`target_vsize` parameter)
+           works accurately 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())
+            for target_vsize in [250, 500, 1250, 2500, 5000, 12500, 25000, 50000, 1000000,
+                                 248, 501, 1085, 3343, 5805, 12289, 25509, 55855,  999998]:
+                tx = wallet.create_self_transfer(utxo_to_spend=utxo, target_vsize=target_vsize)["tx"]
+                assert_equal(tx.get_vsize(), target_vsize)
 
     def test_wallet_tagging(self):
         """Verify that tagged wallet instances are able to send funds."""
diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py
@@ -55,28 +55,28 @@ def test_rbf_carveout_disallowed(self):
         self.generate(node, 1)
 
         # tx_A needs to be RBF'd, set minfee at set size
-        A_weight = 1000
+        A_vsize = 250
         mempoolmin_feerate = node.getmempoolinfo()["mempoolminfee"]
         tx_A = self.wallet.send_self_transfer(
             from_node=node,
             fee_rate=mempoolmin_feerate,
-            target_weight=A_weight,
+            target_vsize=A_vsize,
             utxo_to_spend=rbf_utxo,
             confirmed_only=True
         )
 
         # RBF's tx_A, is not yet submitted
         tx_B = self.wallet.create_self_transfer(
             fee=tx_A["fee"] * 4,
-            target_weight=A_weight,
+            target_vsize=A_vsize,
             utxo_to_spend=rbf_utxo,
             confirmed_only=True
         )
 
         # Spends tx_B's output, too big for cpfp carveout (because that would also increase the descendant limit by 1)
-        non_cpfp_carveout_weight = 40001 # EXTRA_DESCENDANT_TX_SIZE_LIMIT + 1
+        non_cpfp_carveout_vsize = 10001  # EXTRA_DESCENDANT_TX_SIZE_LIMIT + 1
         tx_C = self.wallet.create_self_transfer(
-            target_weight=non_cpfp_carveout_weight,
+            target_vsize=non_cpfp_carveout_vsize,
             fee_rate=mempoolmin_feerate,
             utxo_to_spend=tx_B["new_utxo"],
             confirmed_only=True
@@ -103,14 +103,14 @@ def test_mid_package_eviction(self):
         # UTXOs to be spent by the ultimate child transaction
         parent_utxos = []
 
-        evicted_weight = 8000
+        evicted_vsize = 2000
         # Mempool transaction which is evicted due to being at the "bottom" of the mempool when the
         # mempool overflows and evicts by descendant score. It's important that the eviction doesn't
         # 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_rate=mempoolmin_feerate,
-            target_weight=evicted_weight,
+            target_vsize=evicted_vsize,
             confirmed_only=True
         )
         # Already in mempool when package is submitted.
@@ -132,14 +132,16 @@ def test_mid_package_eviction(self):
 
         # Series of parents that don't need CPFP and are submitted individually. Each one is large and
         # high feerate, which means they should trigger eviction but not be evicted.
-        parent_weight = 100000
+        parent_vsize = 25000
         num_big_parents = 3
-        assert_greater_than(parent_weight * num_big_parents, current_info["maxmempool"] - current_info["bytes"])
+        # Need to be large enough to trigger eviction
+        # (note that the mempool usage of a tx is about three times its vsize)
+        assert_greater_than(parent_vsize * num_big_parents * 3, current_info["maxmempool"] - current_info["bytes"])
         parent_feerate = 100 * mempoolmin_feerate
 
         big_parent_txids = []
         for i in range(num_big_parents):
-            parent = self.wallet.create_self_transfer(fee_rate=parent_feerate, target_weight=parent_weight, confirmed_only=True)
+            parent = self.wallet.create_self_transfer(fee_rate=parent_feerate, target_vsize=parent_vsize, confirmed_only=True)
             parent_utxos.append(parent["new_utxo"])
             package_hex.append(parent["hex"])
             big_parent_txids.append(parent["txid"])
@@ -311,17 +313,18 @@ def run_test(self):
             entry = node.getmempoolentry(txid)
             worst_feerate_btcvb = min(worst_feerate_btcvb, entry["fees"]["descendant"] / entry["descendantsize"])
         # Needs to be large enough to trigger eviction
-        target_weight_each = 200000
-        assert_greater_than(target_weight_each * 2, node.getmempoolinfo()["maxmempool"] - node.getmempoolinfo()["bytes"])
+        # (note that the mempool usage of a tx is about three times its vsize)
+        target_vsize_each = 50000
+        assert_greater_than(target_vsize_each * 2 * 3, node.getmempoolinfo()["maxmempool"] - node.getmempoolinfo()["bytes"])
         # Should be a true CPFP: parent's feerate is just below mempool min feerate
         parent_feerate = mempoolmin_feerate - Decimal("0.000001")  # 0.1 sats/vbyte below min feerate
         # Parent + child is above mempool minimum feerate
         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_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)
+        tx_parent_just_below = miniwallet.create_self_transfer(fee_rate=parent_feerate, target_vsize=target_vsize_each)
+        tx_child_just_above = miniwallet.create_self_transfer(utxo_to_spend=tx_parent_just_below["new_utxo"], fee_rate=child_feerate, target_vsize=target_vsize_each)
         # This package ranks below the lowest descendant package in the mempool
         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()
diff --git a/test/functional/mempool_package_limits.py b/test/functional/mempool_package_limits.py
@@ -4,9 +4,6 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test logic for limiting mempool and package ancestors/descendants."""
 from test_framework.blocktools import COINBASE_MATURITY
-from test_framework.messages import (
-    WITNESS_SCALE_FACTOR,
-)
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
@@ -290,19 +287,18 @@ def test_anc_size_limits(self):
         parent_utxos = []
         target_vsize = 30_000
         high_fee = 10 * target_vsize  # 10 sats/vB
-        target_weight = target_vsize * WITNESS_SCALE_FACTOR
         self.log.info("Check that in-mempool and in-package ancestor size limits are calculated properly in packages")
         # Mempool transactions A and B
         for _ in range(2):
-            bulked_tx = self.wallet.create_self_transfer(target_weight=target_weight)
+            bulked_tx = self.wallet.create_self_transfer(target_vsize=target_vsize)
             self.wallet.sendrawtransaction(from_node=node, tx_hex=bulked_tx["hex"])
             parent_utxos.append(bulked_tx["new_utxo"])
 
         # Package transaction C
-        pc_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=parent_utxos, fee_per_output=high_fee, target_weight=target_weight)
+        pc_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=parent_utxos, fee_per_output=high_fee, target_vsize=target_vsize)
 
         # Package transaction D
-        pd_tx = self.wallet.create_self_transfer(utxo_to_spend=pc_tx["new_utxos"][0], target_weight=target_weight)
+        pd_tx = self.wallet.create_self_transfer(utxo_to_spend=pc_tx["new_utxos"][0], target_vsize=target_vsize)
 
         assert_equal(2, node.getmempoolinfo()["size"])
         return [pc_tx["hex"], pd_tx["hex"]]
@@ -321,20 +317,19 @@ def test_desc_size_limits(self):
         node = self.nodes[0]
         target_vsize = 21_000
         high_fee = 10 * target_vsize  # 10 sats/vB
-        target_weight = target_vsize * WITNESS_SCALE_FACTOR
         self.log.info("Check that in-mempool and in-package descendant sizes are calculated properly in packages")
         # Top parent in mempool, Ma
-        ma_tx = self.wallet.create_self_transfer_multi(num_outputs=2, fee_per_output=high_fee // 2, target_weight=target_weight)
+        ma_tx = self.wallet.create_self_transfer_multi(num_outputs=2, fee_per_output=high_fee // 2, target_vsize=target_vsize)
         self.wallet.sendrawtransaction(from_node=node, tx_hex=ma_tx["hex"])
 
         package_hex = []
         for j in range(2): # Two legs (left and right)
             # Mempool transaction (Mb and Mc)
-            mempool_tx = self.wallet.create_self_transfer(utxo_to_spend=ma_tx["new_utxos"][j], target_weight=target_weight)
+            mempool_tx = self.wallet.create_self_transfer(utxo_to_spend=ma_tx["new_utxos"][j], target_vsize=target_vsize)
             self.wallet.sendrawtransaction(from_node=node, tx_hex=mempool_tx["hex"])
 
             # Package transaction (Pd and Pe)
-            package_tx = self.wallet.create_self_transfer(utxo_to_spend=mempool_tx["new_utxo"], target_weight=target_weight)
+            package_tx = self.wallet.create_self_transfer(utxo_to_spend=mempool_tx["new_utxo"], target_vsize=target_vsize)
             package_hex.append(package_tx["hex"])
 
         assert_equal(3, node.getmempoolinfo()["size"])
diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py
diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py
