From a73dcb7b6d2168e8c1eb1c86ab3097ab0fe32f82 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Fri, 17 Sep 2021 04:02:31 +0000 Subject: [PATCH] Improved handling of IP Banning (#2530) This PR in general improves the handling around peer banning. Specifically there were issues when multiple peers under a single IP connected to us after we banned the IP for poor behaviour. This PR should now handle these peers gracefully as well as make some improvements around how we previously disconnected and banned peers. The logic now goes as follows: - Once a peer gets banned, its gets registered with its known IP addresses - Once enough banned peers exist under a single IP that IP is banned - We retain connections with existing peers under this IP - Any new connections under this IP are rejected --- .../eth2_libp2p/src/peer_manager/mod.rs | 200 ++++++------ .../eth2_libp2p/src/peer_manager/peer_info.rs | 28 +- .../eth2_libp2p/src/peer_manager/peerdb.rs | 293 ++++++++++++++---- .../eth2_libp2p/src/peer_manager/score.rs | 2 + beacon_node/eth2_libp2p/src/rpc/methods.rs | 5 + 5 files changed, 354 insertions(+), 174 deletions(-) diff --git a/beacon_node/eth2_libp2p/src/peer_manager/mod.rs b/beacon_node/eth2_libp2p/src/peer_manager/mod.rs index 7b3a85908fd..12403b578e2 100644 --- a/beacon_node/eth2_libp2p/src/peer_manager/mod.rs +++ b/beacon_node/eth2_libp2p/src/peer_manager/mod.rs @@ -171,17 +171,8 @@ impl PeerManager { ); } - // Update the peerdb and peer state accordingly - if self - .network_globals - .peers - .write() - .disconnect_and_ban(peer_id) - { - // update the state of the peer. - self.events - .push(PeerManagerEvent::DisconnectPeer(*peer_id, reason)); - } + // Update the peerdb and start the disconnection. + self.ban_peer(peer_id, reason); } /// Reports a peer for some action. @@ -333,18 +324,23 @@ impl PeerManager { } } - // Should not be able to connect to a banned peer. Double check here - if self.is_banned(&peer_id) { - warn!(self.log, "Connected to a banned peer"; "peer_id" => %peer_id); - self.events.push(PeerManagerEvent::DisconnectPeer( - peer_id, - GoodbyeReason::Banned, - )); - self.network_globals - .peers - .write() - .notify_disconnecting(peer_id, true); - return; + // Check to make sure the peer is not supposed to be banned + match self.ban_status(&peer_id) { + BanResult::BadScore => { + // This is a faulty state + error!(self.log, "Connected to a banned peer, re-banning"; "peer_id" => %peer_id); + // Reban the peer + self.goodbye_peer(&peer_id, GoodbyeReason::Banned, ReportSource::PeerManager); + 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 => {} } // Check the connection limits @@ -356,14 +352,8 @@ impl PeerManager { .peer_info(&peer_id) .map_or(true, |peer| !peer.has_future_duty()) { - self.events.push(PeerManagerEvent::DisconnectPeer( - peer_id, - GoodbyeReason::TooManyPeers, - )); - self.network_globals - .peers - .write() - .notify_disconnecting(peer_id, false); + // Gracefully disconnect the peer. + self.disconnect_peer(peer_id, GoodbyeReason::TooManyPeers); return; } @@ -465,8 +455,8 @@ impl PeerManager { /// Reports if a peer is banned or not. /// /// This is used to determine if we should accept incoming connections. - pub fn is_banned(&self, peer_id: &PeerId) -> bool { - self.network_globals.peers.read().is_banned(peer_id) + pub fn ban_status(&self, peer_id: &PeerId) -> BanResult { + self.network_globals.peers.read().ban_status(peer_id) } pub fn is_connected(&self, peer_id: &PeerId) -> bool { @@ -707,7 +697,7 @@ impl PeerManager { let mut to_unban_peers = Vec::new(); { - //collect peers with scores + // collect peers with scores let mut guard = self.network_globals.peers.write(); let mut peers: Vec<_> = guard .peers_mut() @@ -793,10 +783,11 @@ impl PeerManager { .write() .inject_disconnect(peer_id) { - self.ban_peer(peer_id); + // The peer was awaiting a ban, continue to ban the peer. + self.ban_peer(peer_id, GoodbyeReason::BadScore); } - // remove the ping and status timer for the peer + // Remove the ping and status timer for the peer self.inbound_ping_peers.remove(peer_id); self.outbound_ping_peers.remove(peer_id); self.status_peers.remove(peer_id); @@ -816,7 +807,7 @@ impl PeerManager { ) -> bool { { let mut peerdb = self.network_globals.peers.write(); - if peerdb.is_banned(peer_id) { + if !matches!(peerdb.ban_status(peer_id), BanResult::NotBanned) { // don't connect if the peer is banned slog::crit!(self.log, "Connection has been allowed to a banned peer"; "peer_id" => %peer_id); } @@ -878,34 +869,37 @@ impl PeerManager { events: &mut SmallVec<[PeerManagerEvent; 16]>, log: &slog::Logger, ) { - if previous_state != info.score_state() { - match info.score_state() { - ScoreState::Banned => { - debug!(log, "Peer has been banned"; "peer_id" => %peer_id, "score" => %info.score()); - to_ban_peers.push(*peer_id); - } - ScoreState::Disconnected => { - debug!(log, "Peer transitioned to disconnect state"; "peer_id" => %peer_id, "score" => %info.score(), "past_state" => %previous_state); - // disconnect the peer if it's currently connected or dialing - if info.is_connected_or_dialing() { - // Change the state to inform that we are disconnecting the peer. - info.disconnecting(false); - events.push(PeerManagerEvent::DisconnectPeer( - *peer_id, - GoodbyeReason::BadScore, - )); - } else if info.is_banned() { - to_unban_peers.push(*peer_id); - } - } - ScoreState::Healthy => { - debug!(log, "Peer transitioned to healthy state"; "peer_id" => %peer_id, "score" => %info.score(), "past_state" => %previous_state); - // unban the peer if it was previously banned. - if info.is_banned() { - to_unban_peers.push(*peer_id); - } + match (info.score_state(), previous_state) { + (ScoreState::Banned, ScoreState::Healthy | ScoreState::Disconnected) => { + debug!(log, "Peer has been banned"; "peer_id" => %peer_id, "score" => %info.score()); + to_ban_peers.push(*peer_id); + } + (ScoreState::Disconnected, ScoreState::Banned | ScoreState::Healthy) => { + debug!(log, "Peer transitioned to disconnect state"; "peer_id" => %peer_id, "score" => %info.score(), "past_state" => %previous_state); + // disconnect the peer if it's currently connected or dialing + if info.is_connected_or_dialing() { + // Change the state to inform that we are disconnecting the peer. + info.disconnecting(false); + events.push(PeerManagerEvent::DisconnectPeer( + *peer_id, + GoodbyeReason::BadScore, + )); + } else if previous_state == ScoreState::Banned { + to_unban_peers.push(*peer_id); } } + (ScoreState::Healthy, ScoreState::Disconnected) => { + debug!(log, "Peer transitioned to healthy state"; "peer_id" => %peer_id, "score" => %info.score(), "past_state" => %previous_state); + } + (ScoreState::Healthy, ScoreState::Banned) => { + debug!(log, "Peer transitioned to healthy state"; "peer_id" => %peer_id, "score" => %info.score(), "past_state" => %previous_state); + // unban the peer if it was previously banned. + to_unban_peers.push(*peer_id); + } + // Explicitly ignore states that haven't transitioned. + (ScoreState::Healthy, ScoreState::Healthy) => {} + (ScoreState::Disconnected, ScoreState::Disconnected) => {} + (ScoreState::Banned, ScoreState::Banned) => {} } } @@ -913,7 +907,7 @@ impl PeerManager { fn ban_and_unban_peers(&mut self, to_ban_peers: Vec, to_unban_peers: Vec) { // process banning peers for peer_id in to_ban_peers { - self.ban_peer(&peer_id); + self.ban_peer(&peer_id, GoodbyeReason::BadScore); } // process unbanning peers for peer_id in to_unban_peers { @@ -953,40 +947,49 @@ impl PeerManager { /// /// Records updates the peers connection status and updates the peer db as well as blocks the /// peer from participating in discovery and removes them from the routing table. - fn ban_peer(&mut self, peer_id: &PeerId) { - { - // write lock scope - let mut peer_db = self.network_globals.peers.write(); + fn ban_peer(&mut self, peer_id: &PeerId, reason: GoodbyeReason) { + // NOTE: When we ban a peer, its IP address can be banned. We do not recursively search + // through all our connected peers banning all other peers that are using this IP address. + // If these peers are behaving fine, we permit their current connections. However, if any new + // nodes or current nodes try to reconnect on a banned IP, they will be instantly banned + // and disconnected. + + let mut peer_db = self.network_globals.peers.write(); - if peer_db.disconnect_and_ban(peer_id) { + match peer_db.disconnect_and_ban(peer_id) { + BanOperation::DisconnectThePeer => { // The peer was currently connected, so we start a disconnection. - self.events.push(PeerManagerEvent::DisconnectPeer( - *peer_id, - GoodbyeReason::BadScore, - )); + // Once the peer has disconnected, this function will be called to again to ban + // at the swarm level. + self.events + .push(PeerManagerEvent::DisconnectPeer(*peer_id, reason)); } - } // end write lock - - // take a read lock - let peer_db = self.network_globals.peers.read(); - - let banned_ip_addresses = peer_db - .peer_info(peer_id) - .map(|info| { - info.seen_addresses() - .filter(|ip| peer_db.is_ip_banned(ip)) - .collect::>() - }) - .unwrap_or_default(); + BanOperation::PeerDisconnecting => { + // The peer is currently being disconnected and will be banned once the + // disconnection completes. + } + BanOperation::ReadyToBan => { + // The peer is not currently connected, we can safely ban it at the swarm + // level. + let banned_ip_addresses = peer_db + .peer_info(peer_id) + .map(|info| { + info.seen_addresses() + .filter(|ip| peer_db.is_ip_banned(ip)) + .collect::>() + }) + .unwrap_or_default(); - // Inform the Swarm to ban the peer - self.events - .push(PeerManagerEvent::Banned(*peer_id, banned_ip_addresses)); + // Inform the Swarm to ban the peer + self.events + .push(PeerManagerEvent::Banned(*peer_id, banned_ip_addresses)); + } + } } /// Unbans a peer. /// - /// Records updates the peers connection status and updates the peer db as well as removes + /// Updates the peer's connection status and updates the peer db as well as removes /// previous bans from discovery. fn unban_peer(&mut self, peer_id: &PeerId) -> Result<(), &'static str> { let mut peer_db = self.network_globals.peers.write(); @@ -1003,6 +1006,16 @@ impl PeerManager { Ok(()) } + // Gracefully disconnects a peer without banning them. + fn disconnect_peer(&mut self, peer_id: PeerId, reason: GoodbyeReason) { + self.events + .push(PeerManagerEvent::DisconnectPeer(peer_id, reason)); + self.network_globals + .peers + .write() + .notify_disconnecting(peer_id, false); + } + /// Run discovery query for additional sync committee peers if we fall below `TARGET_PEERS`. fn maintain_sync_committee_peers(&mut self) { // Remove expired entries @@ -1101,13 +1114,8 @@ impl PeerManager { } } - let mut peer_db = self.network_globals.peers.write(); for peer_id in disconnecting_peers { - peer_db.notify_disconnecting(peer_id, false); - self.events.push(PeerManagerEvent::DisconnectPeer( - peer_id, - GoodbyeReason::TooManyPeers, - )); + self.disconnect_peer(peer_id, GoodbyeReason::TooManyPeers); } } } diff --git a/beacon_node/eth2_libp2p/src/peer_manager/peer_info.rs b/beacon_node/eth2_libp2p/src/peer_manager/peer_info.rs index 717782901de..8d3912041bf 100644 --- a/beacon_node/eth2_libp2p/src/peer_manager/peer_info.rs +++ b/beacon_node/eth2_libp2p/src/peer_manager/peer_info.rs @@ -183,7 +183,7 @@ impl PeerInfo { /// Checks if the status is banned. pub fn is_banned(&self) -> bool { - matches!(self.connection_status, PeerConnectionStatus::Banned { .. }) + matches!(self.score.state(), ScoreState::Banned) } /// Checks if the status is disconnected. @@ -208,7 +208,7 @@ impl PeerInfo { /// Modifies the status to Disconnected and sets the last seen instant to now. Returns None if /// no changes were made. Returns Some(bool) where the bool represents if peer is to now be - /// baned + /// banned. pub fn notify_disconnect(&mut self) -> Option { match self.connection_status { Banned { .. } | Disconnected { .. } => None, @@ -233,17 +233,18 @@ impl PeerInfo { self.connection_status = Disconnecting { to_ban } } - /// Modifies the status to Banned - pub fn ban(&mut self) { - self.connection_status = Banned { - since: Instant::now(), - }; - } - - /// The score system has unbanned the peer. Update the connection status - pub fn unban(&mut self) { - if let PeerConnectionStatus::Banned { since, .. } = self.connection_status { - self.connection_status = PeerConnectionStatus::Disconnected { since }; + /// Modifies the status to banned or unbanned based on the underlying score. + pub fn update_state(&mut self) { + match (&self.connection_status, self.score.state()) { + (Disconnected { .. } | Unknown, ScoreState::Banned) => { + self.connection_status = Banned { + since: Instant::now(), + } + } + (Banned { since }, ScoreState::Healthy | ScoreState::Disconnected) => { + self.connection_status = Disconnected { since: *since } + } + (_, _) => {} } } @@ -358,7 +359,6 @@ pub enum PeerConnectionStatus { /// last time the peer was connected or discovered. since: Instant, }, - /// The peer has been banned and is disconnected. Banned { /// moment when the peer was banned. diff --git a/beacon_node/eth2_libp2p/src/peer_manager/peerdb.rs b/beacon_node/eth2_libp2p/src/peer_manager/peerdb.rs index 691600dd447..7506b94ebae 100644 --- a/beacon_node/eth2_libp2p/src/peer_manager/peerdb.rs +++ b/beacon_node/eth2_libp2p/src/peer_manager/peerdb.rs @@ -34,6 +34,35 @@ pub struct PeerDB { log: slog::Logger, } +/// When attempting to ban a peer provides the peer manager with the operation that must be taken. +pub enum BanOperation { + // The peer is currently connected. Perform a graceful disconnect before banning at the swarm + // level. + DisconnectThePeer, + // The peer is disconnected, it has now been banned and can be banned at the swarm level. + ReadyToBan, + // The peer is currently being disconnected, nothing to do. + PeerDisconnecting, +} + +/// When checking if a peer is banned, it can be banned for multiple reasons. +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) + } +} + pub struct BannedPeersCount { /// The number of banned peers in the database. banned_peers: usize, @@ -176,25 +205,32 @@ impl PeerDB { } } - /// Returns true if the Peer is banned. This doesn't check the connection state, rather the + /// Returns the current [`BanResult`] of the peer. 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 is_banned(&self, peer_id: &PeerId) -> bool { + pub fn ban_status(&self, peer_id: &PeerId) -> BanResult { if let Some(peer) = self.peers.get(peer_id) { match peer.score_state() { - ScoreState::Banned => true, - _ => self.ip_is_banned(peer), + ScoreState::Banned => BanResult::BadScore, + _ => { + if let Some(ip) = self.ip_is_banned(peer) { + BanResult::BannedIp(ip) + } else { + BanResult::NotBanned + } + } } } else { - false + BanResult::NotBanned } } - fn ip_is_banned(&self, peer: &PeerInfo) -> bool { + /// Checks if the peer's known addresses are currently banned. + fn ip_is_banned(&self, peer: &PeerInfo) -> Option { peer.seen_addresses() - .any(|ip| self.banned_peers_count.ip_is_banned(&ip)) + .find(|ip| self.banned_peers_count.ip_is_banned(ip)) } /// Returns true if the IP is banned. @@ -203,11 +239,11 @@ impl PeerDB { } /// Returns true if the Peer is either banned or in the disconnected state. - pub fn is_banned_or_disconnected(&self, peer_id: &PeerId) -> bool { + fn is_banned_or_disconnected(&self, peer_id: &PeerId) -> bool { if let Some(peer) = self.peers.get(peer_id) { match peer.score_state() { ScoreState::Banned | ScoreState::Disconnected => true, - _ => self.ip_is_banned(peer), + _ => self.ip_is_banned(peer).is_some(), } } else { false @@ -489,7 +525,7 @@ impl PeerDB { /// Returns true if the peer is currently connected and false otherwise. // NOTE: If the peer's score is not already low enough to be banned, this will decrease the // peer's score to be a banned state. - pub fn disconnect_and_ban(&mut self, peer_id: &PeerId) -> bool { + pub fn disconnect_and_ban(&mut self, peer_id: &PeerId) -> BanOperation { let log_ref = &self.log; let info = self.peers.entry(*peer_id).or_insert_with(|| { warn!(log_ref, "Banning unknown peer"; @@ -513,43 +549,47 @@ impl PeerDB { // It is possible to ban a peer that has a disconnected score, if there are many // events that score it poorly and are processed after it has disconnected. self.disconnected_peers = self.disconnected_peers.saturating_sub(1); - info.ban(); + info.update_state(); self.banned_peers_count .add_banned_peer(info.seen_addresses()); self.shrink_to_fit(); - false + BanOperation::ReadyToBan } PeerConnectionStatus::Disconnecting { .. } => { // NOTE: This can occur due a rapid downscore of a peer. It goes through the // disconnection phase and straight into banning in a short time-frame. debug!(log_ref, "Banning peer that is currently disconnecting"; "peer_id" => %peer_id); info.disconnecting(true); - false + BanOperation::PeerDisconnecting } PeerConnectionStatus::Banned { .. } => { error!(log_ref, "Banning already banned peer"; "peer_id" => %peer_id); - false + BanOperation::ReadyToBan } PeerConnectionStatus::Connected { .. } | PeerConnectionStatus::Dialing { .. } => { // update the state info.disconnecting(true); - true + BanOperation::DisconnectThePeer } PeerConnectionStatus::Unknown => { // shift the peer straight to banned warn!(log_ref, "Banning a peer of unknown connection state"; "peer_id" => %peer_id); self.banned_peers_count .add_banned_peer(info.seen_addresses()); - info.ban(); + info.update_state(); self.shrink_to_fit(); - false + BanOperation::ReadyToBan } } } /// Unbans a peer. + /// /// This should only be called once a peer's score is no longer banned. /// If this is called for a banned peer, it will error. + /// + /// This updates the connection state of the peer and updates the number of banned peers in the + /// peerdb. pub fn unban(&mut self, peer_id: &PeerId) -> Result<(), &'static str> { let log_ref = &self.log; let info = self.peers.entry(*peer_id).or_insert_with(|| { @@ -558,17 +598,16 @@ impl PeerDB { PeerInfo::default() }); - if !info.is_banned() { - return Err("Unbanning peer that is not banned"); - } - if let ScoreState::Banned = info.score_state() { return Err("Attempted to unban (connection status) a banned peer"); } self.banned_peers_count .remove_banned_peer(info.seen_addresses()); - info.unban(); + + // Update the connection state + info.update_state(); + // This transitions a banned peer to a disconnected peer self.disconnected_peers = self.disconnected_peers.saturating_add(1); self.shrink_to_fit(); @@ -852,43 +891,84 @@ mod tests { println!("{}", random_peer2); println!("{}", random_peer3); + // All 4 peers connected on the same IP pdb.connect_ingoing(&random_peer, multiaddr.clone(), None); pdb.connect_ingoing(&random_peer1, multiaddr.clone(), None); pdb.connect_ingoing(&random_peer2, multiaddr.clone(), None); pdb.connect_ingoing(&random_peer3, multiaddr.clone(), None); + + // Should be no disconnected or banned peers assert_eq!(pdb.disconnected_peers, pdb.disconnected_peers().count()); assert_eq!( pdb.banned_peers_count.banned_peers(), pdb.banned_peers().count() ); - println!("1:{}", pdb.disconnected_peers); - - pdb.connect_ingoing(&random_peer, multiaddr.clone(), None); + // Should be no disconnected peers + println!( + "1:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + // Disconnect one peer pdb.inject_disconnect(&random_peer1); - println!("2:{}", pdb.disconnected_peers); + // Should be 1 disconnected peer + println!( + "2:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + // Disconnect and ban peer 2 pdb.disconnect_and_ban(&random_peer2); - println!("3:{}", pdb.disconnected_peers); + // Should be 1 disconnected peer and one peer in the process of being disconnected + println!( + "3:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + // The peer is now disconnected and banned pdb.inject_disconnect(&random_peer2); - println!("4:{}", pdb.disconnected_peers); + // There should be 2 disconnected peers. + println!( + "4:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + // Now that the peer is disconnected, register the ban. pdb.disconnect_and_ban(&random_peer2); - println!("5:{}", pdb.disconnected_peers); + // There should be 1 disconnected peer and one banned peer. + println!( + "5:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + // Re-connect peer3, should have no effect pdb.connect_ingoing(&random_peer3, multiaddr.clone(), None); assert_eq!(pdb.disconnected_peers, pdb.disconnected_peers().count()); assert_eq!( pdb.banned_peers_count.banned_peers(), pdb.banned_peers().count() ); + // Now ban peer 1. pdb.disconnect_and_ban(&random_peer1); - println!("6:{}", pdb.disconnected_peers); + // There should be no disconnected peers and 2 banned peers + println!( + "6:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + // This should have no effect pdb.inject_disconnect(&random_peer1); - println!("7:{}", pdb.disconnected_peers); + // Should still be no disconnected peers and 2 banned peers + println!( + "7:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + // Same thing here. pdb.disconnect_and_ban(&random_peer1); - println!("8:{}", pdb.disconnected_peers); println!( - "{}, {:?}", + "8:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + println!( + "{}, {:?}, {}", pdb.disconnected_peers, - pdb.disconnected_peers().collect::>() + pdb.disconnected_peers().collect::>(), + pdb.banned_peers_count.banned_peers ); assert_eq!(pdb.disconnected_peers, pdb.disconnected_peers().count()); assert_eq!( @@ -896,50 +976,135 @@ mod tests { pdb.banned_peers().count() ); + // Try and reconnect banned peer 2. pdb.connect_outgoing(&random_peer2, multiaddr.clone(), None); + pdb.peer_info_mut(&random_peer2) + .unwrap() + .add_to_score(100.0); + // This removes the banned peer and should give us 0 disconnected, 1 banned peer + // (peer1) + println!( + "9:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + assert_eq!(pdb.disconnected_peers, pdb.disconnected_peers().count()); assert_eq!( pdb.banned_peers_count.banned_peers(), pdb.banned_peers().count() ); + + // Ban peer 3 pdb.disconnect_and_ban(&random_peer3); pdb.inject_disconnect(&random_peer3); pdb.disconnect_and_ban(&random_peer3); + + // This should add a new banned peer, there should be 0 disconnected and 2 banned + // peers (peer1 and peer3) + println!( + "10:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + assert_eq!(pdb.disconnected_peers, pdb.disconnected_peers().count()); assert_eq!( pdb.banned_peers_count.banned_peers(), pdb.banned_peers().count() ); + // Ban peer 3 pdb.disconnect_and_ban(&random_peer3); pdb.inject_disconnect(&random_peer3); pdb.disconnect_and_ban(&random_peer3); + + // Should still have 2 banned peers + println!( + "11:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + + // Unban peer 1 pdb.connect_ingoing(&random_peer1, multiaddr.clone(), None); + pdb.peer_info_mut(&random_peer1) + .unwrap() + .add_to_score(100.0); + // Should have 1 banned peer (peer 3) + println!( + "12:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + + // Disconnect peer 2 pdb.inject_disconnect(&random_peer2); + + // Should have 1 disconnect (peer 2) and one banned (peer 3) + println!( + "12:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + + // Ban peer 3 pdb.disconnect_and_ban(&random_peer3); pdb.inject_disconnect(&random_peer3); pdb.disconnect_and_ban(&random_peer3); + + // Should have 1 disconnect (peer 2) and one banned (peer 3) + println!( + "13:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); + + // Add peer 0 pdb.connect_ingoing(&random_peer, multiaddr, None); + pdb.peer_info_mut(&random_peer).unwrap().add_to_score(100.0); + + // Should have 1 disconnect (peer 2) and one banned (peer 3) + println!( + "14:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); assert_eq!(pdb.disconnected_peers, pdb.disconnected_peers().count()); assert_eq!( pdb.banned_peers_count.banned_peers(), pdb.banned_peers().count() ); + + // Disconnect peer 0 pdb.inject_disconnect(&random_peer); + // Should have 2 disconnect (peer 0, peer 2) and one banned (peer 3) + println!( + "15:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); assert_eq!(pdb.disconnected_peers, pdb.disconnected_peers().count()); assert_eq!( pdb.banned_peers_count.banned_peers(), pdb.banned_peers().count() ); + // Disconnect peer 0 pdb.inject_disconnect(&random_peer); + // Should have 2 disconnect (peer 0, peer 2) and one banned (peer 3) + println!( + "16:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); assert_eq!(pdb.disconnected_peers, pdb.disconnected_peers().count()); assert_eq!( pdb.banned_peers_count.banned_peers(), pdb.banned_peers().count() ); + + // Ban peer 0 pdb.disconnect_and_ban(&random_peer); pdb.inject_disconnect(&random_peer); + pdb.disconnect_and_ban(&random_peer); + + // Should have 1 disconnect ( peer 2) and two banned (peer0, peer 3) + println!( + "17:{},{}", + pdb.disconnected_peers, pdb.banned_peers_count.banned_peers + ); assert_eq!(pdb.disconnected_peers, pdb.disconnected_peers().count()); } @@ -990,11 +1155,11 @@ mod tests { } //check that ip1 and ip2 are banned but ip3-5 not - assert!(pdb.is_banned(&p1)); - assert!(pdb.is_banned(&p2)); - assert!(!pdb.is_banned(&p3)); - assert!(!pdb.is_banned(&p4)); - assert!(!pdb.is_banned(&p5)); + 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()); //ban also the last peer in peers pdb.disconnect_and_ban(&peers[BANNED_PEERS_PER_IP_THRESHOLD + 1]); @@ -1002,33 +1167,33 @@ mod tests { pdb.disconnect_and_ban(&peers[BANNED_PEERS_PER_IP_THRESHOLD + 1]); //check that ip1-ip4 are banned but ip5 not - assert!(pdb.is_banned(&p1)); - assert!(pdb.is_banned(&p2)); - assert!(pdb.is_banned(&p3)); - assert!(pdb.is_banned(&p4)); - assert!(!pdb.is_banned(&p5)); + 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()); //peers[0] gets unbanned reset_score(&mut pdb, &peers[0]); pdb.unban(&peers[0]).unwrap(); //nothing changed - assert!(pdb.is_banned(&p1)); - assert!(pdb.is_banned(&p2)); - assert!(pdb.is_banned(&p3)); - assert!(pdb.is_banned(&p4)); - assert!(!pdb.is_banned(&p5)); + 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()); //peers[1] gets unbanned reset_score(&mut pdb, &peers[1]); pdb.unban(&peers[1]).unwrap(); //all ips are unbanned - assert!(!pdb.is_banned(&p1)); - assert!(!pdb.is_banned(&p2)); - assert!(!pdb.is_banned(&p3)); - assert!(!pdb.is_banned(&p4)); - assert!(!pdb.is_banned(&p5)); + 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()); } #[test] @@ -1054,16 +1219,16 @@ mod tests { } // check ip is banned - assert!(pdb.is_banned(&p1)); - assert!(!pdb.is_banned(&p2)); + assert!(pdb.ban_status(&p1).is_banned()); + assert!(!pdb.ban_status(&p2).is_banned()); // unban a peer reset_score(&mut pdb, &peers[0]); pdb.unban(&peers[0]).unwrap(); // check not banned anymore - assert!(!pdb.is_banned(&p1)); - assert!(!pdb.is_banned(&p2)); + assert!(!pdb.ban_status(&p1).is_banned()); + assert!(!pdb.ban_status(&p2).is_banned()); // add ip2 to all peers and ban them. let mut socker_addr = Multiaddr::from(ip2); @@ -1076,8 +1241,8 @@ mod tests { } // both IP's are now banned - assert!(pdb.is_banned(&p1)); - assert!(pdb.is_banned(&p2)); + assert!(pdb.ban_status(&p1).is_banned()); + assert!(pdb.ban_status(&p2).is_banned()); // unban all peers for p in &peers { @@ -1093,8 +1258,8 @@ mod tests { } // nothing is banned - assert!(!pdb.is_banned(&p1)); - assert!(!pdb.is_banned(&p2)); + assert!(!pdb.ban_status(&p1).is_banned()); + assert!(!pdb.ban_status(&p2).is_banned()); //reban last peer pdb.disconnect_and_ban(&peers[0]); @@ -1102,8 +1267,8 @@ mod tests { pdb.disconnect_and_ban(&peers[0]); //Ip's are banned again - assert!(pdb.is_banned(&p1)); - assert!(pdb.is_banned(&p2)); + assert!(pdb.ban_status(&p1).is_banned()); + assert!(pdb.ban_status(&p2).is_banned()); } #[test] diff --git a/beacon_node/eth2_libp2p/src/peer_manager/score.rs b/beacon_node/eth2_libp2p/src/peer_manager/score.rs index 8b20192296d..3b67c442d7e 100644 --- a/beacon_node/eth2_libp2p/src/peer_manager/score.rs +++ b/beacon_node/eth2_libp2p/src/peer_manager/score.rs @@ -72,6 +72,7 @@ pub enum ReportSource { RPC, Processor, SyncService, + PeerManager, } impl From for &'static str { @@ -81,6 +82,7 @@ impl From for &'static str { ReportSource::RPC => "rpc_error", ReportSource::Processor => "processor", ReportSource::SyncService => "sync", + ReportSource::PeerManager => "peer_manager", } } } diff --git a/beacon_node/eth2_libp2p/src/rpc/methods.rs b/beacon_node/eth2_libp2p/src/rpc/methods.rs index b2be196474d..09638686b2f 100644 --- a/beacon_node/eth2_libp2p/src/rpc/methods.rs +++ b/beacon_node/eth2_libp2p/src/rpc/methods.rs @@ -141,6 +141,9 @@ pub enum GoodbyeReason { /// The peer is banned Banned = 251, + /// The IP address the peer is using is banned. + BannedIP = 252, + /// Unknown reason. Unknown = 0, } @@ -155,6 +158,7 @@ impl From for GoodbyeReason { 129 => GoodbyeReason::TooManyPeers, 250 => GoodbyeReason::BadScore, 251 => GoodbyeReason::Banned, + 252 => GoodbyeReason::BannedIP, _ => GoodbyeReason::Unknown, } } @@ -396,6 +400,7 @@ impl std::fmt::Display for GoodbyeReason { GoodbyeReason::TooManyPeers => write!(f, "Too many peers"), GoodbyeReason::BadScore => write!(f, "Bad Score"), GoodbyeReason::Banned => write!(f, "Banned"), + GoodbyeReason::BannedIP => write!(f, "BannedIP"), GoodbyeReason::Unknown => write!(f, "Unknown Reason"), } }