lightning-liquidity: Pre-/Refactors to prepare for persistence
#4008
Conversation
.. which streamlines the `PaymentQueue` API a bit, but most importantly can more easily get persisted using macros in the next step.
While bLIP-55 describes that the service should wait at least some cooldown between sending notifications per individual `method`, there is nothing that keeps us from simplifying our approach to apply the cooldown to *any* notifications sent, especially since we just reduced the cooldown period to 1 minute elsewhere. Here, we therefore simplify the `last_notification_sent` field to just be a `Option<LSPSDateTime>`.
If we happened to send a notification while the client is connected to us, we would previously only reset the cooldown once the client connects again. While theoretically it would be preferable to never set the `last_notification_sent` field to begin with if the client is connected to us, allowing the service handler to query the peer connection state would be unnecessarily complex. Here, we therefore simply opt to also reset the `last_notification_sent` state once the peer disconnects from us.
Going forward, we'll add serialization logic for LSPS5 types. To contain the persisted state a bit better (and to align the model with LSPS1/2), we refactor the `LSPS5ServiceHandler` to hold a `PeerState` object.
tnull
added
lightning-liquidity
weekly goal
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4008 +/- ##
==========================================
- Coverage 88.85% 88.84% -0.01%
==========================================
Files 175 175
Lines 127682 127713 +31
Branches 127682 127713 +31
==========================================
+ Hits 113449 113470 +21
- Misses 11675 11684 +9
- Partials 2558 2559 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Previously, we'd constantly check whether or not we can prune stale webhooks. While not wrong, it lead to a bunch of ~unnecessary operations, especially given that we only prune once a day currently. Here we move pruning to `peer_connected`/`peer_disconnected`, which is similar to what we do for LSPS2, and should still be more than enough.
We add the license header to all files in `lightning-liquidity` where it was absent.
tnull
force-pushed
the
2025-08-liquidity-persistence-prefactors
branch
from
7447f32 to
1d490f2
Compare
| webhook.last_used = now; | ||
| webhook.last_notification_sent = Some(now); |
There was a problem hiding this comment.
this was not introduced in this PR, but it's weird that last_used and last_notification_sent are set before send_notification(), which can return an error (the thing that can fail is the sign_message function which there is a good chance it will never fail, but worth mentioning IMO)
| let now = LSPSDateTime::new_from_duration_since_epoch( | ||
| self.time_provider.duration_since_epoch(), | ||
| ); |
There was a problem hiding this comment.
you can re-use the now value from above
| _counterparty_node_id: counterparty_node_id, | ||
| last_used: now, | ||
| last_notification_sent: new_hash_map(), | ||
| if let Some(webhook) = peer_state_lock.webhook_mut(¶ms.app_name.clone()) { |
There was a problem hiding this comment.
I think the clone is not necessary here
| } | ||
|
|
||
| // Returns whether the entire state is empty and can be pruned. | ||
| fn prune_stale_webhooks(&mut self, now: LSPSDateTime) -> bool { |
There was a problem hiding this comment.
this function name is kind of confusing, it sounds like an action but it's not
| if let Some(webhook) = peer_state_lock.webhook_mut(¶ms.app_name.clone()) { | ||
| no_change = webhook.url == params.webhook; | ||
| if !no_change { | ||
| webhook.last_used = now |
There was a problem hiding this comment.
in here you need to set the webhook.url to params.webhook. if not, the update webhook functionality will be broken.
unfortunately, right now the tests are not testing the webhook update feature. they only test that the notification is sent with the updated url, but they don't test that the url is actually updated and persisted :(
here is a regression test that passes on main but fails on this branch
#[test]
fn webhook_update_affects_future_notifications() {
let mock_time_provider = Arc::new(MockTimeProvider::new(1000));
let time_provider = Arc::<MockTimeProvider>::clone(&mock_time_provider);
let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
let (lsps_nodes, _) = lsps5_test_setup(nodes, time_provider);
let LSPSNodes { service_node, client_node } = lsps_nodes;
let service_node_id = service_node.inner.node.get_our_node_id();
let client_node_id = client_node.inner.node.get_our_node_id();
let client_handler = client_node.liquidity_manager.lsps5_client_handler().unwrap();
let service_handler = service_node.liquidity_manager.lsps5_service_handler().unwrap();
let app = "UpdateTestApp";
let url_v1 = "https://example.org/v1";
let url_v2 = "https://example.org/v2";
// register v1
client_handler.set_webhook(service_node_id, app.into(), url_v1.into()).unwrap();
let req = get_lsps_message!(client_node, service_node_id);
service_node.liquidity_manager.handle_custom_message(req, client_node_id).unwrap();
let _ = service_node.liquidity_manager.next_event().unwrap(); // initial webhook_registered
let resp = get_lsps_message!(service_node, client_node_id);
client_node.liquidity_manager.handle_custom_message(resp, service_node_id).unwrap();
let _ = client_node.liquidity_manager.next_event().unwrap();
// update to v2
client_handler.set_webhook(service_node_id, app.into(), url_v2.into()).unwrap();
let upd_req = get_lsps_message!(client_node, service_node_id);
service_node.liquidity_manager.handle_custom_message(upd_req, client_node_id).unwrap();
let update_event = service_node.liquidity_manager.next_event().unwrap();
match update_event {
LiquidityEvent::LSPS5Service(LSPS5ServiceEvent::SendWebhookNotification {
url, ..
}) => {
assert_eq!(url.as_str(), url_v2);
},
_ => panic!("Expected webhook_registered for update"),
}
let upd_resp = get_lsps_message!(service_node, client_node_id);
client_node.liquidity_manager.handle_custom_message(upd_resp, service_node_id).unwrap();
let _ = client_node.liquidity_manager.next_event().unwrap();
// Advance past cooldown and send a notification again
mock_time_provider.advance_time(NOTIFICATION_COOLDOWN_TIME.as_secs() + 1);
service_handler.notify_payment_incoming(client_node_id).unwrap();
let ev = service_node.liquidity_manager.next_event().unwrap();
match ev {
LiquidityEvent::LSPS5Service(LSPS5ServiceEvent::SendWebhookNotification {
url,
notification,
..
}) => {
assert_eq!(notification.method, WebhookNotificationMethod::LSPS5PaymentIncoming);
assert_eq!(url.as_str(), url_v2, "Should target updated URL");
},
_ => panic!("Expected SendWebhookNotification after update"),
}
}There was a problem hiding this comment.
you also need to set last_notification_sent to None so you don't carry the old cooldown to the new url
There was a problem hiding this comment.
so you don't carry the old cooldown to the new url
we can add a test that asserts that a notification can be sent immediately after updating a webhook
|
@tnull left a few small comments but otherwise looks good! |
| &mut self.webhooks | ||
| } | ||
|
|
||
| fn app_names(&self) -> Vec<LSPS5AppName> { |
There was a problem hiding this comment.
This is used in two places as app_names().len(), let's push the allocation to the callsite with a FixedSizeIterator
Before we can introduce persistence to the
lightning-liquiditycrate, we make a number of pre-/refactors to make our lives easier. We split this out here to keep PR sizes manageable and to introducing too many conflicts with concurrent work.In this PR, we move some LSPS2/LSPS5 state data to dedicated types, which will allow use to use our serialization macros in the next step. We also simplify the
last_notification_senttracking in LSPS5 (now only tracking a single timestamp for all notification methods), which was requested on a previous PR. We furthermore now reset the notification cooldown on peer disconnection (useful in case we somehow notified last while the peer was connected), and move to prune the LSPS5 service state only onpeer_{dis}connected, which should be more than enough.(cc @martinsaposnic)