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

Validating checksum when restoring from BIP39 seed #2128

Closed
kacperzuk opened this issue Jan 21, 2017 · 5 comments
Closed

Validating checksum when restoring from BIP39 seed #2128

kacperzuk opened this issue Jan 21, 2017 · 5 comments

Comments

@kacperzuk
Copy link
Contributor

kacperzuk commented Jan 21, 2017

Currently when "BIP39 seed" option is selected in seed dialog, any seed will be accepted and "Seed type:" info will be hidden.

I would like to change the messages shown to user:

  • leave them as they're now when "BIP39 seed" is not selected (so "Seed type: old/standard/2fa")
  • show "Seed type: BIP39 (set in options)" when BIP39 checksum is valid and BIP39 was selected in options
  • show "Seed type: BIP39 (set in options; unknown wordlist!)" when there are words outside of bundled wordlist and BIP39 was selected in options
  • show "Seed type: BIP39 (set in options; invalid checksum!)" when words are from wordlist, but BIP39 checksum is invalid and BIP39 was selected in options

I understand problems with BIP39 (no versioning, checksum based on numbers from worldlist, multiple wordlists for different languages), but UX really suffer if Electrum doesn't warn about invalid checksum. I almost got heart attack when I kept entering seed with a typo and Electrum didn't say anything to me :).

However it will require bundling BIP39 wordlist with Electrum for as long as restoring from BIP39 will be supported and as I understand we can't rely that current wordlist used by Electrum will always be the same as BIP39.

So my question is: would you merge a PR that implements this or is the wordlist issue a deal breaker (I'll totally understand that)?

@ecdsa
Copy link
Member

ecdsa commented Jan 22, 2017

We already use the English wordlist, so it's possible to checksum of English seeds without wordlist additions; I did not do it because I lacked time. I would merge it, but I do not guarantee that BIP39 will forever be supported in Electrum; it will be messy if they extend it to derive segwit addresses.

@westonal
Copy link

westonal commented Aug 14, 2017

I almost got heart attack when I kept entering seed with a typo and Electrum didn't say anything to me

👍

At least let us know somehow if the words typed are not on the English word list.

I do not guarantee that BIP39 will forever be supported in Electrum

But you support it now, why can't the support be better? And why does that imply supporting it forever?

@dabura667
Copy link
Contributor

why can't the support be better?

Supporting one checksum check (English only) is unfair. Every wordlist in the future will come up and say "well, you did it for English! Do it for us!" And then eventually, Thomas is stuck making sure checksum checks work for all the wordlists.

Electrum's BIP39 full support was never guaranteed.

@westonal
Copy link

westonal commented Aug 14, 2017

@dabura667 does #2727 actually cover this? and for all languages.

@dabura667
Copy link
Contributor

@westonal not in its current state. Though the author seems to understand what to do.

A checkbox off by default is fine, that way if it breaks, we can just ping the author of the PR and wait for him to fix it if ever.

@ecdsa ecdsa closed this as completed Aug 16, 2017
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

4 participants