LSPS2: Fail (or abandon) intercepted HTLCs if LSP channel open fails #3712

martinsaposnic · 2025-04-07T16:09:44Z

Allows the LSP to signal when a channel open fails or is abandoned.

Fail variant: All pending payments are failed backward, and the channel state resets to PendingInitialPayment, allowing the payer to retry.
Abandon variant: Instead of resetting for retrial, this prunes all channel-related state, fully discarding the attempt.

Inspired by previous work and a suggestion from @tnull to add an abandon variant apart from the reset.

ldk-reviews-bot · 2025-04-07T16:09:47Z

👋 Thanks for assigning @tnull 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.

codecov · 2025-04-07T16:37:46Z

Codecov Report

Attention: Patch coverage is 77.10843% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.36%. Comparing base (89f5217) to head (bf68eda).

Files with missing lines	Patch %	Lines
lightning-liquidity/src/lsps2/service.rs	77.10%	14 Missing and 5 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3712   +/-   ##
=======================================
  Coverage   89.35%   89.36%           
=======================================
  Files         157      157           
  Lines      124095   124178   +83     
  Branches   124095   124178   +83     
=======================================
+ Hits       110886   110966   +80     
  Misses      10488    10488           
- Partials     2721     2724    +3

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

ldk-reviews-bot · 2025-04-09T17:11:04Z

🔔 1st Reminder

Hey @tnull! 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.

ldk-reviews-bot · 2025-04-12T00:00:27Z

🔔 2nd Reminder

Hey @tnull! 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.

ldk-reviews-bot · 2025-04-14T00:00:47Z

🔔 3rd Reminder

Hey @tnull! 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.

tnull

Thanks for looking into this, and excuse the delay! We'll probably wait with this until after #3509 landed either way though.

tnull · 2025-04-15T08:57:22Z

lightning-liquidity/src/lsps2/service.rs

+	/// without resetting the state for a potential payment retry. It removes the intercept SCID
+	/// mapping along with any outbound channel state and related channel ID mappings associated with
+	/// the specified `user_channel_id`.
+	pub fn channel_open_abandoned(


I think to make this a bit safer, it should probably fail if we're already at PendingPaymentForward or later, i.e., we already opened the channel?

Also, what would happen if this is called after the LSPS2ServiceEvent::OpenChannel has been emitted?

Now when abandon() is called, if it's not at PendingInitialPayment or PendingChannelOpen, then it fails.

Also, what would happen if this is called after the LSPS2ServiceEvent::OpenChannel has been emitted?

You mean if the channel is already opened, but our state machine hasn’t caught up yet and still thinks we're at PendingChannelOpen? (Is that possible?). In that case, if we call abandon(), we’d end up forgetting about a channel that was actually opened, potentially leaving funds locked. If that scenario is possible and we can't reliably detect it, then this version of abandon() isn't safe.

You mean if the channel is already opened, but our state machine hasn’t caught up yet and still thinks we're at PendingChannelOpen? (Is that possible?).

Yes. I think it would be possible if the user had already initiated a channel open but hasn't gotten the ChannelReady yet? It might be worth documenting that the user is responsible for cleaning up any pending channel opens.

tnull · 2025-04-15T08:58:09Z

lightning-liquidity/src/lsps2/service.rs

+			})?;
+
+		let jit_channel = peer_state
+        .outbound_channels_by_intercept_scid


nit: indentation seems off.

tnull · 2025-04-15T08:58:51Z

lightning-liquidity/src/lsps2/service.rs

+            ),
+        })?;
+
+		jit_channel.state = match &jit_channel.state {


Maybe use the matches! macro here?

tnull · 2025-04-15T08:59:05Z

lightning-liquidity/tests/lsps2_integration_tests.rs

@@ -24,6 +25,7 @@ use lightning_invoice::{Bolt11Invoice, InvoiceBuilder, RoutingFees};
 use bitcoin::hashes::{sha256, Hash};
 use bitcoin::secp256k1::{PublicKey, Secp256k1};
 use bitcoin::Network;
+use lightning_types::payment::PaymentHash;


nit: move import up.

ldk-reviews-bot · 2025-04-15T09:00:26Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

ldk-reviews-bot · 2025-04-22T21:22:02Z

🔔 1st Reminder

Hey @tnull! 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.

ldk-reviews-bot · 2025-04-24T21:22:34Z

🔔 2nd Reminder

Hey @tnull! 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.

ldk-reviews-bot · 2025-04-28T00:00:07Z

🔔 3rd Reminder

Hey @tnull! 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.

tnull

Excuse the delay here!

lightning-liquidity/src/lsps2/service.rs

tnull · 2025-04-29T11:46:32Z

lightning-liquidity/src/lsps2/service.rs

@@ -11,6 +11,7 @@

 use alloc::string::{String, ToString};
 use alloc::vec::Vec;
+use lightning::util::hash_tables::HashSet;


Let's move this import down to the other lightning imports.

tnull · 2025-04-29T11:55:38Z

lightning-liquidity/src/lsps2/service.rs

+	/// without resetting the state for a potential payment retry. It removes the intercept SCID
+	/// mapping along with any outbound channel state and related channel ID mappings associated with
+	/// the specified `user_channel_id`.
+	pub fn channel_open_abandoned(


You mean if the channel is already opened, but our state machine hasn’t caught up yet and still thinks we're at PendingChannelOpen? (Is that possible?).

Yes. I think it would be possible if the user had already initiated a channel open but hasn't gotten the ChannelReady yet? It might be worth documenting that the user is responsible for cleaning up any pending channel opens.

tnull · 2025-04-29T11:57:13Z

lightning-liquidity/src/lsps2/service.rs

+			.outbound_channels_by_intercept_scid
+			.get_mut(&intercept_scid)
+			.ok_or_else(|| APIError::APIMisuseError {
+            err: format!(


Formatting is off here.

tnull · 2025-04-29T12:00:10Z

lightning-liquidity/src/lsps2/service.rs

+			if let OutboundJITChannelState::PendingChannelOpen { payment_queue, .. } =
+				&jit_channel.state
+			{
+				Arc::clone(payment_queue)


This needs a rebase as post-#3509 the payment queue isn't held in an Arc. We should also avoid constructions that introduce reachable panics like this.

tnull · 2025-04-29T12:03:50Z

lightning-liquidity/tests/lsps2_integration_tests.rs

+		},
+		_ => panic!("Expected OpenChannel event"),
+	};
+	assert!(open_channel_event);


This is unnecessary as you already panic in the _ arm above. Same below for the reopen_channel_event bool.

tnull · 2025-04-29T12:05:41Z

lightning-liquidity/tests/lsps2_integration_tests.rs

+
+#[test]
+fn channel_open_failed() {
+	let promise_secret = [42; 32];


Can we DRY-up the redudant setup code? It would make the individual test cases much more concise and readable.

Add two LSPS2Service methods: 'Abandoned' prunes all channel open state. 'Failed' resets JIT channel to fail HTLCs. It allows a retry on channel open. Closes lightningdevkit#3479.

Add integration tests to verify channel open reset and pruning handlers. Tests cover: - channel_open_failed resetting state to allow retry. - channel_open_failed error on invalid state. - channel_open_abandoned pruning all open state. - error handling for nonexistent channels.

martinsaposnic · 2025-05-04T02:11:13Z

Thanks for the review @tnull. PR rebased and comments addressed!

ldk-reviews-bot · 2025-05-06T02:11:51Z

🔔 1st Reminder

Hey @tnull! 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.

ldk-reviews-bot · 2025-05-08T02:12:21Z

🔔 2nd Reminder

Hey @tnull! 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.

ldk-reviews-bot · 2025-05-10T02:12:51Z

🔔 3rd Reminder

Hey @tnull! 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.

ldk-reviews-bot · 2025-05-12T02:13:22Z

🔔 4th Reminder

Hey @tnull! 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.

tnull

Excuse the delay here.

tnull · 2025-05-12T11:09:57Z

lightning-liquidity/src/lsps2/service.rs

+	///
+	/// This removes the intercept SCID, any outbound channel state, and associated
+	/// channel‐ID mappings for the specified `user_channel_id`, but only while the JIT
+	/// channel is still in `PendingInitialPayment` or `PendingChannelOpen`.


nit: OutboundJITChannelState is currently not exposed as part of the public API (which might be another task at some point, but this discussion is unrelated). Hence, let's drop these state names and rather describe the behavior.

tnull · 2025-05-12T11:10:21Z

lightning-liquidity/src/lsps2/service.rs

+	///
+	/// Returns an error if:
+	///  - there is no channel matching `user_channel_id`, or
+	///  - the channel has already advanced past `PendingChannelOpen` (e.g. to


Same here: let's drop the state name, and describe what this case means.

tnull · 2025-05-12T11:12:03Z

lightning-liquidity/src/lsps2/service.rs

+	///
+	/// Note: this does *not* close or roll back any on‐chain channel which may already
+	/// have been opened. The caller must only invoke this before a channel is actually
+	/// confirmed (or else provide its own on‐chain cleanup) and is responsible for


Hmm, a user needs to call this before/in-place of initiating the channel open, not only before it's confirmed, no?

tnull · 2025-05-12T11:13:41Z

lightning-liquidity/src/lsps2/service.rs

+		if let Some(jit_channel) =
+			peer_state.outbound_channels_by_intercept_scid.get(&intercept_scid)
+		{
+			if !matches!(


nit: Can we move the matches into a bool variable? This would let the if read a bit better.

tnull · 2025-05-12T11:16:25Z

lightning-liquidity/src/lsps2/service.rs

+				OutboundJITChannelState::PendingInitialPayment { .. }
+					| OutboundJITChannelState::PendingChannelOpen { .. }
+			) {
+				return Err(APIError::APIMisuseError {


Hmm, if we return with an error here, are we fine that we already removed the intercept SCID above? Or does this need to happen atomically, i.e., should we only remove the intercept SCID from intercept_scid_by_user_channel_id if we succeed here?

tnull · 2025-05-12T11:18:42Z

lightning-liquidity/src/lsps2/service.rs

+			),
+		})?;
+
+		if !matches!(jit_channel.state, OutboundJITChannelState::PendingChannelOpen { .. }) {


I think we can drop this, since we're doing the same check matching below anyways?

tnull · 2025-05-12T11:22:37Z

lightning-liquidity/src/lsps2/service.rs

+
+		let mut payment_queue = match &jit_channel.state {
+			OutboundJITChannelState::PendingChannelOpen { payment_queue, .. } => {
+				payment_queue.clone()


We should be able to avoid this clone here if we follow the core::mem::take pattern we use elsewhere for state transitions.

tnull · 2025-05-12T11:25:31Z

lightning-liquidity/src/lsps2/service.rs

+				});
+			},
+		};
+		let payment_hashes: Vec<_> = payment_queue


I don't think deduplication over payment hash warrants collecting twice here. In fact, fail_htlc_backwards_with_reason should work idempotently, so we can just iterate over payment_queue directly.

ldk-reviews-bot requested a review from valentinewallace April 7, 2025 16:20

tnull requested review from tnull and removed request for valentinewallace April 7, 2025 17:10

martinsaposnic force-pushed the handle-channel-open-failure branch from f50496f to 0d8ba10 Compare April 7, 2025 18:09

tnull mentioned this pull request Apr 9, 2025

lightning-liquidity: Introduce EventQueue notifier and wake BP for message processing #3509

Merged

tnull reviewed Apr 15, 2025

View reviewed changes

martinsaposnic force-pushed the handle-channel-open-failure branch from 0d8ba10 to 5539995 Compare April 20, 2025 21:13

martinsaposnic requested a review from tnull April 20, 2025 21:21

tnull reviewed Apr 29, 2025

View reviewed changes

add failed & abandoned HTLC open handlers

000c0a3

Add two LSPS2Service methods: 'Abandoned' prunes all channel open state. 'Failed' resets JIT channel to fail HTLCs. It allows a retry on channel open. Closes lightningdevkit#3479.

martinsaposnic force-pushed the handle-channel-open-failure branch 2 times, most recently from aad24ac to 694006d Compare May 3, 2025 14:57

martinsaposnic force-pushed the handle-channel-open-failure branch from 694006d to bf68eda Compare May 3, 2025 15:28

martinsaposnic requested a review from tnull May 4, 2025 02:10

tnull reviewed May 12, 2025

View reviewed changes

LSPS2: Fail (or abandon) intercepted HTLCs if LSP channel open fails #3712

Are you sure you want to change the base?

LSPS2: Fail (or abandon) intercepted HTLCs if LSP channel open fails #3712

Conversation

martinsaposnic commented Apr 7, 2025

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

codecov bot commented Apr 7, 2025 • edited Loading

Codecov Report

ldk-reviews-bot commented Apr 9, 2025

ldk-reviews-bot commented Apr 12, 2025

ldk-reviews-bot commented Apr 14, 2025

tnull 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

ldk-reviews-bot commented Apr 15, 2025

ldk-reviews-bot commented Apr 22, 2025

ldk-reviews-bot commented Apr 24, 2025

ldk-reviews-bot commented Apr 28, 2025

tnull 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

martinsaposnic commented May 4, 2025

ldk-reviews-bot commented May 6, 2025

ldk-reviews-bot commented May 8, 2025

ldk-reviews-bot commented May 10, 2025

ldk-reviews-bot commented May 12, 2025

tnull 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

ldk-reviews-bot commented Apr 7, 2025 •

edited

Loading

codecov bot commented Apr 7, 2025 •

edited

Loading