graph/db: cross-version graph Store queries - #11175
ViktorT-11 wants to merge 11 commits into
Conversation
When FetchChannelEdgesByID hits a zombie edge, it constructs the partial ChannelEdgeInfo it returns alongside ErrZombieEdge with a hard-coded ChannelID of zero instead of the actual channel ID that was looked up. Callers such as the gossiper's processZombieUpdate receive this struct and may use the ChannelID field; returning zero is incorrect and could mask bugs in downstream code. Pass the looked-up chanID through to NewV1Channel / NewV2Channel so the zombie ChannelEdgeInfo carries the correct ChannelID.
Add two precomputed mapping tables that track the "best" gossip version for each unique node (pub_key) and channel (SCID): - graph_preferred_nodes: pub_key -> node_id - graph_preferred_channels: scid -> channel_id Priority for nodes: v2 announced > v1 announced > v2 shell > v1 shell. Priority for channels: v2 with policies > v1 with policies > v2 > v1. These tables enable simple indexed-join queries for cross-version traversal (ForEachNode, ForEachChannel, ForEachNodeDirectedChannel) without expensive per-row COALESCE subqueries. The tables are populated from existing data during the migration and maintained by upsert/delete queries on every write path (added in the next commit).
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (8 files)
🟠 High (4 files)
🟢 Low (3 files)
AnalysisThis PR adds a new SQL database migration ( To override, add a |
|
|
||
| // TestPreferredChannelFetch tests the two new Store methods: | ||
| // FetchChannelEdgesByIDPreferred and FetchChannelEdgesByOutpointPreferred. | ||
| func TestPreferredChannelFetch(t *testing.T) { |
There was a problem hiding this comment.
nit: the linter fails with this commit
| // additionally skip empty v2 entries so they don't shadow a | ||
| // non-empty v1 feature set; this matches the no-cache | ||
| // FetchNodeFeatures fallback rule that a non-empty | ||
| // lower-version vector wins over an empty higher-version one. |
There was a problem hiding this comment.
populateCache lets the later gossip version win for a SCID, but the preferred tables rank a version carrying policies above a bare one. A channel announced as policed v1 plus bare v2 then resolves to v1 in ForEachChannel and DescribeGraph while the cache holds the bare v2 entry, so it reads as routable over RPC and is unroutable for pathfinding. Could you apply the same ranking inside GraphCache, which also covers the live write path?
| func(node route.Vertex, | ||
| features *lnwire.FeatureVector) error { | ||
|
|
||
| if v == gossipV2 && features.IsEmpty() { |
There was a problem hiding this comment.
The empty v2 skip here stops a bare v2 announcement from shadowing a non-empty v1 feature set, but the live AddNode path has no equivalent, so an empty v2 announcement still wipes the v1 features until a restart repopulates the cache. Could you move the rule into one helper that both paths call, and cover it with a test?
| reset func()) error { | ||
|
|
||
| return c.db.ForEachChannel(ctx, v, cb, reset) | ||
| return c.db.ForEachChannel(ctx, cb, reset) |
There was a problem hiding this comment.
nit: Promoting these off VersionedGraph means the embedded *ChannelGraph methods now serve callers holding a version-pinned graph, so the pin is ignored with no signal at the call site. autopilot and the discovery bootstrapper both receive v1Graph and will iterate every gossip version once v2 data exists. From the docstrings I see it's intentional, but maybe we can find a better solution?
| } | ||
|
|
||
| // FetchChannelEdgesByID attempts to lookup directed edges by channel ID. | ||
| // FetchChannelEdgesByID attempts to lookup directed edges by channel ID, |
There was a problem hiding this comment.
This fetch now spans gossip versions, but deleteAliasEdge in server.go still calls a v1-scoped delete with the SCID it returns. A v2 alias edge is therefore reported to the funding manager as deleted while its row stays in the graph. server.go sits outside this diff, which is what makes it easy to miss. Could you delete the version the fetch resolved to, using the Version on the returned ChannelEdgeInfo?
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: the cross-version assertions check that a policy is present but not what it holds, so a swapped direction or a corrupted fee base would still pass. Could you assert the fee fields and that each policy is attached to the node it belongs to?
| } | ||
|
|
||
| return c.db.FetchNodeFeatures(ctx, lnwire.GossipVersion1, node) | ||
| // The no-cache path only runs against the KV backend, which is |
There was a problem hiding this comment.
nit: graph/db: simplify no-cache fallback paths to v1-only removes the v2-then-v1 fallback loop that graph/db: implement cross-version node traversal adds, so the loop only ever exists between those two commits. Could you squash them so the dead loop never enters history?
|
|
||
| return forEachNodeDirectedChannel( | ||
| ctx, s.db, lnwire.GossipVersion1, nodePub, cb, | ||
| ctx, s.db, gossipV1, nodePub, cb, |
There was a problem hiding this comment.
nit: graph/db: make ForEachNode and ForEachChannel cross-version is not gofmt clean here, missing the blank line before the FetchNodeFeatures doc comment, and the following commit adds it back.
| numNodes := 0 | ||
| v1Graph := NewVersionedGraph(graph, lnwire.GossipVersion1) | ||
| err := v1Graph.ForEachNode( | ||
| err := graph.ForEachNode( |
There was a problem hiding this comment.
Whole-graph iteration moves from a single-table cursor scan to a join through the new mapping tables, and every node, shell node, channel and policy write now issues an extra upsert that multiplies during initial gossip sync. Could you post before and after BenchmarkGraphReadMethods numbers for ForEachNode and ForEachChannel?
| SELECT sub.pub_key, sub.node_id | ||
| FROM ( | ||
| SELECT | ||
| n.pub_key, |
There was a problem hiding this comment.
The preferred tables are a derived index maintained by hand on every write path, and a lost mapping row makes an entity vanish from cross-version reads with no error and no log line. Would a detection-only startup check be worth adding, counting and logging divergence without repairing?
Add version-agnostic channel fetch helpers that choose the preferred gossip-version row for a logical channel: the highest version with policy data, falling back to the highest bare version. Implement the SQL path inside a single read transaction instead of calling SQLStore methods recursively, preserve zombie edge info for SCID lookups, and keep KV behavior as v1-only delegation. Callers that need to know whether any version of a channel exists can rely on the ErrEdgeNotFound returned by the Preferred fetch path. Add coverage for preferred selection, missing channels, missing outpoints, and zombie edge info.
Add UpsertPreferredNode and UpsertPreferredChannel calls to every Store write path so the preferred mapping tables stay consistent: - upsertSourceNode, upsertNode, maybeCreateShellNode, DeleteNode - insertChannel, updateChanEdgePolicy, DeleteChannelEdges - pruneGraphNodes CASCADE deletes on the underlying graph_nodes and graph_channels tables automatically clean up preferred entries when a version is removed, so PruneGraph and DisconnectBlockAtHeight (which delete every version of a SCID) need no explicit upsert call.
Drop the GossipVersion parameter from ForEachNode and ForEachChannel. Both methods now iterate across all gossip versions, yielding one result per unique pub_key or SCID using the preferred mapping tables for pagination. Wire ChannelGraph.FetchChannelEdgesByID and FetchChannelEdgesByOutpoint through the Preferred variants so the public ChannelGraph API loses its GossipVersion parameter and becomes version-agnostic on read.
Make FetchNodeFeatures version-agnostic by trying v2 first and falling back to v1 when v2 features are empty. The no-cache fallback for ForEachNodeDirectedChannel stays version-scoped (v1 only) because the SQL backend always runs with the in-memory cache enabled, and the cache already merges v1+v2 with v2 precedence. populateCache now skips empty v2 feature entries so they don't shadow a non-empty v1 feature set when the cache is rebuilt, matching the FetchNodeFeatures precedence rule. The cache iterates v1 then v2 so v2 data overwrites v1 on key collision.
VersionedGraph wraps ChannelGraph for callers that want to operate against a specific gossip version. After the cross-version refactor, the ForEachNode and ForEachChannel methods on VersionedGraph silently ignored c.v and iterated across all versions — a surprising behaviour for a wrapper whose whole purpose is version-scoping. Move the cross-version iteration to ChannelGraph (where it belongs) and drop the foot-gun overrides from VersionedGraph. *VersionedGraph continues to expose these via the embedded *ChannelGraph, but now they are explicitly methods of the version-agnostic type. Add ChannelGraph.ForEachNode mirroring ChannelGraph.ForEachChannel, and update the only non-embedded callsite (rpcserver describeGraph) to take ChannelGraph directly. The two remaining test helpers also switch to *ChannelGraph since they only ever wanted a node count.
SQLStore.ForEachNodeCached takes a gossip version and uses it correctly when paginating through nodes, but the inner ListChannelsForNodeIDs call hardcoded GossipVersion1 instead of forwarding the requested version. Calling ForEachNodeCached(ctx, GossipVersion2, ...) therefore returned v2 nodes paired with v1 channels — silently inconsistent data. The bug was latent because every existing caller happens to request v1, but it would surface as soon as any v2-scoped caller appears. Add a regression test that creates a v2-only channel between two nodes that exist under both versions and asserts that ForEachNodeCached(ctx, GossipVersion2, ...) reports the channel for each endpoint.
VersionedGraph.GraphSession duplicated ChannelGraph.GraphSession with no observable difference: in the cache-loaded branch, both pass themselves to the callback, and the cache's NodeTraverser surface (GetFeatures, ForEachChannel) has no version concept — so receiving a *VersionedGraph vs a *ChannelGraph routes to the same cache lookups. In the no-cache branch, both delegate to c.db.GraphSession identically. Remove the override and fold its v1-only note into ChannelGraph's docstring so the production invariant (no-cache path is KV-only, which is v1-only) is preserved at the surviving callsite. This mirrors the hygiene from the earlier ForEachNode/ForEachChannel cleanup, where VersionedGraph overrides that ignored c.v were moved off the wrapper.
sqlNodeTraverser.ForEachNodeDirectedChannel and FetchNodeFeatures both pass lnwire.GossipVersion1 to their helpers without explanation. The reason this is correct is non-obvious: sqlNodeTraverser is only ever constructed by SQLStore.GraphSession, which is only reached when the in-memory graph cache is unavailable. Since the SQL backend always runs with the cache enabled in production, this fallback never executes at runtime — the cache-backed NodeTraverser (which already merges v1+v2) is what real callers see. Only tests exercise this code, and they operate on v1 data. Capture that rationale on the type doc so a future reader doesn't mistake the hardcoding for a bug. Also switch the call sites from lnwire.GossipVersion1 to the file-local gossipV1 alias to match the convention used elsewhere in this file.
401a8f5 to
c936793
Compare
|
@ViktorT-11, remember to re-request review from reviewers when ready |
5 participants
Replaces #10714, and rebases the PR on
master.Summary
This PR makes the graph
Storeinterface cross-version so thatForEachNode,ForEachChannel, andForEachNodeDirectedChannelwork across gossip v1 and v2, returning one preferred entry per pub_key/SCID. It also addsPreferHighestfetch helpers andGetVersionsqueries so callers can retrieve channels without knowing which gossip version announced them.Part of the gossip v2 epic: #10293
Spun off from: #10656
Design
Two new materialised mapping tables are introduced:
graph_preferred_nodes— one row per uniquepub_key, pointing at the bestgraph_nodesrow. Priority: v2 announced > v1 announced > v2 shell > v1 shell.graph_preferred_channels— one row per unique SCID, pointing at the bestgraph_channelsrow. Priority: v2 with policies > v1 with policies > v2 bare > v1 bare.These tables are maintained on every write path (
AddNode,AddChannelEdge,UpdateEdgePolicy,DeleteNode,DeleteChannelEdges,PruneGraphNodes) viaUpsertPreferredNode/UpsertPreferredChannelSQL queries, and seeded by a migration for existing data.Changes by commit
sqldb: add preferred-node and preferred-channel mapping tables— migration 000014 creating the tables and populating them from existing data.graph/db: add PreferHighest fetch methods and GetVersions queries— newStoreinterface methods:FetchChannelEdgesByIDPreferHighest,FetchChannelEdgesByOutpointPreferHighest,GetVersionsBySCID,GetVersionsByOutpoint. KV implementations delegate to v1.graph/db: wire preferred-table maintenance into write paths— callsUpsertPreferredNode/UpsertPreferredChannelon all SQL write paths.graph/db: make ForEachNode and ForEachChannel cross-version— removes theGossipVersionparameter fromForEachNodeandForEachChannelon theStoreinterface, replacing the underlying queries with preferred-table-backed paginated queries.graph/db: implement cross-version node traversal— addsForEachNodeDirectedChannelPreferHighestfor cache-disabled SQL path, updatesFetchNodeFeaturesto prefer v2 features with v1 fallback.docs: add release notePerformance
Benchmarked on Apple M1 Pro against a mainnet v1-only graph (16,216 nodes, 51,239 channels). No regressions observed — the preferred-table JOIN adds negligible overhead:
Test plan
TestPreferHighestAndGetVersions,TestPreferHighestForEachNode,TestPreferHighestForEachChannel,TestPreferHighestNodeTraversal,TestPreferHighestNodeDirectedChannelTraversal,TestDeleteNodePreferredRecomputationisSQLDBguard