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

hdkeychain: alternate strict BIP32 child derivation method #2845

Merged
merged 1 commit into from Dec 14, 2021

Conversation

chappjc
Copy link
Member

@chappjc chappjc commented Dec 8, 2021

This creates a new child derivation method, ChildBIP32Std, which creates ExtendedKeys with leading zeroes retained, and whos child derivation methods will generate private keys according to BIP32.

@chappjc chappjc changed the title hdkeychain: alternate strict BIP32 constructors hdkeychain: alternate strict BIP32 child derivation method Dec 8, 2021
hdkeychain/extendedkey.go Outdated Show resolved Hide resolved
hdkeychain/extendedkey.go Outdated Show resolved Hide resolved
hdkeychain/extendedkey_test.go Outdated Show resolved Hide resolved
hdkeychain/extendedkey_test.go Outdated Show resolved Hide resolved
@chappjc
Copy link
Member Author

chappjc commented Dec 9, 2021

This is working with decred/dcrdex#1341, so setting ready for review.
Since I needed to amend the initial commit message, I squashed it down now.

@chappjc chappjc marked this pull request as ready for review December 9, 2021 17:34
@chappjc
Copy link
Member Author

chappjc commented Dec 9, 2021

It occurred to me that https://github.com/decred/dcrd/blob/master/hdkeychain/doc.go could use a mention of the new method and how it differs from Child. Although doc.go just says "based on BIP0032", it's worth calling out the difference.

@davecgh
Copy link
Member

davecgh commented Dec 9, 2021

Good point. Probably the README.md as well.

@davecgh
Copy link
Member

davecgh commented Dec 9, 2021

New text reads well.

Copy link
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just had a few minor nonfunctional comments inline.

hdkeychain/extendedkey_test.go Outdated Show resolved Hide resolved
hdkeychain/extendedkey_test.go Outdated Show resolved Hide resolved
hdkeychain/extendedkey_test.go Outdated Show resolved Hide resolved
hdkeychain/extendedkey.go Show resolved Hide resolved
@chappjc
Copy link
Member Author

chappjc commented Dec 13, 2021

Thanks for the review @rstaudt2. I've left the update in a separate commit @davecgh so the incremental diff is reviewable, but am happy to squash when ready.

@davecgh
Copy link
Member

davecgh commented Dec 13, 2021

You can squash it. Github has the ability to see changes on a rebase too.

EDIT: I've reviewed the updates now too and they look good.

Comment on lines +304 to +308
// Test for referential equality.
if pubKey != pubKey.Neuter() {
t.Errorf("Neuter of extended public key returned " +
"different object address")
return
continue
Copy link
Member Author

@chappjc chappjc Dec 13, 2021

Choose a reason for hiding this comment

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

TestBIP0032Vectors provided the template for TestBIP0032Vector4, but it was cleanest not to force them into one test as vector 4 is specific to the difference between "standard" bip32 and our modified variant.
I updated the typo and the unnecessary return here though.

This creates a new ExtendedKey child derivation method, ChildBIP32Std,
which creates an ExtendedKey with leading zeroes retained for strict
compliance with BIP32. This is not intended for Decred wallet use.
@rstaudt2
Copy link
Member

Updates look good to me as well.

@davecgh davecgh merged commit 0673eb5 into decred:master Dec 14, 2021
@chappjc chappjc deleted the bip32std-child branch December 15, 2021 23:49
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.

None yet

4 participants