summa-tx / coins

Rust implementations of BIP32/39 and Ledger device comms
Other
90 stars 31 forks source link

ineffective `W::get_index()`, `W::get_all()` #107

Closed StackOverflowExcept1on closed 1 year ago

StackOverflowExcept1on commented 1 year ago

https://github.com/summa-tx/bitcoins-rs/blob/db28df1fb0d8dc71f149735bfa9a955d25b54f19/bip39/src/wordlist/mod.rs#L31-L32

These functions look very inefficient and clearly could be improved, I want to use this crate to brute unknown words

prestwich commented 1 year ago

thank you for your feedback, please include benchmarks in your optimization PR so that we can evaluate the performance improvements

StackOverflowExcept1on commented 1 year ago

@prestwich I haven't made a benchmarks but for 12 words BIP-39 algorithm it can make good performance bump. Currently algorithm complexity of get_index() is O(n) but we can use lazy_static! for example to initialize HashSet<usize, &str>

Also I think that this code can be changed too. https://github.com/summa-tx/bitcoins-rs/blob/db28df1fb0d8dc71f149735bfa9a955d25b54f19/bip39/src/wordlist/mod.rs#L38-L41 Currently it allocates vector for no reason and complexity is O(n). We can change this to -> &'static [&'static str] slice and perform all the things on the compile time using the build.rs for example.

StackOverflowExcept1on commented 1 year ago

Also look at the Mnemonic<W: Wordlist>::new_from_phrase(...).

let index = W::get_index(word)?;

Currently this code does O(n^2) because get_index launches another loop

https://github.com/summa-tx/bitcoins-rs/blob/db28df1fb0d8dc71f149735bfa9a955d25b54f19/bip39/src/mnemonic.rs#L85-L94