Use `PeerState::{register,remove}_request` instead of map access
What the developer wrote
Authored by Elias Rohrer
Use `PeerState::{register,remove}_request` instead of map access
We introduce two new methods on `PeerState` to avoid direct access to
the internal `pending_requests` map.
What changed, and why it matters
This commit refactors how LSPS1 (a liquidity service protocol) tracks pending requests. Previously, code directly inserted or removed entries from an internal map. Now it uses two controlled methods that check for duplicate request IDs and unknown request IDs, returning clear errors instead of silently overwriting or missing entries. This is a defensive hardening change that reduces the chance of request-state confusion, but the commit itself does not claim to fix a specific exploitable vulnerability.
Treat as a defensive hardening patch. Review whether duplicate request IDs could previously be exploited to confuse service state or bypass order validation. Monitor the noted TODO regarding `order_state` tracking, as that may represent a separate correctness or security concern. No urgent deployment is indicated solely by this diff.
Security signals we found
Prevents silent overwrite of pending requests by duplicate request IDs
Adds explicit error handling for unknown request IDs during response handling
Reduces direct mutable access to internal state map
Adds debug_asserts for unexpected request types in response path
TODO comment indicates related state-management issue remains unresolved
Evidence from the diff
The change makes PeerState.pending_requests private and adds register_request and remove_request methods. register_request rejects duplicate LSPSRequestId values instead of silently overwriting. remove_request returns an explicit PeerStateError::UnknownRequestId instead of an Option. Callers in service.rs now propagate these errors as LightningError with IgnoreAndLog(Error) or APIError::APIMisuseError. The refactor also tightens the response path so an unexpected pending-request type triggers a dedicated error rather than falling through a catch-all. A TODO comment notes that order_state still needs to be tracked properly in peer/channel state.
Changed components
lightning-liquidity/src/lsps1/peer_state.rslightning-liquidity/src/lsps1/service.rsLSPS1 service request/response handlingInspect captured patch +71 / −16
diff --git a/lightning-liquidity/src/lsps1/peer_state.rs b/lightning-liquidity/src/lsps1/peer_state.rs
index 172ace6..9adc3c9 100644
--- a/lightning-liquidity/src/lsps1/peer_state.rs
+++ b/lightning-liquidity/src/lsps1/peer_state.rs
@@ -14,10 +14,12 @@ use super::msgs::{LSPS1OrderId, LSPS1OrderParams, LSPS1PaymentInfo, LSPS1Request
use crate::lsps0::ser::{LSPSDateTime, LSPSRequestId};
use crate::prelude::HashMap;
+use core::fmt;
+
#[derive(Default)]
pub(super) struct PeerState {
outbound_channels_by_order_id: HashMap<LSPS1OrderId, OutboundCRChannel>,
- pub(super) pending_requests: HashMap<LSPSRequestId, LSPS1Request>,
+ pending_requests: HashMap<LSPSRequestId, LSPS1Request>,
}
impl PeerState {
@@ -33,11 +35,42 @@ impl PeerState {
self.outbound_channels_by_order_id.get(order_id).map(|channel| &channel.order)
}
+ pub(super) fn register_request(
+ &mut self, request_id: LSPSRequestId, request: LSPS1Request,
+ ) -> Result<(), PeerStateError> {
+ if self.pending_requests.contains_key(&request_id) {
+ return Err(PeerStateError::DuplicateRequestId);
+ }
+ self.pending_requests.insert(request_id, request);
+ Ok(())
+ }
+
+ pub(super) fn remove_request(
+ &mut self, request_id: &LSPSRequestId,
+ ) -> Result<LSPS1Request, PeerStateError> {
+ self.pending_requests.remove(request_id).ok_or(PeerStateError::UnknownRequestId)
+ }
+
pub(super) fn has_active_orders(&self) -> bool {
!self.outbound_channels_by_order_id.is_empty()
}
}
+#[derive(Debug, Copy, Clone)]
+pub(super) enum PeerStateError {
+ UnknownRequestId,
+ DuplicateRequestId,
+}
+
+impl fmt::Display for PeerStateError {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ match self {
+ Self::UnknownRequestId => write!(f, "unknown request id"),
+ Self::DuplicateRequestId => write!(f, "duplicate request id"),
+ }
+ }
+}
+
pub(super) struct ChannelOrder {
pub(super) order_params: LSPS1OrderParams,
pub(super) created_at: LSPSDateTime,
diff --git a/lightning-liquidity/src/lsps1/service.rs b/lightning-liquidity/src/lsps1/service.rs
index 0e5eacc..a75db34 100644
--- a/lightning-liquidity/src/lsps1/service.rs
+++ b/lightning-liquidity/src/lsps1/service.rs
@@ -157,9 +157,12 @@ where
.or_insert(Mutex::new(PeerState::default()));
let mut peer_state_lock = inner_state_lock.lock().unwrap();
- peer_state_lock
- .pending_requests
- .insert(request_id.clone(), LSPS1Request::CreateOrder(params.clone()));
+ let request = LSPS1Request::CreateOrder(params.clone());
+ peer_state_lock.register_request(request_id.clone(), request).map_err(|e| {
+ let err = format!("Failed to handle request due to: {}", e);
+ let action = ErrorAction::IgnoreAndLog(Level::Error);
+ LightningError { err, action }
+ })?;
}
event_queue_notifier.enqueue(LSPS1ServiceEvent::RequestForPaymentDetails {
@@ -186,11 +189,15 @@ where
match outer_state_lock.get(counterparty_node_id) {
Some(inner_state_lock) => {
let mut peer_state_lock = inner_state_lock.lock().unwrap();
-
- match peer_state_lock.pending_requests.remove(&request_id) {
- Some(LSPS1Request::CreateOrder(params)) => {
+ let request = peer_state_lock.remove_request(&request_id).map_err(|e| {
+ debug_assert!(false, "Failed to send response due to: {}", e);
+ let err = format!("Failed to send response due to: {}", e);
+ APIError::APIMisuseError { err }
+ })?;
+
+ match request {
+ LSPS1Request::CreateOrder(params) => {
let order_id = self.generate_order_id();
-
peer_state_lock.new_order(
order_id.clone(),
params.order.clone(),
@@ -201,6 +208,9 @@ where
let response = LSPS1Response::CreateOrder(LSPS1CreateOrderResponse {
order: params.order,
order_id,
+
+ // TODO, we need to set this in the peer/channel state, and send the
+ // set value here:
order_state: LSPS1OrderState::Created,
created_at,
payment,
@@ -210,14 +220,22 @@ where
message_queue_notifier.enqueue(counterparty_node_id, msg);
Ok(())
},
-
- _ => Err(APIError::APIMisuseError {
- err: format!("No pending buy request for request_id: {:?}", request_id),
- }),
+ t => {
+ debug_assert!(
+ false,
+ "Failed to send response due to unexpected request type: {:?}",
+ t
+ );
+ let err = format!(
+ "Failed to send response due to unexpected request type: {:?}",
+ t
+ );
+ return Err(APIError::APIMisuseError { err });
+ },
}
},
None => Err(APIError::APIMisuseError {
- err: format!("No state for the counterparty exists: {:?}", counterparty_node_id),
+ err: format!("No state for the counterparty exists: {}", counterparty_node_id),
}),
}
}
@@ -231,9 +249,13 @@ where
match outer_state_lock.get(counterparty_node_id) {
Some(inner_state_lock) => {
let mut peer_state_lock = inner_state_lock.lock().unwrap();
- peer_state_lock
- .pending_requests
- .insert(request_id.clone(), LSPS1Request::GetOrder(params.clone()));
+
+ let request = LSPS1Request::GetOrder(params.clone());
+ peer_state_lock.register_request(request_id.clone(), request).map_err(|e| {
+ let err = format!("Failed to handle request due to: {}", e);
+ let action = ErrorAction::IgnoreAndLog(Level::Error);
+ LightningError { err, action }
+ })?;
event_queue_notifier.enqueue(LSPS1ServiceEvent::CheckPaymentConfirmation {
request_id,
Why this scored 25/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.