[evm] enable Random opcode (EIP-4399) at Redsea height #3940
Conversation
dustinxie
requested review from
CoderZhi,
Liuhaai and
envestcc
as code owners
Codecov Report
@@ Coverage Diff @@
## master #3940 +/- ##
==========================================
+ Coverage 75.38% 76.10% +0.72%
==========================================
Files 303 328 +25
Lines 25923 27991 +2068
==========================================
+ Hits 19541 21303 +1762
- Misses 5360 5593 +233
- Partials 1022 1095 +73
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
action/protocol/execution/evm/evm.go
Outdated
| // Merge enabled at Redsea height | ||
| tipHash := protocol.MustGetBlockchainCtx(ctx).Tip.Hash | ||
| h := common.BytesToHash(tipHash[:]) | ||
| evmParams.context.Random = &h |
There was a problem hiding this comment.
The initialization of the Random field should occur in where evmParams is created.
| @@ -432,7 +438,7 @@ func executeInEVM(ctx context.Context, evmParams *Params, stateDB *StateDBAdapte | |||
| remainingGas -= intriGas | |||
|
|
|||
| // Set up the initial access list | |||
| rules := chainConfig.Rules(evm.Context.BlockNumber, g.IsRedsea(blockHeight)) // Merge enabled at Redsea height | |||
| rules := chainConfig.Rules(evm.Context.BlockNumber, evm.Context.Random != nil) | |||
There was a problem hiding this comment.
I still think using g.IsRedsea() seems clearer than evm.Context.Random != nil, although Ethereum does it that way.
There was a problem hiding this comment.
recently I feel more and more that align with Ethereum code is beneficial to our codebase maintenance in the long run, that's why I changed to evm.Context.Random != nil
| @@ -432,7 +438,7 @@ func executeInEVM(ctx context.Context, evmParams *Params, stateDB *StateDBAdapte | |||
| remainingGas -= intriGas | |||
|
|
|||
| // Set up the initial access list | |||
| rules := chainConfig.Rules(evm.Context.BlockNumber, g.IsRedsea(blockHeight)) // Merge enabled at Redsea height | |||
| rules := chainConfig.Rules(evm.Context.BlockNumber, evm.Context.Random != nil) | |||
action/protocol/execution/evm/evm.go
Outdated
| @@ -418,6 +418,12 @@ func executeInEVM(ctx context.Context, evmParams *Params, stateDB *StateDBAdapte | |||
| config = vmCfg | |||
| } | |||
| chainConfig := getChainConfig(g, blockHeight, evmParams.evmNetworkID) | |||
| if g.IsRedsea(blockHeight) { | |||
| // Merge enabled at Redsea height | |||
| tipHash := protocol.MustGetBlockchainCtx(ctx).Tip.Hash | |||
There was a problem hiding this comment.
TIP.Hash is not of randomness. Here is ETH's design for reference: https://github.com/ethereum/consensus-specs/blob/v1.4.0-beta.3/specs/phase0/beacon-chain.md#randao
There was a problem hiding this comment.
yes, in Beacon chain validator's BLS signature is used as the contribution to entropy, and its hash is XORed into the RANDAO during the update. The Beacon chain maintains the states, and the randomness is reflected as MixDigest common.Hash in Ethereum's block header.
Currently we don't have these in place nor the corresponding data field in our block header. We could implement a similar scheme but that would take quite some time.
There was a problem hiding this comment.
for evm, prevrandao() instruction always return a same value (i.e. equals to Random field of BlockCtx) during one block, according to implementation of ethereum. So it's not randomness during one block.
|
SonarCloud Quality Gate failed.
|
|
EIP-4399 to be enabled in future release |
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #(issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: