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.

Sign up for GitHub

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

fix leading zero and add test #161

Closed
wants to merge 7 commits into from
Closed

fix leading zero and add test #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
51800c0
fix leading zero and add test for here
Mar 3, 2020
150b328
correct grammatical error
Mar 19, 2020
c4d0a59
move test
Apr 4, 2020
19cbbd5
add a flag to switch old and new versions with a fixed error of leadi…
Apr 10, 2020
87a3d91
allow leadingZeroBug slice length greater than 1 value
Apr 12, 2020
07070e2
add a description for the new second optional parameter for the Child…
Apr 15, 2020
ed5f27f
add 2nd parameter as true for Child(i uint32, fixLeadingZeroBug ...bo…
Apr 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions hdkeychain/bench_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func BenchmarkDeriveHardened(b *testing.B) {
b.StartTimer()

for i := 0; i < b.N; i++ {
masterKey.Child(hdkeychain.HardenedKeyStart)
masterKey.Child(hdkeychain.HardenedKeyStart, true)
}
}

Expand All @@ -41,7 +41,7 @@ func BenchmarkDeriveNormal(b *testing.B) {
b.StartTimer()

for i := 0; i < b.N; i++ {
masterKey.Child(0)
masterKey.Child(0, true)
}
}

Expand Down
10 changes: 5 additions & 5 deletions hdkeychain/example_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func Example_defaultWalletLayout() {

// Derive the extended key for account 0. This gives the path:
// m/0H
acct0, err := masterKey.Child(hdkeychain.HardenedKeyStart + 0)
acct0, err := masterKey.Child(hdkeychain.HardenedKeyStart+0, true)
if err != nil {
fmt.Println(err)
return
Expand All @@ -80,7 +80,7 @@ func Example_defaultWalletLayout() {
// Derive the extended key for the account 0 external chain. This
// gives the path:
// m/0H/0
acct0Ext, err := acct0.Child(0)
acct0Ext, err := acct0.Child(0, true)
if err != nil {
fmt.Println(err)
return
Expand All @@ -89,7 +89,7 @@ func Example_defaultWalletLayout() {
// Derive the extended key for the account 0 internal chain. This gives
// the path:
// m/0H/1
acct0Int, err := acct0.Child(1)
acct0Int, err := acct0.Child(1, true)
if err != nil {
fmt.Println(err)
return
Expand All @@ -101,7 +101,7 @@ func Example_defaultWalletLayout() {
// Derive the 10th extended key for the account 0 external chain. This
// gives the path:
// m/0H/0/10
acct0Ext10, err := acct0Ext.Child(10)
acct0Ext10, err := acct0Ext.Child(10, true)
if err != nil {
fmt.Println(err)
return
Expand All @@ -110,7 +110,7 @@ func Example_defaultWalletLayout() {
// Derive the 1st extended key for the account 0 internal chain. This
// gives the path:
// m/0H/1/0
acct0Int0, err := acct0Int.Child(0)
acct0Int0, err := acct0Int.Child(0, true)
if err != nil {
fmt.Println(err)
return
Expand Down
11 changes: 10 additions & 1 deletion hdkeychain/extendedkey.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,11 @@ func (k *ExtendedKey) ParentFingerprint() uint32 {
// index does not derive to a usable child. The ErrInvalidChild error will be
// returned if this should occur, and the caller is expected to ignore the
// invalid child and simply increment to the next index.
func (k *ExtendedKey) Child(i uint32) (*ExtendedKey, error) {
//
// Use the 2nd parameter to fix the bug with leading zeros. This change was made
// as an optional parameter in order to maintain backward compatibility and avoid
// losing wallets created before fixing this bug.
func (k *ExtendedKey) Child(i uint32, fixLeadingZeroBug ...bool) (*ExtendedKey, error) {
Copy link
Member

Choose a reason for hiding this comment

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

This solution strikes me as being...inelegant. With this for the foreseeable future, the API is forced to carry a parameter that conditionally fixes an issue of spec compliance. This isn't backwards compatible as the comment indicates since this is actually itself a breaking API change, so it warrants a v2 major version bump IMO.

Another option would be to add a new method that always applies the leading zero fix, with the existing public one calling that with the fix toggled off.

Copy link
Author

Choose a reason for hiding this comment

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

This solution strikes me as being...inelegant. With this for the foreseeable future, the API is forced to carry a parameter that conditionally fixes an issue of spec compliance. This isn't backwards compatible as the comment indicates since this is actually itself a breaking API change, so it warrants a v2 major version bump IMO.

Can you explain why this cannot be considered backward compatible? This parameter is optional and does not correct the default error if it is absent.

Copy link

Choose a reason for hiding this comment

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

If someone created an interface in their app under the assumption that ExtendedKey fulfills it, and their interface has the call signature of Child(i uint32) (*TheirInterface, error) suddenly ExtendedKey no longer fulfills it.

Therefore it is breaking.

Copy link
Member

Choose a reason for hiding this comment

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

I've come around to this approach, if you can update this PR before #182 does, I think it can land.

// Prevent derivation of children beyond the max allowed depth.
if k.depth == maxUint8 {
return nil, ErrDeriveBeyondMaxDepth
Expand Down Expand Up @@ -295,6 +299,11 @@ func (k *ExtendedKey) Child(i uint32) (*ExtendedKey, error) {
ilNum.Add(ilNum, keyNum)
ilNum.Mod(ilNum, btcec.S256().N)
childKey = ilNum.Bytes()
// Correction a key-length with leading zero
if len(childKey) < 32 && len(fixLeadingZeroBug) > 0 && fixLeadingZeroBug[0] == true {
extra := make([]byte, 32-len(childKey))
childKey = append(extra, childKey...)
}
isPrivate = true
} else {
// Case #3.
Expand Down
21 changes: 15 additions & 6 deletions hdkeychain/extendedkey_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ func TestBIP0032Vectors(t *testing.T) {
testVec1MasterHex := "000102030405060708090a0b0c0d0e0f"
testVec2MasterHex := "fffcf9f6f3f0edeae7e4e1dedbd8d5d2cfccc9c6c3c0bdbab7b4b1aeaba8a5a29f9c999693908d8a8784817e7b7875726f6c696663605d5a5754514e4b484542"
testVec3MasterHex := "4b381541583be4423346c643850da4b320e46a87ae3d2a4e6da11eba819cd4acba45d239319ac14f863b8d5ab5a0d0c64d2e8a1e7d1457df2e5a3c51c73235be"
testVec3aMasterHex := "57fb1e450b8afb95c62afbcd49e4100d6790e0822b8905608679180ac34ca0bd45bf7ccc6c5f5218236d0eb93afc78bd117b9f02a6b7df258ea182dfaef5aad7"
hkStart := uint32(0x80000000)

tests := []struct {
Expand Down Expand Up @@ -153,6 +154,14 @@ func TestBIP0032Vectors(t *testing.T) {
wantPriv: "xprv9uPDJpEQgRQfDcW7BkF7eTya6RPxXeJCqCJGHuCJ4GiRVLzkTXBAJMu2qaMWPrS7AANYqdq6vcBcBUdJCVVFceUvJFjaPdGZ2y9WACViL4L",
net: &chaincfg.MainNetParams,
},
{
name: "test vector 3 chain m/44H/60H/0H",
master: testVec3aMasterHex,
path: []uint32{hkStart + 44, hkStart + 60, hkStart},
wantPub: "xpub6CpsfWjghR6XdCB8yDq7jQRpRKEDP2LT3ZRUgURF9g5xevB7YoTpogkFRqq5nQtVSN8YCMZo2CD8u4zCaxRv85ctCWmzEi9gQ5DBhBFaTNo",
wantPriv: "xprv9yqXG1Cns3YEQi6fsCJ7NGV5sHPiyZcbgLVst61dbLYyn7qy1G9aFtRmaYp481ounqnVf9Go2ymQ4gmxZLEwYSRhU868aDk4ZxzGvqHJVhe",
net: &chaincfg.MainNetParams,
},

// Test vector 1 - Testnet
{
Expand Down Expand Up @@ -224,7 +233,7 @@ tests:

for _, childNum := range test.path {
var err error
extKey, err = extKey.Child(childNum)
extKey, err = extKey.Child(childNum, true)
if err != nil {
t.Errorf("err: %v", err)
continue tests
Expand Down Expand Up @@ -381,7 +390,7 @@ tests:

for _, childNum := range test.path {
var err error
extKey, err = extKey.Child(childNum)
extKey, err = extKey.Child(childNum, true)
if err != nil {
t.Errorf("err: %v", err)
continue tests
Expand Down Expand Up @@ -500,7 +509,7 @@ tests:

for _, childNum := range test.path {
var err error
extKey, err = extKey.Child(childNum)
extKey, err = extKey.Child(childNum, true)
if err != nil {
t.Errorf("err: %v", err)
continue tests
Expand Down Expand Up @@ -830,7 +839,7 @@ func TestErrors(t *testing.T) {
}

// Deriving a hardened child extended key should fail from a public key.
_, err = pubKey.Child(HardenedKeyStart)
_, err = pubKey.Child(HardenedKeyStart, true)
if err != ErrDeriveHardFromPublic {
t.Fatalf("Child: mismatched error -- got: %v, want: %v",
err, ErrDeriveHardFromPublic)
Expand Down Expand Up @@ -1052,14 +1061,14 @@ func TestMaximumDepth(t *testing.T) {
t.Fatalf("extendedkey depth %d should match expected value %d",
extKey.Depth(), i)
}
newKey, err := extKey.Child(1)
newKey, err := extKey.Child(1, true)
if err != nil {
t.Fatalf("Child: unexpected error: %v", err)
}
extKey = newKey
}

noKey, err := extKey.Child(1)
noKey, err := extKey.Child(1, true)
if err != ErrDeriveBeyondMaxDepth {
t.Fatalf("Child: mismatched error: want %v, got %v",
ErrDeriveBeyondMaxDepth, err)
Expand Down