Enable decoding HTLC onions when fully committed #2933

wpaulino · 2024-03-12T17:18:22Z

This PR ensures all new incoming HTLCs going forward will have their onion decoded when they become fully committed to decide how we should proceed with each one. As a result, we'll obtain HTLCHandlingFailed events for any failed HTLC that comes across a channel.

We will now start writing channels with the new serialization version (4), and we will still be able to downgrade back to the commit that introduced it since reading version 4 is supported.

Note that existing pending inbound HTLCs may already have their resolution if they were received in a previous version of LDK. We must support those until we no longer allow downgrading beyond this commit.

codecov-commenter · 2024-03-22T19:10:27Z

Codecov Report

Attention: Patch coverage is 98.41772% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.44%. Comparing base (ad462bd) to head (d8d9dc7).
Report is 62 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	88.46%	0 Missing and 3 partials ⚠️
lightning/src/ln/payment_tests.rs	98.09%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2933      +/-   ##
==========================================
+ Coverage   88.42%   88.44%   +0.02%     
==========================================
  Files         149      149              
  Lines      112959   113363     +404     
  Branches   112959   113363     +404     
==========================================
+ Hits        99883   100268     +385     
  Misses      10605    10605              
- Partials     2471     2490      +19

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

TheBlueMatt · 2024-03-27T22:45:06Z

Tagging 0.0.122 not because we need to land this for 0.0.122, but because we have to at least be roughly confident in it working and the tests in it being sufficient.

valentinewallace · 2024-04-30T14:40:06Z

As discussed offline, we'll want to land a cfg-flagged version of this PR :)

TheBlueMatt · 2024-05-06T16:46:56Z

Okay, sounds like @valentinewallace took a glance at this, and I did too. Nothing too wild here, tests pass, we can do this in the next release (or whenever), but should cfg-flag the changes and land it sooner so that we don't have this branch get toooo stale.

wpaulino · 2024-05-18T05:24:44Z

Before I go down the cfg flag path, does it make sense to merge this now as is if the project is doing branched releases going forward?

TheBlueMatt · 2024-05-19T19:15:55Z

Yea, I don't see why not? Just cause we're branching off for future releases doesn't mean we can't/shouldn't have future code behind cfg flags on the working tip.

jkczyz · 2024-09-06T20:24:39Z

Chatted with @wpaulino today. His quiescence branch is based on this PR and IIUC requires it. He agreed to rebase it soon. @TheBlueMatt @wpaulino Do we still want a cfg flag for this?

TheBlueMatt · 2024-09-10T00:05:10Z

I think we're probably okay without now. @valentinewallace checked today and said support was added in 0.0.123, which means if we ship this we'll support downgrading two versions of LDK but not more, which is pretty normal for us, I think.

jkczyz · 2024-09-12T17:30:35Z

@wpaulino Needs another rebase.

TheBlueMatt · 2024-09-16T15:18:53Z

lightning/src/ln/channel.rs

-			MIN_SERIALIZATION_VERSION
-		};
-		write_ver_prefix!(writer, version_to_write, MIN_SERIALIZATION_VERSION);
+		write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);


Shouldn't we be bumping the MIN_SERIALIZATION_VERSION here (and should have done so in the old code, but it didn't matter then)?

Unfortunately I don't recall some of the context here. It does seem safe to bump it now given that we're not supporting downgrades past 0.0.122. I think it just wasn't bumped in the parent PR because we were still using the named constant to write the current version.

TheBlueMatt · 2024-09-16T15:22:54Z

lightning/src/ln/channel.rs

@@ -6409,7 +6402,7 @@ impl<SP: Deref> Channel<SP> where
 		};
 		let exposure_dust_limit_timeout_sats = htlc_timeout_dust_limit + self.context.counterparty_dust_limit_satoshis;
 		if msg.amount_msat / 1000 < exposure_dust_limit_timeout_sats {
-			let on_counterparty_tx_dust_htlc_exposure_msat = htlc_stats.on_counterparty_tx_dust_exposure_msat + msg.amount_msat;
+			let on_counterparty_tx_dust_htlc_exposure_msat = htlc_stats.on_counterparty_tx_dust_exposure_msat;


Can you justify all the changes to dust calculation here and below?

The incoming HTLC is now included in the pending stats. This wasn't the case before because we'd previously evaluate the incoming add upon receiving it, while now we wait until it's fully committed by both sides.

Can you update comments/git commit message so that its clearer?

valentinewallace · 2024-03-28T18:50:02Z

lightning/src/ln/blinded_payment_tests.rs

 	if intro_fails {
 		let mut updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 		nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
 		do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false);
+		let failed_destination = match check {
+			ForwardCheckFail::InboundOnionCheck => HTLCDestination::InvalidOnion,
+			ForwardCheckFail::ForwardPayloadEncodedAsReceive => HTLCDestination::FailedPayment { payment_hash },


This seems like it should be ::InvalidOnion...?

I would think so too, but the error we get from processing doesn't come out as malformed from construct_pending_htlc_status.

We get a PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC)) with the reason:

Got blinded non final data with an HMAC of 0

I believe this has always been the case and is just being surfaced by this PR now.

The behavior of the intro node returning update_fail rather than malformed there aligns with the route blinding spec; intro nodes always error in the same way regardless of the "real" error. So it seems the issue is that real error isn't being bubbled up to be accurately included in the event, only the blinded one. That said, this case seems quite rare so doesn't seem worth addressing.

Can you confirm this has always been the case but is just being exposed now as a result of the changes? If so, it's probably best to address on its own as a follow-up.

Looked into this again. So the issue is that as coded in fe65648 we'll never map the result of construct_pending_htlc_status to HTLCDestination::InvalidOnion, to do so would require checking the onion error code within and tailoring the HTLCDestination more based on that. Don't think it's worth bothering to address though as this case is quite rare (and also broken for unblinded forwards, just not tested).

valentinewallace · 2024-09-23T05:50:19Z

lightning/src/ln/channel.rs

 			// side, only on the sender's. Note that with anchor outputs we are no longer as
 			// sensitive to fee spikes, so we need to account for them.
 			let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
-			let mut remote_fee_cost_incl_stuck_buffer_msat = self.context.next_remote_commit_tx_fee_msat(htlc_candidate, Some(()));
+			let mut remote_fee_cost_incl_stuck_buffer_msat = self.context.next_remote_commit_tx_fee_msat(htlc_candidate, None);


Comment 2 lines up needs updating. Also, it looks like we'll always pass in None to this method for the fee spike buffer now, may be able to remove that parameter (I'm still trying to figure out why this patch affects whether we include a fee spike buffer, though...)

Edit: no test coverage when I revert this diff btw.

I'm still trying to figure out why this patch affects whether we include a fee spike buffer

This is related to https://github.com/lightningdevkit/rust-lightning/pull/2933/files#r1769645521.

Before this PR, we'd evaluate can_accept_incoming_htlc prior to the incoming HTLC being committed, so all stats computations would not include it. Now, it's evaluated after it has been accepted.

We still check within update_add_htlc that we can accept it (to return an actual error/force close), but the check here is concerned with whether we can accept another HTLC (to prevent stuck channels). The HTLCCandidate argument to next_remote_commit_tx_fee_msat now replaces the use of the fee_spike_buffer_htlc being Some in this context.

Thanks for pointing this out though, because I think the amount of this HTLCCandidate should now be updated to be the smallest non-dust HTLC allowed for the channel? cc @TheBlueMatt

no test coverage when I revert this diff btw

With some of the context above, I assume that's because none of our tests care for whether we can accept more than one HTLC after accepting one, they only care for exactly one more.

Thanks for pointing this out though, because I think the amount of this HTLCCandidate should now be updated to be the smallest non-dust HTLC allowed for the channel? cc @TheBlueMatt

Right, the API for next_remote_commit_tx_fee_msat probably needs to change - the second argument is now always None but here we really want to pass no HTLC for the first argument but Some for the second. Maybe we just make the HTLCCandidate amount an Option?

But indeed we should update the comment here too.

Updated the API and comments.

valentinewallace

LGTM

lightning/src/ln/onion_route_tests.rs

valentinewallace · 2025-01-07T23:35:38Z

lightning/src/ln/blinded_payment_tests.rs

 	if intro_fails {
 		let mut updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 		nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
 		do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false);
+		let failed_destination = match check {
+			ForwardCheckFail::InboundOnionCheck => HTLCDestination::InvalidOnion,
+			ForwardCheckFail::ForwardPayloadEncodedAsReceive => HTLCDestination::FailedPayment { payment_hash },


Looked into this again. So the issue is that as coded in fe65648 we'll never map the result of construct_pending_htlc_status to HTLCDestination::InvalidOnion, to do so would require checking the onion error code within and tailoring the HTLCDestination more based on that. Don't think it's worth bothering to address though as this case is quite rare (and also broken for unblinded forwards, just not tested).

This argument will be asserted on in a future commit to ensure we obtain the intended `HTLCHandlingFailed::failed_next_destination` per HTLC failure.

TheBlueMatt

Need to glance at the dust buffer stuff again but I think this LGTM.

lightning/src/events/mod.rs

This commit ensures all new incoming HTLCs going forward will have their onion decoded when they become fully committed to decide how we should proceed with each one. As a result, we'll obtain `HTLCHandlingFailed` events for _any_ failed HTLC that comes across a channel. Previously, we would evaluate the incoming HTLC within `can_accept_incoming_htlc` upon receiving it, but not yet committed, so we'd always have to account for it ourselves manually when checking certain HTLC limits. With this change, we no longer need to do so as it will already be accounted for within the pending HTLC stats computation. We will now start writing channels with the new serialization version (4), and we will still be able to downgrade back to the commit that introduced it since reading version 4 is supported. Note that existing pending inbound HTLCs may already have their resolution if they were received in a previous version of LDK. We must support those until we no longer allow downgrading beyond this commit.

wpaulino mentioned this pull request Mar 12, 2024

Support decoding HTLC onions once fully committed #2845

Merged

wpaulino force-pushed the enable-decode-htlc-onion-until-committed branch from b66e094 to 4210c01 Compare March 22, 2024 18:33

wpaulino force-pushed the enable-decode-htlc-onion-until-committed branch from 4210c01 to 7c6cff3 Compare March 27, 2024 21:29

TheBlueMatt added this to the 0.0.122 milestone Mar 27, 2024

wpaulino force-pushed the enable-decode-htlc-onion-until-committed branch from 7c6cff3 to 11c18ec Compare March 29, 2024 01:57

TheBlueMatt removed this from the 0.0.123 milestone May 6, 2024

wpaulino force-pushed the enable-decode-htlc-onion-until-committed branch from 11c18ec to 574554b Compare May 18, 2024 05:23

wpaulino force-pushed the enable-decode-htlc-onion-until-committed branch from 574554b to bcdf60e Compare September 6, 2024 20:50

jkczyz mentioned this pull request Sep 12, 2024

Dual-funded channels and Splicing Project Tracking #1621

Open

7 tasks

wpaulino force-pushed the enable-decode-htlc-onion-until-committed branch from bcdf60e to 0562830 Compare September 12, 2024 19:56

TheBlueMatt reviewed Sep 16, 2024

View reviewed changes

valentinewallace reviewed Sep 17, 2024

View reviewed changes

wpaulino force-pushed the enable-decode-htlc-onion-until-committed branch from 0562830 to 183fd66 Compare September 21, 2024 19:50

valentinewallace reviewed Sep 23, 2024

View reviewed changes

wpaulino force-pushed the enable-decode-htlc-onion-until-committed branch from 183fd66 to b011fe2 Compare January 6, 2025 23:21

valentinewallace reviewed Jan 8, 2025

View reviewed changes

valentinewallace mentioned this pull request Jan 9, 2025

Support receiving async payments #3440

Merged

Add expected_htlc_destination argument to run_onion_failure_test

df9f3c0

This argument will be asserted on in a future commit to ensure we obtain the intended `HTLCHandlingFailed::failed_next_destination` per HTLC failure.

wpaulino force-pushed the enable-decode-htlc-onion-until-committed branch from b011fe2 to 1ca2ce7 Compare January 14, 2025 21:06

TheBlueMatt mentioned this pull request Jan 15, 2025

Give HTLCHandlingFailed a reason of some kind #3541

Closed

TheBlueMatt reviewed Jan 15, 2025

View reviewed changes

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

valentinewallace mentioned this pull request Jan 15, 2025

Redo HTLC Handling Pipeline to do more checks async #1302

Open

wpaulino force-pushed the enable-decode-htlc-onion-until-committed branch from 1ca2ce7 to d8d9dc7 Compare January 16, 2025 22:31

wpaulino added the weekly goal Someone wants to land this this week label Jan 22, 2025

TheBlueMatt approved these changes Jan 24, 2025

View reviewed changes

valentinewallace approved these changes Jan 24, 2025

View reviewed changes

valentinewallace merged commit a706159 into lightningdevkit:main Jan 24, 2025
24 of 25 checks passed

wpaulino deleted the enable-decode-htlc-onion-until-committed branch January 24, 2025 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable decoding HTLC onions when fully committed #2933

Enable decoding HTLC onions when fully committed #2933

wpaulino commented Mar 12, 2024 •

edited

Loading

codecov-commenter commented Mar 22, 2024 •

edited by codecov bot

Loading

TheBlueMatt commented Mar 27, 2024

valentinewallace commented Apr 30, 2024 •

edited

Loading

TheBlueMatt commented May 6, 2024

wpaulino commented May 18, 2024

TheBlueMatt commented May 19, 2024

jkczyz commented Sep 6, 2024 •

edited

Loading

TheBlueMatt commented Sep 10, 2024

jkczyz commented Sep 12, 2024

TheBlueMatt Sep 16, 2024

wpaulino Sep 21, 2024

TheBlueMatt Sep 16, 2024

wpaulino Sep 21, 2024

TheBlueMatt Sep 30, 2024

wpaulino Jan 6, 2025

valentinewallace Mar 28, 2024

wpaulino Sep 21, 2024

valentinewallace Sep 23, 2024

wpaulino Jan 6, 2025

valentinewallace Jan 7, 2025

valentinewallace Sep 23, 2024 •

edited

Loading

wpaulino Oct 4, 2024

TheBlueMatt Oct 7, 2024

TheBlueMatt Oct 7, 2024

wpaulino Jan 6, 2025

valentinewallace left a comment

valentinewallace Jan 7, 2025

TheBlueMatt left a comment

Enable decoding HTLC onions when fully committed #2933

Enable decoding HTLC onions when fully committed #2933

Conversation

wpaulino commented Mar 12, 2024 • edited Loading

codecov-commenter commented Mar 22, 2024 • edited by codecov bot Loading

Codecov Report

TheBlueMatt commented Mar 27, 2024

valentinewallace commented Apr 30, 2024 • edited Loading

TheBlueMatt commented May 6, 2024

wpaulino commented May 18, 2024

TheBlueMatt commented May 19, 2024

jkczyz commented Sep 6, 2024 • edited Loading

TheBlueMatt commented Sep 10, 2024

jkczyz commented Sep 12, 2024

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 Sep 23, 2024 • 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

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

wpaulino commented Mar 12, 2024 •

edited

Loading

codecov-commenter commented Mar 22, 2024 •

edited by codecov bot

Loading

valentinewallace commented Apr 30, 2024 •

edited

Loading

jkczyz commented Sep 6, 2024 •

edited

Loading

valentinewallace Sep 23, 2024 •

edited

Loading