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

1. 
   jnewbery commented at 5:46 pm on December 13, 2018: member

   Based on #14565. Please don’t review this until that PR is merged.

   Fixes review comments from @ryanofsky here: #14886#pullrequestreview-183772779

2. Overhaul importmulti logic

   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.

   bdacbda253
3. Add release notes eacff95de4
4. [tests] tidy up wallet_importmulti.py

   Fixes review comments from PR 14886.

   9cf23d4ced
5. [tests] move wallet util functions to wallet_util.py

   Adds a new wallet_util.py module and moves generic helper functions
   there:
   
   - get_key
   - get_multisig
   - test_address

   cf99465749
6. 
   DrahtBot commented at 9:40 pm on December 13, 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:

   • #15032 (Nit fixes for PR 14565 by MeshCollider)

   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.

7. in doc/release-notes-14565.md:5 in cf99465749

   0@@ -0,0 +1,5 @@
   1+Low-level RPC changes
   2+---------------------
   3+
   4+The `importmulti` RPC will now contain a new per-request `warnings` field with strings
   5+that explain when fields are being ignored or inconsistant, if any.
   

   ---

   ---

   

   practicalswift commented at 10:15 am on December 14, 2018:

   Should be “inconsistent” :-)
8. in test/functional/wallet_importmulti.py:52 in cf99465749

   87+    def test_importmulti(self, req, success, error_code=None, error_message=None, warnings=[]):
   88         """Run importmulti and assert success"""
   89         result = self.nodes[1].importmulti([req])
   90+        observed_warnings = []
   91+        if 'warnings' in result[0]:
   92+           observed_warnings = result[0]['warnings']
   

   ---

   ---

   

   practicalswift commented at 10:15 am on December 14, 2018:

   Indentation is not a multiple of four :-)
9. in test/functional/wallet_importmulti.py:47 in cf99465749

   82-                        script_to_p2wsh(script_code),  # p2wsh addr
   83-                        CScript([OP_HASH160, witness_script, OP_EQUAL]).hex(),  # p2sh-p2wsh
   84-                        script_to_p2sh_p2wsh(script_code))  # p2sh-p2wsh addr
   85-
   86-    def test_importmulti(self, req, success, error_code=None, error_message=None):
   87+    def test_importmulti(self, req, success, error_code=None, error_message=None, warnings=[]):
   

   ---

   ---

   

   practicalswift commented at 10:21 am on December 14, 2018:

   I suggest using warnings=None instead to clarify that warnings is not meant to be remembered across calls.

   Background:

   0>>> def test(i, i_arr=[]):
   1...     i_arr.append(i)
   2...     return i_arr
   3...
   4>>> test(1)
   5[1]
   6>>> test(2)
   7[1, 2]
   8>>> test(3)
   9[1, 2, 3]
   

   Suggested alternative:

    0>>> def test(i, i_arr=None):
    1...     if i_arr is None:
    2...         i_arr=[]
    3...     i_arr.append(i)
    4...     return i_arr
    5...
    6>>> test(1)
    7[1]
    8>>> test(2)
    9[2]
   10>>> test(3)
   11[3]
   

10. 
    jnewbery commented at 3:44 pm on December 14, 2018: member

    @practicalswift - please see the PR text:

    Based on #14565. Please don’t review this until that PR is merged.

    All of your review comments above are for commits in #14565.

11. meshcollider added the label Wallet on Dec 14, 2018
12. meshcollider added the label Tests on Dec 14, 2018
13. 
    Sjors commented at 10:07 am on December 15, 2018: member

    Meanwhile, concept ACK on wallet_util.py
14. 
    MarcoFalke commented at 9:41 am on January 5, 2019: member

    Picked up in #15108
15. MarcoFalke closed this on Jan 5, 2019

    ---

16. 
    jnewbery commented at 12:11 pm on January 5, 2019: member

    @amitiuttarwar has offered to pick this up in #15108. Reviewers, please leave review comments there.
17. jnewbery deleted the branch on Jan 5, 2019
18. MarcoFalke referenced this in commit 1cfbb16b5d on Jan 9, 2019
19. DrahtBot locked this on Dec 16, 2021

Contributors
jnewbery DrahtBot practicalswift Sjors MarcoFalke

Labels
Wallet Tests