Depth overflow when deriving the 256th extended keys and onwards

[quapka]

Hi folks,
The serialization format described in BIP32 indirectly restricts valid keys to depth in the range [0, 255], because the depth is serialized as a single byte. The Depth of the keys on lower levels overflows the byte and wraps around.

There might be other derivation code paths in gocoin that achieve the overlow, right now I see at least this one. To see this happening, consider the following test:

diff --git a/lib/btc/wallethd_test.go b/lib/btc/wallethd_test.go
index 27a29e24..bd0bbff7 100644
--- a/lib/btc/wallethd_test.go
+++ b/lib/btc/wallethd_test.go
@@ -6,6 +6,7 @@ This code is taken from:
 package btc

 import (
+    "fmt"
        "bytes"
        "encoding/hex"
        "testing"
@@ -210,6 +211,15 @@ func TestChildren(t *testing.T) {
        }
 }

+func TestDepthOverflow(t *testing.T) {
+       hdwal := MasterKey([]byte("Random seed"), false)
+
+       for i := 0; i < 1000; i++ {
+               hdwal = hdwal.Child(uint32(i | 0x80000000))
+        fmt.Println(hdwal.Depth);
+       }
+}
+
 // benchmarks

 func BenchmarkStringChildPub(b *testing.B) {

Check this bitcoin test case for a similar situations.

While internal "deeper" keys might work fine, their serialization and sharing across other implementation might cause problems.

[piotrnar]

The idea is to leave the responsibility for the depth overflow tracking to the application that uses the HDWallet API.

None of the applications I have been using are even close to overflowing the depth in any real life scenario.
It seems a bit of a waste to add the overflow check into the library code, if the overflow was never to occur.
Besides, what could we do then, other than to trigger a panic?

Or, how would you imagine the code to handle the overflow?

[quapka]

The idea is to leave the responsibility for the depth overflow tracking to the application that uses the HDWallet API.

None of the applications I have been using are even close to overflowing the depth in any real life scenario.

The overflow is guaranteed to happen in the derivation path is long enough, even if not used at the moment. I cannot speak on behalf of whatever applications out there, I am just reporting the possible discrepancy with the original specification.

It seems a bit of a waste to add the overflow check into the library code, if the overflow was never to occur.

Yet checking it at the library level could help any application using it to not derive incorrectly serialized keys.

Besides, what could we do then, other than to trigger a panic?

Or, how would you imagine the code to handle the overflow?

Depends on the interface, of course. Another GO implementation returns an error. I am not much of a gopher myself, so am not sure if panic would be the way to go. I see that the linked code panics on something that cannot be done (xpub -> xprv), this derivation technically can, but then the key depth is incorrect when serialized.