*: Rework reporting of invalid and wrong PeerIds (#2441)

Previously, the negotiated PeerId was included in the swarm event and
inject_dial_failure’s arguments while the expected one was absent. This
patch adds the negotiated PeerId to the DialError and includes the expected
one in the notifications.

Co-authored-by: Roland Kuhn <[email protected]>

Author: mxinden

core/CHANGELOG.md

@@ -14,14 +14,17 @@
 - Enable overriding _dial concurrency factor_ per dial via
   `DialOpts::override_dial_concurrency_factor`.
 
-    - Introduces `libp2p_core::DialOpts` mirroring `libp2p_swarm::DialOpts`.
+  - Introduces `libp2p_core::DialOpts` mirroring `libp2p_swarm::DialOpts`.
       Passed as an argument to `Network::dial`.
-    - Removes `Peer::dial` in favor of `Network::dial`.
+  - Removes `Peer::dial` in favor of `Network::dial`.
 
   See [PR 2404].
 
 - Implement `Serialize` and `Deserialize` for `PeerId` (see [PR 2408])
 
+- Report negotiated and expected `PeerId` as well as remote address in
+  `DialError::WrongPeerId` (see [PR 2428]).
+
 - Allow overriding role when dialing. This option is needed for NAT and firewall
   hole punching.
 
@@ -39,6 +42,7 @@
 [PR 2392]: https://github.com/libp2p/rust-libp2p/pull/2392
 [PR 2404]: https://github.com/libp2p/rust-libp2p/pull/2404
 [PR 2408]: https://github.com/libp2p/rust-libp2p/pull/2408
+[PR 2428]: https://github.com/libp2p/rust-libp2p/pull/2428
 [PR 2363]: https://github.com/libp2p/rust-libp2p/pull/2363
 
 # 0.30.1 [2021-11-16]
@@ -127,7 +131,7 @@
 
 # 0.28.3 [2021-04-26]
 
-- Fix build with secp256k1 disabled [PR 2057](https://github.com/libp2p/rust-libp2p/pull/2057].
+- Fix build with secp256k1 disabled [PR 2057](https://github.com/libp2p/rust-libp2p/pull/2057).
 
 # 0.28.2 [2021-04-13]
 

core/src/connection/error.rs

@@ -18,9 +18,9 @@
 // FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
 // DEALINGS IN THE SOFTWARE.
 
-use crate::connection::ConnectionLimit;
 use crate::transport::TransportError;
 use crate::Multiaddr;
+use crate::{connection::ConnectionLimit, ConnectedPoint, PeerId};
 use std::{fmt, io};
 
 /// Errors that can occur in the context of an established `Connection`.
@@ -84,14 +84,33 @@ pub enum PendingConnectionError<TTransErr> {
     Aborted,
 
     /// The peer identity obtained on the connection did not
-    /// match the one that was expected or is otherwise invalid.
-    InvalidPeerId,
+    /// match the one that was expected or is the local one.
+    WrongPeerId {
+        obtained: PeerId,
+        endpoint: ConnectedPoint,
+    },
 
     /// An I/O error occurred on the connection.
     // TODO: Eventually this should also be a custom error?
     IO(io::Error),
 }
 
+impl<T> PendingConnectionError<T> {
+    pub fn map<U>(self, f: impl FnOnce(T) -> U) -> PendingConnectionError<U> {
+        match self {
+            PendingConnectionError::Transport(t) => PendingConnectionError::Transport(f(t)),
+            PendingConnectionError::ConnectionLimit(l) => {
+                PendingConnectionError::ConnectionLimit(l)
+            }
+            PendingConnectionError::Aborted => PendingConnectionError::Aborted,
+            PendingConnectionError::WrongPeerId { obtained, endpoint } => {
+                PendingConnectionError::WrongPeerId { obtained, endpoint }
+            }
+            PendingConnectionError::IO(e) => PendingConnectionError::IO(e),
+        }
+    }
+}
+
 impl<TTransErr> fmt::Display for PendingConnectionError<TTransErr>
 where
     TTransErr: fmt::Display + fmt::Debug,
@@ -110,8 +129,12 @@ where
             PendingConnectionError::ConnectionLimit(l) => {
                 write!(f, "Connection error: Connection limit: {}.", l)
             }
-            PendingConnectionError::InvalidPeerId => {
-                write!(f, "Pending connection: Invalid peer ID.")
+            PendingConnectionError::WrongPeerId { obtained, endpoint } => {
+                write!(
+                    f,
+                    "Pending connection: Unexpected peer ID {} at {:?}.",
+                    obtained, endpoint
+                )
             }
         }
     }
@@ -125,7 +148,7 @@ where
         match self {
             PendingConnectionError::IO(err) => Some(err),
             PendingConnectionError::Transport(_) => None,
-            PendingConnectionError::InvalidPeerId => None,
+            PendingConnectionError::WrongPeerId { .. } => None,
             PendingConnectionError::Aborted => None,
             PendingConnectionError::ConnectionLimit(..) => None,
         }

core/src/connection/pool.rs

@@ -733,7 +733,7 @@ where
             match event {
                 task::PendingConnectionEvent::ConnectionEstablished {
                     id,
-                    output: (peer_id, muxer),
+                    output: (obtained_peer_id, muxer),
                     outgoing,
                 } => {
                     let PendingConnectionInfo {
@@ -777,49 +777,42 @@ where
                         ),
                     };
 
-                    enum Error {
-                        ConnectionLimit(ConnectionLimit),
-                        InvalidPeerId,
-                    }
-
-                    impl<TransportError> From<Error> for PendingConnectionError<TransportError> {
-                        fn from(error: Error) -> Self {
-                            match error {
-                                Error::ConnectionLimit(limit) => {
-                                    PendingConnectionError::ConnectionLimit(limit)
-                                }
-                                Error::InvalidPeerId => PendingConnectionError::InvalidPeerId,
-                            }
-                        }
-                    }
-
-                    let error = self
+                    let error: Result<(), PendingInboundConnectionError<_>> = self
                         .counters
                         // Check general established connection limit.
                         .check_max_established(&endpoint)
-                        .map_err(Error::ConnectionLimit)
+                        .map_err(PendingConnectionError::ConnectionLimit)
                         // Check per-peer established connection limit.
                         .and_then(|()| {
                             self.counters
                                 .check_max_established_per_peer(num_peer_established(
                                     &self.established,
-                                    peer_id,
+                                    obtained_peer_id,
                                 ))
-                                .map_err(Error::ConnectionLimit)
+                                .map_err(PendingConnectionError::ConnectionLimit)
                         })
                         // Check expected peer id matches.
                         .and_then(|()| {
                             if let Some(peer) = expected_peer_id {
-                                if peer != peer_id {
-                                    return Err(Error::InvalidPeerId);
+                                if peer != obtained_peer_id {
+                                    Err(PendingConnectionError::WrongPeerId {
+                                        obtained: obtained_peer_id,
+                                        endpoint: endpoint.clone(),
+                                    })
+                                } else {
+                                    Ok(())
                                 }
+                            } else {
+                                Ok(())
                             }
-                            Ok(())
                         })
                         // Check peer is not local peer.
                         .and_then(|()| {
-                            if self.local_id == peer_id {
-                                Err(Error::InvalidPeerId)
+                            if self.local_id == obtained_peer_id {
+                                Err(PendingConnectionError::WrongPeerId {
+                                    obtained: obtained_peer_id,
+                                    endpoint: endpoint.clone(),
+                                })
                             } else {
                                 Ok(())
                             }
@@ -832,7 +825,7 @@ where
                                     log::debug!(
                                         "Failed to close connection {:?} to peer {}: {:?}",
                                         id,
-                                        peer_id,
+                                        obtained_peer_id,
                                         e
                                     );
                                 }
@@ -845,9 +838,10 @@ where
                             ConnectedPoint::Dialer { .. } => {
                                 return Poll::Ready(PoolEvent::PendingOutboundConnectionError {
                                     id,
-                                    error: error.into(),
+                                    error: error
+                                        .map(|t| vec![(endpoint.get_remote_address().clone(), t)]),
                                     handler,
-                                    peer: Some(peer_id),
+                                    peer: expected_peer_id.or(Some(obtained_peer_id)),
                                 })
                             }
                             ConnectedPoint::Listener {
@@ -856,7 +850,7 @@ where
                             } => {
                                 return Poll::Ready(PoolEvent::PendingInboundConnectionError {
                                     id,
-                                    error: error.into(),
+                                    error,
                                     handler,
                                     send_back_addr,
                                     local_addr,
@@ -866,7 +860,7 @@ where
                     }
 
                     // Add the connection to the pool.
-                    let conns = self.established.entry(peer_id).or_default();
+                    let conns = self.established.entry(obtained_peer_id).or_default();
                     let other_established_connection_ids = conns.keys().cloned().collect();
                     self.counters.inc_established(&endpoint);
 
@@ -875,20 +869,23 @@ where
                     conns.insert(
                         id,
                         EstablishedConnectionInfo {
-                            peer_id,
+                            peer_id: obtained_peer_id,
                             endpoint: endpoint.clone(),
                             sender: command_sender,
                         },
                     );
 
-                    let connected = Connected { peer_id, endpoint };
+                    let connected = Connected {
+                        peer_id: obtained_peer_id,
+                        endpoint,
+                    };
 
                     let connection =
                         super::Connection::new(muxer, handler.into_handler(&connected));
                     self.spawn(
                         task::new_for_established_connection(
                             id,
-                            peer_id,
+                            obtained_peer_id,
                             connection,
                             command_receiver,
                             self.established_connection_events_tx.clone(),

core/src/network.rs

@@ -37,6 +37,7 @@ use crate::{
     Executor, Multiaddr, PeerId,
 };
 use either::Either;
+use multihash::Multihash;
 use std::{
     convert::TryFrom as _,
     error, fmt,
@@ -241,7 +242,7 @@ where
                     .transpose()
                 {
                     Ok(peer_id) => peer_id,
-                    Err(_) => return Err(DialError::InvalidPeerId { handler }),
+                    Err(multihash) => return Err(DialError::InvalidPeerId { handler, multihash }),
                 };
 
                 (
@@ -576,11 +577,10 @@ pub enum DialError<THandler> {
         handler: THandler,
     },
     /// The dialing attempt is rejected because the peer being dialed is the local peer.
-    LocalPeerId {
-        handler: THandler,
-    },
+    LocalPeerId { handler: THandler },
     InvalidPeerId {
         handler: THandler,
+        multihash: Multihash,
     },
 }