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.

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

Add safe_block_hash to notify_forkchoice_updated #2851

Merged
merged 2 commits into from
Mar 16, 2022

Conversation

mkalinin
Copy link
Collaborator

Specify CL client behaviour around safe_block_hash parameter of engine_forkchoiceUpdatedV1 function call. The PR proposes the following changes:

  • Introduces safe_block_hash parameter of notify_forkhoice_updated function call
  • Stubs safe_block_hash with the head_block_hash value until safe block function is implemented

Alternatively, safe_block_hash may be stubbed with 0x00..00 as introducing "safe" block tag to JSON-RPC API isn't expected within the Merge at least now, thus, safe_block_hash value doesn't matter much. Though, head_block_hash value is slightly preferred because i) most CL clients already implemented this behaviour ii) it's future compatible with the decision to deliver "safe" tag within the Merge but make it an alias to the head

@mkalinin mkalinin requested review from djrtwo and hwwhww March 14, 2022 13:21
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

I guess it doesn't matter much in using head_block_hash or the latest finalized block hash since it's just a stub?

LGTM

specs/bellatrix/validator.md Show resolved Hide resolved
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

looks good. note, this is a no-op for client implementations that use the engine API today because this already was passed between the layers on that API

@mkalinin
Copy link
Collaborator Author

I guess it doesn't matter much in using head_block_hash or the latest finalized block hash since it's just a stub?

Stubbing with the head_block_hash has a matter because existing CL implementations are already using head_block_hash as a stub. And as @djrtwo mentioned this PR should be a no-op for implementers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants