[Feature] Add SLIP-39 Shamir's secret sharing import support for SeedSigner#636
[Feature] Add SLIP-39 Shamir's secret sharing import support for SeedSigner#636alvroble wants to merge 57 commits intoSeedSigner:devfrom
Conversation
|
Very cool. I appreciate you honing your approach to fit the preferences we've adopted for the project thus far. I will be tied up with finish the upcoming multilanguage + Spanish release, but will be looking forward to reviewing this PR in the coming weeks. If we get to 2-3 weeks past the new release and you haven't heard from me, please poke me on telegram and remind me to return here! |
|
Hi team, this PR is ready for review. Please take a look when you can. Happy to address any feedback—thanks! |
|
Great work, Álvaro! I ran the test suite with Python 3.10 and 3.12, and all tests passed: Only when I try to go to the SSS recovery option do I encounter this error (video attached for better flow details): I could to be creating bad .img for my pi0(?), could be a problem with my raspi device(?): (build output)root@d3fbae864001:/opt# time ./build.sh --pi0 --app-repo=https://github.com/alvroble/seedsigner.git --app-branch=slip39_sss_import
Disk disk.img: 25 MiB, 26214400 bytes, 51200 sectors
New situation: Device Boot Start End Sectors Size Id Type The partition table has been altered. real 67m24.812s Maybe you could to build same image for pi0 and I will test it, will be in touch. Best. - |
|
Hi @fedebuyito. Thanks a lot for the review and for the bug catch. The code had not been adapted to a recent refactor in |
|
Hi, @alvroble ! You're welcome, I hope to can help. I could to navigate into sss feature and the bug was not presented anymore. Regarding to UX/UI I saw some bit minor details maybe can be considerered:
Best! |
|
Thanks for testing, @newtonick. Which platform did you use for your tests? I normally test on Raspberry Pi OS, but I also tried the feature on SeedSigner OS to be sure, and the toast does trigger:
However, occasionally it fails to appear. My guess is that the button bounce makes the toast to get "cancelled due to user input" before it gets rendered. |
|
I was testing on Raspberry Pi OS. I’ll try again and also try a SeedSignerOS build. The more I think on this the more I wonder if it makes sense to have this error toast at all. Once the first share is entered it contains the information to know how many more shares are required to recover/finalize. I don’t think the Finalize button shouldn’t appear until it’s possible to recover. If it is a multi share slip39, then after the first share is entered it should say something like “X more shares needed to recover”. |
I did a SeedSignerOS build and it worked until I tried clicking "Finalize" multiple times. IMG_2604.mp4 |
I agree. The toast isn’t necessary: the UI can guide the user instead of erroring. However it's weird how different behavior can be between devices. Mine shows the toast 50%-ish of the tries. This button bounce issue might be a subject for a different PR. |
|
@newtonick What do you think about having these two They could be adjusted to have a different icon (maybe an "info" icon?). |
|
I think it's better without the toast. Also take a look at I don't think the number of shares needs to be in the TopNav. The info in the body of the screen is enough to convey the status. I feel like the green checkmark success icon is left on its own; either the TopNav title or the text immediately below the checkmark icon should say something like "Share Added". Might also be simpler to list the share count as something like: The current text kind of repeats the same info. |
8019837 to
574261b
Compare
Thanks, didn't know about this. I agree with the proposed changes, this would be the status as of 574261b: |
I like the look of these 2 screenshots but I haven't had time to test yet. |
|
Review: Headless Verification & Fix for "Silent Failure" on Finalize Fantastic work on this SSS implementation! I pulled the branch to test the flows in a headless environment and the logic generally looks solid. However, I caught one specific UX edge case: The "Silent Failure" on Finalize. The IssueIf a user enters valid individual shares that do not mathematically combine to form a valid seed (e.g., shares from two different backups), Currently, The FixI implemented a simple error view to give the user clear feedback when reconstruction fails. Here is the patch I verified locally: @securesigner ➜ /workspaces/seedsigner (pr-636) $ git diff src/seedsigner/views/seed_views.py
diff --git a/src/seedsigner/views/seed_views.py b/src/seedsigner/views/seed_views.py
index 6101d54..5ad1057 100644
--- a/src/seedsigner/views/seed_views.py
+++ b/src/seedsigner/views/seed_views.py
@@ -2509,18 +2509,25 @@ class SeedShamirShareOptionsView(View):
self.controller.storage.convert_pending_shamir_share_set_to_pending_seed(finalize=False)
except InvalidSeedException:
logger.exception("Pending Shamir share set unexpectedly failed to reconstruct despite eligibility.")
- return Destination(
- SeedShamirShareOptionsView,
- view_args={
- "can_finalize": self.can_finalize,
- "share_threshold": self.share_threshold,
- "share_count": self.share_count
- }
- )
+ return Destination(SeedShamirReconstructionErrorView)
return Destination(SeedShamirShareFinalizeView)
+class SeedShamirReconstructionErrorView(View):
+ def run(self):
+ self.run_screen(
+ DireWarningScreen,
+ title=_("Reconstruction Error"),
+ status_headline=_("Invalid Share Set"),
+ status_icon_name=SeedSignerIconConstants.ERROR,
+ text=_("Unable to reconstruct seed from the provided shares."),
+ show_back_button=True,
+ )
+ return Destination(BackStackView)
+
+
+
class SeedShamirShareInvalidView(View):
EDIT = ButtonOption("Review & Edit")
DISCARD = ButtonOption("Discard", button_label_color="red") |
* Distinguish between ValueError (incomplete share set) and TypeError (incompatible shares) * Add graceful error handling in SeedShamirShareMnemonicEntryView * Add test for invalid share set scenario
|
Hey @securesigner that was a good catch, thank you! I tested the proposed solution for my device but it actually didn't fix the issue, since it appears earlier in the Moreover, I took a slightly different approach, treating separately both possible exceptions for embit's |
|
I pulled the latest commit (1951ae2) and verified your new approach. I confirmed that handling Ran |
|
I ran this PR with Claude Code and it gave these suggestions: Bug fixes
Style/convention fixes
The PR author can apply it with: git am < pr-636-review-suggestions.patch |















Description
This PR introduces support for importing Shamir Secret Sharing (SSS) shards (SLIP-39) into SeedSigner, focusing on seed recovery for users with existing SSS-based backups. While SeedSigner’s stateless design discourages SSS for routine use, this feature provides flexibility for recovering keys from legacy setups or unique scenarios where SSS is used.
The implementation is strictly limited to key recovery, with no support for creating new SSS setups (this can be discussed along with SeedQR support for SSS). So this feature aligns with SeedSigner’s philosophy of providing versatile recovery options without compromising simplicity.
Relevant issue: #552
Complete flow (updated)
As with Electrum, a start view is included
SeedShamirShareStartView:Then the user is asked about the words per share
SeedShamirShareImportSelectWordCount:Then, user enters words using the SLIP-39 wordlist. For 128-bit seeds, each Shamir's share will be 20 words long. For 256-bit seeds, each Shamir's share will be 33 words long. All shares (up to the threshold number of the original backup) are required to recover the original secret.
SeedShamirShareMnemonicEntryView:SLIP-39 checksum errors are treated as follows
SeedShamirShareInvalidView:After entering each share, user will get asked if they need to add more
SeedShamirShareOptionsView:If the user tries to finalize but the Shamir Share Set cannot be formed yet, an
ErrorToastwill be shown:When finalizing, user will be asked if they want to enter a passphrase in a dedicated
SeedShamirShareFinalizeView:I'm open to comments around this feature as well as to UX improvements so we can get a final version of the PR
This pull request is categorized as a:
Checklist
pytestand made sure all unit tests pass before sumbitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os: