BIP39 implementation #607

atalw · 2022-05-16T06:02:25Z

Description

This PR is a custom BIP39 implementation which closes #561.

There are a couple of features that have been implemented

Parse mnemonic string to Mnenomic type
Generate Mnemonic from entropy
Derive seed from Mnemonic (with and without passphrase)
All language wordlists (with verification test to ensure they were untampered)
Mnemonic test vectors from BIP39
Error handling

Notes to the reviewers

I've tried to keep the function names the same as they were to maintain backward compatibility. There is however a file structure change in the favour of clarity.

I was initially using tabs so earlier commits may fail CI. I'll amend if needed.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

vladimirfomene · 2022-05-16T09:29:19Z

Hello maintainers, before @atalw wrote on that he had started working on this issue, I had already asked to work on the issue and Daniela gave me the go ahead. He then asked whether we could collaborate and I agreed. I proposed we connect on discord and find a way to work together. On Discord, I told him, I already had an implementation of this BIP (https://github.com/vladimirfomene/bip39) in my repos. I asked if he minds me work ing on it alone saying that this was my first contribution and I wanted it to be sizeable. He said he had also started working on it (atalw@3f56f33) and thought it will be more prudent to collaborate on it. Seeing that he had already started working on it, I decide to collaborate with him and two hours after he sends me this PR (atalw#1). In the PR, he has added a new commit which to_seed function which I think is inspired from my BIP39 implementation (check seed.rs in repo). This was frustrating for me because he said let's work on it together but he had already implemented all the functionality. Even more frustrating is that I noticed he had imported the sha2, pbkdf2 and hmac crates and followed the exact algorithm I followed to implement to_seed. He was using the rust-bitcoin hashes for sha256 and had suddenly switched to using sha2 for that just like I did in my implementation. After seeing this, I decided not to collaborate with him and reached out to @danielabrozzoni about the issue. She encouraged me to continue working on my patch and try to open a PR before @atalw . I continued working on my patch, and currently have an implementation that I'm testing (https://github.com/vladimirfomene/bdk/tree/implement-bip-39). If this PR gets merged, I will like to be added as a co-author. Also, why won't my PR be given preference given that I was the first to take up the issue? I understand how this is not easy for you as maintainers. Thank you for the work you do! Find attached my conversation with @atalw.

atalw · 2022-05-16T11:27:46Z

Hey. Firstly, I'd like to say I'm really disappointed this is how our first contribution to BDK is going. Secondly, you mentioned you were willing to collaborate (after seeing the work I had already done) but in DM the first thing you asked is if you could work on it alone. The reason you're saying you didn't choose to collaborate was because I added a 20 line function which implemented all the functionality. I had opened a PR with a list of next steps and I tried to divide it amongst us but there was no response from you so I continued work.

I'm contributing to open-source to improve the projects and the ecosystem. I'm sorry that this situation was poorly handled. If there is no clear solution, to make things easier I could close this PR and find another issue to fix.

P.S the algorithm is similar because it is a BIP. I think there is essentially 1 way to do it with minor differences in coding style. Also I tried to use the rust-bitcoin hash functions (so that we wouldn't have to import a new crate) but pbkdf2 was not accepting it.

notmandatory · 2022-05-16T21:15:34Z

My 2 cents on this is to remind everyone that reviewing someones PR and giving good feed back is as important, if not more important, than writing the actual code. Also that it's not the end of the world if two people write up code for the same issue, in fact it can be a good thing if they can discuss and compare their two solutions and find a way to collaborate on a single PR. Even if one of the two PRs isn't merged the knowledge gained by writing it can be a great way to learn the problem well enough to give good review feedback. Ideally you'll be able to work it out among your selves in a congenial way but worst case there are plenty of issues that need eyes on them and we can find you different issues to work on.

danielabrozzoni · 2022-05-16T21:40:12Z

Firstly, I'd like to say I'm really disappointed this is how our first contribution to BDK is going.

Well, I'm kinda disappointed you couldn't sort this by yourselves, but ¯_(ツ)_/¯

Since you've kinda collaborated, I suggested the co-authoring solution. @atalw can you please squash your commits and put @vladimirfomene as a co-author? (See https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors)

I'll review the PR soon, can you please fix the CI in the meantine? It's failing on cargo clippy:

error: manual `!RangeInclusive::contains` implementation
   --> src/keys/bip39/mod.rs:139:12
    |
139 |         if ent_bits < MIN_ENTROPY_BITS || ent_bits > MAX_ENTROPY_BITS {
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `!(MIN_ENTROPY_BITS..=MAX_ENTROPY_BITS).contains(&ent_bits)`
    |
    = note: `-D clippy::manual-range-contains` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains

error: use of `unwrap_or` followed by a function call
   --> src/keys/bip39/mod.rs:189:58
    |
189 |         let salt = ("mnemonic".to_string() + &passphrase.unwrap_or("".to_string()))
    |                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| "".to_string())`
    |
    = note: `-D clippy::or-fun-call` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call

danielabrozzoni

I have a failure locally:

test src/keys/mod.rs - keys::DerivableKey::into_extended_key (line 454) ... FAILED
---- src/keys/mod.rs - keys::DerivableKey::into_extended_key (line 454) stdout ----
error[E0277]: the trait bound `bdk::keys::bip39::Error: StdError` is not satisfied
  --> src/keys/mod.rs:464:6
   |
12 |     )?
   |      ^ the trait `StdError` is not implemented for `bdk::keys::bip39::Error`
   |
   = note: required because of the requirements on the impl of `From<bdk::keys::bip39::Error>` for `Box<dyn StdError>`
   = note: required because of the requirements on the impl of `FromResidual<Result<Infallible, bdk::keys::bip39::Error>>` for `Result<(), Box<dyn StdError>>`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.

Can you fix?

Also, this is quite difficult to review as the commit history is not super clean... Can you please turn your 10 commits into 2:

One MOVEONLY commit, that only moves all the old code from src/keys/bip39.rs to src/keys/bip39/mod.rs without changing the functionalities (it should look like this: bitcoin/bitcoin@b36e738)
One commit where you actually add all the functionalities

danielabrozzoni · 2022-05-16T21:49:13Z

Cargo.toml

+sha2 = { version = "0.10.2", optional = true }
+hmac = { version = "0.12.1", optional = true }


Minimizing dependencies is important, you don't need those two: hmac and sha512 are already implemented in bitcoin_hashes, which is a dependency of BDK (re-exported by rust-bitcoin)

I tried using them but pbkfd2 wasn't accepting the rust-bitcoin hmac and sha512 traits. Implementing pbkdf2 should fix the issue.

danielabrozzoni · 2022-05-16T21:50:05Z

Cargo.toml

@@ -33,8 +33,11 @@ rocksdb = { version = "0.14", default-features = false, features = ["snappy"], o
 cc = { version = ">=1.0.64", optional = true }
 socks = { version = "0.3", optional = true }
 lazy_static = { version = "1.4", optional = true }
+pbkdf2 = { version = "0.10", optional = true }


The MSRV of pbkdf2 is 1.57, ours is 1.56, maybe we should consider implementing our own pbkdf2 as rust-bip39 is doing? @afilini what do you think?

Are we still planning on having our own implementation of pbkdf2? If @atalw , does not mind, I can try pick this up?

There is an agreement that pbkdf2 should be implemented within BDK (refer here). And yeah sure, feel free to implement it. I'm caught up with some other work and can only do it after 2 weeks.

Go for it @evanlinjin !

Thank you @atalw and @vladimirfomene ! I've done the implementation here: #644 . Please let me know what I can improve on!

Cargo.toml

vladimirfomene

This is a great start! You just need to add more test and implement a couple of verification in your code.

vladimirfomene · 2022-05-16T21:04:34Z

Cargo.toml

+pbkdf2 = { version = "0.10", optional = true }
+unicode-normalization = { version = "0.1.19", optional = true }
+sha2 = { version = "0.10.2", optional = true }
+hmac = { version = "0.12.1", optional = true }


You may want to use rust-bitcoin for sha256, sha512 and Hmac as they are already implemented in the project. As for pbkdf2, maybe you can consider porting the implementation in rust-bip39 while providing the necessary reference following the crate's library.

I tried using them but pbkfd2 wasn't accepting the rust-bitcoin hmac and sha512 traits. Implementing pbkdf2 should fix the issue.

As for porting pbkdf2, we could look into that. My only concern is that it may lead to bloat, especially if we simply copy it without understanding. If there are issues going forward, it will need to be maintained as well. So there's a tradeoff.

I spoke with @afilini and agreed that it's better if we port pbkdf2 into BDK, as rust-bip39 is doing.

vladimirfomene · 2022-05-16T21:15:55Z

src/keys/bip39/mod.rs

+            };
+
+            indices.push(idx);
+        }


After calculating the indices for every word, you may want to recover the entropy, then calculate the checksum to compare it against the binary representation of mnemonic. In case, the calculated checksum does not match the checksum in the binary representation of the mnemonic, you will want to throw an error.

That's a good idea. Would you like to pick this up?

Sorry for the late reply. I will work on it

src/keys/bip39/mod.rs

vladimirfomene · 2022-05-16T22:39:10Z

src/keys/bip39/mod.rs

+    }
+
+    /// Convert a mnemonic to a seed with an optional passphrase
+    fn to_seed(&self, passphrase: Option<String>) -> [u8; 64] {


It might not be a good idea to change the type of passphrase from &str to option<string> as that has the potential of breaking code which consumes this method.

It makes sense to have the passphrase as Option as it really is optional, so if a breaking change is okay we can go ahead with this.

src/keys/bip39/mod.rs

vladimirfomene

This is a great start! You just need to add more test and implement a couple of verification in your code.

There are a couple of features that have been implemented in this commit: - Parse mnemonic string to Mnenomic type - Generate Mnemonic from entropy - Derive seed from Mnemonic (with and without passphrase) - All language wordlists (with verification test to ensure they were untampered) - Mnemonic test vectors from BIP39 - Error handling Function names have mostly been kept the same to maintain backwards compatability. Co-authored-by: Vladimir Fomene <[email protected]>

atalw · 2022-05-17T07:04:10Z

I've done a couple of things

Squashed all the commits into 2 commits – 1 MOVEONLY commit and 1 with all the functionality.
Added @vladimirfomene as a coauthor.
Fixed the clippy warnings and fixed the failing test.

I think it it's good for review now @danielabrozzoni. I'll address your comments and @vladimirfomene comments next.

We are currently only checksum the number of words in the mnemonic and their validity. This implementation also checks the checksum to make sure that it was generated from the entropy of the mnemonic.

Make sure `from_entropy_in` produces error if length of entropy bits is less than 128, greater than 256 and not a multipe of 32.

Throw error if mnemonic sentence is less than 12 words or greater than 24 words or number of words is not a multiple of six or the contains a word not in wordlist or has invalid checksum.

evanlinjin · 2022-06-28T05:18:34Z

src/keys/bip39/mod.rs

+#[cfg_attr(docsrs, doc(cfg(feature = "keys-bip39")))]
+impl<Ctx: ScriptContext> DerivableKey<Ctx> for Seed {
+    fn into_extended_key(self) -> Result<ExtendedKey<Ctx>, KeyError> {
+        Ok(bip32::ExtendedPrivKey::new_master(Network::Bitcoin, &self[..])?.into())


Would it make sense to implement DerivableKey for (bip39::Seed, Network) instead of for bip39::Seed directly. This can apply for the other associated structures in this module as well.

I'm suggesting this because some people may want to derive testnet keys from the Mnemonic/Seed, and this hard-coding may lead to some confusion.

P.S. I bought this up in the discord server

evanlinjin · 2022-06-30T00:58:06Z

I propose that we close this PR and continue the conversation at #644 not sure if @atalw and @vladimirfomene will be okay with this?

danielabrozzoni · 2022-06-30T08:30:48Z

@evanlinjin you might be able to push your code directly on @atalw's branch, so we don't lose all the comments when we change PR

evanlinjin · 2022-06-30T08:44:44Z

@evanlinjin you might be able to push your code directly on @atalw's branch, so we don't lose all the comments when we change PR

That's a good idea, I didn't know I had permission. I'll do some cherry-picking in a bit. 🍒 🌸

evanlinjin · 2022-06-30T09:39:49Z

@evanlinjin you might be able to push your code directly on @atalw's branch, so we don't lose all the comments when we change PR

It seems I do not have the right permissions to push code to his branch.

❯ git push atalw 
ERROR: Permission to atalw/bdk.git denied to evanlinjin.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

danielabrozzoni · 2022-06-30T09:41:02Z

Then we can close this one and move to #644 :)

atalw force-pushed the bip-0039 branch 3 times, most recently from 1138499 to faf6ec4 Compare May 16, 2022 07:58

danielabrozzoni requested changes May 16, 2022

View reviewed changes

vladimirfomene suggested changes May 16, 2022

View reviewed changes

atalw and others added 2 commits May 17, 2022 12:10

MOVEONLY: Move bip39.rs to bip39/mod.rs

a7ea925

atalw force-pushed the bip-0039 branch from faf6ec4 to 58c85e8 Compare May 17, 2022 06:50

f - clippy fixes + rust fmt fix + failing test fix

bf456a4

atalw added 2 commits May 17, 2022 13:36

f - extended mnemonic tests to test parse_in()

ebcc018

BIP39 - Add feature flags for mnemonic languages

612f205

atalw requested a review from danielabrozzoni May 17, 2022 10:08

atalw and others added 4 commits May 17, 2022 18:06

f - rust fmt

22ec999

f - rust fmt wordlists/mod.rs

93cd896

Verify checksum of mnemonic sentence

5d56110

We are currently only checksum the number of words in the mnemonic and their validity. This implementation also checks the checksum to make sure that it was generated from the entropy of the mnemonic.

Test with incorrect entropy size

c12ac39

Make sure `from_entropy_in` produces error if length of entropy bits is less than 128, greater than 256 and not a multipe of 32.

notmandatory assigned vladimirfomene and atalw May 26, 2022

vladimirfomene force-pushed the bip-0039 branch from 1890a70 to 2867ef9 Compare May 27, 2022 07:25

Test invalid mnemonic sentence

d57b09f

Throw error if mnemonic sentence is less than 12 words or greater than 24 words or number of words is not a multiple of six or the contains a word not in wordlist or has invalid checksum.

vladimirfomene force-pushed the bip-0039 branch from 2867ef9 to d57b09f Compare May 27, 2022 10:36

notmandatory mentioned this pull request Jun 7, 2022

W23 BDK Library Team Call bitcoindevkit/.github#8

Closed

16 tasks

notmandatory mentioned this pull request Jun 21, 2022

W25 BDK Library Team Call bitcoindevkit/.github#9

Closed

14 tasks

evanlinjin reviewed Jun 28, 2022

View reviewed changes

evanlinjin mentioned this pull request Jun 29, 2022

BIP39 Implementation #644

Closed

6 tasks

danielabrozzoni closed this Jun 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BIP39 implementation #607

BIP39 implementation #607

atalw commented May 16, 2022 •

edited

Loading

vladimirfomene commented May 16, 2022

atalw commented May 16, 2022 •

edited

Loading

notmandatory commented May 16, 2022 •

edited

Loading

danielabrozzoni commented May 16, 2022

danielabrozzoni left a comment

danielabrozzoni May 16, 2022

atalw May 17, 2022

danielabrozzoni May 16, 2022

evanlinjin Jun 28, 2022

atalw Jun 28, 2022

vladimirfomene Jun 28, 2022

evanlinjin Jun 29, 2022 •

edited

Loading

vladimirfomene left a comment

vladimirfomene May 16, 2022

atalw May 17, 2022

danielabrozzoni Jun 15, 2022

vladimirfomene May 16, 2022

atalw May 17, 2022

vladimirfomene May 19, 2022

vladimirfomene May 16, 2022

atalw May 17, 2022

vladimirfomene left a comment

atalw commented May 17, 2022

evanlinjin Jun 28, 2022 •

edited

Loading

evanlinjin commented Jun 30, 2022 •

edited

Loading

danielabrozzoni commented Jun 30, 2022

evanlinjin commented Jun 30, 2022

evanlinjin commented Jun 30, 2022

danielabrozzoni commented Jun 30, 2022

		sha2 = { version = "0.10.2", optional = true }
		hmac = { version = "0.12.1", optional = true }

BIP39 implementation #607

BIP39 implementation #607

Conversation

atalw commented May 16, 2022 • edited Loading

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

Bugfixes:

vladimirfomene commented May 16, 2022

atalw commented May 16, 2022 • edited Loading

notmandatory commented May 16, 2022 • edited Loading

danielabrozzoni commented May 16, 2022

danielabrozzoni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlinjin Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

vladimirfomene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirfomene left a comment

Choose a reason for hiding this comment

atalw commented May 17, 2022

evanlinjin Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

evanlinjin commented Jun 30, 2022 • edited Loading

danielabrozzoni commented Jun 30, 2022

evanlinjin commented Jun 30, 2022

evanlinjin commented Jun 30, 2022

danielabrozzoni commented Jun 30, 2022

atalw commented May 16, 2022 •

edited

Loading

atalw commented May 16, 2022 •

edited

Loading

notmandatory commented May 16, 2022 •

edited

Loading

evanlinjin Jun 29, 2022 •

edited

Loading

evanlinjin Jun 28, 2022 •

edited

Loading

evanlinjin commented Jun 30, 2022 •

edited

Loading