fixup: make OutboundJITChannelState private, and do bool helper for checking if lsps2 has any pendingChannelOpen or above

Author: martinsaposnic

lightning-liquidity/src/lsps2/service.rs

@@ -107,18 +107,9 @@ struct ForwardPaymentAction(ChannelId, FeePayment);
 #[derive(Debug, PartialEq)]
 struct ForwardHTLCsAction(ChannelId, Vec<InterceptedHTLC>);
 
-#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd)]
-pub(crate) enum OutboundJITStage {
-	PendingInitialPayment,
-	PendingChannelOpen,
-	PendingPaymentForward,
-	PendingPayment,
-	PaymentForwarded,
-}
-
 /// The different states a requested JIT channel can be in.
-#[derive(Debug, PartialEq, Eq, Clone)]
-pub(crate) enum OutboundJITChannelState {
+#[derive(Debug)]
+enum OutboundJITChannelState {
 	/// The JIT channel SCID was created after a buy request, and we are awaiting an initial payment
 	/// of sufficient size to open the channel.
 	PendingInitialPayment { payment_queue: PaymentQueue },
@@ -143,36 +134,6 @@ pub(crate) enum OutboundJITChannelState {
 	PaymentForwarded { channel_id: ChannelId },
 }
 
-impl OutboundJITChannelState {
-	pub(crate) fn stage(&self) -> OutboundJITStage {
-		match self {
-			OutboundJITChannelState::PendingInitialPayment { .. } => {
-				OutboundJITStage::PendingInitialPayment
-			},
-			OutboundJITChannelState::PendingChannelOpen { .. } => {
-				OutboundJITStage::PendingChannelOpen
-			},
-			OutboundJITChannelState::PendingPaymentForward { .. } => {
-				OutboundJITStage::PendingPaymentForward
-			},
-			OutboundJITChannelState::PendingPayment { .. } => OutboundJITStage::PendingPayment,
-			OutboundJITChannelState::PaymentForwarded { .. } => OutboundJITStage::PaymentForwarded,
-		}
-	}
-}
-
-impl PartialOrd for OutboundJITChannelState {
-	fn partial_cmp(&self, other: &Self) -> Option<CmpOrdering> {
-		Some(self.cmp(other))
-	}
-}
-
-impl Ord for OutboundJITChannelState {
-	fn cmp(&self, other: &Self) -> core::cmp::Ordering {
-		self.stage().cmp(&other.stage())
-	}
-}
-
 impl OutboundJITChannelState {
 	fn new() -> Self {
 		OutboundJITChannelState::PendingInitialPayment { payment_queue: PaymentQueue::new() }
@@ -605,13 +566,20 @@ where
 		&self.config
 	}
 
-	pub(crate) fn highest_state_for_peer(
-		&self, counterparty_node_id: &PublicKey,
-	) -> Option<OutboundJITChannelState> {
+	/// Returns whether the peer has any opening or open JIT channels.
+	pub(crate) fn has_opening_or_open_jit_channel(&self, counterparty_node_id: &PublicKey) -> bool {
 		let outer_state_lock = self.per_peer_state.read().unwrap();
-		outer_state_lock.get(counterparty_node_id).and_then(|inner| {
+		outer_state_lock.get(counterparty_node_id).map_or(false, |inner| {
 			let peer_state = inner.lock().unwrap();
-			peer_state.outbound_channels_by_intercept_scid.values().map(|c| c.state.clone()).max()
+			peer_state.outbound_channels_by_intercept_scid.values().any(|chan| {
+				matches!(
+					chan.state,
+					OutboundJITChannelState::PendingChannelOpen { .. }
+						| OutboundJITChannelState::PendingPaymentForward { .. }
+						| OutboundJITChannelState::PendingPayment { .. }
+						| OutboundJITChannelState::PaymentForwarded { .. }
+				)
+			})
 		})
 	}
 
@@ -1948,55 +1916,4 @@ mod tests {
 			);
 		}
 	}
-
-	#[test]
-	fn highest_state_for_peer_orders() {
-		let opening_fee_params = LSPS2OpeningFeeParams {
-			min_fee_msat: 0,
-			proportional: 0,
-			valid_until: LSPSDateTime::from_str("1970-01-01T00:00:00Z").unwrap(),
-			min_lifetime: 0,
-			max_client_to_self_delay: 0,
-			min_payment_size_msat: 0,
-			max_payment_size_msat: 0,
-			promise: String::new(),
-		};
-
-		let mut map = new_hash_map();
-		map.insert(
-			0,
-			OutboundJITChannel {
-				state: OutboundJITChannelState::PendingInitialPayment {
-					payment_queue: PaymentQueue::new(),
-				},
-				user_channel_id: 0,
-				opening_fee_params: opening_fee_params.clone(),
-				payment_size_msat: None,
-			},
-		);
-		map.insert(
-			1,
-			OutboundJITChannel {
-				state: OutboundJITChannelState::PendingChannelOpen {
-					payment_queue: PaymentQueue::new(),
-					opening_fee_msat: 0,
-				},
-				user_channel_id: 1,
-				opening_fee_params: opening_fee_params.clone(),
-				payment_size_msat: None,
-			},
-		);
-		map.insert(
-			2,
-			OutboundJITChannel {
-				state: OutboundJITChannelState::PaymentForwarded { channel_id: ChannelId([0; 32]) },
-				user_channel_id: 2,
-				opening_fee_params,
-				payment_size_msat: None,
-			},
-		);
-
-		let max_state = map.values().map(|c| c.state.clone()).max().unwrap();
-		assert!(matches!(max_state, OutboundJITChannelState::PaymentForwarded { .. }));
-	}
 }

lightning-liquidity/src/lsps5/service.rs

@@ -21,7 +21,6 @@ use crate::prelude::*;
 use crate::sync::{Arc, Mutex, RwLock, RwLockWriteGuard};
 use crate::utils::time::TimeProvider;
 
-use crate::lsps2::service::{OutboundJITChannelState, OutboundJITStage};
 use bitcoin::secp256k1::PublicKey;
 
 use lightning::ln::channelmanager::AChannelManager;
@@ -153,15 +152,14 @@ where
 	/// Returns whether a request from the given client should be accepted.
 	///
 	/// Prior activity includes an existing open channel, an active LSPS1 flow,
-	/// or an LSPS2 flow that has progressed to at least
-	/// [`OutboundJITChannelState::PendingChannelOpen`].
+	/// or an LSPS2 flow that has an opening or open JIT channel.
 	pub(crate) fn can_accept_request(
-		&self, client_id: &PublicKey, lsps2_max_state: Option<OutboundJITChannelState>,
+		&self, client_id: &PublicKey, lsps2_has_opening_or_open_jit_channel: bool,
 		lsps1_has_activity: bool,
 	) -> bool {
 		self.client_has_open_channel(client_id)
+			|| lsps2_has_opening_or_open_jit_channel
 			|| lsps1_has_activity
-			|| lsps2_max_state.map_or(false, |s| s.stage() >= OutboundJITStage::PendingChannelOpen)
 	}
 
 	fn check_prune_stale_webhooks<'a>(
@@ -502,7 +500,7 @@ where
 			.map_err(|_| LSPS5ProtocolError::UnknownError)
 	}
 
-	pub(crate) fn client_has_open_channel(&self, client_id: &PublicKey) -> bool {
+	fn client_has_open_channel(&self, client_id: &PublicKey) -> bool {
 		self.channel_manager
 			.get_cm()
 			.list_channels()

lightning-liquidity/src/manager.rs

@@ -568,10 +568,10 @@ where
 			LSPSMessage::LSPS5(msg @ LSPS5Message::Request(..)) => {
 				match &self.lsps5_service_handler {
 					Some(lsps5_service_handler) => {
-						let lsps2_max_state = self
+						let lsps2_has_opening_or_open_jit_channel = self
 							.lsps2_service_handler
 							.as_ref()
-							.and_then(|h| h.highest_state_for_peer(sender_node_id));
+							.map_or(false, |h| h.has_opening_or_open_jit_channel(sender_node_id));
 						#[cfg(lsps1_service)]
 						let lsps1_has_active_requests = self
 							.lsps1_service_handler
@@ -582,7 +582,7 @@ where
 
 						if !lsps5_service_handler.can_accept_request(
 							sender_node_id,
-							lsps2_max_state,
+							lsps2_has_opening_or_open_jit_channel,
 							lsps1_has_active_requests,
 						) {
 							return Err(LightningError {

lightning-liquidity/tests/lsps5_integration_tests.rs

@@ -1230,6 +1230,49 @@ fn test_notifications_and_peer_connected_resets_cooldown() {
 	}
 }
 
+macro_rules! assert_lsps5_reject {
+	($client_handler:expr, $service_node:expr, $client_node:expr, $service_node_id:expr, $client_node_id:expr) => {{
+		let _ = $client_handler
+			.set_webhook(
+				$service_node_id,
+				"App".to_string(),
+				"https://example.org/webhook".to_string(),
+			)
+			.expect("Request should send");
+		let request = get_lsps_message!($client_node, $service_node_id);
+
+		let result =
+			$service_node.liquidity_manager.handle_custom_message(request, $client_node_id);
+		assert!(result.is_err(), "Service should reject request without prior interaction");
+
+		assert!($service_node.liquidity_manager.get_and_clear_pending_msg().is_empty());
+	}};
+}
+
+macro_rules! assert_lsps5_accept {
+	($client_handler:expr, $service_node:expr, $client_node:expr, $service_node_id:expr, $client_node_id:expr) => {{
+		let _ = $client_handler
+			.set_webhook(
+				$service_node_id,
+				"App".to_string(),
+				"https://example.org/webhook".to_string(),
+			)
+			.expect("Request should send");
+		let request = get_lsps_message!($client_node, $service_node_id);
+
+		let result =
+			$service_node.liquidity_manager.handle_custom_message(request, $client_node_id);
+		assert!(result.is_ok(), "Service should accept request after prior interaction");
+		let _ = $service_node.liquidity_manager.next_event().unwrap();
+		let response = get_lsps_message!($service_node, $client_node_id);
+		$client_node
+			.liquidity_manager
+			.handle_custom_message(response, $service_node_id)
+			.expect("Client should handle response");
+		let _ = $client_node.liquidity_manager.next_event().unwrap();
+	}};
+}
+
 #[test]
 fn webhook_update_affects_future_notifications() {
 	let mock_time_provider = Arc::new(MockTimeProvider::new(1000));
@@ -1306,51 +1349,26 @@ fn dos_protection() {
 
 	let client_handler = client_node.liquidity_manager.lsps5_client_handler().unwrap();
 
-	let assert_reject = || -> () {
-		let _ = client_handler
-			.set_webhook(
-				service_node_id,
-				"App".to_string(),
-				"https://example.org/webhook".to_string(),
-			)
-			.expect("Request should send");
-		let request = get_lsps_message!(client_node, service_node_id);
-
-		let result = service_node.liquidity_manager.handle_custom_message(request, client_node_id);
-		assert!(result.is_err(), "Service should reject request without prior interaction");
-
-		assert!(service_node.liquidity_manager.get_and_clear_pending_msg().is_empty());
-	};
-
-	let assert_accept = || -> () {
-		let _ = client_handler
-			.set_webhook(
-				service_node_id,
-				"App".to_string(),
-				"https://example.org/webhook".to_string(),
-			)
-			.expect("Request should send");
-		let request = get_lsps_message!(client_node, service_node_id);
-
-		let result = service_node.liquidity_manager.handle_custom_message(request, client_node_id);
-		assert!(result.is_ok(), "Service should accept request after prior interaction");
-		let _ = service_node.liquidity_manager.next_event().unwrap();
-		let response = get_lsps_message!(service_node, client_node_id);
-		client_node
-			.liquidity_manager
-			.handle_custom_message(response, service_node_id)
-			.expect("Client should handle response");
-		let _ = client_node.liquidity_manager.next_event().unwrap();
-	};
-
 	// no channel is open so far -> should reject
-	assert_reject();
+	assert_lsps5_reject!(
+		client_handler,
+		service_node,
+		client_node,
+		service_node_id,
+		client_node_id
+	);
 
 	let (_, _, _, channel_id, funding_tx) =
 		create_chan_between_nodes(&service_node.inner, &client_node.inner);
 
 	// now that a channel is open, should accept
-	assert_accept();
+	assert_lsps5_accept!(
+		client_handler,
+		service_node,
+		client_node,
+		service_node_id,
+		client_node_id
+	);
 
 	close_channel(&service_node.inner, &client_node.inner, &channel_id, funding_tx, true);
 	let node_a_reason = ClosureReason::CounterpartyInitiatedCooperativeClosure;
@@ -1359,7 +1377,13 @@ fn dos_protection() {
 	check_closed_event!(client_node.inner, 1, node_b_reason, [service_node_id], 100000);
 
 	// channel is now closed again -> should reject
-	assert_reject();
+	assert_lsps5_reject!(
+		client_handler,
+		service_node,
+		client_node,
+		service_node_id,
+		client_node_id
+	);
 }
 
 #[test]
@@ -1370,20 +1394,28 @@ fn lsps2_state_allows_lsps5_request() {
 	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
 	let (lsps_nodes, _) = lsps5_lsps2_test_setup(nodes, Arc::new(DefaultTimeProvider));
-	establish_lsps2_prior_interaction(&lsps_nodes);
 
-	let LSPSNodes { service_node, client_node } = lsps_nodes;
-	let service_node_id = service_node.inner.node.get_our_node_id();
-	let client_node_id = client_node.inner.node.get_our_node_id();
+	let client_node_id = lsps_nodes.client_node.inner.node.get_our_node_id();
+	let service_node_id = lsps_nodes.service_node.inner.node.get_our_node_id();
+	let client_handler = lsps_nodes.client_node.liquidity_manager.lsps5_client_handler().unwrap();
 
-	let lsps5_client = client_node.liquidity_manager.lsps5_client_handler().unwrap();
+	assert_lsps5_reject!(
+		client_handler,
+		lsps_nodes.service_node,
+		lsps_nodes.client_node,
+		service_node_id,
+		client_node_id
+	);
 
-	let _ = lsps5_client
-		.set_webhook(service_node_id, "App".to_string(), "https://example.org/webhook".to_string())
-		.expect("Request should send");
-	let request = get_lsps_message!(client_node, service_node_id);
-	let result = service_node.liquidity_manager.handle_custom_message(request, client_node_id);
-	assert!(result.is_ok(), "Service should accept request based on LSPS2 state");
+	establish_lsps2_prior_interaction(&lsps_nodes);
+
+	assert_lsps5_accept!(
+		client_handler,
+		lsps_nodes.service_node,
+		lsps_nodes.client_node,
+		service_node_id,
+		client_node_id
+	);
 }
 
 fn establish_lsps2_prior_interaction(lsps_nodes: &LSPSNodes) {