diff --git a/crates/net/network/src/error.rs b/crates/net/network/src/error.rs index 18bf85eccc..7457155498 100644 --- a/crates/net/network/src/error.rs +++ b/crates/net/network/src/error.rs @@ -17,7 +17,7 @@ pub enum NetworkError { } /// Returns true if the error indicates that the corresponding peer should be removed from peer -/// discover +/// discovery, for example if it's using a different genesis hash. pub(crate) fn error_merits_discovery_ban(err: &EthStreamError) -> bool { match err { EthStreamError::P2PStreamError(P2PStreamError::HandshakeError( @@ -25,9 +25,6 @@ pub(crate) fn error_merits_discovery_ban(err: &EthStreamError) -> bool { )) | EthStreamError::P2PStreamError(P2PStreamError::HandshakeError( P2PHandshakeError::NonHelloMessageInHandshake, - )) | - EthStreamError::P2PStreamError(P2PStreamError::Disconnected( - DisconnectReason::UselessPeer, )) => true, EthStreamError::HandshakeError(err) => !matches!(err, HandshakeError::NoResponse), _ => false, @@ -36,12 +33,19 @@ pub(crate) fn error_merits_discovery_ban(err: &EthStreamError) -> bool { /// Returns true if the error indicates that we'll never be able to establish a connection to that /// peer. For example, not matching capabilities or a mismatch in protocols. +/// +/// Note: This does not necessarily mean that either of the peers are in violation of the protocol +/// but rather that they'll never be able to connect with each other. This check is a superset of +/// [`error_merits_discovery_ban`] which checks if the peer should not be part of the gossip +/// network. pub(crate) fn is_fatal_protocol_error(err: &EthStreamError) -> bool { match err { EthStreamError::P2PStreamError(err) => { matches!( err, P2PStreamError::HandshakeError(P2PHandshakeError::NoSharedCapabilities) | + P2PStreamError::HandshakeError(P2PHandshakeError::HelloNotInHandshake) | + P2PStreamError::HandshakeError(P2PHandshakeError::NonHelloMessageInHandshake) | P2PStreamError::UnknownReservedMessageId(_) | P2PStreamError::EmptyProtocolMessage | P2PStreamError::ParseVersionError(_) | diff --git a/crates/net/network/src/peers/manager.rs b/crates/net/network/src/peers/manager.rs index 5c1d532df9..3b3c2a1346 100644 --- a/crates/net/network/src/peers/manager.rs +++ b/crates/net/network/src/peers/manager.rs @@ -278,7 +278,10 @@ impl PeersManager { self.connection_info.decr_state(peer.state); } - // If the error is caused by a peer that should be banned + // ban the peer + self.ban_peer(*peer_id); + + // If the error is caused by a peer that should be banned from discovery if error_merits_discovery_ban(err) { self.queued_actions.push_back(PeerAction::DiscoveryBan { peer_id: *peer_id, @@ -771,6 +774,10 @@ mod test { }, PeersConfig, }; + use reth_eth_wire::{ + error::{EthStreamError, P2PStreamError}, + DisconnectReason, + }; use reth_net_common::ban_list::BanList; use reth_primitives::{PeerId, H512}; use std::{ @@ -837,6 +844,50 @@ mod test { .await; } + #[tokio::test] + async fn test_ban_on_drop() { + let peer = PeerId::random(); + let socket_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 1, 2)), 8008); + let mut peers = PeersManager::default(); + peers.add_discovered_node(peer, socket_addr); + + match event!(peers) { + PeerAction::Connect { peer_id, .. } => { + assert_eq!(peer_id, peer); + } + _ => unreachable!(), + } + + poll_fn(|cx| { + assert!(peers.poll(cx).is_pending()); + Poll::Ready(()) + }) + .await; + + peers.on_connection_dropped( + &socket_addr, + &peer, + &EthStreamError::P2PStreamError(P2PStreamError::Disconnected( + DisconnectReason::UselessPeer, + )), + ); + + match event!(peers) { + PeerAction::BanPeer { peer_id } => { + assert_eq!(peer_id, peer); + } + _ => unreachable!(), + } + + poll_fn(|cx| { + assert!(peers.poll(cx).is_pending()); + Poll::Ready(()) + }) + .await; + + assert!(peers.peers.get(&peer).is_none()); + } + #[tokio::test] async fn test_reputation_change() { let peer = PeerId::random();