rpcserver: key DescribeGraph cache by request

[vbrekher]

Summary

• key cached DescribeGraph responses by include_unannounced and include_auth_proof
• keep the existing cache mutex and eviction behavior while preventing responses from one request shape from being reused for another
• add regression coverage for all four request-flag combinations

Testing

• go test . -run '^TestDescribeGraphCacheKey$' -count=1
• gofmt -w rpcserver.go rpcserver_graph_cache_test.go
• git diff --check

Fixes #11115

[github-actions]

🔴 PR Severity: CRITICAL

file classification | 2 files | 35 lines changed

🔴 Critical (1 file)

• rpcserver.go - core server coordination file; changes the DescribeGraph response caching/keying logic

🟢 Low (1 file)

• rpcserver_graph_cache_test.go - new regression test covering cache-key behavior

Analysis

This PR modifies rpcserver.go, which is explicitly listed as a CRITICAL-tier file (core server coordination). The change fixes a cache-keying bug in DescribeGraph so that responses aren't incorrectly reused across requests with different include_unannounced/include_auth_proof flags. Although the diff itself is small (28 additions / 7 deletions) and well-scoped, it touches shared RPC response-caching logic that affects correctness of data returned to all RPC callers, so it warrants expert review despite the limited size. No severity bump was needed (file count and line count are both well under the bump thresholds).

---

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

[Lrifton92]

Reviewed at 2775b10. The keying itself is right, and I checked that it is complete rather than assuming it: ChannelGraphRequest has exactly two fields, include_unannounced = 1 and include_auth_proof = 2 (lnrpc/lightning.proto:3555-3565), and both are in the key. So there is no third request shape still collapsing onto a shared entry. The map is bounded at four entries by construction, and the evictor nils the whole map, so eviction semantics are unchanged.

Three things before this is ready.

Two lines will fail lint, and CI has not told you so. This repo runs a custom lll with line-length: 80 and tab-width: 8 (.golangci.yml:218, :222). Measured at head:

• rpcserver.go:6241 — if cachedResp := r.describeGraphRespCache[cacheKey]; cachedResp != nil { is 2 tabs + 72 chars = 88 columns.
• rpcserver_graph_cache_test.go:30 — t.Fatalf("expected %d cache entries, got %d", ...) is also 88 columns.

gofmt -l and gofmt -d are both clean, which is why this is easy to miss — gofmt does not enforce line length. gh pr checks currently shows only the two severity jobs; the build and lint workflows have not been approved to run on this PR, so nothing has reported it yet. Splitting the lookup out of the if fixes the first at 63 columns:

cachedResp := r.describeGraphRespCache[cacheKey]
if cachedResp != nil {
        return cachedResp, nil
}

The test does not exercise the fix. TestDescribeGraphCacheKey builds a local map[describeGraphCacheKey]*lnrpc.ChannelGraph and checks that four requests produce four distinct keys. That tests newDescribeGraphCacheKey in isolation; DescribeGraph is never called. A regression that keys the real cache with a constant — which is precisely the shape of #11115 — would leave this test green. The assertion that would actually protect the fix is one that populates the cache through DescribeGraph with one flag combination and shows a request with a different combination does not receive it.

Manual field enumeration is the failure mode that produced #11115. newDescribeGraphCacheKey lists the fields by hand, so a third field added to ChannelGraphRequest later reintroduces exactly this bug, silently and with the same symptom. Since the key is a struct of the request's own fields, a cheap guard is a test that fails when the message grows — for example asserting reflect.TypeOf(lnrpc.ChannelGraphRequest{}).NumField() against the count the key accounts for, so the next person is forced to look here.

Two minor points. There is no docs/release-notes/release-notes-0.22.0.md entry; a cache returning another request's response is user-visible and the neighbouring PRs carry one. And the new test uses t.Fatalf where the rest of the package uses testify require, and neither the new type, the constructor, nor the test carries a doc comment.

[litbot-9000]

@vbrekher, remember to re-request review from reviewers when ready

[vbrekher]

Thanks, I reworked the regression around the actual DescribeGraph path. The test now populates the real cache and verifies that a different request shape cannot receive that cached response. I also added a guard for future ChannelGraphRequest fields, fixed the line-length issue, switched the assertions to require, added the missing doc comments and included the release note. Focused tests, compile, vet and make lint-source are clean.

[Lrifton92]

Reviewed at a2c227d.

All four points from my earlier review are addressed: the lookup is split out of the if (every added .go line is now within 80 columns at tab width 8), the regression test goes through DescribeGraph itself, the proto-descriptor field count guards against a third request field, and there is a release note plus doc comments.

I checked that the new test actually protects the fix rather than taking it on trust. With newDescribeGraphCacheKey mutated to return a constant key, TestDescribeGraphCacheByRequest fails on require.Empty(t, privateResp.Nodes) ("Should be empty, but was [alias:"cached-sentinel"]"). That is exactly the #11115 shape, and the sentinel written into the cached response is a nice way to make the cache hit observable on an empty graph.

One mutant survives, inline: dropping includeAuthProof from the key (includeAuthProof: false) keeps both tests green, because the behavioural test only varies IncludeUnannounced, and the field-count test counts proto fields, not key fields. Looping the same sentinel check over all four request shapes would close it cheaply. Non-blocking.

Minor: graphdb.MakeTestGraph already calls Start() and registers Stop() in t.Cleanup (graph/db/graph.go:1188-1194), so the explicit Start/Stop in the test are no-ops (both are guarded by CompareAndSwap). Harmless, but they can go.

LGTM.

[Lrifton92]

This only varies IncludeUnannounced, so the auth-proof half of the key is not pinned. I mutated the constructor to includeAuthProof: false and both tests still pass. Driving the same sentinel check over all four combinations ({}, unannounced, auth proof, both), populating one and asserting the other three miss, would make each key field load-bearing in this test.

[Lrifton92]

Nit: MakeTestGraph already starts the graph and registers Stop in its own cleanup (graph/db/graph.go:1188-1194), and both calls are idempotent, so this Start and the Cleanup below are redundant.