spesmilo / electrum

Electrum Bitcoin Wallet
https://electrum.org
MIT License
7.44k stars 3.09k forks source link

Electrum asks for an xpub when only Zpub is valid #5688

Open merland opened 5 years ago

merland commented 5 years ago

While setting up a multisig p2wsh scheme, Electrum asks for an xpub where a Zpub should be entered. I suppose an "extended public key in the Zpub format" could be referred to as "an xpub" in everyday speech, but in the Electrum context it will confuse the user.

Steps to reproduce:

File --> New/Restore --> Next

Select "Multi Signature Wallet" --> Next

Leave at 2-of-2 --> Next

At "Add cosigner (1 of 2)", select "I already have a seed" --> Next

Enter a seed. (Example: convince basket olive hip main story august perfect moment strong bind ketchup erosion among huge stadium nature rigid amazing dose crunch rescue then abstract) --> Click Options --> check "bip39 seed" --> OK --> Next

At "Script Type and derivation Path", leave at "native segwit multisig (p2wsh)" --> Next

Master Public Key --> Next

At "Add cosigner (2 of 2)", select "Enter cosigner key" --> Next

Electrum displays:

Add cosigner 2
Please enter the master public key (xpub) of your cosigner. Enter their master private key (xprv) if you want to be able to sign for them.

This instruction is wrong or confusing, since a key with the prefix xpub is not expected. Only a key with prefix Zpub will be accepted here. (I didn't test with any private keys though)

The error message given, if an xpub is supplied:

Cannot add this cosigner
The key type is 'standard', we are 'p2wsh'

Example xpub (invalid): xpub6EN8WPx17YJcPqPtp299PHVEAxH26PgJnXwJFf6RhrqeHu6jGiVi9FuWL3bg3qpkTXLwPSdX6QaVPYGkz4s8VRmqea3UUkJ7HB9YCh5u3oy

Example Zpub (valid): Zpub74vkEy2GyrwwWzwWRQBNdY23EgcBBzLtw2dQji9jqeRp2HJ6YSD7UW5iBAUKc6MVWGeXkypktwg6HH7eigr7EPFiDiYj3yQ5iMYhZoiNApm

SomberNight commented 5 years ago

The keyword "xpub" is overloaded. Sometimes by xpub we (or even people in general) mean any kind of master public key. So not specifically a string that starts with "xpub..." but all of {x,y,z,Y,Z} || "pub". This might be sometimes confusing though.

merland commented 5 years ago

@SomberNight Yes, that's kind of what I meant with the "everyday speech" part. Maybe an addition could be made, giving the user the exact prefix(es) valid in every particular case.

merland commented 5 years ago

The smallest change that would make a considerable improvement would IMO be to simply remove the `(xpub)´ from the instruction. That would remove a lot of the confusion. Suggested change:

Please enter the master public key (xpub) of your cosigner to Please enter the master public key of your cosigner

fresheneesz commented 4 years ago

@merland I'd instead recommend changing it to Please enter the master public key (Zpub) of your cosigner - users should be told specifically what's required. We should probably also label the Master public key in "Wallet Information" to say whether its an xpub, ypub, zpub (etc?)

I was confused by this myself. I tried putting in a vpub from a normal wallet in, and it tells me "Wrong key type p2wpkh". I guess electrum doesn't support segwit cosigners yet?

SomberNight commented 4 years ago

I was confused by this myself. I tried putting in a vpub from a normal wallet in, and it tells me "Wrong key type p2wpkh". I guess electrum doesn't support segwit cosigners yet?

You need a Zpub for that. see e.g. https://github.com/spesmilo/electrum-docs/blob/master/xpub_version_bytes.rst#specification You can convert version bytes using the convert_xkey CLI command, or if you have the seed, when you enter it into the wizard (having pre-selected multisig), it will give you the Zpub. For motivation re the distinct types, also see the above link.

merland commented 4 years ago

@fresheneesz I agree, your suggestion is more clear. In my last comment, I was focusing on what would be the smallest change with the least possible side effects. (Maybe that same string is used in other scenarios)

merland commented 4 years ago

@SomberNight @fresheneesz For xpub -> Zpub conversion, you can also use SeedPicker (Shameless plug, by a co-creator...)

fresheneesz commented 4 years ago

Ah hm, seems like we should automatically convert so that people don't have to care whether they put in a vpub or Zpub. Is that something you think might be easy to add (using code from SeedPicker) @merland ?

merland commented 4 years ago

@fresheneesz Yes that would be the best solution. It did strike my mind but I was thinking it was too big of a bite to take. The xpub conversion logic is pretty simple but I can't judge how easy it would be to incorporate it into the Electrum UX flow. It would sure be a really helpful feature.

For reference, another implementation of the xpub conversion logic is this tool written by @jlopp.

@mflaxman may have some thoughts on this(?)

SomberNight commented 4 years ago

Ah hm, seems like we should automatically convert so that people don't have to care whether they put in a vpub or Zpub

So you suggest stripping the type. Why do you think the master key is typed in the first place? Chesterton's fence comes to mind.

Reading the Motivation and Considerations sections here is a good start.

fresheneesz commented 4 years ago

So my understanding based on the info here is that the type denotes what kinds of things the wallet supports. So stripping that type is likely not safe. According to that info, a ypub (or Ypub? Still not sure the difference.) should be convertible to both a zpub or xpub, but xpub nor zpub can be safely converted to any other type. I could have my understanding totally wrong, but that's what it looks like to me.

So, perhaps the best we can do is just tell people they need a zpub here. Or I suppose we might be able to change the logic to support both BIP32 and BIP49 in this particular flow, in which case we could allow this part to accept any of xpub, ypub, or zpub. I wouldn't be surprised if there were additional complications/constraints here that prevents some of that.

SomberNight commented 4 years ago

Typically, wallets use different derivation paths for different types of scripts. If you just convert the header bytes in a master key (e.g. xpub -> zpub), the derivation path will remain unchanged, which is often not what you would want.

The zpub vs Zpub distinction is there so that public keys are not accidentally reused between p2wpkh and p2wsh multisig. Electrum typically uses different derivation paths when giving out a zpub and a Zpub, and if you were to just convert the header bytes pubkeys would get reused (and funds might not be found in some cases).

Further, if a cosigner gives you a Zpub, they might specifically want you to construct p2wsh multisig scripts. If you changed the header bytes to a say Ypub and constructed different scripts, that would go against what they want and expect (and funds might get lost).

merland commented 4 years ago

@SomberNight Makes sense, thanks for clearing that up. So, I guess this issue is back at authoring a less confusing message.

fresheneesz commented 4 years ago

I'm again coming back to this. Why are the master public keys of single-seed wallets not usable as multisig cosigners? If I create a segwit single-seed wallet, I get a zpub master public key, and if I create a multisig segwit wallet, it gives me a Zpub master public key. Why is this the case? It makes it a lot harder to create multisig wallets and makes use cases like using the same seed for a single-seed and multisig wallet harder.

Beyond the context of why is it like this today, the real question is: can we simplify this for the future? Would unifying the master public key types be possible today? Would it only be possible with a protocol change?

SomberNight commented 4 years ago

@fresheneesz This was possible pre-segwit with "xpubs". We made a design decision to break it going forwards, for two reasons mainly:

(1) to avoid pubkey-reuse. The xpub specifies what public keys to use, so reusing it between a singlesig wallet and as part of a multisig results in reusing the pubkeys. This is visible on the chain and has mostly the same consequences as "address-reuse". It still happens if you reuse a Zpub between different multisigs though.

(2) Users often used to just take one xpub from a multisig, restore a wallet from it, and receive coins there. Most notably, many Electrum 2FA users did this, due to lack of understanding (Electrum 2FA uses 2-of-3 multisig, with one hot user key, one cold user key, one hot service key). zpub vs Zpub suffices to categorically fix this type of user-error.

If you look at (2), I think what you would like (making it easy and convenient), and what we would like (not being a footgun) are fundamentally contradictory :/

ecdsa commented 4 years ago

IIRC there was an instance of (2) where the user was receiving coins on an xpub for which they did not have the private key

fresheneesz commented 4 years ago

Thanks for the explanation!

  1. I see, you're saying the multisig wallet generates the same keys based on the master public key as single sig wallets do. Is there any way to modify how addresses are generated with that master public key (like derivation paths do)?

  2. I don't quite get this one. You're saying users who don't know how a multisig wallet worked tried to restore a single sig wallet with one of the public keys they didn't have the private key to? Are you talking about a watching only wallet?

Well, ok, let's take zpub vs Zpub as something that can't be changed. We could list both the zpub and Zpub addresses for a seed alongside each other in the wallet settings so that the same flow I'm talking about is possible without the foot gun. As long as its clear what these public keys are and they're made easily available, working with them should be pretty easy. Its super confusing to refer to something as the "master public key" for your seed, when there could be multiple master public keys (zpub vs Zpub). And also, zpub vs Zpub is just confusing naming - who decided capitalization of the Z would be the distinguishing characteristic? Maybe we should call it the single sig master public key and the multisig master public key.

ecdsa commented 4 years ago

I decided that the master public keys should have different prefixes, such as zpub, Zpub. That's just a prefix, not a naming decision.

Regarding your suggestion to list both, it would increase key reuse, and complicate user experience ("which one should I use??")

Less is more

fresheneesz commented 4 years ago

I decided that the master public keys should have different prefixes

I think that's a good idea, but are these not standard? Eg, what if someone wants to use the master public key from some other wallet software?

Regarding your suggestion to list both, it would increase key reuse

What do you mean by "key reuse"?

complicate user experience ("which one should I use??")

If we tell users to use the Zpub and their info lists a "zpub" and a "Zpub" it will be very obvious which one they should use. I see this as simplifying the user experience, not complicating it. Right now it just says "use your master public key" or "use an xpub", and that has a far greater "which one should I use?" problem than listing both types of master public keys for a seed. But even if that messaging were improved so that it specifically asks for either a zpub or a Zpub, I don't believe it is simpler to require a user to go through the multisig dialog for a seed they already have a wallet for. In the case of an on-device seed (eg not a hardware wallet), it requires that they input their seed again in a new wallet creation process, which presents more opportunity for the seed to be compromised. Users (myself included) might also expect to be able to obtain the Zpub for their seed inside a wallet already created with that seed, and would be confused (as I was) that its nowhere to be found.

In short, I think the current requirement that every user of a new multisig wallet go through the process of new wallet creation is the more complicated process. Thinking of users who may want to use the same seed for many different multisig wallets (some of which they may not be given viewing access to), the new wallet creation process is cumbersome. I truly think having the Zpub alongsize the zpub in your wallet info more faithfully embodies "less is more" than the current system.

ottomorac commented 1 year ago

I faced this exact issue today and were it not for @merland reporting this issue it would have been harder for me to google this to figure out what was going. Now granted I was porting from a bitpay wallet so ok the xpub vs Zpub is also something I had to learn. :-p

I also agree with @fresheneesz that changing the menu text toPlease enter the master public key (Zpub) of your cosigner would be helpful.

deepsimulation commented 1 year ago

Hi,

I ran into this issue recently. I was trying to import P2WSH multisig wallet, there was no way to specify p2wsh as script type for software before I started providing master keys (while that option shows up for hardware device, that preference can be made mandatory for all multisig wallets). Most folks (i believe) don't know that different prefixes exist or understand their differences. You can point them to: https://github.com/satoshilabs/slips/blob/master/slip-0132.md#registered-hd-version-bytes & help them understand, but they need a way to convert from one format to other especially if their wallets use incorrect / default version bytes (like xpub, tpub) for all wallet types.

I could think of some potential options to solve this:

  1. Allow users to specify the script type right after they select multi-signature number of co-signers & auto-convert the formats.
  2. If user enters xpub instead of Zpub when the previous key was imported with Zpub, then auto-convert to Zpub instead of throwing an error, giving the ability for users to toggle b/n versions (similar to Sparrow Wallet).
  3. Support output descriptors which encodes all this information :-) https://github.com/spesmilo/electrum/issues/8657
Screen Shot 2023-10-22 at 00 36 38
deepsimulation commented 1 year ago

@SomberNight what do you think about this?

grewar commented 6 months ago

Hi, I'm having trouble understanding something about zpub and Zpub.

Here's the scenario:

  1. I create a single signature SegWit wallet, receive a seed, and obtain a zpub in the wallet details.
  2. Then, I use the seed from that single wallet to create a multisig wallet, and I receive a Zpub.

Based on what @SomberNight mentioned earlier, I can convert the zpub from the single wallet to a Zpub that I can use for creating the multisig, using convert_xkey (instead of entering my seed in the multisig wizard in step 2).

However, when I use convert_xkey, I get a Zpub that is different from the Zpub I get in the multisig wizard in step 2. Why is this happening?

SomberNight commented 6 months ago

However, when I use convert_xkey, I get a Zpub that is different from the Zpub I get in the multisig wizard in step 2. Why is this happening?

When you use an electrum "segwit"-type seed for singlesig vs for multisig, different derivation paths are used. This is to avoid reusing the same public keys inside the onchain-visible bitcoin scripts, to protect your privacy. Converting using convert_xkey simply changes the header bytes in the xpub.