Skip to content

Conversation

@tnull
Copy link
Contributor

@tnull tnull commented Nov 25, 2025

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.

@tnull tnull requested a review from TheBlueMatt November 25, 2025 13:45
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Nov 25, 2025

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

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

codecov bot commented Nov 25, 2025

Codecov Report

❌ Patch coverage is 52.88136% with 139 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.31%. Comparing base (bb5504e) to head (c7a57c8).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/peer_handler.rs 52.39% 137 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4244      +/-   ##
==========================================
- Coverage   89.33%   89.31%   -0.02%     
==========================================
  Files         180      180              
  Lines      138680   138824     +144     
  Branches   138680   138824     +144     
==========================================
+ Hits       123888   123994     +106     
- Misses      12172    12203      +31     
- Partials     2620     2627       +7     
Flag Coverage Δ
fuzzing 35.09% <41.19%> (-0.90%) ⬇️
tests 88.64% <28.47%> (-0.06%) ⬇️

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

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

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ldk-reviews-bot
Copy link

👋 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 tnull force-pushed the 2025-11-cleanup-wire-message-writeable branch from 3c0a1d7 to 734b4e2 Compare December 1, 2025 13:12
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.
@tnull tnull force-pushed the 2025-11-cleanup-wire-message-writeable branch from 734b4e2 to c7a57c8 Compare December 1, 2025 13:12
@tnull tnull changed the title Replace wire::Message's Writeable implementation with unreachable! Consistently use wire::Message for encoding Dec 1, 2025
@tnull
Copy link
Contributor Author

tnull commented Dec 1, 2025

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.

@tnull tnull requested a review from TheBlueMatt December 1, 2025 13:19
@ldk-reviews-bot
Copy link

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

@tnull tnull self-assigned this Dec 4, 2025
@tnull tnull moved this to Goal: Merge in Weekly Goals Dec 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Goal: Merge

Development

Successfully merging this pull request may close these issues.

3 participants