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.

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

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

codecov · 2025-05-27T16:28:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.83%. Comparing base (ee0830c) to head (7dd91c0).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3804      +/-   ##
==========================================
+ Coverage   89.74%   89.83%   +0.09%     
==========================================
  Files         159      159              
  Lines      128906   128933      +27     
  Branches   128906   128933      +27     
==========================================
+ Hits       115681   115833     +152     
+ Misses      10520    10404     -116     
+ Partials     2705     2696       -9

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

lightning-liquidity/src/lsps2/event.rs

lightning-liquidity/src/lsps2/client.rs

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.

Ah, right, that difference got lost in the diff here. Yes, then let's still keep returning the hardcoded error strings to the counterparties, but just make sure the right response type is returned.

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?

found a few places where indentation is reduced so I did the if let there 👍

lightning-liquidity/src/lsps2/event.rs

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?

But the rest of the API follows the same pattern of returning Result<(), LightningError>. Changing the result type in this way here would mean we'd get an Err variant that we can't just bubble up at the callsite but rather awkwardly need to map, etc.

Also note that we chose that if let Some(msg) pattern to ensure we're able to drop all locks (in particular peer_state_lock) before enqueuing messages, which might lead to reentry when PeerManager processes the enqueued messages. I.e., it was exactly introduced to avoid handling/enqueing the messages in the match.

The Err variant is already not just bubbled up. Both call sites do a match on the result already?

I was indeed wondering why the lock had to be dropped before enqueueing. Could be worthy of a comment.

This thread is mostly out of curiosity / to learn. No changes requested.

The Err variant is already not just bubbled up

Yes it is, essentially at all callsites?

I was indeed wondering why the lock had to be dropped before enqueueing. Could be worthy of a comment.

Well, it's also best practice in general. But yeah. At some point we could even consider a refactoring that would make it infeasible to enqueue while still holding the lock.

Yes it is, essentially at all callsites?

There is still a match at the call sites.

Well, it's also best practice in general.

You mean best practice to not hold the lock longer than necessary? Or not queueing while holding the lock?

You mean best practice to not hold the lock longer than necessary? Or not queueing while holding the lock?

The latter.

martinsaposnic · 2025-05-30T03:06:36Z

@tnull @joostjager thank you for your reviews! Comments are addressed on fixup commits

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

Rebase with #3712 is done

martinsaposnic · 2025-05-30T03:07:59Z

The build macos-latest 1.63.0 failed with error [build ](error: package lock_api v0.4.13 cannot be built because it requires rustc 1.64.0 or newer, while the currently active rustc version is 1.63.0)

I don't think it's related to my PR but will take a closer look tomorrow

joostjager · 2025-05-30T07:32:37Z

lightning-liquidity/tests/lsps2_integration_tests.rs

+			assert_eq!(counterparty_node_id, service_node_id);
+			assert_eq!(error.code, 1); // LSPS0_CLIENT_REJECTED_ERROR_CODE
+		},
+		_ => panic!("Expected LSPS2ClientEvent::BuyRequestFailed event"),


nit: more indent removal possible using if let? Also further up there are blocks.

tnull · 2025-05-30T08:07:38Z

The build macos-latest 1.63.0 failed with error [build ](error: package lock_api v0.4.13 cannot be built because it requires rustc 1.64.0 or newer, while the currently active rustc version is 1.63.0)

I don't think it's related to my PR but will take a closer look tomorrow

Yeah, no, this is unrelated as they unfortunately bumped the MSRV on a patch release. CI fix is here: #3812

tnull

LGTM, feel free to squash fixups.

I think you may want to rebase post #3712 and #3812

tnull · 2025-05-30T08:11:30Z

lightning-liquidity/src/lsps2/client.rs

-	/// `max(min_fee_msat, proportional*(payment_size_msat/1_000_000))`.
+	/// `max(min_fee_msat, proportional * (payment_size_msat / 1_000_000))`.
+	///
+	/// Returns the used [`LSPSRequestId`], which will be returned via [`InvoiceParametersReady`].


nit: Returns .. which will be returned via is kind of a confusing formulation. So where will it be returned now?

Maybe just "Returns the used LSPSRequestId that was used for the buy request" or similar?

tnull · 2025-05-30T08:21:18Z

lightning-liquidity/tests/lsps2_integration_tests.rs

@@ -50,7 +55,7 @@ fn setup_test_lsps2(
 	};

 	let (service_node, client_node) =
-		create_service_and_client_nodes("webhook_registration_flow", service_config, client_config);
+		create_service_and_client_nodes("default_persist_dir", service_config, client_config);


Can we make this a parameter of setup_test_lsps2 and create test-specific persist dirs? This would be helpful as otherwise all tests might run on the same persisted directories, which would break going forward once we actually persist something.

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.

… tests

martinsaposnic · 2025-05-30T16:58:48Z

Rebased on top of #3712 and #3812. Also added a few fixup commits.

ldk-reviews-bot · 2025-06-02T00:00:16Z

🔔 1st Reminder

Hey @tnull @joostjager! 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-06-02T00:00:17Z

🔔 1st Reminder

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

LGTM, feel free to squash fixups.

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

martinsaposnic force-pushed the issue3459 branch 2 times, most recently from e80ef0d to f1c95fc Compare May 29, 2025 15:55

martinsaposnic requested review from tnull and joostjager May 30, 2025 03:26

joostjager previously approved these changes May 30, 2025

View reviewed changes

tnull reviewed May 30, 2025

View reviewed changes

martinsaposnic added 7 commits May 30, 2025 10:32

Prefactor -> document LSPSRequestId usage on select_opening_params

c60459c

Add error handling events for failed client requests

ff31829

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

f: fix documentation on client

d0b802b

f: make persist dir a param. also use if let to reduce indentation on…

7dd91c0

… tests

martinsaposnic dismissed joostjager’s stale review via 7dd91c0 May 30, 2025 13:46

martinsaposnic force-pushed the issue3459 branch from f1c95fc to 7dd91c0 Compare May 30, 2025 13:46

martinsaposnic requested review from tnull and joostjager May 30, 2025 16:58

joostjager approved these changes Jun 2, 2025

View reviewed changes

tnull reviewed Jun 2, 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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

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!

martinsaposnic commented May 30, 2025

Uh oh!

martinsaposnic commented May 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented May 30, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull May 30, 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!

martinsaposnic commented May 30, 2025

Uh oh!

ldk-reviews-bot commented Jun 2, 2025

Uh oh!

ldk-reviews-bot commented May 27, 2025 •

edited

Loading

codecov bot commented May 27, 2025 •

edited

Loading

tnull May 28, 2025 •

edited

Loading

tnull May 30, 2025 •

edited

Loading