From b5e24f71389092c965e00b2c4c80c511d72fc4b1 Mon Sep 17 00:00:00 2001 From: g11tech Date: Fri, 3 Jun 2022 20:37:13 +0530 Subject: [PATCH] Use justified blocks execution payload hash as safeBlockHash in engine fcU (#4080) * Use justified blocks execution payload hash as safeBlockHash in engine fcU * fix test * safe block hash fix * run merge ci on latest --- .github/workflows/test-sim-merge.yml | 6 ++--- .../lodestar/src/chain/blocks/importBlock.ts | 6 +++-- .../lodestar/src/chain/factory/block/body.ts | 22 ++++++------------- packages/lodestar/src/executionEngine/http.ts | 6 ++--- .../lodestar/src/executionEngine/interface.ts | 1 + packages/lodestar/src/executionEngine/mock.ts | 1 + .../lodestar/test/sim/merge-interop.test.ts | 4 +++- .../test/unit/executionEngine/http.test.ts | 1 + 8 files changed, 22 insertions(+), 25 deletions(-) diff --git a/.github/workflows/test-sim-merge.yml b/.github/workflows/test-sim-merge.yml index 9f560e1f41de..cc69328efac3 100644 --- a/.github/workflows/test-sim-merge.yml +++ b/.github/workflows/test-sim-merge.yml @@ -3,8 +3,8 @@ name: Sim merge tests on: [pull_request, push] env: - GETH_COMMIT: bb5633c5ee3975ce016636066ec790054ec469e4 - NETHERMIND_COMMIT: 00b50532543824dbac65e8b7ab09484e44992c27 + GETH_COMMIT: be9742721f56eb8bb7ebf4f6a03fb01b13a05408 + NETHERMIND_COMMIT: 82f331a3e7ff21712a5f839e0a62ba7c16110e44 jobs: sim-merge-tests: @@ -59,7 +59,7 @@ jobs: with: dotnet-version: "6.0.x" - name: Clone Nethermind merge interop branch - run: git clone -b kiln https://github.com/g11tech/nethermind --recursive && cd nethermind && git reset --hard $NETHERMIND_COMMIT && git submodule update --init --recursive + run: git clone -b kiln-rebased https://github.com/g11tech/nethermind --recursive && cd nethermind && git reset --hard $NETHERMIND_COMMIT && git submodule update --init --recursive - name: Build Nethermind run: cd nethermind/src/Nethermind && dotnet build Nethermind.sln -c Release diff --git a/packages/lodestar/src/chain/blocks/importBlock.ts b/packages/lodestar/src/chain/blocks/importBlock.ts index b106833a2ac4..ed6c5ec47116 100644 --- a/packages/lodestar/src/chain/blocks/importBlock.ts +++ b/packages/lodestar/src/chain/blocks/importBlock.ts @@ -237,9 +237,10 @@ export async function importBlock(chain: ImportBlockModules, fullyVerifiedBlock: * the current finalized block does not contain any execution payload at all (pre MERGE_EPOCH) or if it contains a * zero block hash (pre TTD) */ + const safeBlockHash = chain.forkChoice.getJustifiedBlock().executionPayloadBlockHash ?? ZERO_HASH_HEX; const finalizedBlockHash = chain.forkChoice.getFinalizedBlock().executionPayloadBlockHash ?? ZERO_HASH_HEX; if (headBlockHash !== ZERO_HASH_HEX) { - chain.executionEngine.notifyForkchoiceUpdate(headBlockHash, finalizedBlockHash).catch((e) => { + chain.executionEngine.notifyForkchoiceUpdate(headBlockHash, safeBlockHash, finalizedBlockHash).catch((e) => { chain.logger.error("Error pushing notifyForkchoiceUpdate()", {headBlockHash, finalizedBlockHash}, e); }); } @@ -284,8 +285,9 @@ async function maybeIssueNextProposerEngineFcU( const proposerIndex = prepareState.epochCtx.getBeaconProposer(prepareSlot); const feeRecipient = chain.beaconProposerCache.get(proposerIndex); if (feeRecipient) { + const safeBlockHash = chain.forkChoice.getJustifiedBlock().executionPayloadBlockHash ?? ZERO_HASH_HEX; const finalizedBlockHash = chain.forkChoice.getFinalizedBlock().executionPayloadBlockHash ?? ZERO_HASH_HEX; - return prepareExecutionPayload(chain, finalizedBlockHash, prepareState, feeRecipient); + return prepareExecutionPayload(chain, safeBlockHash, finalizedBlockHash, prepareState, feeRecipient); } } catch (e) { chain.logger.error("Error on issuing next proposer engine fcU", {}, e as Error); diff --git a/packages/lodestar/src/chain/factory/block/body.ts b/packages/lodestar/src/chain/factory/block/body.ts index cb1d12fa4a46..31dbef0615f0 100644 --- a/packages/lodestar/src/chain/factory/block/body.ts +++ b/packages/lodestar/src/chain/factory/block/body.ts @@ -88,29 +88,20 @@ export async function assembleBody( } if (blockEpoch >= chain.config.BELLATRIX_FORK_EPOCH) { - // TODO: Optimize this flow - // - Call prepareExecutionPayload as early as possible - // - Call prepareExecutionPayload again if parameters change - - const finalizedBlockHash = chain.forkChoice.getFinalizedBlock().executionPayloadBlockHash; + const safeBlockHash = chain.forkChoice.getJustifiedBlock().executionPayloadBlockHash ?? ZERO_HASH_HEX; + const finalizedBlockHash = chain.forkChoice.getFinalizedBlock().executionPayloadBlockHash ?? ZERO_HASH_HEX; const feeRecipient = chain.beaconProposerCache.getOrDefault(proposerIndex); // prepareExecutionPayload will throw error via notifyForkchoiceUpdate if // the EL returns Syncing on this request to prepare a payload // - // TODO: - // The payloadId should be extracted from the ones cached in the execution engine - // by the advance firing of the fcU. If no entry in the cache is available then - // continue with the usual firing, but this will most likely not generate a full - // block. However some timing consideration can be done here to bundle some time - // for the same. - // - // For MeV boost integration as well, this is where the execution header will be + // For MeV boost integration, this is where the execution header will be // fetched from the payload id and a blinded block will be produced instead of // fullblock for the validator to sign const payloadId = await prepareExecutionPayload( chain, - finalizedBlockHash ?? ZERO_HASH_HEX, + safeBlockHash, + finalizedBlockHash, currentState as CachedBeaconStateBellatrix, feeRecipient ); @@ -139,6 +130,7 @@ export async function prepareExecutionPayload( executionEngine: IExecutionEngine; config: IChainForkConfig; }, + safeBlockHash: RootHex, finalizedBlockHash: RootHex, state: CachedBeaconStateBellatrix, suggestedFeeRecipient: string @@ -180,7 +172,7 @@ export async function prepareExecutionPayload( prevRandao: toHex(prevRandao), suggestedFeeRecipient, }) ?? - (await chain.executionEngine.notifyForkchoiceUpdate(parentHash, finalizedBlockHash, { + (await chain.executionEngine.notifyForkchoiceUpdate(parentHash, safeBlockHash, finalizedBlockHash, { timestamp, prevRandao, suggestedFeeRecipient, diff --git a/packages/lodestar/src/executionEngine/http.ts b/packages/lodestar/src/executionEngine/http.ts index ef30715b62e0..df02b78fff31 100644 --- a/packages/lodestar/src/executionEngine/http.ts +++ b/packages/lodestar/src/executionEngine/http.ts @@ -191,6 +191,7 @@ export class ExecutionEngineHttp implements IExecutionEngine { */ async notifyForkchoiceUpdate( headBlockHash: Root | RootHex, + safeBlockHash: RootHex, finalizedBlockHash: RootHex, payloadAttributes?: PayloadAttributes ): Promise { @@ -211,10 +212,7 @@ export class ExecutionEngineHttp implements IExecutionEngine { payloadId, } = await this.rpc.fetch({ method, - params: [ - {headBlockHash: headBlockHashData, safeBlockHash: headBlockHashData, finalizedBlockHash}, - apiPayloadAttributes, - ], + params: [{headBlockHash: headBlockHashData, safeBlockHash, finalizedBlockHash}, apiPayloadAttributes], }); switch (status) { diff --git a/packages/lodestar/src/executionEngine/interface.ts b/packages/lodestar/src/executionEngine/interface.ts index f5befcc13643..c8afa7a92ed3 100644 --- a/packages/lodestar/src/executionEngine/interface.ts +++ b/packages/lodestar/src/executionEngine/interface.ts @@ -81,6 +81,7 @@ export interface IExecutionEngine { */ notifyForkchoiceUpdate( headBlockHash: Root | RootHex, + safeBlockHash: RootHex, finalizedBlockHash: RootHex, payloadAttributes?: PayloadAttributes ): Promise; diff --git a/packages/lodestar/src/executionEngine/mock.ts b/packages/lodestar/src/executionEngine/mock.ts index dfc25985c28c..6d7225f8c02c 100644 --- a/packages/lodestar/src/executionEngine/mock.ts +++ b/packages/lodestar/src/executionEngine/mock.ts @@ -82,6 +82,7 @@ export class ExecutionEngineMock implements IExecutionEngine { */ async notifyForkchoiceUpdate( headBlockHash: Root, + safeBlockHash: RootHex, finalizedBlockHash: RootHex, payloadAttributes?: PayloadAttributes ): Promise { diff --git a/packages/lodestar/test/sim/merge-interop.test.ts b/packages/lodestar/test/sim/merge-interop.test.ts index b2d9b1ed5765..b235d19dc0b1 100644 --- a/packages/lodestar/test/sim/merge-interop.test.ts +++ b/packages/lodestar/test/sim/merge-interop.test.ts @@ -176,6 +176,8 @@ describe("executionEngine / ExecutionEngineHttp", function () { const payloadId = await executionEngine.notifyForkchoiceUpdate( genesisBlockHash, + //use finalizedBlockHash as safeBlockHash + finalizedBlockHash, finalizedBlockHash, preparePayloadParams ); @@ -211,7 +213,7 @@ describe("executionEngine / ExecutionEngineHttp", function () { * curl -X POST -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"engine_forkchoiceUpdatedV1","params":[{"headBlockHash":"0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858", "safeBlockHash":"0x3559e851470f6e7bbed1db474980683e8c315bfce99b2a6ef47c057c04de7858", "finalizedBlockHash":"0x3b8fb240d288781d4aac94d3fd16809ee413bc99294a085798a589dae51ddd4a"}, null],"id":67}' http://localhost:8550 **/ - await executionEngine.notifyForkchoiceUpdate(bytesToData(payload.blockHash), genesisBlockHash); + await executionEngine.notifyForkchoiceUpdate(bytesToData(payload.blockHash), genesisBlockHash, genesisBlockHash); if (TX_SCENARIOS.includes("simple")) { const balance = await getBalance(jsonRpcUrl, "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); diff --git a/packages/lodestar/test/unit/executionEngine/http.test.ts b/packages/lodestar/test/unit/executionEngine/http.test.ts index f999ef3cfc54..6b427f733824 100644 --- a/packages/lodestar/test/unit/executionEngine/http.test.ts +++ b/packages/lodestar/test/unit/executionEngine/http.test.ts @@ -140,6 +140,7 @@ describe("ExecutionEngine / http", () => { await executionEngine.notifyForkchoiceUpdate( forkChoiceHeadData.headBlockHash, + forkChoiceHeadData.safeBlockHash, forkChoiceHeadData.finalizedBlockHash );