introduce SLOW_DOWN error so we don't silently fail to send an LSPS5/notification

Author: martinsaposnic

lightning-liquidity/src/lsps5/msgs.rs

@@ -51,6 +51,8 @@ pub const LSPS5_APP_NAME_NOT_FOUND_ERROR_CODE: i32 = 1010;
 pub const LSPS5_UNKNOWN_ERROR_CODE: i32 = 1000;
 /// An error occurred during serialization of LSPS5 webhook notification.
 pub const LSPS5_SERIALIZATION_ERROR_CODE: i32 = 1001;
+/// A notification was sent too frequently.
+pub const LSPS5_SLOW_DOWN_ERROR_CODE: i32 = 1002;
 
 pub(crate) const LSPS5_SET_WEBHOOK_METHOD_NAME: &str = "lsps5.set_webhook";
 pub(crate) const LSPS5_LIST_WEBHOOKS_METHOD_NAME: &str = "lsps5.list_webhooks";
@@ -103,10 +105,18 @@ pub enum LSPS5ProtocolError {
 
 	/// Error during serialization of LSPS5 webhook notification.
 	SerializationError,
+
+	/// A notification was sent too frequently.
+	///
+	/// This error indicates that the LSP is sending notifications
+	/// too quickly, violating the notification cooldown [`DEFAULT_NOTIFICATION_COOLDOWN_HOURS`]
+	///
+	/// [`DEFAULT_NOTIFICATION_COOLDOWN_HOURS`]: super::service::DEFAULT_NOTIFICATION_COOLDOWN_HOURS
+	SlowDownError,
 }
 
 impl LSPS5ProtocolError {
-	/// private code range so we never collide with the spec's codes
+	/// The error code for the LSPS5 protocol error.
 	pub fn code(&self) -> i32 {
 		match self {
 			LSPS5ProtocolError::AppNameTooLong | LSPS5ProtocolError::WebhookUrlTooLong => {
@@ -118,6 +128,7 @@ impl LSPS5ProtocolError {
 			LSPS5ProtocolError::AppNameNotFound => LSPS5_APP_NAME_NOT_FOUND_ERROR_CODE,
 			LSPS5ProtocolError::UnknownError => LSPS5_UNKNOWN_ERROR_CODE,
 			LSPS5ProtocolError::SerializationError => LSPS5_SERIALIZATION_ERROR_CODE,
+			LSPS5ProtocolError::SlowDownError => LSPS5_SLOW_DOWN_ERROR_CODE,
 		}
 	}
 	/// The error message for the LSPS5 protocol error.
@@ -133,6 +144,7 @@ impl LSPS5ProtocolError {
 			LSPS5ProtocolError::SerializationError => {
 				"Error serializing LSPS5 webhook notification"
 			},
+			LSPS5ProtocolError::SlowDownError => "Notification sent too frequently",
 		}
 	}
 }

lightning-liquidity/src/lsps5/service.rs

@@ -329,10 +329,14 @@ where
 	/// This builds a [`WebhookNotificationMethod::LSPS5PaymentIncoming`] webhook notification, signs it with your
 	/// node key, and enqueues HTTP POSTs to all registered webhook URLs for that client.
 	///
+	/// This may fail if a similar notification was sent too recently,
+	/// violating the notification cooldown period defined in [`DEFAULT_NOTIFICATION_COOLDOWN_HOURS`].
+	///
 	/// # Parameters
 	/// - `client_id`: the client's node-ID whose webhooks should be invoked.
 	///
 	/// [`WebhookNotificationMethod::LSPS5PaymentIncoming`]: super::msgs::WebhookNotificationMethod::LSPS5PaymentIncoming
+	/// [`DEFAULT_NOTIFICATION_COOLDOWN_HOURS`]: super::service::DEFAULT_NOTIFICATION_COOLDOWN_HOURS
 	pub fn notify_payment_incoming(&self, client_id: PublicKey) -> Result<(), LSPS5ProtocolError> {
 		let notification = WebhookNotification::payment_incoming();
 		self.send_notifications_to_client_webhooks(client_id, notification)
@@ -346,11 +350,15 @@ where
 	/// the `timeout` block height, signs it, and enqueues HTTP POSTs to the client's
 	/// registered webhooks.
 	///
+	/// This may fail if a similar notification was sent too recently,
+	/// violating the notification cooldown period defined in [`DEFAULT_NOTIFICATION_COOLDOWN_HOURS`].
+	///
 	/// # Parameters
 	/// - `client_id`: the client's node-ID whose webhooks should be invoked.
 	/// - `timeout`: the block height at which the channel contract will expire.
 	///
 	/// [`WebhookNotificationMethod::LSPS5ExpirySoon`]: super::msgs::WebhookNotificationMethod::LSPS5ExpirySoon
+	/// [`DEFAULT_NOTIFICATION_COOLDOWN_HOURS`]: super::service::DEFAULT_NOTIFICATION_COOLDOWN_HOURS
 	pub fn notify_expiry_soon(
 		&self, client_id: PublicKey, timeout: u32,
 	) -> Result<(), LSPS5ProtocolError> {
@@ -364,10 +372,14 @@ where
 	/// liquidity for `client_id`. Builds a [`WebhookNotificationMethod::LSPS5LiquidityManagementRequest`] notification,
 	/// signs it, and sends it to all of the client's registered webhook URLs.
 	///
+	/// This may fail if a similar notification was sent too recently,
+	/// violating the notification cooldown period defined in [`DEFAULT_NOTIFICATION_COOLDOWN_HOURS`].
+	///
 	/// # Parameters
 	/// - `client_id`: the client's node-ID whose webhooks should be invoked.
 	///
 	/// [`WebhookNotificationMethod::LSPS5LiquidityManagementRequest`]: super::msgs::WebhookNotificationMethod::LSPS5LiquidityManagementRequest
+	/// [`DEFAULT_NOTIFICATION_COOLDOWN_HOURS`]: super::service::DEFAULT_NOTIFICATION_COOLDOWN_HOURS
 	pub fn notify_liquidity_management_request(
 		&self, client_id: PublicKey,
 	) -> Result<(), LSPS5ProtocolError> {
@@ -381,10 +393,14 @@ where
 	/// for `client_id` while the client is offline. Builds a [`WebhookNotificationMethod::LSPS5OnionMessageIncoming`]
 	/// notification, signs it, and enqueues HTTP POSTs to each registered webhook.
 	///
+	/// This may fail if a similar notification was sent too recently,
+	/// violating the notification cooldown period defined in [`DEFAULT_NOTIFICATION_COOLDOWN_HOURS`].
+	///
 	/// # Parameters
 	/// - `client_id`: the client's node-ID whose webhooks should be invoked.
 	///
 	/// [`WebhookNotificationMethod::LSPS5OnionMessageIncoming`]: super::msgs::WebhookNotificationMethod::LSPS5OnionMessageIncoming
+	/// [`DEFAULT_NOTIFICATION_COOLDOWN_HOURS`]: super::service::DEFAULT_NOTIFICATION_COOLDOWN_HOURS
 	pub fn notify_onion_message_incoming(
 		&self, client_id: PublicKey,
 	) -> Result<(), LSPS5ProtocolError> {
@@ -405,23 +421,34 @@ where
 		let now =
 			LSPSDateTime::new_from_duration_since_epoch(self.time_provider.duration_since_epoch());
 
-		for (app_name, webhook) in client_webhooks.iter_mut() {
-			if webhook
-				.last_notification_sent
-				.get(&notification.method)
-				.map(|last_sent| now.clone().abs_diff(&last_sent))
-				.map_or(true, |duration| duration >= DEFAULT_NOTIFICATION_COOLDOWN_HOURS.as_secs())
-			{
-				webhook.last_notification_sent.insert(notification.method.clone(), now.clone());
-				webhook.last_used = now.clone();
-				self.send_notification(
-					client_id,
-					app_name.clone(),
-					webhook.url.clone(),
-					notification.clone(),
-				)?;
+		// We must avoid sending multiple notifications of the same method
+		// (other than lsps5.webhook_registered) close in time.
+		if notification.method != WebhookNotificationMethod::LSPS5WebhookRegistered {
+			let rate_limit_applies = client_webhooks.iter().any(|(_, webhook)| {
+				webhook
+					.last_notification_sent
+					.get(&notification.method)
+					.map(|last_sent| now.abs_diff(&last_sent))
+					.map_or(false, |duration| {
+						duration < DEFAULT_NOTIFICATION_COOLDOWN_HOURS.as_secs()
+					})
+			});
+
+			if rate_limit_applies {
+				return Err(LSPS5ProtocolError::SlowDownError);
 			}
 		}
+
+		for (app_name, webhook) in client_webhooks.iter_mut() {
+			webhook.last_notification_sent.insert(notification.method.clone(), now.clone());
+			webhook.last_used = now.clone();
+			self.send_notification(
+				client_id,
+				app_name.clone(),
+				webhook.url.clone(),
+				notification.clone(),
+			)?;
+		}
 		Ok(())
 	}
 

lightning-liquidity/tests/lsps5_integration_tests.rs

@@ -1093,7 +1093,9 @@ fn test_send_notifications_and_peer_connected_resets_cooldown() {
 	}
 
 	// 2. Second notification before cooldown should NOT be sent
-	let _ = service_handler.notify_payment_incoming(client_node_id);
+	let result = service_handler.notify_payment_incoming(client_node_id);
+	let error = result.unwrap_err();
+	assert_eq!(error, LSPS5ProtocolError::SlowDownError);
 	assert!(
 		service_node.liquidity_manager.next_event().is_none(),
 		"Should not emit event due to cooldown"
@@ -1132,7 +1134,11 @@ fn test_send_notifications_and_peer_connected_resets_cooldown() {
 	}
 
 	// 5. Can't send payment_incoming notification again immediately after cooldown
-	let _ = service_handler.notify_payment_incoming(client_node_id);
+	let result = service_handler.notify_payment_incoming(client_node_id);
+
+	let error = result.unwrap_err();
+	assert_eq!(error, LSPS5ProtocolError::SlowDownError);
+
 	assert!(
 		service_node.liquidity_manager.next_event().is_none(),
 		"Should not emit event due to cooldown"