hdkeychain private key padding/length decision

[chappjc]

Decred's hdkeychain.(*ExtendedKey).Child method is known to generate private keys with zero padding removed and thus <32 bytes in length, at a rate of 1/256 hardened key derivations.

The btcsuite/btcutil version of ExtendedKey was modified by making a new Derive method and renaming Child to DeriveNonStandard.

We have been using Decred's hdkeychain, which should be fine as long as we are consistent, but another consideration is that in some cases we are using Decred's hdkeychain internal to asset backends to derive keys for specific paths. For example, in client/asset/eth:

dcrdex/client/asset/eth/eth.go

Lines 207 to 225 in 6c947e0

	// m/44'/60'/0'/0/0
	path := []uint32{hdkeychain.HardenedKeyStart + 44,
	hdkeychain.HardenedKeyStart + 60,
	hdkeychain.HardenedKeyStart,
	0,
	0}
	extKey, err := keygen.GenDeepChild(createWalletParams.Seed, path)
	defer extKey.Zero()
	if err != nil {
	return err
	}
	privateKey, err := extKey.SerializedPrivKey()
	defer encode.ClearBytes(privateKey)
	if err != nil {
	return err
	}
	err = importKeyToNode(node, privateKey, createWalletParams.Pass)

And the serialized private key, which may be less than 32 bytes, is used by go-ethereum/crypto.ToECDSA to initialize the ETH account's keystore.

dcrdex/client/asset/eth/node.go

Lines 257 to 259 in 6c947e0

	func importKeyToNode(node *node.Node, privateKey, password []byte) error {
	ecdsaPrivateKey, err := crypto.ToECDSA(privateKey)
	if err != nil {

Because the eth functions have a strict length requirement by default, this fails occasionally as we have seen on CI recently since this missing error check was added less than 2 weeks ago.

While in this instance we might be able to use ToECDSAUnsafe to skip this check, I think we should consider BIP-32 compliant key derivation, at least when used from inside the specific asset packages where we are trying to be compliant.

On the other hand, I would like to stick with the dcrd/hdkeychain package if possible, so another idea might be to introduce a similar DeriveBIP32Standard method that simply omits the stripping of zero padding:

https://github.com/decred/dcrd/blob/08528c28b7ef2d3ce59ea9a79579a01ac5d5cded/hdkeychain/extendedkey.go#L320-L327

Decisions:

• Where do we use bip-32 compliant key derivation? Just where required for non-Decred packages like the eth account? Also for the app seed -> account keys and wallet seeds paths?
• What do we use for bip-32 compliant derivation? btcutil hdkeychain package? Modify dcrd's for a new hdkeychain/v3.1.0 (or whatever)? e.g. https://github.com/decred/dcrd/compare/master...chappjc:bip32std?expand=1

[chappjc]

This can be triggered in client/asset/eth/eth_test.go as follows:

func TestCreateWallet(t *testing.T) {
	try := func() {
		params := &asset.CreateWalletParams{
			DataDir: "./tester",
			Logger:  tLogger,
			Net:     dex.Simnet,
			Pass:    []byte{1, 2, 3},
			Seed:    encode.RandomBytes(32),
			Type:    "geth",
		}
		err := CreateWallet(params)
		os.RemoveAll("./tester")
		if err != nil {
			t.Fatal(err)
		}
	}
	for {
		try()
	}
}

[buck54321]

I was a bit confused because it wasn't clear why leading zeros should change the key derivation, but it's (arguably?) a bug outlined at btcsuite/btcutil#172. @chappjc has verified that btcutil is producing the BIP-compliant results, and his proposed changes to dcrd/hdkeychain produce identically valid results. We should use btcutil until dcrd/hdkeychain is ready.

[JoeGruffins]

I think using the correct implementation with leading zeros left in all over is optimal, because that was the original intention and no one has used this in dex yet.