ChannelManager read refactor follow ups #4374

joostjager · 2026-02-03T13:05:35Z

Followup commits to improve the readability and maintainability of the ChannelManager deserialization code.

Reorder the struct fields to place all three `_legacy` fields together at the end, allowing the explanatory comment to appear once instead of being duplicated three times. Co-Authored-By: Claude Opus 4.5 <[email protected]>

Initialize pending_claiming_payments and monitor_update_blocked_actions_per_peer with None and resolve with unwrap_or_else, matching the pattern used for other optional hash map fields like pending_intercepted_htlcs_legacy. Co-Authored-By: Claude Opus 4.5 <[email protected]>

ldk-reviews-bot · 2026-02-03T13:05:38Z

👋 Thanks for assigning @valentinewallace as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2026-02-03T15:43:05Z

Codecov Report

❌ Patch coverage is 73.68421% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.02%. Comparing base (35ab03f) to head (b4429bb).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	73.68%	13 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4374      +/-   ##
==========================================
+ Coverage   85.99%   86.02%   +0.02%     
==========================================
  Files         156      156              
  Lines      102766   102844      +78     
  Branches   102766   102844      +78     
==========================================
+ Hits        88378    88471      +93     
+ Misses      11879    11863      -16     
- Partials     2509     2510       +1

Flag	Coverage Δ
tests	`86.02% <73.68%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

valentinewallace

Care to throw in any of these mentioned updates? #4332 (comment) Seems like a good opportunity

valentinewallace · 2026-02-03T17:01:31Z

lightning/src/ln/channelmanager.rs

 			testing_dnssec_proof_offer_resolution_override: Mutex::new(new_hash_map()),
 		};

+		// Step 7: Replay pending claims and fail HTLCs.


In my experience using step numbers in ldk-sample main, it's kinda annoying to have to update all of them when something changes. Maybe that's not as much of a risk here

I thought it might help a bit with navigation in this huge function. In other languages one would quickly extract methods, but in my experience that isn't always in Rust. Would you prefer that? I can try it.

I wouldn't really prefer extracting methods. Just wanted to point it out, we can keep the numbers

Made it unnumbered headings

ldk-reviews-bot · 2026-02-03T17:02:08Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

Add step comments to from_channel_manager_data to clarify the seven logical phases of ChannelManager reconstruction: 1. Channel/monitor reconciliation 2. Initialize missing optional fields 3. Replay in-flight monitor updates 4. Reconstruct HTLC state from monitors 5. Reconstruct claimable payments 6. Construct the ChannelManager 7. Replay pending claims and fail HTLCs Co-Authored-By: Claude Opus 4.5 <[email protected]>

Since there is no semantic difference between None and Some(empty vec) for peer_storage_dir, simplify the type to Vec and use unwrap_or_default() when reading from TLV fields. Co-Authored-By: Claude Opus 4.5 <[email protected]>

Instead of keeping claimable_htlc_onion_fields as a separate field in ChannelManagerData and zipping it in stage 2, perform the zip in stage 1 so that claimable_htlcs_list already contains the onion fields. This simplifies stage 2 by removing the nested if-else for onion fields handling. Co-Authored-By: Claude Opus 4.5 <[email protected]>

Since there is no semantic difference between None and Some(empty map) for in_flight_monitor_updates, simplify the type to HashMap and use unwrap_or_default() when reading from TLV fields. Co-Authored-By: Claude Opus 4.5 <[email protected]>

joostjager · 2026-02-04T10:10:42Z

Care to throw in any of these mentioned updates? #4332 (comment) Seems like a good opportunity

Done

valentinewallace

Nice cleanups here!

valentinewallace · 2026-02-04T16:49:48Z

lightning/src/ln/channelmanager.rs

+		// consistency. Channels that are behind their monitors are force-closed. Channels without
+		// monitors (except those awaiting initial persistence) are rejected. Monitors without


I don't think it's true that channels without monitors will be rejected -- instead, we'll DecodeError::InvalidValue on the entire manager read. Were these comments added by Claude? I'm kinda not sold on them, they add to the review quite a bit and seem error prone/at risk of becoming out-of-date

valentinewallace · 2026-02-04T16:51:15Z

lightning/src/ln/channelmanager.rs

 	async_receive_offer_cache: AsyncReceiveOfferCache,
+	// Marked `_legacy` because in versions > 0.2 we are taking steps to remove the requirement of
+	// regularly persisting the `ChannelManager` and instead rebuild the set of HTLC forwards from
+	// `Channel{Monitor}` data. See [`ChannelManager::read`].


I don't think ChannelManager::read adds anything to the understanding of what's going on here, can probably remove that sentence

Yes, this was pointing to the reconstruct boolean, but can't link that.

valentinewallace · 2026-02-04T17:03:39Z

lightning/src/ln/channelmanager.rs

+				.into_iter()
+				.zip(onion_fields)
+				.map(|((hash, htlcs), onion)| (hash, htlcs, onion))
+				.collect()


Hmm, we introduce an intermediate allocation here. Wonder if the whole creation of claimable_payments can move here, to avoid it? It feels somewhat fitting.

I've considered that, but it would need the keys for the legacy handling branch. But perhaps that's ok? The distinction between stage 1 and 2 isn't carved in stone.

valentinewallace · 2026-02-04T17:10:17Z

lightning/src/ln/channelmanager.rs

 				.unwrap_or_else(new_hash_map),
 			inbound_payment_id_secret,
-			in_flight_monitor_updates,
+			in_flight_monitor_updates: in_flight_monitor_updates.unwrap_or_default(),


Will this use HashMap::new() or new_hash_map (which I think is what we want)? CI seems fine though...

joostjager and others added 2 commits February 3, 2026 13:28

joostjager requested a review from valentinewallace February 3, 2026 13:25

joostjager marked this pull request as ready for review February 3, 2026 13:25

joostjager mentioned this pull request Feb 3, 2026

Split ChannelManager::read into two stages #4332

Merged

valentinewallace reviewed Feb 3, 2026

View reviewed changes

joostjager and others added 4 commits February 4, 2026 10:17

joostjager force-pushed the chanmgr-read-2 branch from 13a8a3f to b4429bb Compare February 4, 2026 10:08

joostjager requested a review from valentinewallace February 4, 2026 10:10

valentinewallace reviewed Feb 4, 2026

View reviewed changes

		// consistency. Channels that are behind their monitors are force-closed. Channels without
		// monitors (except those awaiting initial persistence) are rejected. Monitors without

ChannelManager read refactor follow ups #4374

Are you sure you want to change the base?

ChannelManager read refactor follow ups #4374

Conversation

joostjager commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Feb 3, 2026

Uh oh!

joostjager commented Feb 4, 2026

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joostjager commented Feb 3, 2026 •

edited

Loading

ldk-reviews-bot commented Feb 3, 2026 •

edited

Loading

codecov bot commented Feb 3, 2026 •

edited

Loading

joostjager Feb 4, 2026 •

edited

Loading