fix(net): ban peer on fatal error (#543)

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(_) |

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();