Uh oh!
There was an error while loading. Please reload this page.
Reload channelmanager with channel data from monitors - #4151
Conversation
ldk-reviews-bot
commented
Oct 9, 2025
👋 Hi! I see this is a draft PR. |
dd9d6db to
c9d03d4Compare2eb7981 to
4eb87c1Compare| let mut failed_htlcs = Vec::new(); | ||
| let channel_count: u64 = Readable::read(reader)?; | ||
| // Discard channel manager versions of channels. |
There was a problem hiding this comment.
Looks like we'll need to use these manager versions of channels if the monitor versions aren't set (i.e. reading a manager that was last serialized on <= 0.2). Ideal would be to have an upgrade test, i.e. in upgrade_downgrade_tests.rs.
There was a problem hiding this comment.
I think we want to leave this until we've got solid confidence in the approach taken? Ofc agreed that we need to implement upgrade.
| /// The encoded channel data associated with this ChannelMonitor, if any. | ||
| #[cfg(feature = "safe_channels")] | ||
| pub encoded_channel: Option<Vec<u8>>, |
There was a problem hiding this comment.
Did you explore encoding the FundedChannel itself rather than a bag of bytes? I guess it would require propagating the signer trait parameter, which is kind of annoying?
There was a problem hiding this comment.
I considered it, but indeed, the signer parameter is annoying. But your comment did make me think again. I will try and see what we can do with a copy of FundedChannel just for data storage. Perhaps that's useful too once we start stripping out the redundant fields?
There was a problem hiding this comment.
One thing I noticed while trying this is that ChannelContext contains blocked_monitor_updates: Vec<PendingChannelMonitorUpdate>. PendingChannelMonitorUpdate will contain FundedChannel snapshots. Nesting 😨
There was a problem hiding this comment.
| // for the safe_channels feature. | ||
| #[cfg(feature = "safe_channels")] | ||
| if let Some(ref encoded_channel) = channel_monitor.encoded_channel { | ||
| channel_monitor.check_encoded_channel_consistency(encoded_channel); |
There was a problem hiding this comment.
May be better to have the above docs (or duplicate them) on the method itself
| /// The serialized channel state as provided via the last `ChannelMonitorUpdate` or via a call to | ||
| /// [`ChannelMonitor::update_encoded_channel`]. |
There was a problem hiding this comment.
Maybe note that this is a FundedChannel and explain why we're encoding it here?
| if let Ok(check_data) = check_res { | ||
| debug_assert!( | ||
| check_data.cur_holder_commitment_transaction_number | ||
| <= self.get_cur_holder_commitment_number(), |
There was a problem hiding this comment.
Can you document generally why it's okay for the channel to be behind the monitor (but not ahead)?
| } | ||
| #[cfg(feature = "safe_channels")] | ||
| pub fn read_check_data<R: io::Read>(reader: &mut R) -> Result<ChannelStateCheckData, DecodeError> { |
4eb87c1 to
1adfd7dCompareCodecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #4151 +/- ##
==========================================
- Coverage 89.33% 89.29% -0.05%
==========================================
Files 180 180 Lines 139042 139132 +90 Branches 139042 139132 +90 ==========================================
+ Hits 124219 124231 +12 - Misses 12196 12275 +79 + Partials 2627 2626 -1
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:
|
joostjager
commented
Nov 19, 2025
Added more cfg gating for |
5967f55 to
b096bf7Comparejoostjager
commented
Nov 25, 2025
If we go with unencoded channels in the monitors and monitor updates, this PR would come out along the following lines: https://github.com/lightningdevkit/rust-lightning/compare/main...joostjager:rust-lightning:chanmgr-refactor-passthrough-unencoded?expand=1 In that branch, I've also stripped away all of chanmgr::read except for the static data. This leads to 85 failing tests, which would be the base line to go work off of. |
e87a727 to
5859132Comparejoostjager
commented
Nov 27, 2025
Rebased PR on top of unencoded channel state in |
9168f08 to
74285c1CompareAdditional trace logs to help with debugging.
Add a new `safe_channels` feature flag that gates in-development work toward persisting channel monitors and channels atomically, preventing them from desynchronizing and causing force closures. This commit begins that transition by storing both pieces together and adding consistency checks during writes. These checks mirror what the channel manager currently validates only on reload, but performing them earlier increases coverage and surfaces inconsistencies sooner.
74285c1 to
501c599Comparejoostjager
commented
Mar 20, 2026
Implemented safe channels via #4351 |
PoC branch to try out persisting channels with the monitors rather than chanmgr. Goal: fix force closures.
Builds on #4218