Ban peer race condition (#4140)

It is possible that when we go to ban a peer, there is already an unbanned message in the queue. It could lead to the case that we ban and immediately unban a peer leaving us in a state where a should-be banned peer is unbanned. 

If this banned peer connects to us in this faulty state, we currently do not attempt to re-ban it. This PR does correct this also, so if we do see this error, it will now self-correct (although we shouldn't see the error in the first place). 

I have also incremented the severity of not supporting protocols as I see peers ultimately get banned in a few steps and it seems to make sense to just ban them outright, rather than have them linger.

Author: AgeManning

beacon_node/lighthouse_network/src/peer_manager/mod.rs

@@ -290,11 +290,20 @@ impl<TSpec: EthSpec> PeerManager<TSpec> {
 
                 // If a peer is being banned, this trumps any temporary ban the peer might be
                 // under. We no longer track it in the temporary ban list.
-                self.temporary_banned_peers.raw_remove(peer_id);
-
-                // Inform the Swarm to ban the peer
-                self.events
-                    .push(PeerManagerEvent::Banned(*peer_id, banned_ips));
+                if !self.temporary_banned_peers.raw_remove(peer_id) {
+                    // If the peer is not already banned, inform the Swarm to ban the peer
+                    self.events
+                        .push(PeerManagerEvent::Banned(*peer_id, banned_ips));
+                    // If the peer was in the process of being un-banned, remove it (a rare race
+                    // condition)
+                    self.events.retain(|event| {
+                        if let PeerManagerEvent::UnBanned(unbanned_peer_id, _) = event {
+                            unbanned_peer_id != peer_id // Remove matching peer ids
+                        } else {
+                            true
+                        }
+                    });
+                }
             }
         }
     }
@@ -552,8 +561,8 @@ impl<TSpec: EthSpec> PeerManager<TSpec> {
                     Protocol::BlocksByRoot => return,
                     Protocol::Goodbye => return,
                     Protocol::LightClientBootstrap => return,
-                    Protocol::MetaData => PeerAction::LowToleranceError,
-                    Protocol::Status => PeerAction::LowToleranceError,
+                    Protocol::MetaData => PeerAction::Fatal,
+                    Protocol::Status => PeerAction::Fatal,
                 }
             }
             RPCError::StreamTimeout => match direction {

beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs

@@ -156,8 +156,10 @@ impl<TSpec: EthSpec> PeerManager<TSpec> {
             BanResult::BadScore => {
                 // This is a faulty state
                 error!(self.log, "Connected to a banned peer. Re-banning"; "peer_id" => %peer_id);
-                // Reban the peer
+                // 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) => {

beacon_node/lighthouse_network/src/service/mod.rs

@@ -1119,7 +1119,7 @@ impl<AppReqId: ReqId, TSpec: EthSpec> Network<AppReqId, TSpec> {
                 debug!(self.log, "Peer does not support gossipsub"; "peer_id" => %peer_id);
                 self.peer_manager_mut().report_peer(
                     &peer_id,
-                    PeerAction::LowToleranceError,
+                    PeerAction::Fatal,
                     ReportSource::Gossipsub,
                     Some(GoodbyeReason::Unknown),
                     "does_not_support_gossipsub",