mempool_accept_wtxid.py by using a pre-mined chain instead of generating new blocks, switching to MiniWallet to avoid RPC calls for signing transactions, replacing class ValidWitnessMalleatedTx with valid_witness_malleate_tx() function, and simplifying the code by using txid.hex directly.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33067.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | yuvicc, stratospher, rkrux |
| Stale ACK | enirox001, cedwies |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Tested on MacOS 15.5 (M2, Debug build)
ACK 9b37c1f
mempool_accept
This PR improves mempool_accept_wtxid.py by:
1. Using a pre-mined chain instead of generating new blocks [522bf76](https://github.com/bitcoin/bitcoin/commit/522bf76d7d8058872a008a721831da264881746d) 2. Using MiniWallet to avoid RPC calls for signing transactions [4ec5ae9](https://github.com/bitcoin/bitcoin/commit/4ec5ae9fe126ffabcd429277092e3b27f483d430) 3. Removing child txid variables and using txid.hex directly [361ebd5](https://github.com/bitcoin/bitcoin/commit/361ebd56eb817b1e054ee1aacc87bf60c06c6b94)
Not sure why the pull description is just a copy-paste of the commit list (with outdated commit ids). It would be better to remove those. If anyone wants to look at the list of commit ids, they can already see them directly, no need to copy them.
37 assert_equal(node.getmempoolinfo()['size'], 0)
38
39 self.log.info("Submit parent with multiple script branches to mempool")
40 txgen = ValidWitnessMalleatedTx()
41- parent = txgen.build_parent_tx(txid, 9.99998 * COIN)
42+ parent = wallet.send_to(from_node=node,scriptPubKey=txgen.get_script_pubkey(),amount=int(9.99998 * COIN),fee=1000)
0 parent = wallet.send_to(from_node=node, scriptPubKey=txgen.get_script_pubkey(), amount=int(9.99998 * COIN), fee=1000)
style nit: space after , (here and in other places too)
Concept ACK. I like how you’ve used MiniWallet to remove the signing bits (signrawtransactionwithkey) which needed the private key.
You can simplify this further. ValidWitnessMalleatedTx returned parent transaction and children transaction in 2 different steps because the children transactions needed the parent’s txid which required signing with private key. Now since we don’t do signrawtransactionwithkey, you can replace class ValidWitnessMalleatedTx with a function valid_witness_malleate_tx() which does everything done previously in the class and return parent, child_one, child_two together. I think this would remove the concern in #32385 (review)!
ACK 0cbcdca
Solid test refactor with significant performance improvement:
Test coverage remains identical, CI passes.
Changes look great.
Nice to see the valid_witness_malleate_tx helper instead of the ValidWitnessMalleatedTx class. Makes the test’s logic much clearer IMO.
ACK 0cbcdca
150@@ -150,43 +151,35 @@ def check_script(script):
151 assert False
152
153
154-class ValidWitnessMalleatedTx:
155+def valid_witness_malleate_tx(wallet, node):
d1a2e27: it would be nice to have a more general interface where we can pass the parent + child amount and fees.
cc @vasild - you might be interested in this too. I hadn’t considered passing both wallet and node as arguments to the function in #32385. there are other functions in util.py which do this too - ex: create_lots_of_big_transactions
With the changes in this PR, how would the below code look like?
0 self.log.info("Sending a pair of transactions with the same txid but different valid wtxids via RPC")
1 txgen = ValidWitnessMalleatedTx()
2 funding = wallet.get_utxo()
3 fee_sat = 1000
4 siblings_parent = txgen.build_parent_tx(funding["txid"], amount=funding["value"] * COIN - fee_sat)
5 sibling1, sibling2 = txgen.build_malleated_children(siblings_parent.txid_hex, amount=siblings_parent.vout[0].nValue - fee_sat)
6 self.log.info(f" - sibling1: txid={sibling1.txid_hex}, wtxid={sibling1.wtxid_hex}")
7 self.log.info(f" - sibling2: txid={sibling2.txid_hex}, wtxid={sibling2.wtxid_hex}")
8 assert_equal(sibling1.txid_hex, sibling2.txid_hex)
9 assert_not_equal(sibling1.wtxid_hex, sibling2.wtxid_hex)
10 wallet.sign_tx(siblings_parent)
11 assert_equal(len(tx_originator.getrawmempool()), 1)
12 tx_returner.send_without_ping(msg_tx(siblings_parent))
13 self.wait_until(lambda: len(tx_originator.getrawmempool()) > 1)
14 self.log.info(" - siblings' parent added to the mempool")
15 tx_originator.sendrawtransaction(hexstring=sibling1.serialize_with_witness().hex(), maxfeerate=0.1)
16 self.log.info(" - sent sibling1: ok")
17 ignoring_msg = f"Ignoring unnecessary request to schedule an already scheduled transaction: txid={sibling2.txid_hex}, wtxid={sibling2.wtxid_hex}"
18 with tx_originator.busy_wait_for_debug_log(expected_msgs=[ignoring_msg.encode()]):
19 tx_originator.sendrawtransaction(hexstring=sibling2.serialize_with_witness().hex(), maxfeerate=0.1)
20 self.log.info(" - sibling2 rejected because it has the same txid: ok")
it is found in #29415 in test/functional/p2p_private_broadcast.py.
I have tried to make this work, but first I’d have to change valid_witness_malleate_tx to also return the parent
return parent, child_one, child_two
and then 👇🏽
0
1 siblings_parent, sibling1, sibling2 = valid_witness_malleate_tx(wallet, tx_originator)
2 self.log.info(f" - sibling1: txid={sibling1.txid_hex}, wtxid={sibling1.wtxid_hex}")
3 self.log.info(f" - sibling2: txid={sibling2.txid_hex}, wtxid={sibling2.wtxid_hex}")
4 import pdb; pdb.set_trace()
5 assert_equal(sibling1.txid_hex, sibling2.txid_hex)
6 assert_not_equal(sibling1.wtxid_hex, sibling2.wtxid_hex)
7 assert_equal(len(tx_originator.getrawmempool()), 1) # Should be 1 from previous test
8 parent_raw = siblings_parent['hex']
9 parent_tx = tx_from_hex(parent_raw)
10 tx_returner.send_without_ping(msg_tx(parent_tx))
11 self.wait_until(lambda: len(tx_originator.getrawmempool()) > 1)
12 self.log.info(" - siblings' parent added to the mempool")
13 tx_originator.sendrawtransaction(hexstring=sibling1.serialize_with_witness().hex(), maxfeerate=0.1)
14 self.log.info(" - sent sibling1: ok")
15 ignoring_msg = f"Ignoring unnecessary request to schedule an already scheduled transaction: txid={sibling2.txid_hex}, wtxid={sibling2.wtxid_hex}"
16 with tx_originator.busy_wait_for_debug_log(expected_msgs=[ignoring_msg.encode()]):
17 tx_originator.sendrawtransaction(hexstring=sibling2.serialize_with_witness().hex(), maxfeerate=0.1)
18 self.log.info(" - sibling2 rejected because it has the same txid: ok")
In d1a2e278572f70d0d68a21312b2cc7de27f326df “test: replace class ValidWitnessMalleatedTx with function”
Unlike the malleate_tx_to_invalid_witness function that is quite generic (https://github.com/bitcoin/bitcoin/pull/33793/files#diff-1698fabd4ddd298eb28e234d97f34e744f568852b77361384c8edb38819de5e3R259), this function at the moment doesn’t seem generic enough to be present in an util file. Few points regarding the function I noticed:
s/wallet/mini_walletThe class implementation before this PR did pass around only the transactions without wallet or node, which was quite neat imo.
38- parent = txgen.build_parent_tx(txid, 9.99998 * COIN)
39+ child_one, child_two = valid_witness_malleate_tx(wallet, node)
40
41- privkeys = [node.get_deterministic_priv_key().key]
42- raw_parent = node.signrawtransactionwithkey(hexstring=parent.serialize().hex(), privkeys=privkeys)['hex']
43- signed_parent_txid = node.sendrawtransaction(hexstring=raw_parent, maxfeerate=0)
wallet.send_to() to create parent transaction
21@@ -22,14 +22,12 @@
22 class MempoolWtxidTest(BitcoinTestFramework):
23 def set_test_params(self):
24 self.num_nodes = 1
25- self.setup_clean_chain = True
26+ self.setup_clean_chain = False
In 26577cac8b9cf53d5f69e4502346fd5881c92004 “test: use pre-generated chain”
Nit (not strong opinion): Can remove this line altogether as well because False is the default value.
Concept ACK 0cbcdcaf5a7fe1dd864d0019a7d7e4f8b80b5e7c
Makes sense to use the pre-mined chain and the MiniWallet for this use case.
193+ witness_script = CScript([OP_IF, OP_HASH160, hashlock, OP_EQUAL, OP_ELSE, OP_TRUE, OP_ENDIF])
194+ witness_program = sha256(witness_script)
195+ script_pubkey = CScript([OP_0, witness_program])
196+
197+ # Create parent transaction with a script supporting 2 branches
198+ parent = mini_wallet.send_to(from_node=node, scriptPubKey=script_pubkey, amount=parent_amount, fee=1000)
I feel there is scope for generalising this function further. Similar to how the previous class implementation used only the CTransaction class and the like. IMO, the earlier class implementation was fine as well but if the consensus is to prefer the function implementation then we should not lose the genericness.
Currently, this function is tied to the MiniWallet that I don’t think is necessary. A more generic form of this function should be able to be used by the usual RPC wallets as well.
Consider the following diff that puts the responsibility of building the base parent transaction and submitting the final parent transaction to the caller, thereby allowing this function to not use any wallet or node at all.
0diff --git a/test/functional/mempool_accept_wtxid.py b/test/functional/mempool_accept_wtxid.py
1index a49a886d3a..f895772b08 100755
2--- a/test/functional/mempool_accept_wtxid.py
3+++ b/test/functional/mempool_accept_wtxid.py
4@@ -11,7 +11,7 @@ from test_framework.messages import (
5 COIN,
6 )
7 from test_framework.p2p import P2PTxInvStore
8-from test_framework.script_util import valid_witness_malleate_tx
9+from test_framework.script_util import build_malleated_tx_package
10 from test_framework.test_framework import BitcoinTestFramework
11 from test_framework.util import (
12 assert_not_equal,
13@@ -36,7 +36,9 @@ class MempoolWtxidTest(BitcoinTestFramework):
14 self.log.info("Submit parent with multiple script branches to mempool")
15 parent_amount = int(9.99998 * COIN)
16 child_amount = int(9.99996 * COIN)
17- _, child_one, child_two = valid_witness_malleate_tx(mini_wallet, node, parent_amount, child_amount)
18+ parent = mini_wallet.create_self_transfer(fee_rate=0)["tx"]
19+ parent, child_one, child_two = build_malleated_tx_package(parent, parent_amount, child_amount)
20+ mini_wallet.sendrawtransaction(from_node=node, tx_hex=parent.serialize().hex())
21
22 self.generate(node, 1)
23
24diff --git a/test/functional/test_framework/script_util.py b/test/functional/test_framework/script_util.py
25index f3b3c803ed..862e30c088 100755
26--- a/test/functional/test_framework/script_util.py
27+++ b/test/functional/test_framework/script_util.py
28@@ -6,6 +6,10 @@
29 import unittest
30
31 from copy import deepcopy
32+from test_framework.util import (
33+ assert_greater_than_or_equal,
34+ assert_equal,
35+)
36
37 from test_framework.messages import (
38 COutPoint,
39@@ -150,19 +154,22 @@ def check_script(script):
40 assert False
41
42
43-def valid_witness_malleate_tx(mini_wallet, node, parent_amount, child_amount):
44+def build_malleated_tx_package(parent: CTransaction, parent_amount, child_amount, fee=1000):
45 """
46- Creates a valid witness malleation transaction:
47+ Creates a malleated transaction package with valid witness:
48 - Parent transaction with a script supporting 2 branches
49- - 2 child transactions with the same txid but different wtxids
50+ - 2 child transactions with the same txid but different wtxids because of different witnesses
51 """
52 hashlock = hash160(b'Preimage')
53 witness_script = CScript([OP_IF, OP_HASH160, hashlock, OP_EQUAL, OP_ELSE, OP_TRUE, OP_ENDIF])
54 witness_program = sha256(witness_script)
55 script_pubkey = CScript([OP_0, witness_program])
56
57- # Create parent transaction with a script supporting 2 branches
58- parent = mini_wallet.send_to(from_node=node, scriptPubKey=script_pubkey, amount=parent_amount, fee=1000)
59+ # Append to the transaction the vout containing the script supporting 2 spending conditions
60+ assert_equal(len(parent.vout), 1)
61+ assert_greater_than_or_equal(parent.vout[0].nValue, parent_amount + fee)
62+ parent.vout[0].nValue -= (parent_amount + fee)
63+ parent.vout.append(CTxOut(parent_amount, script_pubkey))
64
65 # Create 2 valid children that differ only in witness data.
66 # 1. Create a new transaction with witness solving first branch
67@@ -171,13 +178,15 @@ def valid_witness_malleate_tx(mini_wallet, node, parent_amount, child_amount):
68 child_script_pubkey = CScript([OP_0, child_witness_program])
69 child_one = CTransaction()
70
71- child_one.vin.append(CTxIn(COutPoint(int(parent['txid'], 16), parent['sent_vout']), b""))
72+ child_one.vin.append(CTxIn(COutPoint(int(parent.txid_hex, 16), len(parent.vout) - 1), b""))
73 child_one.vout.append(CTxOut(child_amount, child_script_pubkey))
74 child_one.wit.vtxinwit.append(CTxInWitness())
75 child_one.wit.vtxinwit[0].scriptWitness.stack = [b'Preimage', b'\x01', witness_script]
76+
77 # 2. Create another identical transaction with witness solving second branch
78 child_two = deepcopy(child_one)
79 child_two.wit.vtxinwit[0].scriptWitness.stack = [b'', witness_script]
80+
81 return parent, child_one, child_two
41
42 self.log.info("Submit parent with multiple script branches to mempool")
43- txgen = ValidWitnessMalleatedTx()
44- parent = txgen.build_parent_tx(txid, 9.99998 * COIN)
45+ parent_amount = int(9.99998 * COIN)
46+ child_amount = int(9.99996 * COIN)
Thanks for addressing the comments earlier.
This PR improves mempool_accept_wtxid.py by using a pre-mined chain instead of generating new blocks, switching to MiniWallet to avoid RPC calls for signing transactions
I haven’t timed the test yet but if some test latency improvement has been noticed (ref: #33067#pullrequestreview-3079794265), then it would be helpful to highlight it in the PR description imo.