`rustfmt`: Run on `peer_handler.rs` #3725

tnull · 2025-04-09T11:54:32Z

No description provided.

ldk-reviews-bot · 2025-04-09T11:54:35Z

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

lightning/src/ln/peer_handler.rs

martinsaposnic

looks good, no code / behavior changes, just formatting. left a minor question

lightning/src/ln/peer_handler.rs

wpaulino · 2025-04-09T16:46:38Z

lightning/src/ln/peer_handler.rs

-					let logger = WithContext::from(&self.logger, peer.their_node_id.map(|p| p.0), None, None);
+					let logger = WithContext::from(
+						&self.logger,
+						peer.their_node_id.map(|p| p.0),


Extract into a var of to save some lines, there are others like this in the file

lightning/src/ln/peer_handler.rs

wpaulino · 2025-04-09T16:52:52Z

lightning/src/ln/peer_handler.rs

+						|| peer.awaiting_pong_timer_tick_intervals as u64
+							> MAX_BUFFER_DRAIN_TICK_INTERVALS_PER_PEER as u64
+								* peers_lock.len() as u64


Extract into a var

This will be multi-line anyways?

Even if it doesn't cut down on lines, rustfmt's breaking up of the pieces here makes it rather unreadable (as the ()s are no longer connected to which indentation things are at).

lightning/src/ln/peer_handler.rs

wpaulino · 2025-04-09T16:56:45Z

lightning/src/ln/peer_handler.rs

+				peers[handler & 1]
+					.message_handler
+					.chan_handler
+					.conn_tracker
+					.fail_connections
+					.store(true, Ordering::Release);


There are many like this throughout the rest of the file that would not blow up as much if we had some intermediate vars

I'm not convinced it's that bad tbh?

Tests that end up spanning three pages of terminal get pretty hard to skim, especially this one which used to be one page :/. Even if individual changes aren't that bad, when there's four changes in a row that all similarly make things super vertical IMO it needs fixing.

tnull

Addressed most pending feedback.

lightning/src/ln/peer_handler.rs

tnull · 2025-04-10T08:29:33Z

lightning/src/ln/peer_handler.rs

-					let logger = WithContext::from(&self.logger, peer.their_node_id.map(|p| p.0), None, None);
+					let logger = WithContext::from(
+						&self.logger,
+						peer.their_node_id.map(|p| p.0),


lightning/src/ln/peer_handler.rs

tnull · 2025-04-10T08:44:14Z

lightning/src/ln/peer_handler.rs

+				peers[handler & 1]
+					.message_handler
+					.chan_handler
+					.conn_tracker
+					.fail_connections
+					.store(true, Ordering::Release);


I'm not convinced it's that bad tbh?

tnull · 2025-04-10T08:45:45Z

lightning/src/ln/peer_handler.rs

+						|| peer.awaiting_pong_timer_tick_intervals as u64
+							> MAX_BUFFER_DRAIN_TICK_INTERVALS_PER_PEER as u64
+								* peers_lock.len() as u64


This will be multi-line anyways?

lightning/src/ln/peer_handler.rs

codecov · 2025-04-10T08:58:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.34%. Comparing base (46cb5ff) to head (6403b34).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3725      +/-   ##
==========================================
+ Coverage   89.11%   89.34%   +0.23%     
==========================================
  Files         156      156              
  Lines      123435   126684    +3249     
  Branches   123435   126684    +3249     
==========================================
+ Hits       109995   113191    +3196     
- Misses      10758    10828      +70     
+ Partials     2682     2665      -17

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

TheBlueMatt

Highlighted a few patterns that could be used to clean up a number of places across the diff.

TheBlueMatt · 2025-04-10T20:56:38Z

lightning/src/ln/peer_handler.rs

+		None
+	}
+	fn handle_reply_channel_range(
+		&self, _their_node_id: PublicKey, _msg: msgs::ReplyChannelRange,


All the dummy impls can probably get their parameters all replaced with just _, which would make them more dense and no less readable.

no less readable

Hmm, I tend to disagree on this one. Even if they are not used, its adding context what is not being used.

I mean sometimes, sure, but when we're implementing a trait with 20 methods where all of the implementations are just "return nothing", I think we probably don't care too much :p

lightning/src/ln/peer_handler.rs

TheBlueMatt · 2025-04-10T21:01:16Z

lightning/src/ln/peer_handler.rs

+					let next_step = peer.channel_encryptor.get_noise_step();
+					match next_step {
+						NextNoiseStep::ActOne => {
+							let act_two = try_potential_handleerror!(


There's a lot of try_potential_handleerror calls that would be more readable with an intermediate variable.

Hmm, I personally find that these macro calls are not exactly getting more readable if the thing part of it isn't inlined. Verticality or not, but I personally would prefer to leave them as is. Not sure how strongly feel about them?

ISTM that moving the actual thing into an intermediate variable makes much clearer to the reader what the macro is doing. The macro is really just a version of try with some extra logging (and which may continue rather than returning in some errors). Thus, first building the Result and then passing it to the macro makes clearer what's going on, vs passing the expression itself to the macro (and it being less clear what the macro does with the expression). While we're at it, we should probably clean up the macro name, something simpler is probably justified given its really just kinda a try variant, we can call it something simpler like try_peererr or try_msg_err or...

lightning/src/ln/peer_handler.rs

TheBlueMatt · 2025-04-17T14:41:34Z

lightning/src/ln/peer_handler.rs

-						MessageSendEvent::SendStfu { ref node_id, ref msg} => {
-							let logger = WithContext::from(&self.logger, Some(*node_id), Some(msg.channel_id), None);
+						MessageSendEvent::SendStfu { ref node_id, ref msg } => {
+							let logger = WithContext::from(


Not like it has to happen in this PR but this really makes clearer that we need some kind of logger_from_msg utility maybe in wire.rs.

TheBlueMatt · 2025-04-17T14:41:52Z

lightning/src/ln/peer_handler.rs

-							match self.message_handler.route_handler.handle_channel_announcement(None, &msg) {
-								Ok(_) | Err(LightningError { action: msgs::ErrorAction::IgnoreDuplicateGossip, .. }) => {
+							match self
+								.message_handler


Were you gonna clean up down here as well?

TheBlueMatt · 2025-04-17T14:43:32Z

lightning/src/ln/peer_handler.rs

+						|| peer.awaiting_pong_timer_tick_intervals as u64
+							> MAX_BUFFER_DRAIN_TICK_INTERVALS_PER_PEER as u64
+								* peers_lock.len() as u64


Even if it doesn't cut down on lines, rustfmt's breaking up of the pieces here makes it rather unreadable (as the ()s are no longer connected to which indentation things are at).

TheBlueMatt · 2025-04-17T14:45:57Z

lightning/src/ln/peer_handler.rs

+				peers[handler & 1]
+					.message_handler
+					.chan_handler
+					.conn_tracker
+					.fail_connections
+					.store(true, Ordering::Release);


Tests that end up spanning three pages of terminal get pretty hard to skim, especially this one which used to be one page :/. Even if individual changes aren't that bad, when there's four changes in a row that all similarly make things super vertical IMO it needs fixing.

rustfmt: Prepare lightning/src/ln/peer_handler.rs

935f84b

ldk-reviews-bot requested a review from wpaulino April 9, 2025 12:04

martinsaposnic reviewed Apr 9, 2025

View reviewed changes

lightning/src/ln/peer_handler.rs Outdated Show resolved Hide resolved

martinsaposnic approved these changes Apr 9, 2025

View reviewed changes

wpaulino reviewed Apr 9, 2025

View reviewed changes

tnull added 8 commits April 10, 2025 09:55

f Skip some skips

5043ba0

f Drop empty line

6ae6c8f

f Pull filter_fn out

3051592

f Pull node_id into a variable

fb72331

f Reduce indentation in do_read_event

f30a96a

f Pull out their_node_id

380d47a

f Pull more node_ids into vars

044ff88

f Pull out peer

e1ae0da

tnull commented Apr 10, 2025

View reviewed changes

tnull force-pushed the 2025-04-rustfmt-peer-handler branch from 5e7f142 to 6403b34 Compare April 10, 2025 08:48

TheBlueMatt reviewed Apr 10, 2025

View reviewed changes

tnull added 4 commits April 17, 2025 12:45

f Pull handlers out in variables

326e122

f Pull warning message parts out to variables

edf8427

rustfmt: Run on lightning/src/ln/peer_handler.rs

a9c828f

rustfmt: Drop lightning/src/ln/peer_handler.rs from exclusion list

365963c

tnull force-pushed the 2025-04-rustfmt-peer-handler branch from 6403b34 to 365963c Compare April 17, 2025 10:53

TheBlueMatt reviewed Apr 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`rustfmt`: Run on `peer_handler.rs` #3725

`rustfmt`: Run on `peer_handler.rs` #3725

tnull commented Apr 9, 2025

ldk-reviews-bot commented Apr 9, 2025 •

edited

Loading

martinsaposnic left a comment

wpaulino Apr 9, 2025

tnull Apr 10, 2025

wpaulino Apr 9, 2025

tnull Apr 10, 2025

TheBlueMatt Apr 17, 2025

wpaulino Apr 9, 2025

tnull Apr 10, 2025

TheBlueMatt Apr 17, 2025

tnull left a comment

tnull Apr 10, 2025

tnull Apr 10, 2025

tnull Apr 10, 2025

codecov bot commented Apr 10, 2025

TheBlueMatt left a comment

TheBlueMatt Apr 10, 2025

tnull Apr 17, 2025

TheBlueMatt Apr 17, 2025

TheBlueMatt Apr 10, 2025

tnull Apr 17, 2025

TheBlueMatt Apr 17, 2025

TheBlueMatt Apr 17, 2025

TheBlueMatt Apr 17, 2025

TheBlueMatt Apr 17, 2025

TheBlueMatt Apr 17, 2025

rustfmt: Run on peer_handler.rs #3725

Are you sure you want to change the base?

rustfmt: Run on peer_handler.rs #3725

Conversation

tnull commented Apr 9, 2025

ldk-reviews-bot commented Apr 9, 2025 • edited Loading

martinsaposnic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 10, 2025

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`rustfmt`: Run on `peer_handler.rs` #3725

`rustfmt`: Run on `peer_handler.rs` #3725

ldk-reviews-bot commented Apr 9, 2025 •

edited

Loading