simplify error handling in block processing (#2337)

* ValidationResult -> Result
* get rid of mixed exception / other styles

hive_integration/nodocker/engine/engine_env.nim

@@ -219,4 +219,4 @@ func version*(env: EngineEnv, time: uint64): Version =
   env.version(time.EthTime)
 
 proc setBlock*(env: EngineEnv, blk: common.EthBlock): bool =
-  env.chain.setBlock(blk) == ValidationResult.OK
+  env.chain.setBlock(blk).isOk()

hive_integration/nodocker/engine/node.nim

@@ -33,8 +33,7 @@ import
 proc processBlock(
     vmState: BaseVMState;  ## Parent environment of header/body block
     blk:     EthBlock;  ## Header/body block to add to the blockchain
-    ): ValidationResult
+    ): Result[void, string] =
-    {.gcsafe, raises: [CatchableError].} =
   ## Generalised function to processes `(header,body)` pair for any network,
   ## regardless of PoA or not.
   ##
@@ -53,13 +52,9 @@ proc processBlock(
       db.applyDAOHardFork()
 
   if header.parentBeaconBlockRoot.isSome:
-    let r = vmState.processBeaconBlockRoot(header.parentBeaconBlockRoot.get)
+    ? vmState.processBeaconBlockRoot(header.parentBeaconBlockRoot.get)
-    if r.isErr:
-      error("error in processing beaconRoot", err=r.error)
 
-  let r = processTransactions(vmState, header, blk.transactions)
+  ? processTransactions(vmState, header, blk.transactions)
-  if r.isErr:
-    error("error in processing transactions", err=r.error)
 
   if vmState.determineFork >= FkShanghai:
     for withdrawal in blk.withdrawals.get:
@@ -78,7 +73,7 @@ proc processBlock(
 
   dbTx.commit()
 
-  ValidationResult.OK
+  ok()
 
 proc getVmState(c: ChainRef, header: BlockHeader):
                  Result[BaseVMState, void] =
@@ -95,8 +90,7 @@ proc getVmState(c: ChainRef, header: BlockHeader):
 
 # A stripped down version of persistBlocks without validation
 # intended to accepts invalid block
-proc setBlock*(c: ChainRef; blk: EthBlock): ValidationResult
+proc setBlock*(c: ChainRef; blk: EthBlock): Result[void, string] =
-                          {.inline, raises: [CatchableError].} =
   template header: BlockHeader = blk.header
   let dbTx = c.db.newTransaction()
   defer: dbTx.dispose()
@@ -106,13 +100,11 @@ proc setBlock*(c: ChainRef; blk: EthBlock): ValidationResult
   # Needed for figuring out whether KVT cleanup is due (see at the end)
   let
     vmState = c.getVmState(header).valueOr:
-      return ValidationResult.Error
+      return err("no vmstate")
     stateRootChpt = vmState.parent.stateRoot # Check point
-    validationResult = vmState.processBlock(blk)
+  ? vmState.processBlock(blk)
-
-  if validationResult != ValidationResult.OK:
-    return validationResult
 
+  try:
     c.db.persistHeaderToDb(
       header, c.com.consensus == ConsensusType.POS, c.com.startOfHistory)
     discard c.db.persistTransactions(header.blockNumber, blk.transactions)
@@ -120,6 +112,8 @@ proc setBlock*(c: ChainRef; blk: EthBlock): ValidationResult
 
     if blk.withdrawals.isSome:
       discard c.db.persistWithdrawals(blk.withdrawals.get)
+  except CatchableError as exc:
+    return err(exc.msg)
 
   # update currentBlock *after* we persist it
   # so the rpc return consistent result
@@ -137,7 +131,7 @@ proc setBlock*(c: ChainRef; blk: EthBlock): ValidationResult
   # `persistent()` together with the respective block number.
   c.db.persistent(header.blockNumber - 1)
 
-  ValidationResult.OK
+  ok()
 
 # ------------------------------------------------------------------------------
 # End

nimbus/core/chain/persist_blocks.nim

@@ -37,13 +37,9 @@ type
 
   PersistBlockFlags = set[PersistBlockFlag]
 
-  PersistStats = tuple
+  PersistStats = tuple[blocks: int, txs: int, gas: GasInt]
-    blocks: int
-    txs: int
-    gas: GasInt
 
-const
+const CleanUpEpoch = 30_000.toBlockNumber
-  CleanUpEpoch = 30_000.toBlockNumber
   ## Regular checks for history clean up (applies to single state DB). This
   ## is mainly a debugging/testing feature so that the database can be held
   ## a bit smaller. It is not applicable to a full node.
@@ -52,30 +48,30 @@ const
 # Private
 # ------------------------------------------------------------------------------
 
-proc getVmState(c: ChainRef, header: BlockHeader):
+proc getVmState(c: ChainRef, header: BlockHeader): Result[BaseVMState, string] =
-                Result[BaseVMState, string] =
   let vmState = BaseVMState()
   if not vmState.init(header, c.com):
     return err("Could not initialise VMState")
   ok(vmState)
 
-proc purgeOlderBlocksFromHistory(
+proc purgeOlderBlocksFromHistory(db: CoreDbRef, bn: BlockNumber) =
-    db: CoreDbRef;
-    bn: BlockNumber;
-      ) {.inline, raises: [RlpError].} =
   ## Remove non-reachable blocks from KVT database
   if 0 < bn:
     var blkNum = bn - 1
     while 0 < blkNum:
+      try:
         if not db.forgetHistory blkNum:
           break
+      except RlpError as exc:
+        warn "Error forgetting history", err = exc.msg
       blkNum = blkNum - 1
 
-proc persistBlocksImpl(c: ChainRef; blocks: openArray[EthBlock];
+proc persistBlocksImpl(
-                       flags: PersistBlockFlags = {}): Result[PersistStats, string]
+    c: ChainRef, blocks: openArray[EthBlock], flags: PersistBlockFlags = {}
-                         {.raises: [CatchableError] .} =
+): Result[PersistStats, string] =
   let dbTx = c.db.newTransaction()
-  defer: dbTx.dispose()
+  defer:
+    dbTx.dispose()
 
   c.com.hardForkTransition(blocks[0].header)
 
@@ -89,12 +85,8 @@ proc persistBlocksImpl(c: ChainRef; blocks: openArray[EthBlock];
 
   var txs = 0
   for blk in blocks:
-    template header: BlockHeader = blk.header
+    template header(): BlockHeader =
-
+      blk.header
-    # # This transaction keeps the current state open for inspection
-    # # if an error occurs (as needed for `Aristo`.).
-    # let lapTx = c.db.newTransaction()
-    # defer: lapTx.dispose()
 
     c.com.hardForkTransition(header)
 
@@ -102,17 +94,12 @@ proc persistBlocksImpl(c: ChainRef; blocks: openArray[EthBlock];
       debug "Cannot update VmState", blockNumber = header.blockNumber
       return err("Cannot update VmState to block " & $header.blockNumber)
 
-    if c.validateBlock and c.extraValidation and
+    if c.validateBlock and c.extraValidation and c.verifyFrom <= header.blockNumber:
-       c.verifyFrom <= header.blockNumber:
-
       # TODO: how to checkseal from here
-      ? c.com.validateHeaderAndKinship(blk, checkSealOK = false)
+      ?c.com.validateHeaderAndKinship(blk, checkSealOK = false)
 
-    let
+    if c.validateBlock:
-      validationResult = if c.validateBlock:
+      ?vmState.processBlock(blk)
-                           vmState.processBlock(blk)
-                         else:
-                           ValidationResult.OK
 
     # when defined(nimbusDumpDebuggingMetaData):
     #   if validationResult == ValidationResult.Error and
@@ -120,12 +107,11 @@ proc persistBlocksImpl(c: ChainRef; blocks: openArray[EthBlock];
     #     vmState.dumpDebuggingMetaData(header, body)
     #     warn "Validation error. Debugging metadata dumped."
 
-    if validationResult != ValidationResult.OK:
+    try:
-      return err("Failed to validate block")
-
       if NoPersistHeader notin flags:
         c.db.persistHeaderToDb(
-        header, c.com.consensus == ConsensusType.POS, c.com.startOfHistory)
+          header, c.com.consensus == ConsensusType.POS, c.com.startOfHistory
+        )
 
       if NoSaveTxs notin flags:
         discard c.db.persistTransactions(header.blockNumber, blk.transactions)
@@ -135,6 +121,8 @@ proc persistBlocksImpl(c: ChainRef; blocks: openArray[EthBlock];
 
       if NoSaveWithdrawals notin flags and blk.withdrawals.isSome:
         discard c.db.persistWithdrawals(blk.withdrawals.get)
+    except CatchableError as exc:
+      return err(exc.msg)
 
     # update currentBlock *after* we persist it
     # so the rpc return consistent result
@@ -157,7 +145,10 @@ proc persistBlocksImpl(c: ChainRef; blocks: openArray[EthBlock];
     let n = fromBlock div CleanUpEpoch
     if 0 < n and n < (toBlock div CleanUpEpoch):
       # Starts at around `2 * CleanUpEpoch`
+      try:
         c.db.purgeOlderBlocksFromHistory(fromBlock - CleanUpEpoch)
+      except CatchableError as exc:
+        warn "Could not clean up old blocks from history", err = exc.msg
 
   ok((blocks.len, txs, vmState.cumulativeGasUsed))
 
@@ -166,54 +157,60 @@ proc persistBlocksImpl(c: ChainRef; blocks: openArray[EthBlock];
 # ------------------------------------------------------------------------------
 
 proc insertBlockWithoutSetHead*(c: ChainRef, blk: EthBlock): Result[void, string] =
-  try:
+  discard ?c.persistBlocksImpl([blk], {NoPersistHeader, NoSaveReceipts})
-    discard ? c.persistBlocksImpl(
-      [blk], {NoPersistHeader, NoSaveReceipts})
 
+  try:
     c.db.persistHeaderToDbWithoutSetHead(blk.header, c.com.startOfHistory)
     ok()
-  except CatchableError as exc:
+  except RlpError as exc:
     err(exc.msg)
 
 proc setCanonical*(c: ChainRef, header: BlockHeader): Result[void, string] =
-  try:
   if header.parentHash == Hash256():
-      discard c.db.setHead(header.blockHash)
+    try:
+      if not c.db.setHead(header.blockHash):
+        return err("setHead failed")
+    except RlpError as exc:
+      # TODO fix exception+bool error return
+      return err(exc.msg)
     return ok()
 
   var body: BlockBody
+  try:
     if not c.db.getBlockBody(header, body):
-      debug "Failed to get BlockBody",
+      debug "Failed to get BlockBody", hash = header.blockHash
-        hash = header.blockHash
       return err("Could not get block body")
+  except RlpError as exc:
+    return err(exc.msg)
 
-    discard ? c.persistBlocksImpl([EthBlock.init(header, move(body))], {NoPersistHeader, NoSaveTxs})
+  discard
+    ?c.persistBlocksImpl(
+      [EthBlock.init(header, move(body))], {NoPersistHeader, NoSaveTxs}
+    )
 
+  try:
     discard c.db.setHead(header.blockHash)
+  except RlpError as exc:
+    return err(exc.msg)
   ok()
-  except CatchableError as exc:
-    err(exc.msg)
 
 proc setCanonical*(c: ChainRef, blockHash: Hash256): Result[void, string] =
   var header: BlockHeader
   if not c.db.getBlockHeader(blockHash, header):
-    debug "Failed to get BlockHeader",
+    debug "Failed to get BlockHeader", hash = blockHash
-      hash = blockHash
     return err("Could not get block header")
 
   setCanonical(c, header)
 
 proc persistBlocks*(
-    c: ChainRef; blocks: openArray[EthBlock]): Result[PersistStats, string] =
+    c: ChainRef, blocks: openArray[EthBlock]
+): Result[PersistStats, string] =
   # Run the VM here
   if blocks.len == 0:
     debug "Nothing to do"
     return ok(default(PersistStats)) # TODO not nice to return nil
 
-  try:
   c.persistBlocksImpl(blocks)
-  except CatchableError as exc:
-    err(exc.msg)
 
 # ------------------------------------------------------------------------------
 # End

nimbus/core/executor/process_block.nim

@@ -28,10 +28,8 @@ import
 # Factored this out of procBlkPreamble so that it can be used directly for
 # stateless execution of specific transactions.
 proc processTransactions*(
-    vmState: BaseVMState;
+    vmState: BaseVMState, header: BlockHeader, transactions: seq[Transaction]
-    header: BlockHeader;
+): Result[void, string] =
-    transactions: seq[Transaction];
-      ): Result[void, string] =
   vmState.receipts = newSeq[Receipt](transactions.len)
   vmState.cumulativeGasUsed = 0
 
@@ -45,71 +43,64 @@ proc processTransactions*(
     vmState.receipts[txIndex] = vmState.makeReceipt(tx.txType)
   ok()
 
-proc procBlkPreamble(vmState: BaseVMState; blk: EthBlock): bool
+proc procBlkPreamble(vmState: BaseVMState, blk: EthBlock): Result[void, string] =
-    {.raises: [CatchableError].} =
+  template header(): BlockHeader =
-  template header: BlockHeader = blk.header
+    blk.header
-  if vmState.com.daoForkSupport and
+
-     vmState.com.daoForkBlock.get == header.blockNumber:
+  if vmState.com.daoForkSupport and vmState.com.daoForkBlock.get == header.blockNumber:
     vmState.mutateStateDB:
       db.applyDAOHardFork()
 
   if blk.transactions.calcTxRoot != header.txRoot:
-    debug "Mismatched txRoot",
+    return err("Mismatched txRoot")
-      blockNumber = header.blockNumber
-    return false
 
   if vmState.determineFork >= FkCancun:
     if header.parentBeaconBlockRoot.isNone:
-      raise ValidationError.newException("Post-Cancun block header must have parentBeaconBlockRoot")
+      return err("Post-Cancun block header must have parentBeaconBlockRoot")
+
+    ?vmState.processBeaconBlockRoot(header.parentBeaconBlockRoot.get)
   else:
     if header.parentBeaconBlockRoot.isSome:
-      raise ValidationError.newException("Pre-Cancun block header must not have parentBeaconBlockRoot")
+      return err("Pre-Cancun block header must not have parentBeaconBlockRoot")
-
-  if header.parentBeaconBlockRoot.isSome:
-    let r = vmState.processBeaconBlockRoot(header.parentBeaconBlockRoot.get)
-    if r.isErr:
-      error("error in processing beaconRoot", err=r.error)
 
   if header.txRoot != EMPTY_ROOT_HASH:
     if blk.transactions.len == 0:
-      debug "No transactions in body",
+      return err("Transactions missing from body")
-        blockNumber = header.blockNumber
+
-      return false
+    ?processTransactions(vmState, header, blk.transactions)
-    else:
+  elif blk.transactions.len > 0:
-      let r = processTransactions(vmState, header, blk.transactions)
+    return err("Transactions in block with empty txRoot")
-      if r.isErr:
-        error("error in processing transactions", err=r.error)
 
   if vmState.determineFork >= FkShanghai:
     if header.withdrawalsRoot.isNone:
-      raise ValidationError.newException("Post-Shanghai block header must have withdrawalsRoot")
+      return err("Post-Shanghai block header must have withdrawalsRoot")
     if blk.withdrawals.isNone:
-      raise ValidationError.newException("Post-Shanghai block body must have withdrawals")
+      return err("Post-Shanghai block body must have withdrawals")
 
     for withdrawal in blk.withdrawals.get:
       vmState.stateDB.addBalance(withdrawal.address, withdrawal.weiAmount)
   else:
     if header.withdrawalsRoot.isSome:
-      raise ValidationError.newException("Pre-Shanghai block header must not have withdrawalsRoot")
+      return err("Pre-Shanghai block header must not have withdrawalsRoot")
     if blk.withdrawals.isSome:
-      raise ValidationError.newException("Pre-Shanghai block body must not have withdrawals")
+      return err("Pre-Shanghai block body must not have withdrawals")
 
   if vmState.cumulativeGasUsed != header.gasUsed:
+    # TODO replace logging with better error
     debug "gasUsed neq cumulativeGasUsed",
-      gasUsed = header.gasUsed,
+      gasUsed = header.gasUsed, cumulativeGasUsed = vmState.cumulativeGasUsed
-      cumulativeGasUsed = vmState.cumulativeGasUsed
+    return err("gasUsed mismatch")
-    return false
 
   if header.ommersHash != EMPTY_UNCLE_HASH:
     let h = vmState.com.db.persistUncles(blk.uncles)
     if h != header.ommersHash:
-      debug "Uncle hash mismatch"
+      return err("ommersHash mismatch")
-      return false
+  elif blk.uncles.len > 0:
+    return err("Uncles in block with empty uncle hash")
 
-  true
+  ok()
 
-proc procBlkEpilogue(vmState: BaseVMState, header: BlockHeader): bool
+proc procBlkEpilogue(vmState: BaseVMState, header: BlockHeader): Result[void, string] =
-    {.gcsafe, raises: [].} =
   # Reward beneficiary
   vmState.mutateStateDB:
     if vmState.collectWitnessData:
@@ -117,56 +108,55 @@ proc procBlkEpilogue(vmState: BaseVMState, header: BlockHeader): bool
 
     db.persist(clearEmptyAccount = vmState.determineFork >= FkSpurious)
 
-  let stateDb = vmState.stateDB
+  let stateDB = vmState.stateDB
-  if header.stateRoot != stateDb.rootHash:
+  if header.stateRoot != stateDB.rootHash:
+    # TODO replace logging with better error
     debug "wrong state root in block",
       blockNumber = header.blockNumber,
       expected = header.stateRoot,
-      actual = stateDb.rootHash,
+      actual = stateDB.rootHash,
       arrivedFrom = vmState.com.db.getCanonicalHead().stateRoot
-    return false
+    return err("stateRoot mismatch")
 
   let bloom = createBloom(vmState.receipts)
   if header.bloom != bloom:
-    debug "wrong bloom in block",
+    return err("bloom mismatch")
-      blockNumber = header.blockNumber
-    return false
 
   let receiptRoot = calcReceiptRoot(vmState.receipts)
   if header.receiptRoot != receiptRoot:
+    # TODO replace logging with better error
     debug "wrong receiptRoot in block",
       blockNumber = header.blockNumber,
       actual = receiptRoot,
       expected = header.receiptRoot
-    return false
+    return err("receiptRoot mismatch")
 
-  true
+  ok()
 
 # ------------------------------------------------------------------------------
 # Public functions
 # ------------------------------------------------------------------------------
 
 proc processBlock*(
-    vmState: BaseVMState;  ## Parent environment of header/body block
+    vmState: BaseVMState, ## Parent environment of header/body block
-    blk:     EthBlock;     ## Header/body block to add to the blockchain
+    blk: EthBlock, ## Header/body block to add to the blockchain
-      ): ValidationResult {.raises: [CatchableError].} =
+): Result[void, string] =
   ## Generalised function to processes `blk` for any network.
   var dbTx = vmState.com.db.newTransaction()
-  defer: dbTx.dispose()
+  defer:
+    dbTx.dispose()
 
-  if not vmState.procBlkPreamble(blk):
+  ?vmState.procBlkPreamble(blk)
-    return ValidationResult.Error
 
   # EIP-3675: no reward for miner in POA/POS
   if vmState.com.consensus == ConsensusType.POW:
     vmState.calculateReward(blk.header, blk.uncles)
 
-  if not vmState.procBlkEpilogue(blk.header):
+  ?vmState.procBlkEpilogue(blk.header)
-    return ValidationResult.Error
 
   dbTx.commit()
 
-  ValidationResult.OK
+  ok()
 
 # ------------------------------------------------------------------------------
 # End

nimbus/rpc/experimental.nim

@@ -51,8 +51,7 @@ proc getMultiKeys*(
   defer: dbTx.dispose()
 
   # Execute the block of transactions and collect the keys of the touched account state
-  let processBlockResult = processBlock(vmState, blk)
+  processBlock(vmState, blk).expect("success")
-  doAssert processBlockResult == ValidationResult.OK
 
   let mkeys = vmState.stateDB.makeMultiKeys()
 

premix/debug.nim

@@ -41,10 +41,10 @@ proc executeBlock(blockEnv: JsonNode, memoryDB: CoreDbRef, blockNumber: UInt256)
     vmState = BaseVMState.new(parent, blk.header, com)
     validationResult = vmState.processBlock(blk)
 
-  if validationResult != ValidationResult.OK:
+  if validationResult.isErr:
-    error "block validation error", validationResult
+    error "block validation error", err = validationResult.error()
   else:
-    info "block validation success", validationResult, blockNumber
+    info "block validation success", blockNumber
 
   transaction.rollback()
   vmState.dumpDebuggingMetaData(blk, false)

premix/no-hunter.nim

@@ -95,7 +95,7 @@ proc putAncestorsIntoDB(vmState: HunterVMState, db: CoreDbRef) =
   for header in vmState.headers.values:
     db.addBlockNumberToHashLookup(header)
 
-proc huntProblematicBlock(blockNumber: UInt256): ValidationResult =
+proc huntProblematicBlock(blockNumber: UInt256): Result[void,  string] =
   let
     # prepare needed state from previous block
     parentNumber = blockNumber - 1
@@ -114,12 +114,12 @@ proc huntProblematicBlock(blockNumber: UInt256): ValidationResult =
     vmState = HunterVMState.new(parentBlock.header, thisBlock.header, com)
     validationResult = vmState.processBlock(thisBlock.header, thisBlock.body)
 
-  if validationResult != ValidationResult.OK:
+  if validationResult.isErr():
     transaction.rollback()
     putAncestorsIntoDB(vmState, com.db)
     vmState.dumpDebuggingMetaData(thisBlock.header, thisBlock.body, false)
 
-  result = validationResult
+  validationResult
 
 proc main() {.used.} =
   let conf = getConfiguration()
@@ -138,7 +138,7 @@ proc main() {.used.} =
 
   while true:
     echo blockNumber
-    if huntProblematicBlock(blockNumber) != ValidationResult.OK:
+    if huntProblematicBlock(blockNumber).isErr:
       echo "shot down problematic block: ", blockNumber
       problematicBlocks.add blockNumber
     blockNumber = blockNumber + 1

premix/regress.nim

@@ -40,8 +40,9 @@ proc validateBlock(com: CommonRef, blockNumber: BlockNumber): BlockNumber =
       vmState = BaseVMState.new(parent, blocks[i].header, com)
       validationResult = vmState.processBlock(blocks[i])
 
-    if validationResult != ValidationResult.OK:
+    if validationResult.isErr:
-      error "block validation error", validationResult, blockNumber = blockNumber + i.u256
+      error "block validation error",
+        err = validationResult.error(), blockNumber = blockNumber + i.u256
 
     parent = blocks[i].header
 

tests/test_txpool/helpers.nim

@@ -184,9 +184,6 @@ proc pp*(w: TxChainGasLimits): string =
 # Public functions, other
 # ------------------------------------------------------------------------------
 
-proc isOk*(rc: ValidationResult): bool =
-  rc == ValidationResult.OK
-
 proc toHex*(acc: EthAddress): string =
   acc.toHex