[test] fundrawtransaction: lock watch-only shared address #12265
Conversation
promag
left a comment
There was a problem hiding this comment.
Concept ACK.
Another approach is to use listunspent --addresses [watchonly_address].
Either way maybe you could extract this "lock unspents by txid and address" to a separate function, it sure makes the test easier to read and might be useful elsewhere.
e3c9a2b to
3e5644b
Compare
|
@promag That's a good point. I moved parts of it into util as |
There was a problem hiding this comment.
Nit, add comment like "because restarting a node looses the locked unspents" (you may find better wording).
3e5644b to
167e6a5
Compare
promag
left a comment
There was a problem hiding this comment.
ACK.
Just want to point that if *rawtransaction were used instead of sendtoaddress then the vout would be known.
There was a problem hiding this comment.
Nit, inline o below (only used once).
There was a problem hiding this comment.
Good point. Fixed.
167e6a5 to
f3ebd07
Compare
|
utACK - can you please change the commit message of f3ebd07 to the format Otherwise tooling that parses the commit messages will make it into one long title. |
f3ebd07 to
8477b83
Compare
|
@laanwj Oops, didn't realize you needed an empty line. Fixed. |
There was a problem hiding this comment.
You can move the assert into find_vout_for_address.
If you don't want to do that, note that python is not type safe, so you don't have to return an integer. None works just fine.
There was a problem hiding this comment.
You mean assert watchonly_vout is not None and not return anything in find_v..?
There was a problem hiding this comment.
Actually nevermind, I'll move the assert instead.
8477b83 to
7309980
Compare
jnewbery
left a comment
There was a problem hiding this comment.
Tested ACK 73099805915ad11b264bc22040015eb3ac137b2b with one nit.
Commits can be squashed IMO.
There was a problem hiding this comment.
nit: please place standard library imports above local project imports
self.nodes[0] creates an address which is watch-only-shared with self.nodes[3]. If nodes[0] spends the associated UTXO during any of its sends later, the watchonly test will fail, as nodes[3] now has insufficient funds. Note that this also adds a new find_vout_for_address function to the test framework.
7309980 to
891beb0
Compare
|
@jnewbery Squashed. I kept them split to make it obvious to reviewers that I am adding a new utility function, not just fixing something in a specific test. |
|
utACK 891beb0. |
| given address. Raises runtime error exception if not found. | ||
| """ | ||
| tx = node.getrawtransaction(txid, True) | ||
| for i in range(len(tx["vout"])): |
There was a problem hiding this comment.
Alternatively:
for i, vout in enumerate(tx["vout"]):
if any([addr == a for a in vout["scriptPubKey"]["addresses"]]):
return iThere was a problem hiding this comment.
Thanks! It doesn't feel strictly necessary but it's marginally better. Will do if this needs a rebase or gets more requests for other changes.
There was a problem hiding this comment.
Agreed, no need to change the branch for just this change
|
utACK 891beb0 |
891beb0 [test] fundrawtransaction: lock watch-only shared address (Karl-Johan Alm) Pull request description: `self.nodes[0]` creates an address which is watch-only-shared with `self.nodes[3]`. If `nodes[0]` spends the associated UTXO during any of its sends later, the watchonly test will fail, as `nodes[3]` now has insufficient funds. I ran into this in #12257 and this commit is in that PR as well, but I figured I'd split it out (and remove from there once/if merged). Tree-SHA512: d04a04b1ecebe82127cccd47c1b3de311bf07f4b51dff80db20ea2f142e1d5c4a85ed6180c5c0b081d550e238c742e119b953f60f487deac5a3f3536e1a8d9fe
Summary: 891beb0 [test] fundrawtransaction: lock watch-only shared address (Karl-Johan Alm) Pull request description: `self.nodes[0]` creates an address which is watch-only-shared with `self.nodes[3]`. If `nodes[0]` spends the associated UTXO during any of its sends later, the watchonly test will fail, as `nodes[3]` now has insufficient funds. I ran into this in #12257 and this commit is in that PR as well, but I figured I'd split it out (and remove from there once/if merged). Tree-SHA512: d04a04b1ecebe82127cccd47c1b3de311bf07f4b51dff80db20ea2f142e1d5c4a85ed6180c5c0b081d550e238c742e119b953f60f487deac5a3f3536e1a8d9fe Backport of Core PR12265 bitcoin/bitcoin#12265 Test Plan: test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4153
…d address 891beb0 [test] fundrawtransaction: lock watch-only shared address (Karl-Johan Alm) Pull request description: `self.nodes[0]` creates an address which is watch-only-shared with `self.nodes[3]`. If `nodes[0]` spends the associated UTXO during any of its sends later, the watchonly test will fail, as `nodes[3]` now has insufficient funds. I ran into this in bitcoin#12257 and this commit is in that PR as well, but I figured I'd split it out (and remove from there once/if merged). Tree-SHA512: d04a04b1ecebe82127cccd47c1b3de311bf07f4b51dff80db20ea2f142e1d5c4a85ed6180c5c0b081d550e238c742e119b953f60f487deac5a3f3536e1a8d9fe
self.nodes[0]creates an address which is watch-only-shared withself.nodes[3]. Ifnodes[0]spends the associated UTXO during any of its sends later, the watchonly test will fail, asnodes[3]now has insufficient funds.I ran into this in #12257 and this commit is in that PR as well, but I figured I'd split it out (and remove from there once/if merged).