Skip to content

waved: reuse cached identity in GetInfo - #1133

Merged
bhandras merged 1 commit into
mainfrom
codex/cache-getinfo-identity
Aug 12, 2026
Merged

waved: reuse cached identity in GetInfo#1133
bhandras merged 1 commit into
mainfrom
codex/cache-getinfo-identity

Conversation

@bhandras

@bhandras bhandras commented Aug 11, 2026

Copy link
Copy Markdown
Member

What changed

GetInfo now returns the daemon identity that startup already derived and
cached. It only derives the key itself when startup has not published the
cached value yet.

The cached descriptor is protected by a read/write lock because GetInfo can
run while wallet startup is publishing it. All production readers use the same
synchronized snapshot.

Why

For btcwallet-backed wallets, deriving this key opens a wallet database write
transaction. Doing that for every status request could make a read-only
GetInfo call wait behind unrelated wallet work.

This matters to mobile clients because the receive readiness check calls
GetInfo. On a physical iPhone, the readiness stage dropped from a 20-second
timeout to 47.583 microseconds with this change.

The later timeout found in the receive authentication stage has a different
cause and is tracked in #1134. This PR remains a focused fix for status calls;
it does not claim to fix that separate wallet recovery stall.

Safety

  • The cached and fallback paths use the same stable key locator: identity key
    family, index 0.
  • Requests made before publication keep the existing fallback behavior.
  • The change does not alter mailbox identities, signatures, or wallet state.
  • A race test covers identity publication while GetInfo is running.

Fixes #1132.

Validation

  • make fmt-changed-check base=origin/main
  • make tidy-module-check
  • make commitmsg-lint range='origin/main..HEAD'
  • make lint-changed-local base=origin/main
  • go test ./waved
  • go test -race ./waved
  • go test -tags='wavewalletrpc swapruntime' ./swapwallet
  • Physical-device validation of the GetInfo readiness stage

@litbot-9000

Copy link
Copy Markdown
Collaborator

📝 Doc drift advisory — waved

This PR's change to waved/rpc_server.go (GetInfo now serves
IdentityPubkey from the cached Server.clientKeyDesc instead of
re-deriving it) introduces a new invariant that waved/CLAUDE.md does not
record; the proposed patch documents the cached identity descriptor and the
read-only-status-poll reason for preferring it.

diff --git a/waved/AGENTS.md b/waved/AGENTS.md
index b778f21c..da2da84d 100644
--- a/waved/AGENTS.md
+++ b/waved/AGENTS.md
@@ -12,8 +12,11 @@ For field-level detail, use `go doc github.com/lightninglabs/wavelength/waved.<S
 
 - `Server` — main daemon. Owns the wallet, DB, chainsource actor, gRPC
   server, and `ActorSystem`. Caches `localMailboxID` (pubkey-derived),
-  `authSigHex` (Schnorr auth), and a single `clk` (`clock.Clock`) shared by
-  all sub-stores for deterministic time injection.
+  `authSigHex` (Schnorr auth), `clientKeyDesc` (the stable daemon identity
+  descriptor, `identityKeyFamily`/index 0, populated by
+  `deriveIdentityKeyEarly` before the wallet is marked ready), and a single
+  `clk` (`clock.Clock`) shared by all sub-stores for deterministic time
+  injection.
 - `RPCServer` — implements the gRPC `DaemonService`. Most write RPCs
   (`Board`, `SendVTXO`, `SendOOR`, `SweepBoardingUTXOs`, `SendOnChain`)
   validate input locally then `Ask` the relevant actor; `GetRound` and
@@ -138,6 +141,15 @@ For field-level detail, use `go doc github.com/lightninglabs/wavelength/waved.<S
   so the bound survives restarts.
 - `operatorTermsFromResponse` and daemon `GetInfo` must preserve
   `FreeRefreshWindowBlocks` end to end.
+- `GetInfo` reports `IdentityPubkey` from the cached
+  `Server.clientKeyDesc.PubKey` whenever it is set, and only falls back to
+  `deriveIdentityPubkey` when it is nil (a status probe issued before the
+  wallet was unlocked). Re-deriving is not free: the btcwallet/lwwallet
+  keyring `DeriveKey` opens a walletdb **write** transaction, so an
+  otherwise read-only status poll would stall behind an unrelated wallet
+  writer. Both paths derive `identityKeyFamily`/index 0, so they must keep
+  yielding the same key; `deriveIdentityPubkey` uses `DeriveKey` (never
+  `DeriveNextKey`) to guarantee that.
 - The VTXO manager reads `FreeRefreshWindowBlocks` from the latest cached
   operator terms on each expiry check. It delays automatic refresh to the
   window boundary only when the local dynamic critical threshold plus retry
diff --git a/waved/CLAUDE.md b/waved/CLAUDE.md
index b778f21c..da2da84d 100644
--- a/waved/CLAUDE.md
+++ b/waved/CLAUDE.md
@@ -12,8 +12,11 @@ For field-level detail, use `go doc github.com/lightninglabs/wavelength/waved.<S
 
 - `Server` — main daemon. Owns the wallet, DB, chainsource actor, gRPC
   server, and `ActorSystem`. Caches `localMailboxID` (pubkey-derived),
-  `authSigHex` (Schnorr auth), and a single `clk` (`clock.Clock`) shared by
-  all sub-stores for deterministic time injection.
+  `authSigHex` (Schnorr auth), `clientKeyDesc` (the stable daemon identity
+  descriptor, `identityKeyFamily`/index 0, populated by
+  `deriveIdentityKeyEarly` before the wallet is marked ready), and a single
+  `clk` (`clock.Clock`) shared by all sub-stores for deterministic time
+  injection.
 - `RPCServer` — implements the gRPC `DaemonService`. Most write RPCs
   (`Board`, `SendVTXO`, `SendOOR`, `SweepBoardingUTXOs`, `SendOnChain`)
   validate input locally then `Ask` the relevant actor; `GetRound` and
@@ -138,6 +141,15 @@ For field-level detail, use `go doc github.com/lightninglabs/wavelength/waved.<S
   so the bound survives restarts.
 - `operatorTermsFromResponse` and daemon `GetInfo` must preserve
   `FreeRefreshWindowBlocks` end to end.
+- `GetInfo` reports `IdentityPubkey` from the cached
+  `Server.clientKeyDesc.PubKey` whenever it is set, and only falls back to
+  `deriveIdentityPubkey` when it is nil (a status probe issued before the
+  wallet was unlocked). Re-deriving is not free: the btcwallet/lwwallet
+  keyring `DeriveKey` opens a walletdb **write** transaction, so an
+  otherwise read-only status poll would stall behind an unrelated wallet
+  writer. Both paths derive `identityKeyFamily`/index 0, so they must keep
+  yielding the same key; `deriveIdentityPubkey` uses `DeriveKey` (never
+  `DeriveNextKey`) to guarantee that.
 - The VTXO manager reads `FreeRefreshWindowBlocks` from the latest cached
   operator terms on each expiry check. It delays automatic refresh to the
   window boundary only when the local dynamic critical threshold plus retry

How to apply: save the diff above and git apply it, or run the
nightly gardener locally with /doc-gardening waved and then
make doc-check. The CLAUDE.md/AGENTS.md pair must stay
byte-identical.

Note: make doc-check on this runner reports one pre-existing,
unrelated error — ./.claude-pr/CLAUDE.md exists but ./.claude-pr/AGENTS.md is missing. .claude-pr/ is untracked CI
scaffolding that scripts/doc-check.sh's find . walks; it is not part of
this diff and no doc was created there. The waved pair check passes.

doc-gardening PR advisory run

@bhandras

Copy link
Copy Markdown
Member Author

Physical-device validation confirms this issue and the proposed change, but
also exposed a separate underlying writer-stall tracked in #1134.

With #1133 included in the iOS framework, the receive path's wallet_ready
stage (which calls GetInfo) completed in 47.583 µs instead of blocking for
the 20-second mobile deadline. This demonstrates that reusing the cached
identity removes GetInfo from bbolt writer contention as intended.

The request subsequently blocked at receive_auth_key. An untruncated
goroutine dump showed that key derivation waiting for bbolt's writer while
btcwallet startup recovery owned it across an Esplora HTTP/2 request. That is a
different defect and does not invalidate this focused fix; it explains why
invoice creation can still time out after GetInfo becomes responsive.

I have left the PR draft because the end-to-end device scenario still depends
on #1134, while the focused behavior covered here is now device-confirmed.

GetInfo derived the stable daemon identity on every request. For
btcwallet-backed wallets, that derivation opens a wallet database write
transaction. A read-only status request can therefore wait behind other
wallet work.

Use the cached identity after mailbox startup publishes it.
Keep the existing derivation fallback for earlier requests.

Synchronize descriptor publication and reads because GetInfo is callable
during startup. Cover both the no-wallet cached path and concurrent
publication with regression tests.
@bhandras
bhandras force-pushed the codex/cache-getinfo-identity branch from 20ef2e3 to 0bf5ddc Compare August 12, 2026 16:51
@bhandras

Copy link
Copy Markdown
Member Author

Applied the documentation advisory in commit 0bf5ddc. Both waved/AGENTS.md and waved/CLAUDE.md now describe the cached identity path, the pre-publication fallback, and the synchronization required because GetInfo can run during startup. The two files remain byte-identical.

@bhandras
bhandras marked this pull request as ready for review August 12, 2026 16:52
@bhandras

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @bhandras's task in 1m 36s —— View job


Code Review — waved: reuse cached identity in GetInfo

  • Read the changed files
  • Analyze concurrency/locking of cached identity
  • Check GetInfo fallback correctness
  • Post review feedback

Reviewed against origin/main. This is a clean, well-scoped fix. Summary and notes below.

Verdict

The change is correct and I found no blocking issues. Caching a stable, immutable identity key (family identityKeyFamily, index 0) and serving GetInfo from it — while keeping the pre-ready fallback — is the right shape, and the locking is sound.

What's done well

  • Complete accessor migration. Every production read/write of clientKeyDesc now goes through loadClientKeyDesc/storeClientKeyDesc. I grepped \.clientKeyDesc\b across non-test waved/*.go and the only remaining direct hits are inside the two accessors themselves (server.go:494, server.go:503). No unsynchronized production reader was left behind, including the mailbox-signing paths (signMailboxAuth, signMailboxTLSBind, signTaggedSchnorr*) and initRPCClients/connectAndBootstrapMailbox.
  • Snapshot consistency. signTaggedSchnorr takes a single loadClientKeyDesc() snapshot and reuses it across the lnd WhenSome branch and the keyring branch, so both paths see one consistent descriptor rather than re-reading the field twice.
  • Fallback parity. The cached fast path (rpc_server.go:645) and deriveIdentityPubkey (rpc_wallet.go:632) both derive locator {identityKeyFamily, 0} via DeriveKey (never DeriveNextKey) and both format via fmt.Sprintf("%x", …SerializeCompressed()), so a request served before publication returns a byte-identical key to one served after. Since the identity locator is stable, the cached value can never go stale.
  • Meaningful race test. TestGetInfoSynchronizesCachedIdentityPublication runs 1,000 concurrent store/GetInfo overlaps, which genuinely exercises the RWMutex under -race. TestGetInfoUsesCachedIdentityKey correctly proves the cached path is taken by omitting the wallet backend, so a fallback would surface as an empty key.
  • Docs kept in sync. CLAUDE.md/AGENTS.md record the new invariant and remain byte-identical.

Minor / non-blocking observations

  1. Test-only direct field writes. A few tests still assign server.clientKeyDesc directly (e.g. rpc_vtxo_forfeit_test.go:344, and struct-literal init in rpc_wallet_test.go, rpc_oor_custom_input_test.go, server_outbound_clients_test.go). These are single-threaded setups so there's no real race today, but they bypass the new accessor and would trip -race if a future test ever calls GetInfo concurrently with such a mutation. Not worth changing now — just a consistency note for future test authors.
  2. storeClientKeyDesc value-copy comment. The doc comment states the returned descriptor's "public-key pointer is immutable." That's the load-bearing assumption for lock-free downstream use, and it holds as long as nobody mutates the pointed-to *btcec.PublicKey. Worth keeping in mind if any code ever mutated a KeyDescriptor.PubKey in place — currently nothing does.

Nothing here requires action; the PR is ready on the merits of the focused fix. The remaining end-to-end device timeout is correctly split out to #1134.
· codex/cache-getinfo-identity

@bhandras
bhandras merged commit e8848d4 into main Aug 12, 2026
36 of 37 checks passed
@bhandras
bhandras deleted the codex/cache-getinfo-identity branch August 12, 2026 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GetInfo can block behind wallet database writes

2 participants