diff --git a/beacon_node/lighthouse_network/src/discovery/mod.rs b/beacon_node/lighthouse_network/src/discovery/mod.rs index 82a371d8a20..bbbbfaca6c9 100644 --- a/beacon_node/lighthouse_network/src/discovery/mod.rs +++ b/beacon_node/lighthouse_network/src/discovery/mod.rs @@ -550,8 +550,6 @@ impl Discovery { 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 { diff --git a/beacon_node/lighthouse_network/src/peer_manager/mod.rs b/beacon_node/lighthouse_network/src/peer_manager/mod.rs index 4f3454f4033..540f7851cf3 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/mod.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/mod.rs @@ -414,7 +414,7 @@ impl PeerManager { /// 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 { self.network_globals.peers.read().ban_status(peer_id) } @@ -802,7 +802,7 @@ impl PeerManager { ) -> 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); } diff --git a/beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs b/beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs index 70f421681a3..f88839694f4 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs +++ b/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 NetworkBehaviour for PeerManager { 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::ConnectionDenied> { - // TODO: we might want to check if we accept this peer or not in the future. - Ok(ConnectionHandler) + ) -> Result, 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 PeerManager { 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 diff --git a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs b/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs index 52f0bbd9dfc..789c7773a06 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs +++ b/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 PeerDB { } } - /// 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 { 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,11 +1889,11 @@ 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]); @@ -1902,11 +1901,11 @@ mod tests { 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]); @@ -1914,11 +1913,11 @@ mod tests { 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,8 +1942,8 @@ 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]); @@ -1952,8 +1951,8 @@ mod tests { 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] diff --git a/beacon_node/lighthouse_network/src/service/behaviour.rs b/beacon_node/lighthouse_network/src/service/behaviour.rs index 6c52a07c14a..8dd750429c3 100644 --- a/beacon_node/lighthouse_network/src/service/behaviour.rs +++ b/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, /// 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. diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index 63e5bcbff66..ae4015724e6 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -339,11 +339,8 @@ impl Network { 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 Network { 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 Network { 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),