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

48 Character seed #7

Closed
docalb opened this issue Jul 24, 2023 · 24 comments
Closed

48 Character seed #7

docalb opened this issue Jul 24, 2023 · 24 comments
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation help wanted Extra attention is needed no-issue-activity

Comments

@docalb
Copy link

docalb commented Jul 24, 2023

Message says spaces aren't necessary when entering seed, but when I combined 2 four length blocks into 1 block of 8 characters, I got a error saying I only entered in 44/48 characters.

@BenWestgate
Copy link
Owner

BenWestgate commented Jul 24, 2023

It should say spaces are not allowed period.

But I allowed them if they were entered exactly as they were displayed in Create Wallet. Ie between each 4 character group.

Spaces could be typos and at any rate they slow down your typing.

Although they may speed up finding your typos. Soon users won't need help with that because I can correct up to 4 errors with the checksum and highlight the correction for user confirmation.

What do you think it should do when it finds a string with spaces?

@docalb
Copy link
Author

docalb commented Jul 24, 2023

If you are implementing updated 4 error checksum then no need for spaces to help with mistake ID.

Make clear in directions to NOT add spaces

In event of wallet recovery, tell user to not include spaces. Many will write down seed in blocks of 4, and will think to include spaces when entering in.

Sample wallet recovery message:

"You may have written down your seed in 4 character blocks to make it easier to read, but don't include spaces when entering seed in now."

@BenWestgate
Copy link
Owner

It used to look that way:

BenWestgate/Bails@15e0aee

Should I revert this commit?

@BenWestgate
Copy link
Owner

"You may have written down your seed in 4 character blocks to make it easier to read, but don't include spaces when entering seed in now."

Message looks good. Should I show it after a user clicks "I already have a seed" to restore wallet? That would be after the gold star in "create seed" when it begins a test restore of the just written down backup.

@docalb
Copy link
Author

docalb commented Jul 24, 2023

yes

@BenWestgate
Copy link
Owner

And also display it after writing down the first share of the codex32 backup? Display that before confirming what they wrote down.

Another tester was adamant on allowing spaces. Getting rid of accepting them would kill another issue where the error messages don't make sense with spaces. They're very descriptive with a solid string.

It was not that hard to display the words without spaces but I like that they defeat copy and paste (they aren't real spaces.) and make it easier to read in 4 characters at a time and write from short term memory and not lose your position.

@docalb
Copy link
Author

docalb commented Jul 24, 2023

I can't speak to the intuition of other testers, if I'm the odd man out, then go with the majority.

If you decide for the no-space route, instructions should be clear that while it spits out 4 character blocks for readability to write down, entering seed back in should be without spaces.

If disallowing spaces eliminates separate problem of unintelligible error message, all the better.

@epiccurious
Copy link

Soon users won't need help with that because I can correct up to 4 errors with the checksum and highlight the correction for user confirmation.

If you are implementing updated 4 error checksum then no need for spaces to help with mistake ID.

I agree with @docalb, no need for allowing spaces if you can detect and highlight the correction.

@BenWestgate
Copy link
Owner

The Only issue regarding auto-correcting errors using the checksum is that a library to do this efficiently has not been released. One in rust is being developed but is not available yet.

See my open issue here:
BlockstreamResearch/codex32#54

===Error Correction===

A codex32 string without a valid checksum MUST NOT be used.
The checksum is designed to be an error correcting code that can correct up to 4 character substitutions, up to 8 unreadable characters (called erasures), or up to 13 consecutive erasures.
Implementations SHOULD provide the user with a corrected valid codex32 string if possible.
However, implementations SHOULD NOT automatically proceed with a corrected codex32 string without user confirmation of the corrected string, either by prompting the user, or returning a corrected string in an error message and allowing the user to repeat their action.
We do not specify how an implementation should implement error correction. However, we recommend that:

  • Implementations make suggestions to substitute non-bech32 characters with bech32 characters in some situations, such as replacing "B" with "8", "O" with "0", "I" with "l", etc.
  • Implementations interpret "?" as an erasure.
  • Implementations optionally interpret other non-bech32 characters, or characters with incorrect case, as erasures.
  • If a string with 8 or fewer erasures can have those erasures filled in to make a valid codex32 string, then the implementation suggests such a string as a correction.
  • If a string consisting of valid bech32 characters in the proper case can be made valid by substituting 4 or fewer characters, then the implementation suggests such a string as a correction.

Right now I'm detecting invalid characters, indexes, hrp, thresholds and non bech32 characters and mixed case and rejecting them. But I'm only able to pass/fail the checksum.

@BenWestgate
Copy link
Owner

BenWestgate commented Jul 24, 2023

I could probably detect and auto-correct a couple character substitutions and correct a few erasures (?) by simply trying every possible substitution in the string until the checksum matches.

This will start to eat up a lot of CPU as the amount of errors to search for grows. There's a mathematical way to correct the errors with the properties of the error correction code without brute force.

my particular way of encoding the codex32 secret actually has an additional 22-bits of checksum. The identifier is hash160 of the seed, and the padding bits in the last payload character of the seed are it's double sha256 (like WIF format's checksum)

So it's not impossible to correct 5 substitution errors and still have good confidence the correction was correct thanks to my addition.

@BenWestgate BenWestgate added bug Something isn't working documentation Improvements or additions to documentation help wanted Extra attention is needed labels Jul 24, 2023
@BenWestgate
Copy link
Owner

Soon users won't need help with that because I can correct up to 4 errors with the checksum and highlight the correction for user confirmation.

If you are implementing updated 4 error checksum then no need for spaces to help with mistake ID.

I agree with @docalb, no need for allowing spaces if you can detect and highlight the correction.

I've opened an issue to create an interim error and erasure correction algorithm by brute force:
#12

@BenWestgate
Copy link
Owner

The only error correction implementation I could push into the Restore Wallet code would be able to suggest a correction from 1 edit (substitution, insertion, deletion) in real time.

Correcting 2 edits takes several seconds and might be slower than the user correcting their input manually.

Is this 1 edit correction a feature you want in now?

Eventually there will be an efficient algorithm to correct up to 4 random errors and 8 random erasures (marked ?) or 13 contiguous erasures.

@docalb
Copy link
Author

docalb commented Jul 27, 2023

The only error correction implementation I could push into the Restore Wallet code would be able to suggest a correction from 1 edit (substitution, insertion, deletion) in real time.

Correcting 2 edits takes several seconds and might be slower than the user correcting their input manually.

Is this 1 edit correction a feature you want in now?

Eventually there will be an efficient algorithm to correct up to 4 random errors and 8 random erasures (marked ?) or 13 contiguous erasures.

@epiccurious do you feel one way or the other on this?

I think that would be an improvement, but don't want to push a feature where I'm perhaps not fully appreciating it's implications.

Also @BenWestgate if this more efficient algo is coming down the pike, maybe it makes sense to just wait for that -- saving you workload.

@BenWestgate
Copy link
Owner

The brute force algorithm that corrects 2-5 errors has been implemented. It works well for the meantime. I can likely improve it with some erasure specific correction characters marked ? or that use a non-bech32 character. Let me know if it performs OK in realistic testing and whether it is useful to save time or improve confidence / reduce stress. If it's too slow I can scale it back to look for smaller corrections faster.

Actually typing and read off handwritten shares, it displays Correct shares UPPERCASE now as well to match the spec, although lower is faster to write, upper is more distinct to read.)

#12

The worst delay I saw was 39 seconds in a 2 core 4GB vm with a 20% synched IBD competing for resources. (I might need to reduce the priority of bitcoind because of this CPU hungry error correction algorithm) and that was correcting 2 inserted characters and one substitution, so still probably faster than doing it manually.

For double errors and single errors it corrects in seconds or instantly.

@BenWestgate
Copy link
Owner

BenWestgate commented Jul 27, 2023

my particular way of encoding the codex32 secret actually has an additional 22-bits of checksum. The identifier is hash160 of the seed, and the padding bits in the last payload character of the seed are it's double sha256 (like WIF format's checksum)

This is going to be changed to the master pubkey fingerprint aka hash160(public_masterkey) now your watch-only wallets will be able to tell you what set of codex32 shares restore spending for it.

And we have a way to deal with wallet name collisions by just appending Wallet [389j] or whatever the identifier is to identify wallets and shares related to each other.

This also gives me a way to look up which encryption public key to use if multiple codex32 backups/wallets are being restored and the user "saves" their progress, which requires writing a private key down.

RIght now if you have a threshold 4 codex32 backup and can't get all your shares before your battery dies you lose your progress. There is a safe way to shutdown without leaving the secrets behind on the Persistent Storage if you encrypt by generating a restore private key. The only problem was how to know which shares go to which wallet, or encryption public key for that matter, without having to decrypt each time to match identifiers, no more confusion with the master pubkey hash.

@BenWestgate
Copy link
Owner

BenWestgate commented Jul 30, 2023

I find the brute force very helpful when a make a single error or two, the delay isn't too bad except when you've made more errors than it corrects. And it will be extremely helpful when I highlight its corrections with a different color or bold/underlined text so you can go directly to your paper string to verify the corrections are legit.

I need to highlight obvious errors in the user's input to give them something to do while waiting for the correction to arrive or not.

Even creating the seed will get a lot faster and easier when I highlight the LOCATIONS of errors.

Still not closing this until the simple initial suggestions are changed to zenity notifications (and highlighted) and the corrections and error locations are colorful. That will be the best experience we can get before the rust ecc library is completed.

@BenWestgate
Copy link
Owner

BenWestgate commented Aug 5, 2023

Tasks remaining to close:

  • Add dialog box saying to not include spaces in entry.
  • Add a center space between two groups of six 4 character groups to make it easier for user to keep their place.
  • Highlight error LOCATIONS during creation aka display_confirm() to make the paper easier verify is correct
    • Suggest to use Color RED as sipa used it in his bech32 error locator demo
  • Add error correction during restore
  • Highlight error corrections in the $displayed string to make them easier to verify against paper during restore
  • Use the same color for error locations in restore, when the user makes a clear mistake but does not have an easy fix
    • share index = s for shares
    • non-bech32 characters besides 1bio
  • Consider using GREEN or whatever color would not cause difficulty for color blind users for highlighting automatic corrections

@BenWestgate BenWestgate self-assigned this Aug 5, 2023
@BenWestgate
Copy link
Owner

Here's our example of error location: that will be implemented for backup creation:

https://bitcoin.sipa.be/bech32/demo/demo.html

Fortunately because we're creating it, we have the whole correct string in memory and can highlight unlimited error locations.

The reason they are not auto-corrected is to ensure the user's paper backup is correct without assistance.

@BenWestgate
Copy link
Owner

BenWestgate commented Aug 6, 2023

I can actually "correct a ton of errors if they're things like B = 8, O = 0, I = 1/l, etc, those suggestions would be in addition to the brute force edit_depth of 1.
And they should be displayed by the python library, not the messy bash function.

Need to check that my error correction tries these common substitutions before feeding the python ecc libary. Will allow corrections beyond 2 and be quite helpful to some.

@BenWestgate
Copy link
Owner

Found a way to make an entry field for confirm new seed backup that highlights which block of 4 characters the user has made a substitution error in real time as they type.

This is huge improvement.

@BenWestgate
Copy link
Owner

Locking the already correct fields will be as well.

Will take some testing to make sure arrow keys, insertions and delete backspace automatically move cursor to expected spot despite correct blocks to the right.

If I spent more than 1/2 hour on this only lock the left most correct blocks. Then it will make a sort of progress bar.

Yes, deactivating or disabling correct fields of the input can be a helpful technique to guide users and improve their experience, especially in scenarios where a specific format needs to be followed, such as entering Codex32 strings.

When users are entering a complex format like a Codex32 string, deactivating correct fields can serve several purposes:

  1. Visual Clarity: By deactivating fields that are already correct, you provide a clear visual indication to users that those sections have been completed accurately. This helps users focus on the remaining sections that need their attention.

  2. Error Detection: Users are more likely to notice errors in fields that are highlighted or active. Disabling the correct fields prompts users to pay closer attention to sections that might contain mistakes.

  3. Reduced Cognitive Load: Users don't have to worry about double-checking the fields they've already completed correctly, reducing cognitive load and potential confusion.

  4. Progress Tracking: Users can easily see their progress as they move through the input, helping them understand how far they've come and how much is left.

  5. Feedback and Validation: If there's a validation step after completing the input, disabling correct fields helps ensure that users only submit fully accurate entries.

To implement this, you can set up your user interface to automatically deactivate fields that have been entered correctly. Once the content of a field matches the expected content (or format) for that specific field, you can disable the field programmatically. This could involve setting a property like sensitive or editable to False for the Gtk.Entry widget representing that field.

Keep in mind that this technique should be used in conjunction with other visual cues and error feedback mechanisms to provide a comprehensive user experience. Additionally, consider the needs and familiarity of your target audience when deciding which techniques to implement.

@BenWestgate
Copy link
Owner

Reminder to self to apply all of these ideas applicable to correction / importing shares as well when I tackle this generation backup issue. BlockstreamResearch/codex32#58

Both will get new Gtk dialog boxes then I can solve BenWestgate/Bails#50 to not open dialogs while the user is typing a share either to confirm a new backup or import an existing.

@BenWestgate BenWestgate transferred this issue from BenWestgate/Bails Mar 25, 2024
Copy link

Stale issue message

@BenWestgate
Copy link
Owner

This is completed for backup creation, it's a beautiful dialog that checks all the boxes.

I have not updated the import dialog yet but that was not the reported issue here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation help wanted Extra attention is needed no-issue-activity
Projects
None yet
Development

No branches or pull requests

3 participants