Skip to content

keychain+walletrpc: add DeriveAndStoreKey - #11095

Open
GeorgeTsagk wants to merge 3 commits into
lightningnetwork:masterfrom
GeorgeTsagk:derive-and-store-key
Open

keychain+walletrpc: add DeriveAndStoreKey#11095
GeorgeTsagk wants to merge 3 commits into
lightningnetwork:masterfrom
GeorgeTsagk:derive-and-store-key

Conversation

@GeorgeTsagk

@GeorgeTsagk GeorgeTsagk commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Description

DeriveNextKey records the key it derives in the wallet, DeriveKey does not. So a key derived at an arbitrary index is invisible to ListAccounts and cannot be signed with, and an external consumer of a key family has no way to tell the wallet how far that family has been used, which matters after a seed-only recovery.

This adds DeriveAndStoreKey, the variant of DeriveKey that records the key, on keychain.KeyRing and as a walletrpc RPC. Recording a key implies recording every key before it, so it also advances the family's index in one call. Monotonic, idempotent, never rewinds, and bounded by MaxKeyIndexExtension. DeriveKey is left as is on purpose, since its callers pass indexes from channel backups and the remote signer signing path.

Related PRs / issues

Consumer: lightninglabs/taproot-assets#2225. Verified end to end against that branch with a matching lndclient wrapper, tapd's seed restore itest passing on sqlite and postgres.

Addresses #8698.

@GeorgeTsagk
keychain: add DeriveAndStoreKey to the KeyRing interface
2de19d9 
DeriveNextKey records the key it derives in the wallet's address
manager, while DeriveKey does not. That makes the two methods behave
differently in a way that matters: since the wallet has no record of a
key derived through DeriveKey, it cannot map that key's public key back
to its key locator, and that mapping is what the wallet needs in order
to sign with the key. Keys derived at an arbitrary index are therefore
not usable for signing, and they show up in neither ListAccounts nor
ListAddresses.

This adds DeriveAndStoreKey, the variant of DeriveKey that records the
key it derives. Recording a key implies recording every key in the
family that precedes it, so the call doubles as a way to advance a key
family's derivation index: an external consumer of a key family that
tracks its own indexes can restore the wallet's index after the wallet
was recovered from seed, instead of calling DeriveNextKey in a loop
until it reaches the index it needs.

The call is monotonic and idempotent, a family is never rewound, and the
number of keys a single call derives is bounded by MaxKeyIndexExtension
so that a request for a far away index is refused instead of filling the
address manager.
@GeorgeTsagk GeorgeTsagk self-assigned this Aug 18, 2026
@GeorgeTsagk
lnrpc+walletrpc: add DeriveAndStoreKey RPC
5a9002e 
Exposes keychain's DeriveAndStoreKey over the WalletKit service, so RPC
consumers of a key family can derive a key that the wallet is able to
sign with later on, and can restore the family's derivation index after
the wallet was recovered from seed.

The macaroon permission matches the one DeriveKey and DeriveNextKey use.
@GeorgeTsagk
itest: cover the DeriveAndStoreKey RPC
5b9f177 
Asserts that a key derived without storing it leaves the key family's
derivation index alone and stays unknown to the wallet, that storing one
advances the index past it and makes it known, that DeriveNextKey does not
hand out a consumed index again, that storing an index the family already
passed does not rewind it, that an index too far ahead is refused instead
of deriving an unbounded number of keys, and that negative locators are
rejected.

The same assertions run against a watch-only node driving a remote signer,
which additionally covers the case the new method exists for: signing with
a key that is referenced by its public key alone requires the wallet to
resolve the key's locator through its address, so it fails for a key that
was only derived and succeeds once the key is stored. That resolution is
skipped on a node with a local wallet, which falls back to scanning a
range of key indexes, so only the remote signer exercises it.
@GeorgeTsagk
GeorgeTsagk force-pushed the derive-and-store-key branch from 7ea238b to 5b9f177 Compare August 18, 2026 10:03
GeorgeTsagk added a commit to lightninglabs/lndclient that referenced this pull request Aug 18, 2026
@GeorgeTsagk
mod: TEMPORARY point lnd at the DeriveAndStoreKey branch
2c8ee79 
The wrapper added in the previous commit calls an RPC that does not exist
in any released LND yet, so this points the LND dependency at the branch of
lightningnetwork/lnd#11095 to make the build work in the meantime.

This commit must be dropped in favor of a normal LND version bump before
this branch can be merged.
@github-actions github-actions Bot added the severity-critical Requires expert review - security/consensus critical label Aug 18, 2026
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown

🔴 PR Severity: CRITICAL

gh pr view | 20 files | 1005 lines changed

🔴 Critical (4 files)
  • keychain/btcwallet.go - private key derivation/management (keychain/*)
  • keychain/derivation.go - private key derivation/management (keychain/*)
  • lnwallet/rpcwallet/rpcwallet.go - remote signer wallet operations (lnwallet/*)
🟠 High (7 files)
  • lnrpc/walletrpc/walletkit_server.go - RPC server implementation (lnrpc/*)
  • lnrpc/walletrpc/walletkit.pb.go - generated RPC types (lnrpc/*)
  • lnrpc/walletrpc/walletkit.pb.gw.go - generated gateway (lnrpc/*)
  • lnrpc/walletrpc/walletkit.pb.json.go - generated JSON marshaling (lnrpc/*)
  • lnrpc/walletrpc/walletkit_grpc.pb.go - generated gRPC service (lnrpc/*)
  • lnrpc/walletrpc/walletkit.swagger.json - API swagger spec (lnrpc/*)
  • lnrpc/walletrpc/walletkit.yaml - API config (lnrpc/*)
  • watchtower/wtmock/keyring.go - watchtower keyring mock (watchtower/*)
🟡 Medium (2 files)
  • lnrpc/walletrpc/walletkit.proto - proto API definition change
  • lnencrypt/test_utils.go - test helper, uncategorized Go file
🟢 Low (7 files)
  • docs/release-notes/release-notes-0.21.0.md
  • itest/list_on_test.go
  • itest/lnd_remote_signer_test.go
  • itest/lnd_signer_test.go
  • keychain/interface_test.go
  • lntest/mock/secretkeyring.go
  • lntest/rpc/wallet_kit.go

Analysis

This PR modifies keychain/btcwallet.go and keychain/derivation.go (private key derivation/management) alongside lnwallet/rpcwallet/rpcwallet.go (remote signer wallet operations), both CRITICAL packages per policy. It also touches multiple distinct critical packages (keychain/* and lnwallet/*), which would otherwise trigger a one-level severity bump — already at the ceiling, so it stays CRITICAL. The change adds new wallet/keychain functionality exposed through the walletrpc API (new proto field, generated RPC code, server handler) with accompanying itest coverage. Given the sensitivity of key derivation and remote signing code, this warrants expert review of the cryptographic/derivation logic in addition to the RPC surface changes.

To override, add a severity-override-{critical,high,medium,low} label.

GeorgeTsagk added a commit to GeorgeTsagk/taproot-assets that referenced this pull request Aug 18, 2026
@GeorgeTsagk
backup: advance the key family index in a single call
4b7047b 
Recording a key in LND implies recording every key that precedes it, so
restoring a key family's index is one DeriveAndStoreKey call per family
instead of one DeriveNextKey per index. LND keeps that operation monotonic
and bounds the number of keys it derives, so the local advance loop, the
rewind guard and the maxKeyIndexAdvance limit are no longer needed here.

Requires an LND with the DeriveAndStoreKey RPC
(lightningnetwork/lnd#11095) and the lndclient wrapper for it
(lightninglabs/lndclient#288).
GeorgeTsagk added a commit to GeorgeTsagk/taproot-assets that referenced this pull request Aug 18, 2026
@GeorgeTsagk
backup: advance the key family index in a single call
b4c6080 
Recording a key in LND implies recording every key that precedes it, so
restoring a key family's index is one DeriveAndStoreKey call per family
instead of one DeriveNextKey per index. LND keeps that operation monotonic
and bounds the number of keys it derives, so the local advance loop, the
rewind guard and the maxKeyIndexAdvance limit are no longer needed here.

Requires an LND with the DeriveAndStoreKey RPC
(lightningnetwork/lnd#11095) and the lndclient wrapper for it
(lightninglabs/lndclient#288).
@GeorgeTsagk GeorgeTsagk mentioned this pull request Aug 18, 2026
Open
GeorgeTsagk added a commit to GeorgeTsagk/taproot-assets that referenced this pull request Aug 18, 2026
@GeorgeTsagk
backup: advance the key family index in a single call
d1d4ccf 
Recording a key in LND implies recording every key that precedes it, so
restoring a key family's index is one DeriveAndStoreKey call per family
instead of one DeriveNextKey per index. LND keeps that operation monotonic
and bounds the number of keys it derives, so the local advance loop, the
rewind guard and the maxKeyIndexAdvance limit are no longer needed here.

Requires an LND with the DeriveAndStoreKey RPC
(lightningnetwork/lnd#11095) and the lndclient wrapper for it
(lightninglabs/lndclient#288).
GeorgeTsagk added a commit to GeorgeTsagk/taproot-assets that referenced this pull request Aug 18, 2026
@GeorgeTsagk
backup: advance the key family index in a single call
c3e936a 
Recording a key in LND implies recording every key that precedes it, so
restoring a key family's index is one DeriveAndStoreKey call per family
instead of one DeriveNextKey per index. LND keeps that operation monotonic
and bounds the number of keys it derives, so the local advance loop, the
rewind guard and the maxKeyIndexAdvance limit are no longer needed here.

Requires an LND with the DeriveAndStoreKey RPC
(lightningnetwork/lnd#11095) and the lndclient wrapper for it
(lightninglabs/lndclient#288).
@jtobin jtobin mentioned this pull request Aug 19, 2026
Open
GeorgeTsagk added a commit to lightninglabs/lndclient that referenced this pull request Aug 19, 2026
@GeorgeTsagk
mod: TEMPORARY point lnd at the DeriveAndStoreKey branch
7d3fdf0 
The wrapper added in the previous commit calls an RPC that does not exist
in any released LND yet, so this points the LND dependency at the branch of
lightningnetwork/lnd#11095 to make the build work in the meantime.

This commit must be dropped in favor of a normal LND version bump before
this branch can be merged.
@jtobin jtobin linked an issue Aug 24, 2026 that may be closed by this pull request
[bug]: full recovery (seed + backup import) resets address key derivation — new addresses reuse historical script keys, breaking receives lightninglabs/taproot-assets#2221
Open
@litbot-9000

litbot-9000 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

@hieblmi: review reminder
@ziggie1984: review reminder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hieblmi hieblmi Awaiting requested review from hieblmi
@ziggie1984 ziggie1984 Awaiting requested review from ziggie1984

At least 2 approving reviews are required to merge this pull request.

Assignees

@GeorgeTsagk GeorgeTsagk

Labels

severity-critical Requires expert review - security/consensus critical

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[bug]: full recovery (seed + backup import) resets address key derivation — new addresses reuse historical script keys, breaking receives

2 participants

@GeorgeTsagk @litbot-9000