hdkeychain: BIP32 maximum depth is 255.

BIP32 keys serialize the depth as a uint8 over the wire. I noticed
uint16 was being used and that the depth was being taken modulo 256
during serialization.

This seems like a bug, as the behaviour is not described in the BIP,
and also introduces incompatibilities which can be hard to make sense
of. For example, the parent fingerprint should be 0x00000000 for a key
of depth zero, whereas with the existing code if depth=256, then the
serialization will set 0 but still set a parent fingerprint.

hdkeychain/extendedkey.go

@@ -49,6 +49,9 @@ const (
 	// fingerprint, 4 bytes child number, 32 bytes chain code, and 33 bytes
 	// public/private key data.
 	serializedKeyLen = 4 + 1 + 4 + 4 + 32 + 33 // 78 bytes
+
+	// maxUint8 is the max positive integer which can be serialized in a uint8
+	maxUint8 = 1<<8 - 1
 )
 
 var (
@@ -57,6 +60,11 @@ var (
 	ErrDeriveHardFromPublic = errors.New("cannot derive a hardened key " +
 		"from a public key")
 
+	// ErrDeriveBeyondMaxDepth describes an error in which the caller
+	// has attempted to derive more than 255 keys from a root key.
+	ErrDeriveBeyondMaxDepth = errors.New("cannot derive a key with more than " +
+		"255 indices in its path")
+
 	// ErrNotPrivExtKey describes an error in which the caller attempted
 	// to extract a private key from a public extended key.
 	ErrNotPrivExtKey = errors.New("unable to create private keys from a " +
@@ -101,7 +109,7 @@ type ExtendedKey struct {
 	key       []byte // This will be the pubkey for extended pub keys
 	pubKey    []byte // This will only be set for extended priv keys
 	chainCode []byte
-	depth     uint16
+	depth     uint8
 	parentFP  []byte
 	childNum  uint32
 	version   []byte
@@ -111,7 +119,7 @@ type ExtendedKey struct {
 // newExtendedKey returns a new instance of an extended key with the given
 // fields.  No error checking is performed here as it's only intended to be a
 // convenience method used to create a populated struct.
-func newExtendedKey(version, key, chainCode, parentFP []byte, depth uint16,
+func newExtendedKey(version, key, chainCode, parentFP []byte, depth uint8,
 	childNum uint32, isPrivate bool) *ExtendedKey {
 
 	// NOTE: The pubKey field is intentionally left nil so it is only
@@ -195,6 +203,10 @@ func (k *ExtendedKey) Child(i uint32) (*ExtendedKey, error) {
 	// 3) Public extended key -> Non-hardened child public extended key
 	// 4) Public extended key -> Hardened child public extended key (INVALID!)
 
+	if k.depth == maxUint8 {
+		return nil, ErrDeriveBeyondMaxDepth
+	}
+
 	// Case #4 is invalid, so error out early.
 	// A hardened child extended key may not be created from a public
 	// extended key.
@@ -376,7 +388,6 @@ func (k *ExtendedKey) String() string {
 	}
 
 	var childNumBytes [4]byte
-	depthByte := byte(k.depth % 256)
 	binary.BigEndian.PutUint32(childNumBytes[:], k.childNum)
 
 	// The serialized format is:
@@ -384,7 +395,7 @@ func (k *ExtendedKey) String() string {
 	//   child num (4) || chain code (32) || key data (33) || checksum (4)
 	serializedBytes := make([]byte, 0, serializedKeyLen+4)
 	serializedBytes = append(serializedBytes, k.version...)
-	serializedBytes = append(serializedBytes, depthByte)
+	serializedBytes = append(serializedBytes, k.depth)
 	serializedBytes = append(serializedBytes, k.parentFP...)
 	serializedBytes = append(serializedBytes, childNumBytes[:]...)
 	serializedBytes = append(serializedBytes, k.chainCode...)
@@ -503,7 +514,7 @@ func NewKeyFromString(key string) (*ExtendedKey, error) {
 
 	// Deserialize each of the payload fields.
 	version := payload[:4]
-	depth := uint16(payload[4:5][0])
+	depth := payload[4:5][0]
 	parentFP := payload[5:9]
 	childNum := binary.BigEndian.Uint32(payload[9:13])
 	chainCode := payload[13:45]

hdkeychain/extendedkey_test.go

@@ -12,6 +12,7 @@ import (
 	"bytes"
 	"encoding/hex"
 	"errors"
+	"math"
 	"reflect"
 	"testing"
 
@@ -1013,3 +1014,35 @@ func TestZero(t *testing.T) {
 		}
 	}
 }
+
+// The serialization of a BIP32 key uses uint8 to encode the depth. This implicitly
+// bounds the depth of the tree to 255 derivations. Here we test that an error is
+// returned after 'max uint8'.
+func TestMaximumDepth(t *testing.T) {
+
+	extKey, err := hdkeychain.NewMaster([]byte(`abcd1234abcd1234abcd1234abcd1234`), &chaincfg.MainNetParams)
+	if err != nil {
+		t.Error("MaxDepthTest: Failed to produce test fixture key from string")
+		return
+	}
+
+	for i := uint8(0); i < math.MaxUint8; i++ {
+		newKey, err := extKey.Child(1)
+		if err != nil {
+			t.Error("MaxDepthTest: Failed to produce key required for test")
+			return
+		}
+		extKey = newKey
+	}
+
+	noKey, err := extKey.Child(1)
+	if noKey != nil {
+		t.Error("MaxDepthTest: Deriving 256th key should not succeed")
+		return
+	}
+
+	if err != hdkeychain.ErrDeriveBeyondMaxDepth {
+		t.Error("MaxDepthTest: Received unexpected error during test")
+	}
+
+}