From: Wilmer Paulino <wilmer@wilmerpaulino.com>
Date: Thu, 18 May 2023 19:12:15 +0000 (-0700)
Subject: Add new DisconnectPeerWithWarning variant to ErrorAction
X-Git-Tag: v0.0.116-alpha1~27^2~1
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=5ba2f8091f06f696d62d94c70a339f14300dbbda;p=rust-lightning

Add new DisconnectPeerWithWarning variant to ErrorAction
---

diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs
index 76ed56635..d32eb3e47 100644
--- a/lightning/src/ln/msgs.rs
+++ b/lightning/src/ln/msgs.rs
@@ -1137,6 +1137,11 @@ pub enum ErrorAction {
 		/// An error message which we should make an effort to send before we disconnect.
 		msg: Option<ErrorMessage>
 	},
+	/// The peer did something incorrect. Tell them without closing any channels and disconnect them.
+	DisconnectPeerWithWarning {
+		/// A warning message which we should make an effort to send before we disconnect.
+		msg: WarningMessage,
+	},
 	/// The peer did something harmless that we weren't able to process, just log and ignore
 	// New code should *not* use this. New code must use IgnoreAndLog, below!
 	IgnoreError,
diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index 000e754c5..a7f35a59c 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -1230,8 +1230,21 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
 								Ok(x) => x,
 								Err(e) => {
 									match e.action {
-										msgs::ErrorAction::DisconnectPeer { msg: _ } => {
-											//TODO: Try to push msg
+										msgs::ErrorAction::DisconnectPeer { .. } => {
+											// We may have an `ErrorMessage` to send to the peer,
+											// but writing to the socket while reading can lead to
+											// re-entrant code and possibly unexpected behavior. The
+											// message send is optimistic anyway, and in this case
+											// we immediately disconnect the peer.
+											log_debug!(self.logger, "Error handling message{}; disconnecting peer with: {}", OptionalFromDebugger(&peer_node_id), e.err);
+											return Err(PeerHandleError { });
+										},
+										msgs::ErrorAction::DisconnectPeerWithWarning { .. } => {
+											// We have a `WarningMessage` to send to the peer, but
+											// writing to the socket while reading can lead to
+											// re-entrant code and possibly unexpected behavior. The
+											// message send is optimistic anyway, and in this case
+											// we immediately disconnect the peer.
 											log_debug!(self.logger, "Error handling message{}; disconnecting peer with: {}", OptionalFromDebugger(&peer_node_id), e.err);
 											return Err(PeerHandleError { });
 										},
@@ -1362,7 +1375,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
 										Ok(x) => x,
 										Err(e) => {
 											match e {
-												// Note that to avoid recursion we never call
+												// Note that to avoid re-entrancy we never call
 												// `do_attempt_write_data` from here, causing
 												// the messages enqueued here to not actually
 												// be sent before the peer is disconnected.
@@ -2064,32 +2077,48 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
 									log_pubkey!(node_id), msg.contents.short_channel_id);
 							self.enqueue_message(&mut *get_peer_for_forwarding!(node_id), msg);
 						},
-						MessageSendEvent::HandleError { ref node_id, ref action } => {
-							match *action {
-								msgs::ErrorAction::DisconnectPeer { ref msg } => {
+						MessageSendEvent::HandleError { node_id, action } => {
+							match action {
+								msgs::ErrorAction::DisconnectPeer { msg } => {
+									if let Some(msg) = msg.as_ref() {
+										log_trace!(self.logger, "Handling DisconnectPeer HandleError event in peer_handler for node {} with message {}",
+											log_pubkey!(node_id), msg.data);
+									} else {
+										log_trace!(self.logger, "Handling DisconnectPeer HandleError event in peer_handler for node {}",
+											log_pubkey!(node_id));
+									}
+									// We do not have the peers write lock, so we just store that we're
+									// about to disconenct 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));
+									peers_to_disconnect.insert(node_id, msg);
+								},
+								msgs::ErrorAction::DisconnectPeerWithWarning { msg } => {
+									log_trace!(self.logger, "Handling DisconnectPeer HandleError event in peer_handler for node {} with message {}",
+										log_pubkey!(node_id), msg.data);
 									// We do not have the peers write lock, so we just store that we're
 									// about to disconenct the peer and do it after we finish
 									// processing most messages.
-									peers_to_disconnect.insert(*node_id, msg.clone());
+									peers_to_disconnect.insert(node_id, Some(wire::Message::Warning(msg)));
 								},
 								msgs::ErrorAction::IgnoreAndLog(level) => {
 									log_given_level!(self.logger, level, "Received a HandleError event to be ignored for node {}", log_pubkey!(node_id));
 								},
 								msgs::ErrorAction::IgnoreDuplicateGossip => {},
 								msgs::ErrorAction::IgnoreError => {
-									log_debug!(self.logger, "Received a HandleError event to be ignored for node {}", log_pubkey!(node_id));
-								},
+										log_debug!(self.logger, "Received a HandleError event to be ignored for node {}", log_pubkey!(node_id));
+									},
 								msgs::ErrorAction::SendErrorMessage { ref msg } => {
 									log_trace!(self.logger, "Handling SendErrorMessage HandleError event in peer_handler for node {} with message {}",
 											log_pubkey!(node_id),
 											msg.data);
-									self.enqueue_message(&mut *get_peer_for_forwarding!(node_id), msg);
+									self.enqueue_message(&mut *get_peer_for_forwarding!(&node_id), msg);
 								},
 								msgs::ErrorAction::SendWarningMessage { ref msg, ref log_level } => {
 									log_given_level!(self.logger, *log_level, "Handling SendWarningMessage HandleError event in peer_handler for node {} with message {}",
 											log_pubkey!(node_id),
 											msg.data);
-									self.enqueue_message(&mut *get_peer_for_forwarding!(node_id), msg);
+									self.enqueue_message(&mut *get_peer_for_forwarding!(&node_id), msg);
 								},
 							}
 						},
@@ -2139,9 +2168,6 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
 						if let Some(peer_mutex) = peers.remove(&descriptor) {
 							let mut peer = peer_mutex.lock().unwrap();
 							if let Some(msg) = msg {
-								log_trace!(self.logger, "Handling DisconnectPeer HandleError event in peer_handler for node {} with message {}",
-										log_pubkey!(node_id),
-										msg.data);
 								self.enqueue_message(&mut *peer, &msg);
 								// This isn't guaranteed to work, but if there is enough free
 								// room in the send buffer, put the error message there...