Remove convert_channel_err! macro #4275

joostjager · 2025-12-11T09:31:50Z

Fewer macros, less pain

ldk-reviews-bot · 2025-12-11T09:31:53Z

👋 Thanks for assigning @tnull 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-11T11:06:23Z

Codecov Report

❌ Patch coverage is 90.64449% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.38%. Comparing base (fd85279) to head (d436cbf).
⚠️ Report is 20 commits behind head on main.

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

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #4275    +/-   ##
========================================
  Coverage   89.37%   89.38%            
========================================
  Files         180      180            
  Lines      139395   139623   +228     
  Branches   139395   139623   +228     
========================================
+ Hits       124591   124797   +206     
- Misses      12216    12241    +25     
+ Partials     2588     2585     -3

Flag	Coverage Δ
fuzzing	`35.23% <48.23%> (+0.04%)`	⬆️
tests	`88.71% <90.64%> (+<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.

tnull

Changes look good to me, but might be good to add some more docs now that we lost the macro's match arms which made sure we did the right thing(tm) before..

tnull · 2025-12-11T14:25:11Z

lightning/src/ln/channelmanager.rs

+	///     [`ChannelManager::finish_close_channel`].
+	///
+	/// Returns a mapped error.
+	fn convert_channel_err_coop(


The replicated docs don't really explain when you're supposed to use which variant now, and the names aren't super clear either (convert_channel_err_coop/convert_channel_err_funded/convert_channel_err). Can we add some more explanations to make up for the lost functionality of the macro match arms?

I've added the channel states to the docs to make it more clear which of the variants should be used. Looked into further documentation, but I think that would lead to more code restructuring to make the docs make sense.

The goal of this PR is just to get rid of the macros without letting the docs become worse.

This method is particularly easy to mis-use, and documentation doesn't make it clear that the code is wrong at the callsite. coop is maybe enough, but it would be very nice to be much more explicit here (its not like the method name is overly long to begin with) so that its clear from the callsite whether we're doing it wrong or not.

It also isn't converting an error. I've now named it get_coop_close_err. Let me know if you think this is better.

ldk-reviews-bot · 2025-12-11T14:26:34Z

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

tnull

LGTM

Relatively straightforward refactor, so landing this.

TheBlueMatt

Please fix the docs and maybe clean it up a bit in a followup.

TheBlueMatt · 2025-12-11T20:19:29Z

lightning/src/ln/channelmanager.rs

 		})
 	}

+	fn convert_funded_channel_err_internal(


Please place methods with other methods doing roughly similar things. There's already a bunch of error-handling down a few hundred lines (finish_close_channel, which even explicitly references some of the new methods). Breaking things up makes things harder to find.

TheBlueMatt · 2025-12-11T20:23:00Z

lightning/src/ln/channelmanager.rs

+	///     [`ChannelManager::finish_close_channel`].
+	///
+	/// Returns a mapped error.
+	fn convert_channel_err_coop(


This method is particularly easy to mis-use, and documentation doesn't make it clear that the code is wrong at the callsite. coop is maybe enough, but it would be very nice to be much more explicit here (its not like the method name is overly long to begin with) so that its clear from the callsite whether we're doing it wrong or not.

TheBlueMatt · 2025-12-11T20:27:20Z

lightning/src/ln/channelmanager.rs

+	}
+
+	/// When a cooperatively closed channel is removed, two things need to happen:
+	/// (a) This must be called in the same `per_peer_state` lock as the channel-closing action,


These docs are now all wrong - its not just "this" that must be called but one of a few methods.

My impression is that for a coop closed channel, this is the one method to call?

TheBlueMatt · 2025-12-11T20:29:59Z

lightning/src/ln/channelmanager.rs

+								&mut peer_state.closed_channel_monitor_update_ids,
+								&mut peer_state.in_flight_monitor_updates,


I really do not find this more readable than the macro was. Its not really a huge deal, but the macro hid some of the crap that had to be passed in because of lifetime restrictions without doing much else.

I do not buy that we should be removing macros for the sake of removing macros. There's some compilation-time wins to be had by reducing codegen steps through macro removal, and they're often hard to read in our codebase, but that doesn't mean that removing them is inherently easier to read just because they're gone, especially if they leave behind more code than we had.

From previous discussions during dev sync, I got that the general consensus is that we want to get rid of macros unless absolutely necessary.

For what it is worth, I personally find macros a source of micro friction. Code navigation breaks, references lookup breaks, stack traces aren't complete, debugging gets more difficult, identifying the applicable match arm is a manual thought step, the syntax is different and rustfmt acts weird sometimes.

I agree to the lifetime argument favoring macros, but in this case I thought it was worth it.

TheBlueMatt · 2025-12-11T20:34:37Z

lightning/src/ln/channelmanager.rs


 	/// When a channel is removed, two things need to happen:
-	/// (a) [`convert_channel_err`] must be called in the same `per_peer_state` lock as the
+	/// (a) [`ChannelManager::convert_channel_err`] must be called in the same `per_peer_state` lock as the


No, or one of the other few methods.

Updated phrasing to the more generic

The close error must be converted to [MsgHandleErrInternal]

TheBlueMatt · 2025-12-11T20:42:03Z

lightning/src/ln/channelmanager.rs

+					"Can't find a peer matching the passed counterparty node_id {}",
+					peer_node_id


As we go it would be nice to inline the format arguments where possible.

Suggested change

"Can't find a peer matching the passed counterparty node_id {}",

peer_node_id

"Can't find a peer matching the passed counterparty node_id {peer_node_id}"

Fixed together with another one further down.

joostjager · 2025-12-12T08:07:50Z

Follow ups: #4281

Convert convert_funded_channel_err fns to methods

004ceef

joostjager force-pushed the remove-convert-channel-err-macro branch from b34da00 to 73f182f Compare December 11, 2025 09:47

joostjager changed the title ~~Remove convert channel err macro~~ Remove convert_channel_err! macro Dec 11, 2025

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

joostjager requested a review from tnull December 11, 2025 11:25

tnull reviewed Dec 11, 2025

View reviewed changes

joostjager added 6 commits December 11, 2025 15:53

Convert macro to convert_channel_err_coop method

87e01ff

Convert macro to convert_channel_err_funded method

36cfb13

Replace macro with direct call to convert_unfunded_channel_err_internal

ec112c4

Convert macro to convert_channel_err method

ee42670

Remove rustfmt::skip from touched methods

7fe270b

Rustfmt touched methods

d436cbf

joostjager force-pushed the remove-convert-channel-err-macro branch from 73f182f to d436cbf Compare December 11, 2025 14:57

tnull approved these changes Dec 11, 2025

View reviewed changes

elnosh approved these changes Dec 11, 2025

View reviewed changes

tnull merged commit 84a791a into lightningdevkit:main Dec 11, 2025
17 checks passed

TheBlueMatt reviewed Dec 11, 2025

View reviewed changes

joostjager mentioned this pull request Dec 12, 2025

Follow ups to "Remove convert channel err macro" #4281

Open

		&mut peer_state.closed_channel_monitor_update_ids,
		&mut peer_state.in_flight_monitor_updates,

		"Can't find a peer matching the passed counterparty node_id {}",
		peer_node_id

Remove convert_channel_err! macro #4275

Remove convert_channel_err! macro #4275

Conversation

joostjager commented Dec 11, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

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

ldk-reviews-bot commented Dec 11, 2025

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

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!

joostjager Dec 12, 2025 • 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!

joostjager commented Dec 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ldk-reviews-bot commented Dec 11, 2025 •

edited

Loading

codecov bot commented Dec 11, 2025 •

edited

Loading

joostjager Dec 12, 2025 •

edited

Loading