Merge bitcoin/bitcoin#33067: test: refactor ValidWitnessMalleatedTx class to helper function

3f5211cba8e73e8eb03781e6ec32ba9c4a263782 test: remove child_one/child_two (w)txid variables (naiyoma)
7cfe790820cf247e8a27bb8091defc54c74d6aec test: replace ValidWitnessMalleatedTx class with function (naiyoma)
81675a781f3ab62a0576a9739d13b4997b63230d test: use pre-generated chain (naiyoma)

Pull request description:

  This PR refactors ` ValidWitnessMalleatedTx` class into a `build_malleated_tx_package` function. As a result, two tests are updated:  `mempool_accept_wtxid` and `p2p_p2p_private_broadcast`. Also included are a  few small refactors in mempool_accept_wtxid , (switching to MiniWallet, using a pre-mined chain, using txid directly.)

  Together, these changes reduce complexity and improve test runtime.

ACKs for top commit:
  stratospher:
    reACK 3f5211c.
  cedwies:
    reACK 3f5211c
  maflcko:
    review ACK 3f5211cba8e73e8eb03781e6ec32ba9c4a263782 👥
  rkrux:
    ACK 3f5211cba8e73e8eb03781e6ec32ba9c4a263782

Tree-SHA512: 1fd02be3432fef6b68e54fbe8b15ed56d2699580bb13d0777b21f9cbe4c6d33bbb710541e3ca2fc93eab771d17bf1c427e4b08fa216d561bdb320cc6b36ac8fc

test/functional/mempool_accept_wtxid.py

@@ -7,70 +7,65 @@ Test mempool acceptance in case of an already known transaction
 with identical non-witness data but different witness.
 """
 
-from test_framework.messages import (
-    COIN,
-)
 from test_framework.p2p import P2PTxInvStore
-from test_framework.script_util import ValidWitnessMalleatedTx
+from test_framework.script_util import build_malleated_tx_package
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_not_equal,
     assert_equal,
 )
-
+from test_framework.wallet import (
+    MiniWallet,
+)
 
 class MempoolWtxidTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
-        self.setup_clean_chain = True
 
     def run_test(self):
         node = self.nodes[0]
+        mini_wallet = MiniWallet(node)
+        self.log.info('Start with pre-generated blocks')
 
-        self.log.info('Start with empty mempool and 101 blocks')
-        # The last 100 coinbase transactions are premature
-        blockhash = self.generate(node, 101)[0]
-        txid = node.getblock(blockhash=blockhash, verbosity=2)["tx"][0]["txid"]
         assert_equal(node.getmempoolinfo()['size'], 0)
 
         self.log.info("Submit parent with multiple script branches to mempool")
-        txgen = ValidWitnessMalleatedTx()
-        parent = txgen.build_parent_tx(txid, 9.99998 * COIN)
 
-        privkeys = [node.get_deterministic_priv_key().key]
-        raw_parent = node.signrawtransactionwithkey(hexstring=parent.serialize().hex(), privkeys=privkeys)['hex']
-        signed_parent_txid = node.sendrawtransaction(hexstring=raw_parent, maxfeerate=0)
+        parent = mini_wallet.create_self_transfer()["tx"]
+        parent_amount = parent.vout[0].nValue - 10000
+        child_amount = parent_amount - 10000
+        parent, child_one, child_two = build_malleated_tx_package(
+            parent=parent,
+            rebalance_parent_output_amount=parent_amount,
+            child_amount=child_amount
+        )
+
+        mini_wallet.sendrawtransaction(from_node=node, tx_hex=parent.serialize().hex())
+
         self.generate(node, 1)
 
         peer_wtxid_relay = node.add_p2p_connection(P2PTxInvStore())
 
-        child_one, child_two = txgen.build_malleated_children(signed_parent_txid, 9.99996 * COIN)
-        child_one_wtxid = child_one.wtxid_hex
-        child_one_txid = child_one.txid_hex
-        child_two_wtxid = child_two.wtxid_hex
-        child_two_txid = child_two.txid_hex
-
-        assert_equal(child_one_txid, child_two_txid)
-        assert_not_equal(child_one_wtxid, child_two_wtxid)
+        assert_equal(child_one.txid_hex, child_two.txid_hex)
+        assert_not_equal(child_one.wtxid_hex, child_two.wtxid_hex)
 
         self.log.info("Submit child_one to the mempool")
         txid_submitted = node.sendrawtransaction(child_one.serialize().hex())
-        assert_equal(node.getmempoolentry(txid_submitted)['wtxid'], child_one_wtxid)
-
-        peer_wtxid_relay.wait_for_broadcast([child_one_wtxid])
+        assert_equal(node.getmempoolentry(txid_submitted)['wtxid'], child_one.wtxid_hex)
+        peer_wtxid_relay.wait_for_broadcast([child_one.wtxid_hex])
         assert_equal(node.getmempoolinfo()["unbroadcastcount"], 0)
 
         # testmempoolaccept reports the "already in mempool" error
         assert_equal(node.testmempoolaccept([child_one.serialize().hex()]), [{
-            "txid": child_one_txid,
-            "wtxid": child_one_wtxid,
+            "txid": child_one.txid_hex,
+            "wtxid": child_one.wtxid_hex,
             "allowed": False,
             "reject-reason": "txn-already-in-mempool",
             "reject-details": "txn-already-in-mempool"
         }])
         assert_equal(node.testmempoolaccept([child_two.serialize().hex()])[0], {
-            "txid": child_two_txid,
-            "wtxid": child_two_wtxid,
+            "txid": child_two.txid_hex,
+            "wtxid": child_two.wtxid_hex,
             "allowed": False,
             "reject-reason": "txn-same-nonwitness-data-in-mempool",
             "reject-details": "txn-same-nonwitness-data-in-mempool"
@@ -88,7 +83,7 @@ class MempoolWtxidTest(BitcoinTestFramework):
 
         # The node should rebroadcast the transaction using the wtxid of the correct transaction
         # (child_one, which is in its mempool).
-        peer_wtxid_relay_2.wait_for_broadcast([child_one_wtxid])
+        peer_wtxid_relay_2.wait_for_broadcast([child_one.wtxid_hex])
         assert_equal(node.getmempoolinfo()["unbroadcastcount"], 0)
 
 if __name__ == '__main__':

test/functional/p2p_private_broadcast.py

@@ -18,7 +18,6 @@ from test_framework.p2p import (
 from test_framework.messages import (
     CAddress,
     CInv,
-    COIN,
     MSG_WTX,
     malleate_tx_to_invalid_witness,
     msg_inv,
@@ -27,7 +26,7 @@ from test_framework.messages import (
 from test_framework.netutil import (
     format_addr_port
 )
-from test_framework.script_util import ValidWitnessMalleatedTx
+from test_framework.script_util import build_malleated_tx_package
 from test_framework.socks5 import (
     Socks5Configuration,
     Socks5Server,
@@ -399,16 +398,17 @@ class P2PPrivateBroadcast(BitcoinTestFramework):
         tx_originator.setmocktime(0) # Let the clock tick again (it will go backwards due to this).
 
         self.log.info("Sending a pair of transactions with the same txid but different valid wtxids via RPC")
-        txgen = ValidWitnessMalleatedTx()
-        funding = wallet.get_utxo()
-        fee_sat = 1000
-        siblings_parent = txgen.build_parent_tx(funding["txid"], amount=funding["value"] * COIN - fee_sat)
-        sibling1, sibling2 = txgen.build_malleated_children(siblings_parent.txid_hex, amount=siblings_parent.vout[0].nValue - fee_sat)
+        parent = wallet.create_self_transfer()["tx"]
+        parent_amount = parent.vout[0].nValue - 10000
+        child_amount = parent_amount - 10000
+        siblings_parent, sibling1, sibling2 = build_malleated_tx_package(
+            parent=parent,
+            rebalance_parent_output_amount=parent_amount,
+            child_amount=child_amount)
         self.log.info(f"  - sibling1: txid={sibling1.txid_hex}, wtxid={sibling1.wtxid_hex}")
         self.log.info(f"  - sibling2: txid={sibling2.txid_hex}, wtxid={sibling2.wtxid_hex}")
         assert_equal(sibling1.txid_hex, sibling2.txid_hex)
         assert_not_equal(sibling1.wtxid_hex, sibling2.wtxid_hex)
-        wallet.sign_tx(siblings_parent)
         assert_equal(len(tx_originator.getrawmempool()), 1)
         tx_returner.send_without_ping(msg_tx(siblings_parent))
         self.wait_until(lambda: len(tx_originator.getrawmempool()) > 1)

test/functional/test_framework/script_util.py

@@ -36,7 +36,10 @@ from test_framework.script import (
     hash160,
 )
 
-from test_framework.util import assert_equal
+from test_framework.util import (
+    assert_greater_than_or_equal,
+    assert_equal,
+)
 
 # Maximum number of potentially executed legacy signature operations in validating a transaction.
 MAX_STD_LEGACY_SIGOPS = 2_500
@@ -164,43 +167,44 @@ def check_script(script):
     assert False
 
 
-class ValidWitnessMalleatedTx:
+def build_malleated_tx_package(*, parent: CTransaction, rebalance_parent_output_amount, child_amount):
     """
-    Creates a valid witness malleation transaction test case:
-    - Parent transaction with a script supporting 2 branches
-    - 2 child transactions with the same txid but different wtxids
+    Returns a transaction package with valid witness:
+    - Parent transaction whose last output contains a script that has two spending conditions
+    - Two malleated child transactions with same txid but different wtxids because of different witnesses
+
+    Args:
+        parent: Transaction with modifiable outputs. Either unsigned (sign after
+        calling this function) or anyone-can-spend (e.g., MiniWallet's OP_TRUE).
     """
-    def __init__(self):
-        hashlock = hash160(b'Preimage')
-        self.witness_script = CScript([OP_IF, OP_HASH160, hashlock, OP_EQUAL, OP_ELSE, OP_TRUE, OP_ENDIF])
+    hashlock = hash160(b'Preimage')
+    witness_script = CScript([OP_IF, OP_HASH160, hashlock, OP_EQUAL, OP_ELSE, OP_TRUE, OP_ENDIF])
+    witness_program = sha256(witness_script)
+    script_pubkey = CScript([OP_0, witness_program])
 
-    def build_parent_tx(self, funding_txid, amount):
-        # Create an unsigned parent transaction paying to the witness script.
-        witness_program = sha256(self.witness_script)
-        script_pubkey = CScript([OP_0, witness_program])
+    # Append to the transaction the vout containing the script supporting 2 spending conditions
+    assert_greater_than_or_equal(len(parent.vout), 1)
+    last_output = parent.vout[len(parent.vout) - 1]
+    assert_greater_than_or_equal(last_output.nValue, rebalance_parent_output_amount)
+    last_output.nValue -= rebalance_parent_output_amount
+    parent.vout.append(CTxOut(rebalance_parent_output_amount, script_pubkey))
 
-        parent = CTransaction()
-        parent.vin.append(CTxIn(COutPoint(int(funding_txid, 16), 0), b""))
-        parent.vout.append(CTxOut(int(amount), script_pubkey))
-        return parent
 
-    def build_malleated_children(self, signed_parent_txid, amount):
-        # Create 2 valid children that differ only in witness data.
-        # 1. Create a new transaction with witness solving first branch
-        child_witness_script = CScript([OP_TRUE])
-        child_witness_program = sha256(child_witness_script)
-        child_script_pubkey = CScript([OP_0, child_witness_program])
+    # Create 2 valid children that differ only in witness data.
+    # 1. Create a new transaction with witness solving first branch
+    child_witness_script = CScript([OP_TRUE])
+    child_witness_program = sha256(child_witness_script)
+    child_script_pubkey = CScript([OP_0, child_witness_program])
+    child_one = CTransaction()
 
-        child_one = CTransaction()
-        child_one.vin.append(CTxIn(COutPoint(int(signed_parent_txid, 16), 0), b""))
-        child_one.vout.append(CTxOut(int(amount), child_script_pubkey))
-        child_one.wit.vtxinwit.append(CTxInWitness())
-        child_one.wit.vtxinwit[0].scriptWitness.stack = [b'Preimage', b'\x01', self.witness_script]
-
-        # 2. Create another identical transaction with witness solving second branch
-        child_two = deepcopy(child_one)
-        child_two.wit.vtxinwit[0].scriptWitness.stack = [b'', self.witness_script]
-        return child_one, child_two
+    child_one.vin.append(CTxIn(COutPoint(int(parent.txid_hex, 16), len(parent.vout) - 1), b""))
+    child_one.vout.append(CTxOut(child_amount, child_script_pubkey))
+    child_one.wit.vtxinwit.append(CTxInWitness())
+    child_one.wit.vtxinwit[0].scriptWitness.stack = [b'Preimage', b'\x01', witness_script]
+    # 2. Create another identical transaction with witness solving second branch
+    child_two = deepcopy(child_one)
+    child_two.wit.vtxinwit[0].scriptWitness.stack = [b'', witness_script]
+    return parent, child_one, child_two
 
 
 class TestFrameworkScriptUtil(unittest.TestCase):