Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use justified blocks execution payload hash as safeBlockHash in engine fcU #4080

Merged
merged 4 commits into from
Jun 3, 2022
Merged

Use justified blocks execution payload hash as safeBlockHash in engine fcU #4080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee49683
Use justified blocks execution payload hash as safeBlockHash in engin…
g11tech May 28, 2022
c78a3e4
fix test
g11tech May 28, 2022
9bd0e59
safe block hash fix
g11tech May 29, 2022
9bd48e5
run merge ci on latest
g11tech May 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/test-sim-merge.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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

Expand Down
6 changes: 4 additions & 2 deletions packages/lodestar/src/chain/blocks/importBlock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to have a separate method like this in forkchoice

def get_safe_execution_payload_hash(store: Store) -> Hash32:
    safe_block_root = get_safe_beacon_block_root(store)
    safe_block = store.blocks[safe_block_root]

    # Return Hash32() if no payload is yet justified
    if compute_epoch_at_slot(safe_block.slot) >= BELLATRIX_FORK_EPOCH:
        return safe_block.body.execution_payload.block_hash
    else:
        return Hash32()

and use it everywhere to make it clear

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ummm justified iproto block in forkchoice will only have executionPayloadBlockHash if its post merge i.e. >= BELLATRIX_FORK_EPOCH.
It is similar to the way we extract finalizedBlockHash, I could add comments for better clarity if you think that would help understanding whats going on? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you think safeBlockHash() will not changed in the future then we're good 👍

Copy link
Contributor Author

@g11tech g11tech Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes its more or less fixed, its the EL counterpart of the justified block, which they can use to tag, prune, throw reorg errors etc may be, earlier it headBlockHash was used as a stub for it, which the ELs were totally ignoring

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);
Expand Down
22 changes: 7 additions & 15 deletions packages/lodestar/src/chain/factory/block/body.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
Expand Down Expand Up @@ -139,6 +130,7 @@ export async function prepareExecutionPayload(
executionEngine: IExecutionEngine;
config: IChainForkConfig;
},
safeBlockHash: RootHex,
finalizedBlockHash: RootHex,
state: CachedBeaconStateBellatrix,
suggestedFeeRecipient: string
Expand Down Expand Up @@ -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,
Expand Down
6 changes: 2 additions & 4 deletions packages/lodestar/src/executionEngine/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ export class ExecutionEngineHttp implements IExecutionEngine {
*/
async notifyForkchoiceUpdate(
headBlockHash: Root | RootHex,
safeBlockHash: RootHex,
finalizedBlockHash: RootHex,
payloadAttributes?: PayloadAttributes
): Promise<PayloadId | null> {
Expand All @@ -212,10 +213,7 @@ export class ExecutionEngineHttp implements IExecutionEngine {
payloadId,
} = await this.rpc.fetch<EngineApiRpcReturnTypes[typeof method], EngineApiRpcParamTypes[typeof method]>({
method,
params: [
{headBlockHash: headBlockHashData, safeBlockHash: headBlockHashData, finalizedBlockHash},
apiPayloadAttributes,
],
params: [{headBlockHash: headBlockHashData, safeBlockHash, finalizedBlockHash}, apiPayloadAttributes],
});

switch (status) {
Expand Down
1 change: 1 addition & 0 deletions packages/lodestar/src/executionEngine/interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export interface IExecutionEngine {
*/
notifyForkchoiceUpdate(
headBlockHash: Root | RootHex,
safeBlockHash: RootHex,
finalizedBlockHash: RootHex,
payloadAttributes?: PayloadAttributes
): Promise<PayloadId | null>;
Expand Down
1 change: 1 addition & 0 deletions packages/lodestar/src/executionEngine/mock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export class ExecutionEngineMock implements IExecutionEngine {
*/
async notifyForkchoiceUpdate(
headBlockHash: Root,
safeBlockHash: RootHex,
finalizedBlockHash: RootHex,
payloadAttributes?: PayloadAttributes
): Promise<PayloadId> {
Expand Down
4 changes: 3 additions & 1 deletion packages/lodestar/test/sim/merge-interop.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,8 @@ describe("executionEngine / ExecutionEngineHttp", function () {

const payloadId = await executionEngine.notifyForkchoiceUpdate(
genesisBlockHash,
//use finalizedBlockHash as safeBlockHash
finalizedBlockHash,
finalizedBlockHash,
preparePayloadParams
);
Expand Down Expand Up @@ -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");
Expand Down
1 change: 1 addition & 0 deletions packages/lodestar/test/unit/executionEngine/http.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ describe("ExecutionEngine / http", () => {

await executionEngine.notifyForkchoiceUpdate(
forkChoiceHeadData.headBlockHash,
forkChoiceHeadData.safeBlockHash,
forkChoiceHeadData.finalizedBlockHash
);

Expand Down