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

Support extendable backup flag #46

Merged
merged 3 commits into from
May 14, 2024
Merged

Support extendable backup flag #46

merged 3 commits into from
May 14, 2024

Conversation

andrewkozlik
Copy link
Contributor

@andrewkozlik andrewkozlik commented May 8, 2024

Related to spec update satoshilabs/slips#1724.

Note that we need to get this merged and released in order for the firmware device tests to pass on the corresponding firmware branch https://github.com/trezor/trezor-firmware/compare/andrewkozlik/slip39.

Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

functionality LGTM.

i added a bunch of commits fixing tiny problems. please review 706934d in particular which is the only questionable one -- it seemed cleaner to pass the full customization string into RS1024 from the outside. but I'm not attached to that solution, i'll remove that commit if you dislike it.

@andrewkozlik
Copy link
Contributor Author

a1cb726 Updated a comment.

The customization string shamir_extendable is not used in the PBKDF2 salt. We wanted to completely decouple the decryption method from the sharing method and the share representation, i.e. not use the customization string from the checksums in decryption, and I believe avoiding the mention of shamir in the PBKDF2 salt is right, because it allows us to cleanly update backups even to different secret sharing schemes in the future if we want.

@andrewkozlik
Copy link
Contributor Author

706934d breaks generate_vectors.py, line 66:

mnemonic = wordlist.mnemonic_from_indices(
    indices + rs1024.create_checksum(indices, False)
)

@andrewkozlik
Copy link
Contributor Author

Fixed in 38964c0. LGTM now. @matejcik please recheck and then we can squash and merge.

@matejcik
Copy link
Contributor

LGTM, go ahead

@andrewkozlik andrewkozlik merged commit c2031a1 into master May 14, 2024
10 checks passed
@matejcik matejcik deleted the ext branch May 14, 2024 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants