Skip to content

Commit a008faf

Browse files
RoasbeefRoasbeef
authored
Merge pull request #269 from lightninglabs/signoutput-fix
lndclient: attach key locator when any component is non-zero
2 parents 1be689d + 18d18df commit a008faf

2 files changed

Lines changed: 220 additions & 12 deletions

File tree

‎signer_client.go‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -252,19 +252,24 @@ func marshallSignDescriptors(signDescriptors []*SignDescriptor,
252252
//nolint:lll
253253
// fullDescriptor is a helper method that creates a fully populated sign
254254
// descriptor that includes both the public key and the key locator (if
255-
// available). For the locator we explicitly check that both the family
256-
// _and_ the index is non-zero. In some applications it's possible that
257-
// the family is always set (because only a specific family is used),
258-
// but the index might be zero because it's the first key, or because it
259-
// isn't known at that particular moment.
255+
// available). The locator is attached whenever either the family _or_
256+
// the index is non-zero; only the all-zero KeyLocator (which is the
257+
// struct's zero value and therefore indistinguishable from "unset") is
258+
// omitted. A prior version of this helper required both components to
259+
// be non-zero, which silently dropped the locator for valid keys whose
260+
// slot has a zero component — most notably LND's node identity key at
261+
// KeyFamilyNodeKey (6) / index 0. Signers that must re-derive the key
262+
// from its locator (restored-from-seed wallets, or identity-key paths
263+
// where only the family is meaningful) could not resolve those keys.
260264
// We aim to be compatible with this method in lnd's wallet:
261265
// https://github.com/lightningnetwork/lnd/blob/master/lnwallet/btcwallet/signer.go#L286
262-
// Because we know all custom families (0 to 255) are derived at wallet
263-
// creation, and the very first index of each family/account is always
264-
// derived, we know that only using the public key for that very first
265-
// index will work. But for a freshly initialized wallet (e.g. restored
266-
// from seed), we won't know any indexes greater than 0, so we _need_ to
267-
// also specify the key locator and not just the public key.
266+
// The very first index of each custom family is always derived at
267+
// wallet creation, so for callers that know the pubkey the backend can
268+
// resolve the key without a locator. But wallets restored from seed
269+
// won't have pre-derived indexes greater than 0, and keys pinned to a
270+
// fixed family slot (like the node identity key) must have the locator
271+
// forwarded so the signer can re-derive them — hence we forward a
272+
// locator whenever the caller has populated even one component.
268273
fullDescriptor := func(
269274
d keychain.KeyDescriptor) *signrpc.KeyDescriptor {
270275

@@ -273,7 +278,7 @@ func marshallSignDescriptors(signDescriptors []*SignDescriptor,
273278
keyDesc.RawKeyBytes = d.PubKey.SerializeCompressed()
274279
}
275280

276-
if d.KeyLocator.Family != 0 && d.KeyLocator.Index != 0 {
281+
if d.KeyLocator.Family != 0 || d.KeyLocator.Index != 0 {
277282
keyDesc.KeyLoc = &signrpc.KeyLocator{
278283
KeyFamily: int32(d.KeyLocator.Family),
279284
KeyIndex: int32(d.KeyLocator.Index),

‎signer_client_test.go‎

Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,203 @@
1+
package lndclient
2+
3+
import (
4+
"testing"
5+
6+
"github.com/btcsuite/btcd/btcec/v2"
7+
"github.com/btcsuite/btcd/wire"
8+
"github.com/lightningnetwork/lnd/keychain"
9+
"github.com/stretchr/testify/require"
10+
)
11+
12+
// newTestPubKey returns a fresh public key for use in descriptor
13+
// marshalling tests.
14+
func newTestPubKey(t *testing.T) *btcec.PublicKey {
15+
t.Helper()
16+
17+
priv, err := btcec.NewPrivateKey()
18+
require.NoError(t, err)
19+
20+
return priv.PubKey()
21+
}
22+
23+
// TestMarshallSignDescriptorsFullLocator pins the contract of the full
24+
// descriptor marshaller used by SignOutputRawKeyLocator: the key locator
25+
// is attached whenever either the family or the index is non-zero, and
26+
// only the all-zero KeyLocator (the struct's zero value, indistinguishable
27+
// from "unset") is omitted.
28+
//
29+
// The family-6 / index-0 case is the regression guard. A prior version of
30+
// the helper required both components to be non-zero and therefore
31+
// silently dropped the locator for LND's node identity key slot. Signers
32+
// that must re-derive the key from its locator — restored-from-seed
33+
// wallets, or family-pinned identity keys — could not resolve the key.
34+
func TestMarshallSignDescriptorsFullLocator(t *testing.T) {
35+
t.Parallel()
36+
37+
pubKey := newTestPubKey(t)
38+
output := &wire.TxOut{PkScript: []byte{0x51}, Value: 1000}
39+
40+
tests := []struct {
41+
name string
42+
family keychain.KeyFamily
43+
index uint32
44+
omitPubKey bool
45+
expectLocator bool
46+
}{
47+
{
48+
name: "all-zero locator is omitted",
49+
family: 0,
50+
index: 0,
51+
expectLocator: false,
52+
},
53+
{
54+
name: "family-only locator is attached",
55+
family: keychain.KeyFamilyNodeKey,
56+
index: 0,
57+
expectLocator: true,
58+
},
59+
{
60+
name: "index-only locator is attached",
61+
family: 0,
62+
index: 5,
63+
expectLocator: true,
64+
},
65+
{
66+
name: "fully populated locator is attached",
67+
family: keychain.KeyFamilyNodeKey,
68+
index: 5,
69+
expectLocator: true,
70+
},
71+
{
72+
name: "family-only locator is attached with " +
73+
"nil pubkey",
74+
family: keychain.KeyFamilyNodeKey,
75+
index: 0,
76+
omitPubKey: true,
77+
expectLocator: true,
78+
},
79+
{
80+
name: "index-only locator is attached with " +
81+
"nil pubkey",
82+
family: 0,
83+
index: 5,
84+
omitPubKey: true,
85+
expectLocator: true,
86+
},
87+
}
88+
89+
for _, tc := range tests {
90+
t.Run(tc.name, func(t *testing.T) {
91+
t.Parallel()
92+
93+
keyDesc := keychain.KeyDescriptor{
94+
KeyLocator: keychain.KeyLocator{
95+
Family: tc.family,
96+
Index: tc.index,
97+
},
98+
}
99+
if !tc.omitPubKey {
100+
keyDesc.PubKey = pubKey
101+
}
102+
103+
descs := []*SignDescriptor{{
104+
KeyDesc: keyDesc,
105+
Output: output,
106+
}}
107+
108+
rpcDescs := marshallSignDescriptors(descs, true)
109+
require.Len(t, rpcDescs, 1)
110+
111+
rpcKeyDesc := rpcDescs[0].KeyDesc
112+
require.NotNil(t, rpcKeyDesc)
113+
114+
if tc.omitPubKey {
115+
require.Empty(t, rpcKeyDesc.RawKeyBytes)
116+
} else {
117+
require.Equal(t,
118+
pubKey.SerializeCompressed(),
119+
rpcKeyDesc.RawKeyBytes,
120+
)
121+
}
122+
123+
if !tc.expectLocator {
124+
require.Nil(t, rpcKeyDesc.KeyLoc)
125+
return
126+
}
127+
128+
require.NotNil(t, rpcKeyDesc.KeyLoc)
129+
require.Equal(t,
130+
int32(tc.family),
131+
rpcKeyDesc.KeyLoc.KeyFamily,
132+
)
133+
require.Equal(t,
134+
int32(tc.index),
135+
rpcKeyDesc.KeyLoc.KeyIndex,
136+
)
137+
})
138+
}
139+
}
140+
141+
// TestMarshallSignDescriptorsPartialOnlyOneOf verifies that the partial
142+
// descriptor marshaller used by the legacy SignOutputRaw populates either
143+
// the public key or the locator, but never both. Applications like Loop
144+
// have adjusted themselves to this behavior, which is why the fix for the
145+
// locator-drop bug lives in the full descriptor path instead.
146+
func TestMarshallSignDescriptorsPartialOnlyOneOf(t *testing.T) {
147+
t.Parallel()
148+
149+
pubKey := newTestPubKey(t)
150+
output := &wire.TxOut{PkScript: []byte{0x51}, Value: 1000}
151+
152+
t.Run("pubkey present drops locator", func(t *testing.T) {
153+
t.Parallel()
154+
155+
descs := []*SignDescriptor{{
156+
KeyDesc: keychain.KeyDescriptor{
157+
PubKey: pubKey,
158+
KeyLocator: keychain.KeyLocator{
159+
Family: keychain.KeyFamilyNodeKey,
160+
Index: 5,
161+
},
162+
},
163+
Output: output,
164+
}}
165+
166+
rpcDescs := marshallSignDescriptors(descs, false)
167+
require.Len(t, rpcDescs, 1)
168+
169+
require.Equal(t,
170+
pubKey.SerializeCompressed(),
171+
rpcDescs[0].KeyDesc.RawKeyBytes,
172+
)
173+
require.Nil(t, rpcDescs[0].KeyDesc.KeyLoc)
174+
})
175+
176+
t.Run("pubkey absent uses locator", func(t *testing.T) {
177+
t.Parallel()
178+
179+
descs := []*SignDescriptor{{
180+
KeyDesc: keychain.KeyDescriptor{
181+
KeyLocator: keychain.KeyLocator{
182+
Family: keychain.KeyFamilyNodeKey,
183+
Index: 5,
184+
},
185+
},
186+
Output: output,
187+
}}
188+
189+
rpcDescs := marshallSignDescriptors(descs, false)
190+
require.Len(t, rpcDescs, 1)
191+
192+
require.Empty(t, rpcDescs[0].KeyDesc.RawKeyBytes)
193+
require.NotNil(t, rpcDescs[0].KeyDesc.KeyLoc)
194+
require.Equal(t,
195+
int32(keychain.KeyFamilyNodeKey),
196+
rpcDescs[0].KeyDesc.KeyLoc.KeyFamily,
197+
)
198+
require.Equal(t,
199+
int32(5),
200+
rpcDescs[0].KeyDesc.KeyLoc.KeyIndex,
201+
)
202+
})
203+
}

0 commit comments

Comments
 (0)