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?

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.

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:

1. 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: [ {} ],
}

2. 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

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.

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):

1. The watch-only multisig has funds I am trying to spend
2. 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)

@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.

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

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.

Concept ACK

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.

2. 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.

@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

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

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

Good point @Rspigler I removed the descriptor templates, but fixed as you suggest: 911ed48#diff-00ec9f97461f6bb94a37daf8069e5acf832ea4d5a31a8d7d4968c285d2fcac9bR144

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

ACK d1ce7bb (tests pass)

ACK e05cd05

Started review

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)

Concept ACK, very nice.

Concept ACK. Functional test is nice addition.

@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: f9479e4. I thought this made most sense, but I can squash it into 1f20501 (and the doc changes into 17dd657) 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: 9de0d94 highlighting some shortcomings of this basic example

@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.

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

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

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

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 9de0d94

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

Conflict with #23207?

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

Conflict with #23207?

Should be fixed in #23303.