LSPS5 Follow ups

[martinsaposnic]

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

[TheBlueMatt]

Also a few more #3662 (review)

[martinsaposnic]

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 ?

[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.

[TheBlueMatt]

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

[tnull]

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).

[tnull]

Seems all open points have been addressed, closing this.