-
Notifications
You must be signed in to change notification settings - Fork 20.5k
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
core, params, beacon/engine: implement EIP 4788 BeaconRoot #27849
Conversation
consensus/misc: add eip4788 consensus/misc: header core/vm: allow for stateful precompiles core/vm: implement 4788 as a stateful precompile core, consensus/misc: update to new 4788 spec core: fix rebase core/vm: add activator core/vm: reduce interface, increase testability, remove opcode params: introduce params.BeaconRootStorageAddress happy lint, happy life core: make sure to only apply beaconroot after cancun! params, consensus: fix review comments
consensus/misc: set beaconroot precompile non-zero nonce (poc)
core/state_processor.go
Outdated
// we can use the header.BaseFee and set that instead. | ||
msg := &Message{ | ||
From: params.SystemAddress, | ||
GasLimit: 100_000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to use gas limit of 30_000_000 to be in line with the Gnosis Chain spec for system transactions.
core/state_processor.go
Outdated
@@ -66,6 +66,7 @@ func (p *StateProcessor) Process(block *types.Block, statedb *state.StateDB, cfg | |||
blockNumber = block.Number() | |||
allLogs []*types.Log | |||
gp = new(GasPool).AddGas(block.GasLimit()) | |||
beaconRoot *common.Hash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just a placeholder until we later activate this by checking block.BeaconRoot
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, yeah we should remove this and use block.BeaconRoot instead. Looks very odd like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah that change is probably in the follow-up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, #27872 is the follow-up, but it still doesn't fill this placeholder :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah no worries, I have these changes in the devnet-8 branch. I'm not sure the best way to be tracking this for everyone as it pretty out-of-sync with master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lightclient thanks for adding your updates, I have also pushed some changes now -- primarily, hooking it into evm t8n
so I could actually test it and see it working in practice.
PTAL
@@ -292,6 +292,14 @@ func Transition(ctx *cli.Context) error { | |||
prestate.Env.Difficulty = calcDifficulty(chainConfig, env.Number, env.Timestamp, | |||
env.ParentTimestamp, env.ParentDifficulty, env.ParentUncleHash) | |||
} | |||
if chainConfig.IsCancun(big.NewInt(int64(prestate.Env.Number)), prestate.Env.Timestamp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
big.NewInt(int64(prestate.Env.Number)
seems like this is used a lot, might be nice to assign above ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Myeah, a couple, but I don't think it's worth it. Might do a bigger refactor later IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, some uses refer to big.NewInt(int64(prestate.Env.Number))
, some refer to int64(prestate.Env.Number)
, and some refer to prestate.Env.Number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the bigger refactor, ptal!
return NewError(ErrorConfig, errors.New("post-cancun env requires parentBeaconBlockRoot to be set")) | ||
} | ||
} else { | ||
env.ParentBeaconBlockRoot = nil // un-set it if it has been set too early |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be an error if beacon root is requested before the fork?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could but in go-ethereum, due to how the json
parser works, we typically ignore extraneous json-fields. I think the engine-api is the sole exception where we explicitly check for, and error on, fields that we do not expect.
For t8n, I'd prefer to keep the older behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
See ethereum/EIPs#7456 & ethereum/go-ethereum#27849. Also set the gas limit for system calls to 30M (previously 2^64-1), which is in line with the [Gnosis spec](https://github.com/gnosischain/specs/blob/master/execution/withdrawals.md#specification), but should be doubled checked for Gnosis Chain.
See ethereum/EIPs#7456 & ethereum/go-ethereum#27849. Also set the gas limit for system calls to 30M (previously 2^64-1), which is in line with the [Gnosis spec](https://github.com/gnosischain/specs/blob/master/execution/withdrawals.md#specification), but should be doubled checked for Gnosis Chain.
…27849) This change implements "EIP 4788 : Beacon block root in the EVM". It implements version-2 of EPI-4788, main difference being that the contract is an actual contract rather than a precompile, as in ethereum#27289.
…thereum#27849)" This reverts commit 2e9477f.
…thereum#27849)" This reverts commit 2e9477f.
Conflicts: cmd/evm/internal/t8ntool/transition.go Callsite for IsShanghai moved to a different function, moved our modification there. Additions: go-ethereum/cmd/evm/testdata/29/exp.json Added Arbitrum field to expected receipt.
EIP 4788 : Beacon block root in the EVM
This is a version-2 of EPI-4788, main difference being that the contract is an actual contract rather than a precompile, as in #27289.
cc @ralexstokes