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

Need a way to represent a cosigner: (xfp, derivation prefix, xpub, script type) #5715

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

Comments

@SomberNight
Copy link
Member

SomberNight commented Oct 18, 2019

(This issue was in part split off from #5694 as it started to become off-topic there)

When handling PSBTs, signers will often want a derivation path prefix ("derivation prefix") for an xpub and the root xpub's fingerprint ("xfp"). 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)).

We want to use PSBTs, and we want to use them for everything; as in all partial transactions will be serialized in the PSBT format.

There are two issues here.


1. We need a way to represent an xfp, a derivation prefix, and an xpub

One issue is that as xpubs don't contain the xfp and the derivation prefix, we need to change the wizard flow to show/expect something that does. I considered using output script descriptors but turns out that does not work for this usecase: see #5694 (comment)
The reason, briefly, is that an output descriptor can be used to describe a "ready" output, but not a partial one, which is what a cosigner would have during wallet creation.

I would rather not come up with a completely custom serialization format for this, but use something that already exists. Except there isn't anything it seems. Well, there are output descriptors.
So we could reuse the format that appears there; which is a subpart of a descriptor; specifically, instead of an xpub we could have something like this:

[abcd0123/48h/0h/0h/2h]xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB

i.e. an xpub prefixed with the xfp and the derivation prefix; to represent a cosigner.

Further note that we would like to still encode the script type somewhere; so the easiest thing for the moment would be to keep using the {x,y,z,Y,Z}|{pub,prv} stuff we've been using since 3.0.
So e.g.:

[abcd0123/48h/0h/0h/2h]Zpub69H7F5d8KSRgmmdJg2KhpAK8SR3DjMwAdkxj3ZuxV27CprR9LgpeyGmXUbC6wb7ERfvrnKZjXoUmmDznezpbZb7ap6r1D3tgFxHmwMkQTPH

(for illustrative purposes only; this ^ Zpub is not valid)

This is ~almost a subset of what the descriptor doc calls a KEY expression; except

  • that we would implicitly use this key to create a x/0/i, x/1/j subtree with the chains used for receiving and change addresses respectively
  • and that we would use y/z pubs instead of only what bip32 describes

One crucial observation is that the xpub itself, according to bip32 contains:

  • the depth
  • fingerprint of direct parent
  • child index (last level in derivation prefix)

Which means that, if the xpub is at depth 0 or depth 1, both the root fingerprint and the derivation prefix can be calculated just from the xpub.

Hence, the wizard could accept:

  • prefixed xpubs
  • xpubs without prefix IF depth is either 0 or 1

When showing a "master public key" to the user, we could do the same if we wanted,

  • either show prefixed xpubs in all cases
  • or maybe only show prefixed xpub if depth is not 0 or 1

2. We need to upgrade existing wallets

Regarding upgrading existing wallets:

  • Electrum seeds and xpubs corresponding to electrum seeds all use depth 0 or 1, hence they can be cleanly upgraded.
  • Hardware wallet keystores have the derivation prefix stored, but not the root fingerprint (existing hw wallets never needed this, except the coldcard, and they implemented storing this just for coldcard keystores from the get-go).
    • We could conceivably implement a way where the user is asked to connect their hw device, or we just opportunistically wait for that to happen, and then we can learn this data from the hw device. Depending on exact details, I am not sure if there is a clean way without drawbacks of doing this.
  • Other bip32 keystores, including those generated from a bip39 seed, or those generated from an intermediate xprv or intermediate xpub, do not have either the root fingerprint or the derivation prefix stored currently.
    • For bip39 seeds, we could prompt the user to enter their bip39 seed for a storage upgrade; or similar... Not ideal if it's mandatory (watch only???....). In any case, we cannot even tell if the wallet was derived from a bip39 seed at all; as that fact is not stored; and if it wasn't there might not even exist a bip39 seed that corresponds to the xpub.

So, for any bip32 keystore, exactly if the xpub depth is 0 or 1, we can reconstruct the missing data.

For the other cases, note that we could fake the xfp and the derivation prefix. See #5672 (comment)
For example, we could set the master fingerprint to the fingerprint of the intermediate xpub, and fill the derivation prefix with 0xffff_ffff ints. Note that, as above, from the xpub, the depth, the direct parent's fingerprint, and the child index can be read out, so it would be wise to fake the rest such that it is plausibly consistent with these fields.
Specifically, the last index in the derivation prefix should be set to the child index; and the depth should be consistent with the derivation prefix length.
If we do fake the missing data for these bip32 keystores, we should put a marker in the wallet file so that later we can tell we have done this.

An important thing to mention is that if we fake the data, basically all current usecases / user stories in Electrum supported currently should keep working. As the reason we don't have this data in the first place is that it was never needed before. Interoperability with other software via PSBTs might in some cases break however (but there was no interoperability before...).

(Although another point is that in the future, if e.g. Trezor starts using PSBTs in its wire protocol, they will want root fingerprints to be correct and at that point the wallet files for which we faked the xfp would stop working (if they are communicating with a Trezor device).)


This issue needs to be handled in some way as part integrating PSBT support into Electrum.
The reason I am concerned right now, is that I have a ~private (WIP) branch that integrates PSBT support, atm it's a ~6k line diff, and it touches lots and lots and lots of parts of the code, and it is very prone to become conflicted with any change on master basically. Which means I would like my branch merged soonish, which means this issue needs to be handled.....

What I currently suggest is to do the conclusion of point (1), i.e. to show prefixed xpubs to users; and re (2) to cleanly upgrade bip32 keystores if they are at depth 0 or depth 1 (all electrum seeds), and to fake the missing data for other bip32 keystores along with putting a marker in the wallet file to signal that we did the faking.

@SomberNight SomberNight added the topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py label Oct 18, 2019
@SomberNight
Copy link
Member Author

SomberNight commented Oct 18, 2019

Re replacing xpubs with prefixed xpubs such as
[abcd0123/48h/0h/0h/2h]xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB
One drawback is that this is already a valid KEY expression according to the output descriptors doc, that describes specifically that key (as a leaf).
To avoid overloading this, what would be really nice is syntax for specifically the m/0/i (receive), m/1/j (change) hierarchy.
I think basically all light wallets use this hierarchy atm; so maybe it could be special cased.

SomberNight added a commit to SomberNight/electrum that referenced this issue Oct 28, 2019
SomberNight added a commit to SomberNight/electrum that referenced this issue Oct 29, 2019
SomberNight added a commit to SomberNight/electrum that referenced this issue Oct 29, 2019
SomberNight added a commit to SomberNight/electrum that referenced this issue Oct 29, 2019
SomberNight added a commit to SomberNight/electrum that referenced this issue Oct 29, 2019
@SomberNight
Copy link
Member Author

Note: changing the xpub of keystores in existing wallet files might have unforeseen implications... For example, the labels plugin relies on the exact xpub string of wallets.

SomberNight added a commit to SomberNight/electrum that referenced this issue Oct 30, 2019
@ecdsa
Copy link
Member

ecdsa commented Oct 31, 2019

To follow-up on yesterday's conversation:

  • For per-input and per-output derivations path in BIP174, I believe that "master key fingerprint" does not necessarily refer to the root master key. Instead, I believe it can be any master key from which the pubkey can be derived.
  • In contrast, the a global master key must have the root fingerprint, because it says "the number of [...] indexes must match the depth".
  • The updater of a PSBT may add a global master key declaration, if it knows the root fingerprint. As far as I understand, this would be required for Coldcard with its current firmware. For most use cases, however, this is not even needed, because the Signer can recognize the fingerprint present in each input/output.

Thus, my opinion is that we do not need to change how cosigners are represented/shown to users for the moment. We can keep passing {x,y,z,Y,Z}pubs between cosigners in the wizard, and we can keep building wallets from them. I think we should wait until we implement script descriptors, before changing that.

Your mentioned that signers "will often want [...] the root fingerprint". I think that it is too early to know that. It seems that currently the only use case would be for passing a PSBT around between several Coldcard wallets, using the SD card, because there would be no updater that can add the global root fingerprint and derivation. I believe that, at this point, that single use case does not justify to change the wizard for all the rest of our wallets.

@SomberNight
Copy link
Member Author

The updater of a PSBT may add a global master key declaration, if it knows the root fingerprint. [...] For most use cases, however, this is not even needed, because the Signer can recognize the fingerprint present in each input/output.

Note that the global xpub field must be there for all cosigners in the case of multisig inputs/outputs before e.g. a hw device signer would be willing to sign. Trezor for example checks that the inputs (pubkeys) and the change outputs (pubkeys) can be derived from the same intermediate xpub; this of course needs all intermediate xpubs available.

I think we should wait until we implement script descriptors, before changing that.

At this point I think that script descriptors are largely unrelated from this issue.

SomberNight added a commit to SomberNight/electrum that referenced this issue Oct 31, 2019
@SomberNight
Copy link
Member Author

SomberNight commented Oct 31, 2019

Note: current thinking is that we will postpone this.
For future reference, here is a commit that implements accepting and displaying master keys with key origin info: SomberNight@591ed4a

There, keys look like this: [8fb71734/49'/1'/0']upub5EqSD23vFpLeJhABec18LiRcHNs3ghVcw1HnLYt2F3bnxUMbRgzQWXZ89JzEKf8NHiKxSdhrYXBurFU8DPdXfAowikwwYbzcfbBgd7NoWji

@Sjors
Copy link

Sjors commented Feb 13, 2020

See my braindump: bitcoin/bitcoin#18142

SomberNight added a commit that referenced this issue Feb 27, 2020
Power-users that know what they are doing can use this method
to populate key origin information for keystore (bip32 root fingerprint
and derivation path prefix).
Try to make method hard to misuse.

Qt console can now be used as e.g.:
```
wallet.get_keystores()[2].add_key_origin(derivation_prefix="m/48h/1h/0h/2h", root_fingerprint="deadbeef")
```

related #5715
related #5955
related #5969
@SomberNight SomberNight added this to the backlog milestone Apr 1, 2020
@benma
Copy link
Contributor

benma commented Nov 25, 2020

Copying my comment from #6780 (comment) for future reference:

You probably want descriptor derivation prefixes to point to the account-level xpubs, and the suffixes to the account addresses, e.g. for a receive address:

wsh(sortedmulti(2,[19542eb0/48h/0h/0h/2h]xpub6EJ2DSMhBRWUiZ3648zS39hMnraMToxPLMd8qntY8se7oUzra1iTw43KKaR9KehAGtb7zYLw9veHiBPegdb1dDoHsN736KFXsioPqVNXoRZ/0/0, [e81a5744/48h/0h/0h/2h]xpub6Duv8Gj9gZeA3sUo5nUMPEv6FZ81GHn3feyaUej5KqcjPKsYLww4xBX4MmYZUPX5NqzaVJWYdYZwGLECtgQruG4FkZMh566RkfUT2pbzsEg/0/0))'

Reasoning:

  • Compatible with the descriptor syntax, including the root fingerprint, derivation prefix (account-level) and suffix (change/address): https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md
  • Only one descriptor needed to describe the whole account. This is important for registration on offline signers (HW wallets such as the BitBox02), where verification happens during registration.

@SomberNight
Copy link
Member Author

The question is what to expect users to copy-paste into the wizard for a given cosigner -- which we would also show them in the wizard, and which we might also want them to backup.

Although what to backup might be different: it is presumably okay to only do a backup after the wallet is created, at which point output descriptors are at least available.
But note that during multisig wallet constructors, descriptors are not even available! You would need to put "?" marks for missing cosigners or something.

wsh(sortedmulti(2,[19542eb0/48h/0h/0h/2h]xpub6EJ2DSMhBRWUiZ3648zS39hMnraMToxPLMd8qntY8se7oUzra1iTw43KKaR9KehAGtb7zYLw9veHiBPegdb1dDoHsN736KFXsioPqVNXoRZ/0/0, [e81a5744/48h/0h/0h/2h]xpub6Duv8Gj9gZeA3sUo5nUMPEv6FZ81GHn3feyaUej5KqcjPKsYLww4xBX4MmYZUPX5NqzaVJWYdYZwGLECtgQruG4FkZMh566RkfUT2pbzsEg/0/0))

  • that descriptor describes a single address, so it is not actually compatible either: bitcoin core e.g. I imagine would create just that address, not the branches we want
  • also, during multisig wallet construction, we would need "?" question marks or something, as cosigners are missing initially. just think about how it works currently: people copy xpubs around, which works because an xpub only describes that cosigner, full output descriptors are not available until AFTER the setup completed

@craigraw
Copy link

One option is a 'partial output descriptor' - for example the two cosigners above would each be:

  • wsh([19542eb0/48h/0h/0h/2h]xpub6EJ2DSMhBRWUiZ3648zS39hMnraMToxPLMd8qntY8se7oUzra1iTw43KKaR9KehAGtb7zYLw9veHiBPegdb1dDoHsN736KFXsioPqVNXoRZ)

  • wsh([e81a5744/48h/0h/0h/2h]xpub6Duv8Gj9gZeA3sUo5nUMPEv6FZ81GHn3feyaUej5KqcjPKsYLww4xBX4MmYZUPX5NqzaVJWYdYZwGLECtgQruG4FkZMh566RkfUT2pbzsEg)

@benma
Copy link
Contributor

benma commented Nov 26, 2020

One option is a 'partial output descriptor' - for example the two cosigners above would each be:

* `wsh([19542eb0/48h/0h/0h/2h]xpub6EJ2DSMhBRWUiZ3648zS39hMnraMToxPLMd8qntY8se7oUzra1iTw43KKaR9KehAGtb7zYLw9veHiBPegdb1dDoHsN736KFXsioPqVNXoRZ)`

* `wsh([e81a5744/48h/0h/0h/2h]xpub6Duv8Gj9gZeA3sUo5nUMPEv6FZ81GHn3feyaUej5KqcjPKsYLww4xBX4MmYZUPX5NqzaVJWYdYZwGLECtgQruG4FkZMh566RkfUT2pbzsEg)`

That sounds like a misuse of descriptors. wsh doesn't mean much here. What about a kind of extension, like account([e81a5744/48h/0h/0h/2h]xpub6Duv8Gj9gZeA3sUo5nUMPEv6FZ81GHn3feyaUej5KqcjPKsYLww4xBX4MmYZUPX5NqzaVJWYdYZwGLECtgQruG4FkZMh566RkfUT2pbzsEg) (or identity() or cosigner() ...)?

@craigraw
Copy link

That's possible, but I disagree the wsh is meaningless - it signifies the intent for which the partial descriptor should be used, which I think is valid information a cosigner device (hww or similar) will want to convey. In this case, use in a P2WSH script.

@benma
Copy link
Contributor

benma commented Nov 26, 2020

What kind of P2WSH script?

In any case, I don't think the cosigner is the entity that wants to convey this info, but the coordinating wallet software. So it would get the identity descriptor and then assemble the final descriptor describing the whole setup.

@craigraw
Copy link

In order to generate the correct xpub at the appropriate default derivation, the script type would need to be chosen on the cosigner device. It seems a shame to lose this information when it could be used to ensure that the cosigner and co-ordinating wallet software agree on the type of wallet being created - for example the wallet software could display a warning if a different script type (and thus derivation) are being shared from the wallet it is creating.

@benma
Copy link
Contributor

benma commented Nov 26, 2020

It could possibly be helpful, but wsh(...) does not convey what script type is used. The script to be hashed is missing (e.g. sortedmulti(...)).

I stil don't see how it would be useful for the HW wallet to encode this in the initial step. The user decides the script type in the wallet, e.g. Electrum, and the setup only needs to be confirmed in the end on the HW wallet once the full script type is known.

@craigraw
Copy link

It's true that wsh only partially identifies the script type. It does however identify the address type. Lacking a BIP48, we don't know if the intention is for 48'/0'/0'/1' and 48'/0'/0'/2' derivations are meant only for standard multisig scripts. It seems unlikely to me, and the existing confusion in this area could only be greater should we try to have different 'standard' derivations for each type of custom script (for example custom time locks etc). It seems far more practical that all cosigners participating in P2WSH scripts should share a common derivation for their keys (unless otherwise specifically defined).

@SomberNight
Copy link
Member Author

note: bluewallet recently implemented multisig. During wallet setup, they show QR codes that encode text such as:

{
    "xfp": "70E21443",
    "xpub": "Zpub75r7nKHXcfHf5VWP9zGEk4SKkoKaaCBxKkfofq41Ln5inkVVy4DXYc4nuztRgnuDXMgDpnSqjsBCdNep9jk7ck9oqGwuHUrwo4x6HR3S95h",
    "path": "m/48'/0'/0'/2'"
}

@SomberNight
Copy link
Member Author

Note; for the time being, the power-user workaround is as-in a987a2b.

That is, the Qt console can be used as e.g.:

wallet.get_keystores()[2].add_key_origin(derivation_prefix="m/48h/1h/0h/2h", root_fingerprint="deadbeef")

@AndreasGassmann
Copy link

@SomberNight Thanks for the workaround through the console, that worked for me.

Is there now a standard how everything can be imported together? We are currently working on PSBT support in AirGap Vault and would like to add an option to encode the QRs in a way that is compatible with Electrum. (For both "watch-only" wallets and as a co-signer).

@craigraw
Copy link

There is a community working towards standards in airgapped data communications: https://github.com/BlockchainCommons/Airgapped-Wallet-Community. The standards themselves can be found here: https://github.com/BlockchainCommons/Research, including the UR encoding format and the use of this format for PSBTs.

@chri2
Copy link

chri2 commented Oct 14, 2021

To add to this: I'm new to all this and have been playing around with specter, electrum and lately specter-diy.

When I started playing with the specter-diy I tried to move a multisig setup from an electrum wallet onto the specter-diy. Since I didn't find any xfp I just invented one. Wouldn't work with the specter-diy: It just does not recognize that one of the xpubs of the imported multisig setup fits to its own seed.

This has been a funny situation for me as a beginner: I compared the multisig xpub of the specter-diy and compared it to the keys I imported for the multisig wallet setup - one key exactly matched the specter-diys xpub for multisig and even though specter-diy didn't recognize that it could sign for that xpub.

I opened an issue with specter-diy and they explained to me that I can't just put any bytes as the xfp in the []-part of the multisig xpub, because specter-diy recognizes the xpub it can sign for from the xfp of the imported xpubs.

I'm no way knowledgable to bitcoin and all its rules and standards. For me as a user it seems like that specter-diy and coldcard expect something valid by the standards, but electrum does not deliver and leaves the arising problems to the user to understand and solve.

I don't see why electrum should not at least offer an option to include the derivation path and xfp wherever it might be needed by other wallets to function alongside with electrum. But this is only the simplifying view of a user and I really do not want to have this understood as a position that I do not understand that there might be internal structures in code and data which do not allow to just do this as simple as I described.

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

7 participants