VC: Fix API timeouts issue. (#5297)

* Fix VC timeouts issue.
Add tests.

* Update AllTests.

* Remove assertion, which could crash VC when API call was interrupted by some higher logic.

AllTests-mainnet.md

@@ -584,10 +584,12 @@ OK: 24/24 Fail: 0/24 Skip: 0/24
 OK: 1/1 Fail: 0/1 Skip: 0/1
 ## Validator Client test suite
 ```diff
++ bestSuccess() API timeout test                                                             OK
++ firstSuccessParallel() API timeout test                                                    OK
 + getAttestationDataScore() test vectors                                                     OK
 + normalizeUri() test vectors                                                                OK
 ```
-OK: 2/2 Fail: 0/2 Skip: 0/2
+OK: 4/4 Fail: 0/4 Skip: 0/4
 ## Validator change pool testing suite
 ```diff
 + addValidatorChangeMessage/getAttesterSlashingMessage                                       OK
@@ -698,4 +700,4 @@ OK: 2/2 Fail: 0/2 Skip: 0/2
 OK: 9/9 Fail: 0/9 Skip: 0/9
 
 ---TOTAL---
-OK: 391/396 Fail: 0/396 Skip: 5/396
+OK: 393/398 Fail: 0/398 Skip: 5/398

beacon_chain/validator_client/api.nim

@@ -116,15 +116,16 @@ proc lazyWait(nodes: seq[BeaconNodeServerRef], requests: seq[FutureBase],
 
 proc apiResponseOr[T](future: FutureBase, timerFut: Future[void],
                       message: string): ApiResponse[T] =
-  if future.finished():
-    doAssert(not(future.cancelled()))
+  if future.finished() and not(future.cancelled()):
     if future.failed():
       ApiResponse[T].err($future.error.msg)
     else:
       ApiResponse[T].ok(Future[T](future).read())
   else:
-    doAssert(timerFut.finished())
-    ApiResponse[T].err(message)
+    if timerFut.finished():
+      ApiResponse[T].err(message)
+    else:
+      ApiResponse[T].err("Interrupted by the caller")
 
 template firstSuccessParallel*(
            vc: ValidatorClientRef,
@@ -198,7 +199,7 @@ template firstSuccessParallel*(
           else:
             raceFut = race(pendingRequests)
 
-            if isNil(timerFut):
+            if not(isNil(timerFut)):
               await raceFut or timerFut
             else:
               await allFutures(raceFut)
@@ -346,7 +347,7 @@ template bestSuccess*(
             try:
               raceFut = race(pendingRequests)
 
-              if isNil(timerFut):
+              if not(isNil(timerFut)):
                 await raceFut or timerFut
               else:
                 await allFutures(raceFut)

beacon_chain/validator_client/duties_service.nim

@@ -473,6 +473,7 @@ proc pollForBeaconProposers*(service: DutiesServiceRef) {.async.} =
             err_msg = exc.msg
 
     service.pruneBeaconProposers(currentEpoch)
+    vc.pruneBlocksSeen(currentEpoch)
 
 proc prepareBeaconProposers*(service: DutiesServiceRef) {.async.} =
   let vc = service.client

tests/test_validator_client.nim

@@ -9,8 +9,8 @@
 {.used.}
 
 import std/strutils
-import unittest2
-import ../beacon_chain/validator_client/[common, scoring]
+import chronos/unittest2/asynctests
+import ../beacon_chain/validator_client/[api, common, scoring, fallback_service]
 
 const
   HostNames = [
@@ -250,3 +250,88 @@ suite "Validator Client test suite":
         roots = createRootsSeen(vector[1])
         score = shortScore(roots.getAttestationDataScore(adata))
       check score == vector[2]
+
+  asyncTest "firstSuccessParallel() API timeout test":
+    let
+      uri = parseUri("http://127.0.0.1/")
+      beaconNodes = @[BeaconNodeServerRef.init(uri, 0).tryGet()]
+      vconf = ValidatorClientConf.load(
+        cmdLine = mapIt(["--beacon-node=http://127.0.0.1"], it))
+      epoch = Epoch(1)
+      strategy = ApiStrategyKind.Priority
+
+    var gotCancellation = false
+    var vc = ValidatorClientRef(config: vconf, beaconNodes: beaconNodes)
+    vc.fallbackService = await FallbackServiceRef.init(vc)
+
+    proc getTestDuties(client: RestClientRef,
+                       epoch: Epoch): Future[RestPlainResponse] {.async.} =
+      try:
+        await sleepAsync(1.seconds)
+      except CancelledError as exc:
+        gotCancellation = true
+        raise exc
+
+    const
+      RequestName = "getTestDuties"
+
+    let response = vc.firstSuccessParallel(
+      RestPlainResponse,
+      uint64,
+      100.milliseconds,
+      AllBeaconNodeStatuses,
+      {BeaconNodeRole.Duties},
+      getTestDuties(it, epoch)):
+        check:
+          apiResponse.isErr()
+          apiResponse.error ==
+            "Timeout exceeded while awaiting for the response"
+        ApiResponse[uint64].err(apiResponse.error)
+
+    check:
+      response.isErr()
+      gotCancellation == true
+
+  asyncTest "bestSuccess() API timeout test":
+    let
+      uri = parseUri("http://127.0.0.1/")
+      beaconNodes = @[BeaconNodeServerRef.init(uri, 0).tryGet()]
+      vconf = ValidatorClientConf.load(
+        cmdLine = mapIt(["--beacon-node=http://127.0.0.1"], it))
+      epoch = Epoch(1)
+      strategy = ApiStrategyKind.Priority
+
+    var gotCancellation = false
+    var vc = ValidatorClientRef(config: vconf, beaconNodes: beaconNodes)
+    vc.fallbackService = await FallbackServiceRef.init(vc)
+
+    proc getTestDuties(client: RestClientRef,
+                       epoch: Epoch): Future[RestPlainResponse] {.async.} =
+      try:
+        await sleepAsync(1.seconds)
+      except CancelledError as exc:
+        gotCancellation = true
+        raise exc
+
+    proc getTestScore(data: uint64): float64 = Inf
+
+    const
+      RequestName = "getTestDuties"
+
+    let response = vc.bestSuccess(
+      RestPlainResponse,
+      uint64,
+      100.milliseconds,
+      AllBeaconNodeStatuses,
+      {BeaconNodeRole.Duties},
+      getTestDuties(it, epoch),
+      getTestScore(itresponse)):
+        check:
+          apiResponse.isErr()
+          apiResponse.error ==
+            "Timeout exceeded while awaiting for the response"
+        ApiResponse[uint64].err(apiResponse.error)
+
+    check:
+      response.isErr()
+      gotCancellation == true