Convert internal update handling fns to methods #4291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

joostjager wants to merge 7 commits into lightningdevkit:main from joostjager:update-handling-refactor

+582 −373

Contributor

joostjager commented Dec 19, 2025

Small readability improvement

ldk-reviews-bot commented Dec 19, 2025 •

edited

Loading

I've assigned @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.

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


          Convert internal update handling fns to methods

dfbf531

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

joostjager force-pushed the update-handling-refactor branch from f687020 to dfbf531 Compare

December 19, 2025 09:18

codecov bot commented Dec 19, 2025 •

edited

Loading

Codecov Report

❌ Patch coverage is 92.53012% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.63%. Comparing base (62c5849) to head (8cd1f51).
⚠️ Report is 10 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4291      +/-   ##
==========================================
- Coverage   89.38%   86.63%   -2.76%     
==========================================
  Files         180      158      -22     
  Lines      139834   102334   -37500     
  Branches   139834   102334   -37500     
==========================================
- Hits       124985    88652   -36333     
+ Misses      12262    11272     -990     
+ Partials     2587     2410     -177

Flag	Coverage Δ
fuzzing	`36.99% <65.06%> (+1.78%)`	⬆️
tests	`85.91% <91.32%> (-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.

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

ldk-reviews-bot commented Dec 22, 2025

🔔 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 commented Dec 24, 2025

🔔 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 commented Dec 27, 2025

🔔 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 commented Dec 29, 2025

🔔 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 commented Dec 31, 2025

🔔 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 commented Jan 3, 2026

🔔 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 commented Jan 5, 2026

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

View reviewed changes

Contributor

valentinewallace left a comment

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

ldk-reviews-bot commented Jan 5, 2026

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

View reviewed changes

Collaborator

TheBlueMatt left a comment

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.

Contributor Author

joostjager commented Jan 6, 2026

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.

Collaborator

TheBlueMatt commented Jan 6, 2026

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.

Contributor Author

joostjager commented Jan 7, 2026

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.

joostjager force-pushed the update-handling-refactor branch from a12908f to 8cd1f51 Compare

January 7, 2026 10:21

Contributor

valentinewallace commented Jan 7, 2026

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

valentinewallace reviewed

View reviewed changes

lightning/src/ln/channelmanager.rs

Comment on lines +12500 to +12502

+              								&mut peer_state.in_flight_monitor_updates,
+              								&mut peer_state.monitor_update_blocked_actions,
+              								&mut peer_state.pending_msg_events,
+              								peer_state.is_connected,

Contributor

valentinewallace Jan 7, 2026

Is it possible to just pass in &mut peer_state instead to DRY things?

lightning/src/ln/channelmanager.rs

+              	/// Like [`Self::handle_new_monitor_update`], but also returns whether this specific update
+              	/// completed (as opposed to being in-progress).
+              	fn handle_new_monitor_update_with_status(

Contributor

valentinewallace Jan 7, 2026

This is only called in one place, seems we can get rid of it

joostjager force-pushed the update-handling-refactor branch 3 times, most recently from 5e22a40 to 4ef8d7c Compare

January 8, 2026 11:41

joostjager and others added 3 commits

January 8, 2026 12:47


          Refactor monitor update completion into helper methods

cc3c1f9

Extract the monitor update completion logic from the
handle_monitor_update_completion macro into two new helper methods:

- prepare_monitor_update_completion_data: Collects all necessary data
  from the channel while peer_state locks are still held

- handle_monitor_update_completion_data: Processes the collected data
  after locks have been released

This refactoring improves code organization by separating the data
extraction phase (which requires locks) from the processing phase
(which should happen after locks are released). The macro is now a
thin wrapper that calls these two methods with proper lock management.

The new MonitorUpdateCompletionData struct serves as a container for
all the data that needs to be passed between the two phases.


          Inline handle_monitor_update_completion! macro

69867b5

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


          Remove handle_new_monitor_update_locked_actions_handled_by_caller macro

46caf9c

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>

joostjager and others added 3 commits

January 8, 2026 12:48


          Remove handle_initial_monitor macro

b4f2a6c

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


          Remove handle_post_close_monitor_update macro

88e4b20

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


          Remove handle_new_monitor_update macro

89a8981

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

joostjager force-pushed the update-handling-refactor branch from 4ef8d7c to 89a8981 Compare

January 8, 2026 11:49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet