htlcswitch+peer [1/2]: thread context through in preparation for passing to graph DB calls #9691

ellemouton · 2025-04-09T06:30:42Z

PR context

This is part of the last step required to complete #9494. We want all calls to the graph DB (and hence, ChannelGraph) to take a context since later, this context will be passed through to any remote graph RPC calls as well as any DB calls to a SQL graph backend.

This PR specifically: This is part 1 of a 2 part PR series that addresses the `htlcswitch`

In this PR, we tackle the htlcswitch & peer packages. This is split over 2 PRs since it is quite a lot. This PR is part 1. See part 2 for the full picture. This package makes calls to the graph's FetchLastChannelUpdate call. So this PR is all aimed at threading contexts through to the call-sites of this call-back. It leaves 1 context.TODO() in the main LND server which will be addressed in a follow up.

Branch strategy:

This series of PRs is basically a big code refactor. So once 19 is shipped, we can merge any work that has been merged into elle-graph and from then on we can just merge into master.

coderabbitai · 2025-04-09T06:30:48Z

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.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ 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 generate docstrings to generate docstrings for this 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.

bhandras

LGTM ✅

yyforyongyu

Ended up reviewing #9699 to understand the full pic. Left some questions.

htlcswitch/interceptable_switch.go

yyforyongyu · 2025-04-16T09:58:41Z

htlcswitch/link.go

 	if update == nil {
 		// Fallback to the non-alias behavior.
 		var err error
-		update, err = l.cfg.FetchLastChannelUpdate(l.ShortChanID())
+		update, err = l.cfg.FetchLastChannelUpdate(ctx, l.ShortChanID())


Since the main purpose is to create ctx to be used in graph related calls, I don't think we need to have this ctx popped up alone the callsite, ending up all the methods in the chain taking a never checked ctx, given we can just create a ctx here via l.cg.Create(context.Background()). And if the link is stopping, that ctx will be canceled.

I think it is dangerous to do that because what if the source of this call was not from internal (ie, from channelLink) but was actually triggered from an external system. Then if that system exists, this DB call wont be cancelled since the ctx is coming from the incorrect source

nding up all the methods in the chain taking a never checked ctx

contexts only need to be "checked" if we are doing something that hangs/retries etc. so anywhere we currently have a select that can hang, then there we should always check it.

Like, for example: createFailureWithUpdate is called by CheckHtlcForward which is called by the switch. so not all call-sites are from within the link. It's safer to just have a ctx as a first param to methods so that we dont need to think about the origins of the call

Let’s look at the specifics? Basically,

If the link.ctx is done, does it mean the switch.ctx is also done? Nope because other systems can call RemoveLink to shut down the link.

If the switch.ctx is done, does it mean the link.ctx is done? Yes, as when switch shuts down, it cleans up all the links in htlcForwarder.

This is because the switch can be viewed as the link manager, and it’s responsible for starting and stopping the links.

Then if that system exists, this DB call wont be cancelled since the ctx is coming from the incorrect source

This is def correct. It was discussed before that maybe we should decouple switch and link. But as for now, we want to maintain the relationship, which means when switch goes down, link goes down.

Like, for example: createFailureWithUpdate is called by CheckHtlcForward which is called by the switch. so not all call-sites are from within the link.

Also correct. Tho given the relationship between switch and link, it does really matter since when switch.ctx is done, link.ctx is also done. I’m also exploring the possibility to make link and private struct managed by switch only.

It’s worth mentioning that things may be different if we took the previous approach, that each Start takes a ctx, which means each system’s internal context can be a child of an external one. Not saying we should go with the previous approach, just wanna mention a simple change there can make a different design choice.

It's safer to just have a ctx as a first param to methods so that we dont need to think about the origins of the call

I’ve been thinking about it these days. It may seem nice to be oblivious about the origin, but I don’t think it’s safe to do so. It gives the external caller the ability to abort actions, while some actions cannot be aborted, it’s case-specific. Good thing is we only have an internal context here, and it’s only used to abort graph-related calls, so one less thing to worry about.

I think it is valuable to have a standard pattern for things like ctx. The standard pattern is that the caller's context is passed through for sync calls.

If the link and switch go down together anyways, then i dont see the harm in just keeping this standard pattern rather than now threading through the incorrect context.

while some actions cannot be aborted, it’s case-specific.

ok but yes it is case specific, for anything really critical like that, we should explicitly not pass in the caller's context.

yyforyongyu · 2025-04-16T10:07:37Z

htlcswitch/link.go

@@ -3207,7 +3210,7 @@ func (l *channelLink) getAliases() []lnwire.ShortChannelID {
 // attachFailAliasUpdate sets the link's FailAliasUpdate function.
 //
 // Part of the scidAliasHandler interface.
-func (l *channelLink) attachFailAliasUpdate(closure func(
+func (l *channelLink) attachFailAliasUpdate(closure func(ctx context.Context,


Then I think we can create a ctx via l.cg.Create(context.Background()) and pass it to the closure below so it still satisfies the signature failAliasUpdate, think will bring way less change, and create fewer conflicts in other people's PRs.

see other comment

htlcswitch/mailbox.go

htlcswitch/switch.go

yyforyongyu · 2025-04-16T10:37:12Z

peer/brontide.go

@@ -1521,7 +1523,7 @@ func (p *Brontide) maybeSendChannelUpdates() {
 		// to fetch the update to send to the remote peer. If the
 		// channel is pending, and not a zero conf channel, we'll get
 		// an error here which we'll ignore.
-		chanUpd, err := p.cfg.FetchLastChanUpdate(scid)
+		chanUpd, err := p.cfg.FetchLastChanUpdate(ctx, scid)


same re ctx, we can derive it from l.cg

see previous comment

Both the `htlcswitch` and `peer` systems take a `FetchLastChanUpdate` call back which underneath will call the `graphBuilder`'s `GetChannelByID` method which calls the graph DB's `FetchChannelEdgeByID` method which will eventually take a context. A couple of `context.TODO()`s are added which will be removed in later commits.

which calls the `FetchLastChannelUpdate` call back.

In this commit we also derive fresh contexts and then call the `cancel` functions of those in the `Stop` methods of: - `InterceptableSwitch` - `Switch` This removes two previously added `context.TODO`s and introduces a few more.

and introduce one more.

And remove one context.TODO

ellemouton added the no-changelog label Apr 9, 2025

ellemouton self-assigned this Apr 9, 2025

ellemouton changed the title ~~htlcswitch: thread context through in preparation for passing to graph DB calls~~ htlcswitch+peer: thread context through in preparation for passing to graph DB calls Apr 9, 2025

ellemouton force-pushed the ctx4 branch 4 times, most recently from ed0f260 to 00150d6 Compare April 10, 2025 11:10

ellemouton changed the title ~~htlcswitch+peer: thread context through in preparation for passing to graph DB calls~~ htlcswitch+peer [1/2]: thread context through in preparation for passing to graph DB calls Apr 10, 2025

ellemouton force-pushed the ctx4 branch from 00150d6 to 81cc646 Compare April 10, 2025 11:28

ellemouton mentioned this pull request Apr 10, 2025

htlcswitch+peer [2/2]: thread context through in preparation for passing to graph DB calls #9699

Closed

saubyk added this to lnd v0.20 Apr 10, 2025

saubyk moved this to In progress in lnd v0.20 Apr 10, 2025

ellemouton force-pushed the ctx4 branch 2 times, most recently from a6321cc to ea67b33 Compare April 14, 2025 12:29

ellemouton requested review from yyforyongyu and bhandras April 14, 2025 14:56

bhandras approved these changes Apr 16, 2025

View reviewed changes

yyforyongyu reviewed Apr 16, 2025

View reviewed changes

ellemouton added 8 commits April 17, 2025 11:57

htlcswitch: thread context to createFailureWithUpdate

7e2c077

which calls the `FetchLastChannelUpdate` call back.

htlcswitch: remove some context.TODOs

7d4ff68

In this commit we also derive fresh contexts and then call the `cancel` functions of those in the `Stop` methods of: - `InterceptableSwitch` - `Switch` This removes two previously added `context.TODO`s and introduces a few more.

htlcswitch: remove one context.TODO

d3f99a1

htlcswitch: remove a context.TODO()

61f1270

htlcswitch: remove a context.TODO

73fb63d

and introduce one more.

htlcswitch: remove a context.TODO

e693653

htlcswitch: add ctx, cancel to memoryMailbox

c31f2c9

And remove one context.TODO

ellemouton force-pushed the ctx4 branch from ea67b33 to c31f2c9 Compare April 17, 2025 10:06

ellemouton requested a review from yyforyongyu April 17, 2025 10:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

htlcswitch+peer [1/2]: thread context through in preparation for passing to graph DB calls #9691

htlcswitch+peer [1/2]: thread context through in preparation for passing to graph DB calls #9691

ellemouton commented Apr 9, 2025 •

edited

Loading

coderabbitai bot commented Apr 9, 2025 •

edited

Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

bhandras left a comment

yyforyongyu left a comment

yyforyongyu Apr 16, 2025

ellemouton Apr 17, 2025

ellemouton Apr 17, 2025

ellemouton Apr 17, 2025

yyforyongyu Apr 18, 2025

ellemouton Apr 22, 2025

yyforyongyu Apr 16, 2025

ellemouton Apr 17, 2025

yyforyongyu Apr 16, 2025

ellemouton Apr 17, 2025

htlcswitch+peer [1/2]: thread context through in preparation for passing to graph DB calls #9691

Are you sure you want to change the base?

htlcswitch+peer [1/2]: thread context through in preparation for passing to graph DB calls #9691

Conversation

ellemouton commented Apr 9, 2025 • edited Loading

PR context

This PR specifically: This is part 1 of a 2 part PR series that addresses the htlcswitch

Branch strategy:

coderabbitai bot commented Apr 9, 2025 • edited Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

bhandras left a 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

Choose a reason for hiding this comment

ellemouton commented Apr 9, 2025 •

edited

Loading

This PR specifically: This is part 1 of a 2 part PR series that addresses the `htlcswitch`

coderabbitai bot commented Apr 9, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)