From 7de6199ba3140c069190762f041bdaaba048023d Mon Sep 17 00:00:00 2001 From: jangko Date: Fri, 3 Nov 2023 21:41:05 +0700 Subject: [PATCH] Engine API: Fix latestValidHash value when invalid timestamp detected --- hive_integration/nodocker/engine/clmock.nim | 18 +- .../engine/engine/invalid_ancestor.nim | 24 +- .../nodocker/engine/engine/reorg.nim | 426 +++++++++--------- hive_integration/nodocker/engine/types.nim | 15 +- nimbus/beacon/api_handler/api_newpayload.nim | 5 +- 5 files changed, 234 insertions(+), 254 deletions(-) diff --git a/hive_integration/nodocker/engine/clmock.nim b/hive_integration/nodocker/engine/clmock.nim index 226d94913..7dd0d2af1 100644 --- a/hive_integration/nodocker/engine/clmock.nim +++ b/hive_integration/nodocker/engine/clmock.nim @@ -438,14 +438,18 @@ proc broadcastNextNewPayload(cl: CLMocker): bool = cl.executedPayloadHistory[number] = cl.latestPayloadBuilt return true -proc broadcastForkchoiceUpdated(cl: CLMocker, eng: EngineEnv, - update: ForkchoiceStateV1): Result[ForkchoiceUpdatedResponse, string] = - let version = cl.latestExecutedPayload.version +proc broadcastForkchoiceUpdated(cl: CLMocker, + eng: EngineEnv, + version: Version, + update: ForkchoiceStateV1): + Result[ForkchoiceUpdatedResponse, string] = eng.client.forkchoiceUpdated(version, update, none(PayloadAttributes)) -proc broadcastLatestForkchoice(cl: CLMocker): bool = +proc broadcastForkchoiceUpdated*(cl: CLMocker, + version: Version, + update: ForkchoiceStateV1): bool = for eng in cl.clients: - let res = cl.broadcastForkchoiceUpdated(eng, cl.latestForkchoice) + let res = cl.broadcastForkchoiceUpdated(eng, version, update) if res.isErr: error "CLMocker: broadcastForkchoiceUpdated Error", msg=res.error return false @@ -474,6 +478,10 @@ proc broadcastLatestForkchoice(cl: CLMocker): bool = return true +proc broadcastLatestForkchoice(cl: CLMocker): bool = + let version = cl.latestExecutedPayload.version + cl.broadcastForkchoiceUpdated(version, cl.latestForkchoice) + func w3Address(x: int): Web3Address = var res: array[20, byte] res[^1] = x.byte diff --git a/hive_integration/nodocker/engine/engine/invalid_ancestor.nim b/hive_integration/nodocker/engine/engine/invalid_ancestor.nim index 520a71211..32d9d2d39 100644 --- a/hive_integration/nodocker/engine/engine/invalid_ancestor.nim +++ b/hive_integration/nodocker/engine/engine/invalid_ancestor.nim @@ -39,13 +39,8 @@ method withMainFork(cs: InvalidMissingAncestorReOrgTest, fork: EngineFork): Base return res method getName(cs: InvalidMissingAncestorReOrgTest): string = - var desc = "Invalid Missing Ancestor ReOrg Invalid" & $cs.invalidField - - if cs.emptyTransactions: - desc.add ", Empty Txs" - - desc.add ", Invalid P" & $cs.invalidIndex & "'" - desc + "Invalid Missing Ancestor ReOrg, $1, EmptyTxs=$2, Invalid P$3" % [ + $cs.invalidField, $cs.emptyTransactions, $cs.invalidIndex] method execute(cs: InvalidMissingAncestorReOrgTest, env: TestEnv): bool = # Wait until TTD is reached by this client @@ -186,25 +181,14 @@ type # or start chain (if specified). reOrgFromCanonical*: bool - method withMainFork(cs: InvalidMissingAncestorReOrgSyncTest, fork: EngineFork): BaseSpec = var res = cs.clone() res.mainFork = fork return res method getName(cs: InvalidMissingAncestorReOrgSyncTest): string = - var name = "Invalid Missing Ancestor ReOrg" - name.add ", Invalid " & $cs.invalidField - - if cs.emptyTransactions: - name.add ", Empty Txs" - - name.add ", Invalid P" & $cs.invalidIndex & "'" - name.add ", Reveal using sync" - - if cs.reOrgFromCanonical: - name.add ", ReOrg from Canonical" - return name + "Invalid Missing Ancestor Syncing ReOrg, $1, EmptyTxs=$2, CanonicalReOrg=$3, Invalid P$4" % [ + $cs.invalidField, $cs.emptyTransactions, $cs.reOrgFromCanonical, $cs.invalidIndex] method execute(cs: InvalidMissingAncestorReOrgSyncTest, env: TestEnv): bool = var sec = env.addEngine(true, cs.reOrgFromCanonical) diff --git a/hive_integration/nodocker/engine/engine/reorg.nim b/hive_integration/nodocker/engine/engine/reorg.nim index dac7a0429..c13e64989 100644 --- a/hive_integration/nodocker/engine/engine/reorg.nim +++ b/hive_integration/nodocker/engine/engine/reorg.nim @@ -12,6 +12,7 @@ import std/strutils, eth/common, chronicles, + stew/byteutils, ./engine_spec, ../cancun/customizer, ../helper @@ -115,7 +116,13 @@ type type TransactionReOrgTest* = ref object of EngineSpec transactionCount*: int - scenario*: TransactionReOrgScenario + scenario*: TransactionReOrgScenario + + ShadowTx = ref object + payload: ExecutionPayload + nextTx: Transaction + tx: Option[Transaction] + sendTransaction: proc(i: int): Transaction {.gcsafe.} method withMainFork(cs: TransactionReOrgTest, fork: EngineFork): BaseSpec = var res = cs.clone() @@ -128,6 +135,9 @@ method getName(cs: TransactionReOrgTest): string = name.add ", " & $cs.scenario return name +func txHash(shadow: ShadowTx): common.Hash256 = + shadow.tx.get.rlpHash + # Test transaction status after a forkchoiceUpdated re-orgs to an alternative hash where a transaction is not present method execute(cs: TransactionReOrgTest, env: TestEnv): bool = # Wait until this client catches up with latest PoS @@ -136,259 +146,218 @@ method execute(cs: TransactionReOrgTest, env: TestEnv): bool = # Produce blocks before starting the test (So we don't try to reorg back to the genesis block) testCond env.clMock.produceBlocks(5, BlockProcessCallbacks()) -#[ - # Create transactions that modify the state in order to check after the reorg. - var ( - err error - txCount = cs.transactionCount - sstoreContractAddr = common.HexToAddress("0000000000000000000000000000000000000317") - ) - if txCount == 0 ( - # Default is to send 5 transactions - txCount = 5 - ) + # Create transactions that modify the state in order to check after the reorg. + var shadow = ShadowTx() # Send a transaction on each payload of the canonical chain - sendTransaction = func(i int) (Transaction, error) ( - data = common.LeftPadBytes([]byte(byte(i)), 32) - t.Logf("transactionReorg, i=%v, data=%v\n", i, data) - return t.sendNextTx( - t.Engine, - BaseTx( - recipient: &sstoreContractAddr, - amount: big0, - payload: data, - txType: cs.txType, - gasLimit: 75000, - ForkConfig: t.ForkConfig, - ), + shadow.sendTransaction = proc(i: int): Transaction {.gcsafe.} = + let sstoreContractAddr = hexToByteArray[20]("0000000000000000000000000000000000000317") + var data: array[32, byte] + data[^1] = i.byte + info "transactionReorg", idx=i + let tc = BaseTx( + recipient: some(sstoreContractAddr), + amount: 0.u256, + payload: @data, + txType: cs.txType, + gasLimit: 75000, ) + var tx = env.makeNextTx(tc) + doAssert env.sendTx(tx) + tx - ) - - var ( - shadow.payload ExecutableData - nextTx Transaction - tx Transaction - ) - - for i = 0; i < txCount; i++ ( + var txCount = cs.transactionCount + if txCount == 0: + # Default is to send 5 transactions + txCount = 5 + for i in 0.. 0 ( + if cs.scenario == TransactionReOrgScenarioReOrgBackIn and i > 0: # Reasoning: Most of the clients do not re-add blob transactions to the pool # after a re-org, so we need to wait until the next tx is sent to actually # verify. - tx = shadow.nextTx - ) - - ), + shadow.tx = some(shadow.nextTx) + return true )) - - ) + testCond pbRes if shadow.tx.isSome: # Produce one last block and verify that the block contains the transaction - env.clMock.produceSingleBlock(BlockProcessCallbacks( + let pbRes = env.clMock.produceSingleBlock(BlockProcessCallbacks( onForkchoiceBroadcast: proc(): bool = - if !TransactionInPayload(env.clMock.latestPayloadBuilt, tx) ( - fatal "Payload built does not contain the transaction: %v", t.TestName, env.clMock.latestPayloadBuilt) - ) + testCond txInPayload(env.clMock.latestPayloadBuilt, shadow.txHash): + fatal "Payload built does not contain the transaction" + # Get the receipt - ctx, cancel = context.WithTimeout(t.TestContext, globals.RPCTimeout) - defer cancel() - receipt, _ = t.Eth.TransactionReceipt(ctx, shadow.txHash) - if receipt == nil ( - fatal "Receipt not obtained after tx included in block: %v", t.TestName, receipt) - ) - ), + let receipt = env.engine.client.txReceipt(shadow.txHash) + testCond receipt.isErr: + fatal "Receipt not obtained after tx included in block" + + return true )) + testCond pbRes - ) - -) -]# + return true # Test that performing a re-org back into a previous block of the canonical chain does not produce errors and the chain # is still capable of progressing. @@ -401,6 +370,13 @@ type # Whether to execute a sidechain payload on the re-org executeSidePayloadOnReOrg*: bool + ShadowCanon = ref object + previousHash: Web3Hash + previousTimestamp: Web3Quantity + payload: ExecutionPayload + parentForkchoice: ForkchoiceStateV1 + parentTimestamp: uint64 + method withMainFork(cs: ReOrgBackToCanonicalTest, fork: EngineFork): BaseSpec = var res = cs.clone() res.mainFork = fork @@ -435,99 +411,99 @@ method execute(cs: ReOrgBackToCanonicalTest, env: TestEnv): bool = # Produce blocks before starting the test (So we don't try to reorg back to the genesis block) testCond env.clMock.produceBlocks(5, BlockProcessCallbacks()) -#[ # We are going to reorg back to a previous hash several times - previousHash = env.clMock.latestForkchoice.headBlockHash - previousTimestamp = env.clMock.latestPayloadBuilt.timestamp + var shadow = ShadowCanon( + previousHash: env.clMock.latestForkchoice.headBlockHash, + previousTimestamp: env.clMock.latestPayloadBuilt.timestamp + ) - if cs.executeSidePayloadOnReOrg ( - var ( - shadow.payload ExecutableData - shadow.parentForkchoice api.ForkchoiceStateV1 - shadow.parentTimestamp uint64 - ) - env.clMock.produceSingleBlock(BlockProcessCallbacks( - OnPayloadAttributesGenerated: proc(): bool = - payloadAttributes = env.clMock.LatestPayloadAttributes - rand.Read(payloadAttributes.Random[:]) - r = env.engine.client.forkchoiceUpdated(env.clMock.latestForkchoice, payloadAttributes, env.clMock.latestHeader.Time) + if cs.executeSidePayloadOnReOrg: + var pbRes = env.clMock.produceSingleBlock(BlockProcessCallbacks( + onPayloadAttributesGenerated: proc(): bool = + var attr = env.clMock.latestPayloadAttributes + attr.prevRandao = Web3Hash.randomBytes() + + var version = env.engine.version(env.clMock.latestHeader.timestamp) + let r = env.engine.client.forkchoiceUpdated(version, env.clMock.latestForkchoice, some(attr)) r.expectNoError() - if r.Response.PayloadID == nil ( - fatal "No payload ID returned by forkchoiceUpdated", t.TestName) - ) - g = env.engine.client.getPayload(r.Response.PayloadID, payloadAttributes) + testcond r.get.payloadID.isSome: + fatal "No payload ID returned by forkchoiceUpdated" + + version = env.engine.version(attr.timestamp) + let g = env.engine.client.getPayload(r.get.payloadID.get, version) g.expectNoError() - shadow.payload = &g.Payload + + shadow.payload = g.get.executionPayload shadow.parentForkchoice = env.clMock.latestForkchoice - shadow.parentTimestamp = env.clMock.latestHeader.Time - ), + shadow.parentTimestamp = env.clMock.latestHeader.timestamp.uint64 + return true )) + testCond pbRes + # Continue producing blocks until we reach the depth of the re-org - testCond env.clMock.produceBlocks(int(cs.GetDepth()-1), BlockProcessCallbacks( + pbRes = env.clMock.produceBlocks(cs.getDepth()-1, BlockProcessCallbacks( onPayloadProducerSelected: proc(): bool = # Send a transaction on each payload of the canonical chain - var err error - _, err = t.sendNextTxs( - t.TestContext, - t.Engine, - BaseTx( - recipient: &ZeroAddr, - amount: big1, - payload: nil, - txType: cs.txType, - gasLimit: 75000, - ForkConfig: t.ForkConfig, - ), - cs.transactionPerPayload, + let tc = BaseTx( + recipient: some(ZeroAddr), + amount: 1.u256, + txType: cs.txType, + gasLimit: 75000, ) + let ok = env.sendNextTxs(env.engine, tc, cs.transactionPerPayload) testCond ok: - fatal "Error trying to send transactions: %v", t.TestName, err) - ) - ), + fatal "Error trying to send transactions" + return true )) + testCond pbRes + # On the last block, before executing the next payload of the canonical chain, # re-org back to the parent of the side payload and execute the side payload first - env.clMock.produceSingleBlock(BlockProcessCallbacks( + pbRes = env.clMock.produceSingleBlock(BlockProcessCallbacks( onGetpayload: proc(): bool = # We are about to execute the new payload of the canonical chain, re-org back to # the side payload - f = env.engine.client.forkchoiceUpdated(shadow.parentForkchoice, nil, shadow.parentTimestamp) + var version = env.engine.version(shadow.parentTimestamp) + let f = env.engine.client.forkchoiceUpdated(version, shadow.parentForkchoice) f.expectPayloadStatus(PayloadExecutionStatus.valid) f.expectLatestValidHash(shadow.parentForkchoice.headBlockHash) + # Execute the side payload - n = env.engine.client.newPayload(shadow.payload) + let n = env.engine.client.newPayload(shadow.payload) n.expectStatus(PayloadExecutionStatus.valid) n.expectLatestValidHash(shadow.payload.blockHash) # At this point the next canonical payload will be executed by the CL mock, so we can # continue producing blocks - ), + return true )) + testCond pbRes else: - testCond env.clMock.produceBlocks(int(cs.GetDepth()), BlockProcessCallbacks( + let pbRes = env.clMock.produceBlocks(cs.getDepth(), BlockProcessCallbacks( onForkchoiceBroadcast: proc(): bool = # Send a fcU with the headBlockHash pointing back to the previous block - forkchoiceUpdatedBack = api.ForkchoiceStateV1( - headBlockHash: previousHash, + let fcu = ForkchoiceStateV1( + headBlockHash: shadow.previousHash, safeBlockHash: env.clMock.latestForkchoice.safeBlockHash, finalizedBlockHash: env.clMock.latestForkchoice.finalizedBlockHash, ) # It is only expected that the client does not produce an error and the CL Mocker is able to progress after the re-org - r = env.engine.client.forkchoiceUpdated(forkchoiceUpdatedBack, nil, previousTimestamp) + var version = env.engine.version(shadow.previousTimestamp) + var r = env.engine.client.forkchoiceUpdated(version, fcu) r.expectNoError() # Re-send the ForkchoiceUpdated that the CLMock had sent - r = env.engine.client.forkchoiceUpdated(env.clMock.latestForkchoice, nil, env.clMock.LatestExecutedPayload.timestamp) + version = env.engine.version(env.clMock.latestExecutedPayload.timestamp) + r = env.engine.client.forkchoiceUpdated(version, env.clMock.latestForkchoice) r.expectNoError() - ), + return true )) - ) + testCond pbRes # Verify that the client is pointing to the latest payload sent - r = env.engine.client.headerByNumber(Head) - r.expectHash(env.clMock.latestPayloadBuilt.blockHash) -) -]# + let r = env.engine.client.namedHeader(Head) + r.expectHash(ethHash env.clMock.latestPayloadBuilt.blockHash) + return true type ReOrgBackFromSyncingTest* = ref object of EngineSpec diff --git a/hive_integration/nodocker/engine/types.nim b/hive_integration/nodocker/engine/types.nim index 97da82063..aa8deeb7f 100644 --- a/hive_integration/nodocker/engine/types.nim +++ b/hive_integration/nodocker/engine/types.nim @@ -165,12 +165,16 @@ template expectLatestValidHash*(res: untyped, expectedHash: Web3Hash) = testCond s.latestValidHash.isSome: error "Expect latest valid hash isSome" testCond s.latestValidHash.get == expectedHash: - error "latest valid hash mismatch", expect=expectedHash, get=s.latestValidHash.get + error "latest valid hash mismatch", + expect=expectedHash.short, + get=s.latestValidHash.get.short else: testCond s.payloadStatus.latestValidHash.isSome: error "Expect latest valid hash isSome" testCond s.payloadStatus.latestValidHash.get == expectedHash: - error "latest valid hash mismatch", expect=expectedHash, get=s.payloadStatus.latestValidHash.get + error "latest valid hash mismatch", + expect=expectedHash.short, + get=s.payloadStatus.latestValidHash.get.short template expectLatestValidHash*(res: untyped) = testCond res.isOk: @@ -297,6 +301,13 @@ template expectPayloadParentHash*(res: untyped, expected: Web3Hash) = testCond rec.executionPayload.parentHash == expected: error "expectPayloadParentHash", expect=expected.short, get=rec.executionPayload.parentHash.short +template expectBlockHash*(res: untyped, expected: common.Hash256) = + testCond res.isOk: + error "expectBlockHash", msg=res.error + let rec = res.get + testCond rec.blockHash == expected: + error "expectBlockHash", expect=expected.short, get=rec.blockHash.short + func timestamp*(x: ExecutableData): auto = x.basePayload.timestamp diff --git a/nimbus/beacon/api_handler/api_newpayload.nim b/nimbus/beacon/api_handler/api_newpayload.nim index f14e367cd..d8e4a91f2 100644 --- a/nimbus/beacon/api_handler/api_newpayload.nim +++ b/nimbus/beacon/api_handler/api_newpayload.nim @@ -126,8 +126,9 @@ proc newPayload*(ben: BeaconEngineRef, if header.timestamp <= parent.timestamp: warn "Invalid timestamp", - parent = header.timestamp, header = header.timestamp - return invalidStatus(db.getHeadBlockHash(), "Invalid timestamp") + number = header.blockNumber, parentNumber = parent.blockNumber, + parent = parent.timestamp, header = header.timestamp + return invalidStatus(parent.blockHash, "Invalid timestamp") if not db.haveBlockAndState(header.parentHash): ben.put(blockHash, header)