[BUG]: Cannot finalize seed w/o passphrase once editing passphrase

[jdlcdl]

Is there an existing issue for this?

I have searched the existing issues

Current Behavior

While finalizing a pending seed, if the user adds a passphrase then decides to edit that passphrase by deleting all characters -- effectively removing the passphrase, they will remain stuck in the "Add Passphrase" entry screen with no option to bail-out other than to finalize the seed with the unwanted passphrase, then discard it in order to start over.

Additionally: If adding a passphrase that consists only of space characters, an exception is raised that reads like:

System Error
(!)
ValueError
seed_screen.py, 1037, in post_init
range() arg 3 must not be zero

Expected Behavior

When finalizing a pending seed, if the user starts to add a passphrase and then edits that passphrase effectively removing it, this should be acceptable and there should be some indication that the fingerprint has not been changed; it has no passphrase.

Additionally: If a passphrase consists only of space characters, this should result in something other than an exception.

Suggested Fix

I do not have any suggested fix for seedsigner codebase at this time, but an investigation is required and I intend to follow up later.

As well, an investigation into the bip39 specification, its reference implementation, and other implementations is needed before I can form an expectation about how passphrases that consist only of space characters should be handled. At the same time, because it appears that seedsigner currently trims leading and trailing spaces from the passphrase, verification that this is standard practice is warranted.

[UPDATE] After some research (see below) my initial code changes will be reflected here. As of Friday Jan 6, 2023, a pull request has been submitted.

Anything else?

This issue was also discussed in #291

[jdlcdl]

On the topic of an empty passphrase, whether the user never added anything or whether they chose to edit during review and then deleted the passphrase, an empty passphrase IS valid -- it's no passphrase at all. No need to review!?!?, just go to Finalize!?!?, where they can change their mind again?

More importantly -- now that I've taken a look:

On the topic of "space" characters within a bip39 passphrase, whether they are leading, trailing, or fill the passphrase entirely...

My finding, so far, is that there is no special treatment to "spaces", as opposed to other characters, and that the untrimmed passphrase is appended to the word "mnemonic" and passed thru to PBKDF2 as the salt argument when creating the bip39 seed. The passphrases ("", " ", " ") will all create unique bip39 seeds and master hd wallets for the same mnemonic phrase. Redundantly, passphrases ("a", " a", and " a ") will all create different wallets.

I am not surprised that nobody has complained about this, but I do consider this a bug that would result in the inability for someone to use seedsigner to recover a bip39-to-bip32-hd-wallet which was created with a passphrase that had leading, trailing, or nothing-but spaces.... assuming that the original ux did NOT also trim the passphrase before handing it over to a bip39 library.

References that I have checked, so far, are:

• https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki
  The bip39 specification has no specific mention of special handling of "space" characters for the passphrase.

• https://github.com/trezor/python-mnemonic
  The bip39 reference implementation does not appear to contain code that makes special treatment for spaces in a passphrase, and I have tested that leading, trailing, and nothing-but-spaces does in fact result in unique bip39 seeds.

• https://github.com/diybitcoinhardware/embit/blob/master/src/embit/bip39.py
  The bip39 implementation within embit also does not appear to treat spaces in a special manner and I have tested that the bip39 seeds embit.bip39 creates are the same as the bip39 reference implementation above, given the same "spacey" passphrases.

• https://iancoleman.io/bip39/#english
  I have little personal experience with this tool, but I've seen it referenced again and again in the seedsigner telegram group and the few experiences I've had have been consistent with my expectations. This tool is producing expected matching bip39 seed results as I've found in the above mentioned implementations.

• electrum v. 4.3.2
  Electrum is installed but I won't pretend to be an experienced user. It allowed me to create a bip39 wallet and also allowed me to put multiple consecutive spaces, with special warnings advising against doing so, into my passphrase... then it created a wallet with the same fingerprint as would be created from the implementations above (as well as the same fingerprint as my seedsigner fix just below).

At this point in time, I'm feeling more confident that seedsigner should NOT be trimming leading and/or trailing spaces on the passphrase via the ".strip()" method, as is happening currently on line +841 of gui/screens/seed_screens.py within the SeedAddPassphraseScreen class. Testing this change is giving me a fingerprint that matches what other bip39 libraries above would create with the same mnemonic and "spacey" passphrase. This would be fix to leading, trailing and nothing-but-spaces in a passphrase. Regarding the ui/ux hint during passphrase review, quotes around the passphrase, or maybe a different background color, or if I understood the codebase much better, a grey-filled rounded rectangle per character just like an enabled key in the keyboard. For now, I've made a change so that if the passphrase would be unchanged after a call to .strip() it displays exactly as it does now, else if .strip() would alter the passphrase (because it has leading, trailing or nothing-but-spaces) then it will be displayed in double-quotes during review.

Just a thought: tabs and newlines and linefeeds are whitespace too... how ugly could this get? Do we need these keys available when adding passphrase, to be sure we can recover any pre-existing wallets? The answer to this, I think, is "No!". Rather than a whitespace issue, it's more a matter of how many UTF-8 characters exist that might have been used in a passphrase; and it's not reasonable to believe the seedsigner keyboard can support them all.

[jdlcdl]

@easyuxd, I'm tagging you here for your thoughts on whether or not it would be acceptable to display a passphrase in double-quotes for cases where the user has set their passphrase with leading, trailing, or nothing-but spaces.

This issue started out as "If the user adds a passphrase and then edits to delete it, then they're stuck with no way to get out.".

While researching, I've found that a bug has existed since 0.4.3 where leading or trailing spaces would derive the wrong hd wallet. My proposed fix is to NOT trim these spaces from the passphrase so that the correct hd wallet IS created, and I'm requesting your feedback on "How to display this passphrase to the user during review?".

The easiest fix (read that as: the accessible one given my limited understanding of the code base) is to wrap the passphrase in double-quotes whenever the user has prefixed/suffixed their passphrase with spaces, else to do nothing new when they have not. ie: a passphrase of ABC would still display like ABC, and ABC with spaces around it would read like " ABC " in the Review Passphrase screen.

Thank you for your time and consideration.

[EverydayBitcoiner]

Since the " character could be part of the passphrase as well it may confuse people. It may be better to use something like an under bracket. Here's a convo I found about exactly how to best display a space.

https://ux.stackexchange.com/questions/91255/how-can-i-best-display-a-blank-space-character

[jdlcdl]

If I were a coding wizard, I'd have already re-implemented the screen where the passphrase is reviewed, and each and every character of the passphrase would be displayed similar to the keys in the keyboard component. Otherwise, no matter which character we use to represent a space, it could get confusing.

A passphrase of:
"That's what she said", he said

Would currently print much as it looks above (with wrapping), while if the user added one trailing space, it would read like:
""That's what she said", he said "

Yuck!

The optimal display, in my mind, is to use some hint that does NOT include wrapping-with or replacing any characters that might also be valid as part of the passphrase (UTF-8 can support over 1 million).

There is good news, however. Open source development is a slow march towards "better" and this pr could take a long long long time to be merged, giving me a chance to figure-out the wizardry that I already know would be much better than my quick fix. Nothing about this issue/pr is pressing until we start hearing complaints of not being able to restore a wallet with a spacey passphrase.

Thank you for your thoughts and for the link @EverydayBitcoiner, and for your issues template that helped me write the first post of this issue. I'm a fan!

[easyuxd]

Very thoughtful, @jdlcdl. I tend to agree with @EverydayBitcoiner that adding quotes on the Review screen could be confusing.

Certainly an edge case, but if leading/trailing spaces were supported it could warrant exploration of other visual affordances that don't conflict with the charset. I'm thinking use of secondary color (grey), maybe adding an underline or input box around the passphrase?

[EverydayBitcoiner]

A few different ideas to throw out there:

[jdlcdl]

Morning thoughts:

While utf-8 may support over one million possible characters and it's not reasonable to believe the seedsigner keyboard can support them all, neither can most other human input devices. In any locale the subset of probable chars in a passphrase is considerably smaller.

@easyuxd and @EverydayBitcoiner pointed out that double-quotes to wrap a passphrase for this edge-case may be confusing. I'll admit it was a very anglophone assumption to make; a francophone might have made the assumption to wrap with << and >>; all of these characters, for wrapping the passphrase, are much more likely to cause confusion because they are more likely available to existing keyboards where a previous passphrase might have been set.

@EverydayBitcoiner pointed me to a discussion where this topic is discussed, and just above has included some examples of replacements that might be appropriate as a solution, but I don't believe that altering font-color inline will be readily available for me.

My current dilemma is the classic bang-for-the-buck consideration: "Is it worth it?".

• I know, deep in my heart, that I'd like for every character in a passphrase to be displayed with a different background color, or a faded border just as @EverydayBitcoiner's examples, or like the grey filled rounded rectangle of an enabled key in our keyboard component, but I also realize that where this code needs changed is currently implemented via appending to a centered TextArea, where I believe that my current options to do so are limited. Wrapping or replacing the passphrase is a one-liner while refactoring the entire screen for this edge-case that has existed for some time but never caused any complaints feels short-term "costly" to my time, even though it would be long-term "rewarding" to my toolbox.

• I know that I DO want to become capable of wizardry in this code base, but this will not come overnight. Perhaps other wizards would have suggestions or commits to apply to the related pr, or would consider a partial fix/merge?

Aiming for "Done!" or at the least "better", and taking @EverydayBitcoiner's hint to replace space characters with "\u2423" (somewhat like underscore) or other easily available utf-8 characters which are NOT readily available in existing keyboard layouts and improbable in passphrases seems like a compromise worth pursuing; I will pursue it. New to github and knowing that this is my first pr ever, I will need to take a look at what I can do as far as pushing to the same branch that I've already issued a pr for in issue #295, I may end up just leaving examples here in an image as I've done above, then asking for your feedback again.

[jdlcdl]

The compromise I have in mind (see above, replacing spaces w/ "\u2423") might read something like:

This string has spaces, but none leading, trailing, or consecutive.

␣This␣one␣has␣a␣leading␣space.

This␣one␣has␣a␣trailing␣space.␣

And␣this␣one␣has␣␣multiple␣consecutive␣spaces␣␣␣within.

This string has single spaces, and the next string has 4 spaces only.

␣␣␣␣

[jdlcdl]

Alternatively, the left-7/8-block "\u2589" would read something like:

This string has spaces, but none leading, trailing, or consecutive.
▉This▉one▉has▉a▉leading▉space.
This▉one▉has▉a▉trailing▉space.▉
And▉this▉one▉has▉▉multiple▉consecutive▉spaces▉▉▉within.
This string has single spaces, and the next string has 4 spaces only.
▉▉▉▉

[jdlcdl]

Many unicode characters that I tried, "'\u23b5" and many geometric shapes are not visible w/ current fonts in seedsigner. The "\u2423" under-score-ish character looks "just ok" when other visible characters are next to it, but it's not obvious that it's a space when the passphrase is only spaces. The left-7/8s block is the clearest one for me. It doesn't join together when next to another space as a full block would, and it also doesn't look as if there is the block AND a space, as is the case when I tried the left 3/4s-block. Is it a space, or is it an un-supported unicode character? that's my only concern for confusion. As an anglophone, I almost prefer wrapped with double-quotes. ho hum :(