Skip to content

LSPS5: Correct notification cooldown & reset logic #3975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 5, 2025
Merged

LSPS5: Correct notification cooldown & reset logic #3975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89962a1
LSPS5: Correct notification cooldown & reset logic
martinsaposnic Jul 30, 2025
bae3224
introduce SLOW_DOWN error so we don't silently fail to send an LSPS5/…
martinsaposnic Aug 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions lightning-liquidity/src/lsps5/event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
14 changes: 13 additions & 1 deletion lightning-liquidity/src/lsps5/msgs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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 => {
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't get there in github, but it seems the above comment

	/// private code range so we never collide with the spec's codes

is wrong given we match spec'd and non-spec'd codes here and elsewhere?

}
}
/// The error message for the LSPS5 protocol error.
Expand All @@ -133,6 +144,7 @@ impl LSPS5ProtocolError {
LSPS5ProtocolError::SerializationError => {
"Error serializing LSPS5 webhook notification"
},
LSPS5ProtocolError::SlowDownError => "Notification sent too frequently",
}
}
}
Expand Down
88 changes: 67 additions & 21 deletions lightning-liquidity/src/lsps5/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EEk, we end up with a HashMap<_, HashMap<_, HashMap<_, _>>> here, let's condense that please. First of all we probably don't need to track the last-sent notification on a per-webhook basis, it seems like it can be per-client.

Secondly, indexing this by WebhookNotificationMethod means we track per-timeout for LSPS5ExpirySoon notifications, which I don't think is what we want. Once we fix that, we have 5 notification types, which definitely don't need to be a HashMap, probably not even a Vec, a [X; 5] should do fine :).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EEk, we end up with a HashMap<_, HashMap<_, HashMap<_, _>>> here, let's condense that please. First of all we probably don't need to track the last-sent notification on a per-webhook basis, it seems like it can be per-client.

Yeah, probably gonna extract that into a PeerState holding the per-peer webhooks for persistence anyways. Will see whether I can condense it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine the last level can simply be removed, there's no reason to track per-webhook last-sent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine the last level can simply be removed, there's no reason to track per-webhook last-sent.

It is described in the spec this way, but I guess there's nothing keeping us from applying a more coarse grained approach in our implementation. Now added a corresponding change to the persistence PR coming up.

}

Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still seems wayyyy too high. If we get two payments an hour apart, just because the first wakeup was missed doesn't mean we don't want to send another. A phone may be disconnected from the network or low on battery when the first was sent and the phone decided not to do a wakeup, but it may well be in a different state an hour later. Yea, we don't want to send 1000 notifications in an hour, but we definitely should be comfortable sending five in an hour. We either need to crank this way down (one a minute?) or track and allow bursts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I tend to agree. We might want to amend the spec first though, as it currently states:

If the client does not come online after some time that a particular method was sent via a webhook, then the LSP may raise it again. This timeout must be measurable in hours or days.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lightning/blips#64 spec change PR


// 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.
Expand All @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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`].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these links should be to the Config not the default constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR changed it so it's not a config anymore. it does not make sense that it's called "default" though, I will change that

///
/// # 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)
Expand All @@ -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> {
Expand All @@ -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> {
Expand All @@ -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> {
Expand All @@ -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(())
}

Expand Down Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit orthogonal, but do we even need a separate last_used field? Mind adding comments on the fields to explain how last_used and last_notification_sent differ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add comments explaining because it was even hard for me to remember the difference.

they are different because

  • last_used is updated either if a notification is sent or if the webhook is updated (via set_webhook)
  • last_notification_sent is a Map<NotificationMethod, timestamp> that is only updated when a notification is sent

last_notification_sent is used for the notification_cooldown logic, and last_used is used for the stale_webhooks logic

}
}
}
}

impl<CM: Deref, NS: Deref, TP: Deref> LSPSProtocolMessageHandler for LSPS5ServiceHandler<CM, NS, TP>
Expand Down
7 changes: 6 additions & 1 deletion lightning-liquidity/src/manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,8 +715,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(())
}
}
Expand Down
6 changes: 1 addition & 5 deletions lightning-liquidity/tests/lsps0_integration_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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),
Expand Down
Loading
Loading