core,syscall: move 4788 system call into separate package #29372
Conversation
lightclient
requested review from
karalabe,
holiman and
rjl493456442
as code owners
| return vm.BlockContext{ | ||
| CanTransfer: func(db vm.StateDB, addr common.Address, amount *uint256.Int) bool { return false }, | ||
| Transfer: func(vm.StateDB, common.Address, common.Address, *uint256.Int) {}, | ||
| GetHash: func(uint64) common.Hash { return common.Hash{} }, |
There was a problem hiding this comment.
The block context is incomplete. While it may suffice for EIP4788, it's likely insufficient for future needs.
There was a problem hiding this comment.
Ideally, we should reuse the block context for transaction processing, ensuring that all necessary environmental fields are reliably set.
There was a problem hiding this comment.
Yeah I was wondering if we could move core/evm.go somewhere else. I needed to recreate this to avoid import cycle!
| evm := vm.NewEVM(vmContext, vm.TxContext{}, statedb, chainConfig, vmConfig) | ||
| core.ProcessBeaconBlockRoot(*beaconRoot, evm, statedb) | ||
| } | ||
| header := &types.Header{ParentBeaconRoot: pre.Env.ParentBeaconBlockRoot, Time: pre.Env.Timestamp} |
There was a problem hiding this comment.
Number is not assigned. It's required for context initialization, isn't it?
| } | ||
|
|
||
| // RunPreBlockHooks executes all relevant pre-block operations. It will update statedb. | ||
| func RunPreBlockHooks(header *types.Header, statedb *state.StateDB, config *params.ChainConfig) { |
There was a problem hiding this comment.
Please pass in vm config here as well.
…rect state The beacon root when applied in `state_processor.go` is performed right before executing transaction. That means that contract reliying on this value would query the same value found in the block header. In that spirit, it means that any tracing/operation relying on state data which touches transaction must have updated the beacon root before any transaction processing. This PR brings this where I spotted where it was important through `StateAtTransaction` and those using with `StateAtBlock` and transactions. For now I've put the beacon root update outside of `StateAtBlock`. I was reluctant to include it in `StateAtBlock` because technically the state at block doesn't include any state happening "in the block". But everywhere that `StateAtBlock` was used, it felt setting the beacon root was needed, so it would be posssible to change the semantic of the function and run any system pre-hooks. Kinda relevant with PR ethereum#29372
|
After discussing as a team, I think we'll be better served by refactoring the block validation / generation code so that we can use the same interface across the many places where we need to copy the same code. |
This is a proposal to simplify and better abstract the system call interface.
Currently we only have one system call, but with Prague we will have at least one additional system call (EIP-7002, a post-block system call) and others are actively being considered. This type of operation is also commonly used downstream by forks of geth to seed other execution environments with custom data.
It seems like there is demand to improve the interface for writing and using system calls.
Originally I considered revamping the
consensus.Engineinterface to initiate these calls. Many of our system calls are directly related to consensus operations: triggering validator exits and writing beacon roots into state. I still think from an organizational standpoint, it makes sense to tie these operations to specific consensus engines. However, it isn't as easy to jam these system calls into the Engine interface as I thought.Pre-block hooks could naturally be executed in the
Prepare(..)method, but currentlyPrepare(..)is only used by the miner as it actually sets the header difficulty directly. It seems weird to call this function instate_processorand modify the header.Post-block hooks could be executed in
Finalize, however with 7002 we're actually pulling data out of a contract which needs to be packaged into the block and verified against the header. Since finalize currently has the semantic of only modifyingstatedbit feels weird to force it to deal with exits.--
For these reasons, I'm proposing the api of:
RunPreBlockHooks(..)RunPostBlockHooks(..)(n/a in this PR, will incorporate in 7002 PR if we accept this API)These two methods can be called in four common code paths we have for block building / validation: the chain maker, miner, state processor, and t8n.