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

Electrum misinterprets master extended key (depth 0) as child extended key with depth 3 #3671

Closed
nym-zone opened this issue Jan 8, 2018 · 10 comments

Comments

@nym-zone
Copy link

nym-zone commented Jan 8, 2018

As briefly explained in my commit log at nym-zone/easyseed@c912169:

Expected behaviour

Wallet created from an “xprv”/“yprv”/“zprv” extended master private key matches wallet restored from BIP 39 mnemonic phrase plus appropriate BIP 32 derivation path.

Actual behaviour

Wallet restored from an “xprv”/“yprv”/“zprv” extended master private key generates/contains addresses different from that restored from the mnemonic as aforesaid. This may cause user loss of funds: If a user creates a wallet from an extended master private key, and uses the matching BIP 39 mnemonic phrase as a backup, then the backup will apparently be useless for restoring the wallet.

Example Test Data

The following “steps to reproduce” use this test vector, generated with easyseed:

Entropy:
50416ff29fdf0d7c1cdf7b076132c1c7

Mnemonic:
expect arena witness distance valid safe inflict urge alter another race moment

Seed:
c641850638402a5afb56e3252c1b4db2f4b15f3ae3e470335a2732baafd5704a6591d6a0103ca58011c020b82df665bf28d95b188f356436b581620cf6b1a57e

### Multiple formats of extended private key for use with Electrum.
### USE ONLY ONE OF THESE!

Old-style address (“1-Address”):
xprv9s21ZrQH143K2XmdMqcWE29L75KLystP5qeMrRh7NJZdKvdRykoR6wNVohA7yUeaRtK2vni2ybAAY7mt8QyAmJDt6EF7f7DhbHFNMit7keL

Segwit backward-compatible P2WPKH-nested-in-P2SH (“3-Address”):
yprvABrGsX5C9janspxkCCQ8S7EqH3TnvVsszxAadpazkJwWP2SfEQxyj12dpu7hyPJVqXRqgGJbSFWiRQPSr7PBZXuUxZwYF23Bs1K1kJTne9J

Segwit Bech32 New/Future Format (“Bravo Charlie One”):
zprvAWgYBBk7JR8Gj89s2ZBkeCLLT1cEs7sNv4goRDUt8KKPS8FtV58YM4gmr75HyHxRFAYeRju9tusGJh11ZooCMmb5pudxpvrg8jNf8rQJz5U

These can be reproduced with the following command, as of nym-zone/easyseed@c912169:

echo 50416ff29fdf0d7c1cdf7b076132c1c7 | xxd -p -r | \
	easyseed -E -A -b 128 -k -

Other test vectors were tried by me, from the set of official test vectors listed in BIP 39. E.g.:

Mnemonic: “hamster diagram private dutch cause delay private meat slide toddler razor book happy fancy gospel tennis maple dilemma loan word shrug inflict delay length”

Passphrase (Electrum “Extra Words”): “TREZOR”

Extended master private key:

xprv9s21ZrQH143K2XTAhys3pMNcGn261Fi5Ta2Pw8PwaVPhg3D8DWkzWQwjTJfskj8ofb81i9NP2cUNKxwjueJHHMQAnxtivTA75uUFqPFeWzk

Steps to Reproduce

  1. Create a wallet “test” in Electrum, using the “xprv”, “yprv”, or “zprv” extended master private key.

  2. Create another wallet “test_verify” in Electrum, using the BIP 39 mnemonic with the appropriate derivation path:

  • For “xprv”: m/44'/0'/0'
  • For “yprv”: m/49'/0'/0'
  • For “zprv”: m/84'/0'/0' (N.b. that I did not expect this to work, because BIP 84 was first published as a draft eleven days ago, on 2017-12-28. It creates a wallet full of old-style “1” addresses, rather than Bech32 “Bravo Charlie” addresses. The other two given derivation paths should certainly work.)
  1. Compare the addresses in the “test” wallet with those in the “test_verify” wallet.

Version Information

Tested with Electrum version 3.0.4.

Additional Information

I do not see how this could be a bug in my software. It is remotely possible that I could make a stupendously moronic mistake; but then, my software would not pass the test vectors as it does every make check (including “hamster diagram private dutch...” and dozens of other in English and Japanese). I tried to look through the Electrum codebase for some cause; but it is an unfamiliar codebase in an unfamiliar language, and I didn’t see anything obvious at a glance/grep through the BIP39/BIP32 parts of lib/keystore.py and lib/bitcoin.py.

After entering a BIP 39 mnemonic phrase into Electrum and clicking for the next screen, a bizarre small, blank window popped up. It did not go away, even when the Electrum process exited. I observe this on v3.0.4; I did not observe it on v3.0.3 or previous versions of Electrum, in which I have tested BIP 39 mnemonic entry.

To be clear: I strongly disagree with the nonstandard “xprv”/“yprv”/“zprv”/etc. scheme, which abuses BIP 32 serialization version numbers to convey information which should be contained in the derivation path. The same problem exists with “tpub”/“tprv” in the spec, insofar as testnet addresses also have their own derivation paths. An extended master private key (or public key) is not tied to only one type of address. However, I preliminarily implemented this in the hope of helping users adopt Segwit with the most popular userfriendly light client—for the good of users (lower fees), and for the good of the network.

@ecdsa
Copy link
Member

ecdsa commented Jan 8, 2018

you derived xprv/yprv/zprv at the root level.
this is not how this spec works.

@ecdsa ecdsa closed this as completed Jan 8, 2018
@nym-zone
Copy link
Author

nym-zone commented Jan 8, 2018

@ecdsa:

you derived xprv/yprv/zprv at the root level.
this is not how this spec works.

To which spec do you refer by “this” spec? Electrum’s? Is it written down somewhere, outside the Python code?

My code perfectly implements BIP 39, and the pertinent portion of BIP 32 for master extended private key generation and serialization. It passes all English and Japanese test vectors, including xprv generation, referenced in the BIP 39 standard; thus, those are also derived at the root level (depth byte 0x00).

If Electrum follows a different spec, I would appreciate a reference I could use to implement it properly. Should the “depth” byte be set to 0x01, and the child number set to 0x8000002c (xprv), 0x80000031 (yprv), 0x80000054 (zprv)? Or...? If so, what should the parent’s fingerprint be set to when there is no parent?

FWIW, I searched the Electrum issues for pertinent keywords before I opened a new issue—and just now, again. Also, obviously I found the place in the Electrum code which contains the magic numbers for yprv and zprv; had a comment there explained or referred to a spec, I would have followed it. It is not as if I didn’t do my homework before writing an implementation, nor as before opening an issue.

@ecdsa
Copy link
Member

ecdsa commented Jan 8, 2018

we will release a BIP shortly
in the meantime please read
http://docs.electrum.org/en/latest/seedphrase.html#list-of-reserved-numbers

@nym-zone
Copy link
Author

nym-zone commented Jan 8, 2018

@ecdsa:

we will release a BIP shortly
in the meantime please read
http://docs.electrum.org/en/latest/seedphrase.html#list-of-reserved-numbers

Thanks for your reply. But this only lists the same magic numbers as I found in Electrum’s lib/bitcoin.py, and added to my code yesterday.

If I did not have those version numbers, then I would not have obtained “yprv” and “zprv” from the base58check encoding. I did not just slap those on, you know.

Now as for xprv, please try Electrum against the BIP 39 test vectors. My software passes. But with the official test vectors, Electrum 3.0.4 gives different results for (0) restoring wallet from xprv, versus (1) restoring wallet from BIP 39 mnemonic plus passphrase. I quoted one in my original comment on this issue (mnemonic: “hamster diagram private dutch...”, passphrase: “TREZOR”; versus xprv xprv9s21ZrQH143...). I tried this in Electrum; did you?

The input I feed to to base58check is laid out as such:

  • [4] version (the part which makes “xprv”, “yprv”, “zprv” after base58check encoding)
  • [1] Depth, set to 0x00 “for master nodes”, per BIP 32.
  • [4] The parent’s fingerprint, “0x00000000 if master key” per BIP 32.
  • [4] The child number. “0x00000000 if master key”, per BIP 32. This is what I asked about, if Electrum expects it to be 0x8000002c (44 decimal with the high bit set) for xprv, 0x80000031 (49 with the high bit set) for yprv, or 0x80000054 (84 with the high bit set) for zprv. This is in the middle of the string, not at the beginning and not the version code. The spec says it should here be zero; and test vectors agree with me.
  • [32] The chain code.
  • [33] The public key or private key data, encoded as specified by BIP 32.

That passes the test vectors.

@dabura667
Copy link
Contributor

I think @ecdsa is saying you need to use m/44'/0'/0' xprv and not m/ xprv to recover, just like the xpub.

So your zprv should be derived:

  1. get root zprv
  2. derive m/84'/0'/0'
  3. export that node (depth:3 index: 0x80000000) as a zprv base58check.

@dabura667
Copy link
Contributor

yup. I was able to get the same addresses by:

> var btc = require('bitcoinjs-lib')
// import your root xprv you gave
> var hd = btc.HDNode.fromBase58('xprv9s21ZrQH143K2XmdMqcWE29L75KLystP5qeMrRh7NJZdKvdRykoR6wNVohA7yUeaRtK2vni2ybAAY7mt8QyAmJDt6EF7f7DhbHFNMit7keL')
// derive m/44'/0'/0' and export as base58check
> hd.deriveHardened(44).deriveHardened(0).deriveHardened(0).toBase58()
'xprv9z7KYqivhqD6FHfsDLbFEPDfWFt6xHzV1j3fG4rHoV4w2ppyd74aY8UwywJb8eqxAoJ7NqiDp1dCsYzicgnGPkhmhbLZkMnWhMwUxa23Uah'

And recovering from that.

@nym-zone
Copy link
Author

nym-zone commented Jan 8, 2018

I think @ecdsa is saying you need to use m/44'/0'/0' xprv and not m/ xprv to recover, just like the xpub.

So your zprv should be derived:

  1. get root zprv
  2. derive m/84'/0'/0'
  3. export that node (depth:3 index: 0x80000000) as a zprv base58check.

Thank you, @dabura667. I will try adding code for this later, if I want to support the Electrum use cases I’d intended.

The question then is why Electrum will accept a master extended private key with depth 0, and neither reject it, nor ask the derivation path and follow it (as it does from a BIP39 mnemonic). It will simply spit out entirely different addresses. This is a bug.

Neither considering my code nor the new Segwit yprv/zprv code:

You wrote whilst I was experimenting with Electrum, and gathering precise reproduction details and screenshots using a test vector from Trezor’s vectors.json. If Electrum will accept both of these and yet generate totally different addresses, then that’s a bug.

Following are the exact steps I followed, starting with an empty .electrum directory, with results and screenshots.

xprv from vectors.json

  1. Wallet name: “from_xprv”
  2. [radio button] Standard Wallet
  3. [radio button] Use public or private keys
  4. Enter the following from Trezor’s vectors.json:
xprv9s21ZrQH143K2XTAhys3pMNcGn261Fi5Ta2Pw8PwaVPhg3D8DWkzWQwjTJfskj8ofb81i9NP2cUNKxwjueJHHMQAnxtivTA75uUFqPFeWzk
  1. Wallet password: leave blank

Wallet addresses, starting from the first:

1ChUYDT2hHsn3uzfSKiZQYFRwTFkc1Nucn
15zPySjyph5wE5W2kbnPKWzTjdgBHeQXre
19EpKxqjoYn4CzFhgmfXQxJygSP5J7iyVC
1Q23FLhZ6h7TtJ6bfLS9GNQi59Qxmr3DP6
1GQ92yy3iR2crF3WDiX9hdU8V7bNfddS1N
[...]

electrum_from_xprv

Seed phrase from vectors.json

  1. File→New/Restore
  2. Wallet name: “from_bip39”
  3. [radio button] Standard wallet
  4. [radio button] I already have a seed
  5. Options button: Check both boxes, “Extend this seed with custom words” (for passphrase) and “BIP39 seed”
  6. Enter seed phrase: “hamster diagram private dutch cause delay private meat slide toddler razor book happy fancy gospel tennis maple dilemma loan word shrug inflict delay length” (checksum: ok)
  7. Seed extension: “TREZOR”
  8. Derivation: leave default m/44'/0'/0'
  9. Wallet password: leave blank

Wallet addresses, starting from the first:

1JocbdmrSq7UnydGJjVX83HjTTbvyUSm61
1J4Us3ijqVBPKjDYJCRvXRCwyx7RSk45RY
1KYk1sAgKh3sJzN8wExqAJRY5TG88jGDRP
1BHNRChgpTGUWa55Aqodw9kjnukaTLHPpV
1PF2YfmJvirVkcXx8G66LH6p19qd29qQAb
[...]

electrum_from_bip39

Suggested fix

Detect a master extended key (public or private) with depth 0, and interpose a dialogue asking the derivation path (just as the dialogue it uses for BIP39 phrases).

@nym-zone
Copy link
Author

nym-zone commented Jan 8, 2018

@dabura667, thank you for the information you provided. I confirmed on my end that the xprv you derived with bitcoinjs-lib generates the same addresses as obtained from BIP39 mnemonic restoration:

1Ei7ffqmSxzKk1tWV8oAKiktrKEnY7xD4Z
1PLpmMCb3c6dMKparSPaWru5HzQ54sih7E
1MencAr37B32cQG3cRvd4mLEEvXVdQfDyW
1G5UFJbHw8TGevuSBR74NKZTy4sFQwa2AR
1PT9LQtofGhP77PdQ5cbc5pc92cLfKARud
[...]

Now I know affirmatively which depth of key Electrum is expecting; so I can code to that. But if Electrum accepts the master extended key (depth byte 0x00, index 0x00000000) and simply gives the wrong addresses, then that is still a bug.

nym-zone added a commit to nym-zone/easyseed that referenced this issue Jan 9, 2018
In addition to previously undocumented features from c912169 and
37defc2, this commit does:

- Update the manpage to document new functionality, for full BIP 39
support and rudimentary BIP 32 support.

- Add option -j to read passphrase from file.  This is principally
intended for use with FIFOs in scripts, for generation of test vectors.

- Modestly refactor a few parts needed to make all passphrase
functionality consistent, and tighten up handling of options in main().

- Refuse to read -k keymat or -j passphrases from a tty.

- Add the undocumented -D option, which at present only causes the -A
output to include the passphrase (if any).  This is for generation of
test vectors.

- Hide the undocumented -E option behind an undocumented compile-time
knob, to prevent potential user funds loss.  See spesmilo/electrum#3671.

- Add a caveat to the manpage against using the output xprv with
Electrum.  See again, spesmilo/electrum#3671.

- Update the README.md to advertise new functionality.
@nym-zone nym-zone changed the title Mismatch between “xprv”/“yprv”/“zprv” wallet and BIP 39 restored wallet Electrum misinterprets master extended key (depth 0) as child extended key with depth 3 Jan 9, 2018
@montvid
Copy link

montvid commented Feb 4, 2018

@ecdsa Thanks for the heads up about writing the BIP. I hope it will be up as soon as possible, please inform the general audience so we can push for a better standard than bip-39. Even the bitcoin core devs agree that bip-39 should be not implemented.

@leafcutterant
Copy link

I agree, this is still a bug.

I can enter the

  • BIP32 Root Key
  • Account Extended Private Key
  • BIP32 Extended Private Key

when restoring a wallet from what Electrum calls "master private key" and they are all accepted, generating totally different wallets.

From trial it looks like Electrum refers to the second (Account Extended Private Key).

(I used the nomenclature from Ian Coleman's implementation.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants