[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 into
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 |
|
I want to start by saying that I support getting this functionality merged. Thank you @alvroble for this contribution. I bet it was a lot of work. I also appreciate everyone who has reviewed and tested it along the way. Let me apologize upfront for how long this comment is and how long it has taken me to do a deep dive. TLDR: There are some blocking steps/bugs IMO that need resolution before this PR can be merged. I have spent quite a few hours learning Trezor Suite + signing device and trezorctl to create single-share, basic multi-share, and advanced group backups so I could understand probably the most common place a user is likely to be coming from when they recover a SLIP-39 wallet. This is recovery functionality that people can use with real backups. I want to help get this PR across the finish line and merged. I remain a concept ACK on this PR. Blocker: embit PR 91 needs to land firstI think embit PR 91 needs to be merged and released before this PR lands. Without it, SeedSigner rejects a valid extendable SLIP-39 share with "not a valid Shamir share". That error is not accurate and could mislead users. My understanding is the Trezor firmware has used the extendable SLIP-39 format for new SLIP-39 backups since the 2024 update to the standard (satoshilabs/slips#1724). Extendable shares set a flag bit in the header, use shamir_extendable rather than shamir for the RS1024 checksum customization string, and omit the identifier from the PBKDF2 salt. The good news is that embit PR 91 reads the extendable flag in Share.parse() and applies the correct checksum and salt behavior. This PR only relies on Share.parse(), ShareSet.recover(), and SLIP39_WORDS, so after 91 is released and the embit pin is bumped, I do not think SeedSigner will need additional code changes for extendable imports. I do not want a user trying to recover a cold-storage wallet to be told that a valid share is invalid. A false checksum failure in a recovery flow is the sort of thing that can make someone think the backup protecting their bitcoin is unusable. Name SLIP-39 explicitly in the UII would like to see SLIP-39 used in more of the user-facing text. There are other Shamir Secret Sharing schemes in use, so Shamir or SSS by itself does not tell the user exactly what SeedSigner supports. Two ideas for where this would help:
That gives someone holding a share card a clear confirmation that they selected the correct import flow. Bug: the status uses the group threshold instead of the member thresholdThe shares remaining calculation reads the wrong SLIP-39 header field. For a single-group backup whose member threshold is greater than 1, the UI says all required shares have been collected after the first share. SeedStorage.get_pending_shamir_threshold() in src/seedsigner/models/seed_storage.py:164-176 returns share.group_threshold, which is the number of groups required. It does not return member_threshold, which is the number of shares required within that group. In a single-group N-of-M backup, group_threshold is always 1. That makes shares_remaining = share_threshold - share_count reach zero after one entry. SeedShamirShareOptionsView then displays: "Required shares collected. Ready to finalize." But Using member_threshold would fix the count for a single-group backup, but one scalar shares remaining value is not enough for an advanced multi-group setup. Progress there has to be evaluated per group. At minimum, the Ready to finalize message should be controlled by can_finalize, not by the share-count arithmetic. Reproduction steps:
Test vector, created only for testing and never used with funds: Bug: a duplicate share leaves the import flow in a dead endEntering the same share twice is accepted without a warning. After that, the set cannot be finalized, even if the user enters the other valid shares, and there is no way to remove only the duplicate. The validation in src/seedsigner/views/seed_views.py:2439-2446 calls slip39.Share.parse() on the new share by itself. That checks whether the individual share parses, but it does not compare it with the shares already stored. The duplicate is appended to the pending set. When SeedSigner later tries to reconstruct the wallet, embit's ShareSet.init raises ValueError("Share indices should be unique") because both copies have the same member_index. ShamirSeed._generate_seed() in src/seedsigner/models/seed.py:278-293 catches every ValueError as IncompleteShamirShareSetException, so can_finalize stays False without telling the user why. Adding the legitimate second share does not help because the duplicate remains in the set. Combined with the status bug above, the screen can say the required shares are collected while continuing to offer only Add another share. The user cannot finalize, no error is displayed, and the only recovery is to back out at Word # 1 and discard the entire set. Reproduction steps using the same 2-of-2 vector:
The duplicate check could happen after Share.parse() succeeds by comparing the parsed (group_index, member_index) with the pending shares, along with the share-set identifier as appropriate. Wrap upI am glad this work exists, and I want to keep helping with testing so it can be completed and merged. SLIP-39 import is a useful recovery option for SeedSigner. I'm going mark this PR as blocked until I think resolving the dependency and the two share-set handling bugs above would move this much closer to the finish line.... |















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: