Merge branch 'unstable' of github.com:jxs/lighthouse into move-to-workspace

Author: jxs

beacon_node/lighthouse_network/src/discovery/mod.rs

@@ -550,8 +550,6 @@ impl<TSpec: EthSpec> Discovery<TSpec> {
         if let Ok(node_id) = peer_id_to_node_id(peer_id) {
             // If we could convert this peer id, remove it from the DHT and ban it from discovery.
             self.discv5.ban_node(&node_id, None);
-            // Remove the node from the routing table.
-            self.discv5.remove_node(&node_id);
         }
 
         for ip_address in ip_addresses {

beacon_node/lighthouse_network/src/peer_manager/mod.rs

@@ -414,7 +414,7 @@ impl<TSpec: EthSpec> PeerManager<TSpec> {
     /// Reports if a peer is banned or not.
     ///
     /// This is used to determine if we should accept incoming connections.
-    pub fn ban_status(&self, peer_id: &PeerId) -> BanResult {
+    pub fn ban_status(&self, peer_id: &PeerId) -> Option<BanResult> {
         self.network_globals.peers.read().ban_status(peer_id)
     }
 
@@ -802,7 +802,7 @@ impl<TSpec: EthSpec> PeerManager<TSpec> {
     ) -> bool {
         {
             let mut peerdb = self.network_globals.peers.write();
-            if !matches!(peerdb.ban_status(peer_id), BanResult::NotBanned) {
+            if peerdb.ban_status(peer_id).is_some() {
                 // don't connect if the peer is banned
                 error!(self.log, "Connection has been allowed to a banned peer"; "peer_id" => %peer_id);
             }

beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs

@@ -8,7 +8,7 @@ use libp2p::identity::PeerId;
 use libp2p::swarm::behaviour::{ConnectionClosed, ConnectionEstablished, DialFailure, FromSwarm};
 use libp2p::swarm::dial_opts::{DialOpts, PeerCondition};
 use libp2p::swarm::dummy::ConnectionHandler;
-use libp2p::swarm::{ConnectionId, NetworkBehaviour, PollParameters, ToSwarm};
+use libp2p::swarm::{ConnectionDenied, ConnectionId, NetworkBehaviour, PollParameters, ToSwarm};
 use slog::{debug, error};
 use types::EthSpec;
 
@@ -157,12 +157,30 @@ impl<TSpec: EthSpec> NetworkBehaviour for PeerManager<TSpec> {
     fn handle_established_inbound_connection(
         &mut self,
         _connection_id: ConnectionId,
-        _peer: PeerId,
+        peer_id: PeerId,
         _local_addr: &libp2p::Multiaddr,
         _remote_addr: &libp2p::Multiaddr,
-    ) -> Result<libp2p::swarm::THandler<Self>, libp2p::swarm::ConnectionDenied> {
-        // TODO: we might want to check if we accept this peer or not in the future.
-        Ok(ConnectionHandler)
+    ) -> Result<libp2p::swarm::THandler<Self>, ConnectionDenied> {
+        // Check to make sure the peer is not supposed to be banned
+        match self.ban_status(&peer_id) {
+            Some(cause @ BanResult::BadScore) => {
+                // This is a faulty state
+                error!(self.log, "Connected to a banned peer. Re-banning"; "peer_id" => %peer_id);
+                // Disconnect the peer.
+                self.goodbye_peer(&peer_id, GoodbyeReason::Banned, ReportSource::PeerManager);
+                // Re-ban the peer to prevent repeated errors.
+                self.events.push(PeerManagerEvent::Banned(peer_id, vec![]));
+                Err(ConnectionDenied::new(cause))
+            }
+            Some(cause @ BanResult::BannedIp(ip_addr)) => {
+                // A good peer has connected to us via a banned IP address. We ban the peer and
+                // prevent future connections.
+                debug!(self.log, "Peer connected via banned IP. Banning"; "peer_id" => %peer_id, "banned_ip" => %ip_addr);
+                self.goodbye_peer(&peer_id, GoodbyeReason::BannedIP, ReportSource::PeerManager);
+                Err(ConnectionDenied::new(cause))
+            }
+            None => Ok(ConnectionHandler),
+        }
     }
 
     fn handle_established_outbound_connection(
@@ -194,28 +212,6 @@ impl<TSpec: EthSpec> PeerManager<TSpec> {
             metrics::check_nat();
         }
 
-        // Check to make sure the peer is not supposed to be banned
-        match self.ban_status(&peer_id) {
-            // TODO: directly emit the ban event?
-            BanResult::BadScore => {
-                // This is a faulty state
-                error!(self.log, "Connected to a banned peer. Re-banning"; "peer_id" => %peer_id);
-                // Disconnect the peer.
-                self.goodbye_peer(&peer_id, GoodbyeReason::Banned, ReportSource::PeerManager);
-                // Re-ban the peer to prevent repeated errors.
-                self.events.push(PeerManagerEvent::Banned(peer_id, vec![]));
-                return;
-            }
-            BanResult::BannedIp(ip_addr) => {
-                // A good peer has connected to us via a banned IP address. We ban the peer and
-                // prevent future connections.
-                debug!(self.log, "Peer connected via banned IP. Banning"; "peer_id" => %peer_id, "banned_ip" => %ip_addr);
-                self.goodbye_peer(&peer_id, GoodbyeReason::BannedIP, ReportSource::PeerManager);
-                return;
-            }
-            BanResult::NotBanned => {}
-        }
-
         // Count dialing peers in the limit if the peer dialed us.
         let count_dialing = endpoint.is_listener();
         // Check the connection limits

beacon_node/lighthouse_network/src/peer_manager/peerdb.rs

@@ -8,10 +8,13 @@ use peer_info::{ConnectionDirection, PeerConnectionStatus, PeerInfo};
 use rand::seq::SliceRandom;
 use score::{PeerAction, ReportSource, Score, ScoreState};
 use slog::{crit, debug, error, trace, warn};
-use std::cmp::Ordering;
-use std::collections::{HashMap, HashSet};
 use std::net::{IpAddr, SocketAddr};
 use std::time::Instant;
+use std::{cmp::Ordering, fmt::Display};
+use std::{
+    collections::{HashMap, HashSet},
+    fmt::Formatter,
+};
 use sync_status::SyncStatus;
 use types::EthSpec;
 
@@ -141,25 +144,19 @@ impl<TSpec: EthSpec> PeerDB<TSpec> {
         }
     }
 
-    /// Returns the current [`BanResult`] of the peer. This doesn't check the connection state, rather the
+    /// Returns the current [`BanResult`] of the peer if banned. This doesn't check the connection state, rather the
     /// underlying score of the peer. A peer may be banned but still in the connected state
     /// temporarily.
     ///
     /// This is used to determine if we should accept incoming connections or not.
-    pub fn ban_status(&self, peer_id: &PeerId) -> BanResult {
+    pub fn ban_status(&self, peer_id: &PeerId) -> Option<BanResult> {
         if let Some(peer) = self.peers.get(peer_id) {
             match peer.score_state() {
-                ScoreState::Banned => BanResult::BadScore,
-                _ => {
-                    if let Some(ip) = self.ip_is_banned(peer) {
-                        BanResult::BannedIp(ip)
-                    } else {
-                        BanResult::NotBanned
-                    }
-                }
+                ScoreState::Banned => Some(BanResult::BadScore),
+                _ => self.ip_is_banned(peer).map(BanResult::BannedIp),
             }
         } else {
-            BanResult::NotBanned
+            None
         }
     }
 
@@ -1206,23 +1203,25 @@ pub enum BanOperation {
 }
 
 /// When checking if a peer is banned, it can be banned for multiple reasons.
+#[derive(Copy, Clone, Debug)]
 pub enum BanResult {
     /// The peer's score is too low causing it to be banned.
     BadScore,
     /// The peer should be banned because it is connecting from a banned IP address.
     BannedIp(IpAddr),
-    /// The peer is not banned.
-    NotBanned,
 }
 
-// Helper function for unit tests
-#[cfg(test)]
-impl BanResult {
-    pub fn is_banned(&self) -> bool {
-        !matches!(self, BanResult::NotBanned)
+impl Display for BanResult {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        match self {
+            BanResult::BadScore => write!(f, "Peer has a bad score"),
+            BanResult::BannedIp(addr) => write!(f, "Peer address: {} is banned", addr),
+        }
     }
 }
 
+impl std::error::Error for BanResult {}
+
 #[derive(Default)]
 pub struct BannedPeersCount {
     /// The number of banned peers in the database.
@@ -1874,11 +1873,11 @@ mod tests {
         }
 
         //check that ip1 and ip2 are banned but ip3-5 not
-        assert!(pdb.ban_status(&p1).is_banned());
-        assert!(pdb.ban_status(&p2).is_banned());
-        assert!(!pdb.ban_status(&p3).is_banned());
-        assert!(!pdb.ban_status(&p4).is_banned());
-        assert!(!pdb.ban_status(&p5).is_banned());
+        assert!(pdb.ban_status(&p1).is_some());
+        assert!(pdb.ban_status(&p2).is_some());
+        assert!(pdb.ban_status(&p3).is_none());
+        assert!(pdb.ban_status(&p4).is_none());
+        assert!(pdb.ban_status(&p5).is_none());
 
         //ban also the last peer in peers
         let _ = pdb.report_peer(
@@ -1890,35 +1889,35 @@ mod tests {
         pdb.inject_disconnect(&peers[BANNED_PEERS_PER_IP_THRESHOLD + 1]);
 
         //check that ip1-ip4 are banned but ip5 not
-        assert!(pdb.ban_status(&p1).is_banned());
-        assert!(pdb.ban_status(&p2).is_banned());
-        assert!(pdb.ban_status(&p3).is_banned());
-        assert!(pdb.ban_status(&p4).is_banned());
-        assert!(!pdb.ban_status(&p5).is_banned());
+        assert!(pdb.ban_status(&p1).is_some());
+        assert!(pdb.ban_status(&p2).is_some());
+        assert!(pdb.ban_status(&p3).is_some());
+        assert!(pdb.ban_status(&p4).is_some());
+        assert!(pdb.ban_status(&p5).is_none());
 
         //peers[0] gets unbanned
         reset_score(&mut pdb, &peers[0]);
         pdb.update_connection_state(&peers[0], NewConnectionState::Unbanned);
         let _ = pdb.shrink_to_fit();
 
         //nothing changed
-        assert!(pdb.ban_status(&p1).is_banned());
-        assert!(pdb.ban_status(&p2).is_banned());
-        assert!(pdb.ban_status(&p3).is_banned());
-        assert!(pdb.ban_status(&p4).is_banned());
-        assert!(!pdb.ban_status(&p5).is_banned());
+        assert!(pdb.ban_status(&p1).is_some());
+        assert!(pdb.ban_status(&p2).is_some());
+        assert!(pdb.ban_status(&p3).is_some());
+        assert!(pdb.ban_status(&p4).is_some());
+        assert!(pdb.ban_status(&p5).is_none());
 
         //peers[1] gets unbanned
         reset_score(&mut pdb, &peers[1]);
         pdb.update_connection_state(&peers[1], NewConnectionState::Unbanned);
         let _ = pdb.shrink_to_fit();
 
         //all ips are unbanned
-        assert!(!pdb.ban_status(&p1).is_banned());
-        assert!(!pdb.ban_status(&p2).is_banned());
-        assert!(!pdb.ban_status(&p3).is_banned());
-        assert!(!pdb.ban_status(&p4).is_banned());
-        assert!(!pdb.ban_status(&p5).is_banned());
+        assert!(pdb.ban_status(&p1).is_none());
+        assert!(pdb.ban_status(&p2).is_none());
+        assert!(pdb.ban_status(&p3).is_none());
+        assert!(pdb.ban_status(&p4).is_none());
+        assert!(pdb.ban_status(&p5).is_none());
     }
 
     #[test]
@@ -1943,17 +1942,17 @@ mod tests {
         }
 
         // check ip is banned
-        assert!(pdb.ban_status(&p1).is_banned());
-        assert!(!pdb.ban_status(&p2).is_banned());
+        assert!(pdb.ban_status(&p1).is_some());
+        assert!(pdb.ban_status(&p2).is_none());
 
         // unban a peer
         reset_score(&mut pdb, &peers[0]);
         pdb.update_connection_state(&peers[0], NewConnectionState::Unbanned);
         let _ = pdb.shrink_to_fit();
 
         // check not banned anymore
-        assert!(!pdb.ban_status(&p1).is_banned());
-        assert!(!pdb.ban_status(&p2).is_banned());
+        assert!(pdb.ban_status(&p1).is_none());
+        assert!(pdb.ban_status(&p2).is_none());
 
         // unban all peers
         for p in &peers {
@@ -1972,8 +1971,8 @@ mod tests {
         }
 
         // both IP's are now banned
-        assert!(pdb.ban_status(&p1).is_banned());
-        assert!(pdb.ban_status(&p2).is_banned());
+        assert!(pdb.ban_status(&p1).is_some());
+        assert!(pdb.ban_status(&p2).is_some());
 
         // unban all peers
         for p in &peers {
@@ -1989,16 +1988,16 @@ mod tests {
         }
 
         // nothing is banned
-        assert!(!pdb.ban_status(&p1).is_banned());
-        assert!(!pdb.ban_status(&p2).is_banned());
+        assert!(pdb.ban_status(&p1).is_none());
+        assert!(pdb.ban_status(&p2).is_none());
 
         // reban last peer
         let _ = pdb.report_peer(&peers[0], PeerAction::Fatal, ReportSource::PeerManager, "");
         pdb.inject_disconnect(&peers[0]);
 
         //Ip's are banned again
-        assert!(pdb.ban_status(&p1).is_banned());
-        assert!(pdb.ban_status(&p2).is_banned());
+        assert!(pdb.ban_status(&p1).is_some());
+        assert!(pdb.ban_status(&p2).is_some());
     }
 
     #[test]

beacon_node/lighthouse_network/src/service/behaviour.rs

@@ -20,8 +20,6 @@ where
     AppReqId: ReqId,
     TSpec: EthSpec,
 {
-    /// Peers banned.
-    pub banned_peers: libp2p::allow_block_list::Behaviour<libp2p::allow_block_list::BlockedPeers>,
     /// Keep track of active and pending connections to enforce hard limits.
     pub connection_limits: libp2p::connection_limits::Behaviour,
     /// The routing pub-sub mechanism for eth2.

beacon_node/lighthouse_network/src/service/mod.rs

@@ -339,11 +339,8 @@ impl<AppReqId: ReqId, TSpec: EthSpec> Network<AppReqId, TSpec> {
             libp2p::connection_limits::Behaviour::new(limits)
         };
 
-        let banned_peers = libp2p::allow_block_list::Behaviour::default();
-
         let behaviour = {
             Behaviour {
-                banned_peers,
                 gossipsub,
                 eth2_rpc,
                 discovery,
@@ -1401,15 +1398,10 @@ impl<AppReqId: ReqId, TSpec: EthSpec> Network<AppReqId, TSpec> {
                 Some(NetworkEvent::PeerDisconnected(peer_id))
             }
             PeerManagerEvent::Banned(peer_id, associated_ips) => {
-                self.swarm.behaviour_mut().banned_peers.block_peer(peer_id);
                 self.discovery_mut().ban_peer(&peer_id, associated_ips);
                 None
             }
             PeerManagerEvent::UnBanned(peer_id, associated_ips) => {
-                self.swarm
-                    .behaviour_mut()
-                    .banned_peers
-                    .unblock_peer(peer_id);
                 self.discovery_mut().unban_peer(&peer_id, associated_ips);
                 None
             }
@@ -1458,7 +1450,6 @@ impl<AppReqId: ReqId, TSpec: EthSpec> Network<AppReqId, TSpec> {
             let maybe_event = match swarm_event {
                 SwarmEvent::Behaviour(behaviour_event) => match behaviour_event {
                     // Handle sub-behaviour events.
-                    BehaviourEvent::BannedPeers(void) => void::unreachable(void),
                     BehaviourEvent::Gossipsub(ge) => self.inject_gs_event(ge),
                     BehaviourEvent::Eth2Rpc(re) => self.inject_rpc_event(re),
                     BehaviourEvent::Discovery(de) => self.inject_discovery_event(de),