[g175] graph/db: add v2 gossip support for channel policies#10542
[g175] graph/db: add v2 gossip support for channel policies#10542ellemouton merged 13 commits intolightningnetwork:elle-g175Prep-basefrom
Conversation
Summary of ChangesHello @ellemouton, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the graph database layer to incorporate full support for Gossip V2 channel policies, building upon previous work to prepare for the 1.75 specification. The changes enable the storage and retrieval of Taproot channel announcements with Schnorr signature aggregation, laying crucial groundwork for future network-level activation of V2 gossip. The modifications ensure the database can correctly handle and differentiate between V1 and V2 policy updates, maintaining compatibility while introducing new capabilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🔴 PR Severity: CRITICAL
🔴 Critical (2 files)
🟠 High (13 files)
AnalysisThis PR adds v2 gossip support for channel policies, which is a significant enhancement to the Lightning Network graph management system. Severity rationale:
Key areas of concern:
Recommended review focus:
To override, add a |
🔴 PR Severity: CRITICAL
🔴 Critical (2 files)
🟠 High (12 files)
🟡 Medium (3 files)
🟢 Low (11 files)
AnalysisThis PR adds v2 gossip support for channel policies and is classified as CRITICAL due to:
Concerns:
This requires expert review from maintainers familiar with the Lightning gossip protocol, graph database layer, and SQL schema evolution. To override, add a |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR adds support for gossip v2 channel policies to the graph database layer. This is a significant change that touches many parts of the codebase, including the database schema, data models, and graph traversal logic. The changes are well-structured, with clear separation between v1 and v2 logic, especially in the sql_store and the new version-aware model constructors. The refactoring of CachedEdgePolicy to use boolean fields instead of bitmasks is a nice improvement for readability. My review focuses on ensuring documentation for new and modified exported functions is sufficiently detailed as per the project's style guide. Overall, this is a solid step towards full gossip v2 support.
| return c.db.DisabledChannelIDs() | ||
| } | ||
|
|
||
| // HasV1ChannelEdge returns true if the database knows of a channel edge. |
There was a problem hiding this comment.
The comment for HasV1ChannelEdge is a bit brief. To align with the style guide's emphasis on detailed comments for exported functions, consider expanding it to explain what the function does and what its return values represent, similar to the comment for this function in the Store interface.
| // HasV1ChannelEdge returns true if the database knows of a channel edge. | |
| // HasV1ChannelEdge returns true if the database knows of a v1 channel edge | |
| // with the passed channel ID, and false otherwise. If an edge with that | |
| // ID is found within the graph, then two time stamps representing the | |
| // last time the edge was updated for both directed edges are returned | |
| // along with the boolean. If it is not found, then the zombie index is | |
| // checked and its result is returned as the second boolean. |
References
- Exported functions require detailed comments for the caller. (link)
| return c.db.HasV1ChannelEdge(chanID) | ||
| } | ||
|
|
||
| // HasChannelEdge returns true if the database knows of a channel edge. |
There was a problem hiding this comment.
The comment for HasChannelEdge is a bit brief. To align with the style guide's emphasis on detailed comments for exported functions, consider expanding it to explain what the function does and what its return values represent, similar to the comment for this function in the Store interface.
// HasChannelEdge returns true if the database knows of a channel edge
// with the passed channel ID and gossip version, and false otherwise.
// If it is not found, then the zombie index is checked and its result
// is returned as the second boolean.References
- Exported functions require detailed comments for the caller. (link)
| return c.db.HasChannelEdge(c.v, chanID) | ||
| } | ||
|
|
||
| // ForEachNodeChannel iterates through all channels of the given node. |
There was a problem hiding this comment.
The comment for ForEachNodeChannel is a bit brief. To align with the style guide's emphasis on detailed comments for exported functions, consider expanding it to explain what the function does, its parameters, and what is passed to the callback, similar to the comment for this function in the Store interface.
// ForEachNodeChannel iterates through all channels of the given node for the
// graph's gossip version, executing the passed callback with an edge info
// structure and the policies of each end of the channel. The first edge
// policy is the outgoing edge *to* the connecting node, while the second is
// the incoming edge *from* the connecting node. If the callback returns an
// error, then the iteration is halted with the error propagated back up to
// the caller.
//
// NOTE: Unknown policies are passed into the callback as nil values.References
- Exported functions require detailed comments for the caller. (link)
| return c.db.ForEachNodeChannel(ctx, c.v, nodePub, cb, reset) | ||
| } | ||
|
|
||
| // ForEachChannel iterates through all channel edges stored within the graph. |
There was a problem hiding this comment.
The comment for ForEachChannel is a bit brief. To align with the style guide's emphasis on detailed comments for exported functions, consider expanding it to explain what the function does and what is passed to the callback, similar to the comment for this function in the Store interface.
// ForEachChannel iterates through all channel edges stored within the graph for
// the graph's gossip version.
//
// NOTE: If an edge can't be found, or wasn't advertised, then a nil
// pointer for that particular channel edge routing policy will be
// passed into the callback.References
- Exported functions require detailed comments for the caller. (link)
🔴 PR Severity: CRITICAL
🔴 Critical (2 files)
🟠 High (13 files)
🟢 Low (1 file)
📝 Test/Generated Files (14 files - excluded from severity calculation)Test files: Auto-generated: AnalysisThis PR adds v2 gossip support for channel policies, which is a critical change that touches:
Recommendation: This PR requires thorough review by engineers familiar with the gossip protocol, graph database architecture, and core server coordination logic. To override, add a |
|
@claude review this |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Extend channel policy queries and structs to support v2-specific fields: - Add BlockHeight field to track the block height for v2 policy updates. - Add DisableFlags field for v2 channel disable messages. Both fields are nullable (sql.NullInt64/Int16) to maintain backwards compatibility with v1 channels. The fields are initialized as null in updateChanEdgePolicy and threaded through all policy-related queries (GetChannelBySCIDWithPolicies, ListChannelsByNodeID, UpsertEdgePolicy, etc.) and the extractChannelPolicies helper. This commit includes both the hand-written SQL query updates and the corresponding sqlc-generated Go code.
🔴 PR Severity: CRITICAL
🔴 Critical (2 files)
🟠 High (11 files)
🟡 Medium (3 files)
🟢 Low/Test (14 files)Test files and auto-generated code (excluded from severity calculation) AnalysisThis PR adds v2 gossip support for channel policies, which is a substantial architectural change touching critical infrastructure: Critical Concerns:
Risk Factors:
Recommendation:
The changes appear well-structured based on the affected files, but the scope and depth warrant careful scrutiny given the critical nature of channel policy storage and retrieval. To override, add a |
🔴 PR Severity: CRITICAL
🟠 High (11 files)
🟡 Medium (2 files)
🟢 Low (17 files)Test files and documentation:
AnalysisThis PR adds v2 gossip support for channel policies, involving substantial changes to the graph database layer. While the individual files are classified as HIGH severity (graph/, discovery/, sqldb/*), the PR meets multiple criteria for bumping to CRITICAL:
Key Concerns:
Recommendation: This PR requires expert review from someone deeply familiar with lnd's graph database architecture, gossip protocol implementation, and the v2 gossip specification. To override, add a |
🔴 PR Severity: CRITICAL
🔴 Critical (2 files)
🟠 High (11 files)
🟡 Medium (2 files)
🟢 Low (1 file)
🧪 Test files (14 files, excluded from severity calculation)
AnalysisThis PR adds v2 gossip support for channel policies, which is a significant architectural change touching critical infrastructure: Why CRITICAL:
Concerns:
Recommendation: This PR requires expert review from engineers familiar with:
To override, add a |
🔴 PR Severity: CRITICAL
🔴 Critical (9 files)
🟠 High (3 files)
🟡 Medium (6 files)
🟢 Low (12 test files)Test files: AnalysisThis PR receives CRITICAL severity based on the following factors:
Key Review Areas:
This is part of the larger gossip 1.75 implementation (#10293) and builds on database foundation work from PRs #10339, #10379, and #10380. To override, add a |
ellemouton
force-pushed
the
g175-db-6
branch
from
d37a5b5 to
c02377c
Compare
🔴 PR Severity: CRITICAL
🔴 Critical (2 files)
🟠 High (22 files)Discovery (1 file)
Routing (6 files)
Graph (13 files)
SQL Database (2 files)
🟡 Medium (4 files)
🟢 Low (2 files)
AnalysisThis PR achieves CRITICAL severity due to modifications in core coordination files ( Additionally, the PR makes substantial changes to the graph database layer:
The scope includes:
Given the combination of critical server files and extensive graph database modifications that affect how channel policies are stored and retrieved (a foundational aspect of Lightning network operation), this PR requires careful expert review to ensure data integrity and backward compatibility. To override, add a |
| return c.ChannelFlags.IsDisabled() | ||
| } | ||
|
|
||
| return !c.DisableFlags.IsEnabled() |
There was a problem hiding this comment.
nit: perhaps worth updating the godoc a bit, to flag that this means that either incoming or outgoing is disabled. Maybe we could also add IsIncomingDisabled() and IsOutgoingDisabled() too (similarly for the cached policy).
There was a problem hiding this comment.
cool - updated the doc. for now, not including IsIncomingDisabled or IsOutgoingDisabled since 1) we dont use that anywhere yet and 2) we dnot have that data for v1 channels so would need to return errors at the moment. Which we can defs add later - but i think for now while we are just squeezing in the two, we want to keep the diff as minimal as possible
There was a problem hiding this comment.
fwiw, the ChanUpdateDisableFlags type does have IncomingDisabled() and OutgoingDisabled() already
| return &ChannelEdgePolicy{ | ||
| Version: lnwire.GossipVersion2, | ||
| SigBytes: upd.Signature.Val.ToSignatureBytes(), | ||
| ChannelID: upd.ShortChannelID.Val.ToUint64(), |
There was a problem hiding this comment.
nit: doesn't seem like a bug, but should we consistently fill this with scid like above?
There was a problem hiding this comment.
great catch! does actually seem like a bug given the comment at the call-site in the gossiper:
// We'll use chanInfo.ChannelID rather than the peer-supplied
// ShortChannelID in the ChannelUpdate to avoid the router having to
// lookup the stored SCID. If we're sending the update, we'll always
// use the SCID stored in the database rather than a potentially
// different alias. This might mean that SigBytes is incorrect as it
// signs a different SCID than the database SCID, but since there will
// only be a difference if AuthProof == nil, this is fine.
There was a problem hiding this comment.
that comment is really confusing
| c.MessageFlags, c.ChannelFlags, c.LastUpdate) | ||
| } | ||
|
|
||
| return fmt.Sprintf("ChannelID=%v, Node1=%v, DisableFlags=%v, "+ |
There was a problem hiding this comment.
nit: should we print the disable status directionally here too?
There was a problem hiding this comment.
good idea! what ive done is to instead update the String method on the lnwire.ChanUpdateDisabledFlags type instead to return this info
| // passed channel ID and this graph's gossip version, and false otherwise. If it | ||
| // is not found, then the zombie index is checked and its result is returned as | ||
| // the second boolean. | ||
| func (c *VersionedGraph) HasChannelEdge(chanID uint64) (bool, bool, error) { |
There was a problem hiding this comment.
nit: not sure where to put this comment, but do we want the same versioning for the channelCache as we've added for the rejectCache?
There was a problem hiding this comment.
yes absolutely - the cache is, however, only ever read/used in ChanUpdatesInHorizon - which we have not "versioned" yet in this PR. So leaving this for the coming follow up PRs :)
ellemouton
force-pushed
the
g175-db-6
branch
from
c02377c to
d317097
Compare
ellemouton
force-pushed
the
g175-db-6
branch
from
d317097 to
8d8a8c2
Compare
🔴 PR Severity: CRITICAL
🔴 Critical (3 files)
🟠 High (14 files)
🟡 Medium/Low (14 files)
AnalysisThis PR introduces v2 gossip support for channel policies, which is a protocol-level change requiring expert review. The severity is CRITICAL because:
Key Review Areas:
To override, add a |
ellemouton
force-pushed
the
g175-db-6
branch
from
8d8a8c2 to
db32651
Compare
|
@claude review this |
|
Claude finished @ellemouton's task in 2m 57s —— View job Code Review CompleteReviewing PR #10542: v2 gossip support for channel policies SummaryThis PR successfully extends the graph database layer to support gossip v2 (1.75) channel policies. The implementation is well-structured with clear version separation and follows good architectural patterns. I've identified several areas that merit attention, though no critical blockers were found. Overall Assessment: ✅ Solid implementation, ready for merge with minor considerations Detailed Review1. Wire Protocol Changes (
|
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
Really great work here, LGTM 🎉
| type ChannelEdgePolicy struct { | ||
| // Version is the gossip version of the channel update that produced | ||
| // this policy. | ||
| Version lnwire.GossipVersion |
There was a problem hiding this comment.
looks like this needs to be set as well in dev_server.go
There was a problem hiding this comment.
great catch! updated
| return &ChannelEdgePolicy{ | ||
| Version: lnwire.GossipVersion2, | ||
| SigBytes: upd.Signature.Val.ToSignatureBytes(), | ||
| ChannelID: upd.ShortChannelID.Val.ToUint64(), |
There was a problem hiding this comment.
that comment is really confusing
| edge2UpdateTime.Unix(), false, | ||
| ) | ||
| edge2.ChannelFlags = 1 |
There was a problem hiding this comment.
looks like ChannelFlags can be deleted as it's set in newEdgePolicy, same below?
There was a problem hiding this comment.
nice - good catch - removed!
| upd1BlockHeight int64 | ||
| upd2BlockHeight int64 |
There was a problem hiding this comment.
nit: maybe could use uint32 because of the block heights?
| name: "edge insertion deletion", | ||
| test: testEdgeInsertionDeletion, | ||
| }, | ||
| { |
There was a problem hiding this comment.
the commit message mentions equalExtraSignedFields, which is not included here (handled by the deep equal in the comparison helper I think)
There was a problem hiding this comment.
nice catch! thanks. due to refactor yesterday. will fix!
| edge1 := newEdgePolicy(v, chanID, nextUpdateTime().Unix(), true) | ||
| edge2 := newEdgePolicy(v, chanID, nextUpdateTime().Unix(), false) |
There was a problem hiding this comment.
nit: perhaps we can set the timestamps to zero as they are updated later?
There was a problem hiding this comment.
hmm i think they are used once before being updated though right?
gonna leave for now
Extend ChannelEdgePolicy to support v2 channel updates by adding: - Version field to track gossip protocol version (v1 or v2). - LastBlockHeight for v2's block-height-based timestamps. - SecondPeer flag to indicate which peer announced the policy in v2. - DisableFlags for v2-specific channel disable signaling. - ExtraSignedFields map for v2 extra signed TLV data. Add version-aware methods: - IsNode1() determines if the policy was announced by node_1, handling both v1 (via ChannelFlags direction bit) and v2 (via SecondPeer). - IsDisabled() checks disable status using ChannelFlags for v1 and DisableFlags for v2. - String() provides version-appropriate string representations. The new fields use zero values for v1 compatibility (Version defaults to GossipVersion1, LastBlockHeight to 0, SecondPeer to false). This lays the groundwork for v2 policy support; a subsequent commit will handle reading and writing these fields from/to the database.
Replace MessageFlags and ChannelFlags bitfields in CachedEdgePolicy with explicit boolean fields to improve clarity and support both v1 and v2 channel updates: - Replace MessageFlags with HasMaxHTLC boolean. - Replace ChannelFlags with IsNode1 and IsDisabled booleans. - Update NewCachedPolicy to extract these fields version-appropriately: - For v1: derive from MessageFlags and ChannelFlags bits. - For v2: derive from policy.SecondPeer and policy.DisableFlags. Update all call sites that used method calls IsNode1() and IsDisabled() to instead access the fields directly. This includes: - graph_cache.go: policy direction and disable checks - unified_edges.go: HasMaxHTLC and IsDisabled checks - Tests: policy construction and assertions This refactoring improves readability by making the cached policy's state explicit rather than encoded in bitfields, and enables seamless support for both gossip protocol versions.
Add ChanEdgePolicyFromWire to construct ChannelEdgePolicy from channel update messages, centralizing v1/v2 field mapping. Update call sites to use the helper: - discovery/gossiper: handleChanUpdate - graph/builder: ApplyChannelUpdate - routing/router_test: ApplyChannelUpdate test helper This consolidates update-to-policy conversion logic across versions.
Ensure policies loaded for graph cache reconstruction are tagged as v1.
Update buildChanPolicy and related functions in both KV and SQL stores to properly construct ChannelEdgePolicy with version-specific fields: KVStore changes: - Reject non-v1 policies in updateEdgePolicy and serializeChanEdgePolicy since KV store only supports v1 gossip protocol. - Set Version to GossipVersion1 when deserializing policies from KV. SQLStore changes: - Add isNode1 parameter to buildChanPolicy functions to properly set SecondPeer field (v2 uses SecondPeer instead of ChannelFlags direction). - Extract Version from database and populate version-specific fields: - For v1: MessageFlags, ChannelFlags, LastUpdate, ExtraOpaqueData - For v2: DisableFlags, LastBlockHeight, ExtraSignedFields - Thread isNode1 through buildChanPolicyWithBatchData and buildCachedChanPolicies call sites. This enables the SQL store to read and reconstruct both v1 and v2 channel policies from the database with proper field mapping.
Make the reject cache version-aware so v1 and v2 policy state can be cached independently per channel ID. Add helpers to store v1 timestamps or v2 block heights and thread the versioned cache key through KV/SQL store cache accesses.
Add a new HasChannelEdge method that takes a gossip version parameter and returns only existence and zombie status, without timestamp data. This supports both v1 and v2 gossip protocols. The original HasChannelEdge method is renamed to HasV1ChannelEdge to preserve v1-specific functionality for callers that need timestamp information. All call sites are updated accordingly. The SQL store implementation now handles both gossip versions, using timestamps for v1 and block heights for v2 policies, with proper reject cache support for both versions.
Update ForEachChannel to accept a gossip version parameter, allowing callers to specify which gossip version's channels should be iterated. This change prepares the graph database for supporting multiple gossip versions while maintaining backward compatibility. The KVStore implementation validates that only GossipVersion1 is requested, returning ErrVersionNotSupportedForKVDB for other versions. The SQLStore implementation validates known versions and passes the version through to the underlying paginated query. Update VersionedGraph to include a ForEachChannel method that automatically uses its configured gossip version, and update the DescribeGraph RPC handler to use the v1Graph instead of the global graphDB.
Update the UpsertChannelPolicy query to apply different staleness checks based on gossip version. For v1 policies, continue checking last_update timestamps. For v2 policies, check block_height instead, using >= comparison to handle policies from the same block. The version-specific WHERE clause ensures that policy updates are only applied when they contain newer information according to the versioning scheme appropriate for that gossip version.
Convert testEdgePolicyCRUD to a versioned test that runs against both v1 and v2 gossip versions. Update the test to use version-specific edge creation helpers and to test version-specific fields and flag behavior (ChannelFlags/MessageFlags for v1, DisableFlags/ ExtraSignedFields for v2).
Update ForEachNodeChannel to accept a gossip version parameter, allowing callers to specify which gossip version's channels should be iterated. This change mirrors the approach taken in ForEachChannel and prepares the graph database for supporting multiple gossip versions while maintaining backward compatibility. The Store interface is updated to include the version parameter, and both KVStore and SQLStore implementations are updated accordingly: - KVStore validates that only GossipVersion1 is requested, returning ErrVersionNotSupportedForKVDB for other versions. - SQLStore passes the version through to the underlying node query, enabling version-specific channel iteration. The ChannelGraph wrapper is updated to accept and pass through the version parameter. VersionedGraph gains a ForEachNodeChannel method that automatically uses its configured gossip version, providing a clean interface for version-specific operations. Update all call sites to explicitly pass lnwire.GossipVersion1, except for the local channel manager in server.go which now uses the v1Graph directly (matching the pattern used in other parts of the codebase).
ellemouton
force-pushed
the
g175-db-6
branch
from
db32651 to
6aebb31
Compare
lightninglabs-deploy
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (3 files)
🟠 High (16 files)
🟡 Medium (0 files)None 🟢 Low (13 files)
AnalysisThis PR adds v2 gossip support for channel policies, which is a CRITICAL change due to:
Recommendation: This PR requires expert review from maintainers familiar with the Lightning Network wire protocol, graph database architecture, and gossip protocol implementation. To override, add a |
ellemouton
merged commit 6c4617f
into
lightningnetwork:elle-g175Prep-base
5 participants
PR Description
This PR extends the graph database layer to support gossip v2 (1.75) channel policies, building on the foundation established in #10339, #10379, and #10380. This work is part of the larger gossip 1.75 implementation effort tracked in #10293.
Context
This PR completes the channel policy layer of the gossip v2 database preparation work:
modelsfor V2 data #10379 extended channel edge info and auth proof models for v2Together, these changes enable LND to store and retrieve Taproot channel announcements with Schnorr signature aggregation, though v2 gossip remains disabled at the network level pending completion of validation and gossiper subsystem work.
Key Changes
Model Extensions
ChannelEdgePolicy:Version,LastBlockHeight,SecondPeer,DisableFlags,ExtraSignedFieldsIsNode1(),IsDisabled(),String()ChanEdgePolicyFromWireconstructor for converting wire messages to policiesDatabase Operations
ForEachChannel,ForEachNodeChannel, andHasChannelEdgeto accept gossip version parameterUpsertChannelPolicy:>(strict ordering)>=(allows same-block updates)Store Implementations
ErrVersionNotSupportedForKVDBAbstractions
VersionedGraphwith policy-specific methods that use configured gossip versionCachedEdgePolicyto use explicit boolean fields (HasMaxHTLC,IsNode1,IsDisabled) instead of bitfields for clarity and version-agnostic cachingGossipVersion1