diff --git a/signer_client.go b/signer_client.go index 0a605b4..f401909 100644 --- a/signer_client.go +++ b/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), diff --git a/signer_client_test.go b/signer_client_test.go new file mode 100644 index 0000000..5af4d2e --- /dev/null +++ b/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, + ) + }) +}