Add APIs to prune completed state for LSPS1/LSPS2/LSPS5#4526
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
| /// | ||
| /// Returns an [`APIError::APIMisuseError`] if the counterparty has no state, the order is | ||
| /// unknown, or the order is in a non-terminal state. | ||
| pub async fn prune_order( |
There was a problem hiding this comment.
Hmm, is the right API to require downstream code list the specific orders to prune or should we have some kind of "prune all failed orders older than X" API? @tnull wdyt?
There was a problem hiding this comment.
You are right @TheBlueMatt, it would be much better something like this:
handler.prune_orders(client_id, Duration::from_secs(30 * 24 * 3600)).await?;
I am going to update that part. Thanks for the early review
There was a problem hiding this comment.
Yeah, I agree, allowing to prune older entries in an interval is great. At some point we still might want to expose the LSPS{1,2} service state via the API, at which point allowing to drop specific orders would make sense, but not sure if that's not better done in a dedicated follow-up.
There was a problem hiding this comment.
I think it would be better in a follow-up when service state listing exists
There was a problem hiding this comment.
Ready the new method using an interval @tnull @TheBlueMatt
f3r10
force-pushed
the
feat/add_apis_to_prune_completed_state
branch
from
25a876b to
ebaa7ed
Compare
|
Trying to add the API for LSPS2, using the same strategy as LSPS1 (using intervals), I realize it is not quite possible because The solution would be to add My question is, should this change be done in this same PR or in a follow-up @tnull @TheBlueMatt wdyt? |
|
Yea, makes sense to add a created-at timestamp in LSPS2 requests imo. |
| (4, opening_fee_params, required), | ||
| (6, payment_size_msat, option), | ||
| (8, trust_model, required), | ||
| (10, created_at, (default_value, LSPSDateTime::new_from_duration_since_epoch(Duration::ZERO))), |
There was a problem hiding this comment.
Bug: The default_value of epoch (Duration::ZERO = 1970-01-01T00:00:00Z) means that any pre-existing OutboundJITChannel deserialized from old persisted state (before this field existed) will have created_at = epoch. When prune_channels is called, now.duration_since(epoch) will return ~54 years, so any max_age shorter than that will prune all pre-upgrade terminal channels. This silently breaks the age-filter semantics for operators who upgrade and then call prune_channels expecting only old-enough channels to be pruned.
Consider using a sentinel value (e.g., Option<LSPSDateTime>) and skipping the age check for channels without a created_at, or document this prominently so operators know the first post-upgrade prune with max_age > 0 will still prune all legacy terminal channels.
There was a problem hiding this comment.
I agree with the suggestion. I am going to convert that field to Option<LSPSDateTime>
In this context, None would mean "unknown age" (pre-upgrade data). In prune_terminal_channels:
- Duration::ZERO → prunes all terminal channels including legacy ones
- Any positive max_age → skips channels with created_at = None, only prunes those with a known created_at old enough
| let should_prune = max_age == Duration::ZERO | ||
| || now.duration_since(&channel.created_at) >= max_age; |
There was a problem hiding this comment.
Bug (pre-existing, but used by new code): LSPSDateTime::duration_since uses abs_diff, so it returns the absolute time difference. If now < created_at (e.g., due to clock skew or time adjustments), this returns a positive duration equal to the gap, which could exceed max_age and cause a channel to be incorrectly pruned even though it was just created.
For age-based pruning, a saturating subtraction (return Duration::ZERO when now < created_at) would be correct — an entry created "in the future" relative to now should never be considered old enough to prune. Same issue applies to the LSPS1 usage in prune_terminal_orders.
| let mut outer_state_lock = self.per_peer_state.write().unwrap(); | ||
| match outer_state_lock.get_mut(&counterparty_node_id) { | ||
| Some(peer_state) => { | ||
| if !peer_state.remove_webhook(app_name) { |
There was a problem hiding this comment.
Nit: remove_webhook unconditionally sets needs_persist |= true (line 822 of peer state) even when no webhook is found. When prune_webhook is called with a non-existent app_name, remove_webhook returns false and this method correctly returns an error — but the needs_persist flag has already been set, causing a needless persistence cycle. Consider moving the needs_persist assignment inside the retain's removal branch in remove_webhook.
|
I've thoroughly reviewed the entire PR diff. All three issues from my previous review have been addressed:
I've verified:
No issues found. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4526 +/- ##
==========================================
+ Coverage 86.18% 86.42% +0.24%
==========================================
Files 160 158 -2
Lines 108410 109651 +1241
Branches 108410 109651 +1241
==========================================
+ Hits 93433 94770 +1337
+ Misses 12344 12330 -14
+ Partials 2633 2551 -82
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:
|
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Please respond (either fix or leave a reply why it doesn't matter/can't be fixed) all the AI review feedback and fix CI.
Yes, I am already fixing the issues reported by Claude |
f3r10
force-pushed
the
feat/add_apis_to_prune_completed_state
branch
from
e56052d to
572648e
Compare
| // Simulate what prune_channel does internally. | ||
| peer_state.outbound_channels_by_intercept_scid.remove(&intercept_scid); | ||
| peer_state.intercept_scid_by_channel_id.retain(|_, iscid| *iscid != intercept_scid); | ||
| peer_state.intercept_scid_by_user_channel_id.retain(|_, iscid| *iscid != intercept_scid); | ||
| peer_state.needs_persist = true; | ||
|
|
||
| // All maps must be empty after pruning. | ||
| assert!(peer_state.outbound_channels_by_intercept_scid.is_empty()); | ||
| assert!(peer_state.intercept_scid_by_channel_id.is_empty()); | ||
| assert!(peer_state.intercept_scid_by_user_channel_id.is_empty()); | ||
| assert!(peer_state.needs_persist); |
There was a problem hiding this comment.
Nit: This test manually reimplements the pruning logic (remove + retain + set flag) instead of calling prune_terminal_channels. If the actual method has a bug — e.g., forgetting to clean up one of the auxiliary maps — this test will still pass because it tests a re-implementation, not the real method. Consider calling peer_state.prune_terminal_channels(&now, Duration::ZERO) and then asserting the maps are empty. The same applies to test_peer_state_prune_channel_non_terminal_rejected below which checks the guard condition manually rather than calling the method.
f3r10
force-pushed
the
feat/add_apis_to_prune_completed_state
branch
3 times, most recently
from
e443713 to
7cf9926
Compare
Add `prune_orders(counterparty_node_id, max_age: Duration)` to both `LSPS1ServiceHandler` and `LSPS1ServiceHandlerSync`. It removes all terminal orders (`CompletedAndChannelOpened` / `FailedAndRefunded`) for a given peer that are at least `max_age` old, persists the updated state, and returns the number of entries removed. Passing `Duration::ZERO` prunes all terminal orders immediately regardless of age, which is the recommended approach to unblock a client that has hit the per-peer request limit due to accumulated failed orders. On the `PeerState` layer, `prune_terminal_orders(now, max_age)` uses `retain` for a single-pass removal and sets `needs_persist` only when at least one entry is removed.
Add a `created_at: LSPSDateTime` field to `OutboundJITChannel` to record when each JIT channel was created (i.e., when the buy request was accepted by the LSP). This timestamp is needed to implement time-based bulk pruning of completed channel state. The field is persisted as TLV type 10 with a `default_value` of Unix epoch, ensuring old serialized data (without TLV 10) is read back successfully with the epoch sentinel rather than failing deserialization.
Add `LSPS2ServiceHandler::prune_channels` that lets the LSP operator remove all channels in the `PaymentForwarded` terminal state whose `created_at` timestamp is at least `max_age` old. Passing `Duration::ZERO` prunes all terminal channels regardless of age. All associated state is cleaned up atomically: - per-peer `intercept_scid_by_channel_id` and `intercept_scid_by_user_channel_id` - handler-level `peer_by_intercept_scid` and `peer_by_channel_id` A new `PeerState::prune_terminal_channels` helper handles the intra-peer map cleanup and returns the removed `(scid, channel_id)` pairs for the handler to clean up the outer maps. Integration tests cover: non-terminal channels not pruned, unknown counterparty errors, age filtering, and successful bulk prune.
Add `LSPS5ServiceHandler::prune_webhook` that removes a single webhook entry identified by `counterparty_node_id` and `LSPS5AppName`. The method is synchronous, consistent with all other public `notify_*` methods on this handler: it marks the peer state as dirty and relies on the normal `LiquidityManager::persist` loop to flush the change to the KVStore. The method reuses the existing private `PeerState::remove_webhook` helper and returns an `APIError::APIMisuseError` if the counterparty has no registered state or the given `app_name` is not found.
…skew Replace abs_diff with a saturating subtraction so that duration_since returns Duration::ZERO when the reference timestamp is in the apparent future (e.g. due to clock skew or monotonicity violations). Callers using the result as an age — prune_terminal_orders (LSPS1), prune_terminal_channels (LSPS2), and the stale-webhook and cooldown checks (LSPS5) — no longer risk spuriously large values triggering premature pruning or incorrect cooldown expiry. Two unit tests are added to document the contract.
f3r10
force-pushed
the
feat/add_apis_to_prune_completed_state
branch
from
7cf9926 to
9e9f96b
Compare
5 participants
Closes #4444.
Add explicit pruning APIs to
LSPS1ServiceHandler,LSPS2ServiceHandler, andLSPS5ServiceHandlerso LSP operators can reclaim memory from completed historical state.