Log pending in-flight updates when we are missing a monitor #2990
Conversation
If we are missing a `ChannelMonitor` for which the `ChannelManager` has pending updates, it may be useful to print what the updates we have actually are, at least useful in identifying the bug(s).
|
Also started logging directly in |
TheBlueMatt
force-pushed
the
2024-04-log-in-flights
branch
from
5398e40 to
73670e1
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2990 +/- ##
==========================================
+ Coverage 88.86% 91.56% +2.69%
==========================================
Files 115 119 +4
Lines 92753 117849 +25096
Branches 92753 117849 +25096
==========================================
+ Hits 82429 107910 +25481
+ Misses 7942 7923 -19
+ Partials 2382 2016 -366 ☔ View full report in Codecov by Sentry. |
TheBlueMatt
force-pushed
the
2024-04-log-in-flights
branch
from
73670e1 to
67e31ea
Compare
lightning/src/chain/chainmonitor.rs
Outdated
| if !monitor_data.has_pending_chainsync_updates(&pending_monitor_updates) { | ||
| let monitor_has_pending_updates = | ||
| monitor_data.has_pending_chainsync_updates(&pending_monitor_updates); | ||
| log_debug!(self.logger, "Completed chain sync monitor update for channel with funding outpoint {:?}, {}", |
There was a problem hiding this comment.
did we leave out update_id intentionally in case of chain-sync?
I understand it resets b/w reloads/restarts but there still might be some correlation.
afaiu, the discussion on ldk-help was related to chain-sync. I am ok either way, just confirming.
There was a problem hiding this comment.
Because I'm already living in the future where we don't persist/block for chain-sync updates :). But, you're right, for now we should log them more clearly.
TheBlueMatt
force-pushed
the
2024-04-log-in-flights
branch
from
67e31ea to
ef6a00d
Compare
| log_trace!(logger, "Finished syncing Channel Monitor for channel {}", log_funding_info!(monitor)), | ||
| log_trace!(logger, "Finished syncing Channel Monitor for channel {} for block-data update_id {}", | ||
| log_funding_info!(monitor), | ||
| chain_sync_update_id |
There was a problem hiding this comment.
nit: if we want to add update_id in case of completed, you might want to add it at L:821 as well.
There was a problem hiding this comment.
Should be printed a few lines up so was being lazy but went ahead and did this.
TheBlueMatt
force-pushed
the
2024-04-log-in-flights
branch
from
ef6a00d to
c614b42
Compare
|
CI is failing, good to merge otherwise. |
When a `ChannelMonitor[Update]` persistence completes, we rely on logging in `ChannelManager` to hear about it. However, this won't happen at all if there's still pending updates as no `MonitorEvent` will be generated. Thus, here, we add logging directly in `ChainMonitor`, ensuring we can deduce when individual updates completed from debug logs.
TheBlueMatt
force-pushed
the
2024-04-log-in-flights
branch
from
c614b42 to
93e7763
Compare
|
Oops, sorry, fixed: $ git diff-tree -U1 c614b42b 93e77632
diff --git a/lightning/src/chain/chainmonitor.rs b/lightning/src/chain/chainmonitor.rs
index 3de8882e1..6e824d1e1 100644
--- a/lightning/src/chain/chainmonitor.rs
+++ b/lightning/src/chain/chainmonitor.rs
@@ -818,3 +818,3 @@ where C::Target: chain::Filter,
log_debug!(logger,
- "Persistence of ChannelMonitorUpdate id {} for channel {} in progress",
+ "Persistence of ChannelMonitorUpdate id {:?} for channel {} in progress",
update_id,
@@ -825,3 +825,3 @@ where C::Target: chain::Filter,
log_debug!(logger,
- "Persistence of ChannelMonitorUpdate id {} for channel {} completed",
+ "Persistence of ChannelMonitorUpdate id {:?} for channel {} completed",
update_id, |
If we are missing a
ChannelMonitorfor which theChannelManagerhas pending updates, it may be useful to print what the updates we have actually are, at least useful in identifying the bug(s).