Add a reason to HTLCHandlingFailed event #3601

carlaKC · 2025-02-13T14:44:57Z

This PR adds a HTLCHandlingFailure reason to HTLCHandlingFailed to fix #3541.

A simpler version of this would be to make HTLCFailReason public and just surface that on the API. Decided against this approach because it exposes not-so-user-friendly types on the API with types like OnionErrorPacket, but happy to go with that way if it's preferred!

TheBlueMatt · 2025-02-13T15:39:27Z

lightning/src/events/mod.rs

+	/// The HTLC was failed by the local node.
+	Local {
+		/// The BOLT 04 failure code providing a specific failure reason.
+		failure_code: u16


I admit I'm not super excited about exposing the failure code as-is for two reasons - (a) we have very different constraints on the code - we may not always want to give the sender all the details about a failure (eg if the sender used a private channel but we didn't mean to expose it we might pretend the channel doesnt exist) vs what we want to tell the user about a failure (basically everything), (b) its not particularly easy to parse - users have to go read the BOLT then map that to what is actually happening in LDK code, if we had a big enum here we'd be able to provide good docs linking to config knobs and reasons why failures happened.

Makes sense, will take a look at adding a more detailed enum or adding to HTLCDestination 👍

we may not always want to give the sender all the details about a failure (eg if the sender used a private channel but we didn't mean to expose it we might pretend the channel doesnt exist)

Maybe I don't understand this fully, but why would you want to hide information from a local api user?

The private channel example is a case where the BOLT 04 code hides information from the sender of the payment, but we do want to surface that information on the API (comment is from a previous approach that just surfaced the BOLT 04 failure code, so wouldn't provide this additional information).

why would you want to hide information from a local api user?

I do think there are some cases where the end user probably doesn't care about very detailed errors. For example, InvalidOnionPayload - do we really care whether the version is unknown or the onion key is bad when there's nothing that we can do about it?

I see, outdated.

About your second remark - the end user is a user of the library, I assume. Maybe hard to know what they care about. But still it's probably more efficient to wait for someone asking for it than it is to add it potentially unnecessarily. In that context interested to know whether the failure code itself is useful? (posted #3541 (comment))

About your second remark - the end user is a user of the library, I assume.

Yeah, whoever is consuming LDK's events 👍

But still it's probably more efficient to wait for someone asking for it than it is to add it potentially unnecessarily.

Complaint driven development FTW 😅

In that context interested to know whether the failure code itself is useful?

As discussed on call, this does fall into the nice to have category. I do also think it's also a nice readability improvement to get rid of the failure codes scattered through the codebase, reduces the chances of using the wrong values IMO.

TheBlueMatt · 2025-02-13T15:41:22Z

lightning/src/events/mod.rs

@@ -1441,6 +1461,10 @@ pub enum Event {
 		prev_channel_id: ChannelId,
 		/// Destination of the HTLC that failed to be processed.
 		failed_next_destination: HTLCDestination,
+		/// The cause of the processing failure.


Its worth noting that HTLCDestination has some "reason"-like attributes (I meant to mention this in the issue, but apparently forgot), they're just incomplete and too broad. Up to you if you want to try to add more details there vs adding a new enum.

carlaKC · 2025-02-21T22:31:04Z

I'm going to be out on vacation until 3 March, but will pick this up when I get back!

carlaKC · 2025-03-11T19:19:40Z

Pushing* for a conceptual look because I haven't found a way of doing this that I'm super happy with.
Specifically interested in thoughts on:

Reason in HTLCDestination: already contains information about the failure, so including a reason in HTLCHandlingFailed would be repetitive for cases like UnknownNextHop.
Surfacing a B04 error code: an alternative approach would be to map all these codes that we don't really care about to another user-friendly enum that summarizes them (eg, InvalidPayload covering blinding/hmac/etc)**. Combining the two gets a bit ugly, and I think that it's reasonably usable to have the human readable value for most common errors and fall back to the code for the more obscure/detailed ones.

If this looks okay, I'll go ahead and clean up tests+docs and add a similar for HTLCDestination::FailedPayment!

* Force pushed because this is a different approach; old version that just surfaces B04 failure codes is here.
** We can't put them in LocalHTLCFailureReason because they erase the actual B04 code, so we'd need to track the summary error and the B04 code together (which is doable, but ugly to have API-facing values being carted around internally).

TheBlueMatt · 2025-03-12T15:30:57Z

lightning/src/ln/onion_utils.rs

+	/// The raw BOLT04 failure code for the HTLC.
+	///
+	/// See: https://github.com/lightning/bolts/blob/master/04-onion-routing.md#returning-errors.
+	FailureCode { code: u16 },


Hmmm, rather than having a variant that just holds the error code (and exposing it), can we just convert every use of error codes to the enum version and then only write it out as a u16 when we go to serialize the failure? This would also have the nice side-effect of ensuring that we have all the error codes we generate written in one place instead of strewn all over the codebase.

Maybe I'm not quite sure I understand why you needed to do this?

an we just convert every use of error codes to the enum version and then only write it out as a u16 when we go to serialize the failure

Yeah can def do that. I thought that some of the error codes were overly-specific (so not something that we want to expose to the end user), but if that's okay then it's a much simpler approach.

valentinewallace · 2025-03-12T18:48:42Z

Played with this a bit locally out of fear of sending you down a dead end... It looks like it's possible to avoid the nested enums and have one big HTLCFailureReason enum in the event, which seems like it might be a better API? Could you check out this commit and let me know your thoughts: 9f9d532 I didn't fix serialization yet but it seems to otherwise compile. We may also want to replace the ::PaymentFailed variant with more specific variants, though that may be out of scope.

carlaKC · 2025-03-14T20:23:27Z

lightning/src/ln/onion_utils.rs

+}
+
+impl LocalHTLCFailureReason {
+	pub(super) fn failure_code(&self) -> u16 {


these methods are quite verbose, and it's annoying to have to specify the values for each enum variant twice (failure_code and u16.into())

could try to do something smarter here, but decided to keep it simple - interested in other opinions here!

Could we assign the value when defining the enum using explicit discriminants?

https://doc.rust-lang.org/reference/items/enumerations.html#r-items.enum.unit-only
https://doc.rust-lang.org/reference/items/enumerations.html#r-items.enum.discriminant.explicit.unit-only

lightning/src/events/mod.rs

This failure code isn't used anywhere in the codebase and is not defined in BOLT 04.

Realm is no longer specified in BOLT04, use the specified version error instead.

ln: persist failure_reason with HTLCFailureReason

To be able to obtain the underlying error reason for the pending HTLC, break up the helper method into two parts. This also removes some unnecessary wrapping/unwrapping of messages in PendingHTLCStatus types.

In upcoming commits, we'll make stronger assertions about the type of failure that we're getting when we set fail_backwards. This will be different depending on the type of route we're using - blinded routes fail malformed onion blinded and regular routes just see a downstream failure. This refactor prepares for adding those assertions by threading a failure reason through. The commitment_signed_dance macro is not updated because it already has an invocation with 5 parameters.

Surface the reason for HTLC failure for forwards. Additional information is unlikely to be useful for InvalidOnion and UnknownNextHop, and adding information to FailedPayment is left for future PRs to keep the scope of this PR down.

carlaKC · 2025-03-21T15:48:39Z

Rebased because there were some non-trivial conflicts, and added tests + docs.

In an effort to keep the scope down, decided not to:

Add a reason to HTLCDestination::FailedPayment: to keep LOC change down.
Refactor queue_add_htlcs to surface LocalHTLCFailureReason because it gets into the guts of channel.
Replace msgs::FailMalformedHTLC's failure_code with LocalHTLCFailureReason: change sprawls a bit.

Of all of these, the most compelling to change in this PR is queue_add_htlcs, because it'll surface liquidity-related failures which are usually interesting to end users. Happy to pull any of these into this PR, or pick them up in follow ups if there's interest!

valentinewallace · 2025-03-25T17:19:28Z

In terms of the API for the event, it looks like it would be best to refactor HTLCDestination to just contain a million variants rather than put those million variants nested in an enum within an enum. This makes sense because the current variants are pretty vague/not good, like FailedPayment. (Thanks @jkczyz who I consulted on this.)

This is a pretty significant refactor though so I agree it would be best to split out some preliminary changes into an initial PR. Do you have any instincts for what would be good to start with? Maybe we could start by just returning the big enum from the internal channel methods, or split out the PR's current changes that replace the onion failure code with this enum?

valentinewallace · 2025-03-25T17:19:50Z

Serialization is also a question here, but I think we can do something similar to what we do for the legacy_failure_reason in the PaymentFailed event.

wvanlint · 2025-03-25T22:08:24Z

Excited for this change! This will help a lot with observability into forwarding failures.

queue_add_htlc

Definitely interested in exposing the cases inside queue_add_htlc, will they otherwise end up as TemporaryChannelFailure?

If this change will expose local failures when forwarding HTLCs, is there a plan to expose failures received at the origin node later on as well?

wvanlint · 2025-03-25T21:25:11Z

lightning/src/ln/onion_utils.rs

+	DroppedPending,
+	/// The HTLC was failed back because its channel is closed and it has timed out on chain.
+	ChannelClosed,
+	/// UnknownFaliureCode represents BOLT04 failure codes that we are not familiar with. We will


Suggested change

/// UnknownFaliureCode represents BOLT04 failure codes that we are not familiar with. We will

/// UnknownFailureCode represents BOLT04 failure codes that we are not familiar with. We will

wvanlint · 2025-03-25T21:25:31Z

lightning/src/ln/onion_utils.rs

+	/// - We read a deprecated failure code from disk that LDK no longer uses.
+	///
+	/// See <https://github.com/lightning/bolts/blob/master/04-onion-routing.md#returning-errors>
+	/// for latesst defined error codes.


Suggested change

/// for latesst defined error codes.

/// for latest defined error codes.

wvanlint · 2025-03-25T21:30:26Z

lightning/src/ln/onion_utils.rs

+const NODE: u16 = 0x2000;
+const UPDATE: u16 = 0x1000;
+
+/// The reason that a HTLC was failed by the local node. These errors either represent direct,


by the local node

Could this potentially be reused later on when exposing the failure reason on the origin side? If so, should the naming be more generic?

wvanlint · 2025-03-25T21:35:12Z

lightning/src/ln/onion_utils.rs

+	PrivateChannelForward,
+	/// The HTLC was failed because it made a request to forward over the real channel ID of a
+	/// channel that implements `option_scid_alias` which is a privacy feature to prevent the
+	/// real


Suggested change

/// real

/// real channel ID from being known.

carlaKC · 2025-03-26T12:48:42Z

In terms of the API for the event, it looks like it would be best to refactor HTLCDestination to just contain a million variants rather than put those million variants nested in an enum within an enum. This makes sense because the current variants are pretty vague/not good, like FailedPayment. (Thanks @jkczyz who I consulted on this.)

I think that the advantage of the current HTLCDestination types is that they do represent summary of the different "types" of failures. If you're building a dashboard with this API, you can easily get an overview of what's going on just from the 4 variants (vs a large set of HTLCDestination variants, where you'd need to classify each yourself to get a summary view) and the nested enum can give you further detail if you want to build more specific dashboards about your own forwards/receives.

Certainly defer to the opinions of the people who've worked on the API longer, but this approach worked reasonably well with LND/LNDMon.

Do you have any instincts for what would be good to start with?

@wvanlint's suggestion to tackle queue_add_htlc seems like a good start to me, since liquidity related failures are usually what people are most interested in.

Maybe we could start by just returning the big enum from the internal channel methods

Just so I'm clear here, this would mean returning HTLCDestination from methods like queue_add_htlc / send_htlc and leaving bolt 04 error code handling as-is (that doesn't seem to belong in channel.rs).

or split out the PR's current changes that replace the onion failure code with this enum?

With the one large HTLCDestination enum approach, would we want one large failure code enum as well?

This is a pretty significant refactor though so I agree it would be best to split out some preliminary changes into an initial PR.

This is a much larger refactor than I was expecting, so I also want to make sure that it's going to be worth the review time for the project (esp since I'm new here)? My main goal was to learn more about the codebase, which I have, so it's already been worth it for me!

valentinewallace · 2025-03-27T02:41:36Z

Good points there and also appreciate the link to the LND APIs, seeing the example of what works in prod today was helpful!

Will get back in more detail on your comment soon, but FYI I finally took a look at the updates from the last big push and most of the commits here already look great. So would def be happy to start with that, especially since this PR is pretty big so it's nice to split it up anyway. How does it sound to start with landing most of the commits prior to the last one that actually surfaces anything in the public API?

carlaKC · 2025-03-27T12:40:21Z

Good points there and also appreciate the link to the LND APIs, seeing the example of what works in prod today was helpful!

Opinions on API solidified a bit for me after offline discussion, I think that deprecating HTLCDestination like you suggested and splitting up into a "type" and a failure reason could work:

enum HTLCHandlingType {
    Forward { node_id: Option<PublicKey>, channel_id: u64 }, 
    Receive { payment_hash: PaymentHash },
    Invalid { requested_forward_scid: u64 },
}

struct HTLCHandlingFailed {
    failure_type: HTLCHandlingType,
    failure_reason: LocalHTLCFailureReason,
}

We'd be able to map these two fields to HTLCDestination to remain backwards compatible, eg a HTLCHandlingType::Invalid + LocalHTLCFailureReason::UnknownNextPeer = HTLCDestiantion::UnknownNextPeer.

How does it sound to start with landing most of the commits prior to the last one that actually surfaces anything in the public API?

sgtm! Perhaps also adding a refactor of queue_add_htlc if it isn't too many LOC, because that has quite a few interesting failure reasons (will investigate).

valentinewallace · 2025-03-27T13:48:52Z

API looks great! Thanks a lot for the patience on this.

sgtm! Perhaps also adding a refactor of queue_add_htlc if it isn't too many LOC, because that has quite a few interesting failure reasons (will investigate).

Definitely think that could make sense!

valentinewallace · 2025-03-27T14:10:39Z

Just so I'm clear here, this would mean returning HTLCDestination from methods like queue_add_htlc / send_htlc and leaving bolt 04 error code handling as-is (that doesn't seem to belong in channel.rs).

Just to be 100% sure we're on the same page -- I chatted with @TheBlueMatt yesterday and he emphasized that (1) the way the current PR code replaces the BOLT 4 raw error codes with the enum is already a solid improvement to readability, and (2) it's really nice that we only have one enum used for both the event and for onion error codes in LN messages. Mainly pointing this out in case there was any risk of rewriting some of the commits here, since they seem to align with these goals as-is.

TheBlueMatt · 2025-03-27T16:38:16Z

Opinions on API solidified a bit for me after offline discussion, I think that deprecating HTLCDestination like you suggested and splitting up into a "type" and a failure reason could work:

This seems fine to me. The reason HTLCDestination is what it is is that there are various error codes that are not valid for some recipients. However, I don't think trying to capture that here is worth it, since its in direct conflict with trying to use the same enum for all HTLC errors both public and private, and it'd mean having several large error-code-class enums, so its probably best to just do it the way you've suggested.

jkczyz

Concept ACK

jkczyz · 2025-03-27T18:16:47Z

lightning/src/ln/onion_utils.rs

+}
+
+impl LocalHTLCFailureReason {
+	pub(super) fn failure_code(&self) -> u16 {


Could we assign the value when defining the enum using explicit discriminants?

https://doc.rust-lang.org/reference/items/enumerations.html#r-items.enum.unit-only
https://doc.rust-lang.org/reference/items/enumerations.html#r-items.enum.discriminant.explicit.unit-only

TheBlueMatt reviewed Feb 13, 2025

View reviewed changes

TheBlueMatt mentioned this pull request Mar 5, 2025

Add a reason enum to HTLCDestination in some variants #3561

Open

TheBlueMatt linked an issue Mar 5, 2025 that may be closed by this pull request

Add a reason enum to HTLCDestination in some variants #3561

Open

carlaKC force-pushed the 3541-failure-reason branch from 4a14deb to 26326ea Compare March 11, 2025 19:17

ldk-reviews-bot requested a review from valentinewallace March 12, 2025 01:20

TheBlueMatt reviewed Mar 12, 2025

View reviewed changes

carlaKC force-pushed the 3541-failure-reason branch from 26326ea to 0882ed5 Compare March 14, 2025 20:19

carlaKC commented Mar 14, 2025

View reviewed changes

joostjager reviewed Mar 17, 2025

View reviewed changes

lightning/src/events/mod.rs Show resolved Hide resolved

carlaKC added 7 commits March 20, 2025 13:12

ln/fix: remove undefined PERM | 1 code from reason method

a7f0034

This failure code isn't used anywhere in the codebase and is not defined in BOLT 04.

ln/fix: invalid_onion_version code for DecodeError::UnknownVersion

f58c320

Realm is no longer specified in BOLT04, use the specified version error instead.

ln/refactor: introduce enum for bolt 04 failure codes

9961505

ln: persist failure_reason with HTLCFailureReason

7bbb177

ln: persist failure_reason with HTLCFailureReason

ln/refactor: split up construct_pending_htlc_status to get error

ae178c9

To be able to obtain the underlying error reason for the pending HTLC, break up the helper method into two parts. This also removes some unnecessary wrapping/unwrapping of messages in PendingHTLCStatus types.

carlaKC force-pushed the 3541-failure-reason branch from 0882ed5 to bb45736 Compare March 21, 2025 15:47

wvanlint reviewed Mar 25, 2025

View reviewed changes

jkczyz reviewed Mar 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a reason to HTLCHandlingFailed event #3601

Add a reason to HTLCHandlingFailed event #3601

carlaKC commented Feb 13, 2025

TheBlueMatt Feb 13, 2025

carlaKC Feb 13, 2025

joostjager Mar 17, 2025

carlaKC Mar 17, 2025

joostjager Mar 17, 2025 •

edited

Loading

carlaKC Mar 17, 2025

TheBlueMatt Feb 13, 2025

carlaKC commented Feb 21, 2025

carlaKC commented Mar 11, 2025

TheBlueMatt Mar 12, 2025

carlaKC Mar 12, 2025

valentinewallace commented Mar 12, 2025 •

edited

Loading

carlaKC Mar 14, 2025 •

edited

Loading

jkczyz Mar 27, 2025

carlaKC commented Mar 21, 2025

valentinewallace commented Mar 25, 2025

valentinewallace commented Mar 25, 2025

wvanlint commented Mar 25, 2025

wvanlint Mar 25, 2025

wvanlint Mar 25, 2025

wvanlint Mar 25, 2025

wvanlint Mar 25, 2025

carlaKC commented Mar 26, 2025

valentinewallace commented Mar 27, 2025

carlaKC commented Mar 27, 2025

valentinewallace commented Mar 27, 2025

valentinewallace commented Mar 27, 2025

TheBlueMatt commented Mar 27, 2025

jkczyz left a comment

jkczyz Mar 27, 2025

	/// UnknownFaliureCode represents BOLT04 failure codes that we are not familiar with. We will
	/// UnknownFailureCode represents BOLT04 failure codes that we are not familiar with. We will

	/// for latesst defined error codes.
	/// for latest defined error codes.

Add a reason to HTLCHandlingFailed event #3601

Are you sure you want to change the base?

Add a reason to HTLCHandlingFailed event #3601

Conversation

carlaKC commented Feb 13, 2025

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

joostjager Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Feb 21, 2025

carlaKC commented Mar 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Mar 12, 2025 • edited Loading

carlaKC Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Mar 21, 2025

valentinewallace commented Mar 25, 2025

valentinewallace commented Mar 25, 2025

wvanlint commented Mar 25, 2025

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 commented Mar 26, 2025

valentinewallace commented Mar 27, 2025

carlaKC commented Mar 27, 2025

valentinewallace commented Mar 27, 2025

valentinewallace commented Mar 27, 2025

TheBlueMatt commented Mar 27, 2025

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager Mar 17, 2025 •

edited

Loading

valentinewallace commented Mar 12, 2025 •

edited

Loading

carlaKC Mar 14, 2025 •

edited

Loading