tests: improve wallet multisig descriptor test and docs #29154

mjdietzx commented at 9:00 PM on December 29, 2023: contributor

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.

DrahtBot commented at 9:00 PM on December 29, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	S3RK, achow101
Stale ACK	rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29156 (tests: add functional test for miniscript decaying multisig by mjdietzx)

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.

DrahtBot added the label Tests on Dec 29, 2023

mjdietzx force-pushed on Dec 30, 2023

DrahtBot added the label CI failed on Jan 15, 2024

achow101 requested review from Sjors on Apr 9, 2024

achow101 requested review from achow101 on Apr 9, 2024

achow101 requested review from S3RK on Apr 9, 2024

in test/functional/wallet_multisig_descriptor_psbt.py:57 in 80ad8690e2 outdated

  61 |              node.createwallet(wallet_name=f"{self.name}_{i}", blank=True, descriptors=True, disable_private_keys=True)
  62 |              multisig = node.get_wallet_rpc(f"{self.name}_{i}")
  63 | -            external = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f'/0/*,'.join(xpubs)}/0/*))")
  64 | -            internal = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f'/1/*,'.join(xpubs)}/1/*))")
  65 | +            external = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f','.join(external_xpubs)}))")
  66 | +            internal = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f','.join(internal_xpubs)}))")

rkrux commented at 7:30 AM on April 13, 2024:

+1 for getting rid of the /1/* hardcodings!

in test/functional/wallet_multisig_descriptor_psbt.py:146 in 8a2021c0f4 outdated

 142 | @@ -139,12 +143,12 @@ def run_test(self):
 143 |  
 144 |          self.log.info("Send another transaction from the multisig, this time with a daisy chained signing flow (one after another in series)!")
 145 |          psbt = participants["multisigs"][0].walletcreatefundedpsbt(inputs=[], outputs={to: value}, feeRate=0.00010)
 146 | -        for m in range(self.M):
 147 | +        for i, m in enumerate(random.sample(range(self.M), self.M)):

rkrux commented at 7:33 AM on April 13, 2024:

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.

S3RK commented at 7:44 AM on May 8, 2024:

In this case random.sample shuffles the order of participants, but I don't see how it matters since all participants are equivalent anyway

mjdietzx commented at 5:48 PM on May 9, 2024:

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

rkrux approved

rkrux commented at 7:34 AM on April 13, 2024: contributor

tACK 8a2021c

I am in favour of this change because it adds verbosity that makes it easier to go through the code. As mentioned in the PR description, I, too, don't see a harm in including key origin information.

in doc/descriptors.md:66 in 80ad8690e2 outdated

  62 | @@ -63,6 +63,7 @@ Output descriptors currently support:
  63 |  - `wsh(sortedmulti(1,xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/1/0/*,xpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3ZuxV27CprR9LgpeyGmXUbC6wb7ERfvrnKZjXoUmmDznezpbZb7ap6r1D3tgFxHmwMkQTPH/0/0/*))` describes a set of *1-of-2* P2WSH multisig outputs where one multisig key is the *1/0/`i`* child of the first specified xpub and the other multisig key is the *0/0/`i`* child of the second specified xpub, and `i` is any number in a configurable range (`0-1000` by default). The order of public keys in the resulting witnessScripts is determined by the lexicographic order of the public keys at that index.
  64 |  - `tr(c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5,{pk(fff97bd5755eeea420453a14355235d382f6472f8568a18b2f057a1460297556),pk(e493dbf1c10d80f3581e4904930b1404cc6c13900ee0758474fa94abe8c4cd13)})` describes a P2TR output with the `c6...` x-only pubkey as internal key, and two script paths.
  65 |  - `tr(c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5,sortedmulti_a(2,2f8bde4d1a07209355b4a7250a5c5128e88b84bddc619ab7cba8d569b240efe4,5cbdf0646e5db4eaa398f365f2ea7a0e3d419b7e0330e39ce92bddedcac4f9bc))` describes a P2TR output with the `c6...` x-only pubkey as internal key, and a single `multi_a` script that needs 2 signatures with 2 specified x-only keys, which will be sorted lexicographically.
  66 | +- `wsh(sortedmulti(2,[6f53d49c/44h/1h/0h]tpubDDjsCRDQ9YzyaAq9rspCfq8RZFrWoBpYnLxK6sS2hS2yukqSczgcYiur8Scx4Hd5AZatxTuzMtJQJhchufv1FRFanLqUP7JHwusSSpfcEp2/0/*,[e6807791/44h/1h/0h]tpubDDAfvogaaAxaFJ6c15ht7Tq6ZmiqFYfrSmZsHu7tHXBgnjMZSHAeHSwhvjARNA6Qybon4ksPksjRbPDVp7yXA1KjTjSd5x18KHqbppnXP1s/0/*,[367c9cfa/44h/1h/0h]tpubDDtPnSgWYk8dDnaDwnof4ehcnjuL5VoUt1eW2MoAed1grPHuXPDnkX1fWMvXfcz3NqFxPbhqNZ3QBdYjLz2hABeM9Z2oqMR1Gt2HHYDoCgh/0/*))#av0kxgw0` describes a *2-of-3* multisig. This descriptor was generated by a test run of [test/functional/wallet_multisig_descriptor_psbt.py](/test/functional/wallet_multisig_descriptor_psbt.py) which is further documented in the [Basic multisig example](#basic-multisig-example) section below. For brevity, the internal "change" descriptor accompanying the above external "receiving" descriptor is not included here, but it typically differs only in the xpub derivation steps, ending in `/1/*` for change addresses.

achow101 commented at 6:37 PM on April 16, 2024:

In 80ad8690e25c1ecab2ab1ff282882d5a6745f496 "tests: improve wallet multisig descriptor test and docs"

I don't think it's necessary to explain where the descriptor came from, just a description of it is fine.

mjdietzx commented at 5:48 PM on May 9, 2024:

Done

maflcko commented at 1:19 PM on May 13, 2024:

Done

Did you push the changes?

mjdietzx commented at 1:08 AM on May 20, 2024:

I just pushed the changes now 🤦‍♂️

in test/functional/wallet_multisig_descriptor_psbt.py:127 in 8a2021c0f4 outdated

 123 | @@ -120,7 +124,7 @@ def run_test(self):
 124 |  
 125 |          psbts = []
 126 |          self.log.info("Now at least M users check the psbt with decodepsbt and (if OK) signs it with walletprocesspsbt...")
 127 | -        for m in range(self.M):
 128 | +        for m in random.sample(range(self.M), self.M):

achow101 commented at 6:46 PM on April 16, 2024:

In 8a2021c0f4f40f6f8a37dd5619ecfa3f38084679 "tests: assert xpub order in sortedmulti descriptors do not matter"

It's not clear to me what this random.sample is supposed to do or how it improves the test.

mjdietzx commented at 5:56 PM on May 9, 2024:

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

~~All participants are equivalent as discussed in related review comment. Maybe it's extra and we shouldn't even bother with random.sample. If you'd prefer that lmk and I can remove~~

~~Why I included it in first place:~~ - all participants being equivalent is a critical property of this multisig. I know it's clear behavior of thresh. But I went ahead and asserted it in this test. Otherwise as signers decay, without a change in ordering, we are always testing that the same m of N signers that can recover at 3 of 4, 2 of 4, 1 of 4. If it happened to be that only the first signer can recover at 1 of 4 it would be bad if user of this multisig didn't expect it and that was the key they lost or couldn't recover. The shuffling tries to prove (in a very simple way) that any keys can be the 3 of 4, 2 of 4, or 1 of 4, independent of ordering in the wallet descriptor

S3RK commented at 7:45 AM on May 8, 2024: contributor

The change looks good to me modulo addressing comments from achow101.

mjdietzx force-pushed on May 20, 2024

tests: improve wallet multisig descriptor test and docs

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.

d93b794709

mjdietzx force-pushed on May 20, 2024

mjdietzx commented at 1:07 AM on May 20, 2024: contributor

I addressed @achow101 's comments:

re this comment, I removed random.sample. it's unnecessary
re this comment, I removed the unnecessary explanation of where descriptor came from

Thank you all for your reviews and patience

DrahtBot removed the label CI failed on May 20, 2024

S3RK commented at 7:40 AM on May 27, 2024: contributor

Code Review ACK d93b79470916b1e6f85c55cc6beb1e41b382196f

DrahtBot requested review from rkrux on May 27, 2024

achow101 commented at 11:52 PM on July 9, 2024: member

ACK d93b79470916b1e6f85c55cc6beb1e41b382196f

achow101 merged this on Jul 10, 2024

achow101 closed this on Jul 10, 2024

bitcoin locked this on Jul 10, 2025