fixup: introduce SLOW_DOWN error so we don't silently fail to send a notification

Author: martinsaposnic

lightning-liquidity/src/lsps5/event.rs

@@ -31,15 +31,13 @@ pub enum LSPS5ServiceEvent {
 	/// The LSP should send an HTTP POST to the [`url`], using the
 	/// JSON-serialized [`notification`] as the body and including the `headers`.
 	/// If the HTTP request fails, the LSP may implement a retry policy according to its
-	/// implementation preferences, but must respect rate-limiting as defined in
-	/// [`notification_cooldown_hours`].
+	/// implementation preferences.
 	///
 	/// The notification is signed using the LSP's node ID to ensure authenticity
 	/// when received by the client. The client verifies this signature using
 	/// [`validate`], which guards against replay attacks and tampering.
 	///
 	/// [`validate`]: super::validator::LSPS5Validator::validate
-	/// [`notification_cooldown_hours`]: super::service::LSPS5ServiceConfig::notification_cooldown_hours
 	/// [`url`]: super::msgs::LSPS5WebhookUrl
 	/// [`notification`]: super::msgs::WebhookNotification
 	SendWebhookNotification {

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,6 +105,14 @@ 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::LSPS5ServiceConfig::DEFAULT_NOTIFICATION_COOLDOWN_HOURS
+	SlowDownError,
 }
 
 impl LSPS5ProtocolError {
@@ -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

@@ -92,8 +92,6 @@ impl Default for LSPS5ServiceConfig {
 ///   - `lsps5.remove_webhook` -> delete a named webhook or return [`app_name_not_found`] error.
 /// - Prune stale webhooks after a client has no open channels and no activity for at least
 /// [`MIN_WEBHOOK_RETENTION_DAYS`].
-/// - Rate-limit repeat notifications of the same method to a client by
-///   [`notification_cooldown_hours`].
 /// - Sign and enqueue outgoing webhook notifications:
 ///   - Construct JSON-RPC 2.0 Notification objects [`WebhookNotification`],
 ///   - Timestamp and LN-style zbase32-sign each payload,
@@ -108,7 +106,6 @@ impl Default for LSPS5ServiceConfig {
 /// [`bLIP-55 / LSPS5`]: https://github.com/lightning/blips/pull/55/files
 /// [`max_webhooks_per_client`]: super::service::LSPS5ServiceConfig::max_webhooks_per_client
 /// [`app_name_not_found`]: super::msgs::LSPS5ProtocolError::AppNameNotFound
-/// [`notification_cooldown_hours`]: super::service::LSPS5ServiceConfig::notification_cooldown_hours
 /// [`WebhookNotification`]: super::msgs::WebhookNotification
 /// [`LSPS5ServiceEvent::SendWebhookNotification`]: super::event::LSPS5ServiceEvent::SendWebhookNotification
 /// [`app_name`]: super::msgs::LSPS5AppName
@@ -224,23 +221,27 @@ where
 		}
 
 		if !no_change {
-			self.send_webhook_registered_notification(
+			let result = self.send_webhook_registered_notification(
 				counterparty_node_id,
 				params.app_name,
 				params.webhook,
-			)
-			.map_err(|e| {
+			);
+
+			// If the send_notification failed because of a SLOW_DOWN_ERROR, it means we sent this
+			// notification recently, and the user has not seen it yet. It's safe to continue, but we still need to handle other error types.
+			if result.is_err() && !matches!(result, Err(LSPS5ProtocolError::SlowDownError)) {
+				let e = result.unwrap_err();
 				let msg = LSPS5Message::Response(
 					request_id.clone(),
 					LSPS5Response::SetWebhookError(e.clone().into()),
 				)
 				.into();
 				self.pending_messages.enqueue(&counterparty_node_id, msg);
-				LightningError {
+				return Err(LightningError {
 					err: e.message().into(),
 					action: ErrorAction::IgnoreAndLog(Level::Info),
-				}
-			})?;
+				});
+			}
 		}
 
 		let msg = LSPS5Message::Response(
@@ -326,10 +327,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::LSPS5ServiceConfig::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)
@@ -343,11 +348,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::LSPS5ServiceConfig::DEFAULT_NOTIFICATION_COOLDOWN_HOURS
 	pub fn notify_expiry_soon(
 		&self, client_id: PublicKey, timeout: u32,
 	) -> Result<(), LSPS5ProtocolError> {
@@ -361,10 +370,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::LSPS5ServiceConfig::DEFAULT_NOTIFICATION_COOLDOWN_HOURS
 	pub fn notify_liquidity_management_request(
 		&self, client_id: PublicKey,
 	) -> Result<(), LSPS5ProtocolError> {
@@ -378,10 +391,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::LSPS5ServiceConfig::DEFAULT_NOTIFICATION_COOLDOWN_HOURS
 	pub fn notify_onion_message_incoming(
 		&self, client_id: PublicKey,
 	) -> Result<(), LSPS5ProtocolError> {
@@ -402,22 +419,27 @@ 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
+		let rate_limit_applies = client_webhooks.iter().any(|(_, webhook)| {
+			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(),
-				)?;
-			}
+				.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"