re-wrote validation with exceptionless functions

why:
  exceptions were from test code should be avoided in production code

nimbus/p2p/validate.nim

@@ -11,7 +11,6 @@
 import
   ../constants,
   ../db/[db_chain, accounts_cache],
-  ../errors,
   ../transaction,
   ../utils,
   ../utils/header,
@@ -19,17 +18,19 @@ import
   ../vm_types,
   ../vm_types2,
   chronicles,
-  eth/[common, rlp],
+  eth/[common, rlp, trie/trie_defs],
-  eth/trie/trie_defs,
   ethash,
   nimcrypto,
   options,
   sets,
-  stew/endians2,
+  stew/[results, endians2],
   strutils,
   tables,
   times
 
+export
+  results
+
 type
   MiningHeader = object
     parentHash  : Hash256
@@ -54,7 +55,34 @@ const
   CACHE_MAX_ITEMS = 10
 
 # ------------------------------------------------------------------------------
-# Private functions
+# Private Helpers
+# ------------------------------------------------------------------------------
+
+func toMiningHeader(header: BlockHeader): MiningHeader =
+  result.parentHash  = header.parentHash
+  result.ommersHash  = header.ommersHash
+  result.coinbase    = header.coinbase
+  result.stateRoot   = header.stateRoot
+  result.txRoot      = header.txRoot
+  result.receiptRoot = header.receiptRoot
+  result.bloom       = header.bloom
+  result.difficulty  = header.difficulty
+  result.blockNumber = header.blockNumber
+  result.gasLimit    = header.gasLimit
+  result.gasUsed     = header.gasUsed
+  result.timestamp   = header.timestamp
+  result.extraData   = header.extraData
+
+
+func hash(header: MiningHeader): Hash256 =
+  keccakHash(rlp.encode(header))
+
+func isGenesis(header: BlockHeader): bool =
+  header.blockNumber == 0.u256 and
+    header.parentHash == GENESIS_PARENT_HASH
+
+# ------------------------------------------------------------------------------
+# Private cache management functions
 # ------------------------------------------------------------------------------
 
 proc mkCacheBytes(blockNumber: uint64): seq[Hash512] =
@@ -100,67 +128,177 @@ func cacheHash(x: openArray[Hash512]): Hash256 =
   ctx.finish result.data
   ctx.clear()
 
+# ------------------------------------------------------------------------------
+# Pivate validator functions
+# ------------------------------------------------------------------------------
 
-proc checkPOW(cacheByEpoch: CacheByEpoch;
+proc checkPOW(blockNumber: Uint256; miningHash, mixHash: Hash256;
-              blockNumber: Uint256; miningHash, mixHash: Hash256,
+              nonce: BlockNonce; difficulty: DifficultyInt;
-              nonce: BlockNonce, difficulty: DifficultyInt) =
+              cacheByEpoch: CacheByEpoch): Result[void,string] =
-  let blockNumber = blockNumber.truncate(uint64)
+  let
-  let cache = cacheByEpoch.getCache(blockNumber)
+    blockNumber = blockNumber.truncate(uint64)
+    cache = cacheByEpoch.getCache(blockNumber)
+    size = getDataSize(blockNumber)
+    miningOutput = hashimotoLight(
+      size, cache, miningHash, uint64.fromBytesBE(nonce))
 
-  let size = getDataSize(blockNumber)
-  let miningOutput = hashimotoLight(
-    size, cache, miningHash, uint64.fromBytesBE(nonce))
   if miningOutput.mixDigest != mixHash:
-    echo "actual: ", miningOutput.mixDigest
+    debug "mixHash mismatch",
-    echo "expected: ", mixHash
+      actual = miningOutput.mixDigest,
-    echo "blockNumber: ", blockNumber
+      expected = mixHash,
-    echo "miningHash: ", miningHash
+      blockNumber = blockNumber,
-    echo "nonce: ", nonce.toHex
+      miningHash = miningHash,
-    echo "difficulty: ", difficulty
+      nonce = nonce.toHex,
-    echo "size: ", size
+      difficulty = difficulty,
-    echo "cache hash: ", cacheHash(cache)
+      size = size,
-    raise newException(ValidationError, "mixHash mismatch")
+      cachedHash = cacheHash(cache)
+    return err("mixHash mismatch")
 
   let value = Uint256.fromBytesBE(miningOutput.value.data)
   if value > Uint256.high div difficulty:
-    raise newException(ValidationError, "mining difficulty error")
+    return err("mining difficulty error")
+
+  result = ok()
 
 
-func toMiningHeader(header: BlockHeader): MiningHeader =
+proc validateSeal(cacheByEpoch: CacheByEpoch;
-  result.parentHash  = header.parentHash
+                  header: BlockHeader): Result[void,string] =
-  result.ommersHash  = header.ommersHash
-  result.coinbase    = header.coinbase
-  result.stateRoot   = header.stateRoot
-  result.txRoot      = header.txRoot
-  result.receiptRoot = header.receiptRoot
-  result.bloom       = header.bloom
-  result.difficulty  = header.difficulty
-  result.blockNumber = header.blockNumber
-  result.gasLimit    = header.gasLimit
-  result.gasUsed     = header.gasUsed
-  result.timestamp   = header.timestamp
-  result.extraData   = header.extraData
-
-
-func hash(header: MiningHeader): Hash256 =
-  keccakHash(rlp.encode(header))
-
-
-proc validateSeal(cacheByEpoch: CacheByEpoch; header: BlockHeader) =
   let miningHeader = header.toMiningHeader
   let miningHash = miningHeader.hash
 
-  cacheByEpoch.checkPOW(header.blockNumber, miningHash,
+  checkPOW(header.blockNumber, miningHash,
-                        header.mixDigest, header.nonce, header.difficulty)
+           header.mixDigest, header.nonce, header.difficulty, cacheByEpoch)
+
+
+proc validateGasLimit(chainDB: BaseChainDB;
+                      header: BlockHeader): Result[void,string] =
+  let parentHeader = chainDB.getBlockHeader(header.parentHash)
+  let (lowBound, highBound) = gasLimitBounds(parentHeader)
+
+  if header.gasLimit < lowBound:
+    return err("The gas limit is too low")
+  if header.gasLimit > highBound:
+    return err("The gas limit is too high")
+
+  result = ok()
+
+
+func validateGasLimit(gasLimit, parentGasLimit: GasInt): Result[void,string] =
+  if gasLimit < GAS_LIMIT_MINIMUM:
+    return err("Gas limit is below minimum")
+  if gasLimit > GAS_LIMIT_MAXIMUM:
+    return err("Gas limit is above maximum")
+
+  let diff = gasLimit - parentGasLimit
+  if diff > (parentGasLimit div GAS_LIMIT_ADJUSTMENT_FACTOR):
+    return err("Gas limit difference to parent is too big")
+
+  result = ok()
+
+proc validateHeader(header, parentHeader: BlockHeader; checkSealOK: bool;
+                     cacheByEpoch: CacheByEpoch): Result[void,string] =
+  if header.extraData.len > 32:
+    return err("BlockHeader.extraData larger than 32 bytes")
+
+  result = validateGasLimit(header.gasLimit, parentHeader.gasLimit)
+  if result.isErr:
+    return
+
+  if header.blockNumber != parentHeader.blockNumber + 1:
+    return err("Blocks must be numbered consecutively.")
+
+  if header.timestamp.toUnix <= parentHeader.timestamp.toUnix:
+    return err("timestamp must be strictly later than parent")
+
+  if checkSealOK:
+    return cacheByEpoch.validateSeal(header)
+
+  result = ok()
+
+
+func validateUncle(currBlock, uncle, uncleParent: BlockHeader):
+                                               Result[void,string] =
+  if uncle.blockNumber >= currBlock.blockNumber:
+    return err("uncle block number larger than current block number")
+
+  if uncle.blockNumber != uncleParent.blockNumber + 1:
+    return err("Uncle number is not one above ancestor's number")
+
+  if uncle.timestamp.toUnix < uncleParent.timestamp.toUnix:
+    return err("Uncle timestamp is before ancestor's timestamp")
+
+  if uncle.gasUsed > uncle.gasLimit:
+    return err("Uncle's gas usage is above the limit")
+
+  result = ok()
+
+
+proc validateUncles(chainDB: BaseChainDB; header: BlockHeader;
+                    uncles: seq[BlockHeader]; checkSealOK: bool;
+                    cacheByEpoch: CacheByEpoch): Result[void,string] =
+  let hasUncles = uncles.len > 0
+  let shouldHaveUncles = header.ommersHash != EMPTY_UNCLE_HASH
+
+  if not hasUncles and not shouldHaveUncles:
+    # optimization to avoid loading ancestors from DB, since the block has
+    # no uncles
+    return ok()
+  if hasUncles and not shouldHaveUncles:
+    return err("Block has uncles but header suggests uncles should be empty")
+  if shouldHaveUncles and not hasUncles:
+    return err("Header suggests block should have uncles but block has none")
+
+  # Check for duplicates
+  var uncleSet = initHashSet[Hash256]()
+  for uncle in uncles:
+    let uncleHash = uncle.hash
+    if uncleHash in uncleSet:
+      return err("Block contains duplicate uncles")
+    else:
+      uncleSet.incl uncleHash
+
+  let recentAncestorHashes = chainDB.getAncestorsHashes(
+                               MAX_UNCLE_DEPTH + 1, header)
+  let recentUncleHashes = chainDB.getUncleHashes(recentAncestorHashes)
+  let blockHash = header.hash
+
+  for uncle in uncles:
+    let uncleHash = uncle.hash
+
+    if uncleHash == blockHash:
+      return err("Uncle has same hash as block")
+
+    # ensure the uncle has not already been included.
+    if uncleHash in recentUncleHashes:
+      return err("Duplicate uncle")
+
+    # ensure that the uncle is not one of the canonical chain blocks.
+    if uncleHash in recentAncestorHashes:
+      return err("Uncle cannot be an ancestor")
+
+    # ensure that the uncle was built off of one of the canonical chain
+    # blocks.
+    if (uncle.parentHash notin recentAncestorHashes) or
+       (uncle.parentHash == header.parentHash):
+      return err("Uncle's parent is not an ancestor")
+
+    # Now perform VM level validation of the uncle
+    if checkSealOK:
+      result = cacheByEpoch.validateSeal(uncle)
+      if result.isErr:
+        return
+
+    let uncleParent = chainDB.getBlockHeader(uncle.parentHash)
+    result = validateUncle(header, uncle, uncleParent)
+    if result.isErr:
+      return
+
+  result = ok()
 
 # ------------------------------------------------------------------------------
-# Puplic function, extracted from executor
+# Public function, extracted from executor
 # ------------------------------------------------------------------------------
 
-proc newCacheByEpoch*(): CacheByEpoch =
-  newOrderedTable[uint64, seq[Hash512]]()
-
-
 proc validateTransaction*(vmState: BaseVMState, tx: Transaction,
                           sender: EthAddress, fork: Fork): bool =
   let balance = vmState.readOnlyStateDB.getBalance(sender)
@@ -201,156 +339,36 @@ proc validateTransaction*(vmState: BaseVMState, tx: Transaction,
 
   result = true
 
-
 # ------------------------------------------------------------------------------
 # Public functions, extracted from test_blockchain_json
 # ------------------------------------------------------------------------------
 
-func validateGasLimit*(gasLimit, parentGasLimit: GasInt) =
+proc newCacheByEpoch*(): CacheByEpoch =
-  if gasLimit < GAS_LIMIT_MINIMUM:
+  newOrderedTable[uint64, seq[Hash512]]()
-    raise newException(ValidationError, "Gas limit is below minimum")
-  if gasLimit > GAS_LIMIT_MAXIMUM:
-    raise newException(ValidationError, "Gas limit is above maximum")
-  let diff = gasLimit - parentGasLimit
-  if diff > (parentGasLimit div GAS_LIMIT_ADJUSTMENT_FACTOR):
-    raise newException(
-      ValidationError, "Gas limit difference to parent is too big")
 
 
-proc validateHeader*(header, parentHeader: BlockHeader;
+proc validateKinship*(chainDB: BaseChainDB; header: BlockHeader;
-                     checkSeal: bool; cacheByEpoch: CacheByEpoch) =
+                      uncles: seq[BlockHeader]; checkSealOK: bool;
-  if header.extraData.len > 32:
+                      cacheByEpoch: CacheByEpoch): Result[void,string] =
-    raise newException(
+  if header.isGenesis:
-      ValidationError, "BlockHeader.extraData larger than 32 bytes")
+    if header.extraData.len > 32:
+      return err("BlockHeader.extraData larger than 32 bytes")
+    return ok()
 
-  validateGasLimit(header.gasLimit, parentHeader.gasLimit)
-
-  if header.blockNumber != parentHeader.blockNumber + 1:
-    raise newException(
-      ValidationError, "Blocks must be numbered consecutively.")
-
-  if header.timestamp.toUnix <= parentHeader.timestamp.toUnix:
-    raise newException(
-      ValidationError, "timestamp must be strictly later than parent")
-
-  if checkSeal:
-    cacheByEpoch.validateSeal(header)
-
-
-func validateUncle*(currBlock, uncle, uncleParent: BlockHeader) =
-  if uncle.blockNumber >= currBlock.blockNumber:
-    raise newException(
-      ValidationError, "uncle block number larger than current block number")
-
-  if uncle.blockNumber != uncleParent.blockNumber + 1:
-    raise newException(
-      ValidationError, "Uncle number is not one above ancestor's number")
-
-  if uncle.timestamp.toUnix < uncleParent.timestamp.toUnix:
-    raise newException(
-      ValidationError, "Uncle timestamp is before ancestor's timestamp")
-
-  if uncle.gasUsed > uncle.gasLimit:
-    raise newException(ValidationError, "Uncle's gas usage is above the limit")
-
-
-proc validateGasLimit*(chainDB: BaseChainDB, header: BlockHeader) =
   let parentHeader = chainDB.getBlockHeader(header.parentHash)
-  let (lowBound, highBound) = gasLimitBounds(parentHeader)
+  result = header.validateHeader(parentHeader, checkSealOK, cacheByEpoch)
-
+  if result.isErr:
-  if header.gasLimit < lowBound:
-    raise newException(ValidationError, "The gas limit is too low")
-  elif header.gasLimit > highBound:
-    raise newException(ValidationError, "The gas limit is too high")
-
-
-proc validateUncles*(chainDB: BaseChainDB; currBlock: EthBlock;
-                     checkSeal: bool; cacheByEpoch: CacheByEpoch) =
-  let hasUncles = currBlock.uncles.len > 0
-  let shouldHaveUncles = currBlock.header.ommersHash != EMPTY_UNCLE_HASH
-
-  if not hasUncles and not shouldHaveUncles:
-    # optimization to avoid loading ancestors from DB, since the block has
-    # no uncles
     return
-  elif hasUncles and not shouldHaveUncles:
-    raise newException(
-      ValidationError,
-      "Block has uncles but header suggests uncles should be empty")
-  elif shouldHaveUncles and not hasUncles:
-    raise newException(
-      ValidationError,
-      "Header suggests block should have uncles but block has none")
 
-  # Check for duplicates
+  if uncles.len > MAX_UNCLES:
-  var uncleSet = initHashSet[Hash256]()
+    return err("Number of uncles exceed limit.")
-  for uncle in currBlock.uncles:
-    let uncleHash = uncle.hash
-    if uncleHash in uncleSet:
-      raise newException(ValidationError, "Block contains duplicate uncles")
-    else:
-      uncleSet.incl uncleHash
 
-  let recentAncestorHashes = chainDB.getAncestorsHashes(
+  if not chainDB.exists(header.stateRoot):
-                               MAX_UNCLE_DEPTH + 1, currBlock.header)
+    return err("`state_root` was not found in the db.")
-  let recentUncleHashes = chainDB.getUncleHashes(recentAncestorHashes)
-  let blockHash =currBlock.header.hash
 
-  for uncle in currBlock.uncles:
+  result = chainDB.validateUncles(header, uncles, checkSealOK, cacheByEpoch)
-    let uncleHash = uncle.hash
+  if result.isOk:
-
+    result = chainDB.validateGaslimit(header)
-    if uncleHash == blockHash:
-      raise newException(ValidationError, "Uncle has same hash as block")
-
-    # ensure the uncle has not already been included.
-    if uncleHash in recentUncleHashes:
-      raise newException(ValidationError, "Duplicate uncle")
-
-    # ensure that the uncle is not one of the canonical chain blocks.
-    if uncleHash in recentAncestorHashes:
-      raise newException(ValidationError, "Uncle cannot be an ancestor")
-
-    # ensure that the uncle was built off of one of the canonical chain
-    # blocks.
-    if (uncle.parentHash notin recentAncestorHashes) or
-       (uncle.parentHash == currBlock.header.parentHash):
-      raise newException(ValidationError, "Uncle's parent is not an ancestor")
-
-    # Now perform VM level validation of the uncle
-    if checkSeal:
-      cacheByEpoch.validateSeal(uncle)
-
-    let uncleParent = chainDB.getBlockHeader(uncle.parentHash)
-    validateUncle(currBlock.header, uncle, uncleParent)
-
-
-func isGenesis*(currBlock: EthBlock): bool =
-  result = currBlock.header.blockNumber == 0.u256 and
-           currBlock.header.parentHash == GENESIS_PARENT_HASH
-
-
-proc validateBlock*(chainDB: BaseChainDB; currBlock: EthBlock;
-                    checkSeal: bool; cacheByEpoch: CacheByEpoch): bool =
-  if currBlock.isGenesis:
-    if currBlock.header.extraData.len > 32:
-      raise newException(
-        ValidationError, "BlockHeader.extraData larger than 32 bytes")
-    return true
-
-  let parentHeader = chainDB.getBlockHeader(currBlock.header.parentHash)
-  validateHeader(currBlock.header, parentHeader, checkSeal, cacheByEpoch)
-
-  if currBlock.uncles.len > MAX_UNCLES:
-    raise newException(ValidationError, "Number of uncles exceed limit.")
-
-  if not chainDB.exists(currBlock.header.stateRoot):
-    raise newException(
-      ValidationError, "`state_root` was not found in the db.")
-
-  validateUncles(chainDB, currBlock, checkSeal, cacheByEpoch)
-  validateGaslimit(chainDB, currBlock.header)
-
-  result = true
 
 # ------------------------------------------------------------------------------
 # End

tests/test_blockchain_json.nim

@@ -216,6 +216,13 @@ proc blockWitness(vmState: BaseVMState, chainDB: BaseChainDB) =
 func validateBlockUnchanged(a, b: EthBlock): bool =
   result = rlp.encode(a) == rlp.encode(b)
 
+proc validateBlock(chainDB: BaseChainDB;
+                   ethBlock: EthBlock; checkSealOK: bool): bool =
+  let rc = chainDB.validateKinship(
+    ethBlock.header, ethBlock.uncles, checkSealOK, cacheByEpoch)
+  if rc.isErr:
+    debugEcho "invalid block: " & rc.error
+  rc.isOk
 
 proc importBlock(tester: var Tester, chainDB: BaseChainDB,
   preminedBlock: EthBlock, tb: TestBlock, checkSeal, validation: bool): EthBlock =
@@ -252,7 +259,7 @@ proc importBlock(tester: var Tester, chainDB: BaseChainDB,
   if validation:
     if not validateBlockUnchanged(result, preminedBlock):
       raise newException(ValidationError, "block changed")
-    if not validateBlock(chainDB, result, checkSeal, cacheByEpoch):
+    if not validateBlock(chainDB, result, checkSeal):
       raise newException(ValidationError, "invalid block")
 
   discard chainDB.persistHeaderToDb(preminedBlock.header)
@@ -297,8 +304,7 @@ proc runTester(tester: var Tester, chainDB: BaseChainDB, testStatusIMPL: var Tes
       try:
         let (preminedBlock, _, _) = tester.applyFixtureBlockToChain(
             testBlock, chainDB, checkSeal, validation = false)  # we manually validate below
-        check validateBlock(
+        check validateBlock(chainDB, preminedBlock, checkSeal) == true
-          chainDB, preminedBlock, checkSeal, cacheByEpoch) == true
       except:
         debugEcho "FATAL ERROR(WE HAVE BUG): ", getCurrentExceptionMsg()