From f8be7c326e3764502284691693f5e696450a5fb1 Mon Sep 17 00:00:00 2001
From: Etan Kissling <etan@status.im>
Date: Wed, 27 Mar 2024 16:00:21 +0100
Subject: [PATCH] be careful not to disconnect syncing peers in fragmented
 network

---
 beacon_chain/networking/peer_protocol.nim |  3 +++
 beacon_chain/sync/branch_discovery.nim    | 21 +++++++++++++++++----
 beacon_chain/sync/sync_manager.nim        |  6 ------
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/beacon_chain/networking/peer_protocol.nim b/beacon_chain/networking/peer_protocol.nim
index ff47b17dd..74b45e12e 100644
--- a/beacon_chain/networking/peer_protocol.nim
+++ b/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
diff --git a/beacon_chain/sync/branch_discovery.nim b/beacon_chain/sync/branch_discovery.nim
index 5be514f43..54d205c48 100644
--- a/beacon_chain/sync/branch_discovery.nim
+++ b/beacon_chain/sync/branch_discovery.nim
@@ -92,10 +92,20 @@ proc discoverBranch(
     peer
     peer_score = peer.getScore()
 
-  let
-    finalizedSlot = self.getFinalizedSlot()
-    peerHeadSlot = peer.getHeadSlot()
+  let oldPeerHeadSlot = peer.getHeadSlot()
+  if Moment.now() - peer.getStatusLastTime() >= StatusExpirationTime:
+    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):
-    peer.updateScore(PeerScoreUseless)
+    # 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)
diff --git a/beacon_chain/sync/sync_manager.nim b/beacon_chain/sync/sync_manager.nim
index 3640980e8..ebe91ec6a 100644
--- a/beacon_chain/sync/sync_manager.nim
+++ b/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: [].}