Persist unresolved ChannelMonitors on empty height change#3442
Conversation
lightning/src/chain/chainmonitor.rs
Outdated
| have_monitors_to_prune = true; | ||
| let (is_fully_resolved, needs_persistence) = monitor_holder.monitor.process_claim_resolution_status(&logger); | ||
| if is_fully_resolved || needs_persistence { | ||
| have_monitors_to_persist_or_prune = true; |
There was a problem hiding this comment.
You can keep the bool as it was, we don't need the write lock to persist a monitor, we can do that in this loop and only do the bottom block if we need to prune.
| /// This function returns true only if [`Self::get_claimable_balances`] has been empty for at least | ||
| /// This function returns a tuple of two booleans, the first indicating whether the monitor is | ||
| /// fully resolved, and the second whether the monitor needs persistence as a consequence of | ||
| /// checking its claim resolution status. |
There was a problem hiding this comment.
This is kinda weird "checking -> needs persistence" should be explained a bit. Maybe "the second whether the monitor needs persistence to ensure it is reliably marked as resolved in 4032 blocks."?
| /// The first boolean is true only if [`Self::get_claimable_balances`] has been empty for at least | ||
| /// 4032 blocks as an additional protection against any bugs resulting in spuriously empty balance sets. | ||
| pub fn is_fully_resolved<L: Logger>(&self, logger: &L) -> bool { | ||
| pub fn process_claim_resolution_status<L: Logger>(&self, logger: &L) -> (bool, bool) { |
There was a problem hiding this comment.
Maybe check_update_fully_resolved_status? claim (singular) is a bit weird as we're not talking about a specific claim but rather all possible funds to claim. Also nice to give it a clear "update" in the name cause we might need to persist and we're updating the fully-resolved-status so that it clears in 4k blocks.
There was a problem hiding this comment.
I honestly hate every name I could come up with and this one, too, but I guess it's the least bad option. I added an and, but can drop it if you prefer
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3442 +/- ##
==========================================
+ Coverage 89.69% 90.14% +0.45%
==========================================
Files 130 130
Lines 107335 110095 +2760
Branches 107335 110095 +2760
==========================================
+ Hits 96273 99249 +2976
+ Misses 8660 8522 -138
+ Partials 2402 2324 -78 ☔ View full report in Codecov by Sentry. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Two doc nits otherwise LGTM, feel free to just fix the nits and squash in one go.
|
|
||
| /// Checks if the monitor is fully resolved. Resolved monitor is one that has claimed all of | ||
| /// its outputs and balances (i.e. [`Self::get_claimable_balances`] returns an empty set). | ||
| /// Additionally updates the empty balances height if necessary. |
There was a problem hiding this comment.
I'm not sure we expect users to know what the "empty balances height" is :).
| /// Additionally updates the empty balances height if necessary. | |
| /// Additionally may update state to track when the balances set became empty. |
arik-so
force-pushed
the
archive-monitor-persistence-trigger
branch
from
0a3c718 to
6f9300f
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
This is trivial, it just a new return value and code to handle it. Commit really could have used a description but its not worth holding this up on that.
2 participants
This hopefully fixes #3121.