Jordan/fix some failing nohive tests (#727)

* continue importing rlp blocks

why:
  a chain of blocks to be imported might have legit blocks
  after rejected blocks

details:
  import loop only stops if the import list is exhausted or if there
  was a decoding error. this adds another four to the count of successful
  no-hive tests.

* verify DAO marked extra data field in block header

why:
  was ignored, scores another two no-hive tests

* verify minimum required difficulty in header validator

why:
  two more nohive tests to succeed

details:
  * subsumed extended header tests under validateKinship() and renamed it
    more appropriately validateHeaderAndKinship()
  * enhanced readability of p2p/chain.nim
  * cleaned up test_blockchain_json.nim

* verify positive gasUsed unless no transactions

why:
  solves another to nohive tests

details:
  straightened test_blockchain_json chech so there is no unconditional
  rejection anymore (based on the input test  scenario)

nimbus/conf_utils.nim

@@ -22,27 +22,40 @@ type
 proc importRlpBlock*(importFile: string; chainDB: BasechainDB): bool =
   let res = io2.readAllBytes(importFile)
   if res.isErr:
-    error "failed to import", fileName = importFile
+    error "failed to import",
+      fileName = importFile
     return false
 
-  var chain = newChain(chainDB, extraValidation = true)
+  var
     # the encoded rlp can contains one or more blocks
-  var rlp = rlpFromBytes(res.get)
+    rlp = rlpFromBytes(res.get)
-  let head = chainDB.getCanonicalHead()
+    chain = newChain(chainDB, extraValidation = true)
+    errorCount = 0
+  let
+    head = chainDB.getCanonicalHead()
 
+  while rlp.hasData:
     try:
-    while true:
+      let
-      let header = rlp.read(EthHeader).header
+        header = rlp.read(EthHeader).header
-      let body = rlp.readRecordType(BlockBody, false)
+        body = rlp.readRecordType(BlockBody, false)
       if header.blockNumber > head.blockNumber:
-        let valid = chain.persistBlocks([header], [body])
+        if chain.persistBlocks([header], [body]) == ValidationResult.Error:
-        if valid == ValidationResult.Error:
+          # register one more error and continue
-          error "failed to import rlp encoded blocks", fileName = importFile
+          errorCount.inc
+    except RlpError as e:
+      # terminate if there was a decoding error
+      error "rlp error",
+        fileName = importFile,
+        msg = e.msg,
+        exception = e.name
       return false
-      if not rlp.hasData:
-        break
     except CatchableError as e:
-    error "rlp error", fileName = importFile, msg = e.msg, exception = e.name
+      # otherwise continue
-    return false
+      error "import error",
+        fileName = importFile,
+        msg = e.msg,
+        exception = e.name
+      errorCount.inc
 
-  return true
+  return errorCount == 0

nimbus/p2p/chain.nim

@@ -3,6 +3,7 @@ import
   ../db/db_chain,
   ../genesis,
   ../utils,
+  ../utils/difficulty,
   ../vm_state,
   ./executor,
   ./validate,
@@ -157,38 +158,39 @@ method persistBlocks*(c: Chain; headers: openarray[BlockHeader];
 
   for i in 0 ..< headers.len:
     let
-      head = c.db.getBlockHeader(headers[i].parentHash)
+      (header, body) = (headers[i], bodies[i])
-      vmState = newBaseVMState(head.stateRoot, headers[i], c.db)
+      parentHeader = c.db.getBlockHeader(header.parentHash)
-      validationResult = processBlock(c.db, headers[i], bodies[i], vmState)
+      vmState = newBaseVMState(parentHeader.stateRoot, header, c.db)
+      validationResult = processBlock(c.db, header, body, vmState)
 
     when not defined(release):
       if validationResult == ValidationResult.Error and
-          bodies[i].transactions.calcTxRoot == headers[i].txRoot:
+         body.transactions.calcTxRoot == header.txRoot:
-        dumpDebuggingMetaData(c.db, headers[i], bodies[i], vmState)
+        dumpDebuggingMetaData(c.db, header, body, vmState)
         warn "Validation error. Debugging metadata dumped."
 
     if validationResult != ValidationResult.OK:
       return validationResult
 
     if c.extraValidation:
-      let res = validateKinship(
+      let res = c.db.validateHeaderAndKinship(
-        c.db, headers[i],
+        header,
-        bodies[i].uncles,
+        body,
         checkSealOK = false, # TODO: how to checkseal from here
         c.cacheByEpoch
       )
       if res.isErr:
-        debug "kinship validation error", msg = res.error
+        debug "block validation error", msg = res.error
         return ValidationResult.Error
 
-    discard c.db.persistHeaderToDb(headers[i])
+    discard c.db.persistHeaderToDb(header)
-    discard c.db.persistTransactions(headers[i].blockNumber, bodies[i].transactions)
+    discard c.db.persistTransactions(header.blockNumber, body.transactions)
     discard c.db.persistReceipts(vmState.receipts)
 
     # update currentBlock *after* we persist it
     # so the rpc return consistent result
     # between eth_blockNumber and eth_syncing
-    c.db.currentBlock = headers[i].blockNumber
+    c.db.currentBlock = header.blockNumber
 
   transaction.commit()
 

nimbus/p2p/validate.nim

@@ -9,31 +9,36 @@
 # according to those terms.
 
 import
+  std/[sequtils, sets, tables, times],
   ../constants,
   ../db/[db_chain, accounts_cache],
   ../transaction,
   ../utils,
-  ../utils/header,
+  ../utils/[difficulty, header],
   ../vm_state,
   ../vm_types,
   ../forks,
+  ./dao,
   ./validate/epoch_hash_cache,
   chronicles,
   eth/[common, rlp, trie/trie_defs],
   ethash,
   nimcrypto,
   options,
-  sets,
+  stew/[results, endians2]
-  stew/[results, endians2],
+
-  strutils,
+from stew/byteutils
-  tables,
+  import nil
-  times
 
 export
   epoch_hash_cache.EpochHashCache,
   epoch_hash_cache.initEpochHashCache,
   results
 
+const
+  daoForkBlockExtraData =
+    byteutils.hexToByteArray[13](DAOForkBlockExtra).toSeq
+
 type
   MiningHeader = object
     parentHash  : Hash256
@@ -162,7 +167,8 @@ func validateGasLimit(gasLimit, parentGasLimit: GasInt): Result[void,string] =
 
   result = ok()
 
-proc validateHeader(header, parentHeader: BlockHeader; checkSealOK: bool;
+proc validateHeader(db: BaseChainDB; header, parentHeader: BlockHeader;
+                    numTransactions: int; checkSealOK: bool;
                     hashCache: var EpochHashCache): Result[void,string] =
   if header.extraData.len > 32:
     return err("BlockHeader.extraData larger than 32 bytes")
@@ -171,12 +177,24 @@ proc validateHeader(header, parentHeader: BlockHeader; checkSealOK: bool;
   if result.isErr:
     return
 
+  if header.gasUsed == 0 and 0 < numTransactions:
+    return err("zero gasUsed but tranactions present");
+
   if header.blockNumber != parentHeader.blockNumber + 1:
-    return err("Blocks must be numbered consecutively.")
+    return err("Blocks must be numbered consecutively")
 
   if header.timestamp.toUnix <= parentHeader.timestamp.toUnix:
     return err("timestamp must be strictly later than parent")
 
+  if db.config.daoForkSupport and
+     db.config.daoForkBlock <= header.blockNumber and
+     header.extraData != daoForkBlockExtraData:
+    return err("header extra data should be marked DAO")
+
+  let calcDiffc = db.config.calcDifficulty(header.timestamp, parentHeader)
+  if header.difficulty < calcDiffc:
+    return err("provided header difficulty is too low")
+
   if checkSealOK:
     return hashCache.validateSeal(header)
 
@@ -317,8 +335,8 @@ proc validateTransaction*(vmState: BaseVMState, tx: Transaction,
 # Public functions, extracted from test_blockchain_json
 # ------------------------------------------------------------------------------
 
-proc validateKinship*(chainDB: BaseChainDB; header: BlockHeader;
+proc validateHeaderAndKinship*(chainDB: BaseChainDB; header: BlockHeader;
-                      uncles: seq[BlockHeader]; checkSealOK: bool;
+            uncles: seq[BlockHeader]; numTransactions: int; checkSealOK: bool;
             hashCache: var EpochHashCache): Result[void,string] =
   if header.isGenesis:
     if header.extraData.len > 32:
@@ -326,7 +344,8 @@ proc validateKinship*(chainDB: BaseChainDB; header: BlockHeader;
     return ok()
 
   let parentHeader = chainDB.getBlockHeader(header.parentHash)
-  result = header.validateHeader(parentHeader, checkSealOK, hashCache)
+  result = chainDB.validateHeader(
+    header, parentHeader,numTransactions,  checkSealOK, hashCache)
   if result.isErr:
     return
 
@@ -340,6 +359,19 @@ proc validateKinship*(chainDB: BaseChainDB; header: BlockHeader;
   if result.isOk:
     result = chainDB.validateGaslimit(header)
 
+
+proc validateHeaderAndKinship*(chainDB: BaseChainDB;
+                      header: BlockHeader; body: BlockBody; checkSealOK: bool;
+                      hashCache: var EpochHashCache): Result[void,string] =
+  chainDB.validateHeaderAndKinship(
+    header, body.uncles, body.transactions.len, checkSealOK, hashCache)
+
+
+proc validateHeaderAndKinship*(chainDB: BaseChainDB; ethBlock: EthBlock;
+        checkSealOK: bool; hashCache: var EpochHashCache): Result[void,string] =
+  chainDB.validateHeaderAndKinship(
+    ethBlock.header, ethBlock.uncles, ethBlock.txs.len, checkSealOK, hashCache)
+
 # ------------------------------------------------------------------------------
 # End
 # ------------------------------------------------------------------------------

tests/test_blockchain_json.nim

@@ -215,17 +215,6 @@ proc blockWitness(vmState: BaseVMState, chainDB: BaseChainDB) =
   if root != rootHash:
     raise newException(ValidationError, "Invalid trie generated from block witness")
 
-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 =
 
@@ -254,15 +243,12 @@ proc importBlock(tester: var Tester, chainDB: BaseChainDB,
     if tester.vmState.generateWitness():
       blockWitness(tester.vmState, chainDB)
 
-  result.header.stateRoot  = tester.vmState.blockHeader.stateRoot
-  result.header.parentHash = parentHeader.hash
-  result.header.difficulty = baseHeaderForImport.difficulty
-
   if validation:
-    if not validateBlockUnchanged(result, preminedBlock):
+    let rc = chainDB.validateHeaderAndKinship(
-      raise newException(ValidationError, "block changed")
+      result.header, body, checkSeal, cacheByEpoch)
-    if not validateBlock(chainDB, result, checkSeal):
+    if rc.isErr:
-      raise newException(ValidationError, "invalid block")
+      raise newException(
+        ValidationError, "validateHeaderAndKinship: " & rc.error)
 
   discard chainDB.persistHeaderToDb(preminedBlock.header)
 
@@ -305,8 +291,12 @@ proc runTester(tester: var Tester, chainDB: BaseChainDB, testStatusIMPL: var Tes
     if testBlock.goodBlock:
       try:
         let (preminedBlock, _, _) = tester.applyFixtureBlockToChain(
-            testBlock, chainDB, checkSeal, validation = false)  # we manually validate below
+            testBlock, chainDB, checkSeal, validation = false)
-        check validateBlock(chainDB, preminedBlock, checkSeal) == true
+
+        # manually validating
+        check chainDB.validateHeaderAndKinship(
+          preminedBlock, checkSeal, cacheByEpoch).isOk
+
       except:
         debugEcho "FATAL ERROR(WE HAVE BUG): ", getCurrentExceptionMsg()