[test] fundrawtransaction: lock watch-only shared address #12265

kallewoof · 2018-01-25T02:04:40Z

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).

promag

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.

kallewoof · 2018-01-25T10:08:49Z

@promag That's a good point. I moved parts of it into util as find_vout_for_address.

promag · 2018-01-25T10:16:21Z

test/functional/rpc_fundrawtransaction.py

@@ -472,6 +488,8 @@ def run_test(self):
        connect_nodes_bi(self.nodes,1,2)
        connect_nodes_bi(self.nodes,0,2)
        connect_nodes_bi(self.nodes,0,3)
+        # Again lock the watchonly UTXO or nodes[0] may spend it
+        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).

promag

ACK.

Just want to point that if *rawtransaction were used instead of sendtoaddress then the vout would be known.

promag · 2018-01-25T10:38:41Z

test/functional/test_framework/util.py

+    """
+    tx = node.getrawtransaction(txid, True)
+    for i in range(len(tx["vout"])):
+        o = tx["vout"][i]


Nit, inline o below (only used once).

Good point. Fixed.

laanwj · 2018-02-15T16:37:40Z

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.

kallewoof · 2018-02-16T01:36:16Z

@laanwj Oops, didn't realize you needed an empty line. Fixed.

maflcko · 2018-02-16T04:10:33Z

test/functional/rpc_fundrawtransaction.py

+
+        # Lock UTXO so nodes[0] doesn't accidentally spend it
+        watchonly_vout = find_vout_for_address(self.nodes[0], watchonly_txid, watchonly_address)
+        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.

jnewbery

Tested ACK 73099805915ad11b264bc22040015eb3ac137b2b with one nit.

Commits can be squashed IMO.

jnewbery · 2018-04-04T18:05:19Z

test/functional/rpc_fundrawtransaction.py

@@ -5,7 +5,17 @@
 """Test the fundrawtransaction RPC."""

 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import *
+from decimal import Decimal


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.

kallewoof · 2018-04-06T04:34:44Z

@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.

promag · 2018-04-06T11:54:21Z

utACK 891beb0.

jnewbery

Tested ACK 891beb0

jnewbery · 2018-04-09T19:48:43Z

test/functional/test_framework/util.py

+    given address. Raises runtime error exception if not found.
+    """
+    tx = node.getrawtransaction(txid, True)
+    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

meshcollider · 2018-05-09T06:56:27Z

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

fanquake added the Tests label Jan 25, 2018

promag reviewed Jan 25, 2018

View reviewed changes

kallewoof force-pushed the test-fundrawtransaction-lockunspent branch from e3c9a2b to 3e5644b Compare January 25, 2018 10:08

promag reviewed Jan 25, 2018

View reviewed changes

kallewoof force-pushed the test-fundrawtransaction-lockunspent branch from 3e5644b to 167e6a5 Compare January 25, 2018 10:21

promag reviewed Jan 25, 2018

View reviewed changes

kallewoof force-pushed the test-fundrawtransaction-lockunspent branch from 167e6a5 to f3ebd07 Compare January 25, 2018 23:10

kallewoof force-pushed the test-fundrawtransaction-lockunspent branch from f3ebd07 to 8477b83 Compare February 16, 2018 01:36

maflcko reviewed Feb 16, 2018

View reviewed changes

kallewoof force-pushed the test-fundrawtransaction-lockunspent branch from 8477b83 to 7309980 Compare February 16, 2018 04:21

kallewoof mentioned this pull request Apr 3, 2018

[wallet] Use destination groups instead of coins in coin select #12257

Merged

jnewbery reviewed Apr 4, 2018

View reviewed changes

kallewoof force-pushed the test-fundrawtransaction-lockunspent branch from 7309980 to 891beb0 Compare April 6, 2018 04:34

jnewbery reviewed Apr 9, 2018

View reviewed changes

laanwj merged commit 891beb0 into bitcoin:master May 9, 2018

kallewoof deleted the test-fundrawtransaction-lockunspent branch May 9, 2018 08:52

Bushstar mentioned this pull request May 10, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#328

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[test] fundrawtransaction: lock watch-only shared address #12265

[test] fundrawtransaction: lock watch-only shared address #12265

Uh oh!

Conversation

kallewoof commented Jan 25, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Jan 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Feb 15, 2018

Uh oh!

kallewoof commented Feb 16, 2018

Uh oh!

maflcko Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Apr 6, 2018

Uh oh!

promag commented Apr 6, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider commented May 9, 2018

Uh oh!

Uh oh!

maflcko Feb 16, 2018 •

edited

Loading