Skip to content

Conversation

notTanveer
Copy link
Contributor

@notTanveer notTanveer commented Aug 15, 2025

Description

fixes #500 .
displays only checksum-valid words when manually entering the final word of a seed phrase.

core implementation

  • added get_valid_final_mnemonic_words() in mnemonic_generation.py
    • computes valid final words per bip-39 checksum
    • 12-word seeds: 128 valid words
    • 24-word seeds: 8 valid words (of 2048)

technical details

  • implements exact bip-39 spec
    • 12-word: 7-bit entropy + 4-bit checksum
    • 24-word: 3-bit entropy + 8-bit checksum

this enhances the UX by filtering out invalid words (which don’t meet the checksum criteria) and makes typing faster by showing fewer possible matches.

EDIT:
the test_invalid_mnemonic test was removed because it tests behavior that is no longer possible with the new seed word prediction feature.

this test was causing the entire test suite to hang indefinitely during execution. the issue occurred at the final word entry stage, where the test attempted to input invalid final words ("zoo" and "zebra"). these entries triggered the get_valid_final_mnemonic_words() function, but since neither word exists in the computed valid word set, the test framework became stuck in an infinite loop

This pull request is categorized as a:

  • New feature

Checklist

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

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

  • Yes

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

@notTanveer notTanveer force-pushed the feat/predict-final-seed-word branch from 2d9a899 to e23e1af Compare August 15, 2025 15:09
@notTanveer notTanveer changed the title Feat/predict final seed word [Feature]: Predict Last Seed Word Aug 15, 2025
@notTanveer notTanveer force-pushed the feat/predict-final-seed-word branch 2 times, most recently from 563b607 to 5282821 Compare August 15, 2025 20:56
@alvroble
Copy link
Contributor

Concept ACK, but implementation NACK.
I tested it, and although the tests pass, it currently breaks Electrum mnemonic entry. It turns out Electrum uses a different checksum format from BIP-39, which means the valid last words proposed are only valid for BIP-39 checksum scheme, and do not match those for Electrum.

This would also limit the reusability of SeedMnemonicEntryScreen for potential future implementations of other mnemonic seed formats such as SLIP-39 (#636) or aezeed (used by LND), which also rely on different checksum schemes.

@notTanveer
Copy link
Contributor Author

TIL, that there are different mnemonic formats. I have made this PR exclusively for BIP-39.

it currently breaks Electrum mnemonic entry

this can be fixed easily by adding a condition to ensure SETTING__ELECTRUM_SEEDS is disabled (so we don’t run through all the extra math to find possible last words). these are very niche features that the majority of people don’t use. I'm not sure how many actually rely on electrum seeds, but i’m pretty sure the number is low. also, the last word of an electrum seed phrase CANT be predicted.

for potential future implementations of other mnemonic seed formats such as SLIP-39 (#636) or aezeed (used by LND)

i believe these are all niche features and are disabled by default. we can handle them the same way as described above.

almost forgot, thank you for reviewing this PR @alvroble !
also if you have a better approach in mind i would love to hear that as well!

that said, lemme know what others think.

@alvroble
Copy link
Contributor

@notTanveer thank you for your work!

this can be fixed easily by adding a condition to ensure SETTING__ELECTRUM_SEEDS is disabled

This would mean disabling the functionality of predicting the last word in all cases where SETTING__ELECTRUM_SEEDS is enabled, even if we are entering a BIP-39 mnemonic phrase. It definitely solves the problem, but it might be overkill.

Another option is to directly use different screen and view for Electrum and other (to be supported) mnemonic phrase formats. This option would completely solve the problem, but I like it even less because it would duplicate a lot of code and would be the messy way of doing it.

these are very niche features that the majority of people don’t use. I'm not sure how many actually rely on electrum seeds, but i’m pretty sure the number is low

Yes, it’s true that this feature is rarely used nowadays, but we shouldn’t allow functionality that SeedSigner already supports to break.

Actually... I think the best approach would be to have a new class attribute in the Seed class (e.g. seed_type) that could be either "bip39" for Seed (default, parent class) or "electrum" for ElectrumSeed. Then in get_valid_final_mnemonic_words check if Seed is "electrum" and return the whole wordlist since that seed format does not support the feature. I would also love some input from other devs here.

@notTanveer notTanveer force-pushed the feat/predict-final-seed-word branch from 759a57a to df56291 Compare August 19, 2025 12:52
@notTanveer
Copy link
Contributor Author

we have two possible approaches:

  1. use is_pending_electrum – This would let us distinguish Electrum seeds from others. It requires minimal changes and still works fine for standard mnemonic entry (BIP-39).

  2. adopt a seed type approach (as suggested by @alvroble), this feels more standardized and makes the code future-proof, especially for supporting Shamir Secret and other upcoming formats.

let me know which one do you guys prefer?

@Chaitanya-Keyal
Copy link
Contributor

How about changing the current _pending_is_electrum to pending_seed_type in SeedStorage? This would be defaulted to "BIP-39", and changed to "electrum" in SeedElectrumMnemonicStartView (and to other types for other seeds in the future).

In that case there would be no need to pass a seed_type argument to the SeedMnemonicEntryView since the attribute depends on the seed insance, and the check could happen even inside get_valid_final_mnemonic_words, so the whole wordlist is returned for SeedElectrum

At the mnemonic entry stage, we only set a pending mnemonic, not a pending seed. Since the seed instance is only created after all valid words have been entered, there’s no seed instance available at that point to check the type. So that approach doesn’t seem viable to me.

@notTanveer
Copy link
Contributor Author

notTanveer commented Aug 19, 2025

thanks for the suggestion, @Chaitanya-Keyal and @alvroble ! updated the PR. feels like this is the best way forward.
open to hearing what others think though.

@notTanveer notTanveer force-pushed the feat/predict-final-seed-word branch from e472df3 to 6f27161 Compare August 19, 2025 20:29
Copy link
Contributor

@alvroble alvroble left a comment

Choose a reason for hiding this comment

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

Tested ACK on Raspberry Pi OS Manual Build as of 6f27161. I like this approach too.

@kdmukai
Copy link
Contributor

kdmukai commented Aug 26, 2025

There is too much bitcoin-related work being done in SeedMnemonicEntryScreen. Ideally Screens shouldn't do any logic beyond what's necessary to present the information the View has provided them.

People have asked seriously about a version of SeedSigner for blind users. If and when that time comes, we'd need very different "Screen" implementations to present the data (perhaps a digital braille device or just a text-to-speech flow). But the goal is that all the important logic would already be contained within the Views, so swapping in alternate Screens should be kind of trivial.

So... all of this wordlist filtering needs to happen in the View.

@kdmukai
Copy link
Contributor

kdmukai commented Aug 26, 2025

I've been unhappy with our existing bip39 vs Electrum juggling in SeedStorage since we added support for Electrum seeds. We could take a cue from Destination and just set a Seed_cls member var AND get rid of the _pending_is_electrum bool completely:

    def init_pending_mnemonic(self, num_words:int = 12, is_electrum:bool = False):
        self._pending_mnemonic = [None] * num_words
        self._Seed_cls = Seed if not is_electrum else ElectrumSeed

So then later we can avoid this:

            if self._pending_is_electrum:
                seed = ElectrumSeed(self._pending_mnemonic)
            else:
                seed = Seed(self._pending_mnemonic)

And just do:

            seed = self._Seed_cls(self._pending_mnemonic)

I haven't tested this nor deeply explored if it creates any problems elsewhere.

Also note that validate_mnemonic is dead code.

This all might make more sense as a separate refactor PR.

@kdmukai kdmukai moved this from SoB Needs Code Review to SoB In Progress in @SeedSigner Development Board Aug 26, 2025
@notTanveer
Copy link
Contributor Author

notTanveer commented Aug 27, 2025

the test_invalid_mnemonic test was removed because it tests behavior that is no longer possible with the new seed word prediction feature.

this test was causing the entire test suite to hang indefinitely during execution. the issue occurred at the final word entry stage, where the test attempted to input invalid final words ("zoo" and "zebra"). these entries triggered the get_valid_final_mnemonic_words() function, but since neither word exists in the computed valid word set, the test framework became stuck in an infinite loop.

@notTanveer notTanveer force-pushed the feat/predict-final-seed-word branch from a02faf1 to b725a8b Compare August 27, 2025 12:56
Comment on lines 7 to 10
class SeedStorage:
PENDING_SEED_TYPE__BIP39 = "bip39"
PENDING_SEED_TYPE__ELECTRUM = "electrum"
# future TODO: PENDING_SEED_TYPE_SLIP39 = "slip39"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a deeper refactor would be better, SeedStorage should stay agnostic of seed types. The responsibility for type should live in the Seed classes themselves, and polymorphism should handle the differences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will be updated once #807 is in

@notTanveer notTanveer force-pushed the feat/predict-final-seed-word branch from b725a8b to d317dad Compare August 29, 2025 15:34
@notTanveer notTanveer force-pushed the feat/predict-final-seed-word branch from d317dad to f0cc53f Compare August 29, 2025 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: SoB In Progress
Development

Successfully merging this pull request may close these issues.

Suggest 12/24th words when entering a seed.
4 participants