From 1a8e7725e5cc46fdbbd34c4ef942f13a6eac9a28 Mon Sep 17 00:00:00 2001 From: Roland Kuhn Date: Tue, 11 Jan 2022 22:19:43 +0100 Subject: [PATCH 1/4] fix peer_id in case of DialError::InvalidPeerId (#2425) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- core/src/connection/error.rs | 12 +++--- core/src/connection/pool.rs | 10 ++--- core/tests/network_dial_error.rs | 65 ++++++++++++++++++++++++++++++++ misc/metrics/src/swarm.rs | 4 +- protocols/kad/src/behaviour.rs | 2 +- swarm/src/lib.rs | 8 ++-- 6 files changed, 83 insertions(+), 18 deletions(-) diff --git a/core/src/connection/error.rs b/core/src/connection/error.rs index e127db7574e..949f8d3cf48 100644 --- a/core/src/connection/error.rs +++ b/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, PeerId}; use std::{fmt, io}; /// Errors that can occur in the context of an established `Connection`. @@ -84,8 +84,8 @@ pub enum PendingConnectionError { 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. + InvalidPeerId(PeerId), /// An I/O error occurred on the connection. // TODO: Eventually this should also be a custom error? @@ -110,8 +110,8 @@ where PendingConnectionError::ConnectionLimit(l) => { write!(f, "Connection error: Connection limit: {}.", l) } - PendingConnectionError::InvalidPeerId => { - write!(f, "Pending connection: Invalid peer ID.") + PendingConnectionError::InvalidPeerId(id) => { + write!(f, "Pending connection: Unexpected peer ID {}.", id) } } } @@ -125,7 +125,7 @@ where match self { PendingConnectionError::IO(err) => Some(err), PendingConnectionError::Transport(_) => None, - PendingConnectionError::InvalidPeerId => None, + PendingConnectionError::InvalidPeerId(_) => None, PendingConnectionError::Aborted => None, PendingConnectionError::ConnectionLimit(..) => None, } diff --git a/core/src/connection/pool.rs b/core/src/connection/pool.rs index 85ac1b8a7ca..13361ec1564 100644 --- a/core/src/connection/pool.rs +++ b/core/src/connection/pool.rs @@ -771,7 +771,7 @@ where enum Error { ConnectionLimit(ConnectionLimit), - InvalidPeerId, + InvalidPeerId(PeerId), } impl From for PendingConnectionError { @@ -780,7 +780,7 @@ where Error::ConnectionLimit(limit) => { PendingConnectionError::ConnectionLimit(limit) } - Error::InvalidPeerId => PendingConnectionError::InvalidPeerId, + Error::InvalidPeerId(x) => PendingConnectionError::InvalidPeerId(x), } } } @@ -803,7 +803,7 @@ where .and_then(|()| { if let Some(peer) = expected_peer_id { if peer != peer_id { - return Err(Error::InvalidPeerId); + return Err(Error::InvalidPeerId(peer_id)); } } Ok(()) @@ -811,7 +811,7 @@ where // Check peer is not local peer. .and_then(|()| { if self.local_id == peer_id { - Err(Error::InvalidPeerId) + Err(Error::InvalidPeerId(peer_id)) } else { Ok(()) } @@ -839,7 +839,7 @@ where id, error: error.into(), handler, - peer: Some(peer_id), + peer: expected_peer_id.or(Some(peer_id)), }) } ConnectedPoint::Listener { diff --git a/core/tests/network_dial_error.rs b/core/tests/network_dial_error.rs index 330061bb0bd..33b63d438c3 100644 --- a/core/tests/network_dial_error.rs +++ b/core/tests/network_dial_error.rs @@ -21,6 +21,7 @@ mod util; use futures::prelude::*; +use libp2p_core::identity; use libp2p_core::multiaddr::multiaddr; use libp2p_core::DialOpts; use libp2p_core::{ @@ -88,6 +89,70 @@ fn deny_incoming_connec() { .unwrap(); } +#[test] +fn invalid_peer_id() { + // Checks whether dialling with the wrong peer id raises the correct error + + let mut swarm1 = test_network(NetworkConfig::default()); + let mut swarm2 = test_network(NetworkConfig::default()); + + swarm1.listen_on("/memory/0".parse().unwrap()).unwrap(); + + let address = async_std::task::block_on(future::poll_fn(|cx| match swarm1.poll(cx) { + Poll::Ready(NetworkEvent::NewListenerAddress { listen_addr, .. }) => { + Poll::Ready(listen_addr) + } + Poll::Pending => Poll::Pending, + _ => panic!("Was expecting the listen address to be reported"), + })); + + let other_keys = identity::Keypair::generate_ed25519(); + let other_id = other_keys.public().to_peer_id(); + let other_addr = address.with(Protocol::P2p(other_id.into())); + + swarm2.dial(&other_addr, TestHandler()).unwrap(); + + let (peer_id, error) = async_std::task::block_on(future::poll_fn( + |cx| -> Poll<(PeerId, PendingConnectionError<()>)> { + if let Poll::Ready(NetworkEvent::IncomingConnection { connection, .. }) = + swarm1.poll(cx) + { + swarm1.accept(connection, TestHandler()).unwrap(); + } + + match swarm2.poll(cx) { + Poll::Ready(NetworkEvent::DialError { + peer_id, + error, + handler: _, + }) => { + let error = match error { + PendingConnectionError::Transport(_) => { + PendingConnectionError::Transport(()) + } + PendingConnectionError::ConnectionLimit(l) => { + PendingConnectionError::ConnectionLimit(l) + } + PendingConnectionError::Aborted => PendingConnectionError::Aborted, + PendingConnectionError::InvalidPeerId(id) => { + PendingConnectionError::InvalidPeerId(id) + } + PendingConnectionError::IO(e) => PendingConnectionError::IO(e), + }; + Poll::Ready((peer_id, error)) + } + Poll::Ready(x) => panic!("unexpected {:?}", x), + Poll::Pending => Poll::Pending, + } + }, + )); + assert_eq!(peer_id, other_id); + match error { + PendingConnectionError::InvalidPeerId(id) => assert_eq!(id, *swarm1.local_peer_id()), + x => panic!("wrong error {:?}", x), + } +} + #[test] fn dial_self() { // Check whether dialing ourselves correctly fails. diff --git a/misc/metrics/src/swarm.rs b/misc/metrics/src/swarm.rs index ea6f3da20b8..b0672413959 100644 --- a/misc/metrics/src/swarm.rs +++ b/misc/metrics/src/swarm.rs @@ -216,7 +216,7 @@ impl super::Recorder { record(OutgoingConnectionErrorError::Aborted) } - libp2p_swarm::DialError::InvalidPeerId => { + libp2p_swarm::DialError::InvalidPeerId(_) => { record(OutgoingConnectionErrorError::InvalidPeerId) } libp2p_swarm::DialError::ConnectionIo(_) => { @@ -317,7 +317,7 @@ impl From<&libp2p_core::connection::PendingInboundConnectionError) -> Self { match error { - libp2p_core::connection::PendingInboundConnectionError::InvalidPeerId => { + libp2p_core::connection::PendingInboundConnectionError::InvalidPeerId(_) => { PendingInboundConnectionError::InvalidPeerId } libp2p_core::connection::PendingInboundConnectionError::ConnectionLimit(_) => { diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index c08e1401507..844c85d9c53 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -1919,7 +1919,7 @@ where DialError::Banned | DialError::ConnectionLimit(_) | DialError::LocalPeerId - | DialError::InvalidPeerId + | DialError::InvalidPeerId(_) | DialError::Aborted | DialError::ConnectionIo(_) | DialError::Transport(_) diff --git a/swarm/src/lib.rs b/swarm/src/lib.rs index cc09ebf22a6..1d83c2941d0 100644 --- a/swarm/src/lib.rs +++ b/swarm/src/lib.rs @@ -1329,7 +1329,7 @@ pub enum DialError { /// The provided peer identity is invalid or the peer identity obtained on /// the connection did not match the one that was expected or is otherwise /// invalid. - InvalidPeerId, + InvalidPeerId(PeerId), /// An I/O error occurred on the connection. ConnectionIo(io::Error), /// An error occurred while negotiating the transport protocol(s) on a connection. @@ -1353,7 +1353,7 @@ impl From> for DialError { match error { PendingConnectionError::ConnectionLimit(limit) => DialError::ConnectionLimit(limit), PendingConnectionError::Aborted => DialError::Aborted, - PendingConnectionError::InvalidPeerId => DialError::InvalidPeerId, + PendingConnectionError::InvalidPeerId(id) => DialError::InvalidPeerId(id), PendingConnectionError::IO(e) => DialError::ConnectionIo(e), PendingConnectionError::Transport(e) => DialError::Transport(e), } @@ -1378,7 +1378,7 @@ impl fmt::Display for DialError { f, "Dial error: Pending connection attempt has been aborted." ), - DialError::InvalidPeerId => write!(f, "Dial error: Invalid peer ID."), + DialError::InvalidPeerId(id) => write!(f, "Dial error: Unexpected peer ID {}.", id), DialError::ConnectionIo(e) => write!( f, "Dial error: An I/O error occurred on the connection: {:?}.", e @@ -1397,7 +1397,7 @@ impl error::Error for DialError { DialError::Banned => None, DialError::DialPeerConditionFalse(_) => None, DialError::Aborted => None, - DialError::InvalidPeerId => None, + DialError::InvalidPeerId(_) => None, DialError::ConnectionIo(_) => None, DialError::Transport(_) => None, } From 4bf8267a403237662ca0b44284a288ff0b2e70dd Mon Sep 17 00:00:00 2001 From: Roland Kuhn Date: Wed, 12 Jan 2022 21:12:21 +0100 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: Max Inden --- core/tests/network_dial_error.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/tests/network_dial_error.rs b/core/tests/network_dial_error.rs index 33b63d438c3..e8bc8179492 100644 --- a/core/tests/network_dial_error.rs +++ b/core/tests/network_dial_error.rs @@ -91,7 +91,8 @@ fn deny_incoming_connec() { #[test] fn invalid_peer_id() { - // Checks whether dialling with the wrong peer id raises the correct error + // Checks whether dialing an address containing the wrong peer id raises an error + // for the expected peer id instead of the obtained peer id. let mut swarm1 = test_network(NetworkConfig::default()); let mut swarm2 = test_network(NetworkConfig::default()); @@ -106,8 +107,7 @@ fn invalid_peer_id() { _ => panic!("Was expecting the listen address to be reported"), })); - let other_keys = identity::Keypair::generate_ed25519(); - let other_id = other_keys.public().to_peer_id(); + let other_id = PeerId::random(); let other_addr = address.with(Protocol::P2p(other_id.into())); swarm2.dial(&other_addr, TestHandler()).unwrap(); From 561b922706e5c124151020ab79d47b354fbd0f98 Mon Sep 17 00:00:00 2001 From: Roland Kuhn Date: Wed, 12 Jan 2022 21:32:34 +0100 Subject: [PATCH 3/4] include remote address in InvalidPeerId error also add changelog entries --- core/CHANGELOG.md | 5 +- core/src/connection/error.rs | 31 +++++++++-- core/src/connection/pool.rs | 60 ++++++++++----------- core/src/network.rs | 8 +-- core/tests/network_dial_error.rs | 69 +++++++++--------------- misc/metrics/src/swarm.rs | 12 +++-- protocols/kad/src/behaviour.rs | 3 +- protocols/ping/tests/ping.rs | 2 +- protocols/rendezvous/tests/harness.rs | 4 +- protocols/rendezvous/tests/rendezvous.rs | 2 +- swarm/CHANGELOG.md | 13 +++-- swarm/src/lib.rs | 26 ++++++--- 12 files changed, 127 insertions(+), 108 deletions(-) diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index b7db3b4f919..30fa6a073f5 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -22,12 +22,15 @@ - Implement `Serialize` and `Deserialize` for `PeerId` (see [PR 2408]) +- Report negotiated and expected PeerId as well as remote address in DialError::InvalidPeerId (see [PR 2428]) + [PR 2339]: https://github.com/libp2p/rust-libp2p/pull/2339 [PR 2350]: https://github.com/libp2p/rust-libp2p/pull/2350 [PR 2352]: https://github.com/libp2p/rust-libp2p/pull/2352 [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 # 0.30.1 [2021-11-16] @@ -115,7 +118,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] diff --git a/core/src/connection/error.rs b/core/src/connection/error.rs index 949f8d3cf48..4db4c59e91c 100644 --- a/core/src/connection/error.rs +++ b/core/src/connection/error.rs @@ -85,13 +85,32 @@ pub enum PendingConnectionError { /// The peer identity obtained on the connection did not /// match the one that was expected or is the local one. - InvalidPeerId(PeerId), + WrongPeerId { + obtained: PeerId, + address: Multiaddr, + }, /// An I/O error occurred on the connection. // TODO: Eventually this should also be a custom error? IO(io::Error), } +impl PendingConnectionError { + pub fn map(self, f: impl FnOnce(T) -> U) -> PendingConnectionError { + match self { + PendingConnectionError::Transport(t) => PendingConnectionError::Transport(f(t)), + PendingConnectionError::ConnectionLimit(l) => { + PendingConnectionError::ConnectionLimit(l) + } + PendingConnectionError::Aborted => PendingConnectionError::Aborted, + PendingConnectionError::WrongPeerId { obtained, address } => { + PendingConnectionError::WrongPeerId { obtained, address } + } + PendingConnectionError::IO(e) => PendingConnectionError::IO(e), + } + } +} + impl fmt::Display for PendingConnectionError where TTransErr: fmt::Display + fmt::Debug, @@ -110,8 +129,12 @@ where PendingConnectionError::ConnectionLimit(l) => { write!(f, "Connection error: Connection limit: {}.", l) } - PendingConnectionError::InvalidPeerId(id) => { - write!(f, "Pending connection: Unexpected peer ID {}.", id) + PendingConnectionError::WrongPeerId { obtained, address } => { + write!( + f, + "Pending connection: Unexpected peer ID {} at {}.", + obtained, address + ) } } } @@ -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, } diff --git a/core/src/connection/pool.rs b/core/src/connection/pool.rs index 13361ec1564..1815dfe2587 100644 --- a/core/src/connection/pool.rs +++ b/core/src/connection/pool.rs @@ -729,7 +729,7 @@ where match event { task::PendingConnectionEvent::ConnectionEstablished { id, - output: (peer_id, muxer), + output: (obtained_peer_id, muxer), outgoing, } => { let PendingConnectionInfo { @@ -769,49 +769,39 @@ where ), }; - enum Error { - ConnectionLimit(ConnectionLimit), - InvalidPeerId(PeerId), - } - - impl From for PendingConnectionError { - fn from(error: Error) -> Self { - match error { - Error::ConnectionLimit(limit) => { - PendingConnectionError::ConnectionLimit(limit) - } - Error::InvalidPeerId(x) => PendingConnectionError::InvalidPeerId(x), - } - } - } - - 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(peer_id)); + if peer != obtained_peer_id { + return Err(PendingConnectionError::WrongPeerId { + obtained: obtained_peer_id, + address: endpoint.get_remote_address().clone(), + }); } } Ok(()) }) // Check peer is not local peer. .and_then(|()| { - if self.local_id == peer_id { - Err(Error::InvalidPeerId(peer_id)) + if self.local_id == obtained_peer_id { + Err(PendingConnectionError::WrongPeerId { + obtained: obtained_peer_id, + address: endpoint.get_remote_address().clone(), + }) } else { Ok(()) } @@ -824,7 +814,7 @@ where log::debug!( "Failed to close connection {:?} to peer {}: {:?}", id, - peer_id, + obtained_peer_id, e ); } @@ -837,9 +827,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: expected_peer_id.or(Some(peer_id)), + peer: expected_peer_id.or(Some(obtained_peer_id)), }) } ConnectedPoint::Listener { @@ -848,7 +839,7 @@ where } => { return Poll::Ready(PoolEvent::PendingInboundConnectionError { id, - error: error.into(), + error, handler, send_back_addr, local_addr, @@ -858,7 +849,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); @@ -867,20 +858,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(), diff --git a/core/src/network.rs b/core/src/network.rs index 63a07573c7f..025ed7c0a60 100644 --- a/core/src/network.rs +++ b/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, @@ -236,7 +237,7 @@ where .transpose() { Ok(peer_id) => peer_id, - Err(_) => return Err(DialError::InvalidPeerId { handler }), + Err(multihash) => return Err(DialError::InvalidPeerId { handler, multihash }), }; (peer_id, Either::Right(std::iter::once(address)), None) @@ -565,11 +566,10 @@ pub enum DialError { 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, }, } diff --git a/core/tests/network_dial_error.rs b/core/tests/network_dial_error.rs index e8bc8179492..f88b92d6c6d 100644 --- a/core/tests/network_dial_error.rs +++ b/core/tests/network_dial_error.rs @@ -21,7 +21,6 @@ mod util; use futures::prelude::*; -use libp2p_core::identity; use libp2p_core::multiaddr::multiaddr; use libp2p_core::DialOpts; use libp2p_core::{ @@ -43,7 +42,7 @@ fn deny_incoming_connec() { swarm1.listen_on("/memory/0".parse().unwrap()).unwrap(); - let address = async_std::task::block_on(future::poll_fn(|cx| match swarm1.poll(cx) { + let address = futures::executor::block_on(future::poll_fn(|cx| match swarm1.poll(cx) { Poll::Ready(NetworkEvent::NewListenerAddress { listen_addr, .. }) => { Poll::Ready(listen_addr) } @@ -60,7 +59,7 @@ fn deny_incoming_connec() { ) .unwrap(); - async_std::task::block_on(future::poll_fn(|cx| -> Poll> { + futures::executor::block_on(future::poll_fn(|cx| -> Poll> { match swarm1.poll(cx) { Poll::Ready(NetworkEvent::IncomingConnection { connection, .. }) => drop(connection), Poll::Ready(_) => unreachable!(), @@ -99,7 +98,7 @@ fn invalid_peer_id() { swarm1.listen_on("/memory/0".parse().unwrap()).unwrap(); - let address = async_std::task::block_on(future::poll_fn(|cx| match swarm1.poll(cx) { + let address = futures::executor::block_on(future::poll_fn(|cx| match swarm1.poll(cx) { Poll::Ready(NetworkEvent::NewListenerAddress { listen_addr, .. }) => { Poll::Ready(listen_addr) } @@ -110,45 +109,27 @@ fn invalid_peer_id() { let other_id = PeerId::random(); let other_addr = address.with(Protocol::P2p(other_id.into())); - swarm2.dial(&other_addr, TestHandler()).unwrap(); + swarm2.dial(TestHandler(), other_addr.clone()).unwrap(); - let (peer_id, error) = async_std::task::block_on(future::poll_fn( - |cx| -> Poll<(PeerId, PendingConnectionError<()>)> { - if let Poll::Ready(NetworkEvent::IncomingConnection { connection, .. }) = - swarm1.poll(cx) - { - swarm1.accept(connection, TestHandler()).unwrap(); - } + let (peer_id, error) = futures::executor::block_on(future::poll_fn(|cx| { + if let Poll::Ready(NetworkEvent::IncomingConnection { connection, .. }) = swarm1.poll(cx) { + swarm1.accept(connection, TestHandler()).unwrap(); + } - match swarm2.poll(cx) { - Poll::Ready(NetworkEvent::DialError { - peer_id, - error, - handler: _, - }) => { - let error = match error { - PendingConnectionError::Transport(_) => { - PendingConnectionError::Transport(()) - } - PendingConnectionError::ConnectionLimit(l) => { - PendingConnectionError::ConnectionLimit(l) - } - PendingConnectionError::Aborted => PendingConnectionError::Aborted, - PendingConnectionError::InvalidPeerId(id) => { - PendingConnectionError::InvalidPeerId(id) - } - PendingConnectionError::IO(e) => PendingConnectionError::IO(e), - }; - Poll::Ready((peer_id, error)) - } - Poll::Ready(x) => panic!("unexpected {:?}", x), - Poll::Pending => Poll::Pending, + match swarm2.poll(cx) { + Poll::Ready(NetworkEvent::DialError { peer_id, error, .. }) => { + Poll::Ready((peer_id, error)) } - }, - )); + Poll::Ready(x) => panic!("unexpected {:?}", x), + Poll::Pending => Poll::Pending, + } + })); assert_eq!(peer_id, other_id); match error { - PendingConnectionError::InvalidPeerId(id) => assert_eq!(id, *swarm1.local_peer_id()), + PendingConnectionError::WrongPeerId { obtained, address } => { + assert_eq!(obtained, *swarm1.local_peer_id()); + assert_eq!(address, other_addr); + } x => panic!("wrong error {:?}", x), } } @@ -168,7 +149,7 @@ fn dial_self() { let mut swarm = test_network(NetworkConfig::default()); swarm.listen_on("/memory/0".parse().unwrap()).unwrap(); - let local_address = async_std::task::block_on(future::poll_fn(|cx| match swarm.poll(cx) { + let local_address = futures::executor::block_on(future::poll_fn(|cx| match swarm.poll(cx) { Poll::Ready(NetworkEvent::NewListenerAddress { listen_addr, .. }) => { Poll::Ready(listen_addr) } @@ -180,12 +161,12 @@ fn dial_self() { let mut got_dial_err = false; let mut got_inc_err = false; - async_std::task::block_on(future::poll_fn(|cx| -> Poll> { + futures::executor::block_on(future::poll_fn(|cx| -> Poll> { loop { match swarm.poll(cx) { Poll::Ready(NetworkEvent::DialError { peer_id, - error: PendingConnectionError::InvalidPeerId { .. }, + error: PendingConnectionError::WrongPeerId { .. }, .. }) => { assert_eq!(&peer_id, swarm.local_peer_id()); @@ -222,7 +203,7 @@ fn dial_self_by_id() { // Trying to dial self by passing the same `PeerId` shouldn't even be possible in the first // place. let mut swarm = test_network(NetworkConfig::default()); - let peer_id = swarm.local_peer_id().clone(); + let peer_id = *swarm.local_peer_id(); assert!(swarm.peer(peer_id).into_disconnected().is_none()); } @@ -246,13 +227,13 @@ fn multiple_addresses_err() { swarm .dial( TestHandler(), - DialOpts::peer_id(target.clone()) + DialOpts::peer_id(target) .addresses(addresses.clone()) .build(), ) .unwrap(); - async_std::task::block_on(future::poll_fn(|cx| -> Poll> { + futures::executor::block_on(future::poll_fn(|cx| -> Poll> { loop { match swarm.poll(cx) { Poll::Ready(NetworkEvent::DialError { diff --git a/misc/metrics/src/swarm.rs b/misc/metrics/src/swarm.rs index b0672413959..07e4aea3d09 100644 --- a/misc/metrics/src/swarm.rs +++ b/misc/metrics/src/swarm.rs @@ -216,9 +216,12 @@ impl super::Recorder { record(OutgoingConnectionErrorError::Aborted) } - libp2p_swarm::DialError::InvalidPeerId(_) => { + libp2p_swarm::DialError::InvalidPeerId { .. } => { record(OutgoingConnectionErrorError::InvalidPeerId) } + libp2p_swarm::DialError::WrongPeerId { .. } => { + record(OutgoingConnectionErrorError::WrongPeerId) + } libp2p_swarm::DialError::ConnectionIo(_) => { record(OutgoingConnectionErrorError::ConnectionIo) } @@ -292,6 +295,7 @@ enum OutgoingConnectionErrorError { DialPeerConditionFalse, Aborted, InvalidPeerId, + WrongPeerId, ConnectionIo, TransportMultiaddrNotSupported, TransportOther, @@ -304,7 +308,7 @@ struct IncomingConnectionErrorLabels { #[derive(Encode, Hash, Clone, Eq, PartialEq)] enum PendingInboundConnectionError { - InvalidPeerId, + WrongPeerId, TransportErrorMultiaddrNotSupported, TransportErrorOther, Aborted, @@ -317,8 +321,8 @@ impl From<&libp2p_core::connection::PendingInboundConnectionError) -> Self { match error { - libp2p_core::connection::PendingInboundConnectionError::InvalidPeerId(_) => { - PendingInboundConnectionError::InvalidPeerId + libp2p_core::connection::PendingInboundConnectionError::WrongPeerId { .. } => { + PendingInboundConnectionError::WrongPeerId } libp2p_core::connection::PendingInboundConnectionError::ConnectionLimit(_) => { PendingInboundConnectionError::ConnectionLimit diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 844c85d9c53..e4a004309ab 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -1919,7 +1919,8 @@ where DialError::Banned | DialError::ConnectionLimit(_) | DialError::LocalPeerId - | DialError::InvalidPeerId(_) + | DialError::InvalidPeerId { .. } + | DialError::WrongPeerId { .. } | DialError::Aborted | DialError::ConnectionIo(_) | DialError::Transport(_) diff --git a/protocols/ping/tests/ping.rs b/protocols/ping/tests/ping.rs index 3a7acd72fb1..dbde7db608d 100644 --- a/protocols/ping/tests/ping.rs +++ b/protocols/ping/tests/ping.rs @@ -30,7 +30,7 @@ use libp2p_core::{ use libp2p_mplex as mplex; use libp2p_noise as noise; use libp2p_ping as ping; -use libp2p_swarm::{dial_opts::DialOpts, DummyBehaviour, KeepAlive, Swarm, SwarmEvent}; +use libp2p_swarm::{DummyBehaviour, KeepAlive, Swarm, SwarmEvent}; use libp2p_tcp::TcpConfig; use libp2p_yamux as yamux; use quickcheck::*; diff --git a/protocols/rendezvous/tests/harness.rs b/protocols/rendezvous/tests/harness.rs index 6b5a202b476..3709e509a06 100644 --- a/protocols/rendezvous/tests/harness.rs +++ b/protocols/rendezvous/tests/harness.rs @@ -29,9 +29,7 @@ use libp2p::core::upgrade::SelectUpgrade; use libp2p::core::{identity, Multiaddr, PeerId, Transport}; use libp2p::mplex::MplexConfig; use libp2p::noise::{Keypair, NoiseConfig, X25519Spec}; -use libp2p::swarm::{ - dial_opts::DialOpts, AddressScore, NetworkBehaviour, Swarm, SwarmBuilder, SwarmEvent, -}; +use libp2p::swarm::{AddressScore, NetworkBehaviour, Swarm, SwarmBuilder, SwarmEvent}; use libp2p::yamux::YamuxConfig; use std::fmt::Debug; use std::time::Duration; diff --git a/protocols/rendezvous/tests/rendezvous.rs b/protocols/rendezvous/tests/rendezvous.rs index 7d02610a5af..458ea588832 100644 --- a/protocols/rendezvous/tests/rendezvous.rs +++ b/protocols/rendezvous/tests/rendezvous.rs @@ -26,7 +26,7 @@ use futures::stream::FuturesUnordered; use futures::StreamExt; use libp2p_core::identity; use libp2p_rendezvous as rendezvous; -use libp2p_swarm::{dial_opts::DialOpts, DialError, Swarm, SwarmEvent}; +use libp2p_swarm::{DialError, Swarm, SwarmEvent}; use std::convert::TryInto; use std::time::Duration; diff --git a/swarm/CHANGELOG.md b/swarm/CHANGELOG.md index a97ea499aa6..c5a1260d8b9 100644 --- a/swarm/CHANGELOG.md +++ b/swarm/CHANGELOG.md @@ -17,6 +17,8 @@ - Enable overriding _dial concurrency factor_ per dial via `DialOpts::override_dial_concurrency_factor`. See [PR 2404]. +- Report negotiated and expected PeerId as well as remote address in DialError::InvalidPeerId (see [PR 2428]) + [PR 2339]: https://github.com/libp2p/rust-libp2p/pull/2339 [PR 2350]: https://github.com/libp2p/rust-libp2p/pull/2350 [PR 2362]: https://github.com/libp2p/rust-libp2p/pull/2362 @@ -24,6 +26,7 @@ [PR 2375]: https://github.com/libp2p/rust-libp2p/pull/2375 [PR 2378]: https://github.com/libp2p/rust-libp2p/pull/2378 [PR 2404]: https://github.com/libp2p/rust-libp2p/pull/2404 +[PR 2428]: https://github.com/libp2p/rust-libp2p/pull/2428 # 0.32.0 [2021-11-16] @@ -36,16 +39,17 @@ Changes required to maintain status quo: - - Previously `swarm.dial(peer_id)` + - Previously `swarm.dial(peer_id)` now `swarm.dial(DialOpts::peer_id(peer_id).build())` or `swarm.dial(peer_id)` given that `DialOpts` implements `From`. - - Previously `swarm.dial_addr(addr)` + - Previously `swarm.dial_addr(addr)` now `swarm.dial(DialOpts::unknown_peer_id().address(addr).build())` or `swarm.dial(addr)` given that `DialOpts` implements `From`. - - Previously `NetworkBehaviourAction::DialPeer { peer_id, condition, handler }` + - Previously `NetworkBehaviourAction::DialPeer { peer_id, condition, handler }` now + ```rust NetworkBehaviourAction::Dial { opts: DialOpts::peer_id(peer_id) @@ -55,8 +59,9 @@ } ``` - - Previously `NetworkBehaviourAction::DialAddress { address, handler }` + - Previously `NetworkBehaviourAction::DialAddress { address, handler }` now + ```rust NetworkBehaviourAction::Dial { opts: DialOpts::unknown_peer_id() diff --git a/swarm/src/lib.rs b/swarm/src/lib.rs index 1d83c2941d0..f1ccce4ab80 100644 --- a/swarm/src/lib.rs +++ b/swarm/src/lib.rs @@ -81,6 +81,7 @@ use libp2p_core::{ EstablishedConnection, IntoConnectionHandler, ListenerId, PendingConnectionError, PendingInboundConnectionError, PendingOutboundConnectionError, Substream, }, + multihash::Multihash, muxing::StreamMuxerBox, network::{ self, peer::ConnectedPeer, ConnectionLimits, Network, NetworkConfig, NetworkEvent, @@ -1326,10 +1327,13 @@ pub enum DialError { DialPeerConditionFalse(dial_opts::PeerCondition), /// Pending connection attempt has been aborted. Aborted, - /// The provided peer identity is invalid or the peer identity obtained on - /// the connection did not match the one that was expected or is otherwise - /// invalid. - InvalidPeerId(PeerId), + /// The provided peer identity is invalid + InvalidPeerId(Multihash), + /// The peer identity obtained on the connection did not match the one that was expected. + WrongPeerId { + obtained: PeerId, + address: Multiaddr, + }, /// An I/O error occurred on the connection. ConnectionIo(io::Error), /// An error occurred while negotiating the transport protocol(s) on a connection. @@ -1343,7 +1347,9 @@ impl DialError { (DialError::ConnectionLimit(limit), handler) } network::DialError::LocalPeerId { handler } => (DialError::LocalPeerId, handler), - network::DialError::InvalidPeerId { handler } => (DialError::InvalidPeerId, handler), + network::DialError::InvalidPeerId { handler, multihash } => { + (DialError::InvalidPeerId(multihash), handler) + } } } } @@ -1353,7 +1359,9 @@ impl From> for DialError { match error { PendingConnectionError::ConnectionLimit(limit) => DialError::ConnectionLimit(limit), PendingConnectionError::Aborted => DialError::Aborted, - PendingConnectionError::InvalidPeerId(id) => DialError::InvalidPeerId(id), + PendingConnectionError::WrongPeerId { obtained, address } => { + DialError::WrongPeerId { obtained, address } + } PendingConnectionError::IO(e) => DialError::ConnectionIo(e), PendingConnectionError::Transport(e) => DialError::Transport(e), } @@ -1378,7 +1386,8 @@ impl fmt::Display for DialError { f, "Dial error: Pending connection attempt has been aborted." ), - DialError::InvalidPeerId(id) => write!(f, "Dial error: Unexpected peer ID {}.", id), + DialError::InvalidPeerId(multihash) => write!(f, "Dial error: multihash {:?} is not a PeerId", multihash), + DialError::WrongPeerId { obtained, address} => write!(f, "Dial error: Unexpected peer ID {} at {}.", obtained, address), DialError::ConnectionIo(e) => write!( f, "Dial error: An I/O error occurred on the connection: {:?}.", e @@ -1397,7 +1406,8 @@ impl error::Error for DialError { DialError::Banned => None, DialError::DialPeerConditionFalse(_) => None, DialError::Aborted => None, - DialError::InvalidPeerId(_) => None, + DialError::InvalidPeerId { .. } => None, + DialError::WrongPeerId { .. } => None, DialError::ConnectionIo(_) => None, DialError::Transport(_) => None, } From 020f9b9b5e04ac4a1cea2a83a6dfd61889d6090e Mon Sep 17 00:00:00 2001 From: Roland Kuhn Date: Sat, 15 Jan 2022 14:52:56 +0100 Subject: [PATCH 4/4] fix changelog entries (InvalidPeerId -> WrongPeerId) --- core/CHANGELOG.md | 6 +++--- swarm/CHANGELOG.md | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index 30fa6a073f5..0bc493b8040 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -14,15 +14,15 @@ - 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::InvalidPeerId (see [PR 2428]) +- Report negotiated and expected PeerId as well as remote address in DialError::WrongPeerId (see [PR 2428]) [PR 2339]: https://github.com/libp2p/rust-libp2p/pull/2339 [PR 2350]: https://github.com/libp2p/rust-libp2p/pull/2350 diff --git a/swarm/CHANGELOG.md b/swarm/CHANGELOG.md index c5a1260d8b9..dce6c7052f8 100644 --- a/swarm/CHANGELOG.md +++ b/swarm/CHANGELOG.md @@ -17,7 +17,7 @@ - Enable overriding _dial concurrency factor_ per dial via `DialOpts::override_dial_concurrency_factor`. See [PR 2404]. -- Report negotiated and expected PeerId as well as remote address in DialError::InvalidPeerId (see [PR 2428]) +- Report negotiated and expected PeerId as well as remote address in DialError::WrongPeerId (see [PR 2428]) [PR 2339]: https://github.com/libp2p/rust-libp2p/pull/2339 [PR 2350]: https://github.com/libp2p/rust-libp2p/pull/2350