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.99329% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.66%. Comparing base (a75fdab) to head (183fd66).
Report is 73 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/payment_tests.rs	98.09%	2 Missing ⚠️
lightning/src/ln/channel.rs	83.33%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #2933    +/-   ##
========================================
  Coverage   89.66%   89.66%            
========================================
  Files         126      126            
  Lines      102411   102739   +328     
  Branches   102411   102739   +328     
========================================
+ Hits        91826    92126   +300     
- Misses       7857     7879    +22     
- Partials     2728     2734     +6

Flag	Coverage Δ
	`89.66% <98.99%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

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

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.

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

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.

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

6 tasks

Add expected_htlc_destination argument to run_onion_failure_test

19ef855

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

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

valentinewallace Mar 28, 2024

wpaulino Sep 21, 2024

valentinewallace Sep 23, 2024

valentinewallace Sep 23, 2024 •

edited

Loading

wpaulino Oct 4, 2024

TheBlueMatt Oct 7, 2024

TheBlueMatt Oct 7, 2024

Enable decoding HTLC onions when fully committed #2933

Are you sure you want to change the base?

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

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

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