Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 3, 2025
+401 −94
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4e77209
Prefactor -> document LSPSRequestId usage on select_opening_params
martinsaposnic May 29, 2025
1ee90ac
Add error handling events for failed client requests
martinsaposnic May 27, 2025
50f3130
Emit error events and return errors in client error handlers
martinsaposnic May 27, 2025
0d0dbc4
Fix error response types for rate limiting in service
martinsaposnic May 27, 2025
cea624e
Add integration tests for error event handling
martinsaposnic May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 37 additions & 5 deletions lightning-liquidity/src/lsps2/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,9 @@ where
/// but MPP can no longer be used to pay it.
///
/// The client agrees to paying an opening fee equal to
/// `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`] that was used for the buy request.
///
/// [`OpeningParametersReady`]: crate::lsps2::event::LSPS2ClientEvent::OpeningParametersReady
/// [`InvoiceParametersReady`]: crate::lsps2::event::LSPS2ClientEvent::InvoiceParametersReady
Expand Down Expand Up @@ -230,8 +232,9 @@ where

fn handle_get_info_error(
&self, request_id: LSPSRequestId, counterparty_node_id: &PublicKey,
_error: LSPSResponseError,
error: LSPSResponseError,
) -> Result<(), LightningError> {
let event_queue_notifier = self.pending_events.notifier();
let outer_state_lock = self.per_peer_state.read().unwrap();
match outer_state_lock.get(counterparty_node_id) {
Some(inner_state_lock) => {
Expand All @@ -247,7 +250,21 @@ where
});
}

Ok(())
let lightning_error = LightningError {
err: format!(
"Received get_info error response for request {:?}: {:?}",
request_id, error
),
action: ErrorAction::IgnoreAndLog(Level::Error),
};

event_queue_notifier.enqueue(LSPS2ClientEvent::GetInfoFailed {
request_id,
counterparty_node_id: *counterparty_node_id,
error,
});

Err(lightning_error)
},
None => {
return Err(LightningError { err: format!("Received error response for a get_info request from an unknown counterparty ({:?})",counterparty_node_id), action: ErrorAction::IgnoreAndLog(Level::Info)});
Expand Down Expand Up @@ -308,8 +325,9 @@ where

fn handle_buy_error(
&self, request_id: LSPSRequestId, counterparty_node_id: &PublicKey,
_error: LSPSResponseError,
error: LSPSResponseError,
) -> Result<(), LightningError> {
let event_queue_notifier = self.pending_events.notifier();
let outer_state_lock = self.per_peer_state.read().unwrap();
match outer_state_lock.get(counterparty_node_id) {
Some(inner_state_lock) => {
Expand All @@ -320,7 +338,21 @@ where
action: ErrorAction::IgnoreAndLog(Level::Info),
})?;

Ok(())
let lightning_error = LightningError {
err: format!(
"Received buy error response for request {:?}: {:?}",
request_id, error
),
action: ErrorAction::IgnoreAndLog(Level::Error),
};

event_queue_notifier.enqueue(LSPS2ClientEvent::BuyRequestFailed {
request_id,
counterparty_node_id: *counterparty_node_id,
error,
});

Err(lightning_error)
},
None => {
return Err(LightningError { err: format!("Received error response for a buy request from an unknown counterparty ({:?})", counterparty_node_id), action: ErrorAction::IgnoreAndLog(Level::Info)});
Expand Down
36 changes: 35 additions & 1 deletion lightning-liquidity/src/lsps2/event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
//! Contains bLIP-52 / LSPS2 event types

use super::msgs::LSPS2OpeningFeeParams;
use crate::lsps0::ser::LSPSRequestId;
use crate::lsps0::ser::{LSPSRequestId, LSPSResponseError};
use alloc::string::String;
use alloc::vec::Vec;

Expand Down Expand Up @@ -61,6 +61,40 @@ pub enum LSPS2ClientEvent {
/// The initial payment size you specified.
payment_size_msat: Option<u64>,
},
/// A request previously issued via [`LSPS2ClientHandler::request_opening_params`]
/// failed as the LSP returned an error response.
///
/// [`LSPS2ClientHandler::request_opening_params`]: crate::lsps2::client::LSPS2ClientHandler::request_opening_params
GetInfoFailed {
/// The identifier of the issued LSPS2 `get_info` request, as returned by
/// [`LSPS2ClientHandler::request_opening_params`].
///
/// This can be used to track which request this event corresponds to.
///
/// [`LSPS2ClientHandler::request_opening_params`]: crate::lsps2::client::LSPS2ClientHandler::request_opening_params
request_id: LSPSRequestId,
/// The node id of the LSP.
counterparty_node_id: PublicKey,
/// The error that was returned.
error: LSPSResponseError,
},
/// 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
BuyRequestFailed {
/// The identifier of the issued LSPS2 `buy` request, as returned by
/// [`LSPS2ClientHandler::select_opening_params`].
///
/// This can be used to track which request this event corresponds to.
///
/// [`LSPS2ClientHandler::select_opening_params`]: crate::lsps2::client::LSPS2ClientHandler::select_opening_params
request_id: LSPSRequestId,
/// The node id of the LSP.
counterparty_node_id: PublicKey,
/// The error that was returned.
error: LSPSResponseError,
},
}

/// An event which an bLIP-52 / LSPS2 server should take some action in response to.
Expand Down
43 changes: 21 additions & 22 deletions lightning-liquidity/src/lsps2/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1348,45 +1348,44 @@ where
&self, peer_state_lock: &mut MutexGuard<'a, PeerState>, request_id: LSPSRequestId,
counterparty_node_id: PublicKey, request: LSPS2Request,
) -> (Result<(), LightningError>, Option<LSPSMessage>) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@tnull tnull May 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The latter.

if self.total_pending_requests.load(Ordering::Relaxed) >= MAX_TOTAL_PENDING_REQUESTS {
let response = LSPS2Response::BuyError(LSPSResponseError {
let create_pending_request_limit_exceeded_response = |error_message: String| {
let error_details = LSPSResponseError {
code: LSPS0_CLIENT_REJECTED_ERROR_CODE,
message: "Reached maximum number of pending requests. Please try again later."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.to_string(),
data: None,
};
let response = match &request {
LSPS2Request::GetInfo(_) => LSPS2Response::GetInfoError(error_details),
LSPS2Request::Buy(_) => LSPS2Response::BuyError(error_details),
};
let msg = Some(LSPS2Message::Response(request_id.clone(), response).into());

let result = Err(LightningError {
err: error_message,
action: ErrorAction::IgnoreAndLog(Level::Debug),
});
let msg = Some(LSPS2Message::Response(request_id, response).into());
(result, msg)
};

let err = format!(
"Peer {} reached maximum number of total pending requests: {}",
counterparty_node_id, MAX_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: {}",
MAX_TOTAL_PENDING_REQUESTS
);
let result =
Err(LightningError { err, action: ErrorAction::IgnoreAndLog(Level::Debug) });
return (result, msg);
return create_pending_request_limit_exceeded_response(error_message);
}

if peer_state_lock.pending_requests_and_channels() < MAX_PENDING_REQUESTS_PER_PEER {
peer_state_lock.pending_requests.insert(request_id, request);
self.total_pending_requests.fetch_add(1, Ordering::Relaxed);
(Ok(()), None)
} else {
let response = LSPS2Response::BuyError(LSPSResponseError {
code: LSPS0_CLIENT_REJECTED_ERROR_CODE,
message: "Reached maximum number of pending requests. Please try again later."
.to_string(),
data: None,
});
let msg = Some(LSPS2Message::Response(request_id, response).into());

let err = format!(
let error_message = format!(
"Peer {} reached maximum number of pending requests: {}",
counterparty_node_id, MAX_PENDING_REQUESTS_PER_PEER
);
let result =
Err(LightningError { err, action: ErrorAction::IgnoreAndLog(Level::Debug) });

(result, msg)
create_pending_request_limit_exceeded_response(error_message)
}
}

Expand Down
Loading
Loading