[g175] refactor: prep for multiple Node versions #10279

ellemouton · 2025-10-08T15:50:34Z

A simple refactor & clean-up PR.

The main thing here is an added Version field to the models.Node struct & using a constructor to create the Node instance so that all the V1 fields are set for a V1 node. This is in preparation for dealing with V2 Nodes.

Part of #10293

gemini-code-assist · 2025-10-08T15:51:14Z

Summary of Changes

Hello @ellemouton, I'm Gemini Code Assist¹! 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 is a refactor and cleanup aimed at preparing the codebase for supporting multiple versions of Lightning Network node announcements. It introduces a Version field to the models.Node struct, replaces direct struct instantiation with dedicated constructors for V1 nodes, and converts optional fields like Color and Alias to use fn.Option types. Additionally, it simplifies several related data structures by removing redundant cached fields and their accessor methods. The changes are applied consistently across the codebase, updating all relevant call sites and tests to align with the new design. This lays the groundwork for future extensions, such as V2 node announcements, while improving code clarity and maintainability.

Highlights

Node Versioning Refactor: The core change introduces a Version field to the models.Node struct and refactors node creation to use constructors (NewV1Node, NewV1ShellNode, NewShellNode). This prepares the codebase for handling multiple node announcement versions (e.g., V1, V2).
Optional Fields with fn.Option: The Color and Alias fields in models.Node are now wrapped in fn.Option types, making their optionality explicit. All usage sites have been updated to correctly handle these optional values using UnwrapOr or WhenSome.
Simplified Data Structures: The HaveNodeAnnouncement boolean field in models.Node has been replaced by a HaveAnnouncement() method. Additionally, cached ecdsa.Signature fields in ChannelAuthProof and btcec.PublicKey fields in ChannelEdgeInfo have been removed, simplifying these structs and their access patterns.
Codebase-Wide Adaption: Numerous files across the project, including tests, database implementations (kv_store, sql_store), RPC server logic, and routing components, have been updated to use the new node constructors and fn.Option access patterns, ensuring consistency with the refactored models.Node.
Graph Interface Consolidation: The graph.DB interface has been removed, and direct usage of *graphdb.ChannelGraph is now prevalent, suggesting a consolidation of graph database access mechanisms.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces a refactor to prepare the codebase for handling multiple Node versions, specifically by adding a Version field to the models.Node struct and using a constructor to create Node instances. The changes primarily involve modifying the autopilot/prefattach_test.go, channeldb/db_test.go, discovery/chan_series.go, graph/builder.go, graph/builder_test.go, graph/db/graph.go, graph/db/graph_test.go, graph/db/interfaces.go, graph/db/kv_store.go, graph/db/models/channel_auth_proof.go, graph/db/models/channel_edge_info.go, graph/db/models/node.go, graph/db/notifications.go, graph/db/sql_migration.go, graph/db/sql_migration_test.go, lnrpc/devrpc/dev_server.go, routing/pathfind_test.go, routing/payment_session_source.go, routing/payment_session_test.go, routing/router_test.go, and server.go files. The review focuses on correctness, maintainability, and adherence to the repository's style guide, particularly concerning code documentation, commenting, spacing, and formatting.

bitromortac

Looking great, a lot of nice clean-ups 🧹

routing/payment_session_source.go

graph/db/models/node.go

lnrpc/devrpc/dev_server.go

graph/db/models/node.go

bitromortac · 2025-10-15T17:02:11Z

graph/db/sql_store.go

+	if dbNode.LastUpdate.Valid {
+		node.LastUpdate = time.Unix(dbNode.LastUpdate.Int64, 0)
+	}


is there a reason why node.LastUpdate is non-optional when the db field is also nullable, whereas alias is optional for example?

we can/should defs change it later on. I think this was mainly due to Alias/Color not being used often so the diff was small. LastUpdate is used more often - so I'll do that change in a commit by itself a bit later on 👍

oh yes, it was also cause things like alias & color do become actual optional fields but update time must always be set & isnt really optional in that sense.

but yeah, we can make this more explicit later with helpers like "ExtractV1Node" or something

bitromortac · 2025-10-15T17:06:14Z

graph/db/models/node.go

+}
+
+// NewV1Node creates a new version 1 node from the passed fields.
+func NewV1Node(pub route.Vertex, n *NodeV1Fields) *Node {


Using NodeV1Fields as a combined parameter somewhat defeats the purpose of making sure that a creator sets all field values, not?

what do you mean by "combined parameter"?
I think having the struct shows clearly what values need to be set? ie, with this constructor we dont expose V2-only-fields that would just be lost on persistence

Imo having a struct argument makes the code slightly more readable. A long list of arguments can end up being very messy.

I think some editors/IDEs don't immediately fill struct fields (e.g. in vim it's doable but not a default) so perhaps @bitromortac was referring to that?

I agree that it's cleaner to have it the way it is, just wasn't sure because of the PR description so that all the V1 fields are set for a V1 node, because you can leave out setting some fields of the struct in principle (in contrast to having all fields as parameters to NewV1Node like @bhandras mentioned). Just wanted to generate some awareness

autopilot/prefattach_test.go

graph/interfaces.go

lightninglabs-deploy · 2025-10-22T18:17:57Z

@bhandras: review reminder
@ellemouton, remember to re-request review from reviewers when ready

Simplify the struct by removing un-used methods and outdated comments.

Remove the 2 sources of truth here. If we have a signature for the node, then we have the announcement.

Add a version field to models.Node and a V1 constructor for it.

Remove various unused fields and methods.

bhandras

LGTM, very nice! 🎉

bhandras · 2025-10-26T17:47:18Z

lnrpc/devrpc/dev_server.go

 			Alias: rpcNode.Alias,
+			// NOTE: this is a workaround to ensure that
+			// HaveAnnouncement() returns true so that the other
+			// fields are properly persisted. However,


nit: comment ending seems missing.

bhandras · 2025-10-26T17:58:34Z

graph/db/models/node.go


 	// Color is the selected color for the node.
-	Color color.RGBA
+	Color fn.Option[color.RGBA]


q: Should NodeV1Fields.Color/Alias be optional too?

bhandras · 2025-10-26T18:02:44Z

graph/db/models/node.go

+}
+
+// NewV1Node creates a new version 1 node from the passed fields.
+func NewV1Node(pub route.Vertex, n *NodeV1Fields) *Node {


Imo having a struct argument makes the code slightly more readable. A long list of arguments can end up being very messy.

I think some editors/IDEs don't immediately fill struct fields (e.g. in vim it's doable but not a default) so perhaps @bitromortac was referring to that?

bitromortac

LGTM ⚡

graph/db/sql_store.go

bitromortac · 2025-10-27T12:47:03Z

graph/db/models/node.go

+}
+
+// NewV1Node creates a new version 1 node from the passed fields.
+func NewV1Node(pub route.Vertex, n *NodeV1Fields) *Node {


I agree that it's cleaner to have it the way it is, just wasn't sure because of the PR description so that all the V1 fields are set for a V1 node, because you can leave out setting some fields of the struct in principle (in contrast to having all fields as parameters to NewV1Node like @bhandras mentioned). Just wanted to generate some awareness

autopilot/prefattach_test.go

ellemouton self-assigned this Oct 8, 2025

gemini-code-assist bot reviewed Oct 8, 2025

View reviewed changes

ellemouton force-pushed the g175Prep2 branch from 2b4aff3 to 5722f62 Compare October 9, 2025 10:22

ellemouton added the no-changelog label Oct 9, 2025

yyforyongyu force-pushed the 0-21-0-staging branch from 496c5e3 to 8eb29f1 Compare October 13, 2025 16:39

ellemouton mentioned this pull request Oct 14, 2025

[epic]: Gossip 1.75 Implementation #10293

Open

10 tasks

ellemouton force-pushed the g175Prep2 branch 2 times, most recently from 6b66a3b to 258ca41 Compare October 14, 2025 12:35

ellemouton requested review from bhandras and bitromortac October 14, 2025 12:36

saubyk added this to v0.21 Oct 14, 2025

saubyk moved this to In progress in v0.21 Oct 14, 2025

saubyk moved this from In progress to In review in v0.21 Oct 14, 2025

bitromortac reviewed Oct 15, 2025

View reviewed changes

ellemouton added 2 commits October 26, 2025 08:40

models: simplify models.Node

10ef680

Simplify the struct by removing un-used methods and outdated comments.

multi: remove HaveNodeAnnouncement field from Node

574b3cc

Remove the 2 sources of truth here. If we have a signature for the node, then we have the announcement.

ellemouton force-pushed the g175Prep2 branch from 258ca41 to d22b1c4 Compare October 26, 2025 07:23

ellemouton added 4 commits October 26, 2025 09:50

multi: add models.Node V1 constructor

6ef8040

Add a version field to models.Node and a V1 constructor for it.

graph/db: simplify auth proof and edge info

c590675

Remove various unused fields and methods.

graph: remove DB interface

7d02ccb

graph/db: remove outdated comment

b2a2a93

ellemouton force-pushed the g175Prep2 branch from d22b1c4 to b2a2a93 Compare October 26, 2025 07:51

ellemouton requested review from bitromortac October 26, 2025 09:16

bhandras approved these changes Oct 26, 2025

View reviewed changes

bitromortac approved these changes Oct 27, 2025

View reviewed changes

Uh oh!

[g175] refactor: prep for multiple Node versions #10279

Are you sure you want to change the base?

[g175] refactor: prep for multiple Node versions #10279

Uh oh!

Conversation

ellemouton commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot commented Oct 8, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lightninglabs-deploy commented Oct 22, 2025

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ellemouton commented Oct 8, 2025 •

edited

Loading