[do not merge] [tests] tidy up wallet_importmulti.py #14952
Conversation
This introduces various changes to the importmulti logic: * Instead of processing input and importing things at the same time, first process all input data and verify it, so no changes are made in case of an error. * Verify that no superfluous information is provided (no keys or scripts that don't contribute to solvability in particular). * Add way more sanity checks, by means of descending into all involved scripts.
Fixes review comments from PR 14886.
Adds a new wallet_util.py module and moves generic helper functions there: - get_key - get_multisig - test_address
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
| --------------------- | ||
|
|
||
| The `importmulti` RPC will now contain a new per-request `warnings` field with strings | ||
| that explain when fields are being ignored or inconsistant, if any. |
There was a problem hiding this comment.
Should be "inconsistent" :-)
| result = self.nodes[1].importmulti([req]) | ||
| observed_warnings = [] | ||
| if 'warnings' in result[0]: | ||
| observed_warnings = result[0]['warnings'] |
There was a problem hiding this comment.
Indentation is not a multiple of four :-)
| script_to_p2sh_p2wsh(script_code)) # p2sh-p2wsh addr | ||
|
|
||
| def test_importmulti(self, req, success, error_code=None, error_message=None): | ||
| def test_importmulti(self, req, success, error_code=None, error_message=None, warnings=[]): |
There was a problem hiding this comment.
I suggest using warnings=None instead to clarify that warnings is not meant to be remembered across calls.
Background:
>>> def test(i, i_arr=[]):
... i_arr.append(i)
... return i_arr
...
>>> test(1)
[1]
>>> test(2)
[1, 2]
>>> test(3)
[1, 2, 3]
Suggested alternative:
>>> def test(i, i_arr=None):
... if i_arr is None:
... i_arr=[]
... i_arr.append(i)
... return i_arr
...
>>> test(1)
[1]
>>> test(2)
[2]
>>> test(3)
[3]
|
@practicalswift - please see the PR text:
All of your review comments above are for commits in #14565. |
|
Meanwhile, concept ACK on |
|
Picked up in #15108 |
|
@amitiuttarwar has offered to pick this up in #15108. Reviewers, please leave review comments there. |
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
7 participants
Based on #14565. Please don't review this until that PR is merged.
Fixes review comments from @ryanofsky here: #14886 (review)