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: implement EIP-2935 #29465
base: master
Are you sure you want to change the base?
core: implement EIP-2935 #29465
Conversation
core/vm/eips.go
Outdated
|
||
func getBlockHashFromContract(number uint64, statedb StateDB) common.Hash { | ||
ringIndex := number % 256 | ||
var pnum 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.
pnum
isn't very helpful. pNum
? but what is p
?
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.
how about parentNumber
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.
Renamed to slot as it is the encoded storage key for a given ancestor.
core/vm/eips.go
Outdated
func enable2935(jt *JumpTable) { | ||
jt[BLOCKHASH] = &operation{ | ||
execute: opBlockhash2935, | ||
// TODO: gas params |
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.
Leftover note to self?
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 ended up deleting the new BLOCKHASH implementation. We can just continue how we did it before. Specially since the spec doesn't update the gas price of the opcode.
@s1na, pinging you to be aware of some extra changes that happened on Kaustinen after you pulled the work from the other branch, see: gballet#411, which some days after where also updated in the EIP ethereum/EIPs@2b73ecb |
if p.config.IsPrague(block.Number(), block.Time()) { | ||
ProcessBlockHashHistory(statedb, block.Header(), p.config, p.bc) | ||
} |
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.
This logic is a bit convoluted. There are basically two paths:
- On the exact fork-block: we need to iterate and add 256 hashes
- On every block after the fork: we need to add the parent hash.
The code, as written, makes path 1
the 'default-path'. We always enter the ProcessBlockHashHistory
, which invokes ProcessParentBlockHash
and just exits before iterating. IMO it would be more intuitive to default to path 2
, invoke ProcessParentBlockHash
by default, and only ever once invoke ProcessBlockHashHistory
.
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.
Think of it this way: the parent's block hash is always inserted. The other 8191 block hashes are only inserted in a certain condition after the parent one. Also having one wrapping method makes the footprint inside state_processor, miner etc. smaller.
64e49c2
to
62cf853
Compare
Co-authored-by: Guillaume Ballet <gballet@gmail.com> Co-authored-by: Ignacio Hagopian <jsign.uy@gmail.com>
Copied over and adapted from https://github.com/gballet/go-ethereum/tree/kaustinen-with-shapella, following the latest changes to the spec: ethereum/EIPs#8166