channeldb+refactor: remove `kvdb.Backend` from channel DB schemas #8117

ellemouton · 2023-10-26T12:35:50Z

Overview

Today, the channeldb.LightningNode, channeldb.ChannelEdgePolicy and
channeldb.ChannelEdgeInfo objects all carry a kvdb.Backend pointer.
Additionally, the ChannelEdgePolicy has a channeldb.LightningNode
member.

This PR is a move towards making the above mentioned channeldb structs
more closely represent the on-disk schemas and does this by removing the
need for them to carry around the kvdb.Backend pointer and also removes
the need for channeldb.ChannelEdgeInfo to contain a
channeldb.LightningNode.

Why?

For the Gossip 1.75 epic, it will be required to create abstractions of the mentioned
objects and so in order to avoid requiring interface methods like "GetDB()" and
"SetDB()", removing the db entirely from the objects will make for much simpler code.

I think this might also be useful for when we migrate channeldb to the sql world.

ellemouton · 2023-10-27T07:01:11Z

realised that with this change we can actually move the ChannelEdgeInfo, ChannelEdgePolicy, ChannelAuthProof and CachedEdgePolicy structs to the channeldb/models package instead.

Lemme know what you guys think & then I'll add a commit on.

bhandras

Nice refactor! LGTM 🎉

bhandras · 2023-10-26T18:38:43Z

autopilot/graph.go

-		if ep == nil {
-			return nil
-		}
+	return d.db.ForEachNodeChannel(d.tx, d.node.PubKeyBytes,


I wonder if there's a specific reason to not use pointer receivers for dbNode methods?

no idea 🤔 i can add a commit to try change it if you'd like?

Yeah I think it's probably cleaner to use pointer receivers, it's also just a small cleanup not really inflating the PR so feel free to add if you agree.

cool - added a commit after this one :)

bhandras · 2023-10-27T11:02:32Z

rpcserver.go

-		return nil
-	}); err != nil {
+			return nil
+		}); err != nil {


nit: maybe it's just the weird diff but formatting seems a bit off.

ah yes - good catch

bhandras · 2023-10-27T15:27:04Z

channeldb/graph.go

-	// this pointer the channel graph can further be traversed.
-	Node *LightningNode
+	// ToNode is the public key of the node that this directed edge leads
+	// to. Using this pointer the channel graph can further be traversed.


nit: "Using this pointer" => "Using this pubkey".

bhandras · 2023-10-27T15:28:16Z

autopilot/graph.go

@@ -104,6 +104,11 @@ func (d dbNode) ForEachChannel(cb func(ChannelEdge) error) error {
 				return nil
 			}

+			node, err := d.db.FetchLightningNode(tx, ep.ToNode)


nit: in commit message: frmo => from, exaclty => exactly

bhandras · 2023-10-27T15:32:30Z

realised that with this change we can actually move the ChannelEdgeInfo, ChannelEdgePolicy, ChannelAuthProof and CachedEdgePolicy structs to the channeldb/models package instead.

Lemme know what you guys think & then I'll add a commit on.

What would be the benefit of this move?

channeldb/graph_test.go

channeldb/graph.go

lightninglabs-deploy · 2023-11-03T16:11:33Z

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

Having a `ForEachChannel` method on the `LightningNode` struct itself results in a `kvdb.Backend` object needing to be stored within the LightningNode struct. In this commit, this method is replaced with a `ForEachNodeChannel` method on the `ChannelGraph` struct will perform the same function without needing the db pointer to be stored within the LightningNode. This change, the LightningNode struct more closely represents the schema on disk. The existing `ForEachNodeChannel` method on `ChannelGraph` is renamed to `ForEachNodeDirectedChannel`. It performs a slightly different function since it's call-back operates on Cached policies.

ellemouton · 2023-11-08T09:28:44Z

What would be the benefit of this move?

Basically just to continue to separate out schema from DB fetching/setting logic. Some packages only need the schemas & so dont need to import the entire channeldb package.

I tacked on a commit at the end that does the move - lemme know if you think it makes sense - otherwise, im happy to drop it for now too :)

bhandras

LGTM 🥇

bhandras · 2023-11-08T12:27:19Z

autopilot/graph.go

-		if ep == nil {
-			return nil
-		}
+	return d.db.ForEachNodeChannel(d.tx, d.node.PubKeyBytes,


Yeah I think it's probably cleaner to use pointer receivers, it's also just a small cleanup not really inflating the PR so feel free to add if you agree.

bhandras · 2023-11-08T12:31:09Z

What would be the benefit of this move?

Basically just to continue to separate out schema from DB fetching/setting logic. Some packages only need the schemas & so dont need to import the entire channeldb package.

I tacked on a commit at the end that does the move - lemme know if you think it makes sense - otherwise, im happy to drop it for now too :)

I like the change, slimming channeldb always makes sense :)

Now that the kvdb.Backend is no longer required, it is removed from the LightningNode struct.

To prepare for the `kvdb.Backend` member of `ChannelEdgeInfo` being removed, the `FetchOtherNode` method is moved from the `ChannelEdgeInfo` to the `ChannelGraph` struct.

In preparation for the next commit which will remove the `*LightningNode` from the `ChannelEdgePolicy` struct, `FetchLightningNode` is modified to take in an optional transaction so that it can be utilised in places where a transaction exists.

Finally, The LightningNode object is removed from ChannelEdgePolicy. This is a step towards letting ChannelEdgePolicy reflect exactly the schema that is on disk. This is also nice because the `Node` object is not necessarily always required when the ChannelEdgePolicy is loaded from the DB, so now it only get's loaded when needed.

This commit moves the ChannelEdgePolicy, ChannelEdgeInfo, ChanelAuthProof and CachedEdgePolicy structs to the `channeldb/models` package.

Crypt-iQ

LGTM 🎲

ellemouton mentioned this pull request Oct 26, 2023

[epic]: Gossip 1.75 #7961

Open

21 tasks

Roasbeef requested review from bhandras and Crypt-iQ October 26, 2023 17:55

Roasbeef added this to the v0.18.0 milestone Oct 26, 2023

ellemouton force-pushed the removeDBFromChannelDBSchemas branch from daf6a5d to c9a27a9 Compare October 27, 2023 06:10

bhandras approved these changes Oct 27, 2023

View reviewed changes

Crypt-iQ reviewed Oct 27, 2023

View reviewed changes

channeldb/graph_test.go Outdated Show resolved Hide resolved

channeldb/graph.go Outdated Show resolved Hide resolved

channeldb/graph.go Outdated Show resolved Hide resolved

saubyk assigned ellemouton Oct 28, 2023

saubyk added refactoring kvdb channels labels Oct 28, 2023

ellemouton force-pushed the removeDBFromChannelDBSchemas branch from c9a27a9 to 4c3252d Compare November 8, 2023 09:25

ellemouton force-pushed the removeDBFromChannelDBSchemas branch 2 times, most recently from 8b0eaea to 0ff271b Compare November 8, 2023 10:08

ellemouton requested review from Crypt-iQ and bhandras November 8, 2023 10:23

bhandras approved these changes Nov 8, 2023

View reviewed changes

ellemouton added 7 commits November 8, 2023 14:50

autopilot: use pointer receivers for dbNode methods

d37f5d7

channeldb: remove kvdb.Backend from LightningNode

6d878c6

Now that the kvdb.Backend is no longer required, it is removed from the LightningNode struct.

channeldb: convert FetchOtherNode to ChannelGraph method

9dc1a1d

To prepare for the `kvdb.Backend` member of `ChannelEdgeInfo` being removed, the `FetchOtherNode` method is moved from the `ChannelEdgeInfo` to the `ChannelGraph` struct.

channeldb: remove kvdb.Backend from ChannelEdgeInfo

f06f08f

channeldb: remove kvdb.Backend from ChannelEdgePolicy

9a62a46

ellemouton added 4 commits November 8, 2023 14:50

channeldb: remove unused func arguments

78ff28f

docs: update release notes

be23986

multi: move DB schemas to channeldb/models

84cdcd6

This commit moves the ChannelEdgePolicy, ChannelEdgeInfo, ChanelAuthProof and CachedEdgePolicy structs to the `channeldb/models` package.

multi: address linter issues

0e82293

ellemouton force-pushed the removeDBFromChannelDBSchemas branch from 0ff271b to 0e82293 Compare November 8, 2023 12:50

Crypt-iQ approved these changes Nov 8, 2023

View reviewed changes

ellemouton added this pull request to the merge queue Nov 8, 2023

github-merge-queue bot removed this pull request from the merge queue due to Branch Protection failures Nov 8, 2023

You're not authorized to push to this branch. Visit "About protected branches" for more information.

ellemouton merged commit 6122452 into lightningnetwork:master Nov 9, 2023
24 of 25 checks passed

ellemouton deleted the removeDBFromChannelDBSchemas branch November 9, 2023 05:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

channeldb+refactor: remove `kvdb.Backend` from channel DB schemas #8117

channeldb+refactor: remove `kvdb.Backend` from channel DB schemas #8117

ellemouton commented Oct 26, 2023 •

edited

Loading

ellemouton commented Oct 27, 2023

bhandras left a comment

bhandras Oct 26, 2023

ellemouton Nov 8, 2023

bhandras Nov 8, 2023

ellemouton Nov 8, 2023

bhandras Oct 27, 2023

ellemouton Nov 8, 2023

bhandras Oct 27, 2023

bhandras Oct 27, 2023

bhandras commented Oct 27, 2023

lightninglabs-deploy commented Nov 3, 2023

ellemouton commented Nov 8, 2023

bhandras left a comment

bhandras Nov 8, 2023

bhandras commented Nov 8, 2023

Crypt-iQ left a comment

channeldb+refactor: remove kvdb.Backend from channel DB schemas #8117

channeldb+refactor: remove kvdb.Backend from channel DB schemas #8117

Conversation

ellemouton commented Oct 26, 2023 • edited Loading

Overview

Why?

ellemouton commented Oct 27, 2023

bhandras 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

bhandras commented Oct 27, 2023

lightninglabs-deploy commented Nov 3, 2023

ellemouton commented Nov 8, 2023

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhandras commented Nov 8, 2023

Crypt-iQ left a comment

Choose a reason for hiding this comment

channeldb+refactor: remove `kvdb.Backend` from channel DB schemas #8117

channeldb+refactor: remove `kvdb.Backend` from channel DB schemas #8117

ellemouton commented Oct 26, 2023 •

edited

Loading