LSPS2: Add error handling events for failed client requests #3804

martinsaposnic · 2025-05-27T16:19:27Z

Add GetInfoFailed and BuyRequestFailed event variants to LSPS2ClientEvent
to handle LSP error responses, similar to the OrderRequestFailed event
added to LSPS1.

Add GetInfoFailed and BuyRequestFailed event variants to LSPS2ClientEvent to handle LSP error responses, similar to the OrderRequestFailed event added to LSPS1. Fixes lightningdevkit#3459

Update handle_get_info_error and handle_buy_error to: - Emit GetInfoFailed and BuyRequestFailed events to notify users - Return LightningError instead of Ok() to properly signal failures - Use the actual error parameter instead of ignoring it This ensures consistent error handling behavior across LSPS implementations.

Replace hardcoded BuyError responses with appropriate error types based on the actual request (GetInfo vs Buy) when rate limiting is triggered. This fixes incorrect error response types and provides an easy way to test the error event handling added in this PR.

Add tests for the new error events: - Test per-peer request limit rejection (GetInfoFailed) - Test global request limit rejection (BuyRequestFailed) - Test invalid token handling (GetInfoFailed) These tests verify that error events are properly emitted when LSP requests fail, ensuring the error handling behavior works end-to-end.

ldk-reviews-bot · 2025-05-27T16:19:31Z

I've assigned @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-05-27T16:28:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.82%. Comparing base (101aa6f) to head (18e791d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3804      +/-   ##
==========================================
+ Coverage   89.76%   89.82%   +0.06%     
==========================================
  Files         159      159              
  Lines      128828   128850      +22     
  Branches   128828   128850      +22     
==========================================
+ Hits       115638   115737      +99     
+ Misses      10508    10432      -76     
+ Partials     2682     2681       -1

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

tnull

Thanks! Mostly looks good, just some comments.

Will review the test changes closer once #3712 landed and we rebased, as the same refactoring is happening there.

tnull · 2025-05-28T11:02:42Z

lightning-liquidity/src/lsps2/event.rs

+	/// A request previously issued via [`LSPS2ClientHandler::select_opening_params`]
+	/// failed as the LSP returned an error response.
+	///
+	/// [`LSPS2ClientHandler::select_opening_params`]: crate::lsps2::client::LSPS2ClientHandler::select_opening_params


Ah, preexisting, but it seems we don't document on that method what the returned LSPSRequestId is for. Can we add that in a separate commit while we're here?

tnull · 2025-05-28T11:06:49Z

lightning-liquidity/src/lsps2/client.rs

+				event_queue_notifier.enqueue(LSPS2ClientEvent::GetInfoFailed {
+					request_id: request_id.clone(),
+					counterparty_node_id: *counterparty_node_id,
+					error: error.clone(),


We can avoid the clones here and below if we first construct the error result before enqueuing the event, which can then move request_id/error.

tnull · 2025-05-28T11:13:03Z

lightning-liquidity/src/lsps2/service.rs

 				code: LSPS0_CLIENT_REJECTED_ERROR_CODE,
-				message: "Reached maximum number of pending requests. Please try again later."


No, it was very intentional that we hardcoded a standardized response here, because while we want to log the full details, the counterparty shouldn't learn anything about our rate limits.

Could you expand what else you mean with "incorrect error response types"?

Could you expand what else you mean with "incorrect error response types"?

insert_pending_request is called by handle_get_info_request and handle_buy_request but it always returns BuyError, even if the request type is GetInfo. Now it returns GetInfoError for GetInfo requests and BuyError for Buy requests.

ldk-reviews-bot · 2025-05-28T11:17:50Z

👋 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-05-28T12:02:31Z

✅ Added second reviewer: @joostjager

joostjager · 2025-05-28T14:45:24Z

lightning-liquidity/tests/lsps2_integration_tests.rs

+	service_node.liquidity_manager.handle_custom_message(get_info_request, client_node_id).unwrap();
+
+	let get_info_event = service_node.liquidity_manager.next_event().unwrap();
+	match get_info_event {


With something like this, you can keep the happy flow unindented for improved readability.

let LiquidityEvent::LSPS2Service(LSPS2ServiceEvent::GetInfo { request_id, counterparty_node_id, token, }) = service_node.liquidity_manager.next_event().unwrap() else { panic!("Unexpected event"); };

Let-else bindings are only stabilized since rustc 1.65, so we can't use them currently due to our 1.63 MSRV.

Hmm too bad. Still if let can I think remove one level of indent?

joostjager · 2025-05-28T15:02:26Z

lightning-liquidity/src/lsps2/event.rs

+		/// The node id of the LSP.
+		counterparty_node_id: PublicKey,
+		/// The error that was returned.
+		error: LSPSResponseError,


Would there be a point in putting these three fields in a struct and reusing that across lsps1 and 2?

No, please let's not break the current API pattern in a single place.

Not in a single place, doing it everywhere. Just a thought, can understand you don't want to change existing API. Although in other cases, we aren't so conservative with API changes?

joostjager · 2025-05-28T15:33:45Z

lightning-liquidity/src/lsps2/service.rs

-				"Peer {} reached maximum number of total pending requests: {}",
+		if self.total_pending_requests.load(Ordering::Relaxed) >= MAX_TOTAL_PENDING_REQUESTS {
+			let error_message = format!(
+				"reached maximum number of total pending requests {}: {}",


Not capitalized anymore?

Is it useful to have counterparty_node_id in this message when the total is exceeded?

joostjager · 2025-05-28T15:37:50Z

lightning-liquidity/src/lsps2/service.rs

@@ -1226,45 +1226,43 @@ where
 		&self, peer_state_lock: &mut MutexGuard<'a, PeerState>, request_id: LSPSRequestId,
 		counterparty_node_id: PublicKey, request: LSPS2Request,
 	) -> (Result<(), LightningError>, Option<LSPSMessage>) {


Pre-existing but shouldn't the type be Result<(), (LightningError, LSPSMessage>)?

No, the method returns either Ok(()) or an error, and optionally a message that needs to be sent to the counterparty.

I thought that in this fn, it's indeed either Ok(()), or an error always accompanied by a msg (not optional). But maybe this is a useful type further up the call stack?

martinsaposnic added 4 commits May 27, 2025 13:14

LSPS2: Add error handling events for failed client requests

d2358c2

Add GetInfoFailed and BuyRequestFailed event variants to LSPS2ClientEvent to handle LSP error responses, similar to the OrderRequestFailed event added to LSPS1. Fixes lightningdevkit#3459

ldk-reviews-bot requested a review from tnull May 27, 2025 16:30

tnull mentioned this pull request May 28, 2025

fuzz Failure: Received counterparty stfu while having pending counterparty updates #3805

Open

tnull reviewed May 28, 2025

View reviewed changes

ldk-reviews-bot requested a review from joostjager May 28, 2025 12:02

joostjager reviewed May 28, 2025

View reviewed changes

		code: LSPS0_CLIENT_REJECTED_ERROR_CODE,
		message: "Reached maximum number of pending requests. Please try again later."

LSPS2: Add error handling events for failed client requests #3804

Are you sure you want to change the base?

LSPS2: Add error handling events for failed client requests #3804

Conversation

martinsaposnic commented May 27, 2025

Uh oh!

ldk-reviews-bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 27, 2025

Codecov Report

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented May 28, 2025

Uh oh!

ldk-reviews-bot commented May 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldk-reviews-bot commented May 27, 2025 •

edited

Loading

tnull May 28, 2025 •

edited

Loading