[2/3] Graph RIP: multi: Graph Source Abstraction #9243

ellemouton · 2024-11-04T09:36:19Z

PR description

Depends on #9236

This PR adds a new GraphSource interface and passes it around to
various sub-systems of LND instead of passing around the raw
*graphdb.ChannelGraph pointer. This will allow us to plug in a different
graph source if we so please. The new ImplementationCfg.GraphProvider
can be be overridden to provide a different graph source.

This PR by itself is a pure refactor with no logic changes.

PR Flow:

The first few commits are just some clean-up/setup commits
Then, the new GraphSource interface is added and one by one we add to
interface and pass it around to more and more parts of the code base.
Remove all `context.TODOs added in the PR

Testing this PR:

See/use this demo multinode binary PR to test this PR.
It shows how one LND node can be used as the remote graph source of
another LND node. See that PR description for a more in depth description.

coderabbitai · 2024-11-04T09:36:29Z

Important

Review skipped

Auto reviews are limited to specific labels.

🏷️ Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

dstadulis · 2024-11-04T16:17:51Z

Exciting functionality!

ellemouton · 2024-11-11T06:33:34Z

quite a big change/update incoming here soon - so not ready to review yet! 🦖

ellemouton · 2024-11-13T09:31:53Z

Alrighty: updated. See the updated PR description for: 1) PR description 2) PR Flow and 3) how to test this PR.

The follow up PR will add the RPC implementation of the GraphSource

Simplify the ChannelGraphSource interface by removing this unused method.

For consistency in the graphsessoin.graph interface, we let the FetchNodeFeatures method take a read transaction just like the ForEachNodeDirectedChannel. This is nice because then all calls in the same pathfinding transaction use the same read transaction.

The package name is kept the same. So this is just for less stuttering at a path level.

This commit adds a new GraphSources interface that LND requires for graph related read-only queries. As of this commit, the interface is empty but it will be populated over the next couple of commits. We add an implementation of this interface backed by a pointer to a graphdb.ChannelGraph. The infrustructure is put into place so that the GraphSoure provided to LND can be overridden by a caller of the lnd.Main function. By default, LND will satisfy the interface itself via the new `graphsource.DBSource` struct.

In this commit, we take the existing graphsession.ReadyOnlyGraph interface and remove its usage of a kvdb.RTx and replace it with a more abstract `RTx` interface type. The new GraphSource interface is expanded to include the graphsession.ReadOnlyGraph interface and the implementation of it, DBSource, is expanded to include the new methods. It converts the given RTx to the underlying kvdb read transaction where needed.

Since the GraphSource interface may be satisfied by an RPC connection, it is best practice to pass a context through to any call in the interface. The ChanGraphSource implementation, which uses a kvdb backend, does not make use of the ctx. Any call-sites are for now given a `context.TODO()` which will all be addressed in follow up commits.

Define a new GraphSource interface required by the invoicerpc server and remove its access to the graphdb.ChannelGraph pointer. Add the new invoicesrpc.GraphSource interface to the GraphSource interface and let DBSource implement it.

And let DBSource implement it.

In this commit, we add a bunch of graph methods to the GraphSource, let DBSource implement it and then we use the graph source for these methods for the GetNodeInfo, VerifyMessage, DescribeGraph, GetNodeMetrics, GetChanInfo and GetNodeInfo RPC calls along with peer alias lookup.

We will later implement this interface with a backing RPC connection and so it makes sense to pass a context through for cancellation.

So that LND can use a different GraphSource for network bootstrapping and does not need to rely on its local graph db.

so that the external graph source can be used to query network information rather than depending on the local graph DB.

So that the calcuation is abstracted behind the interface and not necessarily dependent on LND's local channel graph.

Remove four context.TODO()s

bhandras

Pretty straightforward refactor! Great work! :) I guess my main comment is to rename graph.ChannelGraphSource so we know it is meant to push updates.

bhandras · 2024-12-10T09:40:12Z

graph/db/graph.go

@@ -503,9 +503,12 @@ func (c *ChannelGraph) ForEachChannel(cb func(*models.ChannelEdgeInfo,
 // ForEachNodeDirectedChannel iterates through all channels of a given node,
 // executing the passed callback on the directed edge representing the channel
 // and its incoming policy. If the callback returns an error, then the iteration
-// is halted with the error propagated back up to the caller.
+// is halted with the error propagated back up to the caller. An optional read


nit: commit message graphsessoin => graphsession 🤓

bhandras · 2024-12-10T09:41:21Z

graph/session/graph_session.go

@@ -1,4 +1,4 @@
-package graphsession
+package session


bhandras · 2024-12-10T09:42:54Z

config_builder.go

+type GraphProvider interface {
+	// Graph returns the GraphSource that LND will use for read-only graph
+	// related queries.
+	Graph(context.Context, *DatabaseInstances) (sources.GraphSource, error)


nit: wdyt of calling it GetGraphSource instead?

bhandras · 2024-12-10T09:46:42Z

graph/sources/chan_graph.go

+
+// NewDBGSource returns a new instance of the DBSource backed by a
+// graphdb.ChannelGraph instance.
+func NewDBGSource(db *graphdb.ChannelGraph) *DBSource {


nit: the constructor could also be called NewDBSource. Or if we want the "graph" term in the name the type could also be named DBGraphSource.

bhandras · 2024-12-10T09:58:17Z

discovery/bootstrapper.go


 	// Name returns a human readable string which names the concrete
 	// implementation of the NetworkPeerBootstrapper.
-	Name() string
+	Name(ctx context.Context) string


I think functions that can't return an error should not receive a context or unless we can't know if on context cancellation the returned value is valid. wdyt?

bhandras · 2024-12-10T10:00:25Z

graph/db/models/stats.go

+
+// NetworkStats represents various statistics about the state of the Lightning
+// network graph.
+type NetworkStats struct {


Alternatively stats could jus be implemented locally using a GraphSource.

bhandras · 2024-12-10T10:01:07Z

graph/db/models/stats.go

+
+// BetweennessCentrality represents the betweenness centrality of a node in the
+// graph.
+type BetweennessCentrality struct {


Same comment for centrality. Feels more like an application level feature and less of a source feature.

lightninglabs-deploy · 2025-01-07T11:37:11Z

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

ellemouton self-assigned this Nov 4, 2024

ellemouton added the graph label Nov 4, 2024

ellemouton force-pushed the graphSourceAbstraction branch from 298f526 to 3ce1526 Compare November 4, 2024 09:37

ellemouton added gossip interface abstraction labels Nov 4, 2024

guggero self-requested a review November 4, 2024 16:24

ellemouton marked this pull request as draft November 8, 2024 17:41

guggero removed their request for review November 11, 2024 09:00

ellemouton force-pushed the graphSourceAbstraction branch from 3ce1526 to 2a3db35 Compare November 13, 2024 09:04

ellemouton force-pushed the elle-moveGraphDBCode branch from 1504b31 to 53d1fab Compare November 13, 2024 09:05

ellemouton force-pushed the graphSourceAbstraction branch from 2a3db35 to d04f613 Compare November 13, 2024 09:06

ellemouton marked this pull request as ready for review November 13, 2024 09:06

ellemouton changed the title ~~multi: Graph Source Abstraction~~ [2/3] Graph RIP: multi: Graph Source Abstraction Nov 13, 2024

ellemouton mentioned this pull request Nov 13, 2024

[demo] in-process multinode demonstration ellemouton/lnd#158

Open

ellemouton force-pushed the graphSourceAbstraction branch from 971cca9 to 2dc2543 Compare November 13, 2024 09:30

ellemouton mentioned this pull request Nov 13, 2024

[3/3] Graph RIP: multi: RPC GraphSource #9265

Open

ellemouton force-pushed the elle-moveGraphDBCode branch from 53d1fab to d7d8c39 Compare November 14, 2024 07:45

ellemouton force-pushed the graphSourceAbstraction branch 3 times, most recently from 9335ed9 to eea9022 Compare November 14, 2024 08:36

bhandras self-requested a review November 21, 2024 08:05

ellemouton added 5 commits November 28, 2024 14:52

graph: remove unused ForEachNode method

488fa3e

Simplify the ChannelGraphSource interface by removing this unused method.

graph: rename directory from graphsession to session

755065b

The package name is kept the same. So this is just for less stuttering at a path level.

ellemouton added 16 commits November 28, 2024 14:52

netann+lnd: add netann.ChannelGraph to the GraphSource interface

237151d

And let DBSource implement it.

graph+channeldb: add AddrSource interface to GraphSource

bfe6262

discovery: pass contexts to NetworkPeerBootstrapper methods

0f33d41

We will later implement this interface with a backing RPC connection and so it makes sense to pass a context through for cancellation.

lnd+graph: add GraphBootstrapper to the GraphSource interface

372883a

So that LND can use a different GraphSource for network bootstrapping and does not need to rely on its local graph db.

graph+lnd: add NetworkStats to GraphSource interface

8007061

so that the external graph source can be used to query network information rather than depending on the local graph DB.

graph+lnd: add BetweennessCentrality to GraphSource interface

f36fbd0

So that the calcuation is abstracted behind the interface and not necessarily dependent on LND's local channel graph.

lnd+chanbackup: thread contexts through

2192bf4

Remove four context.TODO()s

invoicesrpc: remove a context.TODO

dcfffd6

blindedpath: remove a context.TODO

75b1069

netann: remove context.TODO

a28102e

routing: remove context.TODOs

c5cc6f1

remove context.TODOs from tests

791ac91

docs: update release notes

15c2161

ellemouton changed the base branch from elle-moveGraphDBCode to master November 28, 2024 13:01

ellemouton force-pushed the graphSourceAbstraction branch from 259b811 to 15c2161 Compare November 28, 2024 13:02

saubyk added this to the 0.20.0 milestone Dec 4, 2024

bhandras reviewed Dec 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2/3] Graph RIP: multi: Graph Source Abstraction #9243

[2/3] Graph RIP: multi: Graph Source Abstraction #9243

ellemouton commented Nov 4, 2024 •

edited

Loading

coderabbitai bot commented Nov 4, 2024 •

edited

Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

dstadulis commented Nov 4, 2024

ellemouton commented Nov 11, 2024

ellemouton commented Nov 13, 2024

bhandras left a comment

bhandras Dec 10, 2024

bhandras Dec 10, 2024

bhandras Dec 10, 2024

bhandras Dec 10, 2024

bhandras Dec 10, 2024

bhandras Dec 10, 2024

bhandras Dec 10, 2024

lightninglabs-deploy commented Jan 7, 2025

[2/3] Graph RIP: multi: Graph Source Abstraction #9243

Are you sure you want to change the base?

[2/3] Graph RIP: multi: Graph Source Abstraction #9243

Conversation

ellemouton commented Nov 4, 2024 • edited Loading

PR description

PR Flow:

Testing this PR:

coderabbitai bot commented Nov 4, 2024 • edited Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

dstadulis commented Nov 4, 2024

ellemouton commented Nov 11, 2024

ellemouton commented Nov 13, 2024

bhandras left a comment

Choose a reason for hiding this comment

bhandras Dec 10, 2024

Choose a reason for hiding this comment

bhandras Dec 10, 2024

Choose a reason for hiding this comment

bhandras Dec 10, 2024

Choose a reason for hiding this comment

bhandras Dec 10, 2024

Choose a reason for hiding this comment

bhandras Dec 10, 2024

Choose a reason for hiding this comment

bhandras Dec 10, 2024

Choose a reason for hiding this comment

bhandras Dec 10, 2024

Choose a reason for hiding this comment

lightninglabs-deploy commented Jan 7, 2025

ellemouton commented Nov 4, 2024 •

edited

Loading

coderabbitai bot commented Nov 4, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)