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).
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.
487 | @@ -472,6 +488,8 @@ def run_test(self): 488 | connect_nodes_bi(self.nodes,1,2) 489 | connect_nodes_bi(self.nodes,0,2) 490 | connect_nodes_bi(self.nodes,0,3) 491 | + # Again lock the watchonly UTXO or nodes[0] may spend it 492 | + self.nodes[0].lockunspent(False, [{"txid": watchonly_txid, "vout": watchonly_vout}])
Nit, add comment like "because restarting a node looses the locked unspents" (you may find better wording).
Done!
576 | + given address. 577 | + Returns -1 if not found, or the vout (index) if found. 578 | + """ 579 | + tx = node.getrawtransaction(txid, True) 580 | + for i in range(len(tx["vout"])): 581 | + o = tx["vout"][i]
Nit, inline o below (only used once).
Good point. Fixed.
ACK.
Just want to point that if *rawtransaction were used instead of sendtoaddress then the vout would be known.
utACK - can you please change the commit message of f3ebd07 to the format
<title (one line)>
<empty line>
<description... (possibly multiple lines)>
Otherwise tooling that parses the commit messages will make it into one long title.
66 | @@ -57,6 +67,12 @@ def run_test(self): 67 | watchonly_amount = Decimal(200) 68 | self.nodes[3].importpubkey(watchonly_pubkey, "", True) 69 | watchonly_txid = self.nodes[0].sendtoaddress(watchonly_address, watchonly_amount) 70 | + 71 | + # Lock UTXO so nodes[0] doesn't accidentally spend it 72 | + watchonly_vout = find_vout_for_address(self.nodes[0], watchonly_txid, watchonly_address) 73 | + assert watchonly_vout != -1 # it must exist
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.
You mean assert watchonly_vout is not None and not return anything in find_v..?
Actually nevermind, I'll move the assert instead.
4 | @@ -5,7 +5,17 @@ 5 | """Test the fundrawtransaction RPC.""" 6 | 7 | from test_framework.test_framework import BitcoinTestFramework 8 | -from test_framework.util import * 9 | +from decimal import Decimal
nit: please place standard library imports above local project imports
Tested ACK 73099805915ad11b264bc22040015eb3ac137b2b with one nit.
Commits can be squashed IMO.
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.
utACK 891beb0.
574 | + """ 575 | + Locate the vout index of the given transaction sending to the 576 | + given address. Raises runtime error exception if not found. 577 | + """ 578 | + tx = node.getrawtransaction(txid, True) 579 | + for i in range(len(tx["vout"])):
Alternatively:
for i, vout in enumerate(tx["vout"]):
if any([addr == a for a in vout["scriptPubKey"]["addresses"]]):
return i
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.
Agreed, no need to change the branch for just this change
Tested ACK 891beb0f8a09810b179e39a680b579c2f6516db7
