tests: improve wallet multisig descriptor test and docs #29154
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
mjdietzx
force-pushed
the
multisig_descriptor_improvement
branch
from
2d1dcdb
to
80ad869
Compare
| external = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f'/0/*,'.join(xpubs)}/0/*))") | ||
| internal = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f'/1/*,'.join(xpubs)}/1/*))") | ||
| external = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f','.join(external_xpubs)}))") | ||
| internal = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f','.join(internal_xpubs)}))") |
There was a problem hiding this comment.
+1 for getting rid of the /1/* hardcodings!
| @@ -139,12 +143,12 @@ def run_test(self): | |||
|
|
|||
| self.log.info("Send another transaction from the multisig, this time with a daisy chained signing flow (one after another in series)!") | |||
| psbt = participants["multisigs"][0].walletcreatefundedpsbt(inputs=[], outputs={to: value}, feeRate=0.00010) | |||
| for m in range(self.M): | |||
| for i, m in enumerate(random.sample(range(self.M), self.M)): | |||
There was a problem hiding this comment.
m could be different in each test run because it uses random.sample as I found out while debugging this test. For thoroughness, would it be more robust to cover all the orderings instead? In this case it's going to be 2 only because self.M = 2.
There was a problem hiding this comment.
In this case random.sample shuffles the order of participants, but I don't see how it matters since all participants are equivalent anyway
There was a problem hiding this comment.
EDIT: ugh I mixed up PRs. not used to comments on individual commit and thought this was related to my other open PR (decaying multisig). I will address this feedback and respond, I lost track of these comments
but I don't see how it matters since all participants are equivalent anyway
exactly, all participants are equivalent, and the intention of this random.sample is to prove that. maybe it's a little extra to even have this random.sample, but participants being equivalent is critical to the intention of this multisig so I thought the randomness was a nice to have
|
The change looks good to me modulo addressing comments from achow101. |
mjdietzx
force-pushed
the
multisig_descriptor_improvement
branch
from
8a2021c
to
2255b12
Compare
It is best to store all key origin information (master key fingerprint and all derivation steps) in the multisig descriptor. Being explicit with this information should be beneficial if this approach is used with other wallets/signers (whether hardware or software). There is no harm including all of this with xpubs (if anything it simplifies the test code) and makes this example/docs more complete and safer incase it is referenced by others.
mjdietzx
force-pushed
the
multisig_descriptor_improvement
branch
from
2255b12
to
d93b794
Compare
|
I addressed @achow101 's comments:
Thank you all for your reviews and patience |
|
Code Review ACK d93b794 |
It is best to store all key origin information
(master key fingerprint and all derivation steps)
in the multisig descriptor. Being explicit with
this information should be beneficial if this approach is used with other wallets/signers (whether hardware or software). There is no harm including all of this with xpubs (if anything it simplifies the test code) and makes this example/docs more complete and safer incase it is referenced by others.