Merge bitcoin/bitcoin#23075: test: Fee estimation functional test cleanups

60ae116 qa: replace assert with test framework assertion helpers in fee estimation test (Antoine Poinsot)
e502139 qa: fee estimation with RBF test cleanups (Antoine Poinsot)
15f5fd6 qa: don't mine non standard txs in fee estimation test (Antoine Poinsot)
eae52dd qa: pass scriptsig directly to txins constructor in fee estimation test (Antoine Poinsot)
1fc0315 qa: split coins in a single tx in fee estimation test (Antoine Poinsot)
cc204b8 qa: use a single p2sh script in fee estimation test (Antoine Poinsot)
19dd91a qa: remove a redundant condition in fee estimation test (Antoine Poinsot)

Pull request description:

  Some cleanups that i noticed would be desirable while working on  #23074 and #22539, which are intentionally not based on it. Mainly simplifications and a slight speedup.

  - Use a single tx to create the `2**9` coins instead of creating `2**8` 2-outputs transactions
  - Use a single P2SH script
  - Avoid the use of non-standard transactions
  - Misc style nits (happy to take more)

ACKs for top commit:
  pg156:
    I ACK all commits up to bitcoin/bitcoin@60ae116 (except bitcoin/bitcoin@1fc0315, where I have a question more for my own learning than actually questioning the PR). I built and ran the test successfully. I agree after the changes, the behavior is kept the same and the code is shorter and easier to reason.
  glozow:
    utACK 60ae116

Tree-SHA512: 57ae2294eb68961ced30f32448c4a530ba1cdee17881594eecb97e1b9ba8927d58c25022b847eb07fb67d676bf436108c416c2f2174864d258fcca5b528b8bbd

Author: MarcoFalke

test/functional/feature_fee_estimation.py

@@ -17,7 +17,6 @@
 from test_framework.script import (
     CScript,
     OP_1,
-    OP_2,
     OP_DROP,
     OP_TRUE,
 )
@@ -36,16 +35,14 @@
 # Construct 2 trivial P2SH's and the ScriptSigs that spend them
 # So we can create many transactions without needing to spend
 # time signing.
-REDEEM_SCRIPT_1 = CScript([OP_1, OP_DROP])
-REDEEM_SCRIPT_2 = CScript([OP_2, OP_DROP])
-P2SH_1 = script_to_p2sh_script(REDEEM_SCRIPT_1)
-P2SH_2 = script_to_p2sh_script(REDEEM_SCRIPT_2)
+SCRIPT = CScript([OP_1, OP_DROP])
+P2SH = script_to_p2sh_script(SCRIPT)
+REDEEM_SCRIPT = CScript([OP_TRUE, SCRIPT])
 
-# Associated ScriptSig's to spend satisfy P2SH_1 and P2SH_2
-SCRIPT_SIG = [CScript([OP_TRUE, REDEEM_SCRIPT_1]), CScript([OP_TRUE, REDEEM_SCRIPT_2])]
 
-
-def small_txpuzzle_randfee(from_node, conflist, unconflist, amount, min_fee, fee_increment):
+def small_txpuzzle_randfee(
+    from_node, conflist, unconflist, amount, min_fee, fee_increment
+):
     """Create and send a transaction with a random fee.
 
     The transaction pays to a trivial P2SH script, and assumes that its inputs
@@ -66,55 +63,22 @@ def small_txpuzzle_randfee(from_node, conflist, unconflist, amount, min_fee, fee
     while total_in <= (amount + fee) and len(conflist) > 0:
         t = conflist.pop(0)
         total_in += t["amount"]
-        tx.vin.append(CTxIn(COutPoint(int(t["txid"], 16), t["vout"]), b""))
+        tx.vin.append(CTxIn(COutPoint(int(t["txid"], 16), t["vout"]), REDEEM_SCRIPT))
+    while total_in <= (amount + fee) and len(unconflist) > 0:
+        t = unconflist.pop(0)
+        total_in += t["amount"]
+        tx.vin.append(CTxIn(COutPoint(int(t["txid"], 16), t["vout"]), REDEEM_SCRIPT))
     if total_in <= amount + fee:
-        while total_in <= (amount + fee) and len(unconflist) > 0:
-            t = unconflist.pop(0)
-            total_in += t["amount"]
-            tx.vin.append(CTxIn(COutPoint(int(t["txid"], 16), t["vout"]), b""))
-        if total_in <= amount + fee:
-            raise RuntimeError(f"Insufficient funds: need {amount + fee}, have {total_in}")
-    tx.vout.append(CTxOut(int((total_in - amount - fee) * COIN), P2SH_1))
-    tx.vout.append(CTxOut(int(amount * COIN), P2SH_2))
-    # These transactions don't need to be signed, but we still have to insert
-    # the ScriptSig that will satisfy the ScriptPubKey.
-    for inp in tx.vin:
-        inp.scriptSig = SCRIPT_SIG[inp.prevout.n]
+        raise RuntimeError(f"Insufficient funds: need {amount + fee}, have {total_in}")
+    tx.vout.append(CTxOut(int((total_in - amount - fee) * COIN), P2SH))
+    tx.vout.append(CTxOut(int(amount * COIN), P2SH))
     txid = from_node.sendrawtransaction(hexstring=tx.serialize().hex(), maxfeerate=0)
     unconflist.append({"txid": txid, "vout": 0, "amount": total_in - amount - fee})
     unconflist.append({"txid": txid, "vout": 1, "amount": amount})
 
     return (tx.serialize().hex(), fee)
 
 
-def split_inputs(from_node, txins, txouts, initial_split=False):
-    """Generate a lot of inputs so we can generate a ton of transactions.
-
-    This function takes an input from txins, and creates and sends a transaction
-    which splits the value into 2 outputs which are appended to txouts.
-    Previously this was designed to be small inputs so they wouldn't have
-    a high coin age when the notion of priority still existed."""
-
-    prevtxout = txins.pop()
-    tx = CTransaction()
-    tx.vin.append(CTxIn(COutPoint(int(prevtxout["txid"], 16), prevtxout["vout"]), b""))
-
-    half_change = satoshi_round(prevtxout["amount"] / 2)
-    rem_change = prevtxout["amount"] - half_change - Decimal("0.00001000")
-    tx.vout.append(CTxOut(int(half_change * COIN), P2SH_1))
-    tx.vout.append(CTxOut(int(rem_change * COIN), P2SH_2))
-
-    # If this is the initial split we actually need to sign the transaction
-    # Otherwise we just need to insert the proper ScriptSig
-    if (initial_split):
-        completetx = from_node.signrawtransactionwithwallet(tx.serialize().hex())["hex"]
-    else:
-        tx.vin[0].scriptSig = SCRIPT_SIG[prevtxout["vout"]]
-        completetx = tx.serialize().hex()
-    txid = from_node.sendrawtransaction(hexstring=completetx, maxfeerate=0)
-    txouts.append({"txid": txid, "vout": 0, "amount": half_change})
-    txouts.append({"txid": txid, "vout": 1, "amount": rem_change})
-
 def check_raw_estimates(node, fees_seen):
     """Call estimaterawfee and verify that the estimates meet certain invariants."""
 
@@ -125,61 +89,67 @@ def check_raw_estimates(node, fees_seen):
             assert_greater_than(feerate, 0)
 
             if feerate + delta < min(fees_seen) or feerate - delta > max(fees_seen):
-                raise AssertionError(f"Estimated fee ({feerate}) out of range ({min(fees_seen)},{max(fees_seen)})")
+                raise AssertionError(
+                    f"Estimated fee ({feerate}) out of range ({min(fees_seen)},{max(fees_seen)})"
+                )
+
 
 def check_smart_estimates(node, fees_seen):
     """Call estimatesmartfee and verify that the estimates meet certain invariants."""
 
     delta = 1.0e-6  # account for rounding error
     last_feerate = float(max(fees_seen))
     all_smart_estimates = [node.estimatesmartfee(i) for i in range(1, 26)]
-    mempoolMinFee = node.getmempoolinfo()['mempoolminfee']
-    minRelaytxFee = node.getmempoolinfo()['minrelaytxfee']
+    mempoolMinFee = node.getmempoolinfo()["mempoolminfee"]
+    minRelaytxFee = node.getmempoolinfo()["minrelaytxfee"]
     for i, e in enumerate(all_smart_estimates):  # estimate is for i+1
         feerate = float(e["feerate"])
         assert_greater_than(feerate, 0)
         assert_greater_than_or_equal(feerate, float(mempoolMinFee))
         assert_greater_than_or_equal(feerate, float(minRelaytxFee))
 
         if feerate + delta < min(fees_seen) or feerate - delta > max(fees_seen):
-            raise AssertionError(f"Estimated fee ({feerate}) out of range ({min(fees_seen)},{max(fees_seen)})")
+            raise AssertionError(
+                f"Estimated fee ({feerate}) out of range ({min(fees_seen)},{max(fees_seen)})"
+            )
         if feerate - delta > last_feerate:
-            raise AssertionError(f"Estimated fee ({feerate}) larger than last fee ({last_feerate}) for lower number of confirms")
+            raise AssertionError(
+                f"Estimated fee ({feerate}) larger than last fee ({last_feerate}) for lower number of confirms"
+            )
         last_feerate = feerate
 
         if i == 0:
             assert_equal(e["blocks"], 2)
         else:
             assert_greater_than_or_equal(i + 1, e["blocks"])
 
+
 def check_estimates(node, fees_seen):
     check_raw_estimates(node, fees_seen)
     check_smart_estimates(node, fees_seen)
 
 
 def send_tx(node, utxo, feerate):
     """Broadcast a 1in-1out transaction with a specific input and feerate (sat/vb)."""
-    overhead, op, scriptsig, nseq, value, spk = 10, 36, 5, 4, 8, 24
-    tx_size = overhead + op + scriptsig + nseq + value + spk
-    fee = tx_size * feerate
-
     tx = CTransaction()
-    tx.vin = [CTxIn(COutPoint(int(utxo["txid"], 16), utxo["vout"]), SCRIPT_SIG[utxo["vout"]])]
-    tx.vout = [CTxOut(int(utxo["amount"] * COIN) - fee, P2SH_1)]
-    txid = node.sendrawtransaction(tx.serialize().hex())
+    tx.vin = [CTxIn(COutPoint(int(utxo["txid"], 16), utxo["vout"]), REDEEM_SCRIPT)]
+    tx.vout = [CTxOut(int(utxo["amount"] * COIN), P2SH)]
+
+    # vbytes == bytes as we are using legacy transactions
+    fee = tx.get_vsize() * feerate
+    tx.vout[0].nValue -= fee
 
-    return txid
+    return node.sendrawtransaction(tx.serialize().hex())
 
 
 class EstimateFeeTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 3
-        # mine non-standard txs (e.g. txs with "dust" outputs)
         # Force fSendTrickle to true (via whitelist.noban)
         self.extra_args = [
-            ["-acceptnonstdtxn", "-[email protected]"],
-            ["-acceptnonstdtxn", "-[email protected]", "-blockmaxweight=68000"],
-            ["-acceptnonstdtxn", "-[email protected]", "-blockmaxweight=32000"],
+            ["[email protected]"],
+            ["[email protected]", "-blockmaxweight=68000"],
+            ["[email protected]", "-blockmaxweight=32000"],
         ]
 
     def skip_test_if_missing_module(self):
@@ -212,11 +182,17 @@ def transact_and_mine(self, numblocks, mining_node):
             random.shuffle(self.confutxo)
             for _ in range(random.randrange(100 - 50, 100 + 50)):
                 from_index = random.randint(1, 2)
-                (txhex, fee) = small_txpuzzle_randfee(self.nodes[from_index], self.confutxo,
-                                                      self.memutxo, Decimal("0.005"), min_fee, min_fee)
+                (txhex, fee) = small_txpuzzle_randfee(
+                    self.nodes[from_index],
+                    self.confutxo,
+                    self.memutxo,
+                    Decimal("0.005"),
+                    min_fee,
+                    min_fee,
+                )
                 tx_kbytes = (len(txhex) // 2) / 1000.0
                 self.fees_per_kb.append(float(fee) / tx_kbytes)
-            self.sync_mempools(wait=.1)
+            self.sync_mempools(wait=0.1)
             mined = mining_node.getblock(self.generate(mining_node, 1)[0], True)["tx"]
             # update which txouts are confirmed
             newmem = []
@@ -229,46 +205,45 @@ def transact_and_mine(self, numblocks, mining_node):
 
     def initial_split(self, node):
         """Split two coinbase UTxOs into many small coins"""
-        self.txouts = []
-        self.txouts2 = []
-        # Split a coinbase into two transaction puzzle outputs
-        split_inputs(node, node.listunspent(0), self.txouts, True)
-
-        # Mine
+        utxo_count = 2048
+        self.confutxo = []
+        splitted_amount = Decimal("0.04")
+        fee = Decimal("0.1")
+        change = Decimal("100") - splitted_amount * utxo_count - fee
+        tx = CTransaction()
+        tx.vin = [
+            CTxIn(COutPoint(int(cb["txid"], 16), cb["vout"]))
+            for cb in node.listunspent()[:2]
+        ]
+        tx.vout = [CTxOut(int(splitted_amount * COIN), P2SH) for _ in range(utxo_count)]
+        tx.vout.append(CTxOut(int(change * COIN), P2SH))
+        txhex = node.signrawtransactionwithwallet(tx.serialize().hex())["hex"]
+        txid = node.sendrawtransaction(txhex)
+        self.confutxo = [
+            {"txid": txid, "vout": i, "amount": splitted_amount}
+            for i in range(utxo_count)
+        ]
         while len(node.getrawmempool()) > 0:
             self.generate(node, 1, sync_fun=self.no_op)
 
-        # Repeatedly split those 2 outputs, doubling twice for each rep
-        # Use txouts to monitor the available utxo, since these won't be tracked in wallet
-        reps = 0
-        while reps < 5:
-            # Double txouts to txouts2
-            while len(self.txouts) > 0:
-                split_inputs(node, self.txouts, self.txouts2)
-            while len(node.getrawmempool()) > 0:
-                self.generate(node, 1, sync_fun=self.no_op)
-            # Double txouts2 to txouts
-            while len(self.txouts2) > 0:
-                split_inputs(node, self.txouts2, self.txouts)
-            while len(node.getrawmempool()) > 0:
-                self.generate(node, 1, sync_fun=self.no_op)
-            reps += 1
-
     def sanity_check_estimates_range(self):
         """Populate estimation buckets, assert estimates are in a sane range and
         are strictly increasing as the target decreases."""
         self.fees_per_kb = []
         self.memutxo = []
-        self.confutxo = self.txouts  # Start with the set of confirmed txouts after splitting
         self.log.info("Will output estimates for 1/2/3/6/15/25 blocks")
 
         for _ in range(2):
-            self.log.info("Creating transactions and mining them with a block size that can't keep up")
+            self.log.info(
+                "Creating transactions and mining them with a block size that can't keep up"
+            )
             # Create transactions and mine 10 small blocks with node 2, but create txs faster than we can mine
             self.transact_and_mine(10, self.nodes[2])
             check_estimates(self.nodes[1], self.fees_per_kb)
 
-            self.log.info("Creating transactions and mining them at a block size that is just big enough")
+            self.log.info(
+                "Creating transactions and mining them at a block size that is just big enough"
+            )
             # Generate transactions while mining 10 more blocks, this time with node1
             # which mines blocks with capacity just above the rate that transactions are being created
             self.transact_and_mine(10, self.nodes[1])
@@ -277,12 +252,13 @@ def sanity_check_estimates_range(self):
         # Finish by mining a normal-sized block:
         while len(self.nodes[1].getrawmempool()) > 0:
             self.generate(self.nodes[1], 1)
+
         self.log.info("Final estimates after emptying mempools")
         check_estimates(self.nodes[1], self.fees_per_kb)
 
     def test_feerate_mempoolminfee(self):
-        high_val = 3*self.nodes[1].estimatesmartfee(1)['feerate']
-        self.restart_node(1, extra_args=[f'-minrelaytxfee={high_val}'])
+        high_val = 3 * self.nodes[1].estimatesmartfee(1)["feerate"]
+        self.restart_node(1, extra_args=[f"-minrelaytxfee={high_val}"])
         check_estimates(self.nodes[1], self.fees_per_kb)
         self.restart_node(1)
 
@@ -303,7 +279,7 @@ def sanity_check_rbf_estimates(self, utxos):
         utxos_to_respend = []
         txids_to_replace = []
 
-        assert len(utxos) >= 250
+        assert_greater_than_or_equal(len(utxos), 250)
         for _ in range(5):
             # Broadcast 45 low fee transactions that will need to be RBF'd
             for _ in range(45):
@@ -315,27 +291,24 @@ def sanity_check_rbf_estimates(self, utxos):
             for _ in range(5):
                 send_tx(node, utxos.pop(0), low_feerate)
             # Mine the transactions on another node
-            self.sync_mempools(wait=.1, nodes=[node, miner])
+            self.sync_mempools(wait=0.1, nodes=[node, miner])
             for txid in txids_to_replace:
                 miner.prioritisetransaction(txid=txid, fee_delta=-COIN)
             self.generate(miner, 1)
             # RBF the low-fee transactions
-            while True:
-                try:
-                    u = utxos_to_respend.pop(0)
-                    send_tx(node, u, high_feerate)
-                except IndexError:
-                    break
+            while len(utxos_to_respend) > 0:
+                u = utxos_to_respend.pop(0)
+                send_tx(node, u, high_feerate)
 
         # Mine the last replacement txs
-        self.sync_mempools(wait=.1, nodes=[node, miner])
+        self.sync_mempools(wait=0.1, nodes=[node, miner])
         self.generate(miner, 1)
 
         # Only 10% of the transactions were really confirmed with a low feerate,
         # the rest needed to be RBF'd. We must return the 90% conf rate feerate.
-        high_feerate_kvb = Decimal(high_feerate) / COIN * 10**3
+        high_feerate_kvb = Decimal(high_feerate) / COIN * 10 ** 3
         est_feerate = node.estimatesmartfee(2)["feerate"]
-        assert est_feerate == high_feerate_kvb
+        assert_equal(est_feerate, high_feerate_kvb)
 
     def run_test(self):
         self.log.info("This test is time consuming, please be patient")
@@ -359,7 +332,9 @@ def run_test(self):
         self.sanity_check_estimates_range()
 
         # check that the effective feerate is greater than or equal to the mempoolminfee even for high mempoolminfee
-        self.log.info("Test fee rate estimation after restarting node with high MempoolMinFee")
+        self.log.info(
+            "Test fee rate estimation after restarting node with high MempoolMinFee"
+        )
         self.test_feerate_mempoolminfee()
 
         self.log.info("Restarting node with fresh estimation")
@@ -375,9 +350,10 @@ def run_test(self):
 
         self.log.info("Testing that fee estimation is disabled in blocksonly.")
         self.restart_node(0, ["-blocksonly"])
-        assert_raises_rpc_error(-32603, "Fee estimation disabled",
-                                self.nodes[0].estimatesmartfee, 2)
+        assert_raises_rpc_error(
+            -32603, "Fee estimation disabled", self.nodes[0].estimatesmartfee, 2
+        )
 
 
-if __name__ == '__main__':
+if __name__ == "__main__":
     EstimateFeeTest().main()