-
Notifications
You must be signed in to change notification settings - Fork 21.5k
core/evm: Random opcode (EIP-4399) #24141
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
Conversation
holiman
left a comment
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.
Generally looks kind of ok, but not sure -- maybe it can be done even simpler, somehow...
core/vm/opcodes.go
Outdated
| CHAINID OpCode = 0x46 | ||
| SELFBALANCE OpCode = 0x47 | ||
| BASEFEE OpCode = 0x48 | ||
| RANDOM OpCode = 0x44 // Same as DIFFICULTY |
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.
Please keep the sort order
tests/state_test_util.go
Outdated
| if t.json.Env.Random != nil { | ||
| // Post-Merge | ||
| genesis.Mixhash = common.BigToHash(t.json.Env.Random) | ||
| genesis.Difficulty = nil |
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.
nil or 0?
core/evm.go
Outdated
| Difficulty: new(big.Int).Set(header.Difficulty), | ||
| BaseFee: baseFee, | ||
| GasLimit: header.GasLimit, | ||
| Random: header.MixDigest, |
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.
Nitpick, I think we should keep this as nil before the merge, just to catch any code accidentally touching this field prematurely.
core/evm.go
Outdated
| BaseFee: baseFee, | ||
| GasLimit: header.GasLimit, | ||
| Random: header.MixDigest, | ||
| IsPostMerge: header.Difficulty.Cmp(common.Big0) == 0, |
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.
Do we need this as a field though? We do pass in the difficult anyway, couldn't we just compare in there? Or look at the random field and see if it's set or not?
core/vm/evm.go
Outdated
| Time *big.Int // Provides information for TIME | ||
| Difficulty *big.Int // Provides information for DIFFICULTY | ||
| BaseFee *big.Int // Provides information for BASEFEE | ||
| Random common.Hash // Provides information for RANDOM |
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.
Couldn't we have this be *common.Hash and use it as a signal for pre/post-merge-ness?
|
I'm currently testing using the following: alloc.json: txs.rlp: With the evm command: And expect to see the storage set to the value of currentRandom, but the storage is unset in the output. |
|
@marioevz I have the following state test: which produces |
|
Found and fixed: Thanks for noticing @marioevz ! |
…ying the merge chainrules
karalabe
left a comment
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.
SGTM
|
Wondering long term if we could "revert" this extra flag to use block numbers. Would be tempted to say after the merge after the block number is finalized, but that would prevent other networks from switching over. Hmm, maybe we can just let other networks switch over based on block number? Probably no because of the TTD. Ugh. |
|
I would also say that we should revert it once we have a block height. We might even hardcode the terminal blockhash for mainnet afterwards |
* core: implement eip-4399 random opcode * core: make vmconfig threadsafe * core: miner: pass vmConfig by value not reference * all: enable 4399 by Rules * core: remove diff (f) * tests: set proper difficulty (f) * smaller diff (f) * eth/catalyst: nit * core: make RANDOM a pointer which is only set post-merge * cmd/evm/internal/t8ntool: fix t8n tracing of 4399 * tests: set difficulty * cmd/evm/internal/t8ntool: check that baserules are london before applying the merge chainrules
This PR implements EIP-4399, the RANDOM opcode post-merge.
Since we don't know the merge block number beforehand, we need to somehow signal to the EVM that the Random opcode should be used now. This is currently done by setting the
isMergeto true which is done in the block context, if the block has a difficulty of 0.Additionally I extended the StateTestUtil to be able to create state tests with the RANDOM opcode. An example for that later.
replaces #23985, #24068