Skip to content

Conversation

@jdlcdl
Copy link

@jdlcdl jdlcdl commented Aug 15, 2024

Purpose

To limit embit.bip39's mnemonic length to 24 words, entropy length to 128 - 256 bits, as defined in:

https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki#generating-the-mnemonic

Changes

This pull-request changes:

  • src/embit/bip39.py:
    • extending an existing condition in mnemonic_to_bytes() to limit: 12 <= len(mnemonic) <= 24 words,
    • extending an existing condition in mnemonic_from_bytes() to limit: 16 <= len(entropy) <= 32 bytes.
  • tests/tests/test_bip39.py
    • extending existing test_invalid_length() to test mnemonic-words and entropy-bytes lengths.

Note: while this could break 3rd party apps that previously exploited this "feature", calling mnemonic_to_seed() with wordlist=None would still allow recovery of bip32 wallets from non-standard bip39 mnemonics. Only encoding/decoding and their internal validation of input length has changed.

Example of non-standard embit.bip39 mis-use

Not that anyone should actually use embit.bip39 in the following manner, but so that they cannot --
unless it is intended that the following code snippet succeed:

from embit import bip39, bip32

mnemonic = 'estate simple blush security shoulder recipe ridge catch half argue siege seat all cruise six hub call indoor forget ski scheme caution spatial unveil cake just fresh food crazy rack remind easily apart hint elite shadow bitter dash oval slow call drink fresh dolphin bamboo defy night hospital hover canal breeze security stone noodle circle company cherry october battle food barrel route remain pizza impulse icon little paddle lizard awake cause assault hunt rebel elite trip velvet crawl magic lottery harvest patient bone under fire develop light method junior gate motion system dragon cruise obtain august hunt poverty blue traffic rare cute nest auction half simple banner excite short awake snow connect clown faith success turtle curve mother differ hockey gesture much battle seed north ribbon like around industry adapt elite uphold bleak noodle citizen unfold inner donate blush food version render remember toss grunt hidden mercy seat tragic stomach express beef use'

# True, but it's an invalid bip39 mnemonic -- too long
assert len(mnemonic.split()) == 153

# this should fail -- AssertionError
assert bip39.mnemonic_is_valid(mnemonic)

# these should fail -- ValueError("Invalid recovery phrase")
assert bip39.mnemonic_from_bytes(bip39.mnemonic_to_bytes(mnemonic)) == mnemonic

assert str(bip32.HDKey.from_seed(bip39.mnemonic_to_seed(mnemonic))) == 'xprv9s21ZrQH143K3KfRKeNCLeDVTJzTPoRwMC5yh25en6WDZeBMddPbTb87vvMi8n1yipUhYYNEoGMbMzF6vcw3oCu5ttPT3RiwYSUJqYWJ2yC' 

assert str(bip32.HDKey.from_seed(bip39.mnemonic_to_seed(mnemonic)).to_public()) == 'xpub661MyMwAqRbcFojtRfuChnAE1LpwoG9niR1aVQVGLS3CSSWWBAhr1PSbn9gW9GkTdaMsBHSv5fTeAXgZn6qMCTR3MLf3PwLceUUiLkgeohr'

@jdlcdl jdlcdl marked this pull request as ready for review August 15, 2024 16:31
@jdlcdl
Copy link
Author

jdlcdl commented Aug 18, 2024

For an absurd example at the limit of what would currently be "valid": a 768 word mnemonic would have checksum as the last 24 words and would be encoding 8192 bits of entropy. Beyond that, current implementation would have problems w/ checksum, would not encode/decode and would not be valid.

from embit import bip39

bits8192 = b"\x00" * 1024
assert len(bits8192) == 1024

words768 = bip39.mnemonic_from_bytes(bits8192)
assert len(words768.split()) == 768

assert bip39.mnemonic_is_valid(words768)
assert bip39.mnemonic_to_bytes(words768) == bits8192

@tadeubas
Copy link
Member

ACK, I think this should target develop and be merged.

@jdlcdl jdlcdl changed the base branch from master to develop April 16, 2025 19:50
@jdlcdl jdlcdl changed the base branch from develop to master April 18, 2025 12:05
@odudex
Copy link
Collaborator

odudex commented May 6, 2025

Can you do a duplicated PR with same commit to develop branch? So we can use this change in Krux while we wait for a review from Stepan or a consensus with other projects.

@odudex
Copy link
Collaborator

odudex commented May 6, 2025

About duplicated PR, it's no needed, as I was able to cherry-pick the 2 commits from this PR directly on develop.

@odudex
Copy link
Collaborator

odudex commented May 6, 2025

The commits from this pull request have now been merged into Embit's develop branch.

@jdlcdl jdlcdl changed the base branch from master to develop May 6, 2025 17:23
@jdlcdl jdlcdl closed this May 6, 2025
@jdlcdl
Copy link
Author

jdlcdl commented May 6, 2025

closed as commits were pulled into develop
reopened and based from develop back to master.

@tadeubas
Copy link
Member

tadeubas commented May 6, 2025

Hey @jdlcdl we are maintaining the PRs opened to master to know the features/additions that now exists on develop

@jdlcdl jdlcdl reopened this May 6, 2025
@jdlcdl jdlcdl changed the base branch from develop to master May 6, 2025 17:56
@newtonick
Copy link

I was attempting to review and test this PR to help get it merged. However I am unable to get the new test added in this PR to pass.

_______________________________________________________________ Bip39Test.test_invalid_length _______________________________________________________________

self = <tests.test_bip39.Bip39Test testMethod=test_invalid_length>

    def test_invalid_length(self):
        invalid_length = [
            # not divisible by 3, too short, too long
            "panel trumpet seek bridge income piano history car flower aim loan accident embark canoe",
            "zoo " * 8 + "zebra",
            "zoo " * 26 + "valley",
        ]
        for words in invalid_length:
>           self.assertFalse(mnemonic_is_valid(words))
E           AssertionError: True is not false

tests/tests/test_bip39.py:183: AssertionError
================================================================== short test summary info ==================================================================
FAILED tests/tests/test_bip39.py::Bip39Test::test_invalid_length - AssertionError: True is not false

@stepansnigirev
Copy link
Contributor

stepansnigirev commented Nov 5, 2025

I was attempting to review and test this PR to help get it merged. However I am unable to get the new test added in this PR to pass.

_______________________________________________________________ Bip39Test.test_invalid_length _______________________________________________________________

self = <tests.test_bip39.Bip39Test testMethod=test_invalid_length>

    def test_invalid_length(self):
        invalid_length = [
            # not divisible by 3, too short, too long
            "panel trumpet seek bridge income piano history car flower aim loan accident embark canoe",
            "zoo " * 8 + "zebra",
            "zoo " * 26 + "valley",
        ]
        for words in invalid_length:
>           self.assertFalse(mnemonic_is_valid(words))
E           AssertionError: True is not false

tests/tests/test_bip39.py:183: AssertionError
================================================================== short test summary info ==================================================================
FAILED tests/tests/test_bip39.py::Bip39Test::test_invalid_length - AssertionError: True is not false

Are you sure you are not testing against master branch? I just checked with this branch both in micropython and pytest - tests pass.

Copy link
Contributor

@stepansnigirev stepansnigirev left a comment

Choose a reason for hiding this comment

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

Makes sense

@tadeubas
Copy link
Member

tadeubas commented Nov 8, 2025

>git status
HEAD detached at jeando/limit_bip39_mnemonic

>./.venv/bin/python ~/.vscode/extensions/ms-python.python-2025.16.0-linux-x64/python_files/unittestadapter/discovery.py --udiscovery -v -s ./tests -p test*.py

....

----------------------------------------------------------------------
Ran 167 tests in 19.336s

OK
Finished running tests!

ACK - Here pytests passed ⬆️ , could not test micropython yet

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants