Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add functions to handle unique prefix mnemonics #99

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gwillen
Copy link

@gwillen gwillen commented May 15, 2019

Common practice with bip39 mnemonics is to use unique four-character prefixes
of wordlist words. In some cases (e.g. "steel" storage devices) there is not
space to store more than four characters. So, enable users to enter unique
prefixes instead of whole words.

This is an alternative to #91 for fixing #89 . If you like the approach, I can add tests and so forth. Unfortunately I forgot that C doesn't have default parameters, which makes this slightly messier than it would have been in C++. I also couldn't take advantage of bsearch due to the interface, which is slightly annoying but shouldn't be a big deal (wordlists are very short, time to search them should be negligible.)

Common practice with bip39 mnemonics is to use unique four-character prefixes
of wordlist words. In some cases (e.g. "steel" storage devices) there is not
space to store more than four characters. So, enable users to enter unique
prefixes instead of whole words.
@gwillen
Copy link
Author

gwillen commented May 15, 2019

(to avoid duplication, it might actually make sense to change the original interface instead of adding another one, if you like this approach -- but adding an interface is certainly more backwards-compatible. I think making this the main interface should be okay though. It would eliminate the use of bsearch, but as mentioned above, the amount of time this takes should be absolutely negligible, and it should never be getting called in a tight loop anyway.)

@greenaddress
Copy link
Contributor

concept ack, looking good to add tests IMHO

I think the new function approach is preferable here because of backwards compatibility - if it was something we had just added or that we were still iterating a bit on like liquid there'd be a stronger argument to break ABI but not so much in this case and not without leaving the prior function as deprecated but around for a while (which we can still do later if we decide that's the way forward).

@@ -83,6 +83,24 @@ size_t wordlist_lookup_word(const struct words *w, const char *word)
return found ? found - w->indices + 1u : 0u;
}

size_t wordlist_lookup_prefix(const struct words *w, const char *prefix)
Copy link
Contributor

@instagibbs instagibbs Jul 16, 2019

Choose a reason for hiding this comment

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

this breaks with act versus actual/actress/actor/etc mnemonic words, since act prefix matches for 2+ entries.

I think we should restrict the prefix input to length 3 or 4, and if length 3 it must be an exact match.

@instagibbs
Copy link
Contributor

instagibbs commented Jul 16, 2019

suggested fix and test here: https://github.com/instagibbs/libwally-core/tree/prefix_fix

also good to know at least SLIP39 doesn't allow 3 letter words

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants