refactor: move graph responsibilities from routing.ChannelRouter to new graph.Builder #8848

ellemouton · 2024-06-18T19:56:42Z

This PR achieves 2 goals:

Moves graph building and maintaining responsibilities from the ChannelRouter to the new graph.Builder. These responsibilities include:
- graph pruning
- validation of new network updates & persisting new updates (the validation should actually be completely within the gossiper but let's do that in another PR)
- notifying topology update clients of any changes.
Removes the ChannelRouter's direct access to the channeldb.ChannelGraph pointer and replace it with an interface instead. This will open up the possibility of providing various other types of graph backends.

This chips away at the first few steps noted in issue #8833 and also takes a step towards #8835 since with this PR, the ChannelRouter now only does path finding and payments.

This change is

coderabbitai · 2024-06-18T19:56:47Z

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>.
- 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 generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @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 as 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.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration 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.

Roasbeef · 2024-06-18T20:37:33Z

cc @calvinrzachman

Roasbeef

Great PR! Particularly like the attention to detail re the commit structure to make some of the larger mostly-move commits easier to follow.

Will do another pass, but at initial glance, the main thing that jumped out at me is if we're able to eliminate awareness of the underlying graph db transaction RwTx from the interface all together. We may be able to wrap things in a sort of graphSession to abstract away those details, and give the future SQL schema for the graph a foil.

routing/graph.go

routing/payment_session_source.go

routing/graph.go

graph/log.go

graph/builder.go

graph/setup_test.go

routing/router.go

server.go

ellemouton

Thanks @Roasbeef 🙏

Updated 👍 for the extra indirection, it was a bit of a mind bend so added a temporary (rough) commit at the end to see if this is what you had in mind.

rpcserver.go

ellemouton · 2024-06-19T23:01:48Z

graph/interfaces.go

+// topology is synchronized.
+//
+//nolint:interfacebloat
+type ChannelGraphSource interface {


@Roasbeef - if we go with the "put interface where it is used" pattern, shall I go ahead an move this to the gossiper instead?

From what I've experienced I think we usually go with the "put interface where it is implemented" tho.

channeldb/interfaces.go

graph/log.go

routing/router.go

server.go

routing/graph.go

routing/payment_session_source.go

routing/graph.go

ellemouton

Thanks again @Roasbeef - I think this new iteration is looking a bit better (again see last temp commit) 🙏

routing/graph.go

Roasbeef

Dig the latest approach. This way the interface the router declares has no direct references to the kvdb paradigm.

routing/graph.go

ellemouton · 2024-06-26T16:25:31Z

Thanks for the review @Roasbeef 🚀 Squashed in the latest approach - this is ready for another look 👍

Roasbeef

Reviewed 12 of 24 files at r1, 29 of 29 files at r2, 32 of 38 files at r4, 8 of 8 files at r5, 17 of 17 files at r6, all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @djkazic, @ellemouton, and @yyforyongyu)

channeldb/graphsession/graph_session.go line 51 at r6 (raw file):

type Session struct {
	graph graph
	tx    kvdb.RTx

Is the intent that a new DB backend implement an entirely new Session struct? As is, the references to kvdb.RTx still exist, forcing awareness of the old paradigm.

One potentially slim way to add this extra lay of direction would be to create a new interface in place of kvdb.RTx. This would represent an abstract notion of a transaction within the context of this graph, not necessarily tied to the underlying database.

The end-goal here as I perceive it is that this package makes no direct reference to the current DB abstractions. Instead way might have a struct that lives on the top level lnd package, which serves as a sort of glue/adapter between the old worlds and new.

channeldb/graphsession/graph_session.go line 131 at r6 (raw file):

	//
	// Unknown policies are passed into the callback as nil values.
	ForEachNodeDirectedChannel(tx kvdb.RTx, node route.Vertex,

Re the comment above, rather than calling NewPathFindTx() to generate a tx, to then pass into this method, this would be hidden in a private attribute by a concrete implementation of the interface.

Or we add a layer of indirection here with a custom db tx interface.

In this commit, we further reduce the routingGraph interface and this time we make it more node-agnostic so that it can be backed by any graph and not one with a concept of "sourceNode".

In preparation for structs outside of the `routing` package implementing this interface, export `routingGraph` and rename it to `Graph` so as to avoid stuttering.

In preparation for the next commit.

ellemouton

Thanks all ! updated :)

ellemouton · 2024-07-15T11:39:45Z

channeldb/graphsession/graph_session.go

+	tx    kvdb.RTx
+}
+
+// NewRoutingGraph constructs a Session that which does not first start a


e we should make that assumption more clear on the

cool - updated the comment 👍

channeldb/graphsession/graph_session.go

ellemouton · 2024-07-15T11:40:58Z

channeldb/graphsession/graph_session.go

+	// up to the caller.
+	//
+	// Unknown policies are passed into the callback as nil values.
+	ForEachNodeDirectedChannel(tx kvdb.RTx, node route.Vertex,


cool - thanks for the suggestion - added!

routing/graph.go

graph/interfaces.go

ellemouton · 2024-07-15T12:38:32Z

server.go

+		NextPaymentID:      sequencer.NextID,
+		PathFindingConfig:  pathFindingConfig,
+		Clock:              clock.NewDefaultClock(),
+		ApplyChannelUpdate: s.graphBuilder.ApplyChannelUpdate,


ah ok I see now that this is more inline with the first diagram you posted (ie the ideal vision)

so gonna leave this as is for now if that is ok and then we do that change later on?

ellemouton · 2024-07-15T13:02:26Z

routing/payment_session.go

-		// Get a routing graph.
-		routingGraph, cleanup, err := p.getRoutingGraph()
+		// Get a routing graph session.
+		routingGraph, err := p.graphSessFactory.NewSession()


added a commit!

ellemouton · 2024-07-15T13:03:06Z

graph/builder.go

+
+// Builder builds and maintains a view of the Lightning Network graph.
+type Builder struct {
+	started uint32 // To be used atomically.


also added a commit at the end to convert the other Builder members to use atomic.Unit*

ellemouton · 2024-07-15T13:03:41Z

graph/log.go

+
+// logClosure is used to provide a closure over expensive logging operations so
+// don't have to be performed when the logging level doesn't warrant it.
+type logClosure func() string


for this line:

log.Tracef("New channel update applied: %v", newLogClosure(func() string { return spew.Sdump(msg) }))

graph/builder.go

In this commit, we completely remove the Router's dependence on a Graph source that requires a `kvdb.RTx`. In so doing, we are more prepared for a future where the Graph source is backed by different DB structure such as pure SQL. The two areas affected here are: the ChannelRouter's graph access that it uses for pathfinding. And the SessionSource's graph access that it uses for payments. The ChannelRouter gets given a Graph and the SessionSource is given a GraphSessionFactory which it can use to create a new session. Behind the scenes, this will acquire a kvdb.RTx that will be used for calls to the Graph's `ForEachNodeChannel` method.

Instead of querying it from the graph since this will be removed in a future commit.

In preparation for having a clean graph DB interface, refactor FetchChanInfos so that no transaction can be provided.

In preparation for adding a clean Graph DB interface, we create a version of FetchLightningNode that doesnt allow a caller to provide in a transaction.

In prep for a clean Graph DB interface, we add a version of ForEachNodeChannel that does not take in an existing db transaction.

... to the new `graph` package in preparation for the implementation of the interface being moved to this new package.

..which describes the database methods that are required for graph maintaining and building.

yyforyongyu

LGTM🌊⛵️ (looks like linter failed tho)

yyforyongyu · 2024-07-15T12:54:46Z

channeldb/graphsession/graph_session.go

+	// up to the caller.
+	//
+	// Unknown policies are passed into the callback as nil values.
+	ForEachNodeDirectedChannel(tx kvdb.RTx, node route.Vertex,


ok I see what's going on here now - so graph is like a placeholder to access the methods defined in channebldb, aliasing the two methods. Feels a bit weird but guess it's ok.

yyforyongyu · 2024-07-15T12:55:10Z

channeldb/graphsession/graph_session.go

+	tx    kvdb.RTx
+}
+
+// NewRoutingGraph constructs a Session that which does not first start a


Q: why do we want to maintain a db tx ourselves in the first place?

yyforyongyu · 2024-07-15T12:56:28Z

routing/graph.go

+// can then be used for a path-finding session. Depending on the implementation,
+// the GraphSession will represent a DB connection where a read-lock is being
+// held across calls to the backing Graph.
+type GraphSessionFactory interface {


Cool let's keep the PR a pure refactor.

yyforyongyu · 2024-07-15T13:00:38Z

graph/interfaces.go

+	"github.com/lightningnetwork/lnd/routing/route"
+)
+
+// ChannelGraphSource represents the source of information about the topology


yeah it's just a nit so feel free to ignore - and yes agree the approach if you plan to as it makes reviewing way easier.

yyforyongyu · 2024-07-15T13:01:47Z

graph/builder.go

+
+// Builder builds and maintains a view of the Lightning Network graph.
+type Builder struct {
+	started uint32 // To be used atomically.


Or atomic.Bool?

yyforyongyu · 2024-07-15T13:06:12Z

graph/builder.go

+// Start launches all the goroutines the Builder requires to carry out its
+// duties. If the builder has already been started, then this method is a noop.
+func (b *Builder) Start() error {
+	if !b.started.CompareAndSwap(0, 1) {


I'm proposing a new pattern to start/stop systems here, so the idea is,

always log starting... in Start.

return an error when it's called twice.

This is to help us debug issues in the future, same for Stop. No need to change here tho, just wanna bring awareness that this discussion is going on.

yyforyongyu · 2024-07-15T13:06:44Z

graph/log.go

+
+// logClosure is used to provide a closure over expensive logging operations so
+// don't have to be performed when the logging level doesn't warrant it.
+type logClosure func() string


think the linter will throw an error if it isn't used?

yyforyongyu · 2024-07-15T13:10:05Z

graph/builder.go

+
+// ApplyChannelUpdate validates a channel update and if valid, applies it to the
+// database. It returns a bool indicating whether the updates were successful.
+func (b *Builder) ApplyChannelUpdate(msg *lnwire.ChannelUpdate) bool {


I see the other place uses s.graphBuilder.ApplyChannelUpdate,, and wonder if we should also replace s.applyChannelUpdate with this newer method.

yyforyongyu · 2024-07-15T13:12:24Z

routing/payment_session.go

@@ -294,6 +294,12 @@ func (p *paymentSession) RequestRoute(maxAmt, feeLimit lnwire.MilliSatoshi,
 		// attempt, because concurrent payments may change balances.
 		bandwidthHints, err := p.getBandwidthHints(graph)
 		if err != nil {
+			// Close routing graph session.
+			if graphErr := closeGraph(); graphErr != nil {


This is preparation for an upcoming commit that will move over various responsibilities from the ChannelRouter to the graph Builder. So that that commit can be a pure code-move commit, the template for the new sub-system is added up front here.

This commit is a large refactor that moves over various responsibilities from the ChannelRouter to the graph.Builder. These include all graph related tasks such as: - graph pruning - validation of new network updates & persisting new updates - notifying topology update clients of any changes. This is a large commit but: - many of the files are purely moved from `routing` to `graph` - the business logic put in the graph Builder is copied exactly as is from the ChannelRouter with one exception: - The ChannelRouter just needs to be able to call the Builder's `ApplyChannelUpdate` method. So this is now exported and provided to the ChannelRouter as a config option. - The trickiest part was just moving over the test code since quite a bit had to be duplicated.

This is done in a separate commit so as to keep the original code-move commit mostly a pure code move.

Instead of relying on devs to remember that they must only be accessed atomically.

Ensure that the graph session used during pathfinding is properly closed if the call to getBandwidthHints fails.

Also move incorrect entry from 18.2 to 18.3

ellemouton · 2024-07-16T06:51:28Z

Note that the two required failing tests (unit cover and neutrino itest) are failing at result-upload time. The actual checks have passed.

ellemouton changed the title ~~multi: move graph responsibilities from routing.ChannelRouter to new graph.Builder~~ refactor: move graph responsibilities from routing.ChannelRouter to new graph.Builder Jun 18, 2024

ellemouton force-pushed the graphManager branch 3 times, most recently from 649fff5 to 44c6007 Compare June 18, 2024 22:20

saubyk assigned ellemouton Jun 19, 2024

saubyk added this to the v0.18.1 milestone Jun 19, 2024

Roasbeef requested changes Jun 19, 2024

View reviewed changes

ellemouton force-pushed the graphManager branch 2 times, most recently from c588ba0 to 25593ae Compare June 20, 2024 00:58

ellemouton commented Jun 20, 2024

View reviewed changes

ellemouton requested a review from Roasbeef June 20, 2024 01:01

Roasbeef requested changes Jun 20, 2024

View reviewed changes

routing/graph.go Outdated Show resolved Hide resolved

routing/graph.go Outdated Show resolved Hide resolved

routing/graph.go Outdated Show resolved Hide resolved

routing/graph.go Outdated Show resolved Hide resolved

ellemouton force-pushed the graphManager branch 3 times, most recently from 092aee1 to 009d60d Compare June 21, 2024 16:00

ellemouton commented Jun 21, 2024

View reviewed changes

routing/graph.go Outdated Show resolved Hide resolved

routing/graph.go Outdated Show resolved Hide resolved

ellemouton requested a review from Roasbeef June 21, 2024 16:02

saubyk added graph gossip P1 MUST be fixed or reviewed labels Jun 25, 2024

saubyk requested a review from yyforyongyu June 25, 2024 16:49

Roasbeef reviewed Jun 25, 2024

View reviewed changes

routing/graph.go Outdated Show resolved Hide resolved

ellemouton force-pushed the graphManager branch 3 times, most recently from f905ced to c9818f3 Compare June 26, 2024 16:24

ellemouton requested a review from Roasbeef June 26, 2024 16:24

Roasbeef reviewed Jun 27, 2024

View reviewed changes

ellemouton added 3 commits July 15, 2024 13:10

routing: remove sourceNode from routingGraph interface

5a903c2

In this commit, we further reduce the routingGraph interface and this time we make it more node-agnostic so that it can be backed by any graph and not one with a concept of "sourceNode".

routing: rename and export routingGraph

3f121cb

In preparation for structs outside of the `routing` package implementing this interface, export `routingGraph` and rename it to `Graph` so as to avoid stuttering.

routing+refactor: remove the need to give CachedGraph source node access

90d6b86

In preparation for the next commit.

ellemouton force-pushed the graphManager branch from c9818f3 to e6d6f34 Compare July 15, 2024 13:04

ellemouton commented Jul 15, 2024

View reviewed changes

ellemouton added 8 commits July 15, 2024 15:13

routing: let SelfNode be passed via config

cf3de72

Instead of querying it from the graph since this will be removed in a future commit.

multi+refactor: let FetchChanInfos not take tx

71e9352

In preparation for having a clean graph DB interface, refactor FetchChanInfos so that no transaction can be provided.

refactor: create FetchLightningNode with no tx param

c20d759

In preparation for adding a clean Graph DB interface, we create a version of FetchLightningNode that doesnt allow a caller to provide in a transaction.

multi+refactor: create ForEachNodeChannel with no tx param

e9c89ae

In prep for a clean Graph DB interface, we add a version of ForEachNodeChannel that does not take in an existing db transaction.

multi: move ChannelGraphSource interface

c1d7a9d

... to the new `graph` package in preparation for the implementation of the interface being moved to this new package.

channeldb: add a graph.DB interface

be84d69

..which describes the database methods that are required for graph maintaining and building.

routing: use new graph.DB interface in ChannelRouter

30e6671

ellemouton force-pushed the graphManager branch from e6d6f34 to efe4d33 Compare July 15, 2024 13:14

yyforyongyu approved these changes Jul 15, 2024

View reviewed changes

ellemouton force-pushed the graphManager branch 2 times, most recently from 5b8c54e to 048e4ba Compare July 15, 2024 13:47

ellemouton added 8 commits July 15, 2024 15:56

discovery: rename Gossiper graph dep

9327a83

funding: rename from router graph to graph

743502f

graph+routing: address linter errors

fe34d62

This is done in a separate commit so as to keep the original code-move commit mostly a pure code move.

graph: updated builder to use atomic ints

90dff73

Instead of relying on devs to remember that they must only be accessed atomically.

routing: close graph session if getBandwidthHints fails

d1c54d7

Ensure that the graph session used during pathfinding is properly closed if the call to getBandwidthHints fails.

docs: update release notes

b112e10

Also move incorrect entry from 18.2 to 18.3

ellemouton force-pushed the graphManager branch from 048e4ba to b112e10 Compare July 15, 2024 13:56

guggero merged commit fae7e0c into lightningnetwork:master Jul 16, 2024
24 of 32 checks passed

ellemouton deleted the graphManager branch July 16, 2024 06:57

ellemouton mentioned this pull request Sep 9, 2024

[epic]: ChannelDB, Graph, Gossiper and Router separation #8833

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move graph responsibilities from routing.ChannelRouter to new graph.Builder #8848

refactor: move graph responsibilities from routing.ChannelRouter to new graph.Builder #8848

ellemouton commented Jun 18, 2024 •

edited by Roasbeef

Loading

coderabbitai bot commented Jun 18, 2024

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

Roasbeef commented Jun 18, 2024

Roasbeef left a comment

ellemouton left a comment

ellemouton Jun 19, 2024

yyforyongyu Jul 10, 2024

ellemouton left a comment

Roasbeef left a comment

ellemouton commented Jun 26, 2024

Roasbeef left a comment

ellemouton left a comment

ellemouton Jul 15, 2024

ellemouton Jul 15, 2024

ellemouton Jul 15, 2024

ellemouton Jul 15, 2024

ellemouton Jul 15, 2024

ellemouton Jul 15, 2024

yyforyongyu left a comment

yyforyongyu Jul 15, 2024

yyforyongyu Jul 15, 2024

yyforyongyu Jul 15, 2024

yyforyongyu Jul 15, 2024

yyforyongyu Jul 15, 2024

yyforyongyu Jul 15, 2024

yyforyongyu Jul 15, 2024

yyforyongyu Jul 15, 2024

yyforyongyu Jul 15, 2024

ellemouton commented Jul 16, 2024

refactor: move graph responsibilities from routing.ChannelRouter to new graph.Builder #8848

refactor: move graph responsibilities from routing.ChannelRouter to new graph.Builder #8848

Conversation

ellemouton commented Jun 18, 2024 • edited by Roasbeef Loading

coderabbitai bot commented Jun 18, 2024

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

Roasbeef commented Jun 18, 2024

Roasbeef left a comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

ellemouton commented Jun 26, 2024

Roasbeef left a comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton commented Jul 16, 2024

ellemouton commented Jun 18, 2024 •

edited by Roasbeef

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)