[tests] Refactor importmulti tests #14886

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.

jnewbery cross-referenced this on Dec 6, 2018 from issue Overhaul importmulti logic by sipa

DrahtBot commented at 3:54 PM on December 6, 2018: contributor

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: contributor

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

Sjors cross-referenced this on Dec 12, 2018 from issue Travis: enforce Python 3.4 support through linter by Sjors

jnewbery cross-referenced this on Dec 13, 2018 from issue [do not merge] [tests] tidy up wallet_importmulti.py by jnewbery

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

amitiuttarwar cross-referenced this on Jan 5, 2019 from issue [tests] tidy up wallet_importmulti.py by amitiuttarwar

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

bitcoin locked this on Sep 8, 2021