Consistently use `wire::Message` for encoding #4244

tnull · 2025-11-25T13:45:06Z

Consistently use wire::Message for encoding network messages
Previously, `enqueue_message` took an `M: Type + Writeable` reference,
which didn't make use of our `wire::Message` type, which turned out to
be rather confusing. Here, we use `Message` consistently in
`PeerManager`'s `enqueue_message`, but also in `encrypt_message`, etc.

Now that we consistently use `wire::Message` everywhere, it's easier to
simply use `Message::write`/`Type::write` instead of heaving yet another
`wire::write` around. Here we drop `wire::write`, replace the
`encode_msg` macro with a method that takes `wire::Message`, and convert
a bunch of additional places to move semantics.

ldk-reviews-bot · 2025-11-25T13:45:08Z

👋 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-11-25T15:02:00Z

Codecov Report

❌ Patch coverage is 53.55649% with 111 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.34%. Comparing base (bb5504e) to head (3b186c2).
⚠️ Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/peer_handler.rs	53.19%	108 Missing and 2 partials ⚠️
lightning/src/ln/channelmanager.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4244      +/-   ##
==========================================
+ Coverage   89.33%   89.34%   +0.01%     
==========================================
  Files         180      180              
  Lines      138680   139092     +412     
  Branches   138680   139092     +412     
==========================================
+ Hits       123888   124272     +384     
- Misses      12172    12192      +20     
- Partials     2620     2628       +8

Flag	Coverage Δ
fuzzing	`35.05% <41.66%> (-0.94%)`	⬇️
tests	`88.68% <31.79%> (-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.

ldk-reviews-bot · 2025-11-27T13:45:25Z

🔔 1st Reminder

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

It seems weird to replace a correct but unused implementation with an incorrect/panicing one? Can we remove the bound instead?

ldk-reviews-bot · 2025-11-29T00:49:44Z

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

Previously, `enqueue_message` took an `M: Type + Writeable` reference, which didn't make use of our `wire::Message` type, which turned out to be rather confusing. Here, we use `Message` consistently in `PeerManager`'s `enqueue_message`, but also in `encrypt_message`, etc. While at it we also switch to move semantics, which is a nice cleanup.

tnull · 2025-12-01T13:17:09Z

It seems weird to replace a correct but unused implementation with an incorrect/panicing one? Can we remove the bound instead?

Hmm, turns out I had overlooked a single case where it was used (only when writing out messages on shutdown via peers_to_disconnect). In any case, having all these multiple ways how to write messages around and figuring out which is used when is rather confusing. I now updated this PR to:

a) always consistently use wire::Message when writing (e.g., in enqueue_message/encrypt_message etc).
b) drop wire::write, refactor the encode_msg macro to be a method taking wire::Message
c) convert a lot of places to move semantics while at it.

ldk-reviews-bot · 2025-12-03T13:20:23Z

🔔 1st Reminder

Hey @TheBlueMatt! 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 · 2025-12-05T13:05:33Z

lightning/src/ln/channelmanager.rs

 							&MessageSendEvent::UpdateHTLCs { .. } => false,
 							&MessageSendEvent::SendRevokeAndACK { .. } => false,
 							&MessageSendEvent::SendClosingSigned { .. } => false,
+							#[cfg(simple_close)]


we've historically not cfg-flagged messages even if the implementation in LDK isn't complete. There doesn't really seem any reason to imo.

we've historically not cfg-flagged messages even if the implementation in LDK isn't complete. There doesn't really seem any reason to imo.

Hmm, yeah, debatable. But this PR seems the wrong place to drop the cfg(simple_close). I only added the instances that became necessary by the new approach.

lightning/src/ln/peer_handler.rs

TheBlueMatt · 2025-12-05T13:13:27Z

lightning/src/ln/peer_handler.rs


 	/// Append a message to a peer's pending outbound/write buffer
-	fn enqueue_message<M: wire::Type>(&self, peer: &mut Peer, message: &M) {
+	fn enqueue_message<T: wire::Type>(&self, peer: &mut Peer, message: Message<T>) {


Oh nice, this should reduce codegen somewhat, though you should be able to remove lots of : Message<<CMH::Target as CustomMessageReader>::CustomMessage> across the file if you make this non-generic:

Suggested change

fn enqueue_message<T: wire::Type>(&self, peer: &mut Peer, message: Message<T>) {

fn enqueue_message(&self, peer: &mut Peer, message: Message<<CMH::Target as CustomMessageReader>::CustomMessage>>) {

Ah, indeed, now added a fixup.

Now that we consistently use `wire::Message` everywhere, it's easier to simply use `Message::write`/`Type::write` instead of heaving yet another `wire::write` around. Here we drop `wire::write`, replace the `encode_msg` macro with a method that takes `wire::Message`, and convert a bunch of additional places to move semantics.

TheBlueMatt

Feel free to squash

tnull requested a review from TheBlueMatt November 25, 2025 13:45

tnull force-pushed the 2025-11-cleanup-wire-message-writeable branch from bea6480 to 3c0a1d7 Compare November 25, 2025 13:45

TheBlueMatt reviewed Nov 29, 2025

View reviewed changes

tnull force-pushed the 2025-11-cleanup-wire-message-writeable branch 2 times, most recently from 734b4e2 to c7a57c8 Compare December 1, 2025 13:12

tnull changed the title ~~Replace wire::Message's Writeable implementation with unreachable!~~ Consistently use wire::Message for encoding Dec 1, 2025

tnull requested a review from TheBlueMatt December 1, 2025 13:19

tnull added this to Weekly Goals Dec 4, 2025

tnull self-assigned this Dec 4, 2025

tnull moved this to Goal: Merge in Weekly Goals Dec 4, 2025

TheBlueMatt reviewed Dec 5, 2025

View reviewed changes

tnull added 2 commits December 5, 2025 14:59

f Concretize enqueue_message

2c7f1cf

tnull force-pushed the 2025-11-cleanup-wire-message-writeable branch from c7a57c8 to 3b186c2 Compare December 5, 2025 14:00

tnull requested a review from TheBlueMatt December 5, 2025 14:41

TheBlueMatt reviewed Dec 5, 2025

View reviewed changes

	fn enqueue_message<T: wire::Type>(&self, peer: &mut Peer, message: Message<T>) {
	fn enqueue_message(&self, peer: &mut Peer, message: Message<<CMH::Target as CustomMessageReader>::CustomMessage>>) {

Consistently use wire::Message for encoding #4244

Are you sure you want to change the base?

Consistently use wire::Message for encoding #4244

Uh oh!

Conversation

tnull commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

codecov bot commented Nov 25, 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 Nov 27, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Nov 29, 2025

Uh oh!

tnull commented Dec 1, 2025

Uh oh!

ldk-reviews-bot commented Dec 3, 2025

Uh oh!

TheBlueMatt Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

tnull Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

tnull Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Consistently use `wire::Message` for encoding #4244

Consistently use `wire::Message` for encoding #4244

tnull commented Nov 25, 2025 •

edited

Loading

ldk-reviews-bot commented Nov 25, 2025 •

edited

Loading

codecov bot commented Nov 25, 2025 •

edited

Loading