From a49a812879aea22c6ba165c203a2631b432e8603 Mon Sep 17 00:00:00 2001 From: Jordan Hrycaj Date: Thu, 24 Jun 2021 16:29:21 +0100 Subject: [PATCH] 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 | 51 +++++++++++++++++++----------- nimbus/p2p/chain.nim | 26 ++++++++------- nimbus/p2p/validate.nim | 58 ++++++++++++++++++++++++++-------- tests/test_blockchain_json.nim | 32 +++++++------------ 4 files changed, 102 insertions(+), 65 deletions(-) diff --git a/nimbus/conf_utils.nim b/nimbus/conf_utils.nim index ab2aa090c..f3e93ef0e 100644 --- a/nimbus/conf_utils.nim +++ b/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) - # the encoded rlp can contains one or more blocks - var rlp = rlpFromBytes(res.get) - let head = chainDB.getCanonicalHead() + var + # the encoded rlp can contains one or more blocks + rlp = rlpFromBytes(res.get) + chain = newChain(chainDB, extraValidation = true) + errorCount = 0 + let + head = chainDB.getCanonicalHead() - try: - while true: - let header = rlp.read(EthHeader).header - let body = rlp.readRecordType(BlockBody, false) + while rlp.hasData: + try: + let + header = rlp.read(EthHeader).header + body = rlp.readRecordType(BlockBody, false) if header.blockNumber > head.blockNumber: - let valid = chain.persistBlocks([header], [body]) - if valid == ValidationResult.Error: - error "failed to import rlp encoded blocks", fileName = importFile - return false - if not rlp.hasData: - break - except CatchableError as e: - error "rlp error", fileName = importFile, msg = e.msg, exception = e.name - return false + if chain.persistBlocks([header], [body]) == ValidationResult.Error: + # register one more error and continue + 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 + except CatchableError as e: + # otherwise continue + error "import error", + fileName = importFile, + msg = e.msg, + exception = e.name + errorCount.inc - return true + return errorCount == 0 diff --git a/nimbus/p2p/chain.nim b/nimbus/p2p/chain.nim index 7d7fc2c7b..ff1d101e8 100644 --- a/nimbus/p2p/chain.nim +++ b/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) - vmState = newBaseVMState(head.stateRoot, headers[i], c.db) - validationResult = processBlock(c.db, headers[i], bodies[i], vmState) + (header, body) = (headers[i], bodies[i]) + parentHeader = c.db.getBlockHeader(header.parentHash) + 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: - dumpDebuggingMetaData(c.db, headers[i], bodies[i], vmState) + body.transactions.calcTxRoot == header.txRoot: + dumpDebuggingMetaData(c.db, header, body, vmState) warn "Validation error. Debugging metadata dumped." if validationResult != ValidationResult.OK: return validationResult if c.extraValidation: - let res = validateKinship( - c.db, headers[i], - bodies[i].uncles, + let res = c.db.validateHeaderAndKinship( + header, + 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.persistTransactions(headers[i].blockNumber, bodies[i].transactions) + discard c.db.persistHeaderToDb(header) + 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() diff --git a/nimbus/p2p/validate.nim b/nimbus/p2p/validate.nim index 70a2e2cfe..779f6c680 100644 --- a/nimbus/p2p/validate.nim +++ b/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], - strutils, - tables, - times + stew/[results, endians2] + +from stew/byteutils + import nil export epoch_hash_cache.EpochHashCache, epoch_hash_cache.initEpochHashCache, results +const + daoForkBlockExtraData = + byteutils.hexToByteArray[13](DAOForkBlockExtra).toSeq + type MiningHeader = object parentHash : Hash256 @@ -162,8 +167,9 @@ func validateGasLimit(gasLimit, parentGasLimit: GasInt): Result[void,string] = result = ok() -proc validateHeader(header, parentHeader: BlockHeader; checkSealOK: bool; - hashCache: var EpochHashCache): Result[void,string] = +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,16 +335,17 @@ proc validateTransaction*(vmState: BaseVMState, tx: Transaction, # Public functions, extracted from test_blockchain_json # ------------------------------------------------------------------------------ -proc validateKinship*(chainDB: BaseChainDB; header: BlockHeader; - uncles: seq[BlockHeader]; checkSealOK: bool; - hashCache: var EpochHashCache): Result[void,string] = +proc validateHeaderAndKinship*(chainDB: BaseChainDB; header: BlockHeader; + uncles: seq[BlockHeader]; numTransactions: int; checkSealOK: bool; + hashCache: var EpochHashCache): Result[void,string] = if header.isGenesis: if header.extraData.len > 32: return err("BlockHeader.extraData larger than 32 bytes") 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 # ------------------------------------------------------------------------------ diff --git a/tests/test_blockchain_json.nim b/tests/test_blockchain_json.nim index 3e2dd1ea9..cd25cb454 100644 --- a/tests/test_blockchain_json.nim +++ b/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): - raise newException(ValidationError, "block changed") - if not validateBlock(chainDB, result, checkSeal): - raise newException(ValidationError, "invalid block") + let rc = chainDB.validateHeaderAndKinship( + result.header, body, checkSeal, cacheByEpoch) + if rc.isErr: + 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 - check validateBlock(chainDB, preminedBlock, checkSeal) == true + testBlock, chainDB, checkSeal, validation = false) + + # manually validating + check chainDB.validateHeaderAndKinship( + preminedBlock, checkSeal, cacheByEpoch).isOk + except: debugEcho "FATAL ERROR(WE HAVE BUG): ", getCurrentExceptionMsg()