From 2b4baff8ec6329cd0bc8afb7e05ea8fc8c7445c5 Mon Sep 17 00:00:00 2001 From: jangko Date: Mon, 4 Jul 2022 19:31:41 +0700 Subject: [PATCH] move block validation from execution payload generator to engine api --- hive_integration/nodocker/engine/clmock.nim | 7 +-- .../nodocker/engine/engine_tests.nim | 20 +++---- nimbus/db/db_chain.nim | 50 ++++++++--------- nimbus/genesis.nim | 1 - nimbus/nimbus.nim | 2 +- nimbus/p2p/chain/chain_desc.nim | 8 +-- nimbus/p2p/chain/persist_blocks.nim | 54 ++++++++++++++++--- nimbus/rpc/engine_api.nim | 26 ++++++--- nimbus/sealer.nim | 9 ---- tests/test_custom_network.nim | 2 +- 10 files changed, 109 insertions(+), 70 deletions(-) diff --git a/hive_integration/nodocker/engine/clmock.nim b/hive_integration/nodocker/engine/clmock.nim index 703112eca..319eb1777 100644 --- a/hive_integration/nodocker/engine/clmock.nim +++ b/hive_integration/nodocker/engine/clmock.nim @@ -80,13 +80,14 @@ proc waitForTTD*(cl: CLMocker): Future[bool] {.async.} = let res = cl.client.forkchoiceUpdatedV1(cl.latestForkchoice) if res.isErr: - error "forkchoiceUpdated error", msg=res.error + error "waitForTTD: forkchoiceUpdated error", msg=res.error return false let s = res.get() if s.payloadStatus.status != PayloadExecutionStatus.valid: - error "forkchoiceUpdated response", - status=s.payloadStatus.status + error "waitForTTD: forkchoiceUpdated response unexpected", + expect = PayloadExecutionStatus.valid, + get = s.payloadStatus.status return false return true diff --git a/hive_integration/nodocker/engine/engine_tests.nim b/hive_integration/nodocker/engine/engine_tests.nim index 220e1da04..580ef55b4 100644 --- a/hive_integration/nodocker/engine/engine_tests.nim +++ b/hive_integration/nodocker/engine/engine_tests.nim @@ -417,15 +417,11 @@ template invalidPayloadAttributesGen(procname: untyped, syncingCond: bool) = when syncingCond: # If we are SYNCING, the outcome should be SYNCING regardless of the validity of the payload atttributes let r = client.forkchoiceUpdatedV1(fcu, some(attr)) - let s = r.get() - if s.payloadStatus.status != PayloadExecutionStatus.syncing: - return false - if s.payloadId.isSome: - return false + testFCU(r, syncing) else: let r = client.forkchoiceUpdatedV1(fcu, some(attr)) - if r.isOk: - return false + testCond r.isOk: + error "Unexpected error", msg = r.error # Check that the forkchoice was applied, regardless of the error testLatestHeader(client, BlockHash blockHash.data) @@ -1326,8 +1322,7 @@ proc reorgBack(t: TestEnv): TestStatus = let clMock = t.clMock let client = t.rpcClient - let r1 = clMock.produceSingleBlock(BlockProcessCallbacks()) - testCond r1 + testCond clMock.produceSingleBlock(BlockProcessCallbacks()) # We are going to reorg back to this previous hash several times let previousHash = clMock.latestForkchoice.headBlockHash @@ -1344,9 +1339,8 @@ proc reorgBack(t: TestEnv): TestStatus = # It is only expected that the client does not produce an error and the CL Mocker is able to progress after the re-org let r = client.forkchoiceUpdatedV1(forkchoiceUpdatedBack) - if r.isErr: + testCond r.isOk: error "failed to reorg back", msg = r.error - return false return true )) testCond r2 @@ -1966,11 +1960,11 @@ const engineTestList* = [ ), # Eth RPC Status on ForkchoiceUpdated Events - TestSpec( # TODO: fix/debug + TestSpec( name: "Latest Block after NewPayload", run: blockStatusExecPayload1, ), - TestSpec( # TODO: fix/debug + TestSpec( name: "Latest Block after NewPayload (Transition Block)", run: blockStatusExecPayload2, ttd: 5, diff --git a/nimbus/db/db_chain.nim b/nimbus/db/db_chain.nim index 2737a757c..a93cca01f 100644 --- a/nimbus/db/db_chain.nim +++ b/nimbus/db/db_chain.nim @@ -19,7 +19,6 @@ type networkId*: NetworkId config* : ChainConfig genesis* : Genesis - totalDifficulty*: DifficultyInt # startingBlock, currentBlock, and highestBlock # are progress indicator @@ -31,16 +30,6 @@ type blockNumber: BlockNumber index: int -proc getTotalDifficulty*(self: BaseChainDB): UInt256 = - # this is actually a combination of `getHash` and `getScore` - const key = canonicalHeadHashKey() - let data = self.db.get(key.toOpenArray) - if data.len == 0: - return 0.u256 - - let blockHash = rlp.decode(data, Hash256) - rlp.decode(self.db.get(blockHashToScoreKey(blockHash).toOpenArray), UInt256) - proc newBaseChainDB*( db: TrieDatabaseRef, pruneTrie: bool = true, @@ -53,7 +42,6 @@ proc newBaseChainDB*( result.networkId = id result.config = params.config result.genesis = params.genesis - result.totalDifficulty = result.getTotalDifficulty() proc `$`*(db: BaseChainDB): string = result = "BaseChainDB" @@ -165,6 +153,16 @@ proc getTd*(self: BaseChainDB; blockHash: Hash256, td: var UInt256): bool = return false return true +proc headTotalDifficulty*(self: BaseChainDB): UInt256 = + # this is actually a combination of `getHash` and `getScore` + const key = canonicalHeadHashKey() + let data = self.db.get(key.toOpenArray) + if data.len == 0: + return 0.u256 + + let blockHash = rlp.decode(data, Hash256) + rlp.decode(self.db.get(blockHashToScoreKey(blockHash).toOpenArray), UInt256) + proc getAncestorsHashes*(self: BaseChainDB, limit: UInt256, header: BlockHeader): seq[Hash256] = var ancestorCount = min(header.blockNumber, limit).truncate(int) var h = header @@ -258,21 +256,24 @@ proc getUncles*(self: BaseChainDB, ommersHash: Hash256): seq[BlockHeader] = if encodedUncles.len != 0: result = rlp.decode(encodedUncles, seq[BlockHeader]) +proc getBlockBody*(self: BaseChainDB, header: BlockHeader, output: var BlockBody): bool = + result = true + output.transactions = @[] + output.uncles = @[] + for encodedTx in self.getBlockTransactionData(header.txRoot): + output.transactions.add(rlp.decode(encodedTx, Transaction)) + + if header.ommersHash != EMPTY_UNCLE_HASH: + let encodedUncles = self.db.get(genericHashKey(header.ommersHash).toOpenArray) + if encodedUncles.len != 0: + output.uncles = rlp.decode(encodedUncles, seq[BlockHeader]) + else: + result = false + proc getBlockBody*(self: BaseChainDB, blockHash: Hash256, output: var BlockBody): bool = var header: BlockHeader if self.getBlockHeader(blockHash, header): - result = true - output.transactions = @[] - output.uncles = @[] - for encodedTx in self.getBlockTransactionData(header.txRoot): - output.transactions.add(rlp.decode(encodedTx, Transaction)) - - if header.ommersHash != EMPTY_UNCLE_HASH: - let encodedUncles = self.db.get(genericHashKey(header.ommersHash).toOpenArray) - if encodedUncles.len != 0: - output.uncles = rlp.decode(encodedUncles, seq[BlockHeader]) - else: - result = false + return self.getBlockBody(header, output) proc getBlockBody*(self: BaseChainDB, hash: Hash256): BlockBody = if not self.getBlockBody(hash, result): @@ -465,7 +466,6 @@ proc persistHeaderToDb*(self: BaseChainDB; header: BlockHeader): seq[BlockHeader self.writeTerminalHash(headerHash) if score > headScore: - self.totalDifficulty = score result = self.setAsCanonicalChainHead(headerHash) proc persistHeaderToDbWithoutSetHead*(self: BaseChainDB; header: BlockHeader) = diff --git a/nimbus/genesis.nim b/nimbus/genesis.nim index 322a48fa3..d6523abb2 100644 --- a/nimbus/genesis.nim +++ b/nimbus/genesis.nim @@ -113,7 +113,6 @@ proc initializeEmptyDb*(cdb: BaseChainDB) let header = cdb.toGenesisHeader(sdb) doAssert(header.blockNumber.isZero, "can't commit genesis block with number > 0") # faster lookup of curent total difficulty - cdb.totalDifficulty = header.difficulty discard cdb.persistHeaderToDb(header) # ------------------------------------------------------------------------------ diff --git a/nimbus/nimbus.nim b/nimbus/nimbus.nim index 1b6590d53..99fa21c09 100644 --- a/nimbus/nimbus.nim +++ b/nimbus/nimbus.nim @@ -247,7 +247,7 @@ proc localServices(nimbus: NimbusNode, conf: NimbusConf, # always create sealing engine instanca but not always run it # e.g. engine api need sealing engine without it running var initialState = EngineStopped - if chainDB.totalDifficulty > chainDB.ttd: + if chainDB.headTotalDifficulty() > chainDB.ttd: initialState = EnginePostMerge nimbus.sealingEngine = SealingEngineRef.new( nimbus.chainRef, nimbus.ctx, conf.engineSigner, diff --git a/nimbus/p2p/chain/chain_desc.nim b/nimbus/p2p/chain/chain_desc.nim index b2cc4d861..4b169740f 100644 --- a/nimbus/p2p/chain/chain_desc.nim +++ b/nimbus/p2p/chain/chain_desc.nim @@ -84,17 +84,19 @@ func toNextFork(n: Option[BlockNumber]): uint64 = else: 0'u64 -func isBlockAfterTtd*(c: Chain, blockHeader: BlockHeader): bool = +proc isBlockAfterTtd*(c: Chain, header: BlockHeader): bool + {.gcsafe, raises: [Defect,CatchableError].} = let ttd = c.db.ttd - totalDifficulty = c.db.totalDifficulty + blockHeader.difficulty + ptd = c.db.getScore(header.parentHash) + td = ptd + header.difficulty # c.db.totalDifficulty is parent.totalDifficulty # TerminalBlock is defined as header.totalDifficulty >= TTD # and parent.totalDifficulty < TTD # So blockAfterTTD must be both header.totalDifficulty >= TTD # and parent.totalDifficulty >= TTD - c.db.totalDifficulty >= ttd and totalDifficulty >= ttd + ptd >= ttd and td >= ttd func getNextFork(c: ChainConfig, fork: ChainFork): uint64 = let next: array[ChainFork, uint64] = [ diff --git a/nimbus/p2p/chain/persist_blocks.nim b/nimbus/p2p/chain/persist_blocks.nim index a3326c72b..e339f0d72 100644 --- a/nimbus/p2p/chain/persist_blocks.nim +++ b/nimbus/p2p/chain/persist_blocks.nim @@ -27,6 +27,14 @@ when not defined(release): ../../tracer, ../../utils +type + PersistBlockFlag = enum + NoPersistHeader + NoSaveTxs + NoSaveReceipts + + PersistBlockFlags = set[PersistBlockFlag] + {.push raises: [Defect].} # ------------------------------------------------------------------------------ @@ -34,7 +42,8 @@ when not defined(release): # ------------------------------------------------------------------------------ proc persistBlocksImpl(c: Chain; headers: openArray[BlockHeader]; - bodies: openArray[BlockBody], setHead: bool = true): ValidationResult + bodies: openArray[BlockBody], + flags: PersistBlockFlags = {}): ValidationResult # wildcard exception, wrapped below in public section {.inline, raises: [Exception].} = c.db.highestBlock = headers[^1].blockNumber @@ -98,13 +107,14 @@ proc persistBlocksImpl(c: Chain; headers: openArray[BlockHeader]; msg = res.error return ValidationResult.Error - if setHead: + if NoPersistHeader notin flags: discard c.db.persistHeaderToDb(header) - else: - c.db.persistHeaderToDbWithoutSetHead(header) - discard c.db.persistTransactions(header.blockNumber, body.transactions) - discard c.db.persistReceipts(vmState.receipts) + if NoSaveTxs notin flags: + discard c.db.persistTransactions(header.blockNumber, body.transactions) + + if NoSaveReceipts notin flags: + discard c.db.persistReceipts(vmState.receipts) # update currentBlock *after* we persist it # so the rpc return consistent result @@ -122,7 +132,37 @@ proc insertBlockWithoutSetHead*(c: Chain, header: BlockHeader, {.gcsafe, raises: [Defect,CatchableError].} = safeP2PChain("persistBlocks"): - result = c.persistBlocksImpl([header], [body], setHead = false) + result = c.persistBlocksImpl([header], [body], {NoPersistHeader, NoSaveReceipts}) + if result == ValidationResult.OK: + c.db.persistHeaderToDbWithoutSetHead(header) + +proc setCanonical*(c: Chain, header: BlockHeader): ValidationResult + {.gcsafe, raises: [Defect,CatchableError].} = + + if header.parentHash == Hash256(): + discard c.db.setHead(header.blockHash) + return ValidationResult.OK + + var body: BlockBody + if not c.db.getBlockBody(header, body): + debug "Failed to get BlockBody", + hash = header.blockHash + return ValidationResult.Error + + safeP2PChain("persistBlocks"): + result = c.persistBlocksImpl([header], [body], {NoPersistHeader, NoSaveTxs}) + if result == ValidationResult.OK: + discard c.db.setHead(header.blockHash) + +proc setCanonical*(c: Chain, blockHash: Hash256): ValidationResult + {.gcsafe, raises: [Defect,CatchableError].} = + var header: BlockHeader + if not c.db.getBlockHeader(blockHash, header): + debug "Failed to get BlockHeader", + hash = blockHash + return ValidationResult.Error + + setCanonical(c, header) # ------------------------------------------------------------------------------ # Public `AbstractChainDB` overload method diff --git a/nimbus/rpc/engine_api.nim b/nimbus/rpc/engine_api.nim index 9b9aa7bc5..fc0f7067a 100644 --- a/nimbus/rpc/engine_api.nim +++ b/nimbus/rpc/engine_api.nim @@ -16,7 +16,8 @@ import ".."/db/db_chain, ".."/p2p/chain/[chain_desc, persist_blocks], ".."/[sealer, constants], - ".."/merge/[mergetypes, mergeutils] + ".."/merge/[mergetypes, mergeutils], + ".."/utils/tx_pool proc latestValidHash(db: BaseChainDB, parent: EthBlockHeader, ttd: DifficultyInt): Hash256 = let ptd = db.getScore(parent.parentHash) @@ -27,6 +28,14 @@ proc latestValidHash(db: BaseChainDB, parent: EthBlockHeader, ttd: DifficultyInt # latestValidHash MUST be set to ZERO Hash256() +proc invalidFCU(db: BaseChainDB, header: EthBlockHeader): ForkchoiceUpdatedResponse = + var parent: EthBlockHeader + if not db.getBlockHeader(header.parentHash, parent): + return invalidFCU(Hash256()) + + let blockHash = latestValidHash(db, parent, db.ttd()) + invalidFCU(blockHash) + proc setupEngineAPI*( sealingEngine: SealingEngineRef, server: RpcServer) = @@ -190,7 +199,8 @@ proc setupEngineAPI*( update: ForkchoiceStateV1, payloadAttributes: Option[PayloadAttributesV1]) -> ForkchoiceUpdatedResponse: let - db = sealingEngine.chain.db + chain = sealingEngine.chain + db = chain.db blockHash = update.headBlockHash.asEthHash if blockHash == Hash256(): @@ -258,10 +268,10 @@ proc setupEngineAPI*( # TODO should this be possible? # If we allow these types of reorgs, we will do lots and lots of reorgs during sync warn "Reorg to previous block" - if not db.setHead(blockHash): - return simpleFCU(PayloadExecutionStatus.invalid) - elif not db.setHead(blockHash): - return simpleFCU(PayloadExecutionStatus.invalid) + if chain.setCanonical(header) != ValidationResult.OK: + return invalidFCU(db, header) + elif chain.setCanonical(header) != ValidationResult.OK: + return invalidFCU(db, header) # If the beacon client also advertised a finalized block, mark the local # chain final and completely in PoS mode. @@ -326,7 +336,9 @@ proc setupEngineAPI*( api.put(id, payload) info "Created payload for sealing", - id = id.toHex + id = id.toHex, + hash = payload.blockHash, + number = payload.blockNumber.uint64 return validFCU(some(id), blockHash) diff --git a/nimbus/sealer.nim b/nimbus/sealer.nim index 53885a077..033c59243 100644 --- a/nimbus/sealer.nim +++ b/nimbus/sealer.nim @@ -267,19 +267,10 @@ proc generateExecutionPayload*(engine: SealingEngineRef, blk.header.prevRandao = prevRandao blk.header.fee = some(blk.header.fee.get(UInt256.zero)) # force it with some(UInt256) - let res = engine.chain.persistBlocks([blk.header], [ - BlockBody(transactions: blk.txs, uncles: blk.uncles) - ]) - let blockHash = rlpHash(blk.header) - if res != ValidationResult.OK: - return err("Error when validating generated block. hash=" & blockHash.data.toHex) - if blk.header.extraData.len > 32: return err "extraData length should not exceed 32 bytes" - discard engine.txPool.smartHead(blk.header) # add transactions update jobs - payloadRes.parentHash = Web3BlockHash blk.header.parentHash.data payloadRes.feeRecipient = Web3Address blk.header.coinbase payloadRes.stateRoot = Web3BlockHash blk.header.stateRoot.data diff --git a/tests/test_custom_network.nim b/tests/test_custom_network.nim index acfc5de01..c716f8996 100644 --- a/tests/test_custom_network.nim +++ b/tests/test_custom_network.nim @@ -183,7 +183,7 @@ proc isOk(rc: ValidationResult): bool = proc ttdReached(db: BaseChainDB): bool = if db.config.terminalTotalDifficulty.isSome: - return db.config.terminalTotalDifficulty.get <= db.totalDifficulty + return db.config.terminalTotalDifficulty.get <= db.headTotalDifficulty() proc importBlocks(c: Chain; h: seq[BlockHeader]; b: seq[BlockBody]; noisy = false): bool =