htlcswitch: interceptor watchdog #6831

joostjager · 2022-08-15T15:32:09Z

Fixes a left-over of the htlc interception api implementation that prevents channels from force-closing if a connected interceptor client does not resolve or resume an htlc in time.

Original mention: #6212 (review)

Crypt-iQ

looks good, have one comment

lncfg/config.go

carlaKC

Nice to see this change come in! I think it would be a nice addition to surface the height that htlcs will be auto-released on the interceptor api so that calling code can be mindful of that height.

htlcswitch/interceptable_switch.go

htlcswitch/held_htlc_set.go

joostjager · 2022-09-01T08:12:15Z

@Crypt-iQ do you want to assign a second Lightning Labs reviewer to this?

carlaKC

Sorry for the delay on looking a this, looks good now.

I do still think that we should surface the "release height" on the interceptor rpc so that callers know when their htlcs are going to be dropped. Otherwise you need to configure your code with something like --lnd.interceptordelta=x, which will be incorrect if LND changes its defaults/the value is made configurable. Can be done in a follow up, but I think it's important to make this feature more user-friendly.

htlcswitch/held_htlc_set.go

joostjager · 2022-09-06T07:06:24Z

Yes, agreed that adding a height to the interceptor request rpc message is useful. Even just during development. Added.

Crypt-iQ

I think we could use another reviewer, not because we need LL reviewers, but because we're dealing with switch-related code and it can be tricky

Roasbeef · 2022-10-07T01:43:13Z

I think this just needs a rebase then it can land.

joostjager · 2022-10-07T07:15:20Z

Rebased

lightninglabs-deploy · 2022-10-14T07:38:08Z

@yyforyongyu: review reminder

yyforyongyu

Overall looks good. Left several nits and a question about using panic. I think one day we'll need to unify block consumption in one place.

htlcswitch/interceptable_switch.go

htlcswitch/switch_test.go

yyforyongyu · 2022-10-14T08:11:26Z

htlcswitch/switch_test.go

@@ -3754,9 +3754,22 @@ func assertOutgoingLinkReceiveIntercepted(t *testing.T,
 	}
 }

-func TestSwitchHoldForward(t *testing.T) {
-	t.Parallel()
+type interceptableSwitchTestContext struct {


don't quite get the idea behind this test context, looks like in the end it will hold everything.

The goal is to make the test context and methods operating within the context re-usable for the unit test that is added in the next commit.

htlcswitch/held_htlc_set.go

yyforyongyu · 2022-10-14T08:36:16Z

lncfg/config.go

+	// be greater than CltvRejectDelta, because we don't want to offer htlcs
+	// to the interceptor client for which there is no time left to resolve
+	// them anymore.
+	DefaultCltvInterceptDelta = DefaultFinalCltvRejectDelta + 3


What's the reason behind 3?

Looked like a reasonable interval for an htlc interceptor application to respond. Suppose there would be three blocks in quick succession - 10 sec/block - the application still has 30 seconds to process the intercept.

yyforyongyu

LGTM🎉 Thanks for adding the tests!

htlcswitch/held_htlc_set_test.go

htlcswitch/switch_test.go

guggero · 2022-10-18T14:35:44Z

GitHub shows a conflict, but doesn't say in which file... Can you try rebasing please?

Preparation for adding more config options.

Preparation for making the interceptable switch aware of expiring htlcs.

Isolation of the set logic so that it will be easier to add watchdog functionality later.

Refactor to prepare for adding more tests.

Prepares for parameter validation.

joostjager · 2022-10-18T16:06:05Z

Done. Problem was the release notes.

Make the interceptable switch aware of htlc expiry and fail back htlcs in-time to prevent the counterparty from force-closing the channel.

saubyk added the interceptors label Aug 15, 2022

joostjager force-pushed the interceptor-watchdog branch 3 times, most recently from 65b783c to 58317fe Compare August 16, 2022 12:35

joostjager marked this pull request as ready for review August 16, 2022 12:35

joostjager force-pushed the interceptor-watchdog branch from 58317fe to 6c9c4c0 Compare August 16, 2022 12:37

joostjager requested a review from Crypt-iQ August 16, 2022 13:12

joostjager force-pushed the interceptor-watchdog branch from 6c9c4c0 to 04131bd Compare August 16, 2022 13:16

joostjager requested a review from carlaKC August 16, 2022 13:17

Crypt-iQ reviewed Aug 16, 2022

View reviewed changes

lncfg/config.go Outdated Show resolved Hide resolved

Roasbeef added this to the v0.16.0 milestone Aug 16, 2022

Roasbeef added the enhancement Improvements to existing features / behaviour label Aug 16, 2022

joostjager force-pushed the interceptor-watchdog branch from 04131bd to 7c8035a Compare August 17, 2022 11:37

joostjager requested a review from Crypt-iQ August 17, 2022 11:38

joostjager force-pushed the interceptor-watchdog branch 3 times, most recently from 7547a6f to 1361a5b Compare August 17, 2022 12:12

Crypt-iQ approved these changes Aug 17, 2022

View reviewed changes

carlaKC reviewed Aug 18, 2022

View reviewed changes

htlcswitch/interceptable_switch.go Outdated Show resolved Hide resolved

htlcswitch/interceptable_switch.go Show resolved Hide resolved

htlcswitch/held_htlc_set.go Outdated Show resolved Hide resolved

joostjager force-pushed the interceptor-watchdog branch from 1361a5b to 0183b31 Compare August 22, 2022 08:36

joostjager requested a review from carlaKC August 22, 2022 14:32

joostjager force-pushed the interceptor-watchdog branch 2 times, most recently from 979c782 to 4c39d78 Compare August 30, 2022 07:44

carlaKC approved these changes Sep 5, 2022

View reviewed changes

htlcswitch/held_htlc_set.go Outdated Show resolved Hide resolved

joostjager force-pushed the interceptor-watchdog branch from 4c39d78 to 2aa7095 Compare September 6, 2022 07:03

joostjager mentioned this pull request Sep 7, 2022

htlcswitch: add always-required interceptor mode #6896

Open

Crypt-iQ approved these changes Sep 29, 2022

View reviewed changes

Crypt-iQ requested a review from yyforyongyu September 29, 2022 22:19

joostjager force-pushed the interceptor-watchdog branch from 2aa7095 to 2be79bc Compare October 7, 2022 07:15

yyforyongyu requested changes Oct 14, 2022

View reviewed changes

joostjager force-pushed the interceptor-watchdog branch 6 times, most recently from d17f1d0 to c1007cc Compare October 14, 2022 14:14

joostjager requested a review from yyforyongyu October 14, 2022 15:43

yyforyongyu approved these changes Oct 18, 2022

View reviewed changes

htlcswitch/held_htlc_set_test.go Show resolved Hide resolved

htlcswitch/switch_test.go Outdated Show resolved Hide resolved

joostjager force-pushed the interceptor-watchdog branch from c1007cc to 156918d Compare October 18, 2022 13:46

joostjager added 5 commits October 18, 2022 18:04

htlcswitch: add InterceptableSwitchConfig

4a3e90f

Preparation for adding more config options.

htlcswitch: add dedicated block subscription to interceptable switch

a6df956

Preparation for making the interceptable switch aware of expiring htlcs.

htlcswitch: add heldHtlcSet

9c063db

Isolation of the set logic so that it will be easier to add watchdog functionality later.

htlcswitch/test: create interceptableSwitchTestContext

93a7cab

Refactor to prepare for adding more tests.

htlcswitch: add error return value to NewInterceptableSwitch

74eeb95

Prepares for parameter validation.

joostjager force-pushed the interceptor-watchdog branch from 156918d to b9dd50e Compare October 18, 2022 16:05

htlcswitch: auto-fail held htlcs

a0a50fa

Make the interceptable switch aware of htlc expiry and fail back htlcs in-time to prevent the counterparty from force-closing the channel.

joostjager force-pushed the interceptor-watchdog branch from b9dd50e to a0a50fa Compare October 18, 2022 16:06

guggero merged commit 7274cf4 into lightningnetwork:master Oct 18, 2022

antonilol mentioned this pull request Oct 21, 2022

[feature]: configure interceptdelta in lnd.conf (#6831) #7068

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

htlcswitch: interceptor watchdog #6831

htlcswitch: interceptor watchdog #6831

joostjager commented Aug 15, 2022

Crypt-iQ left a comment

carlaKC left a comment

joostjager commented Sep 1, 2022

carlaKC left a comment

joostjager commented Sep 6, 2022

Crypt-iQ left a comment

Roasbeef commented Oct 7, 2022

joostjager commented Oct 7, 2022

lightninglabs-deploy commented Oct 14, 2022

yyforyongyu left a comment

yyforyongyu Oct 14, 2022

joostjager Oct 14, 2022

yyforyongyu Oct 14, 2022

joostjager Oct 14, 2022 •

edited

Loading

yyforyongyu left a comment

guggero commented Oct 18, 2022

joostjager commented Oct 18, 2022

htlcswitch: interceptor watchdog #6831

htlcswitch: interceptor watchdog #6831

Conversation

joostjager commented Aug 15, 2022

Crypt-iQ left a comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

joostjager commented Sep 1, 2022

carlaKC left a comment

Choose a reason for hiding this comment

joostjager commented Sep 6, 2022

Crypt-iQ left a comment

Choose a reason for hiding this comment

Roasbeef commented Oct 7, 2022

joostjager commented Oct 7, 2022

lightninglabs-deploy commented Oct 14, 2022

yyforyongyu left a comment

Choose a reason for hiding this comment

yyforyongyu Oct 14, 2022

Choose a reason for hiding this comment

joostjager Oct 14, 2022

Choose a reason for hiding this comment

yyforyongyu Oct 14, 2022

Choose a reason for hiding this comment

joostjager Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

guggero commented Oct 18, 2022

joostjager commented Oct 18, 2022

joostjager Oct 14, 2022 •

edited

Loading