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

eth/catalyst: implement kintsugi-spec v3 #24067

Merged
merged 5 commits into from Dec 17, 2021

Conversation

MariusVanDerWijden
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden commented Dec 6, 2021

This PR implements kintsugi-spec v3 (with engine api version v1.0.0-alpha.5)
A rundown of all changes can be found here: https://hackmd.io/@n0ble/kintsugi-spec
It includes a new fork block MergeForkBlock which should be updated before TTD is reached.
This MergeForkBlock is only used for the forkID calculation, nothing else.
This is a block that should happen before the merge to fork non-merge ready nodes off the network.

Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

So essentially the MergeForkBlock is set as usual fork block, but indicates "we are starting the transition right now, but transition will happen in the next a few blocks"?

eth/catalyst/api.go Outdated Show resolved Hide resolved
eth/catalyst/api.go Outdated Show resolved Hide resolved
@holiman
Copy link
Contributor

holiman commented Dec 14, 2021

From the spec

EL client software MUST:
 -   set mixHash value to the value of the random field of ExecutionPayload or PayloadAttributes structures
  -  replace the difficulty value with the mixHash value in the EVM context and change the return value of the DIFFICULTY opcode to the mixHash

I don't see that being done in this PR?

@MariusVanDerWijden
Copy link
Member Author

I don't see that being done in this PR?

Yes, I have a follow-up PR that implements the RANDOM opcode which will also do the changes needed for mixhash

@MariusVanDerWijden
Copy link
Member Author

So essentially the MergeForkBlock is set as usual fork block, but indicates "we are starting the transition right now, but transition will happen in the next a few blocks"?

Yes exactly, it should just help us to kick off unwanted nodes early from, rather than connecting to them and then yeeting them later on

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

Code int
Message string
Code int
ValidationError string
Copy link
Contributor

Choose a reason for hiding this comment

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

This error type is kind of misplaced in package rpc. I'm merging anyway, but please move it in the next iteration.

@fjl fjl merged commit 2295640 into ethereum:master Dec 17, 2021
@fjl fjl added this to the 1.10.14 milestone Dec 17, 2021
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Dec 19, 2021
JacekGlen pushed a commit to JacekGlen/go-ethereum that referenced this pull request May 26, 2022
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.

None yet

5 participants