ChannelManager read refactor follow ups by joostjager · Pull Request #4374 · lightningdevkit/rust-lightning

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

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

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 53.19149% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.01%. Comparing base (35ab03f) to head (b509c4a).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	53.19%	43 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4374      +/-   ##
==========================================
+ Coverage   85.99%   86.01%   +0.01%     
==========================================
  Files         156      156              
  Lines      102766   102846      +80     
  Branches   102766   102846      +80     
==========================================
+ Hits        88378    88463      +85     
+ Misses      11879    11873       -6     
- Partials     2509     2510       +1

Flag	Coverage Δ
tests	`86.01% <53.19%> (+0.01%)`	⬆️

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.

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

Yes, clauded indeed. Unfortunate. First I went and corrected them, but I also felt what you mention about becoming out-of-date. In my opinion, there aren't enough comments in the code base, but comments that are located a bit further away from the code aren't ideal. Dropped the commit.

lightning/src/ln/channelmanager.rs

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.

Updated the commit with this. It's seem better indeed.

I like this update 👌

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...

I think it is the same?

HashMap::default() -> creates HashMap with RandomState::default() -> which calls RandomState::new() which is what new_hash_map does too?

If true, we could replace all new_hash_map calls with HashMap::default(), to make it look slightly less custom.

new_hash_map calls a different RandomState impl based on whether we're in std or no-std 🤔 It seems different, seems like a bug that CI is passing, potentially. Pretty sure there's a reason we'd have a custom call to new_hash_map everywhere. Here's the commit where it was added: 3096061

I think we should double-check this, but landed since it conflicts with #4303 and we can do another minor follow-up if it's an issue.

Ok, will look into it

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 <noreply@anthropic.com>

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 <noreply@anthropic.com>

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 <noreply@anthropic.com>

Move the reconstruction of claimable_payments from the main ChannelManager::read into ChannelManagerData::read. This removes the separate claimable_htlc_purposes and claimable_htlc_onion_fields fields from ChannelManagerData, replacing them with the combined claimable_payments HashMap. This requires adding a node_signer parameter to ChannelManagerDataReadArgs to support verification of legacy hop data when reconstructing payment purposes for very old serialized data (pre-0.0.107). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

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 <noreply@anthropic.com>

joostjager · 2026-02-05T09:43:44Z

Diff: https://github.com/lightningdevkit/rust-lightning/compare/b4429bb4f5bcab729e47028b1c4ce12b2960b136..b509c4a93d5cf46df24d1d1f20b5008741d61745

valentinewallace · 2026-02-05T16:52:29Z

lightning/src/ln/channelmanager.rs

+		} else {
+			// LDK versions prior to 0.0.107 did not write a `pending_htlc_purposes`, but do
+			// include a `_legacy_hop_data` in the `OnionPayload`.
+			for (payment_hash, htlcs) in claimable_htlcs_list.into_iter() {


There's a few minor differences in the code move, like this goes from drain to into_iter here. Maybe it's fine for this PR but in general it's best to keep a pure code move in its own commit.

I indeed verified before pushing using git diff -M, and from what I saw that was the only change. True that I could have kept that as is.

valentinewallace · 2026-02-05T16:56:03Z

lightning/src/ln/channelmanager.rs

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


I like this update 👌

valentinewallace · 2026-02-05T17:01:28Z

Landed since this conflicts with #4303 but should look into whether we need to switch to a new_hash_map call: #4374 (comment)

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 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

joostjager and others added 5 commits February 5, 2026 10:23

joostjager force-pushed the chanmgr-read-2 branch from b4429bb to b509c4a Compare February 5, 2026 09:40

joostjager requested a review from valentinewallace February 5, 2026 09:44

valentinewallace approved these changes Feb 5, 2026

View reviewed changes

valentinewallace merged commit 86f914c into lightningdevkit:main Feb 5, 2026
21 checks passed

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

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager Feb 5, 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!

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!

joostjager commented Feb 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjager Feb 5, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

valentinewallace commented Feb 5, 2026

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

joostjager Feb 5, 2026 •

edited

Loading

joostjager Feb 5, 2026 •

edited

Loading