lndclient: attach key locator when any component is non-zero#269
Merged
lndclient: attach key locator when any component is non-zero#269
Conversation
SignOutputRawKeyLocator was introduced to propagate the key locator alongside the raw public key, but its marshaller required both the family and the index to be non-zero before attaching the KeyLoc. That silently dropped the locator for valid keys whose slot has a zero component — most visibly LND's node identity key at family 6 / index 0, but any first-index key in a non-zero family is affected. Attach the locator whenever either component is non-zero, and keep omitting it only for the all-zero KeyLocator, which is the struct's zero value and therefore indistinguishable from "locator not set" at call sites. This lets callers that sign against the node identity key path, or any first-index key in a restored-from-seed wallet, resolve through the fully populated descriptor instead of falling back to pubkey scanning.
starius
approved these changes
Apr 18, 2026
Contributor
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
LGTM! 💾
Great catch! I propose to add a couple of test cases and to remove the redundant tc := tc (lint finding).
| func TestMarshallSignDescriptorsFullLocator(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| pubKey := newTestPubKey(t) |
Contributor
There was a problem hiding this comment.
I propose to add two more tests where KeyDesc.PubKey is nil and either family!=0 or index!=0. I.e. replicate "family-only locator is attached" and "index-only locator is attached" for the KeyDesc.PubKey=nil case.
ziggie1984
approved these changes
Apr 20, 2026
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
LGTM, this should not effect any implementations using lndclient with LND.
Pin the contract that SignOutputRawKeyLocator's marshaller attaches the key locator whenever either the family or the index is non-zero and omits it only for the all-zero KeyLocator. The family-only case (KeyFamilyNodeKey / index 0) is the specific regression guard for the previously "both components non-zero" predicate that silently dropped the locator for LND's node identity key slot. Also cover the legacy partial descriptor path used by SignOutputRaw to pin that it still populates either the public key or the locator but never both, matching the behavior Loop and other downstream callers adjusted themselves to.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this PR, we fix a latent bug in
marshallSignDescriptors'sfullDescriptorinner closure (the one used bySignOutputRawKeyLocator). The helper currently attaches the RPCKeyLoconly when bothKeyLocator.Family != 0andKeyLocator.Index != 0. That&&is wrong: it silently drops the locator for any valid key whose slot has a zero component, and callers that need the locator to re-derive the key (i.e., they can't just scan for a matching pubkey) end up with a descriptor that no longer identifies the key.The most visible case is LND's node identity key at
keychain.KeyFamilyNodeKey(family 6) / index 0. Family 6 is meaningful, index 0 is meaningful (it's the first key), so a caller that populates both ends up with a descriptor that forwards neither. The same goes for any first-index key in a non-zero family, which is exactly what a restored-from-seed wallet has before anything else is derived. We hit this downstream indarepo-clientand had to carry a full local copy ofSignOutputRawjust to relax this one predicate. Once this lands the local copy can go away.The fix flips
&&to||: attach the locator whenever either the family or the index is non-zero, and only omit it for the all-zeroKeyLocator(which is the struct's zero value and therefore indistinguishable from "unset" at call sites). We also rewrite the surrounding comment block so the new rule and its rationale are clear at the call site.The legacy partial descriptor path used by
SignOutputRawis intentionally untouched. Loop and other downstream callers have adjusted themselves to its "populate either the pubkey or the locator, not both" shape, which is the whole reasonSignOutputRawKeyLocatorexists as a separate method in the first place.Test Plan
A new
signer_client_test.gopins the contract with table-driven tests:TestMarshallSignDescriptorsFullLocatorwalks the four (family, index) quadrants against thefullDescriptorpath. Thefamily-only_locator_is_attachedsubtest (family 6, index 0) is the specific regression guard and fails against the old&&predicate.TestMarshallSignDescriptorsPartialOnlyOneOfcovers the legacypartialDescriptorpath to pin that it still populates either the pubkey or the locator but never both, so we don't regress Loop or similar downstream callers.