BIP39 Implementation

[evanlinjin]

Description

This is a continuation of PR #607 which closes #561

This PR includes commits for own implementation of PBKFD2. I've also modified the .gitignore, I hope that is okay.

Notes to the reviewers

Although complete, I still have some security concerns for the current implementation (please check my comment below).

Checklists

All Submissions:

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

New Features:

• [x] Add pbkfd2 implementation
• [x] I've added docs for the new feature
• [x] I've updated CHANGELOG.md

[evanlinjin]

I still have some security concerns regarding the current implementation (although, I am no security expert, just based on what I've read on the internet). I will list them here, and hopefully someone knowledgeable enough could provide some clarity.

1. Should we avoid using heap memory, and keep everything on the stack? Apparently, heap memory is more prone to exploints. Reference: github.com/shellphish/how2heap

2. Is implementing std::fmt::Display and Debug a good idea? As it may leak the secrets to logs. We can potentially remove these implementations completely, or provide implementations with redacted secrets, or use an a crate such as secrey.

Thank you all in advance!

P.S. Test blockchain::esplora::bdk_blockchain_tests::test_sync_stop_gap_20 seems to fail occasionally.

[danielabrozzoni]

Should we avoid using heap memory, and keep everything on the stack? Apparently, heap memory is more prone to exploints. Reference: github.com/shellphish/how2heap

We could look into that, but it's better if we do so in a new PR. This one is already quite big, and the bigger it gets, the more difficult it is to collect reviews :)

Is implementing std::fmt::Display and Debug a good idea? As it may leak the secrets to logs. We can potentially remove these implementations completely, or provide implementations with redacted secrets, or use an a crate such as secrey.

This, instead, I think should be tackled here: for now, avoiding Debug and Display (or manually implementing a really generic one) should be enough (with an appropriate comment on why we do so). I'd avoid adding YA dependency :)

[evanlinjin]

Another aspect I've been thinking about, is the great majority of the time people will be using English (which shouldn't require Unicode normalization). For the passphrase, we can do a check only (and fail if not normalized).

Since normalization sometimes requires resizing the vector (so it's a heap operation), and for most people, it also means one less dependably.

[evanlinjin]

This, instead, I think should be tackled here: for now, avoiding Debug and Display (or manually implementing a really generic one) should be enough (with an appropriate comment on why we do so). I'd avoid adding YA dependency :)

Addressed in 08e1cc9ebfdb5f9e0f4bfd929eb71b94eedea89c.

[afilini]

Should we avoid using heap memory, and keep everything on the stack?

One advantage of this (on top of the extra safety) is that it would be much easier to then port to embedded hardware. We have many features in bdk which I guess are not really fit for hardware wallets, but mnemonics are for sure one that we'll need to have.

Are you able to do a rough estimation of how much longer/how much harder it would be to implement in this way?

[evanlinjin]

Should we avoid using heap memory, and keep everything on the stack?

One advantage of this (on top of the extra safety) is that it would be much easier to then port to embedded hardware. We have many features in bdk which I guess are not really fit for hardware wallets, but mnemonics are for sure one that we'll need to have.

Are you able to do a rough estimation of how much longer/how much harder it would be to implement in this way?

Less than a week. But I'm stuck into multi descriptor wallet business 😅😂

[afilini]

Yes, multi-descriptor is definitely the priority right now. We'll get back to this once you are done there :)

[rajarshimaitra]

Is this a good idea to have it in bdk_core eventually? Or we wanna do key generation outside of core separately?

[danielabrozzoni]

We closed #561, let's close this one as well :)