Repository navigation
Add LSPS5 webhook notification support - #993
Camillarhi wants to merge 1 commit into
Conversation
|
👋 Thanks for assigning @f3r10 as a reviewer! |
1d750fb to
9d4f022
Compare
2601d0f to
9eb148f
Compare
| }, | ||
| }; | ||
|
|
||
| pending_set_webhook_requests_lock.insert(request_id, sender); |
There was a problem hiding this comment.
LSPS1/LSPS2 wrap the same kind of map in PendingRequest/PendingRequestGuard (see client/lsps2.rs), held across the .await and removing its own entry on drop.
Here nothing removes the entry when tokio::time::timeout(...) in lsps5_set_webhook/lsps5_list_webhooks/lsps5_remove_webhook gives up — the oneshot::Sender and its HashMap entry stay behind for the life of the node. On a node with a slow or flaky LSPS5 LSP this grows unbounded. Same pattern at L124 (pending_list_webhooks_requests) and L176 (pending_remove_webhook_requests)
Could we reuse PendingRequestGuard here, the way LSPS2 does? None of these three calls need the fan-out (followers) side of PendingRequest — only one caller ever awaits a given set_webhook/list_webhooks/remove_webhook — but the drop-cleanup is exactly what's missing.
There was a problem hiding this comment.
Thanks! This has been updated to use PendingRequestGuard just like LSPS2 does
| let lsps2_service_config = | ||
| self.lsps2_service.as_ref().map(|s| s.ldk_service_config.clone()); | ||
| let lsps5_service_config = self.lsps5_service.clone(); | ||
| let advertise_service = self |
There was a problem hiding this comment.
advertise_service sets the shared LSPS feature bit for any configured service, not just LSPS2 — but this only reads it off lsps2_service. A node with enable_liquidity_provider(None, Some(lsps5_cfg)) always gets advertise_service = false, with no way to turn it on.
Is that intentional, or should LSPS5-only providers be able to advertise too?
There was a problem hiding this comment.
Yeah, it's reachable. Though the flag sets the shared LSPS0, so it was never really an LSPS2 thing. If I add it to the LSPS5 config too, then LSPS1 service lands, and that's three copies of the same flag. One node-level setting is probably where this should end up, so I'll take a look at that instead of duplicating it
| e | ||
| ), | ||
| } | ||
| Error::LiquidityNotifyWebhookFailed |
There was a problem hiding this comment.
SlowDownError (the notification cooldown) and every other failure both map to Error::LiquidityNotifyWebhookFailed. A caller can't tell "you're rate-limited, retry shortly" from "this genuinely failed" without parsing logs.
Worth a distinct Error::LiquidityNotifyRateLimited (or similar) so callers can branch on it?
There was a problem hiding this comment.
Thanks. This will be updated to return a distict error for slow down
8a8e045 to
5fd1680
Compare
| /// [LSPS2]: https://github.com/BitcoinAndLightningLayerSpecs/lsp/blob/main/LSPS2/README.md | ||
| /// [bLIP-52 / LSPS2]: https://github.com/lightning/blips/blob/master/blip-0052.md | ||
| /// [bLIP-55 / LSPS5]: https://github.com/lightning/blips/blob/master/blip-0055.md | ||
| pub fn enable_liquidity_provider( |
There was a problem hiding this comment.
I noticed this method now takes two Option configs plus a trailing bool, and every call site (7 in the integration tests) has had to add arguments as it grew. I also see the builder has separate methods per chain source (set_chain_source_esplora, set_chain_source_electrum, etc.) rather than one method with growing options.
Would splitting into enable_lsps2_service(cfg: LSPS2ServiceConfig, advertise: bool) and enable_lsps5_service(cfg: LSPS5ServiceConfig, advertise: bool) (or a shared set_advertise_service(bool) alongside two single-purpose enable calls) fit better? Non-breaking for a future third protocol, and each call site says exactly one thing again.
There was a problem hiding this comment.
The single method was a decision made during the liquidity refactor #792 (comment), so the one method with an options shape is deliberate, and LSPS1 service slots in as a third Option rather than a third method.
On the bool, advertise isn't per protocol. The LSPS feature bit is set at the liquidity level rather than per service: when a service is configured and advertise_service is true. That's why it moved out of LSPS2ServiceConfig here in the first place.
enable_lsps2_service(cfg, advertise) plus enable_lsps5_service(cfg, advertise) would put two bools behind one bit, and we'd be back to the duplication.
The docs should say the advertise part more plainly though, so I'll expand that paragraph to name the shared feature.
There was a problem hiding this comment.
Ah, that makes sense misseed that advertise_service is a single shared bit rather than per-protocol state. Thanks for pointing to #792 for context too. The docs clarification sounds good.
Implement bLIP-55 / LSPS5 on top of the multi-LSP liquidity module. Clients register, list and remove webhooks with a given LSP via `Liquidity::lsps5`. Each call takes the LSP's node ID explicitly, as the notification delivery service verifies every notification against the node ID of the LSP that sent it. The service side delivers signed notifications over HTTPS to wake offline clients: - HTLCs to a client's private channels are intercepted and held for up to 10 seconds while the client connects. - Onion messages for a client are kept in the onion message mailbox until it reconnects. - Clients are notified when an HTLC nears the height at which the LSP would have to force-close their channel. - Operators can ask a client to come online before reclaiming liquidity. `enable_liquidity_provider` now takes optional LSPS2 and LSPS5 configs alongside `advertise_service`, which moves out of `LSPS2ServiceConfig` as the LSPS feature bit it sets is shared by all services.
3 participants
Integrates LSPS5 (bLIP-0055) from lightning-liquidity, enabling webhook-based push notifications so clients can be alerted to events while their app is offline. Built on the refactored multi-LSP liquidity module
(src/liquidity/{client,service}).Client
Exposed via
Node::liquidity().lsps5():Each call takes the LSP's node ID, as bLIP-55 has the notification delivery service verify every notification against the node ID of the LSP that sent it.
Service
Lets a node act as an LSPS5 server, enabled via
Builder::enable_liquidity_provider. Notifications are signed and delivered over HTTPS, and only to clients that are currently offline, as bLIP-55 requires.payment_incoming: HTLCs to a client's private channels are intercepted (ToOfflinePrivateChannels) and held for up to 10 seconds while the client connects, then forwarded, or failed back if the client doesn't make it in time.onion_message_incoming: onion messages for offline peers are intercepted and kept in the onion message mailbox until the peer reconnects.expiry_soon: a periodic task notifies clients when an HTLC, in either direction, nears the height at which the LSP would have to force-close their channel.liquidity_management_request: exposed for operators to call before reclaiming liquidity. ReturnsError::LiquidityNotifyWebhookRateLimitedwhen called again within the notification cooldown.Reopening after an accidental force-push pushed the branch to main's tip and auto-closed #729.
Fixes: #1017
Closes #762
Closes #646