htlcswitch: add an always on mode to htlc interceptor by joostjager · Pull Request #6232 · lightningnetwork/lnd

joostjager · 2022-02-03T14:35:04Z

Adds a flag that makes htlc interception a requirement. The switch will queue htlcs as long as there is no interceptor connected. When the stream disconnects, htlcs waiting for resolution will not resume automatically.

Supersedes #6165

Solves #6161

joostjager · 2022-02-03T14:35:43Z

htlcswitch/interceptable_switch.go

To prevent startup order issues, I think it is key to have this boolean here in the interceptable switch.

See also #6165 (comment)

joostjager · 2022-02-03T14:37:00Z

htlcswitch/interceptable_switch.go

Upon further inspection, it seems that linkQuit is not worth the hassle to thread through.

This effectively removes the feature entirely, doesn't it? Since packets always come through here. I'm not sure about the merits linkQuit has, or lacks, but it IMO this is not the PR to kill it or the way to do it. If desired, it should be removed completely from htlcSwitch rather than silently disused.

Not killing it complicates matters because we also have the resume path to the switch. In current master, linkQuit is stored and may not be usable anymore when resume is called. I think that might be a bug.

I think it is indeed best to kill it completely, but first wanted to await review of the change. Probably going to cause a large but simple delta.

Added commit deleting linkQuit.

I also deleted a complicated test case TestChannelLinkShutdownDuringForward testing linkQuit. To me it seems that the case where the switch is completely blocked isn't realistic, but maybe someone with more context can weigh in.

joostjager · 2022-02-03T14:38:53Z

lnrpc/routerrpc/forward_interceptor.go

forwardInterceptor can be cleaned up a lot now that it doesn't need the synchronization anymore to manage the hold list.

joostjager · 2022-02-03T14:40:35Z

htlcswitch/interceptable_switch.go

This is the main change. holdForwards are now kept here. This prevents complexities with InterceptableSwitch and forwardInterceptor both trying to do things with held htlcs.

htlcswitch/interceptable_switch.go

joostjager · 2022-02-03T15:33:09Z

@carlaKC @champo I tried a few variations of the always on interceptor and ended up with what's in this PR. Tried to address the outstanding comments in #6165

joostjager · 2022-02-03T17:03:39Z

htlcswitch/interceptable_switch.go

Need to think how the onchain interception flow (#6219) can connect to this.

I rebased #6219 to demonstrate how it can be done.

joostjager · 2022-02-03T17:32:10Z

htlcswitch/interceptable_switch.go

Height watching can happen in this loop as well.

Discussed offline that this will be a follow up PR

htlcswitch/interceptable_switch.go

champo · 2022-02-08T14:24:05Z

htlcswitch/interceptable_switch.go

Why should an error here stop the loop? Feels like either LND should crash or carry on.

Good point. Converted to log and carry on.

champo · 2022-02-08T14:27:22Z

htlcswitch/interceptable_switch.go

nit: I'd reference the comment on ForwardPackets below regarding linkQuit here

server.go

champo · 2022-02-08T14:57:03Z

htlcswitch/interceptable_switch.go

given that s.intercepted is an unbuffered channel, doesn't this stop the switch processing completely? ForwardPackets would block on s.intercepted <- packets until an interceptor is registered. I checked the call sites to ForwardPackets and they are not ready for it to be a blocking call. In particular, it would block the links main loop which smells like it can trigger unwanted force closes (I'm clearly not an expert on that subsystem and I can certainly be wrong).

IMO interceptForward should be handling this by simply adding the packet to the map. The logic to re-send intercepted packets on setInterceptor would handle the rest when the intercept comes back up.

Excellent point. @carlaKC also expressed concerns about this blocking behavior.

Implemented your suggestion, which is indeed simpler and cleaner.

champo · 2022-02-08T15:06:10Z

htlcswitch/interceptable_switch.go

This effectively removes the feature entirely, doesn't it? Since packets always come through here. I'm not sure about the merits linkQuit has, or lacks, but it IMO this is not the PR to kill it or the way to do it. If desired, it should be removed completely from htlcSwitch rather than silently disused.

champo · 2022-02-08T15:06:29Z

This is a great improvement! Moving the logic to interceptable switch makes a lot of sense.

carlaKC

Great refactor and nice solution to the "always on" question.

htlcswitch/interceptable_switch.go

champo · 2022-02-17T17:09:47Z

htlcswitch/interceptable_switch.go

shouldn't this pass the interceptable switch quit channel?

This code is no longer running on the link goroutine. The original reason was to prevent a deadlock with switch and link. I think the deadlock can't happen in this case.

lightninglabs-deploy · 2022-02-28T09:14:02Z

@yyforyongyu: review reminder

In this commit we move the tracking of the outstanding intercepted htlcs to InterceptableSwitch. This is a preparation for making the htlc interceptor required. Required interception involves tracking outstanding htlcs across multiple grpc client sessions. The per-session routerrpc forwardInterceptor object is therefore no longer the best place for that.

joostjager · 2022-03-15T08:48:21Z

Rebased

bhandras · 2022-03-16T15:01:42Z

Taking over review from @yyforyongyu to balance his load a bit.

bhandras · 2022-03-16T16:33:38Z

htlcswitch/interceptable_switch.go

+
+	log.Infof("Interceptor disconnected, resolving held packets")
+
+	for _, fwd := range s.holdForwards {


This is preexisting but looping through s.holdForwards may change the order of packets.

Interesting. Do you think there is a point in guarantees about ordering? I am not sure to what extend order is maintained elsewhere. For example when update_add messages come in in the link. Also a replay from the on-chain flow to the interceptor (next pr) just cuts through whatever order there may have been.

Had to dig a bit deeper to fully understand how the forwarding works. There doesn't seem to be any risk in reordering since the interceptor sits right before the Switch.

htlcswitch/interceptable_switch.go

bhandras

LGTM 🎉

htlcswitch/switch_test.go

cmd/lncli/cmd_intercept.go

Co-authored-by: Juan Pablo Civile <elementohb@gmail.com>

joostjager · 2022-03-18T18:25:43Z

Who can hit the merge button on this triple green check pr? 🙏 There is more work to do within this theme! 🚀

joostjager commented Feb 3, 2022

View reviewed changes

joostjager force-pushed the require-interceptor branch from 7fa5a7e to ce8c203 Compare February 3, 2022 15:24

joostjager commented Feb 3, 2022

View reviewed changes

htlcswitch/interceptable_switch.go Outdated Show resolved Hide resolved

joostjager commented Feb 3, 2022

View reviewed changes

htlcswitch/interceptable_switch.go Outdated Show resolved Hide resolved

joostjager requested a review from carlaKC February 3, 2022 15:31

joostjager force-pushed the require-interceptor branch from ce8c203 to be81292 Compare February 3, 2022 17:00

joostjager commented Feb 3, 2022

View reviewed changes

joostjager force-pushed the require-interceptor branch from be81292 to 368c715 Compare February 3, 2022 17:30

joostjager commented Feb 3, 2022

View reviewed changes

joostjager mentioned this pull request Feb 4, 2022

contractcourt: onchain htlc interceptor #6219

Merged

joostjager force-pushed the require-interceptor branch 4 times, most recently from ae808b2 to 2499a6a Compare February 7, 2022 07:38

joostjager changed the title ~~refactor htlc interceptor~~ htlcswitch: add an always on mode to htlc interceptor Feb 7, 2022

joostjager marked this pull request as ready for review February 7, 2022 07:48

joostjager force-pushed the require-interceptor branch 2 times, most recently from 390dc05 to 64580c5 Compare February 7, 2022 07:58

joostjager mentioned this pull request Feb 7, 2022

lnrpc,routing: add an always on mode to HTLC interceptor #6165

Closed

joostjager removed the request for review from carlaKC February 7, 2022 08:15

joostjager force-pushed the require-interceptor branch 3 times, most recently from cbe83d6 to d01e22f Compare February 7, 2022 08:54

joostjager requested a review from carlaKC February 7, 2022 09:01

champo reviewed Feb 8, 2022

View reviewed changes

carlaKC approved these changes Feb 17, 2022

View reviewed changes

htlcswitch/interceptable_switch.go Outdated Show resolved Hide resolved

joostjager force-pushed the require-interceptor branch from c3d1b4a to f4d855d Compare February 17, 2022 15:26

champo approved these changes Feb 17, 2022

View reviewed changes

carlaKC requested a review from yyforyongyu February 21, 2022 08:26

Roasbeef added bug fix htlcswitch P1 MUST be fixed or reviewed labels Mar 7, 2022

Roasbeef modified the milestones: v0.16.0, v0.15.0 Mar 7, 2022

joostjager force-pushed the require-interceptor branch 2 times, most recently from 01a3dac to 5c89c14 Compare March 15, 2022 07:54

bhandras requested review from bhandras and removed request for yyforyongyu March 16, 2022 14:59

bhandras reviewed Mar 16, 2022

View reviewed changes

joostjager requested a review from bhandras March 17, 2022 10:40

Neil-LL assigned bhandras Mar 17, 2022

bhandras approved these changes Mar 17, 2022

View reviewed changes

htlcswitch/switch_test.go Outdated Show resolved Hide resolved

cmd/lncli/cmd_intercept.go Outdated Show resolved Hide resolved

htlcswitch: add an always on mode to interceptable switch

ae314ec

Co-authored-by: Juan Pablo Civile <elementohb@gmail.com>

joostjager force-pushed the require-interceptor branch from 5c89c14 to ae314ec Compare March 17, 2022 16:39

guggero merged commit 57840bb into lightningnetwork:master Mar 19, 2022

joostjager mentioned this pull request Mar 22, 2022

htlcswitch: interceptor expiry check #6212

Merged

Roasbeef mentioned this pull request Mar 30, 2022

Htlc interceptor race condition on startup #6161

Closed

joostjager mentioned this pull request Aug 31, 2022

htlcswitch: add interceptor channel filter #6874

Open

1 task

joostjager mentioned this pull request Sep 7, 2022

htlcswitch: add always-required interceptor mode #6896

Open


		log.Infof("Interceptor disconnected, resolving held packets")

		for _, fwd := range s.holdForwards {

Conversation

joostjager commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joostjager Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joostjager commented Feb 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

champo commented Feb 8, 2022

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightninglabs-deploy commented Feb 28, 2022

Uh oh!

joostjager commented Mar 15, 2022

Uh oh!

bhandras commented Mar 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joostjager commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

joostjager commented Feb 3, 2022 •

edited

Loading

joostjager Feb 3, 2022 •

edited

Loading

joostjager commented Mar 18, 2022 •

edited

Loading