feat: don't report inbound stream upgrade errors to handler

When an inbound stream upgrade fails, there isn't a whole lot we can do about that in the handler. In fact, for several errors, we wouldn't even know which specific handler to target, for example, `NegotiationFailed`. Similiarly, in case of an IO error during the upgrade, we don't know which handler the stream was eventually meant to be for. Pull-Request: #3605.
libp2p · May 8, 2023 · 53e5370 · 53e5370
1 parent 2130923
commit 53e5370
Show file tree

Hide file tree

Showing 21 changed files with 89 additions and 552 deletions.
diff --git a/misc/metrics/src/relay.rs b/misc/metrics/src/relay.rs
@@ -54,7 +54,6 @@ enum EventType {
     ReservationReqDenied,
     ReservationReqDenyFailed,
     ReservationTimedOut,
-    CircuitReqReceiveFailed,
     CircuitReqDenied,
     CircuitReqDenyFailed,
     CircuitReqOutboundConnectFailed,
@@ -75,9 +74,6 @@ impl From<&libp2p_relay::Event> for EventType {
                 EventType::ReservationReqDenyFailed
             }
             libp2p_relay::Event::ReservationTimedOut { .. } => EventType::ReservationTimedOut,
-            libp2p_relay::Event::CircuitReqReceiveFailed { .. } => {
-                EventType::CircuitReqReceiveFailed
-            }
             libp2p_relay::Event::CircuitReqDenied { .. } => EventType::CircuitReqDenied,
             libp2p_relay::Event::CircuitReqOutboundConnectFailed { .. } => {
                 EventType::CircuitReqOutboundConnectFailed

diff --git a/protocols/dcutr/src/handler/relayed.rs b/protocols/dcutr/src/handler/relayed.rs
@@ -212,45 +212,12 @@ impl Handler {
             <Self as ConnectionHandler>::InboundProtocol,
         >,
     ) {
-        match error {
-            ConnectionHandlerUpgrErr::Timeout => {
-                self.queued_events.push_back(ConnectionHandlerEvent::Custom(
-                    Event::InboundNegotiationFailed {
-                        error: ConnectionHandlerUpgrErr::Timeout,
-                    },
-                ));
-            }
-            ConnectionHandlerUpgrErr::Timer => {
-                self.queued_events.push_back(ConnectionHandlerEvent::Custom(
-                    Event::InboundNegotiationFailed {
-                        error: ConnectionHandlerUpgrErr::Timer,
-                    },
-                ));
-            }
-            ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(NegotiationError::Failed)) => {
-                // The remote merely doesn't support the DCUtR protocol.
-                // This is no reason to close the connection, which may
-                // successfully communicate with other protocols already.
-                self.keep_alive = KeepAlive::No;
-                self.queued_events.push_back(ConnectionHandlerEvent::Custom(
-                    Event::InboundNegotiationFailed {
-                        error: ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Select(
-                            NegotiationError::Failed,
-                        )),
-                    },
-                ));
-            }
-            _ => {
-                // Anything else is considered a fatal error or misbehaviour of
-                // the remote peer and results in closing the connection.
-                self.pending_error = Some(error.map_upgrade_err(|e| {
-                    e.map_err(|e| match e {
-                        Either::Left(e) => Either::Left(e),
-                        Either::Right(v) => void::unreachable(v),
-                    })
-                }));
-            }
-        }
+        self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(UpgradeError::Apply(
+            match error {
+                Either::Left(e) => Either::Left(e),
+                Either::Right(v) => void::unreachable(v),
+            },
+        )));
     }
 
     fn on_dial_upgrade_error(

diff --git a/protocols/gossipsub/src/handler.rs b/protocols/gossipsub/src/handler.rs
@@ -526,7 +526,7 @@ impl ConnectionHandler for Handler {
                         handler.on_fully_negotiated_outbound(fully_negotiated_outbound)
                     }
                     ConnectionEvent::DialUpgradeError(DialUpgradeError {
-                        error: ConnectionHandlerUpgrErr::Timeout | ConnectionHandlerUpgrErr::Timer,
+                        error: ConnectionHandlerUpgrErr::Timeout,
                         ..
                     }) => {
                         log::debug!("Dial upgrade error: Protocol negotiation timeout");

diff --git a/protocols/perf/src/client/handler.rs b/protocols/perf/src/client/handler.rs
@@ -150,14 +150,7 @@ impl ConnectionHandler for Handler {
                     }));
             }
             ConnectionEvent::ListenUpgradeError(ListenUpgradeError { info: (), error }) => {
-                match error {
-                    ConnectionHandlerUpgrErr::Timeout => {}
-                    ConnectionHandlerUpgrErr::Timer => {}
-                    ConnectionHandlerUpgrErr::Upgrade(error) => match error {
-                        libp2p_core::UpgradeError::Select(_) => {}
-                        libp2p_core::UpgradeError::Apply(v) => void::unreachable(v),
-                    },
-                }
+                void::unreachable(error)
             }
         }
     }

diff --git a/protocols/perf/src/server/handler.rs b/protocols/perf/src/server/handler.rs
@@ -30,8 +30,7 @@ use libp2p_swarm::{
         ConnectionEvent, DialUpgradeError, FullyNegotiatedInbound, FullyNegotiatedOutbound,
         ListenUpgradeError,
     },
-    ConnectionHandler, ConnectionHandlerEvent, ConnectionHandlerUpgrErr, KeepAlive, StreamProtocol,
-    SubstreamProtocol,
+    ConnectionHandler, ConnectionHandlerEvent, KeepAlive, StreamProtocol, SubstreamProtocol,
 };
 use log::error;
 use void::Void;
@@ -106,14 +105,7 @@ impl ConnectionHandler for Handler {
             }
             ConnectionEvent::AddressChange(_) => {}
             ConnectionEvent::ListenUpgradeError(ListenUpgradeError { info: (), error }) => {
-                match error {
-                    ConnectionHandlerUpgrErr::Timeout => {}
-                    ConnectionHandlerUpgrErr::Timer => {}
-                    ConnectionHandlerUpgrErr::Upgrade(error) => match error {
-                        libp2p_core::UpgradeError::Select(_) => {}
-                        libp2p_core::UpgradeError::Apply(v) => void::unreachable(v),
-                    },
-                }
+                void::unreachable(error)
             }
         }
     }

diff --git a/protocols/relay/CHANGELOG.md b/protocols/relay/CHANGELOG.md
@@ -6,6 +6,11 @@
 - Hide internals of `Connection` and expose only `AsyncRead` and `AsyncWrite`.
   See [PR 3829].
 
+- Remove `Event::CircuitReqReceiveFailed` and `Event::InboundCircuitReqFailed` variants.
+  These variants are no longer constructed.
+  See [PR 3605].
+
+[PR 3605]: https://github.com/libp2p/rust-libp2p/pull/3605
 [PR 3715]: https://github.com/libp2p/rust-libp2p/pull/3715
 [PR 3829]: https://github.com/libp2p/rust-libp2p/pull/3829
 

diff --git a/protocols/relay/src/behaviour.rs b/protocols/relay/src/behaviour.rs
@@ -150,10 +150,6 @@ pub enum Event {
     },
     /// An inbound reservation has timed out.
     ReservationTimedOut { src_peer_id: PeerId },
-    CircuitReqReceiveFailed {
-        src_peer_id: PeerId,
-        error: ConnectionHandlerUpgrErr<void::Void>,
-    },
     /// An inbound circuit request has been denied.
     CircuitReqDenied {
         src_peer_id: PeerId,
@@ -537,15 +533,6 @@ impl NetworkBehaviour for Behaviour {
                 };
                 self.queued_actions.push_back(action.into());
             }
-            handler::Event::CircuitReqReceiveFailed { error } => {
-                self.queued_actions.push_back(
-                    ToSwarm::GenerateEvent(Event::CircuitReqReceiveFailed {
-                        src_peer_id: event_source,
-                        error,
-                    })
-                    .into(),
-                );
-            }
             handler::Event::CircuitReqDenied {
                 circuit_id,
                 dst_peer_id,

diff --git a/protocols/relay/src/behaviour/handler.rs b/protocols/relay/src/behaviour/handler.rs
@@ -163,10 +163,6 @@ pub enum Event {
         inbound_circuit_req: inbound_hop::CircuitReq,
         endpoint: ConnectedPoint,
     },
-    /// Receiving an inbound circuit request failed.
-    CircuitReqReceiveFailed {
-        error: ConnectionHandlerUpgrErr<void::Void>,
-    },
     /// An inbound circuit request has been denied.
     CircuitReqDenied {
         circuit_id: Option<CircuitId>,
@@ -252,10 +248,6 @@ impl fmt::Debug for Event {
                 .debug_struct("Event::CircuitReqReceived")
                 .field("endpoint", endpoint)
                 .finish(),
-            Event::CircuitReqReceiveFailed { error } => f
-                .debug_struct("Event::CircuitReqReceiveFailed")
-                .field("error", error)
-                .finish(),
             Event::CircuitReqDenied {
                 circuit_id,
                 dst_peer_id,
@@ -471,41 +463,16 @@ impl Handler {
 
     fn on_listen_upgrade_error(
         &mut self,
-        ListenUpgradeError { error, .. }: ListenUpgradeError<
+        ListenUpgradeError {
+            error: inbound_hop::UpgradeError::Fatal(error),
+            ..
+        }: ListenUpgradeError<
             <Self as ConnectionHandler>::InboundOpenInfo,
             <Self as ConnectionHandler>::InboundProtocol,
         >,
     ) {
-        let non_fatal_error = match error {
-            ConnectionHandlerUpgrErr::Timeout => ConnectionHandlerUpgrErr::Timeout,
-            ConnectionHandlerUpgrErr::Timer => ConnectionHandlerUpgrErr::Timer,
-            ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
-                upgrade::NegotiationError::Failed,
-            )) => ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
-                upgrade::NegotiationError::Failed,
-            )),
-            ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
-                upgrade::NegotiationError::ProtocolError(e),
-            )) => {
-                self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(
-                    upgrade::UpgradeError::Select(upgrade::NegotiationError::ProtocolError(e)),
-                ));
-                return;
-            }
-            ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Apply(
-                inbound_hop::UpgradeError::Fatal(error),
-            )) => {
-                self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(
-                    upgrade::UpgradeError::Apply(Either::Left(error)),
-                ));
-                return;
-            }
-        };
-
-        self.queued_events.push_back(ConnectionHandlerEvent::Custom(
-            Event::CircuitReqReceiveFailed {
-                error: non_fatal_error,
-            },
+        self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(
+            upgrade::UpgradeError::Apply(Either::Left(error)),
         ));
     }
 
@@ -524,10 +491,6 @@ impl Handler {
                 ConnectionHandlerUpgrErr::Timeout,
                 proto::Status::CONNECTION_FAILED,
             ),
-            ConnectionHandlerUpgrErr::Timer => (
-                ConnectionHandlerUpgrErr::Timer,
-                proto::Status::CONNECTION_FAILED,
-            ),
             ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
                 upgrade::NegotiationError::Failed,
             )) => {

diff --git a/protocols/relay/src/priv_client.rs b/protocols/relay/src/priv_client.rs
@@ -79,10 +79,6 @@ pub enum Event {
         src_peer_id: PeerId,
         limit: Option<protocol::Limit>,
     },
-    InboundCircuitReqFailed {
-        relay_peer_id: PeerId,
-        error: ConnectionHandlerUpgrErr<void::Void>,
-    },
     /// An inbound circuit request has been denied.
     InboundCircuitReqDenied { src_peer_id: PeerId },
     /// Denying an inbound circuit request failed.
@@ -282,10 +278,6 @@ impl NetworkBehaviour for Behaviour {
             handler::Event::InboundCircuitEstablished { src_peer_id, limit } => {
                 Event::InboundCircuitEstablished { src_peer_id, limit }
             }
-            handler::Event::InboundCircuitReqFailed { error } => Event::InboundCircuitReqFailed {
-                relay_peer_id: event_source,
-                error,
-            },
             handler::Event::InboundCircuitReqDenied { src_peer_id } => {
                 Event::InboundCircuitReqDenied { src_peer_id }
             }

diff --git a/protocols/relay/src/priv_client/handler.rs b/protocols/relay/src/priv_client/handler.rs
@@ -97,10 +97,6 @@ pub enum Event {
         src_peer_id: PeerId,
         limit: Option<protocol::Limit>,
     },
-    /// An inbound circuit request has failed.
-    InboundCircuitReqFailed {
-        error: ConnectionHandlerUpgrErr<void::Void>,
-    },
     /// An inbound circuit request has been denied.
     InboundCircuitReqDenied { src_peer_id: PeerId },
     /// Denying an inbound circuit request failed.
@@ -295,41 +291,16 @@ impl Handler {
 
     fn on_listen_upgrade_error(
         &mut self,
-        ListenUpgradeError { error, .. }: ListenUpgradeError<
+        ListenUpgradeError {
+            error: inbound_stop::UpgradeError::Fatal(error),
+            ..
+        }: ListenUpgradeError<
             <Self as ConnectionHandler>::InboundOpenInfo,
             <Self as ConnectionHandler>::InboundProtocol,
         >,
     ) {
-        let non_fatal_error = match error {
-            ConnectionHandlerUpgrErr::Timeout => ConnectionHandlerUpgrErr::Timeout,
-            ConnectionHandlerUpgrErr::Timer => ConnectionHandlerUpgrErr::Timer,
-            ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
-                upgrade::NegotiationError::Failed,
-            )) => ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
-                upgrade::NegotiationError::Failed,
-            )),
-            ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
-                upgrade::NegotiationError::ProtocolError(e),
-            )) => {
-                self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(
-                    upgrade::UpgradeError::Select(upgrade::NegotiationError::ProtocolError(e)),
-                ));
-                return;
-            }
-            ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Apply(
-                inbound_stop::UpgradeError::Fatal(error),
-            )) => {
-                self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(
-                    upgrade::UpgradeError::Apply(Either::Left(error)),
-                ));
-                return;
-            }
-        };
-
-        self.queued_events.push_back(ConnectionHandlerEvent::Custom(
-            Event::InboundCircuitReqFailed {
-                error: non_fatal_error,
-            },
+        self.pending_error = Some(ConnectionHandlerUpgrErr::Upgrade(
+            upgrade::UpgradeError::Apply(Either::Left(error)),
         ));
     }
 
@@ -347,7 +318,6 @@ impl Handler {
             OutboundOpenInfo::Reserve { mut to_listener } => {
                 let non_fatal_error = match error {
                     ConnectionHandlerUpgrErr::Timeout => ConnectionHandlerUpgrErr::Timeout,
-                    ConnectionHandlerUpgrErr::Timer => ConnectionHandlerUpgrErr::Timer,
                     ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
                         upgrade::NegotiationError::Failed,
                     )) => ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
@@ -410,7 +380,6 @@ impl Handler {
             OutboundOpenInfo::Connect { send_back } => {
                 let non_fatal_error = match error {
                     ConnectionHandlerUpgrErr::Timeout => ConnectionHandlerUpgrErr::Timeout,
-                    ConnectionHandlerUpgrErr::Timer => ConnectionHandlerUpgrErr::Timer,
                     ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(
                         upgrade::NegotiationError::Failed,
                     )) => ConnectionHandlerUpgrErr::Upgrade(upgrade::UpgradeError::Select(

diff --git a/protocols/request-response/CHANGELOG.md b/protocols/request-response/CHANGELOG.md
@@ -4,6 +4,11 @@
   See [PR 3715].
 - Remove deprecated `RequestResponse` prefixed items. See [PR 3702].
 
+- Remove `InboundFailure::UnsupportedProtocols` and `InboundFailure::InboundTimeout`.
+  These variants are no longer constructed.
+  See [PR 3605].
+
+[PR 3605]: https://github.com/libp2p/rust-libp2p/pull/3605
 [PR 3715]: https://github.com/libp2p/rust-libp2p/pull/3715
 [PR 3702]: https://github.com/libp2p/rust-libp2p/pull/3702