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

htlcswitch: add htlc interceptor failure control #6177

Merged
merged 1 commit into from
Feb 2, 2022
Merged

htlcswitch: add htlc interceptor failure control #6177

merged 1 commit into from
Feb 2, 2022

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Jan 18, 2022 •
edited
Loading

This PR extends the HTLC interceptor API so that users can control the exact failure message that is returned to the sender of the payment.

@joostjager joostjager mentioned this pull request Jan 18, 2022
Closed
@alexbosworth
Copy link
Contributor

the failure_reason allows for crafting a custom failure message, like an unknown payment hash that appears to be from the next hop?

@joostjager
Copy link
Contributor Author

Yes that is indeed the use case. The htlc interceptor is basically a 'link-over-grpc'.

@alexbosworth
Copy link
Contributor

Yes that is indeed the use case. The htlc interceptor is basically a 'link-over-grpc'.

That's great, I think this is a superior approach to #4785

@Roasbeef Roasbeef added this to the v0.15.0 milestone Jan 21, 2022
@Roasbeef Roasbeef added interface abstraction invoices rpc Related to the RPC interface labels Jan 21, 2022
champo
champo reviewed Jan 22, 2022
View reviewed changes
Copy link
Contributor

@champo champo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this, makes the interceptor way more useful.

lnrpc/routerrpc/router.proto Outdated
bytes failure_reason = 4;

// Return UpdateFailMalformedHTLC in case the resolve action is Fail.
MalformedHtlcCode failure_malformed_htlc = 5;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you choose to scope this to malformed htlc errors? As written, it would be easy to add other cases like CodeChannelDisabled, CodeTemporaryChannelFailure, CodePermanentChannelFailure which would be useful too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all that I needed for my purpose. It could be generalized, but then also all the failure message parameters need to be marshaled over rpc? Or would you do a hybrid where you only specify the code and lnd tries to fill in the message parameters?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ones I was thinking about are either very similar to existing cases or have no parameters. In any case, none of this needs to be implemented now. I was mostly wondering why the thight scope for the semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked the PR to widen the scope.

@joostjager joostjager force-pushed the interceptor-failures branch 4 times, most recently from c251faa to 7284033 Compare January 24, 2022 08:03
carlaKC
carlaKC approved these changes Jan 24, 2022
View reviewed changes
Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition to the interceptor!

htlcswitch/switch_test.go Outdated Show resolved Hide resolved
htlcswitch/switch_test.go Show resolved Hide resolved
lnrpc/routerrpc/forward_interceptor.go Outdated Show resolved Hide resolved
@carlaKC carlaKC requested a review from bhandras January 24, 2022 09:00
@joostjager joostjager force-pushed the interceptor-failures branch 3 times, most recently from e345400 to 3ba4f33 Compare January 25, 2022 09:19
@lightninglabs-deploy
Copy link

@joostjager, remember to re-request review from reviewers when ready

bhandras
bhandras approved these changes Feb 1, 2022
View reviewed changes
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few nits 👍

htlcswitch/interceptable_switch.go Outdated Show resolved Hide resolved
htlcswitch/interceptable_switch.go Show resolved Hide resolved
htlcswitch/interceptable_switch.go Outdated Show resolved Hide resolved
htlcswitch/interfaces.go Outdated Show resolved Hide resolved
htlcswitch/switch_test.go Outdated Show resolved Hide resolved
htlcswitch/switch_test.go Outdated Show resolved Hide resolved
htlcswitch/switch_test.go Outdated Show resolved Hide resolved
htlcswitch/switch_test.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/forward_interceptor.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the interceptor-failures branch 2 times, most recently from df1dad5 to db41e72 Compare February 1, 2022 16:16
@joostjager joostjager requested review from bhandras and removed request for bhandras February 1, 2022 16:16
Roasbeef
Roasbeef reviewed Feb 2, 2022
View reviewed changes
lnrpc/routerrpc/forward_interceptor.go
code = lnwire.CodeInvalidOnionVersion

// Default to TemporaryChannelFailure.
case 0, lnrpc.Failure_TEMPORARY_CHANNEL_FAILURE:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting way to override the default here programatically...🤔

@Roasbeef Roasbeef merged commit 5a827dd into lightningnetwork:master Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roasbeef Roasbeef Roasbeef left review comments

@champo champo champo approved these changes

@bhandras bhandras bhandras approved these changes

@carlaKC carlaKC carlaKC approved these changes

Assignees
No one assigned
Labels
interface abstraction invoices rpc Related to the RPC interface
Projects
None yet
Milestone
v0.15.0
Development

Successfully merging this pull request may close these issues.
7 participants
@joostjager @alexbosworth @lightninglabs-deploy @champo @bhandras @Roasbeef @carlaKC