Prefactor: Simplify last_notification_sent tracking

While bLIP-55 describes that the service should wait at least some
cooldown between sending notifications per individual `method`, there is
nothing that keeps us from simplifying our approach to apply the
cooldown to *any* notifications sent, especially since we just reduced
the cooldown period to 1 minute elsewhere. Here, we therefore simplify
the `last_notification_sent` field to just be a `Option<LSPSDateTime>`.

Author: tnull

lightning-liquidity/src/lsps5/service.rs

@@ -54,9 +54,9 @@ struct Webhook {
 	// 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>,
+	// Timestamp when we last sent a notification to the client. This is used to enforce
+	// notification cooldowns.
+	last_notification_sent: Option<LSPSDateTime>,
 }
 
 /// Server-side configuration options for LSPS5 Webhook Registration.
@@ -184,11 +184,8 @@ where
 		match client_webhooks.entry(params.app_name.clone()) {
 			Entry::Occupied(mut entry) => {
 				no_change = entry.get().url == params.webhook;
-				let (last_used, last_notification_sent) = if no_change {
-					(entry.get().last_used, entry.get().last_notification_sent.clone())
-				} else {
-					(now, new_hash_map())
-				};
+				let last_used = if no_change { entry.get().last_used } else { now };
+				let last_notification_sent = entry.get().last_notification_sent;
 				entry.insert(Webhook {
 					_app_name: params.app_name.clone(),
 					url: params.webhook.clone(),
@@ -217,7 +214,7 @@ where
 					url: params.webhook.clone(),
 					_counterparty_node_id: counterparty_node_id,
 					last_used: now,
-					last_notification_sent: new_hash_map(),
+					last_notification_sent: None,
 				});
 			},
 		}
@@ -425,11 +422,9 @@ where
 		// (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.duration_since(&last_sent))
-					.map_or(false, |duration| duration < NOTIFICATION_COOLDOWN_TIME)
+				webhook.last_notification_sent.as_ref().map_or(false, |last_sent| {
+					now.duration_since(&last_sent) < NOTIFICATION_COOLDOWN_TIME
+				})
 			});
 
 			if rate_limit_applies {
@@ -438,14 +433,14 @@ where
 		}
 
 		for (app_name, webhook) in client_webhooks.iter_mut() {
-			webhook.last_notification_sent.insert(notification.method.clone(), now);
-			webhook.last_used = now;
 			self.send_notification(
 				client_id,
 				app_name.clone(),
 				webhook.url.clone(),
 				notification.clone(),
 			)?;
+			webhook.last_used = now;
+			webhook.last_notification_sent = Some(now);
 		}
 		Ok(())
 	}
@@ -527,7 +522,7 @@ where
 		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();
+				webhook.last_notification_sent = None;
 			}
 		}
 	}

lightning-liquidity/tests/lsps5_integration_tests.rs

@@ -411,11 +411,13 @@ fn webhook_error_handling_test() {
 
 #[test]
 fn webhook_notification_delivery_test() {
+	let mock_time_provider = Arc::new(MockTimeProvider::new(1000));
+	let time_provider = Arc::<MockTimeProvider>::clone(&mock_time_provider);
 	let chanmon_cfgs = create_chanmon_cfgs(2);
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
-	let (lsps_nodes, validator) = lsps5_test_setup(nodes, Arc::new(DefaultTimeProvider));
+	let (lsps_nodes, validator) = lsps5_test_setup(nodes, time_provider);
 	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();
@@ -499,6 +501,8 @@ fn webhook_notification_delivery_test() {
 		"No event should be emitted due to cooldown"
 	);
 
+	mock_time_provider.advance_time(NOTIFICATION_COOLDOWN_TIME.as_secs() + 1);
+
 	let timeout_block = 700000; // Some future block height
 	let _ = service_handler.notify_expiry_soon(client_node_id, timeout_block);
 
@@ -719,11 +723,13 @@ fn idempotency_set_webhook_test() {
 
 #[test]
 fn replay_prevention_test() {
+	let mock_time_provider = Arc::new(MockTimeProvider::new(1000));
+	let time_provider = Arc::<MockTimeProvider>::clone(&mock_time_provider);
 	let chanmon_cfgs = create_chanmon_cfgs(2);
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
-	let (lsps_nodes, validator) = lsps5_test_setup(nodes, Arc::new(DefaultTimeProvider));
+	let (lsps_nodes, validator) = lsps5_test_setup(nodes, time_provider);
 	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();
@@ -774,6 +780,9 @@ fn replay_prevention_test() {
 
 	// Fill up the validator's signature cache to push out the original signature.
 	for i in 0..MAX_RECENT_SIGNATURES {
+		// Advance time, allowing for another notification
+		mock_time_provider.advance_time(NOTIFICATION_COOLDOWN_TIME.as_secs() + 1);
+
 		let timeout_block = 700000 + i as u32;
 		let _ = service_handler.notify_expiry_soon(client_node_id, timeout_block);
 		let event = service_node.liquidity_manager.next_event().unwrap();
@@ -871,11 +880,13 @@ fn stale_webhooks() {
 
 #[test]
 fn test_all_notifications() {
+	let mock_time_provider = Arc::new(MockTimeProvider::new(1000));
+	let time_provider = Arc::<MockTimeProvider>::clone(&mock_time_provider);
 	let chanmon_cfgs = create_chanmon_cfgs(2);
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
-	let (lsps_nodes, validator) = lsps5_test_setup(nodes, Arc::new(DefaultTimeProvider));
+	let (lsps_nodes, validator) = lsps5_test_setup(nodes, time_provider);
 	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();
@@ -894,9 +905,16 @@ fn test_all_notifications() {
 	// consume initial SendWebhookNotification
 	let _ = service_node.liquidity_manager.next_event().unwrap();
 
+	mock_time_provider.advance_time(NOTIFICATION_COOLDOWN_TIME.as_secs() + 1);
 	let _ = service_handler.notify_onion_message_incoming(client_node_id);
+
+	mock_time_provider.advance_time(NOTIFICATION_COOLDOWN_TIME.as_secs() + 1);
 	let _ = service_handler.notify_payment_incoming(client_node_id);
+
+	mock_time_provider.advance_time(NOTIFICATION_COOLDOWN_TIME.as_secs() + 1);
 	let _ = service_handler.notify_expiry_soon(client_node_id, 1000);
+
+	mock_time_provider.advance_time(NOTIFICATION_COOLDOWN_TIME.as_secs() + 1);
 	let _ = service_handler.notify_liquidity_management_request(client_node_id);
 
 	let expected_notifications = vec![
@@ -1101,24 +1119,7 @@ fn test_send_notifications_and_peer_connected_resets_cooldown() {
 		"Should not emit event due to cooldown"
 	);
 
-	// 3. Notification of a different method CAN be sent
-	let timeout_block = 424242;
-	let _ = service_handler.notify_expiry_soon(client_node_id, timeout_block);
-	let event = service_node.liquidity_manager.next_event().unwrap();
-	match event {
-		LiquidityEvent::LSPS5Service(LSPS5ServiceEvent::SendWebhookNotification {
-			notification,
-			..
-		}) => {
-			assert!(matches!(
-				notification.method,
-				WebhookNotificationMethod::LSPS5ExpirySoon { timeout } if timeout == timeout_block
-			));
-		},
-		_ => panic!("Expected SendWebhookNotification event for expiry_soon"),
-	}
-
-	// 4. Advance time past cooldown and ensure payment_incoming can be sent again
+	// 3. Advance time past cooldown and ensure payment_incoming can be sent again
 	mock_time_provider.advance_time(NOTIFICATION_COOLDOWN_TIME.as_secs() + 1);
 
 	let _ = service_handler.notify_payment_incoming(client_node_id);
@@ -1133,7 +1134,7 @@ fn test_send_notifications_and_peer_connected_resets_cooldown() {
 		_ => panic!("Expected SendWebhookNotification event after cooldown"),
 	}
 
-	// 5. Can't send payment_incoming notification again immediately after cooldown
+	// 4. Can't send payment_incoming notification again immediately after cooldown
 	let result = service_handler.notify_payment_incoming(client_node_id);
 
 	let error = result.unwrap_err();
@@ -1144,7 +1145,7 @@ fn test_send_notifications_and_peer_connected_resets_cooldown() {
 		"Should not emit event due to cooldown"
 	);
 
-	// 6. After peer_connected, notification should be sent again immediately
+	// 5. After peer_connected, notification should be sent again immediately
 	let init_msg = Init {
 		features: lightning_types::features::InitFeatures::empty(),
 		remote_network_address: None,