htlcswitch: interceptor expiry check #6212

joostjager · 2022-01-28T11:57:39Z

Adds a basic protection to make sure that we aren't letting the interceptor handle htlcs that are too close to the channel force-close broadcast height.

carlaKC

I think its worth thinking about the full solution for the expiry handling that we want for the interceptor here. Specifically, the case where the interceptor holds on to a htlc for too long (risking force close), similar to hold invoices.

I think it would make sense to make the delta configurable through the interceptor api or add a force_release_height field to intercepted hltcs so that calling code knows when they'll just be dropped to prevent a force close. I would lean towards the latter, since I imagine most people will just use the sane default we set?

server.go

joostjager · 2022-01-28T13:44:27Z

I think its worth thinking about the full solution for the expiry handling that we want for the interceptor here. Specifically, the case where the interceptor holds on to a htlc for too long (risking force close), similar to hold invoices.

Not having the full solution is probably asking for trouble indeed. Especially in combination with #6165, force-closes will already happen when the interceptor is only down for too long.

I think it would make sense to make the delta configurable through the interceptor api or add a force_release_height field to intercepted hltcs so that calling code knows when they'll just be dropped to prevent a force close. I would lean towards the latter, since I imagine most people will just use the sane default we set?

If we have auto-release, then force_release_height sounds like a useful addition.

joostjager · 2022-03-22T08:36:07Z

Rebased this PR after the merge of #6232.

I know that ideally we'd like to have more functionality for expiry watching, but this PR is the minimum required for short-hold interception applications to allow those applications to not worry about expiration.

carlaKC

Happy with minimal approach to start with. Just one error handling question from me.

htlcswitch/interceptable_switch.go

carlaKC · 2022-03-23T07:19:37Z

htlcswitch/interceptable_switch.go

+			log.Errorf("Error handling intercepted htlc "+
+				"that expires too soon: circuit=%v, "+
+				"incoming_timeout=%v, err=%v",
+				packet.inKey(), packet.incomingTimeout, err)


Do we want to proceed here if we errored out/did not handle (ie FailWithCode errored in handleExpired), shouldn't we return false?

Good one, I indeed forgot a return statement there.

Interesting that you say return false, because I was thinking of returning true. With return false, the packet is passed on to the switch as normal and this violates the always-on requirement, if that is configured. Returning true isn't ideal either, because the htlc will remain stuck.

Maybe this is all hypothetical, because there is no way for an error to happen unless there is a bug in the code?

In the hypothetical case, isn't it better to release to the switch and let it handle the expiry accordingly? Chances are that there's nowhere to forward it onwards (since generally there's no next hop when intercepting), so it'll fail back anyway?

this violates the always-on requirement

always on vs stuck htlc, difficult call

Whether there is nothing to forward onwards depends on the application I think. For external invoice handling it is true, but I think the interceptor is also used for the interception of regular routed traffic?

I think that one can come up with a high stakes htlc case where a stuck htlc and force-closed channel is preferable to skipping the interceptor.

But as this error path should be possible in theory only, I am fine with returning false. Updated.

carlaKC

🥇

peer/test_utils.go

htlcswitch/switch_test.go

htlcswitch/interceptable_switch.go

bhandras · 2022-03-23T13:23:22Z

Fyi the inteceptor tests have a data race (also in this PR), ptal #6353

joostjager · 2022-03-23T15:42:30Z

Fyi the inteceptor tests have a data race (also in this PR), ptal #6353

I think I didn't add more of those in this PR. Didn't rebase, but hope for a clean auto-merge after #6353 is merged.

bhandras

LGTM 🎉

bhandras · 2022-03-23T15:57:03Z

htlcswitch/switch_test.go

+
+	// Simulate an error during the composition of the failure message.
+	currentCallback := s.cfg.FetchLastChannelUpdate
+	s.cfg.FetchLastChannelUpdate = func(


Nice that the error is so simple to fake 👍

Yes, got away cleaner than expected 😅

peer/test_utils.go

Roasbeef · 2022-03-23T23:18:10Z

htlcswitch/interceptable_switch.go

@@ -55,6 +55,10 @@ type InterceptableSwitch struct {
 	// holdForwards keeps track of outstanding intercepted forwards.
 	holdForwards map[channeldb.CircuitKey]InterceptedForward

+	// cltvRejectDelta defines the number of blocks before the expiry of the
+	// htlc where we no longer intercept it and instead cancel it back.
+	cltvRejectDelta uint32


We already have a value for this in OutgoingCltvRejectDelta, so no need to make a new value here.

Yes, OutgoingCltvRejectDelta is passed around in several structures, but afaik it isn't directly accessible here. Or do you suggest to hardcode it to lncfg.DefaultOutgoingCltvRejectDelta here?

Was thinking that we should use DefaultOutgoingCltvRejectDelta but then saw that we use the final CLTV delta here, which I think is more apt as in most cases the intercepted HTLC will be terminated by the program operating the HTLC interceptor.

lightninglabs-deploy · 2022-04-07T08:14:15Z

@Roasbeef: review reminder

Roasbeef

LGTM 🏒

Roasbeef · 2022-04-12T03:06:36Z

This should be good to land after a rebase to clear up the conflicts.

Decouple the tests somewhat and fix a bug along the way where the test passed because of a left-over package from a prior test.

joostjager force-pushed the interceptor-check branch from d899502 to 54852b9 Compare January 28, 2022 11:59

joostjager mentioned this pull request Jan 28, 2022

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

Closed

joostjager requested a review from carlaKC January 28, 2022 12:04

carlaKC reviewed Jan 28, 2022

View reviewed changes

server.go Show resolved Hide resolved

Roasbeef added this to the v0.15.0 milestone Jan 28, 2022

Roasbeef added htlcswitch P1 MUST be fixed or reviewed bug fix labels Feb 14, 2022

Roasbeef requested a review from bhandras February 14, 2022 18:23

joostjager force-pushed the interceptor-check branch 2 times, most recently from 91fc959 to 0a67ab2 Compare March 22, 2022 08:31

joostjager requested a review from carlaKC March 22, 2022 08:59

carlaKC reviewed Mar 23, 2022

View reviewed changes

joostjager force-pushed the interceptor-check branch from 0a67ab2 to 9d32265 Compare March 23, 2022 07:38

joostjager requested a review from carlaKC March 23, 2022 07:39

joostjager force-pushed the interceptor-check branch from 9d32265 to cf0bb13 Compare March 23, 2022 08:33

carlaKC approved these changes Mar 23, 2022

View reviewed changes

bhandras reviewed Mar 23, 2022

View reviewed changes

peer/test_utils.go Outdated Show resolved Hide resolved

htlcswitch/switch_test.go Outdated Show resolved Hide resolved

htlcswitch/interceptable_switch.go Show resolved Hide resolved

joostjager force-pushed the interceptor-check branch from cf0bb13 to 9ab7d2c Compare March 23, 2022 15:41

joostjager requested a review from bhandras March 23, 2022 15:46

bhandras approved these changes Mar 23, 2022

View reviewed changes

joostjager force-pushed the interceptor-check branch from 9ab7d2c to 83a940c Compare March 23, 2022 16:02

Roasbeef requested changes Mar 23, 2022

View reviewed changes

joostjager requested a review from Roasbeef March 24, 2022 07:27

joostjager force-pushed the interceptor-check branch from 83a940c to 53265fc Compare April 2, 2022 22:27

joostjager mentioned this pull request Apr 3, 2022

contractcourt: onchain htlc interceptor #6219

Merged

Roasbeef approved these changes Apr 12, 2022

View reviewed changes

joostjager added 2 commits April 12, 2022 08:27

htlcswitch/test: isolate test packets

bae0b6b

Decouple the tests somewhat and fix a bug along the way where the test passed because of a left-over package from a prior test.

htlcswitch: interceptor expiry check

13dff2f

joostjager force-pushed the interceptor-check branch from 53265fc to 13dff2f Compare April 12, 2022 06:27

guggero merged commit 8047f23 into lightningnetwork:master Apr 12, 2022

joostjager mentioned this pull request Aug 15, 2022

htlcswitch: interceptor watchdog #6831

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

htlcswitch: interceptor expiry check #6212

htlcswitch: interceptor expiry check #6212

joostjager commented Jan 28, 2022

carlaKC left a comment

joostjager commented Jan 28, 2022

joostjager commented Mar 22, 2022 •

edited

Loading

carlaKC left a comment

carlaKC Mar 23, 2022

joostjager Mar 23, 2022

carlaKC Mar 23, 2022

joostjager Mar 23, 2022

carlaKC left a comment

bhandras commented Mar 23, 2022

joostjager commented Mar 23, 2022

bhandras left a comment

bhandras Mar 23, 2022

joostjager Mar 23, 2022

Roasbeef Mar 23, 2022

joostjager Mar 24, 2022

Roasbeef Apr 12, 2022

lightninglabs-deploy commented Apr 7, 2022

Roasbeef left a comment

Roasbeef commented Apr 12, 2022

htlcswitch: interceptor expiry check #6212

htlcswitch: interceptor expiry check #6212

Conversation

joostjager commented Jan 28, 2022

carlaKC left a comment

Choose a reason for hiding this comment

joostjager commented Jan 28, 2022

joostjager commented Mar 22, 2022 • edited Loading

carlaKC 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

carlaKC left a comment

Choose a reason for hiding this comment

bhandras commented Mar 23, 2022

joostjager commented Mar 23, 2022

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

lightninglabs-deploy commented Apr 7, 2022

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef commented Apr 12, 2022

joostjager commented Mar 22, 2022 •

edited

Loading