Skip to content

Conversation

@stepansnigirev
Copy link
Contributor

Previously mnemonics with leading / trailing / double spaces or with \n or \r as word separator were treated as valid by mnemonic_is_valid and underlying mnemonic_to_entropy functions. This could cause a problem because this mnemonic was passed to pbkdf2 as is, without any normalization. So mnemonics like this would give an invalid seed.

I was considering either normalizing mnemonics before sending to pbkdf2 and be more forgiving to user input, or be strict and enforce single spaces between words. In this situation I think it's better to be more strict, and wallet designers can relax this requirements by doing something like " ".join(mnemonic.split()) if they want, or using mnemonic.strip() especially when reading from text file. Just keep in mind that doing something like that will create additional copies of the mnemonic in RAM.

Copy link
Collaborator

@odudex odudex left a comment

Choose a reason for hiding this comment

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

tACK

On Krux, this change caught a test we were doing with a trailing " ".
Commit from this PR was merged to develop branch.
Later on, this PR will require a manual merge for a simple conflict with #63.

@tadeubas
Copy link
Member

ACK, makes sense

@odudex
Copy link
Collaborator

odudex commented Nov 17, 2025

@miketlk and @jdlcdl, what do you think about using this PR to start to do some merges to master. To ensure everything is solid, it would be nice if you could both give it a test on Specter-DIY and SeedSigner before we merge.

@kdmukai
Copy link
Contributor

kdmukai commented Dec 2, 2025

Concept ACK.

Though longer term it seems more sensible to me to refactor all the mnemonic handling to only accept list[str]. It's always felt fragile to pass it as a whole string.

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