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 in SLIP-39 #3825

Merged
merged 6 commits into from
May 28, 2024
Merged

Conversation

andrewkozlik
Copy link
Contributor

@andrewkozlik andrewkozlik commented May 15, 2024

Related to:

TODO:

  • Update shamir-mnemonic package once version 0.3.0 is released.
  • Update screens.
  • Maybe add some device/upgrade tests based on the QA list below.

Notes to @trezor/qa:

  • Thoroughly test Shamir backup.
  • Test case 1:
    1. Install old firmware (2.7.0 or earlier).
    2. Reset device and choose Shamir backup, but do not back up.
    3. Write down some address or XPUB from passphrase-less wallet.
    4. Write down some address or XPUB from passphrased wallet.
    5. Install new firmware.
    6. Ensure that the addresses of the wallets remain the same.
    7. Create a backup and reboot.
    8. Ensure that the addresses of the wallets remain the same.
    9. Wipe the device and recover using the backup.
    10. Ensure that the addresses of the wallets remain the same.
  • Test case 2:
    1. Install old firmware.
    2. Reset device and choose Shamir backup and create a backup.
    3. Write down some address or XPUB from passphrase-less wallet.
    4. Write down some address or XPUB from passphrased wallet.
    5. Install new firmware.
    6. Ensure that the addresses of the wallets remain the same.
    7. Wipe the device and recover using the backup.
    8. Ensure that the addresses of the wallets remain the same.
  • TC3: Ensure that Shamir backups created with new firmware can be successfully recovered on new firmware, i.e. check that addresses remain the same.
  • TC4: Ensure that Shamir backups created with new firmware are not accepted by old firmware.

@andrewkozlik andrewkozlik added core Trezor Core firmware. Runs on Trezor Model T and T2B1. feature Product related issue visible for end user R&D Research and development team related labels May 15, 2024
@andrewkozlik andrewkozlik self-assigned this May 15, 2024
Copy link

github-actions bot commented May 15, 2024

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T2B1 Safe 3 3280 test(screens) main(screens) 2724
T3T1 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@andrewkozlik
Copy link
Contributor Author

andrewkozlik commented May 15, 2024

One would expect the UI tests to differ in only the second word and last three checksum words. The reason why they also differ in the share values is that the PRNG is called by the storage when writing the extendable flag, which desynchronizes the PRNG between the old and new UI tests before the shares are generated. The actual encrypted master secret is probably the same.

Maybe we will store the information about extendability in _BACKUP_TYPE instead of a separate storage entry. See https://www.notion.so/satoshilabs/Shamirization-and-the-random-identifier-09a8b1af23bc45809915c4a74df903d5?pvs=4#a786472062e1484fb9ff2b5d95a3c04f.

@MiroslavProchazka
Copy link

MiroslavProchazka commented May 20, 2024

QA test task here

@matejcik
Copy link
Contributor

we talked about dropping the stored identifier for extendable backups, do you still intend to do that?

@andrewkozlik
Copy link
Contributor Author

andrewkozlik commented May 22, 2024

we talked about dropping the stored identifier for extendable backups, do you still intend to do that?

I do, and I was also hoping to write some upgrade test(s). Maybe I can make some time tomorrow afternoon.

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.

implementation LGTM

i have caught a mistake in docs, and I believe we need to add a couple checks to the upgrade-test

docs/core/misc/slip0039.md Outdated Show resolved Hide resolved
docs/core/misc/slip0039.md Outdated Show resolved Hide resolved
tests/upgrade_tests/test_firmware_upgrades.py Show resolved Hide resolved
@andrewkozlik andrewkozlik added the translations Put this label on a PR to run tests in all languages label May 27, 2024
Copy link

github-actions bot commented May 28, 2024

legacy UI changes device test(screens) main(screens)

@andrewkozlik andrewkozlik merged commit 162649a into main May 28, 2024
106 of 108 checks passed
@andrewkozlik andrewkozlik deleted the andrewkozlik/slip39 branch May 28, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Trezor Core firmware. Runs on Trezor Model T and T2B1. feature Product related issue visible for end user R&D Research and development team related translations Put this label on a PR to run tests in all languages
Projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

None yet

3 participants