Add a reason to HTLCHandlingFailed event #3601
Conversation
lightning/src/events/mod.rs
Outdated
| /// The HTLC was failed by the local node. | ||
| Local { | ||
| /// The BOLT 04 failure code providing a specific failure reason. | ||
| failure_code: u16 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Makes sense, will take a look at adding a more detailed enum or adding to HTLCDestination 👍
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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))
There was a problem hiding this 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.
lightning/src/events/mod.rs
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
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.
|
I'm going to be out on vacation until 3 March, but will pick this up when I get back! |
carlaKC
force-pushed
the
3541-failure-reason
branch
from
4a14deb to
26326ea
Compare
|
Pushing* for a conceptual look because I haven't found a way of doing this that I'm super happy with.
If this looks okay, I'll go ahead and clean up tests+docs and add a similar for * Force pushed because this is a different approach; old version that just surfaces B04 failure codes is here. |
lightning/src/ln/onion_utils.rs
Outdated
| /// 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 }, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
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 |
carlaKC
force-pushed
the
3541-failure-reason
branch
from
26326ea to
0882ed5
Compare
| } | ||
|
|
||
| impl LocalHTLCFailureReason { | ||
| pub(super) fn failure_code(&self) -> u16 { |
There was a problem hiding this comment.
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!
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
force-pushed
the
3541-failure-reason
branch
from
0882ed5 to
bb45736
Compare
|
Rebased because there were some non-trivial conflicts, and added tests + docs. In an effort to keep the scope down, decided not to:
Of all of these, the most compelling to change in this PR is |
This PR adds a
HTLCHandlingFailurereason toHTLCHandlingFailedto fix #3541.A simpler version of this would be to make
HTLCFailReasonpublic and just surface that on the API. Decided against this approach because it exposes not-so-user-friendly types on the API with types likeOnionErrorPacket, but happy to go with that way if it's preferred!