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

carlaKC · 2025-04-02T20:35:45Z

This PR surfaces a failure reason in HTLCHandlingFailed events. Opening up early to add some context to the prefactor PR #3601.

The heart of the PR is in d35d35f, and it could probably be reduced to just this commit. I've made some quite opinionated renaming / deprecation decisions in the other commits which aren't required for this change, but I think make for a more readable API overall - happy to drop them if it ain't broke, don't fix it applies.

Fixes: #3561
Fixes: #3541

ldk-reviews-bot · 2025-04-02T20:35:48Z

👋 Thanks for assigning @joostjager as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

lightning/src/events/mod.rs

codecov · 2025-04-11T21:52:28Z

Codecov Report

Attention: Patch coverage is 86.55462% with 32 lines in your changes missing coverage. Please review.

Project coverage is 89.60%. Comparing base (0ee1def) to head (5badb5e).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/events/mod.rs	10.71%	25 Missing ⚠️
lightning/src/ln/channelmanager.rs	90.24%	4 Missing ⚠️
lightning-liquidity/src/lsps2/service.rs	0.00%	2 Missing ⚠️
lightning/src/ln/monitor_tests.rs	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3700      +/-   ##
==========================================
+ Coverage   89.13%   89.60%   +0.46%     
==========================================
  Files         156      158       +2     
  Lines      123600   127663    +4063     
  Branches   123600   127663    +4063     
==========================================
+ Hits       110176   114390    +4214     
+ Misses      10757    10640     -117     
+ Partials     2667     2633      -34

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

valentinewallace · 2025-04-16T14:32:19Z

Needs rebase 🥳

lightning/src/events/mod.rs

lightning/src/ln/onion_utils.rs

ldk-reviews-bot · 2025-04-21T00:00:38Z

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

carlaKC · 2025-04-21T13:40:48Z

Pushed suggested comment/style changes + rename of HTLCFailReasonRepr to go with outer struct rename.

Happy to drop the last two commits as discussed here if the renames aren't strong enough to justify the change!

valentinewallace · 2025-04-22T22:20:22Z

lightning/src/events/mod.rs

+	Downstream,
+	/// The HTLC was failed locally by our node.
+	Local {
+		/// The reason that our node chose to fail the HTLC.
+		reason: LocalHTLCFailureReason,
+	},


I'm wondering if it would be nicer to avoid this outer enum and just document on the field "Will be None if the HTLC failed downstream or this event was created prior to 0.2." Seems simpler, loses a bit of information for a period of time though.

I think that it would be nice to surface hold_time along with Downstream in future (nice thing to put on a dashboard IMO) so tend towards keeping the enum for the sake of being able to easily add that.

Opinion not very strongly held though.

Oh, I didn't realize that was a potential goal but that makes sense! We may want to document this on the attributable failures issue, if there is one.

valentinewallace · 2025-04-22T22:33:06Z

lightning/src/events/mod.rs

+					// [`LocalHTLCFailureReason::UnknownNextHop`]. This will cover both the case
+					// where we have a legacy event


nit: incomplete sentence

valentinewallace · 2025-04-22T23:14:37Z

lightning/src/events/mod.rs

@@ -480,12 +480,16 @@ pub enum HTLCHandlingType {
 		channel_id: ChannelId,
 	},
 	/// Scenario where we are unsure of the next node to forward the HTLC to.
+	///
+	/// Deprecated: will only be used in versions before LDK v0.2.0.


I think you can use the official deprecated attribute on this: https://doc.rust-lang.org/reference/attributes/diagnostics.html#r-attributes.diagnostics.deprecated

TIL this exists!

Struggling to #[allow(deprecated)] for the impl_writeable_tlv_based_enum_upgradable invocation to silence the cargo warning though..

carlaKC · 2025-04-23T17:36:35Z

Removed the last 2 rename commits (I don't think they're that much of an improvement) + fixed up comment.

Couldn't find a way to use deprecation attribute without making changes to the serialization macro so left it out.

valentinewallace · 2025-04-25T17:34:57Z

lightning/src/events/mod.rs

+		/// The type of HTLC that was handled.
+		handling_type: HTLCHandlingType,
+		/// The reason that the HTLC failed.
+		///
+		/// This field will be `None` only for objects serialized prior to LDK 0.2.0.
+		handling_failure: Option<HTLCHandlingFailureReason>


Consulted @jkczyz our resident naming expert --

He suggested renaming HTLCHandlingType to HTLCHandlingFailureType, and renaming the fields within the event to failure_type and failure_reason. This would also imply possibly renaming the HTLCHandlingFailureType::ReceiveFailed to just ::Receive, similarly for forwards. Reasoning being that the type of HTLC handling is moreso the type of failure, conceptually (Jeff feel free to correct me here).

Thoughts on this naming scheme? cc @joostjager

Sounds like an improvement to me.

🫡 gone ahead and also changed ForwardFailed -> Forward with the same reasoning as ReceiveFailed -> Receive

HTLCDestination currently contains a combination of information about the type of HTLC we handled to fail (a payment, a forward etc) and error information for a select set of cases (unknown next peer, for example). In preparation for a refactor that will split the failure reason out into its own enum, this commit renames HTLCDestination to HTLCHandlingType.

Rename variant to be more specific to the current context - a FailedPayment could be a payment that we failed to dispatch or one that we rejected on receive.

Standardize naming within the HTLCHandlingType struct to present more consistent API terminology.

This variant of HTLCHandlingType contains information about the failure cause along with its type - as an UnknownNextPeer is just an InvalidForward that has the failure type UnknownNextPeer. This commit deprecates the variant's use, while still writing it to disk to allow the option to downgrade. The deprecated attribute is not used because it cannot be silenced in impl_writeable_tlv_based_enum, which uses UnknownNextPeer to remain downgradable.

valentinewallace

Nothing blocking if we want to Just Land and save the ser tweak for follow-up

lightning/src/events/mod.rs

valentinewallace · 2025-04-25T23:35:20Z

lightning/src/events/mod.rs

+				let downgradable_type = match (failure_type, failure_reason) {
+					(HTLCHandlingFailureType::InvalidForward { requested_forward_scid },
+						Some(HTLCHandlingFailureReason::Local {
+							reason: LocalHTLCFailureReason::UnknownNextPeer
+						}))
+						=> HTLCHandlingFailureType::UnknownNextHop {
+							requested_forward_scid: *requested_forward_scid,
+						},
+					_ => failure_type.clone()
+				};


Apologies if I signed off on this previously, but I think it would make more sense to always write the non-deprecated type and then include a release note that downgrades will result in reading the deprecated type as ::InvalidForward, since it'll save some complexity here and keep the door open for us to delete the variant in the future.

Having done it this way, I think it looks much better 👍 doesn't seem like that much of a loss to have less information on downgrade in comparison

Are there no functional consequences after the downgrade for this loss of information?

valentinewallace · 2025-04-25T23:51:37Z

lightning/src/events/mod.rs

+									failure_reason: Some(LocalHTLCFailureReason::UnknownNextPeer.into()),
+								}
+						}
+						_ => panic!("HTLCHandlingFailed wrong type")


Wait, is this panic reachable? May want to just remove it or use debug_assert instead

Yikes yeah, reverted to just match on HTLCHandlingType (was like this before change suggestion) - think the panic should be ok if it's for a variant we just declared right above?

This allows us to more easily get rid of this variant (as we stop writing it), and removes the need for custom serialization. The tradeoff here is that downgrading nodes lose some information, as the UnknownNextPeer variant will be represented as InvalidForward (because they can't read the accompanying reason that specifies that it's a UnknownNextPeer.

carlaKC mentioned this pull request Apr 2, 2025

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

Merged

carlaKC changed the title ~~Add Failure Reason to HTLCHandlingFailed~~ (3/3) Add Failure Reason to HTLCHandlingFailed Apr 2, 2025

carlaKC force-pushed the 3541-api-changes branch 2 times, most recently from d049301 to 6138779 Compare April 4, 2025 20:36

valentinewallace reviewed Apr 4, 2025

View reviewed changes

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

valentinewallace mentioned this pull request Apr 10, 2025

WIP: Trampoline forwarding #3711

Open

4 tasks

carlaKC force-pushed the 3541-api-changes branch from 6138779 to 2d39ef8 Compare April 11, 2025 21:43

carlaKC force-pushed the 3541-api-changes branch from 2d39ef8 to b48c6ce Compare April 15, 2025 18:51

carlaKC force-pushed the 3541-api-changes branch from b48c6ce to cda8de5 Compare April 17, 2025 18:07

carlaKC marked this pull request as ready for review April 17, 2025 19:24

ldk-reviews-bot requested a review from wpaulino April 17, 2025 19:24

carlaKC requested a review from valentinewallace April 17, 2025 19:25

joostjager requested review from joostjager and removed request for wpaulino April 17, 2025 19:53

joostjager reviewed Apr 18, 2025

View reviewed changes

carlaKC force-pushed the 3541-api-changes branch from cda8de5 to 6638118 Compare April 21, 2025 13:37

valentinewallace reviewed Apr 22, 2025

View reviewed changes

carlaKC force-pushed the 3541-api-changes branch from 6638118 to c78304f Compare April 23, 2025 17:35

carlaKC requested a review from joostjager April 24, 2025 12:35

valentinewallace reviewed Apr 25, 2025

View reviewed changes

carlaKC added 4 commits April 25, 2025 16:14

ln+events+liquidity/refactor: rename FailedPayment to Receive

85f07b9

Rename variant to be more specific to the current context - a FailedPayment could be a payment that we failed to dispatch or one that we rejected on receive.

ln+events+liquidity/refactor: NextHopChannel renamed Forward

df03203

Standardize naming within the HTLCHandlingType struct to present more consistent API terminology.

ln+events: add htlc failure reason to HTLCHandlingFailed

37b7dbe

carlaKC force-pushed the 3541-api-changes branch from c78304f to 51eb743 Compare April 25, 2025 20:46

valentinewallace previously approved these changes Apr 25, 2025

View reviewed changes

valentinewallace reviewed Apr 25, 2025

View reviewed changes

carlaKC dismissed valentinewallace’s stale review via 322fa64 April 26, 2025 13:45

pending_changelog: add notes for lightningdevkit#3700

5badb5e

carlaKC added a commit to carlaKC/rust-lightning that referenced this pull request Apr 26, 2025

pending_changelog: add notes for lightningdevkit#3700

a195aab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

carlaKC commented Apr 2, 2025 •

edited

Loading

ldk-reviews-bot commented Apr 2, 2025 •

edited

Loading

codecov bot commented Apr 11, 2025 •

edited

Loading

valentinewallace commented Apr 16, 2025

ldk-reviews-bot commented Apr 21, 2025

carlaKC commented Apr 21, 2025

valentinewallace Apr 22, 2025

carlaKC Apr 23, 2025

valentinewallace Apr 23, 2025

valentinewallace Apr 22, 2025

valentinewallace Apr 22, 2025

carlaKC Apr 23, 2025

carlaKC commented Apr 23, 2025

valentinewallace Apr 25, 2025

joostjager Apr 25, 2025

carlaKC Apr 25, 2025

valentinewallace left a comment

valentinewallace Apr 25, 2025

carlaKC Apr 26, 2025

joostjager Apr 26, 2025

valentinewallace Apr 25, 2025 •

edited

Loading

carlaKC Apr 26, 2025

		// [`LocalHTLCFailureReason::UnknownNextHop`]. This will cover both the case
		// where we have a legacy event

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

Are you sure you want to change the base?

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

Conversation

carlaKC commented Apr 2, 2025 • edited Loading

ldk-reviews-bot commented Apr 2, 2025 • edited Loading

codecov bot commented Apr 11, 2025 • edited Loading

Codecov Report

valentinewallace commented Apr 16, 2025

ldk-reviews-bot commented Apr 21, 2025

carlaKC commented Apr 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Apr 23, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Apr 2, 2025 •

edited

Loading

ldk-reviews-bot commented Apr 2, 2025 •

edited

Loading

codecov bot commented Apr 11, 2025 •

edited

Loading

valentinewallace Apr 25, 2025 •

edited

Loading