lndclient: attach key locator when any component is non-zero (lnd-21-0)

signer_client.go

@@ -271,19 +271,24 @@ func marshallSignDescriptors(signDescriptors []*SignDescriptor,
 	//nolint:lll
 	// fullDescriptor is a helper method that creates a fully populated sign
 	// descriptor that includes both the public key and the key locator (if
-	// available). For the locator we explicitly check that both the family
-	// _and_ the index is non-zero. In some applications it's possible that
-	// the family is always set (because only a specific family is used),
-	// but the index might be zero because it's the first key, or because it
-	// isn't known at that particular moment.
+	// available). The locator is attached whenever either the family _or_
+	// the index is non-zero; only the all-zero KeyLocator (which is the
+	// struct's zero value and therefore indistinguishable from "unset") is
+	// omitted. A prior version of this helper required both components to
+	// be non-zero, which silently dropped the locator for valid keys whose
+	// slot has a zero component — most notably LND's node identity key at
+	// KeyFamilyNodeKey (6) / index 0. Signers that must re-derive the key
+	// from its locator (restored-from-seed wallets, or identity-key paths
+	// where only the family is meaningful) could not resolve those keys.
 	// We aim to be compatible with this method in lnd's wallet:
 	// https://github.com/lightningnetwork/lnd/blob/master/lnwallet/btcwallet/signer.go#L286
-	// Because we know all custom families (0 to 255) are derived at wallet
-	// creation, and the very first index of each family/account is always
-	// derived, we know that only using the public key for that very first
-	// index will work. But for a freshly initialized wallet (e.g. restored
-	// from seed), we won't know any indexes greater than 0, so we _need_ to
-	// also specify the key locator and not just the public key.
+	// The very first index of each custom family is always derived at
+	// wallet creation, so for callers that know the pubkey the backend can
+	// resolve the key without a locator. But wallets restored from seed
+	// won't have pre-derived indexes greater than 0, and keys pinned to a
+	// fixed family slot (like the node identity key) must have the locator
+	// forwarded so the signer can re-derive them — hence we forward a
+	// locator whenever the caller has populated even one component.
 	fullDescriptor := func(
 		d keychain.KeyDescriptor) *signrpc.KeyDescriptor {
 
@@ -292,7 +297,7 @@ func marshallSignDescriptors(signDescriptors []*SignDescriptor,
 			keyDesc.RawKeyBytes = d.PubKey.SerializeCompressed()
 		}
 
-		if d.KeyLocator.Family != 0 && d.KeyLocator.Index != 0 {
+		if d.KeyLocator.Family != 0 || d.KeyLocator.Index != 0 {
 			keyDesc.KeyLoc = &signrpc.KeyLocator{
 				KeyFamily: int32(d.KeyLocator.Family),
 				KeyIndex:  int32(d.KeyLocator.Index),

signer_client_test.go

@@ -0,0 +1,203 @@
+package lndclient
+
+import (
+	"testing"
+
+	"github.com/btcsuite/btcd/btcec/v2"
+	"github.com/btcsuite/btcd/wire"
+	"github.com/lightningnetwork/lnd/keychain"
+	"github.com/stretchr/testify/require"
+)
+
+// newTestPubKey returns a fresh public key for use in descriptor
+// marshalling tests.
+func newTestPubKey(t *testing.T) *btcec.PublicKey {
+	t.Helper()
+
+	priv, err := btcec.NewPrivateKey()
+	require.NoError(t, err)
+
+	return priv.PubKey()
+}
+
+// TestMarshallSignDescriptorsFullLocator pins the contract of the full
+// descriptor marshaller used by SignOutputRawKeyLocator: the key locator
+// is attached whenever either the family or the index is non-zero, and
+// only the all-zero KeyLocator (the struct's zero value, indistinguishable
+// from "unset") is omitted.
+//
+// The family-6 / index-0 case is the regression guard. A prior version of
+// the helper required both components to be non-zero and therefore
+// silently dropped the locator for LND's node identity key slot. Signers
+// that must re-derive the key from its locator — restored-from-seed
+// wallets, or family-pinned identity keys — could not resolve the key.
+func TestMarshallSignDescriptorsFullLocator(t *testing.T) {
+	t.Parallel()
+
+	pubKey := newTestPubKey(t)
+	output := &wire.TxOut{PkScript: []byte{0x51}, Value: 1000}
+
+	tests := []struct {
+		name          string
+		family        keychain.KeyFamily
+		index         uint32
+		omitPubKey    bool
+		expectLocator bool
+	}{
+		{
+			name:          "all-zero locator is omitted",
+			family:        0,
+			index:         0,
+			expectLocator: false,
+		},
+		{
+			name:          "family-only locator is attached",
+			family:        keychain.KeyFamilyNodeKey,
+			index:         0,
+			expectLocator: true,
+		},
+		{
+			name:          "index-only locator is attached",
+			family:        0,
+			index:         5,
+			expectLocator: true,
+		},
+		{
+			name:          "fully populated locator is attached",
+			family:        keychain.KeyFamilyNodeKey,
+			index:         5,
+			expectLocator: true,
+		},
+		{
+			name: "family-only locator is attached with " +
+				"nil pubkey",
+			family:        keychain.KeyFamilyNodeKey,
+			index:         0,
+			omitPubKey:    true,
+			expectLocator: true,
+		},
+		{
+			name: "index-only locator is attached with " +
+				"nil pubkey",
+			family:        0,
+			index:         5,
+			omitPubKey:    true,
+			expectLocator: true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			keyDesc := keychain.KeyDescriptor{
+				KeyLocator: keychain.KeyLocator{
+					Family: tc.family,
+					Index:  tc.index,
+				},
+			}
+			if !tc.omitPubKey {
+				keyDesc.PubKey = pubKey
+			}
+
+			descs := []*SignDescriptor{{
+				KeyDesc: keyDesc,
+				Output:  output,
+			}}
+
+			rpcDescs := marshallSignDescriptors(descs, true)
+			require.Len(t, rpcDescs, 1)
+
+			rpcKeyDesc := rpcDescs[0].KeyDesc
+			require.NotNil(t, rpcKeyDesc)
+
+			if tc.omitPubKey {
+				require.Empty(t, rpcKeyDesc.RawKeyBytes)
+			} else {
+				require.Equal(t,
+					pubKey.SerializeCompressed(),
+					rpcKeyDesc.RawKeyBytes,
+				)
+			}
+
+			if !tc.expectLocator {
+				require.Nil(t, rpcKeyDesc.KeyLoc)
+				return
+			}
+
+			require.NotNil(t, rpcKeyDesc.KeyLoc)
+			require.Equal(t,
+				int32(tc.family),
+				rpcKeyDesc.KeyLoc.KeyFamily,
+			)
+			require.Equal(t,
+				int32(tc.index),
+				rpcKeyDesc.KeyLoc.KeyIndex,
+			)
+		})
+	}
+}
+
+// TestMarshallSignDescriptorsPartialOnlyOneOf verifies that the partial
+// descriptor marshaller used by the legacy SignOutputRaw populates either
+// the public key or the locator, but never both. Applications like Loop
+// have adjusted themselves to this behavior, which is why the fix for the
+// locator-drop bug lives in the full descriptor path instead.
+func TestMarshallSignDescriptorsPartialOnlyOneOf(t *testing.T) {
+	t.Parallel()
+
+	pubKey := newTestPubKey(t)
+	output := &wire.TxOut{PkScript: []byte{0x51}, Value: 1000}
+
+	t.Run("pubkey present drops locator", func(t *testing.T) {
+		t.Parallel()
+
+		descs := []*SignDescriptor{{
+			KeyDesc: keychain.KeyDescriptor{
+				PubKey: pubKey,
+				KeyLocator: keychain.KeyLocator{
+					Family: keychain.KeyFamilyNodeKey,
+					Index:  5,
+				},
+			},
+			Output: output,
+		}}
+
+		rpcDescs := marshallSignDescriptors(descs, false)
+		require.Len(t, rpcDescs, 1)
+
+		require.Equal(t,
+			pubKey.SerializeCompressed(),
+			rpcDescs[0].KeyDesc.RawKeyBytes,
+		)
+		require.Nil(t, rpcDescs[0].KeyDesc.KeyLoc)
+	})
+
+	t.Run("pubkey absent uses locator", func(t *testing.T) {
+		t.Parallel()
+
+		descs := []*SignDescriptor{{
+			KeyDesc: keychain.KeyDescriptor{
+				KeyLocator: keychain.KeyLocator{
+					Family: keychain.KeyFamilyNodeKey,
+					Index:  5,
+				},
+			},
+			Output: output,
+		}}
+
+		rpcDescs := marshallSignDescriptors(descs, false)
+		require.Len(t, rpcDescs, 1)
+
+		require.Empty(t, rpcDescs[0].KeyDesc.RawKeyBytes)
+		require.NotNil(t, rpcDescs[0].KeyDesc.KeyLoc)
+		require.Equal(t,
+			int32(keychain.KeyFamilyNodeKey),
+			rpcDescs[0].KeyDesc.KeyLoc.KeyFamily,
+		)
+		require.Equal(t,
+			int32(5),
+			rpcDescs[0].KeyDesc.KeyLoc.KeyIndex,
+		)
+	})
+}