Skip to content
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

Merged
merged 6 commits into from
Jan 6, 2023

Conversation

MariusVanDerWijden
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden commented Sep 27, 2022

This PR changes Shanghai activation to be based on timestamps instead of block numbers

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

Copy link
Contributor

@roberto-bayardo roberto-bayardo Dec 2, 2022

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.

yperbasis added a commit to erigontech/erigon that referenced this pull request Dec 16, 2022
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>
@MariusVanDerWijden
Copy link
Member Author

Forkid filtering is broken in this pr, needs MariusVanDerWijden#42

@karalabe karalabe force-pushed the shanghai-by-time branch 2 times, most recently from 1bbb82c to ca4002e Compare January 4, 2023 10:35
@karalabe karalabe added this to the 1.11.0 milestone Jan 4, 2023
Copy link
Member

@karalabe karalabe left a 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.

Copy link
Member Author

@MariusVanDerWijden MariusVanDerWijden left a 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

Copy link
Member

@lightclient lightclient left a 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.

core/forkid/forkid.go Outdated Show resolved Hide resolved
core/headerchain.go Outdated Show resolved Hide resolved
core/headerchain.go Outdated Show resolved Hide resolved
Copy link
Member

@rjl493456442 rjl493456442 left a 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!

@karalabe karalabe merged commit 2189773 into ethereum:master Jan 6, 2023
Rjected added a commit to Rjected/ethers-rs that referenced this pull request Jan 12, 2023
 * 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.
Rjected added a commit to Rjected/ethers-rs that referenced this pull request Jan 12, 2023
 * 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.
gakonst pushed a commit to gakonst/ethers-rs that referenced this pull request Jan 13, 2023
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants