nicer error message for failed backfill (#4188)

* nicer error message for failed backfill

Many checkpoint sources don't support block download

* RestGenericError -> RestErrorMessage

...and other assorted fixes to bring rest types closer to spec

* fix tests

AllTests-mainnet.md

@@ -369,8 +369,8 @@ OK: 3/3 Fail: 0/3 Skip: 0/3
 OK: 1/1 Fail: 0/1 Skip: 0/1
 ## Serialization/deserialization test suite
 ```diff
-+ RestGenericError parser tests                                                              OK
-+ RestGenericError writer tests                                                              OK
++ RestErrorMessage parser tests                                                              OK
++ RestErrorMessage writer tests                                                              OK
 ```
 OK: 2/2 Fail: 0/2 Skip: 0/2
 ## Slashing Interchange tests  [Preset: mainnet]

beacon_chain/deposits.nim

@@ -214,7 +214,7 @@ proc restValidatorExit(config: BeaconNodeConf) {.async.} =
         quit 0
       else:
         let responseError = try:
-              Json.decode(response.data, RestGenericError)
+              Json.decode(response.data, RestErrorMessage)
         except CatchableError as err:
           fatal "Failed to decode invalid error server response on `submitPoolVoluntaryExit` request",
             err = err.msg

beacon_chain/rpc/rest_beacon_api.nim

@@ -969,20 +969,20 @@ proc installBeaconApiHandlers*(router: var RestRouter, node: BeaconNode) =
         res
     let failures =
       block:
-        var res: seq[RestAttestationsFailure]
+        var res: seq[RestIndexedErrorMessageItem]
         await allFutures(pending)
         for index, future in pending:
           if future.done():
             let fres = future.read()
             if fres.isErr():
-              let failure = RestAttestationsFailure(index: uint64(index),
-                                                    message: $fres.error())
+              let failure = RestIndexedErrorMessageItem(index: index,
+                                                        message: $fres.error())
               res.add(failure)
           elif future.failed() or future.cancelled():
             # This is unexpected failure, so we log the error message.
             let exc = future.readError()
-            let failure = RestAttestationsFailure(index: uint64(index),
-                                                  message: $exc.msg)
+            let failure = RestIndexedErrorMessageItem(index: index,
+                                                      message: $exc.msg)
             res.add(failure)
         res
 
@@ -1073,11 +1073,11 @@ proc installBeaconApiHandlers*(router: var RestRouter, node: BeaconNode) =
 
     let failures =
       block:
-        var res: seq[RestAttestationsFailure]
+        var res: seq[RestIndexedErrorMessageItem]
         for index, item in results:
           if item.isErr():
-            res.add(RestAttestationsFailure(index: uint64(index),
-                                            message: $item.error()))
+            res.add(RestIndexedErrorMessageItem(index: index,
+                                                message: $item.error()))
         res
     if len(failures) > 0:
       return RestApiResponse.jsonErrorList(Http400,

beacon_chain/rpc/rest_validator_api.nim

@@ -742,20 +742,20 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) =
 
     let failures =
       block:
-        var res: seq[RestAttestationsFailure]
+        var res: seq[RestIndexedErrorMessageItem]
         await allFutures(pending)
         for index, future in pending:
           if future.done():
             let fres = future.read()
             if fres.isErr():
-              let failure = RestAttestationsFailure(index: uint64(index),
-                                                    message: $fres.error())
+              let failure = RestIndexedErrorMessageItem(index: index,
+                                                        message: $fres.error())
               res.add(failure)
           elif future.failed() or future.cancelled():
             # This is unexpected failure, so we log the error message.
             let exc = future.readError()
-            let failure = RestAttestationsFailure(index: uint64(index),
-                                                  message: $exc.msg)
+            let failure = RestIndexedErrorMessageItem(index: index,
+                                                      message: $exc.msg)
             res.add(failure)
         res
 

beacon_chain/spec/eth2_apis/eth2_rest_serialization.nim

@@ -72,16 +72,6 @@ const
 type
   EmptyBody* = object
 
-  RestGenericError* = object
-    code*: uint64
-    message*: string
-    stacktraces*: Option[seq[string]]
-
-  RestDutyError* = object
-    code*: uint64
-    message*: string
-    failures*: seq[RestFailureItem]
-
   EncodeTypes* =
     AttesterSlashing |
     DeleteKeystoresBody |
@@ -128,8 +118,8 @@ type
     ListFeeRecipientResponse |
     PrepareBeaconProposer |
     ProduceBlockResponseV2 |
-    RestDutyError |
-    RestGenericError |
+    RestIndexedErrorMessage |
+    RestErrorMessage |
     RestValidator |
     Web3SignerErrorResponse |
     Web3SignerKeysResponse |
@@ -2220,12 +2210,12 @@ proc writeValue*(writer: var JsonWriter[RestJson],
     writer.writeField("is_optimistic", value.is_optimistic.get())
   writer.endRecord()
 
-## RestGenericError
+## RestErrorMessage
 proc readValue*(reader: var JsonReader[RestJson],
-                value: var RestGenericError) {.
+                value: var RestErrorMessage) {.
      raises: [SerializationError, IOError, Defect].} =
   var
-    code: Opt[uint64]
+    code: Opt[int]
     message: Opt[string]
     stacktraces: Option[seq[string]]
 
@@ -2234,30 +2224,34 @@ proc readValue*(reader: var JsonReader[RestJson],
     of "code":
       if code.isSome():
         reader.raiseUnexpectedField("Multiple `code` fields found",
-                                    "RestGenericError")
+                                    "RestErrorMessage")
       let ires =
         try:
           let res = reader.readValue(int)
           if res < 0:
             reader.raiseUnexpectedValue("Invalid `code` field value")
-          Opt.some(uint64(res))
+          Opt.some(res)
         except SerializationError:
-          Opt.none(uint64)
+          Opt.none(int)
       if ires.isNone():
-        let sres = Base10.decode(uint64, reader.readValue(string)).valueOr:
-          reader.raiseUnexpectedValue("Invalid `code` field format")
+        let sres =
+          try: parseInt(reader.readValue(string))
+          except ValueError:
+            reader.raiseUnexpectedValue("Invalid `code` field format")
+        if sres < 0:
+          reader.raiseUnexpectedValue("Invalid `code` field value")
         code = Opt.some(sres)
       else:
         code = ires
     of "message":
       if message.isSome():
         reader.raiseUnexpectedField("Multiple `message` fields found",
-                                    "RestGenericError")
+                                    "RestErrorMessage")
       message = Opt.some(reader.readValue(string))
     of "stacktraces":
       if stacktraces.isSome():
         reader.raiseUnexpectedField("Multiple `stacktraces` fields found",
-                                    "RestGenericError")
+                                    "RestErrorMessage")
       stacktraces = some(reader.readValue(seq[string]))
     else:
       # We ignore all additional fields.
@@ -2268,7 +2262,7 @@ proc readValue*(reader: var JsonReader[RestJson],
   if message.isNone():
     reader.raiseUnexpectedValue("Missing or invalid `message` value")
 
-  value = RestGenericError(
+  value = RestErrorMessage(
     code: code.get(), message: message.get(),
     stacktraces: stacktraces
   )

beacon_chain/spec/eth2_apis/rest_beacon_calls.nim

@@ -157,16 +157,16 @@ proc getBlockV2*(client: RestClientRef, block_id: BlockIdent,
     of 404:
       none(ref ForkedSignedBeaconBlock)
     of 400, 500:
-      let error = decodeBytes(RestGenericError, resp.data,
+      let error = decodeBytes(RestErrorMessage, resp.data,
                               resp.contentType).valueOr:
         let msg = "Incorrect response error format (" & $resp.status &
                   ") [" & $error & "]"
-        raise newException(RestError, msg)
+        raise (ref RestResponseError)(msg: msg, status: resp.status)
       let msg = "Error response (" & $resp.status & ") [" & error.message & "]"
-      raise newException(RestError, msg)
+      raise (ref RestResponseError)(
+        msg: msg, status: error.code, message: error.message)
     else:
-      let msg = "Unknown response status error (" & $resp.status & ")"
-      raise newException(RestError, msg)
+      raiseRestResponseError(resp)
 
 proc getBlockRoot*(block_id: BlockIdent): RestResponse[GetBlockRootResponse] {.
      rest, endpoint: "/eth/v1/beacon/blocks/{block_id}/root",

beacon_chain/spec/eth2_apis/rest_common.nim

@@ -19,7 +19,7 @@ proc raiseGenericError*(resp: RestPlainResponse) {.
      noreturn, raises: [RestError, Defect].} =
   let error =
     block:
-      let res = decodeBytes(RestGenericError, resp.data, resp.contentType)
+      let res = decodeBytes(RestErrorMessage, resp.data, resp.contentType)
       if res.isErr():
         let msg = "Incorrect response error format (" & $resp.status &
                   ") [" & $res.error() & "]"

beacon_chain/spec/eth2_apis/rest_debug_calls.nim

@@ -37,7 +37,7 @@ proc getStateV2*(client: RestClientRef, state_id: StateIdent,
       await client.getStateV2Plain(state_id, restAcceptType = restAccept)
     else:
       await client.getStateV2Plain(state_id)
-  let data =
+  return
     case resp.status
     of 200:
       if resp.contentType.isNone() or
@@ -64,17 +64,13 @@ proc getStateV2*(client: RestClientRef, state_id: StateIdent,
     of 404:
       nil
     of 400, 500:
-      let error =
-        block:
-          let res = decodeBytes(RestGenericError, resp.data, resp.contentType)
-          if res.isErr():
-            let msg = "Incorrect response error format (" & $resp.status &
-                      ") [" & $res.error() & "]"
-            raise newException(RestError, msg)
-          res.get()
+      let error = decodeBytes(RestErrorMessage, resp.data,
+                              resp.contentType).valueOr:
+        let msg = "Incorrect response error format (" & $resp.status &
+                  ") [" & $error & "]"
+        raise (ref RestResponseError)(msg: msg, status: resp.status)
       let msg = "Error response (" & $resp.status & ") [" & error.message & "]"
-      raise newException(RestError, msg)
+      raise (ref RestResponseError)(
+        msg: msg, status: error.code, message: error.message)
     else:
-      let msg = "Unknown response status error (" & $resp.status & ")"
-      raise newException(RestError, msg)
-  return data
+      raiseRestResponseError(resp)

beacon_chain/spec/eth2_apis/rest_types.nim

@@ -167,12 +167,20 @@ type
     slot*: Slot
     validators*: seq[ValidatorIndex]
 
-  RestFailureItem* = object
-    index*: uint64
+  RestErrorMessage* = object
+    ## https://github.com/ethereum/beacon-APIs/blob/v2.3.0/types/http.yaml#L86
+    code*: int
     message*: string
+    stacktraces*: Option[seq[string]]
 
-  RestAttestationsFailure* = object
-    index*: uint64
+  RestIndexedErrorMessage* = object
+    ## https://github.com/ethereum/beacon-APIs/blob/v2.3.0/types/http.yaml#L101
+    code*: int
+    message*: string
+    failures*: seq[RestIndexedErrorMessageItem]
+
+  RestIndexedErrorMessageItem* = object
+    index*: int
     message*: string
 
   RestValidator* = object

beacon_chain/trusted_node_sync.nim

@@ -106,17 +106,21 @@ proc doTrustedNodeSync*(
     for i in 0..<3:
       try:
         return await client.getBlockV2(BlockIdent.init(slot), cfg)
+      except RestResponseError as exc:
+        lastError = exc
+        notice "Server does not support block downloads / backfilling",
+          msg = exc.msg
+        break
       except CatchableError as exc:
+        # We'll assume this may be a connectivity error or something similar
         lastError = exc
+
         warn "Retrying download of block", slot, err = exc.msg
         client = RestClientRef.new(restUrl).valueOr:
           error "Cannot connect to server", url = restUrl, error = error
           quit 1
 
-    error "Unable to download block - backfill incomplete, but will resume when you start the beacon node",
-      slot, error = lastError.msg, url = client.address
-
-    quit 1
+    raise lastError
 
   let
     localGenesisRoot = db.getGenesisBlock().valueOr:
@@ -280,8 +284,8 @@ proc doTrustedNodeSync*(
         dbCache.update(blck)
     (checkpointSlot, checkpointBlock[].root)
   else:
-    notice "Skipping checkpoint download, database already exists",
-      head = shortLog(dbHead.get())
+    notice "Skipping checkpoint download, database already exists (remove db directory to get a fresh snapshot)",
+      databaseDir, head = shortLog(dbHead.get())
     (headSlot, dbHead.get())
 
   # Coming this far, we've done what ChainDAGRef.preInit would normally do -
@@ -383,23 +387,27 @@ proc doTrustedNodeSync*(
     # Download blocks backwards from the checkpoint slot, skipping the ones we
     # already have in the database. We'll do a few downloads in parallel which
     # risks having some redundant downloads going on, but speeds things up
-    for i in 0'u64..<(checkpointSlot.uint64 + gets.lenu64()):
-      if not isNil(gets[int(i mod gets.lenu64)]):
-        await processBlock(
-          gets[int(i mod gets.lenu64)],
-          checkpointSlot + gets.lenu64() - uint64(i))
-        gets[int(i mod gets.lenu64)] = nil
-
-      if i < checkpointSlot:
-        let slot = checkpointSlot - i
-        if dbCache.isKnown(slot):
-          continue
-
-        gets[int(i mod gets.lenu64)] = downloadBlock(slot)
-
-      if i mod 1024 == 0:
-        db.checkpoint() # Transfer stuff from wal periodically
-    true
+    try:
+      for i in 0'u64..<(checkpointSlot.uint64 + gets.lenu64()):
+        if not isNil(gets[int(i mod gets.lenu64)]):
+          await processBlock(
+            gets[int(i mod gets.lenu64)],
+            checkpointSlot + gets.lenu64() - uint64(i))
+          gets[int(i mod gets.lenu64)] = nil
+
+        if i < checkpointSlot:
+          let slot = checkpointSlot - i
+          if dbCache.isKnown(slot):
+            continue
+
+          gets[int(i mod gets.lenu64)] = downloadBlock(slot)
+
+        if i mod 1024 == 0:
+          db.checkpoint() # Transfer stuff from wal periodically
+      true
+    except CatchableError as exc: # Block download failed
+      notice "Backfilling incomplete - blocks will be downloaded when starting the node", msg = exc.msg
+      false
   else:
     notice "Database initialized, historical blocks will be backfilled when starting the node",
       missingSlots