be careful not to disconnect syncing peers in fragmented network

beacon_chain/networking/peer_protocol.nim

@@ -208,6 +208,9 @@ proc handleStatus(peer: Peer,
       await peer.handlePeer()
     true
 
+const StatusExpirationTime* = chronos.minutes(2)
+  ## Time time it takes for the peer's status information to expire.
+
 proc updateStatus*(peer: Peer): Future[bool] {.async: (raises: [CancelledError]).} =
   ## Request `status` of remote peer ``peer``.
   let

beacon_chain/sync/branch_discovery.nim

@@ -92,10 +92,20 @@ proc discoverBranch(
     peer
     peer_score = peer.getScore()
 
-  let
+  let oldPeerHeadSlot = peer.getHeadSlot()
-    finalizedSlot = self.getFinalizedSlot()
+  if Moment.now() - peer.getStatusLastTime() >= StatusExpirationTime:
-    peerHeadSlot = peer.getHeadSlot()
+    if not(await peer.updateStatus()):
+      peer.updateScore(PeerScoreNoStatus)
+      debug "Failed to update status"
+      return
+  let peerHeadSlot = peer.getHeadSlot()
+  if peerHeadSlot != oldPeerHeadSlot:
+    peer.updateScore(PeerScoreGoodStatus)
+    debug "Peer has synced to a new head", oldPeerHeadSlot, peerHeadSlot
+
+  let finalizedSlot = self.getFinalizedSlot()
   if peerHeadSlot <= finalizedSlot:
+    # This peer can sync from different peers, it is useless to us at this time
     peer.updateScore(PeerScoreUseless)
     debug "Peer's head slot is already finalized", peerHeadSlot, finalizedSlot
     return
@@ -103,11 +113,14 @@ proc discoverBranch(
   var blockRoot = peer.getHeadRoot()
   logScope: blockRoot
   if self.isBlockKnown(blockRoot):
+    # This peer may be actively syncing from us, only descore if no disconnect
+    if peer.getScore() >= PeerScoreLowLimit - PeerScoreUseless:
       peer.updateScore(PeerScoreUseless)
     debug "Peer's head block root is already known"
     return
 
   # Many peers disconnect on rate limit, we have to avoid getting hit by it
+  # to have a chance in picking up branches that don't have good propagation
   const
     maxRequestsPerBurst = 15
     burstDuration = chronos.seconds(30)

beacon_chain/sync/sync_manager.nim

@@ -28,12 +28,6 @@ const
   SyncWorkersCount* = 10
     ## Number of sync workers to spawn
 
-  StatusUpdateInterval* = chronos.minutes(1)
-    ## Minimum time between two subsequent calls to update peer's status
-
-  StatusExpirationTime* = chronos.minutes(2)
-    ## Time time it takes for the peer's status information to expire.
-
 type
   PeerSyncer*[T] = proc(peer: T) {.gcsafe, raises: [].}