[tests] Refactor importmulti tests #14886

jnewbery · 2018-12-06T15:46:47Z

#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 · 2018-12-06T15:54:24Z

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 · 2018-12-06T17:24:14Z

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

meshcollider · 2018-12-06T18:00:24Z

Concept ACK, will review very soon

jnewbery · 2018-12-06T18:30:13Z

Added a new commit updating the docstring.

practicalswift · 2018-12-07T20:14:50Z

test/functional/wallet_importmulti.py

+        addr_info = self.nodes[1].getaddressinfo(address)
+        for key, value in kwargs.items():
+            if addr_info[key] != value:
+                raise(AssertionError("key {} value {} did not match expected value {}".format(key, addr_info[key], value)))


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

fixed. Thanks!

practicalswift · 2018-12-07T20:15:40Z

test/functional/wallet_importmulti.py

+        Returns a named tuple of privkeys, pubkeys and all address and scripts."""
+        addrs = []
+        pubkeys = []
+        for i in range(3):


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

jnewbery · 2018-12-07T20:30:06Z

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

meshcollider

utACK 12e59a6, 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.

meshcollider · 2018-12-09T01:06:30Z

test/functional/wallet_importmulti.py

        self.log.info("Should import an address")
-        address = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
+        key = self.get_key()
+        key = self.get_key()


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

oops. Fixed intermediate commit.

meshcollider · 2018-12-09T01:08:26Z

test/functional/wallet_importmulti.py

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


yes, removed

meshcollider · 2018-12-09T02:23:07Z

test/functional/wallet_importmulti.py

meshcollider · 2018-12-09T02:29:01Z

test/functional/wallet_importmulti.py

This should also test for solvability

meshcollider · 2018-12-09T02:38:13Z

test/functional/wallet_importmulti.py

This should test watchonly=True

meshcollider · 2018-12-09T02:45:27Z

test/functional/wallet_importmulti.py

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

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

meshcollider · 2018-12-09T02:47:10Z

test/functional/wallet_importmulti.py

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

meshcollider · 2018-12-09T02:47:18Z

test/functional/wallet_importmulti.py

meshcollider · 2018-12-09T02:50:19Z

test/functional/wallet_importmulti.py

test !ismine here?

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

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

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

Adds a new test_address method for testing the imported addresses.

Adds a docstring describing the new importmulti test.

jnewbery · 2018-12-10T20:40:51Z

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.

ryanofsky

utACK ee3b21d. 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.

ryanofsky · 2018-12-11T16:28:06Z

test/functional/wallet_importmulti.py

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


In commit "[tests] add get_key function to wallet_importmulti.py" (7c99614)

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.

ryanofsky · 2018-12-11T16:30:40Z

test/functional/wallet_importmulti.py

+        pkh = hash160(hex_str_to_bytes(pubkey))
+        return Key(self.nodes[0].dumpprivkey(addr),
+                   pubkey,
+                   CScript([OP_DUP, OP_HASH160, pkh, OP_EQUALVERIFY, OP_CHECKSIG]).hex(),  # p2pkh


In commit "[tests] add get_key function to wallet_importmulti.py" (7c99614)

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

ryanofsky · 2018-12-11T16:44:11Z

test/functional/wallet_importmulti.py

-        sig_address_1 = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
-        sig_address_2 = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
-        sig_address_3 = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
-        multi_sig_script = self.nodes[0].createmultisig(2, [sig_address_1['pubkey'], sig_address_2['pubkey'], sig_address_3['pubkey']])


In commit "[tests] add get_multisig function to wallet_importmulti.py" (08a4a0f)

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.

ryanofsky · 2018-12-11T16:45:52Z

test/functional/wallet_importmulti.py

                   key_to_p2sh_p2wpkh(pubkey))  # p2sh-p2wpkh addr

+    def get_multisig(self):
+        """Generate a fresh multisig on node0


In commit "[tests] add get_multisig function to wallet_importmulti.py" (08a4a0f)

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

ryanofsky · 2018-12-11T16:52:36Z

test/functional/wallet_importmulti.py

                        CScript([OP_HASH160, witness_script, OP_EQUAL]).hex(),  # p2sh-p2wsh
                        script_to_p2sh_p2wsh(script_code))  # p2sh-p2wsh addr

+    def test_importmulti(self, req, success, error_code=None, error_message=None):


In commit "[tests] add test_importmulti method to wallet_import.py" (fd3a02c)

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.

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

ryanofsky · 2018-12-11T16:58:05Z

test/functional/wallet_importmulti.py

            assert_equal(result[0]['error']['code'], error_code)
            assert_equal(result[0]['error']['message'], error_message)

+    def test_address(self, address, **kwargs):


In commit "[tests] add test_address method to wallet_import.py" (fbdba40)

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

meshcollider · 2018-12-11T17:23:33Z

re-utACK ee3b21d

Only change since my last review was addressing my comments above

ee3b21d [tests] Add docstring for wallet_importmulti.py (John Newbery) fbdba40 [tests] add test_address method to wallet_import.py (John Newbery) fd3a02c [tests] add test_importmulti method to wallet_import.py (John Newbery) 08a4a0f [tests] add get_multisig function to wallet_importmulti.py (John Newbery) 7c99614 [tests] add get_key function to wallet_importmulti.py (John Newbery) e5a8ea8 [tests] tidy up imports in wallet_importmulti.py (John Newbery) cb41ade [tests] fix flake8 warnings in wallet_importmulti.py (John Newbery) Pull request description: bitcoin#14565 needs test coverage. This PR refactors wallet_importmulti.py to the following pattern: 1. Add `get_key()` and `get_multisig()` methods, which generate keys on node0 and return the priv/pubkeys and all scriptPubKey and address variants. 2. Add `test_importmulti()` method, which takes an importmulti request, sends it to node1 and tests against success and error codes/messages. 3. 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 bitcoin#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. Tree-SHA512: e0ae9d3436f0b4eec4f6b9bdc0f02aef49c5a16bbac319fd47b2cfcaf01d01780d7b296280e8760686a57fac63275eec09e2959d8aaeceae1b406d8eff768435

Sjors · 2018-12-12T08:42:53Z

Post merge ACK

jnewbery · 2018-12-13T17:47:07Z

Thanks all for the review.

@ryanofsky - your review comments are fixed here: #14952

@ryanofsky

2d5f1ea [tests] move wallet util functions to wallet_util.py (John Newbery) 6be64ef [tests] tidy up wallet_importmulti.py (John Newbery) Pull request description: Cherry picks un-merged commits from #14952, which "fixes review comments from @ryanofsky here: #14886 (review)" Tree-SHA512: 5f389196b0140d013a533d500f1812786a3a5cfb65980e13eaeacc459fddb55f43d05da3ab5e7cc8c997f26c0b667eed081ab6de2d125e631c70a7dd4c06e350

Summary: Partial backport of Core [[bitcoin/bitcoin#14886 | PR14886]] bitcoin/bitcoin@e5a8ea8 The parts excluded are only used in the witness-related code. Test Plan: ./test_runner.py wallet_importmulti Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6081

Summary: Adds a new get_key function which generates a new key and returns the public key, private key and all script and address types. Partial backport of Core [[bitcoin/bitcoin#14886 | PR14886]] bitcoin/bitcoin@7c99614 Depends on D6051 and D6081 Test Plan: test_runner.py wallet_importmulti Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6083

Summary: Adds a new get_multisig function which generates a new multisig and returns the public keys, private keys and all script and address types. Backport of Core [[bitcoin/bitcoin#14886 | PR14886]] bitcoin/bitcoin@08a4a0f Depends on D6083 Test Plan: ./test_runner.py wallet_importmulti Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6086

Summary: Adds a new test_importmulti method for testing the importmulti RPC method. Partial backport of Core [[bitcoin/bitcoin#14886 | PR14886]] bitcoin/bitcoin@fd3a02c Depends on D6086 Test Plan: ./test_runner.py wallet_importmulti Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6087

Summary: Adds a new test_address method for testing the imported addresses. Partial backport of Core [[bitcoin/bitcoin#14886 | PR14886]] bitcoin/bitcoin@fbdba40 Depends on D6087 Test Plan: ./test_runner.py wallet_importmulti Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6088

Summary: Partial backport of Core [[bitcoin/bitcoin#14886 | PR14886]] bitcoin/bitcoin@e5a8ea8 The parts excluded are only used in the witness-related code. Test Plan: ./test_runner.py wallet_importmulti Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6081

jnewbery mentioned this pull request Dec 6, 2018

Overhaul importmulti logic #14565

Merged

jnewbery added 2 commits December 6, 2018 12:18

[tests] fix flake8 warnings in wallet_importmulti.py

cb41ade

[tests] tidy up imports in wallet_importmulti.py

e5a8ea8

jnewbery force-pushed the importmulti_tests branch from 0fc6f36 to a1b4482 Compare December 6, 2018 17:23

fanquake added the Tests label Dec 6, 2018

practicalswift reviewed Dec 7, 2018

View reviewed changes

jnewbery force-pushed the importmulti_tests branch from 6c76af0 to 12e59a6 Compare December 7, 2018 20:29

meshcollider reviewed Dec 9, 2018

View reviewed changes

jnewbery added 5 commits December 10, 2018 15:17

[tests] add get_key function to wallet_importmulti.py

7c99614

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

[tests] add get_multisig function to wallet_importmulti.py

08a4a0f

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

[tests] add test_importmulti method to wallet_import.py

fd3a02c

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

[tests] add test_address method to wallet_import.py

fbdba40

Adds a new test_address method for testing the imported addresses.

[tests] Add docstring for wallet_importmulti.py

ee3b21d

Adds a docstring describing the new importmulti test.

jnewbery force-pushed the importmulti_tests branch from 12e59a6 to ee3b21d Compare December 10, 2018 20:37

ryanofsky approved these changes Dec 11, 2018

View reviewed changes

maflcko merged commit ee3b21d into bitcoin:master Dec 11, 2018

Sjors mentioned this pull request Dec 12, 2018

Travis: enforce Python 3.4 support through linter #14884

Merged

jnewbery mentioned this pull request Dec 13, 2018

[do not merge] [tests] tidy up wallet_importmulti.py #14952

Closed

jnewbery deleted the importmulti_tests branch December 13, 2018 17:47

amitiuttarwar mentioned this pull request Jan 5, 2019

[tests] tidy up wallet_importmulti.py #15108

Merged

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

[tests] Refactor importmulti tests #14886

[tests] Refactor importmulti tests #14886

Uh oh!

Conversation

jnewbery commented Dec 6, 2018

Uh oh!

DrahtBot commented Dec 6, 2018

Conflicts

Uh oh!

jnewbery commented Dec 6, 2018

Uh oh!

meshcollider commented Dec 6, 2018

Uh oh!

jnewbery commented Dec 6, 2018

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!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Dec 7, 2018

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider Dec 9, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Dec 10, 2018

Uh oh!

ryanofsky 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!

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 Dec 11, 2018

Uh oh!

Sjors commented Dec 12, 2018

Uh oh!

jnewbery commented Dec 13, 2018

meshcollider Dec 9, 2018 •

edited

Loading