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

Correct BIP-32 derivation issue #182

Merged
merged 1 commit into from
Nov 4, 2020

Conversation

devrandom
Copy link
Contributor

Fixes issue #172.

This issue affects 1 in 256 hardened key derivations. For standard BIP-44 accounts, this affects the coin and account derivations, so 1 in 128 accounts will not be compatible with other BIP-44 implementations.

The intention is that wallet implementations using btcutil can use the IsAffectedByIssue172 call to check if they are affected, and potentially migrate (or ask the user to create a new wallet).

@silencer-Tsai
Copy link

Well done!

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this set of changes! Picking this over the older one assuming that we can get this one in sooner. Also the commit message should have the package/folder prefix as specified in the contribution guidelines.

copy(data[1:], k.key)
// Additionally, right align it if it's shorter than 32 bytes.
offset := 33 - len(k.key)
copy(data[offset:], k.key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than silently changing the behavior of this method, which can cause issues for any wallet that relies on this quirk as is (say when rescanning), I think we should instead break this method, and force callers to choose one or the other.

In other words: add a new variable to this method, and/or a helper func that assumes a new default value for this variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed ChildNonStandard to DeriveNonStandard and Child to Derive. added a usage note

hdkeychain/extendedkey_test.go Show resolved Hide resolved
hdkeychain/extendedkey_test.go Show resolved Hide resolved
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🏆

@Roasbeef
Copy link
Member

cc @guggero

Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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.

4 participants