why not add leading zero bytes to make childkey 32 bytes long?

[bibibong]

See extendedkey.go:297
The bytes length may be smaller than 32. Is there any bug when deriving hardened private key?
The derived key with padding differs.

[onyb]

We only ever need to pad the private key bytes when further deriving a child extended key. This is already taken care of here:

btcutil/hdkeychain/extendedkey.go

Lines 250 to 258 in 4649e4b

	keyLen := 33
	data := make([]byte, keyLen+4)
	if isChildHardened {
	// Case #1.
	// When the child is a hardened child, the key is known to be a
	// private key due to the above early return. Pad it with a
	// leading zero as required by [BIP32] for deriving the child.
	copy(data[1:], k.key)
	} else {

The other thing we care about is whether the integer corresponding to the private child key bytes is valid for the secp256k1 elliptic curve. Here it doesn't matter if the serialization was done in 32 bytes or less, as long as the underlying crypto library can convert it to a big.Int number.

Note that the hdkeychain package is tested against the official BIP-0032 test vectors, so I would be very surprised if there is a bug of the nature you describe.

Let me know if this answers your question. Otherwise, if you can provide a failing test case, I'll gladly take a look.

[silencer-Tsai]

From my point of view, this implementatin might get a wrong result in some case. As the BIP32 said, in the case of computing a child extended hardened private key from the parent private key, I = HMAC-SHA512(Key = cpar, Data = 0x00 || ser256(kpar) || ser32(i)).
As for implementation, see extendedkey.go.

btcutil/hdkeychain/extendedkey.go

Lines 250 to 272 in 4649e4b

	keyLen := 33
	data := make([]byte, keyLen+4)
	if isChildHardened {
	// Case #1.
	// When the child is a hardened child, the key is known to be a
	// private key due to the above early return. Pad it with a
	// leading zero as required by [BIP32] for deriving the child.
	copy(data[1:], k.key)
	} else {
	// Case #2 or #3.
	// This is either a public or private extended key, but in
	// either case, the data which is used to derive the child key
	// starts with the secp256k1 compressed public key bytes.
	copy(data, k.pubKeyBytes())
	}
	binary.BigEndian.PutUint32(data[keyLen:], i)
	// Take the HMAC-SHA512 of the current key's chain code and the derived
	// data:
	// I = HMAC-SHA512(Key = chainCode, Data = data)
	hmac512 := hmac.New(sha512.New, k.chainCode)
	hmac512.Write(data)
	ilr := hmac512.Sum(nil)

If the length of k.key is less than 32, for example 31, then the Data = 0x00 || 31 bytes || 0x00 || ser32(i). ser256(kpar) just performs like another key leading with this 31 bytes and followed one 0x00. This is not consistent with the regulations of BIP32.

Here is a test vector.

Seed(Hex): 1cae71ac5ed584ff88a078a119512d12bb61e5398521785e123b6d08809d44b2

• Chain m
  • ext pub: xpub661MyMwAqRbcEmwzw5S7mHW26Urp4kngnBFwoZUXSgakbHGs5bgZ7RYsqX9nyCP3YKqrJ2gVfaJc6waZBJC2VFsnmJB7iPSNA5LvmZpcBcQ
  • ext priv: xprv9s21ZrQH143K2HsXq3u7Q9ZHYT2KfJ4qQxLM1B4utM3miUwiY4NJZdEPzDpzbH7xxtMr3QfT2VH13rabABKkw1eLU83YC1QMeXsX3DBe2yP
• Chain m/44'
  • ext pub: xpub695cM7RLktQbMS9DgS2SmkGF6W4msU7dW3ZkshyPV5PVWsWyoNxtvSBtm6VPSbcBR3a1NSp9BYy3v3QhUTi8T1HDa6rMShYmF682N6BaYpk
  • ext priv: xprv9v6FwbtSvWrJ8x4kaQVSQcKWYUEHU1Pn8peA5KZmvjrWe5BqFqeeNdsQuqDXN9JqeAxmAvs5v682JLDQZJsB8Up4guNVPSGidN19N2iH1Lr
• Chain m/44'/0'
  • ext pub: xpub6Begh3MMx7oX3A7A1m6cCu2tXgqTPVQgoxQKx7TCsUreVbSqhxbTPt42whTwYgSmnSuVkMzLeBa734foBe6QvzcsoddEVyad9N5iW4ZBqSh
  • ext priv: xprv9xfLHXpU7kFDpg2gujZbqm69yezxz2gqSjUj9j3bK9Kfco7hARHCr5jZ6RAWF6E6drNhziP3UJ3iWGaEc7whBuqkzgzytsdkePT6b6iWKki

In Chain m/44'/0', this implementaion gets a wrong result. This is because the length of the serialization of its parant private key is 31 and we are deriving a hardened child private key. I tried to fix this bug with leading 0x00 and got another result.

Seed(Hex): 1cae71ac5ed584ff88a078a119512d12bb61e5398521785e123b6d08809d44b2

• Chain m/44'/0'
  • ext pub: xpub6Begh3MMx7oX2KoJb5D9sfJruuqsqsrqBZTBBznYMRfk7RBo8EcKDodYJm529ykTr2wrK1KBKXCbdSPu74pA37hZmPxkCP3hbEJBuqJgruy
  • ext priv: xprv9xfLHXpU7kFDoqiqV3g9WXN8Mt1PSR8ypLXaPcNvo68mEcreahJ4g1K4TWqn4qu6HCKByGeivW9neAEzSS7idYdpGaGXJgvb79fxvV4qhse

I got the same result on bip32.org.

As the result shows, this implementation might calculate a child key different from the one specified in BIP32 in some scenarios. As for the official BIP-0032 test vectors, the length of the serialization of their private key are all 32.

[bibibong]

@silencer-Tsai good case👍

[devrandom]

@ksedgwic just ran into this issue, and I noticed that there is this already open issue. I submitted a PR just now, linked above.

[silencer-Tsai]

@ksedgwic just ran into this issue, and I noticed that there is this already open issue. I submitted a PR just now, linked above.

I try to replace the original code with the following code. Hope to help.

btcutil/hdkeychain/extendedkey.go

Line 257 in 4649e4b

copy(data[1:], k.key)

copy(data[keyLen-len(k.key):], k.key)

[klim0v]

#161

[afk11]

@silencer-Tsai if you haven't already, please submit it to BIP32 with the correct result? People randomly implementing and copy/pasting fixtures might run into it then. Plus a note to explain what to look for if it fails? :)

Always thought it'd be cool to write a standard testing interface for bitcoin libraries, implement the interface for each lib, then test against vectors in the suite. This kind of bug (non-portable results due to implementation quirks) sucks and can be hard to detect for a long time if you always use the same software.. I can imagine a lot of stress trying to find out why electrum shows an empty balance but the servers say otherwise!

[benma]

Is this issue resolved?

[silencer-Tsai]

Is this issue resolved?

@benma Yes. See #182

[Roasbeef]

This has been resolved.

[Sintayew4]

#172