Test and document a basic M-of-N multisig using descriptor wallets and PSBTs #22067

mjdietzx commented at 12:58 AM on May 26, 2021: contributor

Aims to resolve issue #21278. I try to follow the steps laanwj outlined there exactly, with the exception of using combinepsbt instead of joinpsbts. I wrote a functional test to make sure it works as expected before doing the docs, and figured it would also be a good source of documentation. So I kept the test as simple as possible and didn't go crazy with edge-cases and various checks. I do have a lot more test-cases I've written that I will follow up with (either in a separate PR or another commit - lmk if you have a preference), but I want to do it in a way that doesn't bloat this test so it remains useful as a quickstart (unless that's a bad idea)?

mjdietzx force-pushed on May 26, 2021

DrahtBot added the label Docs on May 26, 2021

DrahtBot added the label Tests on May 26, 2021

mjdietzx force-pushed on May 26, 2021

in test/functional/wallet_multisig_descriptor_psbt.py:27 in 4069660cac outdated

  23 | +        self.wallet_names = []
  24 | +        self.extra_args = [["-keypool=100"]] * self.num_nodes
  25 | +
  26 | +    def skip_test_if_missing_module(self):
  27 | +        self.skip_if_no_wallet()
  28 | +        self.skip_if_no_sqlite()

Sjors commented at 1:13 PM on May 26, 2021:

I believe you can omit --descriptors when this check is present, @achow101?

in test/functional/wallet_multisig_descriptor_psbt.py:2 in 4069660cac outdated

   0 | @@ -0,0 +1,145 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2019-2020 The Bitcoin Core developers

Sjors commented at 1:13 PM on May 26, 2021:

Nit: 2021

in doc/descriptors.md:140 in 4069660cac outdated

 135 | +
 136 | +For a good example of a basic M-of-N multisig between multiple participants using descriptor
 137 | +wallets and PSBTs, as well as a signing flow, see [this functional test](/test/functional/wallet_multisig_descriptor_psbt.py).
 138 | +The basic steps are:
 139 | +
 140 | +  1. Every participant generates an xpub. The most straightforward way is to create a new wallet

Sjors commented at 1:22 PM on May 26, 2021:

"is to create a new descriptor wallet. Avoid reusing this wallet for any other purpose."

Could add a hint here on how to extract the xpub. Using listdescriptors and maybe pick the one from pkh, since you're least likely to accidentally reuse that (legacy addresses).

mjdietzx commented at 2:47 PM on May 26, 2021:

Just for my understanding, what are the practical drawbacks/risks of someone re-using this wallet for other purposes? To me it's not obvious why I shouldn't have a "signer" wallet that signs for a few multisigs and also has some of its own funds

Sjors commented at 3:08 PM on May 26, 2021:

It's bad for your privacy if (and only if) you share that xpub with co-signers. They'll be able to guess the single-signature wallet derivation and see your transactions. If the M-of-N is just you then it doesn't matter.

mjdietzx commented at 3:11 PM on May 26, 2021:

Ah OK got it - thanks!

sipa commented at 3:15 PM on May 26, 2021:

If you have a wallet with just signers, this may not matter, but generally, if you have one wallet with multiple unrelated funds/descriptors imported, its balances will be the sum of all, with no way to separate them out. That may not be what you want.

in doc/descriptors.md:141 in 4069660cac outdated

 136 | +For a good example of a basic M-of-N multisig between multiple participants using descriptor
 137 | +wallets and PSBTs, as well as a signing flow, see [this functional test](/test/functional/wallet_multisig_descriptor_psbt.py).
 138 | +The basic steps are:
 139 | +
 140 | +  1. Every participant generates an xpub. The most straightforward way is to create a new wallet
 141 | +  2. The multisig is created by importing the following descriptors: `wsh(sortedmulti(<M>,xpubA/<0,1>/*,xpubB/<0,1>/*,…,xpubN/<0,1>/*))`

Sjors commented at 1:23 PM on May 26, 2021:

So they have to make a fresh watch-only wallet first.

in doc/descriptors.md:145 in 4069660cac outdated

 140 | +  1. Every participant generates an xpub. The most straightforward way is to create a new wallet
 141 | +  2. The multisig is created by importing the following descriptors: `wsh(sortedmulti(<M>,xpubA/<0,1>/*,xpubB/<0,1>/*,…,xpubN/<0,1>/*))`
 142 | +    (one descriptor w/ `0` for receiving addresses and another w/ `1` for change). The resulting
 143 | +    wallet is watch-only and every participant does this
 144 | +  3. A receiving address is generated for the multisig
 145 | +  4. Funds are sent to the resulting address (every participant checks that it gets the same addresses)

Sjors commented at 1:24 PM on May 26, 2021:

every participant checks that it gets the same addresses) maybe move to step 3

in doc/descriptors.md:146 in 4069660cac outdated

 141 | +  2. The multisig is created by importing the following descriptors: `wsh(sortedmulti(<M>,xpubA/<0,1>/*,xpubB/<0,1>/*,…,xpubN/<0,1>/*))`
 142 | +    (one descriptor w/ `0` for receiving addresses and another w/ `1` for change). The resulting
 143 | +    wallet is watch-only and every participant does this
 144 | +  3. A receiving address is generated for the multisig
 145 | +  4. Funds are sent to the resulting address (every participant checks that it gets the same addresses)
 146 | +  5. A sending transaction is created using `walletcreatefundedpsbt` (anyone can initiate this)

Sjors commented at 1:25 PM on May 26, 2021:

(much easier in the GUI)

in doc/descriptors.md:147 in 4069660cac outdated

 142 | +    (one descriptor w/ `0` for receiving addresses and another w/ `1` for change). The resulting
 143 | +    wallet is watch-only and every participant does this
 144 | +  3. A receiving address is generated for the multisig
 145 | +  4. Funds are sent to the resulting address (every participant checks that it gets the same addresses)
 146 | +  5. A sending transaction is created using `walletcreatefundedpsbt` (anyone can initiate this)
 147 | +  6. At least `M` users check the PSBT with `decodepsbt` and (if OK) signs it with `walletprocesspsbt`

Sjors commented at 1:25 PM on May 26, 2021:

(or load psbt in the GUI, which takes care of 7 too)

Sjors commented at 1:29 PM on May 26, 2021: member

Concept ACK

This documentation and example also help illustrate where things could be made easier, e.g.:

having a wallet with just a seed, but no descriptors, and a way to ask it for an xpub
and RPC call (or wallet-tool command) to create a multisig descriptor given a bunch xpubs

mjdietzx force-pushed on May 26, 2021

mjdietzx commented at 2:45 PM on May 26, 2021: contributor

Thanks for the thorough review and suggestions @Sjors ! I've addressed all your feedback in my latest push.

This documentation and example also help illustrate where things could be made easier, e.g.:

having a wallet with just a seed, but no descriptors, and a way to ask it for an xpub

I see the benefit of this - I found it tricky to extract a wallet's xpubs in bitcoin core (didn't find a way to do this w/ the GUI, the only way I know is w/ the rpc command listdescritptors as documented/tested here, but that means users will need to use the console for this). While most other wallets seem to make this easily accessible

and RPC call (or wallet-tool command) to create a multisig descriptor given a bunch xpubs

Interesting idea - I guess this would take care of step 2 in one easy command? Is the reason you suggest this to push forward "standardization" of multisigs similar to the approach outlined here?

mjdietzx force-pushed on May 26, 2021

Sjors commented at 3:11 PM on May 26, 2021: member

Eventually it would be nice if step (1) and (2) are trivial to do with RPC or standalone tools. And then hopefully also easy to add GUI support for them. E.g. a screen where you can share your xpub, copy-paste xpubs from your co-signers, set the threshold, etc. Standardisation is another issue.

mjdietzx commented at 4:37 PM on May 26, 2021: contributor

Any thoughts about me adding some caution/disclaimer text in the docs about the interoperability of this mulitsig with (external) common hardware wallets? Based on my testing so far it isn't as straightforward as just getting an xpub from your HW device and using that as a signer in the multisig. For example, ColdCard has some restrictions/checks (and a proprietary configuration text file for multisig), and I haven't been able to get a ColdCard to sign one of these PSBTs (even after trying to setup everything how they want it 😕). It's strange, and I plan on following up with popular HW wallet providers because I would expect it to work seamlessly, but as of now it doesn't seem to (and could lead to locked funds if someone tries this before HW wallets get updated).

How I would expect a wallet like ColdCard to handle this is when the user goes to sign the PSBT w/ their HW wallet:

The wallet runs > decodepsbt "psbt" (the PSBT base64 string) and sees something like:

{
  tx: {
    txid: '...',
    hash: '...',
    version: 2,
    size: 94,
    vsize: 94,
    weight: 376,
    ...
    vin: [ [Object] ],
    vout: [ [Object] ]
  },
  ...
  inputs: [
    {
      witness_utxo: [Object],
      non_witness_utxo: [Object],
      witness_script: [Object],
      bip32_derivs: [Array]
    }
  ],
  outputs: [ {} ],
}

Now (before signing the PSBT) the HW wallet can just check the inputs: witness_script.asm to verify this is a standard N-of-M multisig transfer, and bip32_derivs to verify that this device's master_fingerprint is present and to determine the path/pubkey it needs to sign for:

...
witness_script: {
  asm: '2 <pubkeyA> <pubkeyB> <pubkeyC> 3 OP_CHECKMULTISIG',
  hex: '...',
  type: 'multisig'
},
bip32_derivs: [
  {
    pubkey: <pubkeyA>,
    master_fingerprint: <deviceAFingerprint>,
    path: 'm/0/0'
  },
  {
    pubkey: <pubkeyB>,
    master_fingerprint: <deviceBFingerprint>,
    path: 'm/0/0'
  },
  {
    pubkey: <pubkeyC>,
    master_fingerprint: <deviceCFingerprint>,
    path: 'm/0/0'
  }
]

And after these very simple checks it should be able to sign the PSBT without any additional setup/configuration from the end user. Am I over simplifying this or missing anything? Just want to get some feedback before I chase issues down with HW wallets

Sjors commented at 5:44 PM on May 26, 2021: member

I think the ColdCard file format for communicating wallet info is essentially the Electrum format.

Afaik with Trezor and Ledger it is a simple matter of getting an xpub (e.g. using HWI).

It's probably still most practical to use something Specter Desktop to configure multisig wallets. Once setup, spending can be done with just Bitcoin Core afaik.

Note that for signing each device should not just check that it's own keys are present, but also that cosigners weren't changed. So these need to be registered. ColdCard does that, which is why it's a bit more complicated to setup. Trezor and Ledger don't, so they're easier to setup, but also easier to fool.

There are folks working on standardization of the setup process, e.g. BIP 86 and BIP 129.

Also take a look at a PSBT extension that would includes xpubs: #17034: that could theoretically let you switch to trust-on-first-use when spending. But you really should verify an address before receiving coins on it, not when you spend from it.

mjdietzx commented at 6:10 PM on May 26, 2021: contributor

Thanks for the explanation and links, very helpful. I'll look into all of this more.

But you really should verify an address before receiving coins on it, not when you spend from it.

I guess this last sentence is where I get caught up. By the time I try to sign a PSBT (as a multisig co-signer):

The watch-only multisig has funds I am trying to spend
I really care about where I am sending these funds, the amount, and the fee. And that's what I'm verifying on my device, as usual

So is there a practical attack vector where somehow I am given a PSBT that has changed/different cosigners? I guess I'm missing the point of the extra complexity and setup on the HW side (and now I have to trust software outside of bitcoin core like Specter). Especially when I would think a lot of multisigs are either self-contained (ie my own M-of-N multis) or between "friends" (it's not like I'm signing random PSBTs I get from who knows where). Or I'm still missing something?

I guess the worst case scenarios is the multisig "coordinator" gets me to sign a PSBT that's still a standard M-of-N multisig as documented here, and that I'm a cosigner for, but the other cosigners have been changed? Maybe they can get me to do this a few times (to extract some info?) but what's the harm since they shouldn't be able to trick me into sending funds anywhere?

Sending to the multisig seems like an entirely separate concern, and seems to be handled pretty well in this process (as outlined in these docs) because participants create/import their watch-only multisig once, and then would obviously generate new receive addresses there (and all participants are checking that the receive addresses match)

Sjors commented at 6:14 PM on May 26, 2021: member

@mjdietzx one thing to worry about is an attacker modifying the change address to a 2-of-3, with two of his keys, and hold you ransom.

mjdietzx commented at 6:18 PM on May 26, 2021: contributor

one thing to worry about is an attacker modifying the change address to a 2-of-3, with two of his keys, and hold you ransom.

Isn't that checked against here, where every participant checks the PSBT w/ decodepsbt with their watch-only multisig before signing (step 6 in the docs this PR adds)? https://github.com/bitcoin/bitcoin/pull/22067/files#diff-94f52fa5de899f58d07aca5026545378bc3da616d4927d71435fa0224eda589bR41

Sjors commented at 6:29 PM on May 26, 2021: member

Yes, but the reason you can do this check is because you imported the xpubs from other devices. That's what makes the setup tedious.

in test/functional/wallet_multisig_descriptor_psbt.py:56 in 932de9b3cb outdated

  51 | +
  52 | +    def participants_import_descriptors(self, participants, xpubs):
  53 | +        """The multisig is created by importing the following descriptors. The resulting wallet is watch-only and every participant can do this."""
  54 | +        # some simple validation, this will throw if the base58 checksum of any xpub is invalid
  55 | +        assert_equal(len(xpubs), self.N)
  56 | +        [base58_to_byte(xpub) for xpub in xpubs]

lsilva01 commented at 2:10 AM on May 30, 2021:

This line can be removed, can't it ?

mjdietzx commented at 2:51 PM on June 1, 2021:

This line is not strictly necessary. base58_to_byte will throw an exception if one of the provided xpubs checksum is invalid.

But at second glance this is confusing how I did it. I may get rid of it, or make it more obvious this is input validation

mjdietzx commented at 8:35 PM on July 14, 2021:

I made this more clear in the code/comment: https://github.com/bitcoin/bitcoin/pull/22067/files#diff-94f52fa5de899f58d07aca5026545378bc3da616d4927d71435fa0224eda589bR56-R58

lsilva01 approved

lsilva01 commented at 2:12 AM on May 30, 2021: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/22067/commits/932de9b3cb7baf128a506dda3c3ec5a88e07b6ca

theStack commented at 1:44 PM on June 9, 2021: member

Concept ACK

Rspigler commented at 8:21 PM on June 28, 2021: contributor

Thank you for working on this!

I think this is a great first step, but ultimately we will need BIP 129, BIP 87, (possibly) BIP 88, and PSBT v2 support.

As @Sjors pointed out, I think this is useful because it also provides us a list of To Do's (besides the above):

having a wallet with just a seed, but no descriptors, and a way to ask it for an xpub
RPC call (or wallet-tool command) to create a multisig descriptor given a bunch xpubs
GUI support

It's probably still most practical to use something Specter Desktop to configure multisig wallets. Once setup, spending can be done with just Bitcoin Core afaik.

I agree this is the case for now, but hopefully in the future Bitcoin Core can be a full fledged coordinator.

Create a watch-only descriptor wallet (blank, private keys disabled). Now the multisig is created by importing the following descriptors: wsh(sortedmulti(<M>,xpubA/<0,1>/*,xpubB/<0,1>/*,…,xpubN/<0,1>/*))

This isn't the proper way to write two descriptors (see here). I think using a descriptor template would be better.

mjdietzx force-pushed on Jul 14, 2021

mjdietzx commented at 8:39 PM on July 14, 2021: contributor

@Rspigler Thanks for the review, re point 2:

This isn't the proper way to write two descriptors (see here). I think using a descriptor template would be better.

Is this better? https://github.com/bitcoin/bitcoin/pull/22067/files#diff-00ec9f97461f6bb94a37daf8069e5acf832ea4d5a31a8d7d4968c285d2fcac9bR144

mjdietzx commented at 8:42 PM on July 14, 2021: contributor

@laanwj If you have a chance can you give this a quick review?

Rspigler commented at 1:09 AM on July 15, 2021: contributor

wsh(sortedmulti(<M>,XPUB1/**,XPUB2/**,…,XPUBN/**)) /0/*,/1/*

Yes, that is the proper template!

However, now that I think about it again, since this is documenting the setup for Bitcoin Core which doesn't have support for templates yet, the directions should probably write out explicitly the importing of both descriptors separately - which is what users will have to do. So something like this:

Now the multisig is created by importing the two descriptors; For example: wsh(sortedmulti(<M>,XPUB1/0/*,XPUB2/0/*,...XPUBN/0/*)) wsh(sortedmulti(<M>,XPUB1/1/*,XPUB2/1/*,...XPUBN/1/*))

(one descriptor w/ 0 in the derivation path for receiving addresses and another w/ 1 for change). Every participant does this

And maybe templates not be mentioned at all. Sorry for flip flopping on you

mjdietzx force-pushed on Jul 15, 2021

mjdietzx commented at 2:21 PM on July 15, 2021: contributor

Good point @Rspigler I removed the descriptor templates, but fixed as you suggest: https://github.com/bitcoin/bitcoin/pull/22067/commits/911ed484515f324c82e3121c84ce91751fdd4949#diff-00ec9f97461f6bb94a37daf8069e5acf832ea4d5a31a8d7d4968c285d2fcac9bR144

Rspigler commented at 2:39 PM on July 15, 2021: contributor

ACK d1ce7bb4fb1f8cd929e23eae7aa9dd0862e938f2 (doc changes; not yet fully comfortable with my understanding of the tests in Core).

OliverOffing commented at 1:18 PM on August 6, 2021: none

ACK d1ce7bb (tests pass)

in test/functional/wallet_multisig_descriptor_psbt.py:71 in d1ce7bb4fb outdated

  66 | +                    "active": True,
  67 | +                    "internal": False,
  68 | +                    "timestamp": "now",
  69 | +                },
  70 | +                {  # change addresses (internal: True)
  71 | +                    "desc": descsum_create(f"wsh(sortedmulti({self.M},{f'/{1}/*,'.join(xpubs)}/{1}/*))"),

lsilva01 commented at 4:49 AM on August 12, 2021:

nit: By using getdescriptorinfo, it eliminates the need for descsum_create, so the test will only use node functions without an additional script for the checksum.

from test_framework.descriptors import descsum_create can also be removed.

            multisig = node.get_wallet_rpc(f"{self.name}_{i}")
            
            external_desc = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f'/{0}/*,'.join(xpubs)}/{0}/*))")
            internal_desc = multisig.getdescriptorinfo(f"wsh(sortedmulti({self.M},{f'/{1}/*,'.join(xpubs)}/{1}/*))")
            
            result = multisig.importdescriptors([
                {  # receiving addresses (internal: False)
                    "desc": external_desc["descriptor"],
                    "active": True,
                    "internal": False,
                    "timestamp": "now",
                },
                {  # change addresses (internal: True)
                    "desc": internal_desc["descriptor"],

mjdietzx commented at 2:16 PM on September 3, 2021:

I took this suggestion and added the improvement in https://github.com/bitcoin/bitcoin/pull/22067/commits/1f20501efce041d34e63ab9a11359bedf4a82cd5. Marking this resolved

lsilva01 approved

lsilva01 commented at 4:50 AM on August 12, 2021: contributor

reACK https://github.com/bitcoin/bitcoin/pull/22067/commits/d1ce7bb4fb1f8cd929e23eae7aa9dd0862e938f2

test: add functional test for multisig flow with descriptor wallets and PSBTs 1f20501efc

doc: M-of-N multisig using descriptor wallets and PSBTs, as well as a signing flow 17dd657300

doc: add another signing flow for multisig with descriptor wallets and PSBTs e05cd0546a

mjdietzx force-pushed on Aug 16, 2021

mjdietzx commented at 5:45 AM on August 16, 2021: contributor

Thanks for the review @lsilva01

By using importdescriptors, it eliminates the need for descsum_create, so the test will only use node functions without an additional script for the checksum.

Good idea, I just pushed this improvement 👍

fanquake requested review from achow101 on Aug 16, 2021

achow101 commented at 5:57 PM on August 16, 2021: member

ACK e05cd0546a155afcd45c43ce730c4abecd40dfed

in test/functional/wallet_multisig_descriptor_psbt.py:88 in 1f20501efc outdated

  83 | +        receiving_address = multisig.getnewaddress()
  84 | +        for i in range(1, self.N):
  85 | +            assert_equal(receiving_address, self.nodes[i].get_wallet_rpc(f"{self.name}_{i}").getnewaddress())
  86 | +        return receiving_address
  87 | +
  88 | +    def make_sending_transaction(self, to, value):

S3RK commented at 6:53 AM on August 26, 2021:

nit: I think it'd be clearer if you just inline this function

mjdietzx commented at 6:50 PM on September 3, 2021:

Done in f9479e4626f6b5126ff8cdab3a7e718c609429ef

in test/functional/wallet_multisig_descriptor_psbt.py:127 in 1f20501efc outdated

 122 | +        psbts = []
 123 | +        self.log.info("At least M users check the psbt with decodepsbt and (if OK) signs it with walletprocesspsbt...")
 124 | +        for m in range(self.M):
 125 | +            signers_multisig = self.nodes[m].get_wallet_rpc(f"{self.name}_{m}")
 126 | +            self._check_psbt(psbt["psbt"], to, value, signers_multisig)
 127 | +            signing_wallet = self.nodes[m].get_wallet_rpc(f"participant_{m}")

S3RK commented at 7:11 AM on August 26, 2021:

nit: here and it other places you're repeating the convention like self.nodes[i].get_wallet_rpc(f"participant_{i}") and self.nodes[i].get_wallet_rpc({self.name}_{i}) to access i-th participant signer and multisig wallet. Not a huge fan of this approach

I suggest creating all wallets in the begging of the test. This will also make it clearer that each party needs two separate wallets and their purposes. Then you can either: A) store two lists of wallet RPC handlers in a variable for further reuse and/or B) add helper function to access i-th signer and multisig

P.S. it also becomes cleaner if you have just one wallet

mjdietzx commented at 6:50 PM on September 3, 2021:

Done in f9479e4626f6b5126ff8cdab3a7e718c609429ef

in doc/descriptors.md:150 in 17dd657300 outdated

 145 | +wallets and PSBTs, as well as a signing flow, see [this functional test](/test/functional/wallet_multisig_descriptor_psbt.py).
 146 | +The basic steps are:
 147 | +
 148 | +  1. Every participant generates an xpub. The most straightforward way is to create a new descriptor wallet.
 149 | +     Avoid reusing this wallet for any other purpose. Hint: extract the wallet's xpubs using `listdescriptors`
 150 | +     and pick the one from the `pkh` descriptor since it's least likely to be accidentally reused (legacy addresses)

S3RK commented at 7:37 AM on August 26, 2021:

To avoid address reuse you can also deactivate the descriptor. It won't generate any new addresses, but you will be still able to sign psbt with it.

To deactivate descriptor you need to re-import with setting active: false

in doc/descriptors.md:159 in 17dd657300 outdated

 154 | +  3. A receiving address is generated for the multisig. As a check to ensure step 2 was done correctly, every participant
 155 | +     should verify they get the same addresses
 156 | +  4. Funds are sent to the resulting address
 157 | +  5. A sending transaction is created using `walletcreatefundedpsbt` (anyone can initiate this). It is simple to do this in
 158 | +     the GUI by going to the `Send` tab in the multisig wallet and creating an unsigned transaction (PSBT)
 159 | +  6. At least `M` users check the PSBT with `decodepsbt` and (if OK) signs it with `walletprocesspsbt`. It is simple to do

S3RK commented at 7:42 AM on August 26, 2021:

Especially here, but in general in steps 5-7, it's not clear which one of two wallets you need to use for which command.

Maybe split step 6 into two and mention in the begging which wallet you need to use.

P.S. not relevant if you have just one wallet

mjdietzx commented at 6:50 PM on September 3, 2021:

Done in f9479e4626f6b5126ff8cdab3a7e718c609429ef

in test/functional/wallet_multisig_descriptor_psbt.py:146 in e05cd0546a outdated

 138 | @@ -139,5 +139,21 @@ def run_test(self):
 139 |          assert_approx(self.nodes[0].get_wallet_rpc(f"{self.name}_{0}").getbalance(), deposit_amount - value, vspan=0.001)
 140 |          assert_equal(self.nodes[self.N - 1].get_wallet_rpc(f"participant_{self.N - 1}").getbalance(), value)
 141 |  
 142 | +        self.log.info("Send another transaction from the multisig, this time with a daisy chained signing flow (one after another in series)!")
 143 | +        psbt = self.make_sending_transaction(to, value)
 144 | +        for m in range(self.M):
 145 | +            signing_wallet = self.nodes[m].get_wallet_rpc(f"participant_{m}")
 146 | +            psbt = signing_wallet.walletprocesspsbt(psbt["psbt"])

S3RK commented at 7:47 AM on August 26, 2021:

nit: should add _check_psbt call as well?

mjdietzx commented at 6:50 PM on September 3, 2021:

Done in f9479e4626f6b5126ff8cdab3a7e718c609429ef

S3RK commented at 7:59 AM on August 26, 2021: member

Started review

josibake commented at 6:51 AM on September 1, 2021: member

Concept ACK

Reading through the documentation, it sounds good. Also rebased and ran the new test. Setting aside time later today to try and follow your documentation step by step and will post some feedback (if any)

S3RK changes_requested

S3RK commented at 7:27 AM on September 2, 2021: member

Why not have just one wallet? Having two wallets is less intuitive, complicates backups and could lead to user confusion and even loss of funds. Especially taking into account that you need to backup both wallets to restore your funds. This doesn't come through the documentation.

There are two possibilities here:

Import multisig descriptor into the wallet you used to generate xpub. You can do it by taking your xpriv and others' xpub. Additionally you can disable the original descriptors. The downside you'd have disabled and unused descriptors lingering in the wallet.
Import multisig descriptor with your xpriv into new wallet and then drop the original wallet. You have a perfectly clean setup, but you still need two create two wallets and drop one of them.

I also left a bunch of nits, but they are secondary compared to my concern with two wallets.

jonatack commented at 1:45 PM on September 2, 2021: member

Concept ACK, very nice.

michaelfolkson commented at 3:48 PM on September 2, 2021: contributor

Concept ACK. Functional test is nice addition.

test, doc: basic M-of-N multisig minor cleanup and clarifications

wallet_multisig_descriptor_psbt.py is refactored in this commit. While
behavior doesn't change we do cleanup the way wallets are accessed
throughout the test as this is done a lot for the various signers
and their multisigs. We also get rid of some shallow methods and
instead inline them for improved readability.

descriptors.md is improved to be more explicit about which wallet
(ie the signer or multisig) is required for each step.

f9479e4626

doc: add disclaimer highlighting shortcomings of the basic multisig example 9de0d94508

mjdietzx commented at 6:56 PM on September 3, 2021: contributor

@S3RK thank you for the review and great suggestions.

Because I feel this PR is already pretty far along and has a lot of reviews, I addressed your minor/refactor comments in an additional commit: f9479e4626f6b5126ff8cdab3a7e718c609429ef. I thought this made most sense, but I can squash it into 1f20501efce041d34e63ab9a11359bedf4a82cd5 (and the doc changes into 17dd6573008c8aca9fc0da9419225c85a4f94330) if I get some comments suggesting that'd be better.

As for your main concern:

Why not have just one wallet? Having two wallets is less intuitive, complicates backups and could lead to user confusion and even loss of funds. Especially taking into account that you need to backup both wallets to restore your funds. This doesn't come through the documentation.

I think it's a great point. And your ideas on how to do this are really good. I have plans to add test coverage and capabilities to multsigs like this. And it's something I will try out and hopefully have a followup PR for. But for this PR I want to keep things as basic and simple as possible. Therefore I am shelving this (unless someone else gets to it first), but will def follow up (whether as an author or reviewer). For now though, I thought it was best (and good in general) to add a disclaimer in the docs: 9de0d94508828f5fdfaf688ccda5a91d38b32c58 highlighting some shortcomings of this basic example

S3RK commented at 4:20 PM on September 16, 2021: member

@mjdietzx thanks for addressing my nits.

The rest is just my opinion and I don't know what other contributors think about it.

I understand that you want to keep things simple, but we shouldn't sacrifice safety for it. The key problem is separation of multisig metadata (i.e. descriptor with other xpubs and derivation paths) and the keys. These things are pointless on their own and I'm not comfortable ACKing an approach where we separate them into two wallets. You can ignore the rest of my suggestions to disable descriptors or use temporary wallet to generate xprv. Agree, this is all icing on a cake.

From user perspective one wallet is simpler than two. So I don't think workflow with one wallet will be overall more complicated than with two.

Happy to help with implementation or advice.

Rspigler commented at 4:48 AM on September 21, 2021: contributor

I think how it is done currently with 2 wallets is better for same reasons as described here

S3RK commented at 6:48 AM on September 27, 2021: member

@Rspigler that's a good argument. I need to think about it more and will come back with a reply

S3RK commented at 7:20 AM on October 7, 2021: member

Code review ACK 9de0d94. Rspigler's argument convinced me that we should leave the workflow with two wallets. I assume using multisig with external signers is a popular use-case and it's important to keep compatibility.

Edit@laanwj: removed @ from ACK message for merge

laanwj commented at 1:27 PM on October 13, 2021: member

Thanks for adding this example, and the test. I was initially confused by the need to use two wallets but I understand it.

Code and documentation review ACK 9de0d94508828f5fdfaf688ccda5a91d38b32c58

laanwj merged this on Oct 18, 2021

laanwj closed this on Oct 18, 2021

sidhujag referenced this in commit 8edbe15e47 on Oct 18, 2021

hebasto commented at 7:50 PM on October 18, 2021: member

It looks some CI task fail: https://cirrus-ci.com/build/6343453790437376

Conflict with #23207?

hebasto commented at 9:07 PM on October 18, 2021: member

It looks some CI task fail: https://cirrus-ci.com/build/6343453790437376

Conflict with #23207?

Should be fixed in #23303.

MarcoFalke referenced this in commit 077e98c6c2 on Oct 19, 2021

sidhujag referenced this in commit c3dcd95e0e on Oct 19, 2021

in test/functional/wallet_multisig_descriptor_psbt.py:88 in 9de0d94508

  83 | +        participants = {
  84 | +            # Every participant generates an xpub. The most straightforward way is to create a new descriptor wallet.
  85 | +            # This wallet will be the participant's `signer` for the resulting multisig. Avoid reusing this wallet for any other purpose (for privacy reasons).
  86 | +            "signers": [node.get_wallet_rpc(node.createwallet(wallet_name=f"participant_{self.nodes.index(node)}", descriptors=True)["name"]) for node in self.nodes],
  87 | +            # After participants generate and exchange their xpubs they will each create their own watch-only multisig.
  88 | +            # Note: these multisigs are all the same, this justs highlights that each participant can independently verify everything on their own node.

hebasto commented at 1:27 PM on November 7, 2021:

typo: justs ==> just

MarcoFalke referenced this in commit 87ce2d646b on Dec 14, 2021

sidhujag referenced this in commit b094bdfb15 on Dec 14, 2021

DrahtBot locked this on Nov 7, 2022