Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement output script descriptors #5694

Open
SomberNight opened this issue Oct 10, 2019 · 5 comments
Open

implement output script descriptors #5694

SomberNight opened this issue Oct 10, 2019 · 5 comments
Assignees
Labels
topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py
Milestone

Comments

@SomberNight
Copy link
Member

We should consider implementing support for output script descriptors.

See:

These would most likely replace the ypubs/zpubs we currently use (deprecate them).
These would allow e.g. importing and spending from p2pk scripts, or non-HD multisig scripts.

Perhaps more importantly however, when handling PSBTs, signers will often want a derivation path prefix for an xpub and the root xpub's fingerprint. This data is not contained in ypubs/zpubs. Hence, e.g. a wallet created from a zpub cannot create a PSBT that a coldcard will sign; and it's even worse in case of multisig wallets (see #5672 (comment)).


Unfortunately Ypubs/Zpubs don't map neatly onto output script descriptors. An output script descriptor describes the whole output script, so e.g. often contains multiple xpubs. Hence a Zpub is basically a partial descriptor.
Further, our wallet code given an xpub always creates an m/0/i, m/1/j hierarchy (where the latter is for change); but this can only be described via two output script descriptors, not one.

The above means we might have to change the wizard (UI) flow, for example.

To illustrate, see example taken from HWI docs:

$ ./hwi.py -f 8038ecd9 getkeypool --wpkh 0 1000
[
  {
    "desc": "wpkh([8038ecd9/84h/0h/0h]xpub6DR4rqx16YnCcfwFqgwvJdKiWrjDRzqxYTY44aoyHwZDSeSB5n2tqt42aYr9qPKhSKUdftPdTjhHrKKD6WGKVbuyhMvGH76VyKKZubg8o4P/0/*)#36sal9a4",
    "internal": false,
    "range": [0, 1000],
    "timestamp": "now",
    "keypool": true,
    "watchonly": true
  },
  {
    "desc": "wpkh([8038ecd9/84h/0h/0h]xpub6DR4rqx16YnCcfwFqgwvJdKiWrjDRzqxYTY44aoyHwZDSeSB5n2tqt42aYr9qPKhSKUdftPdTjhHrKKD6WGKVbuyhMvGH76VyKKZubg8o4P/1/*)#nl2rc26w",
    "internal": true,
    "range": [0, 1000],
    "timestamp": "now",
    "keypool": true,
    "watchonly": true
  }
]
@SomberNight SomberNight added the topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py label Oct 10, 2019
@SomberNight
Copy link
Member Author

It would make sense to implement PSBT support before this: #4615
(but note, that as arguments in OP suggest, "proper" PSBT support kind of needs this)

@SomberNight
Copy link
Member Author

SomberNight commented Oct 18, 2019

Unfortunately Ypubs/Zpubs don't map neatly onto output script descriptors.

Consider the below 2of2 multisig example.

This is how Bitcoin Core / HWI is describing a "wallet":
(some fields here, e.g. checksums, are incorrect, it's for illustrative purposes only)

two descriptors, one for change ("internal"), one for receiving addresses

[
  {
    "desc": "wsh(sortedmulti(2,[abcd0123/48h/0h/0h/2h]xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/0/*,[deadb33f/48h/0h/0h/2h]xpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3ZuxV27CprR9LgpeyGmXUbC6wb7ERfvrnKZjXoUmmDznezpbZb7ap6r1D3tgFxHmwMkQTPH/0/*))#36sal9a4",
    "internal": false,
    "range": [0, 1000],
    "timestamp": "now",
    "keypool": true,
    "watchonly": true
  },
  {
    "desc": "wsh(sortedmulti(2,[abcd0123/48h/0h/0h/2h]xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/1/*,[deadb33f/48h/0h/0h/2h]xpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3ZuxV27CprR9LgpeyGmXUbC6wb7ERfvrnKZjXoUmmDznezpbZb7ap6r1D3tgFxHmwMkQTPH/1/*))#nl2rc26w",
    "internal": true,
    "range": [0, 1000],
    "timestamp": "now",
    "keypool": true,
    "watchonly": true
  }
]

Here is how in Electrum you would currently create an analogous wallet:
(Zpubs also fake, checksum is incorrect, just illustrating)

  • in wizard, choose multisig, select m and n for m-of-n (2 of 2 in this case)
  • enter cosigner "intermediate xpub" (Zpub specifically, to signal p2wsh), for cosigner1: Zpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB
  • enter cosigner "intermediate xpub" (Zpub specifically, to signal p2wsh), for cosigner2:
    Zpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3ZuxV27CprR9LgpeyGmXUbC6wb7ERfvrnKZjXoUmmDznezpbZb7ap6r1D3tgFxHmwMkQTPH
  • at this point we create a wallet that will implicitly use Zpub1/0/* and Zpub2/0/* for receiving addresses; and Zpub1/1/* and Zpub2/1/* for change addresses

Note that instead of an "intermediate xpub", the user can opt to e.g. enter their seed words, and we generate the xpub from that in that case. Further note that in this case, we show the user the intermediate xpub at this point; so that they can save/share their own xpubs with other cosigners.
>> Crucially I am pondering about what to show to the user at this specific point. <<

It is not clear how we could change this flow to end up using output script descriptors.

Also note that e.g. the checksum in the output script descriptor is only there for the "full"/"ready" descriptor. So whatever partial strings cosigners are sharing with each other would likely not fully be protected with a checksum.


We could maybe show individual users something like this?
[abcd0123/48h/0h/0h/2h]xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB

Problems(?) with this though:

  • checksum only for xpub itself
  • script type missing (or we could replace the xpub with a Zpub??)
  • /0/i and /0/j for receiving and change addresses respectively still implicit

(Or obviously we could change the whole wizard flow, but to what??)

@craigraw
Copy link

Given that output descriptors, being more extensible, will likely eventually become the default, this seems a problem of transition. The following comments suggest one approach and may be of use:

  • I think it would be clearer if the script type was explicitly chosen in an early multisig wizard step, rather than derived from the xpub header. This could be similar to the Segwit/Legacy choice elsewhere.
  • The Add Cosigner options "Use a master key" or "Enter cosigner key" are somewhat confusing in the current wizard when one has an intermediate xpub - I think xpub is probably a better recognised term now, but perhaps both could be used in the label.
  • I assume that it is the intention in the wizard step that captures the xpub to also capture the master key fingerprint and derivation path, without which PSBTs cannot be created.
  • Possibly this step should as it's first text field accept an xpub, Zpub or output descriptor (the latter describing all three fields). The derivation and fingerprint fields below then change/become non-editable depending on what is entered into the first field.
  • Should the user enter seed words, it seems that these three fields should again be displayed (xpub, fingerprint, derivation) depending the script type previously chosen. The xpub value could be a Zpub etc if the output descriptor is also shown (see next point).
  • Whenever these three fields are displayed there could be an option to display the output descriptor for the cosigner, perhaps as an uneditable label.
  • Before the step that asks for the wallet password, the entire configuration of the multisig wallet can be displayed, with the script type, policy (m of n), and the three fields for each of the cosigners. At this step the full output descriptor can also be shown e.g. wsh(sortedmulti(... This step also provides a place for buttons to export the wallet details to various formats (HWI, Coldcard etc).
  • It doesn't appear necessary to add the trailing child derivation to any output descriptor. I'd suggest it's safe to assume they are implicit. Not including them makes it more difficult to copy/paste directly to HWI, but I'm not sure the additional complexity is worth it.
  • Checksums on output descriptors are also optional, and perhaps best left off for simplicity if direct compatibility with HWI is not the goal.

@SomberNight
Copy link
Member Author

SomberNight commented Oct 18, 2019

  • Possibly this step should as it's first text field accept an xpub, Zpub or output descriptor (the latter describing all three fields). The derivation and fingerprint fields below then change/become non-editable depending on what is entered into the first field.
  • Should the user enter seed words, it seems that these three fields should again be displayed (xpub, fingerprint, derivation) depending the script type previously chosen. The xpub value could be a Zpub etc if the output descriptor is also shown (see next point).
  • Whenever these three fields are displayed there could be an option to display the output descriptor for the cosigner, perhaps as an uneditable label.

You seem to be missing the point. There does not seem to exist a format for an output descriptor "for a cosigner". Output descriptors can be used when all the cosigners are known but seemingly not before. This is the concern I was expressing above.

Given that output descriptors, being more extensible, will likely eventually become the default, this seems a problem of transition.

Well that's the thing; I don't think it is.

  • I assume that it is the intention in the wizard step that captures the xpub to also capture the master key fingerprint and derivation path, without which PSBTs cannot be created.

Yes, that is one point that would make this change rather urgent... :/

@craigraw
Copy link

Apologies, I did miss the point, and assumed your suggestion of a partial output descriptor (without a script) was reasonable. That said, it is perhaps more generally compatible at this time to show the three fields separately for each cosigner, and the output descriptor at the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py
Projects
None yet
Development

No branches or pull requests

2 participants