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>`.
tnull
added
lightning-liquidity
weekly goal
|
👋 Thanks for assigning @martinsaposnic as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4008 +/- ##
==========================================
- Coverage 88.85% 88.69% -0.16%
==========================================
Files 175 174 -1
Lines 127682 127627 -55
Branches 127682 127627 -55
==========================================
- Hits 113449 113201 -248
- Misses 11675 11941 +266
+ Partials 2558 2485 -73
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}
tnull
force-pushed
the
2025-08-liquidity-persistence-prefactors
branch
from
7447f32 to
1d490f2
Compare
| } | ||
|
|
||
| // 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
There was a problem hiding this comment.
this function name is kind of confusing, it sounds like an action but it's not
It is an action though, as it drops stale webhooks?
| 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
There was a problem hiding this comment.
in here you need to set the
webhook.urltoparams.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
Ah, good catch, that's indeed a behavior change. I added a fix and included the test, thanks for that.
|
@tnull left a few small comments but otherwise looks good! |
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.
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
1d490f2 to
e952cbd
Compare
|
Addressed pending comments. |
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)