LSPS5 Follow ups

PR #3662 got too big, so a few small fixes will come in follow up PRs once the base PR is merged

Fixes coming in a follow up:

With PR:

avoid timestamp validation on validator LSPS5 implementation #3662 (comment) Simplify LSPS5/validator: drop time checks & custom signature storage #3961
drop signature config, only have a few signatures for the replay attack check LSPS5 implementation #3662 (comment) Simplify LSPS5/validator: drop time checks & custom signature storage #3961
drop TimeProvider from LSPS5ClientHandler, only keep X pending requests LSPS5 implementation #3662 (comment) [LSPS5 FollowUp] Simplify LSPS5/client peer state #3969
clean up the overall PeerState once it's empty LSPS5 implementation #3662 (comment) [LSPS5 FollowUp] Simplify LSPS5/client peer state #3969
Move time_provider trait into crate::utils::time or similar LSPS5 implementation #3662 (comment) LSPS5 follow up: Move TimeProvider to utils::time module for better organization #3962
drop unnecesary time_provider.clone() in manager LSPS5 implementation #3662 (comment) LSPS5 - small follow ups #3963
do not duplicate DEFAULT_RESPONSE_MAX_AGE_SECS LSPS5 implementation #3662 (comment) LSPS5 - small follow ups #3963
rename broadcast_notification in service (LSPS5 implementation #3662 (comment)) LSPS5 - small follow ups #3963
fix incorrect log::info / log::error (could be a peer sending us garbage) on lsps5 client LSPS5 implementation #3662 (comment) / LSPS5 implementation #3662 (comment) LSPS5 -> more follow ups #3972
Update README on lightning liquidity Add bLIP 55: Webhook Registration (LSPS5) lightning/blips#55 (comment) LSPS5 -> more follow ups #3972
fix notification_cooldown_hours on lsps5 service LSPS5 implementation #3662 (comment) LSPS5: Correct notification cooldown & reset logic #3975
Reset notification_cooldown when peer is connected LSPS5 Follow ups #3944 (comment) LSPS5: Correct notification cooldown & reset logic #3975
copy LSPSDateTime instead of cloning LSPS5 implementation #3662 (comment) LSPS5 -> More follow ups #3987
docs wrong and return Duration instead of u64 on abs_diff LSPS5 implementation #3662 (comment) LSPS5 -> More follow ups #3987
LSPS5 DOS protection Add LSPS5 DOS protections. #3993
fix links on service docs LSPS5: Correct notification cooldown & reset logic #3975 (review) [LSPS5] Change notification cooldown time to 1 minute, also update docs #3994
change notification_cooldown_hours should be 1 minute LSPS5: Correct notification cooldown & reset logic #3975 (review) [LSPS5] Change notification cooldown time to 1 minute, also update docs #3994
refactor last_notification_sent so it's not HashMap<, HashMap<, HashMap<_, _>>> lightning-liquidity: Pre-/Refactors to prepare for persistence #4008

@tnull

I found a tiny implementation bug

From the spec: https://github.com/lightning/blips/pull/55/files#diff-0446c6230ffe4cf26243c752fedea2998f61da34f7e3230e3dcbc9da872423e6R618

LSP: Rate-limiting Notifications By method

An LSP implementation must avoid sending multiple LSPS5 notifications of the same method (other than
lsps5.webhook_registered) close in time, as long as the client has not connected to it.
For example, if a payment arrives and the LSP thus wants to send lsps5.payment_incoming notification to a registered webhook, and another payment arrives before the client comes online, the LSP must not send another lsps5.payment_incoming notification.
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.
The timeout should be reset once the client comes online and then goes offline.

I misunderstood this and implemented a notification_cooldown_hours (with a 24 hours default) that prevents the service from sending the same notification method for 24 hours, which is completely ridiculous (sorry 🙃 ). Relevant code https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-liquidity/src/lsps5/service.rs#L397

That logic is completely wrong and needs to go, but for implementing the spec correctly, I think we would need to expose a new public function client_connected on the service so it can be notified that the app went back online so it can reset the timeout. do we have any other alternatives? what do you think @tnull ?

I found a tiny implementation bug

From the spec: https://github.com/lightning/blips/pull/55/files#diff-0446c6230ffe4cf26243c752fedea2998f61da34f7e3230e3dcbc9da872423e6R618

LSP: Rate-limiting Notifications By method

An LSP implementation must avoid sending multiple LSPS5 notifications of the same method (other than
lsps5.webhook_registered) close in time, as long as the client has not connected to it.
For example, if a payment arrives and the LSP thus wants to send lsps5.payment_incoming notification to a registered webhook, and another payment arrives before the client comes online, the LSP must not send another lsps5.payment_incoming notification.
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.
The timeout should be reset once the client comes online and then goes offline.

I misunderstood this and implemented a notification_cooldown_hours (with a 24 hours default) that prevents the service from sending the same notification method for 24 hours, which is completely ridiculous (sorry 🙃 ). Relevant code https://github.com/lightningdevkit/rust-lightning/blob/main/lightning-liquidity/src/lsps5/service.rs#L397

That logic is completely wrong and needs to go, but for implementing the spec correctly,

Huh, I don't think that logic is 'completely wrong', we just need to reset the cooldown once the client connects, right? Thanks for catching it though!

I think we would need to expose a new public function client_connected on the service so it can be notified that the app went back online so it can reset the timeout. do we have any other alternatives? what do you think @tnull ?

No, let's just have a pub(crate) fn peer_connected(..) on the LSPS5 service and have it called by LiquidityManager::peer_connected.

FWIW, we could also consider reducing the cooldown to an hour or a few hours. I think it is only meant to ensure that the LSP doen't bombard the client with unnecessary notifications.

IMO a minute or five would be much more appropriate than an hour - https://github.com/lightningdevkit/rust-lightning/pull/3975/files#r2254509585

Also https://github.com/lightningdevkit/rust-lightning/pull/3975/files#r2254524535

IMO a minute or five would be much more appropriate than an hour - https://github.com/lightningdevkit/rust-lightning/pull/3975/files#r2254509585

Also https://github.com/lightningdevkit/rust-lightning/pull/3975/files#r2254524535

Seem my responses there: for the former we might need to amend the spec. The latter I'll see to pickup as part of the persistence work, for which I'll probably introduce a per-peer state anyways (was just waiting so far for the conflicting LSPS5 PRs to land).

Seems all open points have been addressed, closing this.

Navigation Menu

Repository navigation

LSPS5 Follow ups #3944

LSP: Rate-limiting Notifications By `method`

LSP: Rate-limiting Notifications By `method`

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

LSPS5 Follow ups #3944

Description

Activity

TheBlueMatt commented on Jul 24, 2025

martinsaposnic commented on Jul 29, 2025

LSP: Rate-limiting Notifications By method

tnull commented on Jul 30, 2025

LSP: Rate-limiting Notifications By method

TheBlueMatt commented on Aug 5, 2025

tnull commented on Aug 7, 2025

tnull commented on Sep 4, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

LSP: Rate-limiting Notifications By `method`

LSP: Rate-limiting Notifications By `method`