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
Implement the bLIP-55 / LSPS5 webhook registration protocol on top of the
multi-LSP liquidity module (src/liquidity/{client,service}).
Client side, exposed via Node::liquidity().lsps5():
- set_webhook / list_webhooks / remove_webhook to manage webhook
registrations with a given LSP. Each takes the LSP's node ID explicitly:
bLIP-55 has the notification service verify the x-lsps5-signature header
against the signing LSP's node ID, so a registration is meaningful only
for one LSP at a time.
Service side, enabled by passing an LSPS5ServiceConfig to
Builder::enable_liquidity_provider():
- Deliver outgoing webhook notifications over HTTPS in response to
LSPS5ServiceEvent::SendWebhookNotification.
- Send lsps5.payment_incoming when an inbound HTLC forward to a client
fails because the client is offline (wired from LdkEvent::
HTLCHandlingFailed with LocalHTLCFailureReason::PeerOffline).
- Send lsps5.onion_message_incoming when an intercepted onion message
targets a client that is currently offline.
- Send lsps5.expiry_soon from a periodic task that scans channels for outbound
HTLCs approaching their cltv_expiry, so a client that went offline holding an
HTLC has a chance to come online and settle before it expires.
Adds integration tests covering webhook registration and the
payment_incoming trigger, and wires the feature through the UniFFI
bindings.
| /// [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.
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}).When
node_idisNone,set_webhookandremove_webhookfan out to every LSPS5-capable LSP, so a webhook can be configured once across all configured LSPs.Lets a node act as an LSPS5 server:
LSPS5ServiceEvent::SendWebhookNotification.onion_message_incomingnotification when an intercepted onion message targets a client that is currently offline (wired fromLdkEvent::OnionMessageIntercepted, gated on peer connectivity via the peer manager).Reopening after an accidental force-push pushed the branch to main's tip and auto-closed #729.
Fixes: #1017