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.
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 | stratospher, cedwies, rkrux |
| Concept ACK | yuvicc |
| Stale ACK | enirox001 |
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 assert_equal(sibling1.txid_hex, sibling2.txid_hex)
5 assert_not_equal(sibling1.wtxid_hex, sibling2.wtxid_hex)
6 assert_equal(len(tx_originator.getrawmempool()), 1) # Should be 1 from previous test
7 parent_raw = siblings_parent['hex']
8 parent_tx = tx_from_hex(parent_raw)
9 tx_returner.send_without_ping(msg_tx(parent_tx))
10 self.wait_until(lambda: len(tx_originator.getrawmempool()) > 1)
11 self.log.info(" - siblings' parent added to the mempool")
12 tx_originator.sendrawtransaction(hexstring=sibling1.serialize_with_witness().hex(), maxfeerate=0.1)
13 self.log.info(" - sent sibling1: ok")
14 ignoring_msg = f"Ignoring unnecessary request to schedule an already scheduled transaction: txid={sibling2.txid_hex}, wtxid={sibling2.wtxid_hex}"
15 with tx_originator.busy_wait_for_debug_log(expected_msgs=[ignoring_msg.encode()]):
16 tx_originator.sendrawtransaction(hexstring=sibling2.serialize_with_witness().hex(), maxfeerate=0.1)
17 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
I feel there is scope for generalising this function further. Similar to how the previous class implementation used only the
CTransactionclass 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.
The motivation to change this to a function was to address this -> #33067#pullrequestreview-3063408783, #32385 (review), with the current implementation, i’m trying to balance between making this simpler to use and integrate with other tests and to maintain some genericness, IMO the diff above doesnt ease the burden of the caller
Ok, I have gone through both the linked comments now.
The main issue that I see in the current implementation is that both the MiniWallet and the node are required to be passed in this function, which unnecessarily ties the two of them together in an utility function that only needs to manipulate & create the related transaction.
From the #32385 (comment):
one has to create a new dedicated parent transaction, get it in the mempool and then broadcast the two children. Also, one has to handle the fees manually.
The diff that I suggested earlier helps in avoiding creating a dedicated parent transaction within the utility function, and even doesn’t broadcast to the mempool. The following diff below, slightly altered from the previous implementation, doesn’t even accept a fee and let the caller handle. Although the final diff suggested in the 32385 linked comment suggests a completely different approach that malleates only one transaction instead of creating a transaction package - if that approach is preferred, I’m fine with that as well.
IMO the diff above doesnt ease the burden of the caller
I don’t think it’s a burden for the caller but rather provides flexibility to the caller to create a starting parent transaction and broadcast it whenever it wishes.
0diff --git a/test/functional/mempool_accept_wtxid.py b/test/functional/mempool_accept_wtxid.py
1index a49a886d3a..179ddb0d38 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@@ -34,9 +34,10 @@ class MempoolWtxidTest(BitcoinTestFramework):
14 assert_equal(node.getmempoolinfo()['size'], 0)
15
16 self.log.info("Submit parent with multiple script branches to mempool")
17- parent_amount = int(9.99998 * COIN)
18- child_amount = int(9.99996 * COIN)
19- _, child_one, child_two = valid_witness_malleate_tx(mini_wallet, node, parent_amount, child_amount)
20+ parent, child_one, child_two = build_malleated_tx_package(parent=mini_wallet.create_self_transfer()["tx"],
21+ rebalance_parent_output_amount=int(9.99998 * COIN),
22+ malleated_child_amount=int(9.99996 * COIN))
23+ mini_wallet.sendrawtransaction(from_node=node, tx_hex=parent.serialize().hex())
24
25 self.generate(node, 1)
26
27diff --git a/test/functional/test_framework/script_util.py b/test/functional/test_framework/script_util.py
28index f3b3c803ed..5ab97d6753 100755
29--- a/test/functional/test_framework/script_util.py
30+++ b/test/functional/test_framework/script_util.py
31@@ -6,6 +6,9 @@
32 import unittest
33
34 from copy import deepcopy
35+from test_framework.util import (
36+ assert_greater_than_or_equal,
37+)
38
39 from test_framework.messages import (
40 COutPoint,
41@@ -150,19 +153,23 @@ def check_script(script):
42 assert False
43
44
45-def valid_witness_malleate_tx(mini_wallet, node, parent_amount, child_amount):
46+def build_malleated_tx_package(*, parent: CTransaction, rebalance_parent_output_amount, malleated_child_amount):
47 """
48- Creates a valid witness malleation transaction:
49- - Parent transaction with a script supporting 2 branches
50- - 2 child transactions with the same txid but different wtxids
51+ Returns a transaction package with valid witness:
52+ - Parent transaction whose last output contains a script that has two spending conditions
53+ - Two malleated child transactions with same txid but different wtxids because of different witnesses
54 """
55 hashlock = hash160(b'Preimage')
56 witness_script = CScript([OP_IF, OP_HASH160, hashlock, OP_EQUAL, OP_ELSE, OP_TRUE, OP_ENDIF])
57 witness_program = sha256(witness_script)
58 script_pubkey = CScript([OP_0, witness_program])
59
60- # Create parent transaction with a script supporting 2 branches
61- parent = mini_wallet.send_to(from_node=node, scriptPubKey=script_pubkey, amount=parent_amount, fee=1000)
62+ # Append to the transaction the vout containing the script supporting two spending conditions
63+ assert_greater_than_or_equal(len(parent.vout), 1)
64+ last_output = parent.vout[len(parent.vout) - 1]
65+ assert_greater_than_or_equal(last_output.nValue, rebalance_parent_output_amount)
66+ last_output.nValue -= rebalance_parent_output_amount
67+ parent.vout.append(CTxOut(rebalance_parent_output_amount, script_pubkey))
68
69 # Create 2 valid children that differ only in witness data.
70 # 1. Create a new transaction with witness solving first branch
71@@ -171,13 +178,15 @@ def valid_witness_malleate_tx(mini_wallet, node, parent_amount, child_amount):
72 child_script_pubkey = CScript([OP_0, child_witness_program])
73 child_one = CTransaction()
74
75- child_one.vin.append(CTxIn(COutPoint(int(parent['txid'], 16), parent['sent_vout']), b""))
76- child_one.vout.append(CTxOut(child_amount, child_script_pubkey))
77+ child_one.vin.append(CTxIn(COutPoint(int(parent.txid_hex, 16), len(parent.vout) - 1), b""))
78+ child_one.vout.append(CTxOut(malleated_child_amount, child_script_pubkey))
79 child_one.wit.vtxinwit.append(CTxInWitness())
80 child_one.wit.vtxinwit[0].scriptWitness.stack = [b'Preimage', b'\x01', witness_script]
81+
82 # 2. Create another identical transaction with witness solving second branch
83 child_two = deepcopy(child_one)
84 child_two.wit.vtxinwit[0].scriptWitness.stack = [b'', witness_script]
85+
86 return parent, child_one, child_two
Having said this, if other reviewers prefer the current approach in this PR, I don’t see a strong reason to block (n-a-c-k) the PR based on this but I don’t find the function generic enough to treat it as a utility function.
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.
mempool_accept_wtxid, but simplifying build_malleated_tx_package affects another test, so I broadened the description to reflect this.
40- parent = txgen.build_parent_tx(txid, 9.99998 * COIN)
41
42- privkeys = [node.get_deterministic_priv_key().key]
43- raw_parent = node.signrawtransactionwithkey(hexstring=parent.serialize().hex(), privkeys=privkeys)['hex']
44- signed_parent_txid = node.sendrawtransaction(hexstring=raw_parent, maxfeerate=0)
45+ parent, child_one, child_two = build_malleated_tx_package(parent=mini_wallet.create_self_transfer()["tx"],
30ea904: style nit: here and in p2p_private_broadcast.py
0parent, child_one, child_two = build_malleated_tx_package(
1 parent=mini_wallet.create_self_transfer()["tx"],
2 rebalance_parent_output_amount=int(9.99998 * COIN),
3 child_amount=int(9.99996 * COIN)
4)
172 """
173- Creates a valid witness malleation transaction test case:
174- - Parent transaction with a script supporting 2 branches
175- - 2 child transactions with the same txid but different wtxids
176+ Returns a transaction package with valid witness:
177+ - Parent transaction whose last output contains a script that has two spending conditions
30ea9047: maybe also useful to mention the kind of parent we’re expecting to be passed into build_malleated_tx_package. something like:
Expects parent with anyone-can-spend inputs (ex: MiniWallet’s OP_TRUE) to allow output modification.
we could also remove wallet.sign_tx(siblings_parent) in p2p_private_broadcast.py since signing doesn’t really do anything - just adds OP_TRUE again in the witness.
Simplify the witness malleation test helper by converting the
ValidWitnessMalleatedTx class to a standalone function
build_malleated_tx_package() and updating call sites.
Co-authored-by: rkrux <rkrux.connect@gmail.com>
Nits if you end up touching the PR for some other reason:
0diff --git a/test/functional/test_framework/script_util.py b/test/functional/test_framework/script_util.py
1index 812d9bbd0a..f4d7d371e5 100755
2--- a/test/functional/test_framework/script_util.py
3+++ b/test/functional/test_framework/script_util.py
4@@ -169,7 +169,7 @@ def check_script(script):
5
6 def build_malleated_tx_package(*, parent: CTransaction, rebalance_parent_output_amount, child_amount):
7 """
8- Returns a transaction package with valid witness:
9+ Returns a transaction package with valid witnesses:
10 - Parent transaction whose last output contains a script that has two spending conditions
11 - Two malleated child transactions with same txid but different wtxids because of different witnesses
12
13@@ -189,7 +189,6 @@ def build_malleated_tx_package(*, parent: CTransaction, rebalance_parent_output_
14 last_output.nValue -= rebalance_parent_output_amount
15 parent.vout.append(CTxOut(rebalance_parent_output_amount, script_pubkey))
16
17-
18 # Create 2 valid children that differ only in witness data.
19 # 1. Create a new transaction with witness solving first branch
20 child_witness_script = CScript([OP_TRUE])
21@@ -201,6 +200,7 @@ def build_malleated_tx_package(*, parent: CTransaction, rebalance_parent_output_
22 child_one.vout.append(CTxOut(child_amount, child_script_pubkey))
23 child_one.wit.vtxinwit.append(CTxInWitness())
24 child_one.wit.vtxinwit[0].scriptWitness.stack = [b'Preimage', b'\x01', witness_script]
25+
26 # 2. Create another identical transaction with witness solving second branch
27 child_two = deepcopy(child_one)
28 child_two.wit.vtxinwit[0].scriptWitness.stack = [b'', witness_script]
29(END)
43- privkeys = [node.get_deterministic_priv_key().key]
44- raw_parent = node.signrawtransactionwithkey(hexstring=parent.serialize().hex(), privkeys=privkeys)['hex']
45- signed_parent_txid = node.sendrawtransaction(hexstring=raw_parent, maxfeerate=0)
46+ parent = mini_wallet.create_self_transfer()["tx"]
47+ parent_amount = parent.vout[0].nValue - 10000
48+ child_amount = parent_amount - 10000
I believe it’s unlikely this’d be flaky but it’s quite odd to calculate the parent amount based on the first output that ultimately gets subtracted from the last output in case there are multiple outputs in the transaction. I’d prefer to only use the last output for these calculations.
Also, the C = A - B phrase reads fine for the child amount because the reader can realise that B is going away in fees when A is spent, but using the same phrase for calculating the parent amount that’s only going to be rebalanced can be confusing for the reader - can just split the last output amount into two.
0diff --git a/test/functional/mempool_accept_wtxid.py b/test/functional/mempool_accept_wtxid.py
1index a9edea6d64..c44b6b0544 100755
2--- a/test/functional/mempool_accept_wtxid.py
3+++ b/test/functional/mempool_accept_wtxid.py
4@@ -32,8 +32,8 @@ class MempoolWtxidTest(BitcoinTestFramework):
5 self.log.info("Submit parent with multiple script branches to mempool")
6
7 parent = mini_wallet.create_self_transfer()["tx"]
8- parent_amount = parent.vout[0].nValue - 10000
9- child_amount = parent_amount - 10000
10+ parent_amount = int(parent.vout[len(parent.vout) - 1].nValue / 2)
11+ child_amount = parent_amount - 10000 # less fees
12 parent, child_one, child_two = build_malleated_tx_package(
13 parent=parent,
14 rebalance_parent_output_amount=parent_amount,
ACK 3f5211cba8e73e8eb03781e6ec32ba9c4a263782
Thanks for incorporating the previous suggestions. The latest implementation deals only with the CTransaction object just like the previous class implementation, and is not dependent on the wallet or the node, which is preferable IMO.
While the function implementation is okay, I don’t feel only it should be highlighted in the title of the PR. There are other test improvements as well (Miniwallet, pre mined chain), maybe alter the title?
The nits I shared below can be done if there is any other need to touch this PR.