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 basic bip39 phrase parsing and generation #292

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@stevenroose
Copy link
Collaborator

commented Jul 11, 2019

Implementation based on https://github.com/rust-bitcoin/rust-wallet/blob/master/wallet/src/mnemonic.rs.

This is only half the BIP39 spec. The seed generation requires PBKDF2 for which I would have to add an extra dependency.

So followup on merging this would be my question if we're ok with adding a dependency for PBKDF2. RustCrypto has a pbkdf2 crate that would be sufficient.

I know we try to avoid extra crypto dependencies, that's why I'm asking here.

We could also not merge this into rust-bitcoin and favor the bip39 crate instead, but the implementation there seems quite a lot less efficient. (It allocates a lot more data in the parse steps, maintains a reverse-lookup map for the wordlists.)

@stevenroose stevenroose force-pushed the stevenroose:bip39 branch from b9bf13a to 39127b5 Jul 11, 2019

@apoelstra

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Concept NAK adding BIP39 support, as per IRC discussion.

Definite NAK regarding any RustCrypto dependencies. Their attitude toward software stability is incompatible with rust-bitcoin.

@dpc

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

Could this be a separate rust-bitcoin-bip39 crate? Regarding RustCrypto - could it be an intermediate step? Or could it be just forked and modified to guarantee stability?

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2019

@apoelstra yeah I implemented mnemonic generation and parsing first and then when I saw the pbkdf2 requirement, I already realized that's probably the reason it wasn't already in rust-bitcoin in the first place.

@dpc there is the bip39 crate that seems to have all functionality. I guess wallet implementers don't care all that much about performance anyway.

@dpc

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

@stevenroose My concern was mostly about trustworthiness of another crate with another author vs maintained by rust-bitcoin.

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2019

I see your concern. I also noticed the coding style in the bip39 crate is quite different from rust-bitcoin's standards. But well, it is what it is and the bip39 crate works. I've been using it successfully for a week now :)
Sad thing they use failure for errors.

@dpc

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

I went ahead and reviewed it again.

Makes me think - maybe rust-bitcoin could have rust-bitcoin-3rd-party crate where it re-exports 3rd party crates pinned to versions that passed the review of project members or something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.