Merge pull request lightningdevkit#3975 from martinsaposnic/fix-lsps5-notification-cooldown-logic

LSPS5: Correct notification cooldown & reset logic

Author: tnull

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,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

@@ -51,7 +51,11 @@ struct StoredWebhook {
 	_app_name: LSPS5AppName,
 	url: LSPS5WebhookUrl,
 	_counterparty_node_id: PublicKey,
+	// Timestamp used for tracking when the webhook was created / updated, or when the last notification was sent.
+	// This is used to determine if the webhook is stale and should be pruned.
 	last_used: LSPSDateTime,
+	// Map of last notification sent timestamps for each notification method.
+	// This is used to enforce notification cooldowns.
 	last_notification_sent: HashMap<WebhookNotificationMethod, LSPSDateTime>,
 }
 
@@ -60,8 +64,18 @@ struct StoredWebhook {
 pub struct LSPS5ServiceConfig {
 	/// Maximum number of webhooks allowed per client.
 	pub max_webhooks_per_client: u32,
-	/// Minimum time between sending the same notification type in hours (default: 24)
-	pub notification_cooldown_hours: Duration,
+}
+
+/// Default maximum number of webhooks allowed per client.
+pub const DEFAULT_MAX_WEBHOOKS_PER_CLIENT: u32 = 10;
+/// Default notification cooldown time in hours.
+pub const DEFAULT_NOTIFICATION_COOLDOWN_HOURS: Duration = Duration::from_secs(60 * 60); // 1 hour
+
+// Default configuration for LSPS5 service.
+impl Default for LSPS5ServiceConfig {
+	fn default() -> Self {
+		Self { max_webhooks_per_client: DEFAULT_MAX_WEBHOOKS_PER_CLIENT }
+	}
 }
 
 /// Service-side handler for the [`bLIP-55 / LSPS5`] webhook registration protocol.
@@ -78,8 +92,6 @@ pub struct 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,
@@ -94,7 +106,6 @@ pub struct 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
@@ -318,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)
@@ -335,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> {
@@ -353,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> {
@@ -370,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> {
@@ -394,24 +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 >= self.config.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(())
 	}
 
@@ -487,6 +524,15 @@ where
 			.iter()
 			.any(|c| c.is_usable && c.counterparty.node_id == *client_id)
 	}
+
+	pub(crate) fn peer_connected(&self, counterparty_node_id: &PublicKey) {
+		let mut webhooks = self.webhooks.lock().unwrap();
+		if let Some(client_webhooks) = webhooks.get_mut(counterparty_node_id) {
+			for webhook in client_webhooks.values_mut() {
+				webhook.last_notification_sent.clear();
+			}
+		}
+	}
 }
 
 impl<CM: Deref, NS: Deref, TP: Deref> LSPSProtocolMessageHandler for LSPS5ServiceHandler<CM, NS, TP>

lightning-liquidity/src/manager.rs

@@ -714,8 +714,13 @@ where
 		}
 	}
 	fn peer_connected(
-		&self, _: bitcoin::secp256k1::PublicKey, _: &lightning::ln::msgs::Init, _: bool,
+		&self, counterparty_node_id: bitcoin::secp256k1::PublicKey, _: &lightning::ln::msgs::Init,
+		_: bool,
 	) -> Result<(), ()> {
+		if let Some(lsps5_service_handler) = self.lsps5_service_handler.as_ref() {
+			lsps5_service_handler.peer_connected(&counterparty_node_id);
+		}
+
 		Ok(())
 	}
 }

lightning-liquidity/tests/lsps0_integration_tests.rs

@@ -23,7 +23,6 @@ use lightning::ln::functional_test_utils::{
 use lightning::ln::peer_handler::CustomMessageHandler;
 
 use std::sync::Arc;
-use std::time::Duration;
 
 #[test]
 fn list_protocols_integration_test() {
@@ -36,10 +35,7 @@ fn list_protocols_integration_test() {
 	let lsps2_service_config = LSPS2ServiceConfig { promise_secret };
 	#[cfg(lsps1_service)]
 	let lsps1_service_config = LSPS1ServiceConfig { supported_options: None, token: None };
-	let lsps5_service_config = LSPS5ServiceConfig {
-		max_webhooks_per_client: 10,
-		notification_cooldown_hours: Duration::from_secs(3600),
-	};
+	let lsps5_service_config = LSPS5ServiceConfig::default();
 	let service_config = LiquidityServiceConfig {
 		#[cfg(lsps1_service)]
 		lsps1_service_config: Some(lsps1_service_config),