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

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

No description provided.

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

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

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

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

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

lightning/src/ln/peer_handler.rs

tnull

Excuse the delay here. Addressed most pending feedback. Let me know if there's something particular left to address.

lightning/src/ln/peer_handler.rs

wpaulino · 2025-04-29T18:03:57Z

Let me know if there's something particular left to address.

The tests could be cleaned up a bit more #3725 (comment).

lightning/src/ln/peer_handler.rs

tnull · 2025-06-11T11:59:24Z

The tests could be cleaned up a bit more #3725 (comment).

Excuse the delay here once more!

Now rebased and addressed this remaining comment by pulling out some more handlers into intermediate variables as a fixup. Let me know if there's something else you'd like to see.

TheBlueMatt

Basically just #3725 (comment) but a few other nits that look trivial to fix.

lightning/src/ln/peer_handler.rs

TheBlueMatt

Feel free to squash, IMO

tnull · 2025-06-12T14:39:23Z

Feel free to squash, IMO

Squashed without further changes.

TheBlueMatt

Ugh, looks like feedback was missed

lightning/src/ln/peer_handler.rs

tnull · 2025-06-13T07:21:57Z

Force-pushed with the following changes:

diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index bcae7690f..73c831884 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -4420,48 +4420,40 @@ mod tests {
 
 		let act_three_with_init_b = fd_b.outbound_data.lock().unwrap().split_off(0);
-		assert!(!peer_a
-			.peers
-			.read()
-			.unwrap()
-			.get(&fd_a)
-			.unwrap()
-			.lock()
-			.unwrap()
-			.handshake_complete());
+		{
+			let peer_a_lock = peer_a.peers.read().unwrap();
+			let handshake_complete =
+				peer_a_lock.get(&fd_a).unwrap().lock().unwrap().handshake_complete();
+			assert!(!handshake_complete);
+		}
+
 		assert_eq!(peer_a.read_event(&mut fd_a, &act_three_with_init_b).unwrap(), false);
 		peer_a.process_events();
-		assert!(peer_a
-			.peers
-			.read()
-			.unwrap()
-			.get(&fd_a)
-			.unwrap()
-			.lock()
-			.unwrap()
-			.handshake_complete());
+
+		{
+			let peer_a_lock = peer_a.peers.read().unwrap();
+			let handshake_complete =
+				peer_a_lock.get(&fd_a).unwrap().lock().unwrap().handshake_complete();
+			assert!(handshake_complete);
+		}
 
 		let init_a = fd_a.outbound_data.lock().unwrap().split_off(0);
 		assert!(!init_a.is_empty());
 
-		assert!(!peer_b
-			.peers
-			.read()
-			.unwrap()
-			.get(&fd_b)
-			.unwrap()
-			.lock()
-			.unwrap()
-			.handshake_complete());
+		{
+			let peer_b_lock = peer_b.peers.read().unwrap();
+			let handshake_complete =
+				peer_b_lock.get(&fd_b).unwrap().lock().unwrap().handshake_complete();
+			assert!(!handshake_complete);
+		}
+
 		assert_eq!(peer_b.read_event(&mut fd_b, &init_a).unwrap(), false);
 		peer_b.process_events();
-		assert!(peer_b
-			.peers
-			.read()
-			.unwrap()
-			.get(&fd_b)
-			.unwrap()
-			.lock()
-			.unwrap()
-			.handshake_complete());
+
+		{
+			let peer_b_lock = peer_b.peers.read().unwrap();
+			let handshake_complete =
+				peer_b_lock.get(&fd_b).unwrap().lock().unwrap().handshake_complete();
+			assert!(handshake_complete);
+		}
 
 		// Make sure we're still connected.
@@ -4565,17 +4557,10 @@ mod tests {
 		}
 
-		assert_eq!(
-			peers[0]
-				.peers
-				.read()
-				.unwrap()
-				.get(&fd_a)
-				.unwrap()
-				.lock()
-				.unwrap()
-				.gossip_broadcast_buffer
-				.len(),
-			OUTBOUND_BUFFER_LIMIT_DROP_GOSSIP
-		);
+		{
+			let peer_a_lock = peers[0].peers.read().unwrap();
+			let buf_len =
+				peer_a_lock.get(&fd_a).unwrap().lock().unwrap().gossip_broadcast_buffer.len();
+			assert_eq!(buf_len, OUTBOUND_BUFFER_LIMIT_DROP_GOSSIP);
+		}
 
 		// Check that if a broadcast message comes in from the channel handler (i.e. it is an
@@ -4583,17 +4568,11 @@ mod tests {
 		cfgs[0].chan_handler.pending_events.lock().unwrap().push(msg_ev);
 		peers[0].process_events();
-		assert_eq!(
-			peers[0]
-				.peers
-				.read()
-				.unwrap()
-				.get(&fd_a)
-				.unwrap()
-				.lock()
-				.unwrap()
-				.gossip_broadcast_buffer
-				.len(),
-			OUTBOUND_BUFFER_LIMIT_DROP_GOSSIP + 1
-		);
+
+		{
+			let peer_a_lock = peers[0].peers.read().unwrap();
+			let buf_len =
+				peer_a_lock.get(&fd_a).unwrap().lock().unwrap().gossip_broadcast_buffer.len();
+			assert_eq!(buf_len, OUTBOUND_BUFFER_LIMIT_DROP_GOSSIP + 1);
+		}
 
 		// Finally, deliver all the messages and make sure we got the right count. Note that there
@@ -4605,14 +4584,11 @@ mod tests {
 
 		drain_queues!();
-		assert!(peers[0]
-			.peers
-			.read()
-			.unwrap()
-			.get(&fd_a)
-			.unwrap()
-			.lock()
-			.unwrap()
-			.gossip_broadcast_buffer
-			.is_empty());
+		{
+			let peer_a_lock = peers[0].peers.read().unwrap();
+			let empty =
+				peer_a_lock.get(&fd_a).unwrap().lock().unwrap().gossip_broadcast_buffer.is_empty();
+			assert!(empty);
+		}
+
 		assert_eq!(
 			cfgs[1].routing_handler.chan_anns_recvd.load(Ordering::Relaxed),

ldk-reviews-bot · 2025-06-16T08:08:35Z

🔔 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

Landing, but will open a quick PR to address the followups.

TheBlueMatt · 2025-06-16T14:20:30Z

lightning/src/ln/peer_handler.rs

+		let their_node_id = peer.their_node_id.map(|p| p.0);
+		let logger = WithContext::from(&self.logger, their_node_id, None, None);
+		// `unwrap` SAFETY: `their_node_id` is guaranteed to be `Some` after the handshake
+		let node_id = peer.their_node_id.unwrap().0;


We map then unwrap the same thing two lines apart. Seems like we can just unwrap.

TheBlueMatt · 2025-06-16T14:22:45Z

lightning/src/ln/peer_handler.rs

+									// Check that the peers map is consistent with the
+									// node_id_to_descriptor map, as this has been broken
+									// before.


This comment is about the debug_assert so should come after the their_node_id wipe.

TheBlueMatt · 2025-06-16T14:33:11Z

lightning/src/ln/peer_handler.rs

-			if let Err(()) = self.message_handler.route_handler.peer_connected(their_node_id, &msg, peer_lock.inbound_connection) {
-				log_debug!(logger, "Route Handler decided we couldn't communicate with peer {}", log_pubkey!(their_node_id));
-				return Err(PeerHandleError { }.into());
+			let connection = peer_lock.inbound_connection;


Ha, inbound may be a more readable name :)

TheBlueMatt · 2025-06-16T14:48:12Z

lightning/src/ln/peer_handler.rs

@@ -2878,18 +3486,20 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
 				debug_assert!(peer.channel_encryptor.is_ready_for_encryption());
 				debug_assert!(peer.their_node_id.is_some());

-				loop { // Used as a `goto` to skip writing a Ping message.
+				loop {
+					// Used as a `goto` to skip writing a Ping message.


This is now ambiguous whether its referring to the loop or the following line.

TheBlueMatt · 2025-06-16T14:51:08Z

lightning/src/ln/peer_handler.rs

+	fn create_network<'a>(
+		peer_count: usize, cfgs: &'a Vec<PeerManagerCfg>,
+	) -> Vec<
+		PeerManager<


Could use TestPeer here.

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

tnull force-pushed the 2025-04-rustfmt-peer-handler branch 2 times, most recently from a96f916 to 56cf495 Compare April 29, 2025 13:42

tnull commented Apr 29, 2025

View reviewed changes

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

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

TheBlueMatt reviewed Apr 30, 2025

View reviewed changes

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

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

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

TheBlueMatt mentioned this pull request May 1, 2025

rustfmt non-test ln module's not-currently-being-edited files #3763

Merged

joostjager added the rustfmt label May 12, 2025

joostjager mentioned this pull request Jun 11, 2025

Convert file-level rustfmt skip to fn level skips #3809

Merged

tnull force-pushed the 2025-04-rustfmt-peer-handler branch from 56cf495 to b7dbd05 Compare June 11, 2025 11:57

TheBlueMatt self-requested a review June 11, 2025 21:05

TheBlueMatt reviewed Jun 11, 2025

View reviewed changes

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

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

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

tnull force-pushed the 2025-04-rustfmt-peer-handler branch from b7dbd05 to 6934b64 Compare June 12, 2025 08:35

tnull requested review from wpaulino and TheBlueMatt June 12, 2025 08:36

TheBlueMatt reviewed Jun 12, 2025

View reviewed changes

tnull force-pushed the 2025-04-rustfmt-peer-handler branch from 6934b64 to 8f3fad6 Compare June 12, 2025 14:39

TheBlueMatt reviewed Jun 12, 2025

View reviewed changes

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

tnull added 2 commits June 13, 2025 09:17

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

e620310

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

8f48f95

tnull force-pushed the 2025-04-rustfmt-peer-handler branch from 8f3fad6 to 8f48f95 Compare June 13, 2025 07:18

wpaulino approved these changes Jun 13, 2025

View reviewed changes

tnull requested a review from TheBlueMatt June 16, 2025 08:08

TheBlueMatt approved these changes Jun 16, 2025

View reviewed changes

TheBlueMatt merged commit ad20383 into lightningdevkit:main Jun 16, 2025
27 of 28 checks passed

rustfmt: Run on peer_handler.rs #3725

rustfmt: Run on peer_handler.rs #3725

Uh oh!

Conversation

tnull commented Apr 9, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

martinsaposnic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 10, 2025

Codecov Report

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wpaulino commented Apr 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull commented Jun 11, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

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

ldk-reviews-bot commented Apr 9, 2025 •

edited

Loading