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

Automatic fix of non-base58check characters #7520

Closed
wants to merge 1 commit into from

Conversation

iamthen0nce
Copy link

Non-technical people in Bitcoin may not know that their private key isn't accepted due to mistyping an 'o' for an 'O' or a zero. So I propose to automatically replace those characters like here or alternatively display an error message that explains it (however I don't have enough QT experience to implement it; this is more of a PoC.

Non-technical people in Bitcoin may not know that their private key isn't accepted due to mistyping an 'o' for an 'O' or a zero. So I propose to automatically replace those characters like here or alternatively display an error message that explains it (however I don't have enough QT experience to implement it; this is more of a PoC.
Copy link
Contributor

@dhruv-joshi-7 dhruv-joshi-7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel raising an error would be a better option as there will be many more combinations that might occur here for replacing the letters.

@@ -1010,7 +1012,7 @@ def get_private_keys(text, *, allow_spaces_inside_key=True, raise_on_error=False


def is_private_key_list(text, *, allow_spaces_inside_key=True, raise_on_error=False):
return bool(get_private_keys(text,
return bool(get_private_keys(text.replace('O', 'o').replace('0', 'o').replace('I', 'i').replace('l', 'i'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need for text.replace('O', 'o').replace('0', 'o').replace('I', 'i').replace('l', 'i') as you are already doing the same process in get_private_keys

@iamthen0nce
Copy link
Author

I feel raising an error would be a better option as there will be many more combinations that might occur here for replacing the letters.

What do you mean, more combinations might occur? And error would be fine as well, sure, I just didn't want to get into GUI coding to propose such a change to be honest ;)

But I feel this issue really needs to be addressed, if not only to decrease the risk of people going to fake 'seed recovery services' maybe thinking something's wrong with their keys and stuff...

Another easy fix would be to have a static message above the seed word dialogue, explaining 'there are no capital O's and zeroes in a private key.' (and the same for I / i / l...)

@dhruv-joshi-7
Copy link
Contributor

dhruv-joshi-7 commented Oct 21, 2021

What do you mean, more combinations might occur? And error would be fine as well, sure, I just didn't want to get into GUI coding to propose such a change to be honest ;)

For example, combinations like typing l for 1.

@SomberNight
Copy link
Member

So the use case is that some people might have WIF keys on paper and they might manually type them and make a typo?
You suggest either (1) automatically and silently replacing characters to valid lookalikes or (2) displaying a warning in the UI that some characters are invalid.

Are there other programs/services that do either?


I strongly think we should not do (1).

  • why do you convert l->i, instead of e.g. l->1? and if you concede all three look similar then maybe the idea of silent-auto-replace is problematic...
  • this would effectively mean we accept some invalid strings as valid without complaining to the user, and note that
    • if the user tried to import the same keys to another application it would inexplicably fail
    • relatedly, note that the "other application" could be a future version of Electrum if we later decided to remove the silent-auto-replace; meaning we could not easily change our mind and revert this (as once valid keys would become invalid)
  • also, note that the typo might already be present in the user's backup they are copying the key from (rather than newly introduced during copying), in which case it would probably be better to notify the user somehow

I guess if we really wanted to, we could do something like (2), e.g. highlight non-base58 characters in the GUI or show an error dialog to the user explaining this, or something of that sort; but that assumes you can reliably tell what the user entered is supposed to be in base58 (only they mistyped some chars) (as opposed to the user-input being in a completely different encoding), which might severely limit the UI and in some cases already (and maybe more in the future) is not true. In particular, consider a generic input box that accepts keys in a number of encodings (not just base58).

Frankly I don't think the motivation/usecase is strong enough to justify action.

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

Successfully merging this pull request may close these issues.

None yet

3 participants