Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tests] Refactor importmulti tests #14886

Merged
merged 7 commits into from Dec 11, 2018

Conversation

Projects
None yet
8 participants
@jnewbery
Copy link
Member

commented Dec 6, 2018

#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 #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

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

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 jnewbery added this to Blockers in High-priority for review Dec 6, 2018

@jnewbery jnewbery force-pushed the jnewbery:importmulti_tests branch to a1b4482 Dec 6, 2018

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

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

@meshcollider

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

Concept ACK, will review very soon

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

Added a new commit updating the docstring.

@fanquake fanquake added the Tests label Dec 6, 2018

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 7, 2018

Member

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 7, 2018

Author Member

fixed. Thanks!

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

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 7, 2018

Member

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 7, 2018

Author Member

fixed

@jnewbery jnewbery force-pushed the jnewbery:importmulti_tests branch from 6c76af0 Dec 7, 2018

@jnewbery

This comment has been minimized.

@meshcollider
Copy link
Member

left a comment

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.

@@ -55,20 +93,22 @@ def run_test(self):

# Bitcoin Address (implicit non-internal)
self.log.info("Should import an address")
address = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
key = self.get_key()
key = self.get_key()

This comment has been minimized.

Copy link
@meshcollider

meshcollider Dec 9, 2018

Member

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 10, 2018

Author Member

oops. Fixed intermediate commit.

assert_equal(address_assert['iswatchonly'], True)
assert_equal(address_assert['ismine'], False)
assert_equal(address_assert['timestamp'], timestamp)
assert_equal(address_assert['ischange'], True)

# 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()
address = key.p2pkh_addr

This comment has been minimized.

Copy link
@meshcollider

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 10, 2018

Author Member

yes, removed

test/functional/wallet_importmulti.py Outdated

- `get_key()` and `get_multisig()` are called to generate keys on node0 and
return the privkeys, pubkeys and all variants of scriptPubKey and address.
- `test_importmulti() is called to send an importmulti call to node1, test

This comment has been minimized.

Copy link
@meshcollider

meshcollider Dec 9, 2018

Member

missing `

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 10, 2018

Author Member

fixed

assert_equal(address_assert['iswatchonly'], True)
assert_equal(address_assert['ismine'], False)
assert_equal(address_assert['timestamp'], timestamp)
self.test_address(address,

This comment has been minimized.

Copy link
@meshcollider

meshcollider Dec 9, 2018

Member

This should also test for solvability

address_assert = self.nodes[1].getaddressinfo(multisig.p2sh_addr)
assert_equal(address_assert['solvable'], False)
self.test_address(multisig.p2sh_addr,
solvable=False)

This comment has been minimized.

Copy link
@meshcollider

meshcollider Dec 9, 2018

Member

This should test watchonly=True

address_assert = self.nodes[1].getaddressinfo(address)
assert_equal(address_assert['iswatchonly'], False)
assert_equal(address_assert['ismine'], False)
assert_equal('timestamp' in address_assert, False)

This comment has been minimized.

Copy link
@meshcollider

meshcollider Dec 9, 2018

Member

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 10, 2018

Author Member

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

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

This comment has been minimized.

Copy link
@meshcollider

meshcollider Dec 9, 2018

Member

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

address_assert = self.nodes[1].getaddressinfo(multisig.p2sh_addr)
assert_equal(address_assert['timestamp'], timestamp)
self.test_address(multisig.p2sh_addr,
timestamp=timestamp)

This comment has been minimized.

Copy link
@meshcollider

meshcollider Dec 9, 2018

Member

Same here

address_assert = self.nodes[1].getaddressinfo(multisig.p2sh_addr)
assert_equal(address_assert['solvable'], True)
self.test_address(address,
solvable=True)

This comment has been minimized.

Copy link
@meshcollider

meshcollider Dec 9, 2018

Member

test !ismine here?

jnewbery added some commits Dec 5, 2018

[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.
[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.
[tests] add test_importmulti method to wallet_import.py
Adds a new test_importmulti method for testing the
importmulti RPC method.
[tests] add test_address method to wallet_import.py
Adds a new test_address method for testing the
imported addresses.
[tests] Add docstring for wallet_importmulti.py
Adds a docstring describing the new importmulti test.

@jnewbery jnewbery force-pushed the jnewbery:importmulti_tests branch to ee3b21d Dec 10, 2018

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2018

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
Copy link
Contributor

left a comment

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.

assert_equal(address_assert['iswatchonly'], True)
assert_equal(address_assert['ismine'], False)
assert_equal(address_assert['timestamp'], timestamp)
assert_equal(address_assert['ischange'], True)

# 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()

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 11, 2018

Contributor

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.

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 11, 2018

Contributor

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.

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']])

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 11, 2018

Contributor

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.

@@ -70,6 +86,28 @@ def get_key(self):
CScript([OP_0, pkh]).hex(), # p2sh-p2wpkh redeem script
key_to_p2sh_p2wpkh(pubkey)) # p2sh-p2wpkh addr

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 11, 2018

Contributor

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

@@ -108,6 +108,14 @@ def get_multisig(self):
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):

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 11, 2018

Contributor

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.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 11, 2018

Member

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

@@ -116,6 +116,16 @@ def test_importmulti(self, req, success, error_code=None, error_message=None):
assert_equal(result[0]['error']['code'], error_code)
assert_equal(result[0]['error']['message'], error_message)

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 11, 2018

Contributor

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

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

re-utACK ee3b21d

Only change since my last review was addressing my comments above

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Dec 11, 2018

Merge bitcoin#14886: [tests] Refactor importmulti tests
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

@MarcoFalke MarcoFalke merged commit ee3b21d into bitcoin:master Dec 11, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@meshcollider meshcollider removed this from Blockers in High-priority for review Dec 11, 2018

@Sjors

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Post merge ACK

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2018

Thanks all for the review.

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

@jnewbery jnewbery deleted the jnewbery:importmulti_tests branch Dec 13, 2018

MarcoFalke added a commit that referenced this pull request Jan 9, 2019

Merge #15108: [tests] tidy up wallet_importmulti.py
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.