Convert monitor update macros to ChannelManager methods #4291

joostjager · 2025-12-19T09:09:27Z

Replaces five monitor update handling macros with proper methods on ChannelManager:

handle_monitor_update_completion!
handle_initial_monitor!
handle_post_close_monitor_update!
handle_new_monitor_update_locked_actions_handled_by_caller!
handle_new_monitor_update!

Introduces MonitorUpdateCompletionData enum to cleanly separate the data extraction phase (requires locks) from the processing phase (after locks released).

Motivation

Macros make this code hard to debug, navigate, and understand due to implicit variable capture and non-obvious control flow. Methods provide clear signatures, explicit parameters, and proper stack traces.

No behavioral changes.

ldk-reviews-bot · 2025-12-19T09:09:30Z

👋 Thanks for assigning @TheBlueMatt 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 · 2025-12-19T10:55:32Z

Codecov Report

❌ Patch coverage is 93.56984% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.61%. Comparing base (62c5849) to head (20e8526).
⚠️ Report is 46 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	93.56%	26 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4291      +/-   ##
==========================================
- Coverage   89.38%   86.61%   -2.77%     
==========================================
  Files         180      158      -22     
  Lines      139834   102720   -37114     
  Branches   139834   102720   -37114     
==========================================
- Hits       124985    88975   -36010     
+ Misses      12262    11330     -932     
+ Partials     2587     2415     -172

Flag	Coverage Δ
fuzzing	`37.08% <64.30%> (+1.87%)`	⬆️
tests	`85.91% <93.12%> (-2.81%)`	⬇️

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.

ldk-reviews-bot · 2025-12-22T00:01:03Z

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-12-24T00:01:47Z

🔔 2nd Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-12-27T00:00:48Z

🔔 3rd Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-12-29T00:01:38Z

🔔 4th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-12-31T00:02:31Z

🔔 5th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-01-03T00:00:35Z

🔔 6th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-01-05T00:01:23Z

🔔 7th Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

valentinewallace

I'm kind of meh on whether this improves readability, tbh. Is it just the CM argument?

ldk-reviews-bot · 2026-01-05T16:33:10Z

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

TheBlueMatt

IMO this is a readability reduction - instead of having the code next to the macros that call it (that its effectively an implementation detail of), this moves the code to some unrelated part of the file. If we can make the macros a function great, but separating the code from the code calling it seems like a loss.

joostjager · 2026-01-06T08:12:31Z

In my opinion those functions really are methods based on what they access inside ChannelManager. Having them be actual methods - even when not located next to the macro code - is an improvement for me, but of course this is a matter of preference.

But as you both don't share this preference, the code should probably remain what it is.

TheBlueMatt · 2026-01-06T16:32:21Z

In my opinion those functions really are methods based on what they access inside ChannelManager.

I agree with this, but that really applies to the things that are still macros after this PR - the things that are being called should be methods on ChannelManager, not macros. The internal details of those things should remain with them, though. You may well be able to move them out of macros though - the borrow checker is way smarter than it was when some of the macros were written and some of them might not have been necessary when they were written either.

joostjager · 2026-01-07T10:20:30Z

Done in the latest push. This does introduce some duplication with the lock dropping and completion handling repeated at each call site. However, I think this is a worthwhile trade-off. As someone relatively new to the codebase, the macros have been a constant source of confusion. They obscure control flow, don't show up in symbol navigation, and create gaps in the call stack during debugging. The explicit code is much easier to follow and reason about, in my opinion.

Let's decide if we want to go this way (I think we should), and then I'll do a closer inspection of the claude generated code to see if it's all sound.

valentinewallace · 2026-01-07T15:54:46Z

Concept ACK, needs a fair amount of cleanup and would prefer if the last commit were broken up a bunch though

lightning/src/ln/channelmanager.rs

joostjager · 2026-01-08T12:07:23Z

Broke up the big refactor commit and scrutinized the claude code. I think it should be much more reviewable now.

ldk-reviews-bot · 2026-01-10T12:08:10Z

🔔 1st Reminder

Hey @TheBlueMatt @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-01-10T16:17:15Z

🔔 1st Reminder

Hey @TheBlueMatt @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-01-12T12:08:55Z

🔔 2nd Reminder

Hey @TheBlueMatt @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt

nice

TheBlueMatt · 2026-01-12T14:12:23Z

lightning/src/ln/channelmanager.rs

 			if let Some((_, funding_txo, _, update)) = shutdown_res.monitor_update.take() {
-				handle_new_monitor_update_locked_actions_handled_by_caller!(
-					self,
+				self.handle_new_monitor_update_internal(


Please do not call a method called _internal :). Rename it if its supposed to be called or improve the call semantics if not.

Agreed. Renamed to the simple update_channel_monitor.

update_channel_monitor absolutely doesn't capture the "this function is weird, you probably shouldn't use it, its only for cases where you have to handle the post-update stuff manually for some specific reason". We're gonna screw this up in the future and someone is gonna use update_channel_monitor thinking its just a normal method to call when they need to update the channel monitor and no one is gonna catch it in review.

The previous "_internal" at least communicated some of that... Do you have a suggestion? update_channel_monitor_without_completion_handling?

Or perhaps use the removed macro name handle_new_monitor_update_locked_actions_handled_by_caller ?

Going with handle_new_monitor_update_locked_actions_handled_by_caller, to also address @valentinewallace 's concern #4291 (comment)

TheBlueMatt · 2026-01-12T14:14:35Z

lightning/src/ln/channelmanager.rs

+
+	/// Processes monitor update completion data after locks have been released.
+	/// Call this after dropping peer_state_lock and per_peer_state locks.
+	fn handle_monitor_update_completion_data(&self, data: MonitorUpdateCompletionData) {


Please debug-assert locks not held.

TheBlueMatt · 2026-01-12T14:14:50Z

lightning/src/ln/channelmanager.rs

 );

+/// Data extracted from a channel while locks are held, to be processed after locks are released.
+enum MonitorUpdateCompletionData {


This needs a #[must_use]

valentinewallace

Really nice code navigation improvement

lightning/src/ln/channelmanager.rs

valentinewallace · 2026-01-12T18:43:54Z

lightning/src/ln/channelmanager.rs

-/// Useful because monitor updates need to be handled in the same mutex where the channel generated
-/// them (otherwise they can end up getting applied out-of-order) but it's not always possible to
-/// drop the aforementioned peer state locks at a given callsite. In this situation, use this macro
-/// to apply the monitor update immediately and handle the monitor update completion actions at a
-/// later time.


I think we should keep these docs around somewhere

Moved part of this to update_channel_monitor.

valentinewallace · 2026-01-12T19:00:04Z

lightning/src/ln/channelmanager.rs

+enum MonitorUpdateCompletionData {
+	/// Channel has blocked monitor updates pending. Only process update actions.
+	Blocked { update_actions: Vec<MonitorUpdateCompletionAction> },
+	/// Channel is fully unblocked and can be resumed.


The below fields get returned if the channel is successfully resumed, so this doesn't seem accurate to me

Both comments updated.

lightning/src/ln/channelmanager.rs

joostjager · 2026-01-13T09:39:15Z

Comments addressed. Not a fun job with the split into commits. diff

Pure code move except for the context logger which is now instantiated when needed in update_channel_monitor.

Extract the monitor update completion logic from the handle_monitor_update_completion macro into two new helper methods: - try_resume_channel_post_monitor_update: Attempts to resume a channel after a monitor update completes while locks are still held - handle_post_monitor_update_chan_resume: Completes channel resumption after locks have been released This refactoring improves code organization by separating the locked phase (which may resume the channel) from the unlocked phase (which processes remaining work). The macro is now a thin wrapper that calls these two methods with proper lock management. The new PostMonitorUpdateChanResume enum captures the result of the resume attempt, containing any remaining work to process after locks are released.

Expand the macro at its three call sites and remove the macro definition, as it no longer provides significant abstraction benefit after refactoring the core logic into helper methods. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>

🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>

Convert the handle_initial_monitor! macro to a method that returns optional completion data, allowing callers to release locks before processing the completion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>

Convert the handle_post_close_monitor_update! macro to a method that returns optional completion actions, allowing callers to release locks before processing the completion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>

Convert the handle_new_monitor_update macro to methods and update all call sites. Adds handle_new_monitor_update and handle_new_monitor_update_with_status methods that return completion data for processing after locks are released. Also removes handle_monitor_update_completion macro as it's no longer needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>

valentinewallace

Really liking this change.

I think some of the methods could be reordered (like, start with higher level ones handle_initial_monitor_update -> handle_new_update -> handle_post_close_update, with the more internal-only ones below). But not gonna hold this up, I think it's a strict improvement as-is

valentinewallace · 2026-01-13T20:58:51Z

lightning/src/ln/channelmanager.rs

@@ -14767,12 +14993,12 @@ where
 											insert_short_channel_id!(short_to_chan_info, funded_channel);


I question whether we're losing something by removing the locked_actions_handled_by_caller part of the name. But also, I don't even see the actions being handled at some of these callsites, so not sure I have full context here.

Renamed update_channel_monitor to the old name

TheBlueMatt

This takes cargo rustc --profile=test --lib -- -Zunpretty=expanded from 479,448 LoC to 478,346 LoC. Honestly I was hoping it'd be much more :(

TheBlueMatt · 2026-01-13T22:25:14Z

lightning/src/ln/channelmanager.rs

 			if let Some((_, funding_txo, _, update)) = shutdown_res.monitor_update.take() {
-				handle_new_monitor_update_locked_actions_handled_by_caller!(
-					self,
+				self.handle_new_monitor_update_internal(


update_channel_monitor absolutely doesn't capture the "this function is weird, you probably shouldn't use it, its only for cases where you have to handle the post-update stuff manually for some specific reason". We're gonna screw this up in the future and someone is gonna use update_channel_monitor thinking its just a normal method to call when they need to update the channel monitor and no one is gonna catch it in review.

…ions_handled_by_caller The previous name didn't capture that this function is unusual and requires the caller to handle post-update actions manually. The new name makes it clear that callers are responsible for handling locked actions themselves, reducing the risk of misuse. Co-Authored-By: Claude Opus 4.5 <[email protected]>

joostjager · 2026-01-14T18:31:47Z

@TheBlueMatt pushed rename in a separate commit. Didn't want to go through the whole commit stack with its macros once again.

joostjager force-pushed the update-handling-refactor branch from dd25afd to f687020 Compare December 19, 2025 09:10

joostjager changed the title ~~Convert internal update handling methods to methods~~ Convert internal update handling fns to methods Dec 19, 2025

joostjager force-pushed the update-handling-refactor branch from f687020 to dfbf531 Compare December 19, 2025 09:18

joostjager marked this pull request as ready for review December 19, 2025 11:41

ldk-reviews-bot requested a review from valentinewallace December 19, 2025 11:42

valentinewallace reviewed Jan 5, 2026

View reviewed changes

TheBlueMatt reviewed Jan 5, 2026

View reviewed changes

joostjager force-pushed the update-handling-refactor branch from a12908f to 8cd1f51 Compare January 7, 2026 10:21

valentinewallace reviewed Jan 7, 2026

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

joostjager force-pushed the update-handling-refactor branch 5 times, most recently from 89a8981 to f59d1ab Compare January 8, 2026 12:06

joostjager requested a review from valentinewallace January 8, 2026 12:07

joostjager changed the title ~~Convert internal update handling fns to methods~~ Convert monitor update macros to ChannelManager methods Jan 8, 2026

joostjager force-pushed the update-handling-refactor branch from f59d1ab to abdbf82 Compare January 8, 2026 12:14

joostjager requested a review from TheBlueMatt January 8, 2026 16:16

joostjager added this to Weekly Goals Jan 8, 2026

joostjager self-assigned this Jan 8, 2026

TheBlueMatt reviewed Jan 12, 2026

View reviewed changes

valentinewallace reviewed Jan 12, 2026

View reviewed changes

joostjager force-pushed the update-handling-refactor branch from abdbf82 to 1bb7c18 Compare January 13, 2026 09:37

joostjager and others added 7 commits January 13, 2026 10:39

Convert internal update handling fns to methods

b6b622f

Pure code move except for the context logger which is now instantiated when needed in update_channel_monitor.

Remove handle_new_monitor_update_locked_actions_handled_by_caller macro

a9656dd

🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>

joostjager force-pushed the update-handling-refactor branch from 1bb7c18 to a160bbf Compare January 13, 2026 09:53

joostjager requested review from TheBlueMatt and valentinewallace January 13, 2026 11:56

valentinewallace previously approved these changes Jan 13, 2026

View reviewed changes

TheBlueMatt reviewed Jan 13, 2026

View reviewed changes

joostjager dismissed valentinewallace’s stale review via 20e8526 January 14, 2026 18:30

joostjager requested a review from TheBlueMatt January 14, 2026 18:33

		@@ -14767,12 +14993,12 @@ where
		insert_short_channel_id!(short_to_chan_info, funded_channel);

Convert monitor update macros to ChannelManager methods #4291

Are you sure you want to change the base?

Convert monitor update macros to ChannelManager methods #4291

Conversation

joostjager commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Uh oh!

ldk-reviews-bot commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ldk-reviews-bot commented Dec 22, 2025

Uh oh!

ldk-reviews-bot commented Dec 24, 2025

Uh oh!

ldk-reviews-bot commented Dec 27, 2025

Uh oh!

ldk-reviews-bot commented Dec 29, 2025

Uh oh!

ldk-reviews-bot commented Dec 31, 2025

Uh oh!

ldk-reviews-bot commented Jan 3, 2026

Uh oh!

ldk-reviews-bot commented Jan 5, 2026

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Jan 5, 2026

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

joostjager commented Jan 6, 2026

Uh oh!

TheBlueMatt commented Jan 6, 2026

Uh oh!

joostjager commented Jan 7, 2026

Uh oh!

valentinewallace commented Jan 7, 2026

Uh oh!

Uh oh!

Uh oh!

joostjager commented Jan 8, 2026

Uh oh!

ldk-reviews-bot commented Jan 10, 2026

Uh oh!

ldk-reviews-bot commented Jan 10, 2026

Uh oh!

ldk-reviews-bot commented Jan 12, 2026

Uh oh!

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

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!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joostjager commented Dec 19, 2025 •

edited

Loading

ldk-reviews-bot commented Dec 19, 2025 •

edited

Loading

codecov bot commented Dec 19, 2025 •

edited

Loading

joostjager commented Jan 13, 2026 •

edited

Loading