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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33067.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Tested on MacOS 15.5 (M2, Debug build)
ACK 9b37c1f
Concep ACK
This would make the test consistent with 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)
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?
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)
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)
self.log.info(" - siblings' parent added to the mempool")
tx_originator.sendrawtransaction(hexstring=sibling1.serialize_with_witness().hex(), maxfeerate=0.1)
self.log.info(" - sent sibling1: ok")
ignoring_msg = f"Ignoring unnecessary request to schedule an already scheduled transaction: txid={sibling2.txid_hex}, wtxid={sibling2.wtxid_hex}"
with tx_originator.busy_wait_for_debug_log(expected_msgs=[ignoring_msg.encode()]):
tx_originator.sendrawtransaction(hexstring=sibling2.serialize_with_witness().hex(), maxfeerate=0.1)
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 👇🏽
siblings_parent, sibling1, sibling2 = valid_witness_malleate_tx(wallet, tx_originator)
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)
assert_equal(len(tx_originator.getrawmempool()), 1) # Should be 1 from previous test
parent_raw = siblings_parent['hex']
parent_tx = tx_from_hex(parent_raw)
tx_returner.send_without_ping(msg_tx(parent_tx))
self.wait_until(lambda: len(tx_originator.getrawmempool()) > 1)
self.log.info(" - siblings' parent added to the mempool")
tx_originator.sendrawtransaction(hexstring=sibling1.serialize_with_witness().hex(), maxfeerate=0.1)
self.log.info(" - sent sibling1: ok")
ignoring_msg = f"Ignoring unnecessary request to schedule an already scheduled transaction: txid={sibling2.txid_hex}, wtxid={sibling2.wtxid_hex}"
with tx_originator.busy_wait_for_debug_log(expected_msgs=[ignoring_msg.encode()]):
tx_originator.sendrawtransaction(hexstring=sibling2.serialize_with_witness().hex(), maxfeerate=0.1)
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.
thanks for the review, I’ve pushed an update and made the function more generic
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)
d1a2e27: any reason for removing this line? (comment mentions submitting parent to mempool)
this isn't necessary anymore, i'm using 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.
Done
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.
<details> <summary>Diff</summary>
diff --git a/test/functional/mempool_accept_wtxid.py b/test/functional/mempool_accept_wtxid.py
index a49a886d3a..f895772b08 100755
--- a/test/functional/mempool_accept_wtxid.py
+++ b/test/functional/mempool_accept_wtxid.py
@@ -11,7 +11,7 @@ from test_framework.messages import (
COIN,
)
from test_framework.p2p import P2PTxInvStore
-from test_framework.script_util import valid_witness_malleate_tx
+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,
@@ -36,7 +36,9 @@ class MempoolWtxidTest(BitcoinTestFramework):
self.log.info("Submit parent with multiple script branches to mempool")
parent_amount = int(9.99998 * COIN)
child_amount = int(9.99996 * COIN)
- _, child_one, child_two = valid_witness_malleate_tx(mini_wallet, node, parent_amount, child_amount)
+ parent = mini_wallet.create_self_transfer(fee_rate=0)["tx"]
+ parent, child_one, child_two = build_malleated_tx_package(parent, parent_amount, child_amount)
+ mini_wallet.sendrawtransaction(from_node=node, tx_hex=parent.serialize().hex())
self.generate(node, 1)
diff --git a/test/functional/test_framework/script_util.py b/test/functional/test_framework/script_util.py
index f3b3c803ed..862e30c088 100755
--- a/test/functional/test_framework/script_util.py
+++ b/test/functional/test_framework/script_util.py
@@ -6,6 +6,10 @@
import unittest
from copy import deepcopy
+from test_framework.util import (
+ assert_greater_than_or_equal,
+ assert_equal,
+)
from test_framework.messages import (
COutPoint,
@@ -150,19 +154,22 @@ def check_script(script):
assert False
-def valid_witness_malleate_tx(mini_wallet, node, parent_amount, child_amount):
+def build_malleated_tx_package(parent: CTransaction, parent_amount, child_amount, fee=1000):
"""
- Creates a valid witness malleation transaction:
+ Creates a malleated transaction package with valid witness:
- Parent transaction with a script supporting 2 branches
- - 2 child transactions with the same txid but different wtxids
+ - 2 child transactions with the same txid but different wtxids because of different witnesses
"""
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])
- # Create parent transaction with a script supporting 2 branches
- parent = mini_wallet.send_to(from_node=node, scriptPubKey=script_pubkey, amount=parent_amount, fee=1000)
+ # Append to the transaction the vout containing the script supporting 2 spending conditions
+ assert_equal(len(parent.vout), 1)
+ assert_greater_than_or_equal(parent.vout[0].nValue, parent_amount + fee)
+ parent.vout[0].nValue -= (parent_amount + fee)
+ parent.vout.append(CTxOut(parent_amount, script_pubkey))
# Create 2 valid children that differ only in witness data.
# 1. Create a new transaction with witness solving first branch
@@ -171,13 +178,15 @@ def valid_witness_malleate_tx(mini_wallet, node, parent_amount, child_amount):
child_script_pubkey = CScript([OP_0, child_witness_program])
child_one = CTransaction()
- child_one.vin.append(CTxIn(COutPoint(int(parent['txid'], 16), parent['sent_vout']), b""))
+ 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
</details>
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.
<details open> <summary>Slightly altered diff with named arguments and allowing multiple tx outputs</summary>
diff --git a/test/functional/mempool_accept_wtxid.py b/test/functional/mempool_accept_wtxid.py
index a49a886d3a..179ddb0d38 100755
--- a/test/functional/mempool_accept_wtxid.py
+++ b/test/functional/mempool_accept_wtxid.py
@@ -11,7 +11,7 @@ from test_framework.messages import (
COIN,
)
from test_framework.p2p import P2PTxInvStore
-from test_framework.script_util import valid_witness_malleate_tx
+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,
@@ -34,9 +34,10 @@ class MempoolWtxidTest(BitcoinTestFramework):
assert_equal(node.getmempoolinfo()['size'], 0)
self.log.info("Submit parent with multiple script branches to mempool")
- parent_amount = int(9.99998 * COIN)
- child_amount = int(9.99996 * COIN)
- _, child_one, child_two = valid_witness_malleate_tx(mini_wallet, node, parent_amount, child_amount)
+ parent, child_one, child_two = build_malleated_tx_package(parent=mini_wallet.create_self_transfer()["tx"],
+ rebalance_parent_output_amount=int(9.99998 * COIN),
+ malleated_child_amount=int(9.99996 * COIN))
+ mini_wallet.sendrawtransaction(from_node=node, tx_hex=parent.serialize().hex())
self.generate(node, 1)
diff --git a/test/functional/test_framework/script_util.py b/test/functional/test_framework/script_util.py
index f3b3c803ed..5ab97d6753 100755
--- a/test/functional/test_framework/script_util.py
+++ b/test/functional/test_framework/script_util.py
@@ -6,6 +6,9 @@
import unittest
from copy import deepcopy
+from test_framework.util import (
+ assert_greater_than_or_equal,
+)
from test_framework.messages import (
COutPoint,
@@ -150,19 +153,23 @@ def check_script(script):
assert False
-def valid_witness_malleate_tx(mini_wallet, node, parent_amount, child_amount):
+def build_malleated_tx_package(*, parent: CTransaction, rebalance_parent_output_amount, malleated_child_amount):
"""
- Creates a valid witness malleation transaction:
- - 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
"""
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])
- # Create parent transaction with a script supporting 2 branches
- parent = mini_wallet.send_to(from_node=node, scriptPubKey=script_pubkey, amount=parent_amount, fee=1000)
+ # Append to the transaction the vout containing the script supporting two 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))
# Create 2 valid children that differ only in witness data.
# 1. Create a new transaction with witness solving first branch
@@ -171,13 +178,15 @@ def valid_witness_malleate_tx(mini_wallet, node, parent_amount, child_amount):
child_script_pubkey = CScript([OP_0, child_witness_program])
child_one = CTransaction()
- child_one.vin.append(CTxIn(COutPoint(int(parent['txid'], 16), parent['sent_vout']), b""))
- child_one.vout.append(CTxOut(child_amount, child_script_pubkey))
+ child_one.vin.append(CTxIn(COutPoint(int(parent.txid_hex, 16), len(parent.vout) - 1), b""))
+ child_one.vout.append(CTxOut(malleated_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
</details>
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.
Thanks for the diff—I’ve adopted it and pushed an update with it. If other reviewers agree, I’m open to reverting the changes to the class implementation and keeping the scope of this PR to small refactors in test_mempool_accept_wtxid, but for now I’m keeping it as is.
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)
Nit: It would be nice to calculate these amounts based on the wallet balance or the utxo being spent by the wallet instead of hardcoding that relies on some MiniWallet assumptions.
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.
I updated the PR description. It was originally focused on reafctoring 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
parent, child_one, child_two = build_malleated_tx_package(
parent=mini_wallet.create_self_transfer()["tx"],
rebalance_parent_output_amount=int(9.99998 * COIN),
child_amount=int(9.99996 * COIN)
)
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.
Added the explanation in the Doc String https://github.com/bitcoin/bitcoin/pull/33067/commits/4538bb0b11862a14f870185a2ef71b3b22774c20
ACK 6cacc1a.
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>
Nit: This commit (3f5211cba8e73e8eb03781e6ec32ba9c4a263782 test: remove child_one/child_two (w)txid variables) could have been merged with the first one as I feel it's not critical enough to be a separate commit.
Nits if you end up touching the PR for some other reason:
diff --git a/test/functional/test_framework/script_util.py b/test/functional/test_framework/script_util.py
index 812d9bbd0a..f4d7d371e5 100755
--- a/test/functional/test_framework/script_util.py
+++ b/test/functional/test_framework/script_util.py
@@ -169,7 +169,7 @@ def check_script(script):
def build_malleated_tx_package(*, parent: CTransaction, rebalance_parent_output_amount, child_amount):
"""
- Returns a transaction package with valid witness:
+ Returns a transaction package with valid witnesses:
- 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
@@ -189,7 +189,6 @@ def build_malleated_tx_package(*, parent: CTransaction, rebalance_parent_output_
last_output.nValue -= rebalance_parent_output_amount
parent.vout.append(CTxOut(rebalance_parent_output_amount, script_pubkey))
-
# 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])
@@ -201,6 +200,7 @@ def build_malleated_tx_package(*, parent: CTransaction, rebalance_parent_output_
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]
(END)
reACK 3f5211c.
reACK 3f5211c
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.
diff --git a/test/functional/mempool_accept_wtxid.py b/test/functional/mempool_accept_wtxid.py
index a9edea6d64..c44b6b0544 100755
--- a/test/functional/mempool_accept_wtxid.py
+++ b/test/functional/mempool_accept_wtxid.py
@@ -32,8 +32,8 @@ class MempoolWtxidTest(BitcoinTestFramework):
self.log.info("Submit parent with multiple script branches to mempool")
parent = mini_wallet.create_self_transfer()["tx"]
- parent_amount = parent.vout[0].nValue - 10000
- child_amount = parent_amount - 10000
+ parent_amount = int(parent.vout[len(parent.vout) - 1].nValue / 2)
+ child_amount = parent_amount - 10000 # less fees
parent, child_one, child_two = build_malleated_tx_package(
parent=parent,
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.
review ACK 3f5211cba8e73e8eb03781e6ec32ba9c4a263782 👥
<details><summary>Show signature</summary>
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 3f5211cba8e73e8eb03781e6ec32ba9c4a263782 👥
tvA7x2kTq199J33btf5mwMyybHYoRCli3U0kXP1KtdMyWOqPgsneefDkC6Wp0GBoxTZX5Hhn+rptBr1JuLSGCg==
</details>