Allow stale ChannelMonitors if we are sure they don't have funds
#4146
Conversation
|
I've assigned @jkczyz as a reviewer! |
TheBlueMatt
force-pushed
the
2025-10-less-panicy-reads
branch
2 times, most recently
from
58ad915 to
2b29c85
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4146 +/- ##
==========================================
+ Coverage 88.63% 88.73% +0.09%
==========================================
Files 180 180
Lines 135241 135568 +327
Branches 135241 135568 +327
==========================================
+ Hits 119869 120294 +425
+ Misses 12607 12506 -101
- Partials 2765 2768 +3
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:
|
TheBlueMatt
force-pushed
the
2025-10-less-panicy-reads
branch
from
2b29c85 to
696e213
Compare
| } else { | ||
| panic!("Found monitor for channel {channel_id} with no updates since v0.0.118. \ | ||
| These monitors are no longer supported. \ | ||
| To continue, run a v0.1 release, send/route a payment over the channel or close it."); |
There was a problem hiding this comment.
Hm, so what if the monitor is already closed and you still have balances to claim? Is there any way you can realistically start up? I guess this would be pretty unlikely, unless you have an outbound payment you still need to time out on chain.
There was a problem hiding this comment.
No, you can't without manually deleting the monitor from your store. It really sucks and I'm not really happy with the state of things but it seems a bit late to walk this back :/
| // return Ok(None) to allow it to be skipped and not loaded. | ||
| return Ok(None); | ||
| } else { | ||
| panic!("Found monitor for channel {channel_id} with no updates since v0.0.118. \ |
There was a problem hiding this comment.
This mentions 0.0.118 but the commit mentions 0.0.116
There was a problem hiding this comment.
Oh, that is from 601bf4b. I'll fix but looks like it was actually in 0.0.119
| }); | ||
|
|
||
| if counterparty_node_id.is_none() { | ||
| if (holder_tx_signed || lockdown_from_offchain) && monitor.get_claimable_balances().is_empty() { |
There was a problem hiding this comment.
Do we actually need to bother checking for (holder_tx_signed || lockdown_from_offchain)?
There was a problem hiding this comment.
Probably not but it seemed like a nice additional check to include cause I'm not at all confident in the balance set for really old monitors.
TheBlueMatt
force-pushed
the
2025-10-less-panicy-reads
branch
from
696e213 to
74ec7a6
Compare
|
Tweaked the commit message to refer to the correct version number and also pushed a small fixup. |
|
LGTM after squash |
In 601bf4b we started refusing to load `ChannelMonitor`s which were created prior to LDK 0.0.110 and which were last updated prior to LDK 0.0.119. While this is likely fine for nearly all of our users, there's some risk that some (like me) have ancient un-archived `ChannelMonitor`s. We do need to start auto-archiving `ChannelMonitor`s but for now we need some way for such users to at least not fail on startup. Sadly, ancient `ChannelMonitor`s often don't contain some of the data we need to accurately calculate `Balance`s, but in cases where they do, and where there are no claimable `Balance`s, we can be confident we don't need the `ChannelMonitor`s. Thus, here, we adapt the `ChannelMonitor` `Readable` implementation used in the persistence wrappers to detect this case and simply skip loading such `ChannelMonitor`s. Its not clear if this is sufficient, but its at least better than the current state of affairs after 601bf4b.
TheBlueMatt
force-pushed
the
2025-10-less-panicy-reads
branch
from
74ec7a6 to
b1fc759
Compare
|
Squashed without further changes. |
|
This is a trivial patch, at least if you take |
| "ChannelMonitor was stale, with no updates since LDK 0.0.118. \ | ||
| It cannot be read by modern versions of LDK, though also does not contain any funds left to sweep. \ | ||
| You should manually delete it instead", |
There was a problem hiding this comment.
Might be worth including the monitor_key given this is called indirectly from read_all_channel_monitors_with_updates.
4 participants
In 601bf4b we started refusing to load
ChannelMonitors which were created prior to LDK 0.0.110 and which were last updated prior to LDK 0.0.116. While this is likely fine for nearly all of our users, there's some risk that some (like me) have ancient un-archivedChannelMonitors.We do need to start auto-archiving
ChannelMonitors but for now we need some way for such users to at least not fail on startup. Sadly, ancientChannelMonitors often don't contain some of the data we need to accurately calculateBalances, but in cases where they do, and where there are no claimableBalances, we can be confident we don't need theChannelMonitors.Thus, here, we adapt the
ChannelMonitorReadableimplementation used in the persistence wrappers to detect this case and simply skip loading suchChannelMonitors. Its not clear if this is sufficient, but its at least better than the current state of affairs after 601bf4b.