From 5ba2f8091f06f696d62d94c70a339f14300dbbda Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 18 May 2023 12:12:15 -0700 Subject: [PATCH] Add new DisconnectPeerWithWarning variant to ErrorAction --- lightning/src/ln/msgs.rs | 5 +++ lightning/src/ln/peer_handler.rs | 54 +++++++++++++++++++++++--------- 2 files changed, 45 insertions(+), 14 deletions(-) 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 }, + /// 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 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 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 { - 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::<<::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