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 |
|---|---|
| ACK | enirox001, cedwies |
| Concept ACK | yuvicc, stratospher |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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")
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