Skip to content

[Feature] Add SLIP-39 Shamir's secret sharing import support for SeedSigner - #636

Open
alvroble wants to merge 57 commits into
SeedSigner:devfrom
alvroble:slip39_sss_import
Open

[Feature] Add SLIP-39 Shamir's secret sharing import support for SeedSigner#636
alvroble wants to merge 57 commits into
SeedSigner:devfrom
alvroble:slip39_sss_import

Conversation

@alvroble

@alvroble alvroble commented Dec 19, 2024

Copy link
Copy Markdown
Contributor

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:

SeedShamirShareStartView

Then the user is asked about the words per share SeedShamirShareImportSelectWordCount:

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:

SeedShamirShareMnemonicEntryView

SLIP-39 checksum errors are treated as follows SeedShamirShareInvalidView:

SeedShamirShareInvalidView

After entering each share, user will get asked if they need to add more SeedShamirShareOptionsView:

SeedShamirShareOptionsView

If the user tries to finalize but the Shamir Share Set cannot be formed yet, an ErrorToast will be shown:

SeedShamirShareOptionsView_ErrorToast

When finalizing, user will be asked if they want to enter a passphrase in a dedicated SeedShamirShareFinalizeView:

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:

  • New feature
  • Bug fix
  • Code refactor
  • Documentation
  • Other

Checklist

  • I’ve run pytest and made sure all unit tests pass before sumbitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

  • No, I’m a fool
  • Yes
  • N/A

I have tested this PR on the following platforms/os:

@kdmukai

kdmukai commented Dec 19, 2024

Copy link
Copy Markdown
Contributor

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!

@alvroble
alvroble marked this pull request as ready for review April 11, 2025 14:43
@alvroble

Copy link
Copy Markdown
Contributor Author

Hi team, this PR is ready for review. Please take a look when you can. Happy to address any feedback—thanks!

@bitcoinprecept bitcoinprecept moved this from 🆕9.0 In Progress to 9.0 Needs Code Review in @SeedSigner Development Board Apr 26, 2025
@fedebuyito

fedebuyito commented May 3, 2025

Copy link
Copy Markdown
Contributor

Great work, Álvaro!

I ran the test suite with Python 3.10 and 3.12, and all tests passed:

image
image

Only when I try to go to the SSS recovery option do I encounter this error (video attached for better flow details):
https://github.com/user-attachments/assets/dd50f6eb-d4d7-4777-965b-c35519fafeb3

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

Executing post-image script ../pi0/board/post-image-seedsigner.sh
25+0 records in
25+0 records out
26214400 bytes (26 MB, 25 MiB) copied, 0.0324031 s, 809 MB/s
Checking that no-one is using this disk right now ... OK

Disk disk.img: 25 MiB, 26214400 bytes, 51200 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes

Script header accepted.
Script header accepted.
Created a new DOS disklabel with disk identifier 0xba5eba11.
disk.img1: Created a new partition 1 of type 'W95 FAT32 (LBA)' and of size 24 MiB.
disk.img2: Done.

New situation:
Disklabel type: dos
Disk identifier: 0xba5eba11

Device Boot Start End Sectors Size Id Type
disk.img1 * 2048 51199 49152 24M c W95 FAT32 (LBA)

The partition table has been altered.
Syncing disks.
mkfs.fat 4.2 (2021-01-31)
/opt/buildroot
a9a65184bed7880e7ac0e12bb6a1de189fa11b7d072739c1d658da301e5dca89 /opt/../images/seedsigner_os.slip39_sss_import.pi0.img

real 67m24.812s
user 32m34.416s
sys 24m59.847s

Maybe you could to build same image for pi0 and I will test it, will be in touch.

Best. -

@alvroble

alvroble commented May 4, 2025

Copy link
Copy Markdown
Contributor Author

Hi @fedebuyito. Thanks a lot for the review and for the bug catch. The code had not been adapted to a recent refactor in HardwareButtons's wait_for method. I solved it and tested different import flows (with and without passphrase, and it should be working now. I also made some cleanup from previous code iterations.

@fedebuyito

Copy link
Copy Markdown
Contributor

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:

  • When the user selects '0' as the threshold, an unhandled error is shown:
    image

  • It might be more intuitive to refer to 'entropy' in this view so that the word count for BIP39 (12/24) doesn't get mixed up with the number of words entered for SLIP39 (20/33):
    image

  • When user step to that view and go back, an system error is showned:
    image

  • Finalizing the SLIP39 for SSS is different from finalizing the original BIP39 seed in its views. Can't they be similar in terms of displaying fingerprint data before completion or during passphrase entry?
    image
    image

Best!

@newtonick

newtonick commented Nov 14, 2025

Copy link
Copy Markdown
Collaborator

When testing, I was unable to get this Toast Error message to appear.
image
When I select "Finalize" after only inputing the first share on a Basic 2-of-3 (128 bits) it does not display the "Need more shares to reconstruct the seed" toast. The only thing that happens is the SeedShamirShareOptionsView redrawn with the "Add another share" button selected.

I was testing with this test vector:

"4. Basic sharing 2-of-3 (128 bits)",
    [
      "shadow pistol academic always adequate wildlife fancy gross oasis cylinder mustang wrist rescue view short owner flip making coding armed",
      "shadow pistol academic acid actress prayer class unknown daughter sweater depict flip twice unkind craft early superior advocate guest smoking"
    ],
    "b43ceb7e57a0ea8766221624d01b0864",
    "xprv9s21ZrQH143K2nNuAbfWPHBtfiSCS14XQgb3otW4pX655q58EEZeC8zmjEUwucBu9dPnxdpbZLCn57yx45RBkwJHnwHFjZK4XPJ8SyeYjYg"

@alvroble

Copy link
Copy Markdown
Contributor Author

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:

imagen

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.

@newtonick

Copy link
Copy Markdown
Collaborator

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”.

@newtonick

Copy link
Copy Markdown
Collaborator

I was testing on Raspberry Pi OS. I’ll try again and also try a SeedSignerOS build.

I did a SeedSignerOS build and it worked until I tried clicking "Finalize" multiple times.

IMG_2604.mp4

@alvroble

Copy link
Copy Markdown
Contributor Author

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 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.

@alvroble

Copy link
Copy Markdown
Contributor Author

@newtonick What do you think about having these two SeedShamirShareOptionsView alternatives instead of the toast error?

SeedShamirShareOptionsView_AddShare SeedShamirShareOptionsView_Finalize

They could be adjusted to have a different icon (maybe an "info" icon?).

@kdmukai

kdmukai commented Dec 4, 2025

Copy link
Copy Markdown
Contributor

I think it's better without the toast.

Also take a look at ngettext for handling singular vs plural text ("1 more share..." vs "2 more shares...").

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:

shares entered: 2
shares remaining: 1

The current text kind of repeats the same info.

@alvroble

alvroble commented Dec 5, 2025

Copy link
Copy Markdown
Contributor Author

Also take a look at ngettext for handling singular vs plural text ("1 more share..." vs "2 more shares...").

Thanks, didn't know about this.

I agree with the proposed changes, this would be the status as of 574261b:

SeedShamirShareOptionsView_AddShare SeedShamirShareOptionsView_Finalize

@newtonick

Copy link
Copy Markdown
Collaborator

Also take a look at ngettext for handling singular vs plural text ("1 more share..." vs "2 more shares...").

Thanks, didn't know about this.

I agree with the proposed changes, this would be the status as of 574261b:

SeedShamirShareOptionsView_AddShare SeedShamirShareOptionsView_Finalize

I like the look of these 2 screenshots but I haven't had time to test yet.

@newtonick newtonick moved this from 0.8.7 Needs Code Review to 0.9.0 Needs Code Review in @SeedSigner Development Board Dec 12, 2025
@newtonick newtonick modified the milestones: 0.8.7, 0.9.0 Dec 13, 2025
@securesigner

securesigner commented Dec 17, 2025

Copy link
Copy Markdown
Contributor

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 Issue

If a user enters valid individual shares that do not mathematically combine to form a valid seed (e.g., shares from two different backups), recover_mnemonic() raises InvalidSeedException.

Currently, SeedShamirShareOptionsView catches this exception but simply reloads the current screen. The user clicks "Finalize", the screen refreshes, and nothing happens. This creates a confusion loop where the user might think the button is unresponsive.

The Fix

I 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
@alvroble

alvroble commented Dec 20, 2025

Copy link
Copy Markdown
Contributor Author

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 SeedShamirShareMnemonicEntryView.

Moreover, I took a slightly different approach, treating separately both possible exceptions for embit's slip39: ValueError (incomplete set) and TypeError (shares from different secrets). The UX should show an ErrorScreen now if the latter is catched.

@securesigner

Copy link
Copy Markdown
Contributor

I pulled the latest commit (1951ae2) and verified your new approach.

I confirmed that handling TypeError (mismatched shares) vs ValueError inside SeedShamirShareMnemonicEntryView works as described.

Ran pytest tests/test_flows_seed.py and confirmed that the Settings.HOSTNAME cleanup caused no regressions.

@berlinxray

Copy link
Copy Markdown

I ran this PR with Claude Code and it gave these suggestions:

Bug fixes

  1. Removed broken mnemonic_display_str/mnemonic_display_list methods — they referenced non-existent self._mnemonics (plural) and broke the @Property interface contract from the base Seed class. Replaced with a useful ShamirSeed.validate_share() static method.
  2. Fixed back button in SeedShamirShareStartView — the warning screen ignored back presses and always proceeded. Now shows the back button and handles RET_CODE__BACK_BUTTON.
  3. Removed redundant set_passphrase() in convert_pending_shamir_share_set_to_pending_seed — ShamirSeed.init already calls it.

Style/convention fixes

  1. Removed # TODO: Add "is_" prefix comments from three base Seed properties — unrelated refactoring noise.
    Replaced getattr() with direct attribute access — pending_shamir_share_set_length always exists (set in init), so getattr fallback was unnecessary.
  2. Reverted leading-space hack in LoadSeedView button labels (" Scan a SeedQR" → "Scan a SeedQR").
  3. Moved share validation into model layer — replaced inline from embit import slip39; slip39.Share.parse(...) in the view with ShamirSeed.validate_share(), keeping crypto logic in the model.
  4. Wrapped settings strings in _mft() for translation consistency.
  5. Fixed SeedShamirShareImportSelectWordCount back button — was going to MainMenuView with clear_history=True, now goes to BackStackView like other views.
  6. Added trailing newlines to 4 files that were missing them.

The PR author can apply it with: git am < pr-636-review-suggestions.patch

pr-636-review-suggestions.patch

@newtonick

newtonick commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

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 first

I 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 UI

I 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:

  • Include SLIP-39 in the settings entry that enables the feature.
  • Label the word-count choices 20 Words (SLIP-39) and 33 Words (SLIP-39).

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 threshold

The 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
at the same time, can_finalize is correctly False, so the only button shown is Add another share. The message and available action contradict each other.

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:

  1. Create a single-group 2-of-2 backup, or use the extendable test vector below after applying embit PR 91.
  2. Enable the SLIP-39 import and enter only Share 1.
  3. The UI says Required shares collected. Ready to finalize. but only offers Add another share.
  4. I would expect Shares entered: 1 and Shares remaining: 1, with status text that agrees with the button.

Test vector, created only for testing and never used with funds:
Share 1: royal lily academic acid angel editor acrobat wolf fitness syndrome axis trend smear slavery verdict garbage national luxury flea voter
Share 2: royal lily academic agency detailed prayer custody sled sugar duckling pancake leaf parcel election lend gross deploy favorite holy hunting

Bug: a duplicate share leaves the import flow in a dead end

Entering 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:

  1. Enter Share 1. It is accepted.
  2. Enter Share 1 again. It is accepted again with no duplicate warning.
  3. Enter Share 2. Finalize still never appears even though both distinct shares are present.
  4. I would expect step 2 to be rejected with a message such as Share already entered.

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 up

I 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....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: 0.9.0 Needs Code Review

Development

Successfully merging this pull request may close these issues.

7 participants