-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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
params: core: enable shanghai based on timestamps #25878
Conversation
params/config.go
Outdated
func (c *ChainConfig) IsShanghai(num *big.Int) bool { | ||
return isForked(c.ShanghaiBlock, num) | ||
// IsShanghai returns whether time is either equal to the Shanghai fork time or greater. | ||
func (c *ChainConfig) IsShanghai(time *big.Int) bool { |
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.
func (c *ChainConfig) IsShanghai(time *big.Int) bool { | |
func (c *ChainConfig) IsShanghai(time uint64) bool { |
It would be nice if either this took a uint64
or if we don't want IsShanghai
to take a different parameter type than other isForked
methods, maybe a new getter on types.Block
and types.Header
"TimeBig()
" so that each time we check IsShanghai
, don't need to jump through a bunch of casting hoops?
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.
It might be better to have a different type here to detect at compile time if someone is trying to pass in a block number instead of a timestamp. e.g. there are calls to IsShanghai() in the withdrawals PR and EIP-4844 repo that would need to be updated but would not be flagged by the compiler.
2d85996
to
1cd8782
Compare
Starting from [Shanghai](https://github.com/ethereum/execution-specs/blob/master/network-upgrades/mainnet-upgrades/shanghai.md), forks are based on timestamps rather than block heights (see PR #6238). This PR extends [EIP-2124](https://eips.ethereum.org/EIPS/eip-2124) Fork ID to include timestamp-based blocks. See also ethereum/go-ethereum#25878. Co-authored-by: Marius van der Wijden <m.vanderwijden@live.de>
Forkid filtering is broken in this pr, needs MariusVanDerWijden#42 |
495ffd7
to
a2d3873
Compare
1bbb82c
to
ca4002e
Compare
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, but I made a ton of changes, so would appreciate a review from someone else now.
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, reviewed @karalabe's latest changes
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.
Took a look at the new changes and tests, generally all seem good.
ca4002e
to
3669ca6
Compare
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.
Mostly looks good to me!
3669ca6
to
754f1b0
Compare
ae1330d
to
b56c796
Compare
* geth switched to time-based forking in ethereum/go-ethereum#25878, this changes the name of the `shanghai_block` field to `shanghaiTime` so it is compatible with geth.
* geth switched to time-based forking in ethereum/go-ethereum#25878, this changes the name of the `shanghai_block` field to `shanghaiTime` so it is compatible with geth.
* geth switched to time-based forking in ethereum/go-ethereum#25878, this changes the name of the `shanghai_block` field to `shanghaiTime` so it is compatible with geth.
This PR changes Shanghai activation to be based on timestamps instead of block numbers