(2/3) Add Enum for HTLCHandlingFailed Reasons #3601

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

This PR adds an enum to represent all possible BOLT 04 error codes, along with some variants that surface additional information that will be useful to the end user.

Depends on #3699
API changes in #3700

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.

lightning/src/ln/onion_utils.rs

lightning/src/events/mod.rs

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?

lightning/src/ln/onion_utils.rs

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?

Outbound payments failure reasons are already reasonably covered byPaymentFailureReason IMO so going to leave as-is for now, especially because the original sender would only get the (less interesting) subset of variants that map directly to bolt 04 codes.

lightning/src/ln/onion_utils.rs

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

lightning/src/ln/onion_utils.rs

carlaKC · 2025-03-31T13:20:03Z

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

IIUC explicit discriminants require a unique value per variant, so they won't work here because we're mapping many variants to one error code.

joostjager

I think - but not sure - that I would have preferred a PR that is just a mechanical substitution of all the u16s for enum values, and inside that a clear separation between code that is only literal search/replace and the rest. Maybe I am overcautious with this change set though.

Then talk about the rest in one or more follow ups.

lightning/src/ln/onion_payment.rs

lightning/src/ln/channelmanager.rs

joostjager · 2025-04-11T09:22:38Z

lightning/src/ln/onion_utils.rs

+
+impl Into<LocalHTLCFailureReason> for u16 {
+	fn into(self) -> LocalHTLCFailureReason {
+		if self == (NODE | 2) {


I had to think of a map here for readability, but no idea of the implications of that and whether it is worth it.

I tried something like this here. The downside of a map is that you can have a runtime failure to find the code you're looking for if you forget to add something to the map; I felt that the compiler check that match has all the variants was more compelling than the readability improvement.

Explicit discriminants were also suggested, but they require a unique value per variant.

Could also have this be something like this, which has the benefit of only defining the values once:

if self == LocalHTLCFailureReason::TemporaryNodeFailure.failure_code() { LocalHTLCFailureReason::TemporaryNodeFailure }

Looks a little ungodly to me, but happy to consider something different here (I don't love it either).

Could be:

macro_rules! impl_into_u16_for_local_failure { ($($variant:path),+ $(,)?) => { impl Into<LocalHTLCFailureReason> for u16 { fn into(self) -> LocalHTLCFailureReason { $( if self == $variant.failure_code() { return $variant; } )+ LocalHTLCFailureReason::UnknownFailureCode { code: self } } } }; } impl_into_u16_for_local_failure!( LocalHTLCFailureReason::TemporaryNodeFailure, LocalHTLCFailureReason::PermanentNodeFailure, ... all bolt 04 variants )

Not necessarily advocating this, but you could use the macro to define the enum as well. This would ensure each one is used, whereas otherwise impl_into_u16_for_local_failure needs to be updated whenever a new variant is added.

For some creative uses of macros see:

https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-types/src/features.rs

https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-custom-message/src/lib.rs

rust-lightning/lightning/src/util/ser_macros.rs

Line 1050 in 83e9e80

macro_rules! tlv_stream {

For instance, you could use a pared down version of composite_custom_message_handler's rule to specify each variant as something like:

macro_rules! failure_reason { ($($variant:ident($code:expr)),* $(,)*) => { // ... } } failure_reason!( TemporaryNodeFailure(UPDATE | 7), // ... )

Adding docs may not be as nice. We do something for features like this:

rust-lightning/lightning-types/src/features.rs

Line 257 in 83e9e80

#[doc = $doc]

Ok, if you all start threatening with macros, I'll back off 😅

lightning/src/ln/onion_utils.rs

joostjager · 2025-04-11T11:02:03Z

lightning/src/ln/async_payments_tests.rs

@@ -20,7 +20,7 @@ use crate::ln::msgs::{
 	BaseMessageHandler, ChannelMessageHandler, MessageSendEvent, OnionMessageHandler,
 };
 use crate::ln::offers_tests;
-use crate::ln::onion_utils::INVALID_ONION_BLINDING;
+use crate::ln::onion_utils::LocalHTLCFailureReason;


This commit is just so intense. Getting nervous of reviewing so many trivial changes. I wonder if it is worth to:

Do the renames from err_code to reason separately in a non-review commit

Introduce the LocalHTLCFailureReason enum, still unused

Have a commit that contains just mechanical search/replaces that needs no review. Possibly list all the replaces that were applied.

Then finish with a commit with the semi-manual leftovers.

Maybe it's too much of an ask though.

I'll take a shot at splitting this up 👍

It is incredible how much more reviewable this change makes your PR. Thanks so much! The anxiety is gone 😅

lightning/src/ln/onion_utils.rs

joostjager · 2025-04-11T11:23:58Z

lightning/src/ln/onion_utils.rs

+			| Self::DustLimitHolder
+			| Self::DustLimitCounterparty
+			| Self::FeeSpikeBuffer
+			| Self::ChannelNotReady => UPDATE | 7,


This seems to be a fundamental design choice. To allow multiple reasons to map to the same bolt code. Somehow it suggests a layering violation, but perhaps this is just the practical way to do it.

It does make this commit more than the basic 'replace u16 with enum'.

This seems to be a fundamental design choice. To allow multiple reasons to map to the same bolt code.

Indeed, this is one variant because it allows us to surface this enum directly on the API. Without that, we'd need some sort of wrapper enum which allows either a direct BOLT 04 mapping or an "enriched" failure that then maps to the BOLT04 mapping.

enum Failure { Bolt04(Bolt04Failure) Enriched(LocalFailure) }

Since we want to surface additional information about the errors, we have to accept that this many to one mapping has to happen somewhere - it either happens in one enum, or across two.

The above Failure structure also doesn't really make sense to surface to the end user, so we'd then need to map this further to a flat enum, which adds a lot of boiler plate.

Somehow it suggests a layering violation, but perhaps this is just the practical way to do it.

While it does feel a bit like a layering violation because these values end up on the API, it is also just a more accurate way of representing internal error states. The BOLT04 error codes erase a lot of information that's internally available, so perhaps the actual layering violation is to use wire-level error codes internally? For example, this change also gives us more precise unit testing because we can now assert that the TemporaryChannelFailure we're receiving is actually the one we're expecting.

The BOLT04 error codes erase a lot of information that's internally available, so perhaps the actual layering violation is to use wire-level error codes internally?

I love this inversion. Sold.

lightning/src/ln/onion_utils.rs

jkczyz · 2025-04-11T14:02:12Z

lightning/src/ln/onion_utils.rs

+	}
+}
+
+impl Into<LocalHTLCFailureReason> for u16 {


Prefer From over Into as per the Into docs: https://doc.rust-lang.org/std/convert/trait.Into.html

jkczyz · 2025-04-11T14:07:31Z

lightning/src/ln/onion_utils.rs

+
+impl Into<LocalHTLCFailureReason> for u16 {
+	fn into(self) -> LocalHTLCFailureReason {
+		if self == (NODE | 2) {


Not necessarily advocating this, but you could use the macro to define the enum as well. This would ensure each one is used, whereas otherwise impl_into_u16_for_local_failure needs to be updated whenever a new variant is added.

For some creative uses of macros see:

https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-types/src/features.rs

https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-custom-message/src/lib.rs

rust-lightning/lightning/src/util/ser_macros.rs

Line 1050 in 83e9e80

macro_rules! tlv_stream {

For instance, you could use a pared down version of composite_custom_message_handler's rule to specify each variant as something like:

macro_rules! failure_reason { ($($variant:ident($code:expr)),* $(,)*) => { // ... } } failure_reason!( TemporaryNodeFailure(UPDATE | 7), // ... )

Adding docs may not be as nice. We do something for features like this:

rust-lightning/lightning-types/src/features.rs

Line 257 in 83e9e80

#[doc = $doc]

carlaKC · 2025-04-11T21:44:58Z

Broke the large commit up into smaller ones, no changes in logic except for implementing From instead of Into as suggested.

lightning/src/ln/onion_utils.rs

joostjager · 2025-04-14T08:25:57Z

lightning/src/ln/async_payments_tests.rs

@@ -20,7 +20,7 @@ use crate::ln::msgs::{
 	BaseMessageHandler, ChannelMessageHandler, MessageSendEvent, OnionMessageHandler,
 };
 use crate::ln::offers_tests;
-use crate::ln::onion_utils::INVALID_ONION_BLINDING;
+use crate::ln::onion_utils::LocalHTLCFailureReason;


It is incredible how much more reviewable this change makes your PR. Thanks so much! The anxiety is gone 😅

joostjager

Only non-blocking comments. Ready to approve when you consider the PR done.

lightning/src/ln/onion_payment.rs

joostjager · 2025-04-15T07:10:19Z

lightning/src/ln/onion_utils.rs

+	}
+}
+
+impl From<u16> for LocalHTLCFailureReason {


I had to think of implementing the inverse instead of failure_code() to have some automatic conversion.

joostjager · 2025-04-15T07:13:12Z

lightning/src/ln/onion_utils.rs

-			HTLCFailReasonRepr::Reason { ref failure_code, .. } => {
-				write!(f, "HTLC error code {}", failure_code)
+			HTLCFailReasonRepr::Reason { ref failure_reason, .. } => {
+				write!(f, "HTLC error code {}", failure_reason.failure_code())


Also print the internal code? Maybe full internal reason with code in parenthesis? Or a Debug impl on LocalHTLCFailureReason doing this.

joostjager · 2025-04-15T07:18:34Z

lightning/src/ln/onion_utils.rs

-		(0, failure_code, required),
+		(0, _failure_code, (legacy, u16,
+			|r: &HTLCFailReasonRepr| match r {
+				HTLCFailReasonRepr::LightningError{ .. } => None,


It surprised me that LightningError needs to be matched again here, as it seemed at this point we are already in the Reason scope. But that's just my lack of understanding of these macros I believe.

joostjager · 2025-04-15T07:31:14Z

lightning/src/ln/onion_utils.rs

 			Self::InvalidOnionVersion => BADONION | PERM | 4,
 			Self::InvalidOnionHMAC => BADONION | PERM | 5,
 			Self::InvalidOnionKey => BADONION | PERM | 6,
-			Self::TemporaryChannelFailure => UPDATE | 7,
-			Self::PermanentChannelFailure => PERM | 8,
+			Self::TemporaryChannelFailure


Could it now be the case that with the specialization to local codes, that some of these values are now only used when received from downstream, but never produced locally (because there's a more specialized variant available)? Perhaps the generic ones can even be removed in some cases?

Passing around the values, logging, converting into a bolt code is all good, but when actually doing something with the value, maybe care needs to be taken that all the variants are matched on now?

The most important area where this happens is of course the sender interpreting the failure code, although there it's all u16 based. On the one hand, you'd think, why not use the strong type there too? But on the other hand that could lead to match arms on internal-only codes that are never received. Maybe still a slight bit of that friction left between internal and bolt codes?

Wondering if there are other areas that had some kind of if statement that now doesn't cover the new variants while it should. Or is this an unfounded concern?

Thoughts inline, but I think at a high level there's just always going to be some tension having one enum that's used for internal/api/bolt04 which we'll need to live with.

Could it now be the case that with the specialization to local codes, that some of these values are now only used when received from downstream, but never produced locally (because there's a more specialized variant available)?

Yes, this is the case for PermanentNodeFailure, for example. But I think that the readability improvement of having this value in tests and future proofing of being able to use a readable value over a u16 is worth it (this PR series fixed 3x cases of incorrect u16 being used, for example).

Perhaps the generic ones can even be removed in some cases?

If we do this, then BOLT codes received from downstream malformed will all be UnknownFailureCode which I think is ugly to surface on the API.

Wondering if there are other areas that had some kind of if statement that now doesn't cover the new variants while it should. Or is this an unfounded concern?

Meaning, for example (?):

Code that wants to match on aTemporaryChannelFailure

It gets HTLCMaximum and doesn't understand that translated to TemporaryChannelFailure?

We would have run into that in this PR replacing all the codes (like is done with is_temporary()), but certainly something that will need to be handled if we have a future case for matching on a specific BOLT04 variant of the enum.

I didn't think of downstream malformed. But indeed, becomes ugly when removing those bolt codes.

Meaning, for example

Indeed, something like that. Seems also unlikely to happen, because it looks like things are trending towards less specialized handling of failures to prevent routing nodes from gaming the scores. When processing the failure message at the sender, matching happens on pretty broad failure groups.

Perhaps there's a future where the actual code is simply ignored, because it isn't reliable anymore.

But all good then.

joostjager · 2025-04-15T07:35:13Z

lightning/src/ln/channelmanager.rs

-						reason: failure.data,
-						attribution_data: failure.attribution_data,
-					}));
+	fn construct_pending_htlc_fail_msg<'a>(


One less macro, no objections.

joostjager · 2025-04-15T07:39:44Z

lightning/src/ln/channelmanager.rs

 						let htlc_destination = get_failed_htlc_destination(outgoing_scid_opt, update_add_htlc.payment_hash);
+						let htlc_fail = self.construct_pending_htlc_fail_msg(&update_add_htlc, &incoming_counterparty_node_id, shared_secret, inbound_err);


Nice refactor / simplification.

joostjager · 2025-04-15T08:03:20Z

lightning/src/ln/channel.rs

 	fn send_htlc<F: Deref, L: Deref>(
 		&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource,
 		onion_routing_packet: msgs::OnionPacket, mut force_holding_cell: bool,
 		skimmed_fee_msat: Option<u64>, blinding_point: Option<PublicKey>,
 		fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
-	) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError>
+	) -> Result<Option<msgs::UpdateAddHTLC>, (LocalHTLCFailureReason, String)>


Having detailed reasons now, but still needing a message - isn't that a bit too much now? Could add those additional text fields to the LocalHTLCFailureReason enum perhaps.

Will pick up in a follow up 👍

valentinewallace

Echoing Joost, ready to ACK when you are!

lightning/src/ln/channelmanager.rs

valentinewallace · 2025-04-14T16:40:13Z

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,


nit: s/either//

Per: https://github.com/lightning/blips/blob/master/blip-0003.md

Introduce an enum representation of BOLT04 error codes that will be introduced in the commits that follow, along with being extended to represent failures that are otherwise erased by BOLT04 error codes (insufficient liquidity which is erased by temporary channel failure, for example).

Now that we're passing around more than a BOLT04 error code, rename the field accordingly.

Now that we're not passing BOLT04 error codes around, rename the field accordingly.

Now that we're not passing around error codes, update variable names accordingly.

Update the failure reason that's persisted in HTLCFailReasonRepr to store our newly added enum, rather than the old u16. In the commits that follow, we'll add more variants create a many-to-one mapping from enum to BOLT04 code (as we're surfacing internal information that is intentionally erased by the BOLT04 codes). Without this change to persistence, we'll lose the detail that these variants contain by storing them as their erased BOLT04 code.

Sometimes the error codes that we return to the sender intentionally obscure information about the payment to prevent probing/ leaking information. This commit updates our internal representation to surface some of these failures, which will be converted to their corresponding bolt 04 codes when they're sent over the wire.

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 · 2025-04-15T18:53:05Z

Rebased, squashed + addressed last outstanding nits!

valentinewallace · 2025-04-15T19:23:01Z

One favor -- we generally try to push the rebase on main separately to make it easier to see the "real" diff since the previous push :) Or post the diff in a comment.

joostjager

Final-checked the nits through the rebase, looks good! Thorough, thoughtful work, as always. Thanks!

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

Closed

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

Closed

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

lightning/src/ln/onion_utils.rs Show resolved Hide resolved

joostjager reviewed Mar 17, 2025

View reviewed changes

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

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

lightning/src/ln/onion_utils.rs Show resolved Hide resolved

carlaKC mentioned this pull request Mar 31, 2025

Handle receiving payments via Trampoline #3670

Merged

carlaKC force-pushed the 3541-failure-reason branch from bb45736 to ad60f71 Compare April 2, 2025 20:26

This was referenced Apr 2, 2025

(1/3) Minor error code fixes #3699

Merged

(3/3) Add Failure Reason to HTLCHandlingFailed #3700

Open

TheBlueMatt requested a review from joostjager April 10, 2025 18:39

joostjager reviewed Apr 11, 2025

View reviewed changes

jkczyz reviewed Apr 11, 2025

View reviewed changes

carlaKC force-pushed the 3541-failure-reason branch from 147072f to 125fb63 Compare April 11, 2025 21:43

joostjager reviewed Apr 14, 2025

View reviewed changes

carlaKC requested a review from joostjager April 14, 2025 18:39

joostjager reviewed Apr 15, 2025

View reviewed changes

valentinewallace self-requested a review April 15, 2025 14:49

valentinewallace reviewed Apr 15, 2025

View reviewed changes

carlaKC added 11 commits April 15, 2025 13:39

ln/refactor: move incorrect payment data write to helper function

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

fcb2902

ln/fix: return incorrect payment details for invalid keysend

b76dbdd

Per: https://github.com/lightning/blips/blob/master/blip-0003.md

ln/refactor: replace u16 failure codes with BOLT04 failure codes

980aab0

ln/refactor: rename InboundHTLCErr err_code/reason

8e47178

Now that we're passing around more than a BOLT04 error code, rename the field accordingly.

ln/refactor: rename OnionDecodeErr err_code to reason

daae398

Now that we're not passing BOLT04 error codes around, rename the field accordingly.

ln/refactor: rename misc error_code related vars in channelmanager

6f442d9

Now that we're not passing around error codes, update variable names accordingly.

ln/refactor: split up construct_pending_htlc_status to get error

5179d8a

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.

ln: surface LocalHTLCFailureReason in queue_add_htlc

Loading
Loading status checks…

f839b92

carlaKC force-pushed the 3541-failure-reason branch from 15ff2c4 to f839b92 Compare April 15, 2025 18:51

carlaKC requested review from joostjager and valentinewallace April 15, 2025 19:18

valentinewallace approved these changes Apr 15, 2025

View reviewed changes

joostjager approved these changes Apr 15, 2025

View reviewed changes

valentinewallace merged commit 0ee1def into lightningdevkit:main Apr 16, 2025
26 of 27 checks passed

carlaKC mentioned this pull request Apr 17, 2025

Use LocalHTLCFailureReason in Onion Processing #3744

Open

		let htlc_destination = get_failed_htlc_destination(outgoing_scid_opt, update_add_htlc.payment_hash);
		let htlc_fail = self.construct_pending_htlc_fail_msg(&update_add_htlc, &incoming_counterparty_node_id, shared_secret, inbound_err);

(2/3) Add Enum for HTLCHandlingFailed Reasons #3601

(2/3) Add Enum for HTLCHandlingFailed Reasons #3601

Conversation

carlaKC commented Feb 13, 2025 • edited Loading

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 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

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

carlaKC commented Mar 31, 2025

joostjager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC Apr 11, 2025 • edited Loading

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 Apr 11, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Apr 11, 2025

Choose a reason for hiding this comment

joostjager 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

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

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Apr 15, 2025

valentinewallace commented Apr 15, 2025

joostjager left a comment

Choose a reason for hiding this comment

carlaKC commented Feb 13, 2025 •

edited

Loading

joostjager Mar 17, 2025 •

edited

Loading

valentinewallace commented Mar 12, 2025 •

edited

Loading

carlaKC Apr 11, 2025 •

edited

Loading

joostjager Apr 11, 2025 •

edited

Loading