Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 22 commits into
base: elle-moveGraphDBCode
Choose a base branch
from
Open

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

wants to merge 22 commits into from

Conversation

ellemouton
Copy link
Collaborator

@ellemouton ellemouton commented Nov 4, 2024 •
edited
Loading

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.

@ellemouton ellemouton self-assigned this Nov 4, 2024
@coderabbitai coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2024 •
edited
Loading

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
Copy link
Collaborator

Exciting functionality!

@guggero guggero self-requested a review November 4, 2024 16:24
@ellemouton ellemouton marked this pull request as draft November 8, 2024 17:41
@ellemouton
Copy link
Collaborator Author

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

@guggero guggero removed their request for review November 11, 2024 09:00
@ellemouton ellemouton marked this pull request as ready for review November 13, 2024 09:06
@ellemouton ellemouton changed the title multi: Graph Source Abstraction [2/3] Graph RIP: multi: Graph Source Abstraction Nov 13, 2024
@ellemouton ellemouton mentioned this pull request Nov 13, 2024
Open
@ellemouton
Copy link
Collaborator Author

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

@ellemouton ellemouton mentioned this pull request Nov 13, 2024
Open
@ellemouton
graph: remove unused ForEachNode method
07afe62 
Simplify the ChannelGraphSource interface by removing this unused
method.
@ellemouton
graph: let FetchNodeFeatures take an optional read tx
e2b2917 
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.
@ellemouton
graph: rename directory from graphsession to session
6a51a6a 
The package name is kept the same. So this is just for less stuttering
at a path level.
@ellemouton
lnd+graph: add GraphSource interface and implementation
7288374 
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.
@ellemouton
graph: add ReadOnlyGraph interface to GraphSource interface
fcd8ec9 
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.
@ellemouton
graph: add contexts to the ReadOnlyGraph interface
541b093 
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.
@ellemouton
invoicesrpc: remove invoicerpc server's access to ChannelGraph pointer
2704e60 
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.
@ellemouton
netann+lnd: add netann.ChannelGraph to the GraphSource interface
c2fe017 
And let DBSource implement it.
@ellemouton
graph+channeldb: add AddrSource interface to GraphSource
64c5cf4
@ellemouton
graph+lnd: add various calls to GraphSource
5dbeb97 
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.
@ellemouton
discovery: pass contexts to NetworkPeerBootstrapper methods
6103bc1 
We will later implement this interface with a backing RPC connection and
so it makes sense to pass a context through for cancellation.
@ellemouton
lnd+graph: add GraphBootstrapper to the GraphSource interface
801d672 
So that LND can use a different GraphSource for network bootstrapping
and does not need to rely on its local graph db.
@ellemouton
graph+lnd: add NetworkStats to GraphSource interface
6cb3be5 
so that the external graph source can be used to query network
information rather than depending on the local graph DB.
@ellemouton
graph+lnd: add BetweennessCentrality to GraphSource interface
044f371 
So that the calcuation is abstracted behind the interface and not
necessarily dependent on LND's local channel graph.
@bhandras bhandras self-requested a review November 21, 2024 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bhandras bhandras Awaiting requested review from bhandras

Assignees

@ellemouton ellemouton

Labels
gossip graph interface abstraction
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ellemouton @dstadulis