From b53e6faad005596b14e43aa61dfe2f314d8a7427 Mon Sep 17 00:00:00 2001
From: Anton Kaliaev <anton.kalyaev@gmail.com>
Date: Tue, 12 Mar 2024 18:22:57 +0800
Subject: [PATCH] fix(blocksync): use timer instead of time.After (#2584)

`time.After` gets reset after each iteration of the `for` loop, which is
not what we want. We want the timer to fire after 30 sec if both peers
(first and second) don't respond.
---
 blocksync/pool.go | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/blocksync/pool.go b/blocksync/pool.go
index 2d742293313..2916f935b94 100644
--- a/blocksync/pool.go
+++ b/blocksync/pool.go
@@ -720,11 +720,11 @@ PICK_PEER_LOOP:
 
 // Picks a second peer and sends a request to it. If the second peer is already
 // set, does nothing.
-func (bpr *bpRequester) pickSecondPeerAndSendRequest() {
+func (bpr *bpRequester) pickSecondPeerAndSendRequest() (picked bool) {
 	bpr.mtx.Lock()
 	if bpr.secondPeerID != "" {
 		bpr.mtx.Unlock()
-		return
+		return false
 	}
 	peerID := bpr.peerID
 	bpr.mtx.Unlock()
@@ -736,7 +736,10 @@ func (bpr *bpRequester) pickSecondPeerAndSendRequest() {
 		bpr.mtx.Unlock()
 
 		bpr.pool.sendRequest(bpr.height, secondPeer.id)
+		return true
 	}
+
+	return false
 }
 
 // Informs the requester of a new pool's height.
@@ -761,6 +764,9 @@ OUTER_LOOP:
 			bpr.pickSecondPeerAndSendRequest()
 		}
 
+		retryTimer := time.NewTimer(requestRetrySeconds * time.Second)
+		defer retryTimer.Stop()
+
 		for {
 			select {
 			case <-bpr.pool.Quit():
@@ -770,7 +776,7 @@ OUTER_LOOP:
 				return
 			case <-bpr.Quit():
 				return
-			case <-time.After(requestRetrySeconds * time.Second):
+			case <-retryTimer.C:
 				if !gotBlock {
 					bpr.Logger.Debug("Retrying block request(s) after timeout", "height", bpr.height, "peer", bpr.peerID, "secondPeerID", bpr.secondPeerID)
 					bpr.reset(bpr.peerID)
@@ -787,12 +793,21 @@ OUTER_LOOP:
 				// If both peers returned NoBlockResponse or bad block, reschedule both
 				// requests. If not, wait for the other peer.
 				if len(bpr.requestedFrom()) == 0 {
+					retryTimer.Stop()
 					continue OUTER_LOOP
 				}
 			case newHeight := <-bpr.newHeightCh:
 				if !gotBlock && bpr.height-newHeight < minBlocksForSingleRequest {
 					// The operation is a noop if the second peer is already set. The cost is checking a mutex.
-					bpr.pickSecondPeerAndSendRequest()
+					//
+					// If the second peer was just set, reset the retryTimer to give the
+					// second peer a chance to respond.
+					if picked := bpr.pickSecondPeerAndSendRequest(); picked {
+						if !retryTimer.Stop() {
+							<-retryTimer.C
+						}
+						retryTimer.Reset(requestRetrySeconds * time.Second)
+					}
 				}
 			case <-bpr.gotBlockCh:
 				gotBlock = true