[tests] Refactor importmulti tests

jnewbery commented at 3:46 pm on December 6, 2018: member

#14565 needs test coverage. This PR refactors wallet_importmulti.py to the following pattern:

Add get_key() and get_multisig() methods, which generate keys on node0 and return the priv/pubkeys and all scriptPubKey and address variants.
Add test_importmulti() method, which takes an importmulti request, sends it to node1 and tests against success and error codes/messages.
Add test_address() method, which takes an address, sends it as a getaddressinfo request to node1 and tests the values returned.

This does not add any specific testing for #14565, but makes it very straightforward to add that testing: test_importmulti() can be easily updated to test for returned warnings, and test_address() can be called multiple times against the different address variants for a singlesig/multisig.

DrahtBot commented at 3:54 pm on December 6, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14565 (Overhaul importmulti logic by sipa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jnewbery added this to the "Blockers" column in a project

[tests] fix flake8 warnings in wallet_importmulti.py cb41ade6b1

[tests] tidy up imports in wallet_importmulti.py e5a8ea8f14

jnewbery force-pushed on Dec 6, 2018

jnewbery commented at 5:24 pm on December 6, 2018: member

I’ve tidied up the intermediate commits and force-pushed. Should be ready for review.

meshcollider commented at 6:00 pm on December 6, 2018: contributor

Concept ACK, will review very soon

jnewbery commented at 6:30 pm on December 6, 2018: member

Added a new commit updating the docstring.

fanquake added the label Tests on Dec 6, 2018

in test/functional/wallet_importmulti.py:135 in 6c76af02e8 outdated

131+    def test_address(self, address, **kwargs):
132+        """Get address info for `address` and test whether the returned values are as expected."""
133+        addr_info = self.nodes[1].getaddressinfo(address)
134+        for key, value in kwargs.items():
135+            if addr_info[key] != value:
136+                raise(AssertionError("key {} value {} did not match expected value {}".format(key, addr_info[key], value)))

practicalswift commented at 8:14 pm on December 7, 2018:

Nit: raise AssertionError(…) is more idiomatic and consistent with the rest of the code base. Drop the unnecessary parens :-)

jnewbery commented at 8:29 pm on December 7, 2018:

fixed. Thanks!

in test/functional/wallet_importmulti.py:106 in 6c76af02e8 outdated

102+        """Generate a fresh multisig on node0
103+
104+        Returns a named tuple of privkeys, pubkeys and all address and scripts."""
105+        addrs = []
106+        pubkeys = []
107+        for i in range(3):

practicalswift commented at 8:15 pm on December 7, 2018:

Nit: Use _ instead of i to show that the variable is intentionally unused.

jnewbery commented at 8:29 pm on December 7, 2018:

fixed

jnewbery force-pushed on Dec 7, 2018

jnewbery commented at 8:30 pm on December 7, 2018: member

@practicalswift nits fixed and redundant code removed: https://github.com/bitcoin/bitcoin/compare/6c76af02e8188d0dc66c315e4ecd80308e2fa0a1..12e59a60019732fa228872848121cca4431e7513

in test/functional/wallet_importmulti.py:97 in b676025e5f outdated

92@@ -55,20 +93,22 @@ def run_test(self):
93 
94         # Bitcoin Address (implicit non-internal)
95         self.log.info("Should import an address")
96-        address = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
97+        key = self.get_key()
98+        key = self.get_key()

meshcollider commented at 1:06 am on December 9, 2018:

Why do you call this twice? EDIT: don’t worry, I see the second one is removed in a later commit.

jnewbery commented at 8:12 pm on December 10, 2018:

oops. Fixed intermediate commit.

in test/functional/wallet_importmulti.py:144 in b676025e5f outdated

143 
144         # ScriptPubKey + internal + label
145         self.log.info("Should not allow a label to be specified when internal is true")
146-        address = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
147+        key = self.get_key()
148+        address = key.p2pkh_addr

meshcollider commented at 1:08 am on December 9, 2018:

Unused?

jnewbery commented at 8:17 pm on December 10, 2018:

yes, removed

in test/functional/wallet_importmulti.py:13 in 12e59a6001 outdated

 9+addresses on node1 and then testing the address info for the different address
10+variants.
11+
12+- `get_key()` and `get_multisig()` are called to generate keys on node0 and
13+  return the privkeys, pubkeys and all variants of scriptPubKey and address.
14+- `test_importmulti() is called to send an importmulti call to node1, test

meshcollider commented at 2:23 am on December 9, 2018:

missing `

jnewbery commented at 8:13 pm on December 10, 2018:

fixed

in test/functional/wallet_importmulti.py:231 in 7e5a03aeb0 outdated

214@@ -209,10 +215,10 @@ def run_test(self):
215                                "pubkeys": [key.pubkey],
216                                "internal": False},
217                               True)
218-        address_assert = self.nodes[1].getaddressinfo(address)
219-        assert_equal(address_assert['iswatchonly'], True)
220-        assert_equal(address_assert['ismine'], False)
221-        assert_equal(address_assert['timestamp'], timestamp)
222+        self.test_address(address,

meshcollider commented at 2:29 am on December 9, 2018:

This should also test for solvability

in test/functional/wallet_importmulti.py:543 in 7e5a03aeb0 outdated

523                                "timestamp": "now"},
524                               True)
525-        address_assert = self.nodes[1].getaddressinfo(multisig.p2sh_addr)
526-        assert_equal(address_assert['solvable'], False)
527+        self.test_address(multisig.p2sh_addr,
528+                          solvable=False)

meshcollider commented at 2:38 am on December 9, 2018:

This should test watchonly=True

in test/functional/wallet_importmulti.py:258 in 7e5a03aeb0 outdated

307@@ -304,10 +308,9 @@ def run_test(self):
308                               False,
309                               error_code=-8,
310                               error_message='Internal must be set to true for nonstandard scriptPubKey imports.')
311-        address_assert = self.nodes[1].getaddressinfo(address)
312-        assert_equal(address_assert['iswatchonly'], False)
313-        assert_equal(address_assert['ismine'], False)
314-        assert_equal('timestamp' in address_assert, False)

meshcollider commented at 2:45 am on December 9, 2018:

n.b. we’ve lost this test now, but that seems fine

jnewbery commented at 8:36 pm on December 10, 2018:

I didn’t think this test was important, so I removed it, but I’ve now readded it.

in test/functional/wallet_importmulti.py:362 in 7e5a03aeb0 outdated

342@@ -340,8 +343,7 @@ def run_test(self):
343                                "timestamp": "now",
344                                "redeemscript": multisig.redeem_script},
345                               True)
346-        address_assert = self.nodes[1].getaddressinfo(multisig.p2sh_addr)
347-        assert_equal(address_assert['timestamp'], timestamp)
348+        self.test_address(multisig.p2sh_addr, timestamp=timestamp)

meshcollider commented at 2:47 am on December 9, 2018:

IMO it wouldn’t hurt to test for solvability here too to ensure the same output as listunspent below

in test/functional/wallet_importmulti.py:382 in 7e5a03aeb0 outdated

361@@ -360,8 +362,8 @@ def run_test(self):
362                                "redeemscript": multisig.redeem_script,
363                                "keys": multisig.privkeys[0:2]},
364                               True)
365-        address_assert = self.nodes[1].getaddressinfo(multisig.p2sh_addr)
366-        assert_equal(address_assert['timestamp'], timestamp)
367+        self.test_address(multisig.p2sh_addr,
368+                          timestamp=timestamp)

meshcollider commented at 2:47 am on December 9, 2018:

Same here

in test/functional/wallet_importmulti.py:601 in 7e5a03aeb0 outdated

576@@ -579,8 +577,8 @@ def run_test(self):
577                                "redeemscript": multisig.p2wsh_script,
578                                "witnessscript": multisig.redeem_script},
579                               True)
580-        address_assert = self.nodes[1].getaddressinfo(multisig.p2sh_addr)
581-        assert_equal(address_assert['solvable'], True)
582+        self.test_address(address,
583+                          solvable=True)

meshcollider commented at 2:50 am on December 9, 2018:

test !ismine here?

meshcollider commented at 2:51 am on December 9, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/14886/commits/12e59a60019732fa228872848121cca4431e7513, very nice, I’ve left a couple of minor comments inline but I assume you might want to keep this as a refactor rather than adding any extra testing here, so feel free to ignore the last few comments.

[tests] add get_key function to wallet_importmulti.py

Adds a new get_key function which generates
a new key and returns the public key,
private key and all script and address types.

7c99614b40

[tests] add get_multisig function to wallet_importmulti.py

Adds a new get_multisig function which generates
a new multisig and returns the public keys,
private keys and all script and address types.

08a4a0f70f

[tests] add test_importmulti method to wallet_import.py

Adds a new test_importmulti method for testing the
importmulti RPC method.

fd3a02c381

[tests] add test_address method to wallet_import.py

Adds a new test_address method for testing the
imported addresses.

fbdba40594

[tests] Add docstring for wallet_importmulti.py

Adds a docstring describing the new importmulti test.

ee3b21dccb

jnewbery force-pushed on Dec 10, 2018

jnewbery commented at 8:40 pm on December 10, 2018: member

Thanks @MeshCollider . I’ve addressed your review comments and pushed: https://github.com/bitcoin/bitcoin/compare/12e59a60019732fa228872848121cca4431e7513..ee3b21dccbeb0f9d4e99de869dbfaf625c159c7f

I’ve left a couple of minor comments inline but I assume you might want to keep this as a refactor rather than adding any extra testing here, so feel free to ignore the last few comments.

Yes, the intent was for this to be a straight refactor. I think more comprehensive testing can be added as part of #14565. In fact, I’ve started doing that here: https://github.com/jnewbery/bitcoin/tree/pr14565.tests. We should go through that commit and make sure that it’s comprehensively testing all return values.

in test/functional/wallet_importmulti.py:141 in 7c99614b40 outdated

140         assert_equal(address_assert['ischange'], True)
141 
142         # ScriptPubKey + internal + label
143         self.log.info("Should not allow a label to be specified when internal is true")
144-        address = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
145+        key = self.get_key()

ryanofsky commented at 4:28 pm on December 11, 2018:

In commit “[tests] add get_key function to wallet_importmulti.py” (7c99614b40301daba5e526c6e3ab281965e869c6)

You seem to be dropping the address = assignment in this case, while keeping it the other cases. Would be good to fix the inconsistency, I think ideally by dropping the address variable everywhere and replacing it with key.p2pkh_addr.

in test/functional/wallet_importmulti.py:65 in 7c99614b40 outdated

60+        addr = self.nodes[0].getnewaddress()
61+        pubkey = self.nodes[0].getaddressinfo(addr)['pubkey']
62+        pkh = hash160(hex_str_to_bytes(pubkey))
63+        return Key(self.nodes[0].dumpprivkey(addr),
64+                   pubkey,
65+                   CScript([OP_DUP, OP_HASH160, pkh, OP_EQUALVERIFY, OP_CHECKSIG]).hex(),  # p2pkh

ryanofsky commented at 4:30 pm on December 11, 2018:

In commit “[tests] add get_key function to wallet_importmulti.py” (7c99614b40301daba5e526c6e3ab281965e869c6)

Would probably be better to use named arguments instead of having these little # p2pkh per argument comments.

in test/functional/wallet_importmulti.py:308 in 08a4a0f70f outdated

339@@ -302,102 +340,90 @@ def run_test(self):
340         assert_equal('timestamp' in address_assert, False)
341 
342         # P2SH address
343-        sig_address_1 = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
344-        sig_address_2 = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
345-        sig_address_3 = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
346-        multi_sig_script = self.nodes[0].createmultisig(2, [sig_address_1['pubkey'], sig_address_2['pubkey'], sig_address_3['pubkey']])

ryanofsky commented at 4:44 pm on December 11, 2018:

In commit “[tests] add get_multisig function to wallet_importmulti.py” (08a4a0f70f545e114bc35dc76056282cb04bf013)

Note: We lose a bit of test coverage here for createmultisig method here, but this should be ok given the dedicated rpc_createmultisig.py test.

in test/functional/wallet_importmulti.py:90 in 08a4a0f70f outdated

85@@ -70,6 +86,28 @@ def get_key(self):
86                    CScript([OP_0, pkh]).hex(),  # p2sh-p2wpkh redeem script
87                    key_to_p2sh_p2wpkh(pubkey))  # p2sh-p2wpkh addr
88 
89+    def get_multisig(self):
90+        """Generate a fresh multisig on node0

ryanofsky commented at 4:45 pm on December 11, 2018:

In commit “[tests] add get_multisig function to wallet_importmulti.py” (08a4a0f70f545e114bc35dc76056282cb04bf013)

Might be good to say this is a 2 of 3 multisig (or even add num_required/num_keys arguments).

in test/functional/wallet_importmulti.py:111 in fd3a02c381 outdated

107@@ -108,6 +108,14 @@ def get_multisig(self):
108                         CScript([OP_HASH160, witness_script, OP_EQUAL]).hex(),  # p2sh-p2wsh
109                         script_to_p2sh_p2wsh(script_code))  # p2sh-p2wsh addr
110 
111+    def test_importmulti(self, req, success, error_code=None, error_message=None):

ryanofsky commented at 4:52 pm on December 11, 2018:

In commit “[tests] add test_importmulti method to wallet_import.py” (fd3a02c381ee136e54579b5a2e3466c016101515)

Should consider dropping the success argument because it can be implied from the error arguments, and makes test_importmulti calls awkward to write & read with unnamed False/True arguments.

MarcoFalke commented at 6:57 pm on December 11, 2018:

Agree, that at a minimum named arguments should be used at the caller.

in test/functional/wallet_importmulti.py:119 in fbdba40594 outdated

115@@ -116,6 +116,16 @@ def test_importmulti(self, req, success, error_code=None, error_message=None):
116             assert_equal(result[0]['error']['code'], error_code)
117             assert_equal(result[0]['error']['message'], error_message)
118 
119+    def test_address(self, address, **kwargs):

ryanofsky commented at 4:58 pm on December 11, 2018:

In commit “[tests] add test_address method to wallet_import.py” (fbdba40594d549c76aaf15401fe994ac5cfc4f73)

Seems like this same function would also be useful for wallet_import_with_label.py. Maybe move it to a common place.

ryanofsky approved

ryanofsky commented at 5:06 pm on December 11, 2018: member

utACK ee3b21dccbeb0f9d4e99de869dbfaf625c159c7f. This is a test-only change and nice cleanup, and I think it should be merged as is. There are more tests in John’s branch that depend on this, and #14565 will depend on those tests, and there are several other prs based on #14565, so this should be given some priority.

meshcollider commented at 5:23 pm on December 11, 2018: contributor

re-utACK https://github.com/bitcoin/bitcoin/pull/14886/commits/ee3b21dccbeb0f9d4e99de869dbfaf625c159c7f

Only change since my last review was addressing my comments above

MarcoFalke referenced this in commit f65bce858f on Dec 11, 2018

MarcoFalke merged this on Dec 11, 2018

MarcoFalke closed this on Dec 11, 2018

meshcollider removed this from the "Blockers" column in a project

Sjors commented at 8:42 am on December 12, 2018: member

Post merge ACK

jnewbery commented at 5:47 pm on December 13, 2018: member

Thanks all for the review. @ryanofsky - your review comments are fixed here: https://github.com/bitcoin/bitcoin/pull/14952

jnewbery deleted the branch on Dec 13, 2018

MarcoFalke referenced this in commit 1cfbb16b5d on Jan 9, 2019

deadalnix referenced this in commit 73ce44f302 on May 15, 2020

deadalnix referenced this in commit 689070f97e on May 15, 2020

deadalnix referenced this in commit bd95bed5a0 on May 15, 2020

deadalnix referenced this in commit 353446e540 on May 15, 2020

deadalnix referenced this in commit e87bc138f4 on May 15, 2020

ftrader referenced this in commit 469258bf73 on Aug 17, 2020

MarcoFalke locked this on Sep 8, 2021

[tests] Refactor importmulti tests #14886

Conflicts