From: Gursharan Singh <3442979+G8XSU@users.noreply.github.com>
Date: Fri, 5 Apr 2024 10:26:51 +0000 (+0200)
Subject: Merge pull request #2967 from tnull/2024-03-refactor-drop-handle-message
X-Git-Tag: v0.0.123-beta~17
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=3eb61f7e9b55531626936cf84c57cd7530a878a4;hp=-c;p=rust-lightning

Merge pull request #2967 from tnull/2024-03-refactor-drop-handle-message

Split `PeerManager::handle_message` to avoid explicit `mem::drop`
---

3eb61f7e9b55531626936cf84c57cd7530a878a4
diff --combined lightning/src/ln/peer_handler.rs
index cfa32a00,0188130d..188023dc
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@@ -116,9 -116,7 +116,9 @@@ impl RoutingMessageHandler for Ignoring
  	fn handle_query_short_channel_ids(&self, _their_node_id: &PublicKey, _msg: msgs::QueryShortChannelIds) -> Result<(), LightningError> { Ok(()) }
  	fn provided_node_features(&self) -> NodeFeatures { NodeFeatures::empty() }
  	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 -		InitFeatures::empty()
 +		let mut features = InitFeatures::empty();
 +		features.set_gossip_queries_optional();
 +		features
  	}
  	fn processing_queue_high(&self) -> bool { false }
  }
@@@ -1553,7 -1551,6 +1553,7 @@@ impl<Descriptor: SocketDescriptor, CM: 
  												}
  												(msgs::DecodeError::BadLengthDescriptor, _) => return Err(PeerHandleError { }),
  												(msgs::DecodeError::Io(_), _) => return Err(PeerHandleError { }),
 +												(msgs::DecodeError::DangerousValue, _) => return Err(PeerHandleError { }),
  											}
  										}
  									};
@@@ -1591,15 -1588,37 +1591,37 @@@
  	}
  
  	/// Process an incoming message and return a decision (ok, lightning error, peer handling error) regarding the next action with the peer
+ 	///
  	/// Returns the message back if it needs to be broadcasted to all other peers.
  	fn handle_message(
  		&self,
  		peer_mutex: &Mutex<Peer>,
- 		mut peer_lock: MutexGuard<Peer>,
- 		message: wire::Message<<<CMH as core::ops::Deref>::Target as wire::CustomMessageReader>::CustomMessage>
- 	) -> Result<Option<wire::Message<<<CMH as core::ops::Deref>::Target as wire::CustomMessageReader>::CustomMessage>>, MessageHandlingError> {
+ 		peer_lock: MutexGuard<Peer>,
+ 		message: wire::Message<<<CMH as Deref>::Target as wire::CustomMessageReader>::CustomMessage>
+ 	) -> Result<Option<wire::Message<<<CMH as Deref>::Target as wire::CustomMessageReader>::CustomMessage>>, MessageHandlingError> {
  		let their_node_id = peer_lock.their_node_id.clone().expect("We know the peer's public key by the time we receive messages").0;
  		let logger = WithContext::from(&self.logger, Some(their_node_id), None);
+ 
+ 		let message = match self.do_handle_message_holding_peer_lock(peer_lock, message, &their_node_id, &logger)? {
+ 			Some(processed_message) => processed_message,
+ 			None => return Ok(None),
+ 		};
+ 
+ 		self.do_handle_message_without_peer_lock(peer_mutex, message, &their_node_id, &logger)
+ 	}
+ 
+ 	// Conducts all message processing that requires us to hold the `peer_lock`.
+ 	//
+ 	// Returns `None` if the message was fully processed and otherwise returns the message back to
+ 	// allow it to be subsequently processed by `do_handle_message_without_peer_lock`.
+ 	fn do_handle_message_holding_peer_lock<'a>(
+ 		&self,
+ 		mut peer_lock: MutexGuard<Peer>,
+ 		message: wire::Message<<<CMH as Deref>::Target as wire::CustomMessageReader>::CustomMessage>,
+ 		their_node_id: &PublicKey,
+ 		logger: &WithContext<'a, L>
+ 	) -> Result<Option<wire::Message<<<CMH as Deref>::Target as wire::CustomMessageReader>::CustomMessage>>, MessageHandlingError>
+ 	{
  		peer_lock.received_message_since_timer_tick = true;
  
  		// Need an Init as first message
@@@ -1680,8 -1699,20 +1702,20 @@@
  			peer_lock.received_channel_announce_since_backlogged = true;
  		}
  
- 		mem::drop(peer_lock);
+ 		Ok(Some(message))
+ 	}
  
+ 	// Conducts all message processing that doesn't require us to hold the `peer_lock`.
+ 	//
+ 	// Returns the message back if it needs to be broadcasted to all other peers.
+ 	fn do_handle_message_without_peer_lock<'a>(
+ 		&self,
+ 		peer_mutex: &Mutex<Peer>,
+ 		message: wire::Message<<<CMH as Deref>::Target as wire::CustomMessageReader>::CustomMessage>,
+ 		their_node_id: &PublicKey,
+ 		logger: &WithContext<'a, L>
+ 	) -> Result<Option<wire::Message<<<CMH as Deref>::Target as wire::CustomMessageReader>::CustomMessage>>, MessageHandlingError>
+ 	{
  		if is_gossip_msg(message.type_id()) {
  			log_gossip!(logger, "Received message {:?} from {}", message, log_pubkey!(their_node_id));
  		} else {
@@@ -1883,7 -1914,7 +1917,7 @@@
  		Ok(should_forward)
  	}
  
- 	fn forward_broadcast_msg(&self, peers: &HashMap<Descriptor, Mutex<Peer>>, msg: &wire::Message<<<CMH as core::ops::Deref>::Target as wire::CustomMessageReader>::CustomMessage>, except_node: Option<&PublicKey>) {
+ 	fn forward_broadcast_msg(&self, peers: &HashMap<Descriptor, Mutex<Peer>>, msg: &wire::Message<<<CMH as Deref>::Target as wire::CustomMessageReader>::CustomMessage>, except_node: Option<&PublicKey>) {
  		match msg {
  			wire::Message::ChannelAnnouncement(ref msg) => {
  				log_gossip!(self.logger, "Sending message to all peers except {:?} or the announced channel's counterparties: {:?}", except_node, msg);
@@@ -2275,7 -2306,7 +2309,7 @@@
  									// We do not have the peers write lock, so we just store that we're
  									// about to disconnect the peer and do it after we finish
  									// processing most messages.
- 									let msg = msg.map(|msg| wire::Message::<<<CMH as core::ops::Deref>::Target as wire::CustomMessageReader>::CustomMessage>::Error(msg));
+ 									let msg = msg.map(|msg| wire::Message::<<<CMH as Deref>::Target as wire::CustomMessageReader>::CustomMessage>::Error(msg));
  									peers_to_disconnect.insert(node_id, msg);
  								},
  								msgs::ErrorAction::DisconnectPeerWithWarning { msg } => {