diff --git a/beacon_node/eth2_libp2p/src/peer_manager/mod.rs b/beacon_node/eth2_libp2p/src/peer_manager/mod.rs index 7b3a85908f..12403b578e 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. - if peer_db.disconnect_and_ban(peer_id) { + let mut peer_db = self.network_globals.peers.write(); + + 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 + 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(); - // 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(); - - // 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 717782901d..8d3912041b 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 691600dd44..7506b94eba 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); - pdb.inject_disconnect(&random_peer1); - println!("7:{}", pdb.disconnected_peers); - pdb.disconnect_and_ban(&random_peer1); - println!("8:{}", 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); + // 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, 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); - assert_eq!(pdb.disconnected_peers, pdb.disconnected_peers().count()); - assert_eq!( - pdb.banned_peers_count.banned_peers(), - pdb.banned_peers().count() + 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 ); - pdb.disconnect_and_ban(&random_peer3); - pdb.inject_disconnect(&random_peer3); - pdb.disconnect_and_ban(&random_peer3); + 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); - pdb.connect_ingoing(&random_peer, multiaddr, None); - assert_eq!(pdb.disconnected_peers, pdb.disconnected_peers().count()); - assert_eq!( - pdb.banned_peers_count.banned_peers(), - pdb.banned_peers().count() + + // 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 ); - pdb.inject_disconnect(&random_peer); 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 8b20192296..3b67c442d7 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 b2be196474..09638686b2 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"), } }