search

btcsuite / btcutil

Provides bitcoin-specific convenience functions and types
475 stars 408 forks source link

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

Closed bibibong closed 2 years ago

bibibong commented 4 years ago

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 commented 3 years ago

We only ever need to pad the private key bytes when further deriving a child extended key. This is already taken care of here: https://github.com/btcsuite/btcutil/blob/4649e4b73b34090724fff8a302751260c20551fd/hdkeychain/extendedkey.go#L250-L258

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 commented 3 years ago

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. https://github.com/btcsuite/btcutil/blob/4649e4b73b34090724fff8a302751260c20551fd/hdkeychain/extendedkey.go#L250-L272 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

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

I got the same result on bip32.org.

image

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 commented 3 years ago

@silencer-Tsai good case👍

devrandom commented 3 years ago

@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 commented 3 years ago

@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. https://github.com/btcsuite/btcutil/blob/4649e4b73b34090724fff8a302751260c20551fd/hdkeychain/extendedkey.go#L257

copy(data[keyLen-len(k.key):], k.key)
klim0v commented 3 years ago

https://github.com/btcsuite/btcutil/pull/161

afk11 commented 3 years ago

@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 commented 2 years ago

Is this issue resolved?

silencer-Tsai commented 2 years ago

Is this issue resolved?

@benma Yes. See https://github.com/btcsuite/btcutil/pull/182

Roasbeef commented 2 years ago

This has been resolved.

Sintayew4 commented 6 months ago

172