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

Extra BIP32 test (new test vector #4) #1646

Merged
merged 2 commits into from
Jun 3, 2021
Merged

Extra BIP32 test (new test vector #4) #1646

merged 2 commits into from
Jun 3, 2021

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Jun 3, 2021

An issue was found with some BIP32 implementation, causing incorrect keys with certain seeds (starting with zeroes), see
https://blog.polychainlabs.com/bitcoin,/bip32,/bip39,/kdf/2021/05/17/inconsistent-bip32-derivations.html

Trezor implementation is not affected.

Nonetheless the newly added test vector can be included as an extra test (BIP32 test vector #4).
Test vector 4 is being added to the BIP32 BIP here:
bitcoin/bips#1030

@optout21 optout21 changed the title Extra BIP32 test (test vector 4) Extra BIP32 test (new test vector #4) Jun 3, 2021
@prusnak
Copy link
Member

prusnak commented Jun 3, 2021

For the reference, suggested test vector is not yet merged into the BIP32, but it will be a matter of days before it does. It got several high-profile ACKs already - see bitcoin/bips#1030

@prusnak
Copy link
Member

prusnak commented Jun 3, 2021

@catenocrypt can you please add the test for path m/0H/1H as well? It is included in the test vector, but not included in your code.

See: https://github.com/bitcoin/bips/pull/1030/files#diff-6440a1bbe14b80097ee2af26d776827f9114db1a961ca17b364c7e9f2a693957R287-R289

@optout21
Copy link
Contributor Author

optout21 commented Jun 3, 2021

@catenocrypt can you please add the test for path m/0H/1H as well? It is included in the test vector, but not included in your code.

I have included this too now. The existing vector 1--3 do not include all paths, that's why I have not included all.

@prusnak prusnak merged commit c4bf522 into trezor:master Jun 3, 2021
@prusnak
Copy link
Member

prusnak commented Jun 3, 2021

Thanks!

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