Automatically archive resolved ChannelMonitors in the BP
#4288
Conversation
When we first added auto-archiving of resolved `ChannelMonitor`s, we wanted to be somewhat cautious of flipping it on by default as archiving a `ChannelMonitor` too soon would be a critical bug and, while we were confident in it, we weren't 100%. Since then its been used extensively in various LDK deployments, including `ldk-node`. Given its now seen substantial use, and performs an important anti-DoS function, here we flip to calling it by default on a new timer in `lightning-background-processor`. Fixes lightningdevkit#218
|
👋 Thanks for assigning @tnull as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4288 +/- ##
==========================================
+ Coverage 89.38% 89.39% +0.01%
==========================================
Files 180 180
Lines 139834 139953 +119
Branches 139834 139953 +119
==========================================
+ Hits 124985 125113 +128
+ Misses 12262 12251 -11
- Partials 2587 2589 +2
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:
|
| if archive_timer_elapsed { | ||
| log_trace!(logger, "Archiving stale ChannelMonitors."); | ||
| chain_monitor.archive_fully_resolved_channel_monitors(); | ||
| have_archived = true; |
There was a problem hiding this comment.
I do wonder if we should have a more verbose return value and then use that to explicitly prune the corresponding OutputSweeper entries now, rather than just adding a buffer on top of the archival delay.
There was a problem hiding this comment.
Possibly, yea, though if we're still concerned about it its much easier to double the prune delay in the sweeper...Either way seems unrelated to this PR (which just makes sure the sweeper works properly...)
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
3 participants
When we first added auto-archiving of resolved
ChannelMonitors, we wanted to be somewhat cautious of flipping it on by default as archiving aChannelMonitortoo soon would be a critical bug and, while we were confident in it, we weren't 100%. Since then its been used extensively in various LDK deployments, includingldk-node.Given its now seen substantial use, and performs an important anti-DoS function, here we flip to calling it by default on a new timer in
lightning-background-processor.Fixes #218