-
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
core/vm, params: ensure order of forks, prevent overflow #29023
Conversation
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
params/config.go
Outdated
@@ -910,6 +910,11 @@ func (c *ChainConfig) Rules(num *big.Int, isMerge bool, timestamp uint64) Rules | |||
if chainID == nil { | |||
chainID = new(big.Int) | |||
} | |||
if c.IsShanghai(num, timestamp) { |
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 think it's pretty weird. It can happen that:
London, Merge, Shanghai forks are all configured, but merge is not activated (ttd is not reached). In this case even the London is activated and pre-configured shanghai timestamp is reached, Shanghai shouldn't be enabled.
23773e5
to
64d32e5
Compare
This PR fixes an overflow which can could happen if inconsistent blockchain rules were configured. Additionally, it tries to prevent such inconsistencies from occurring by making sure that merge cannot be enabled unless previous fork(s) are also enabled. (cherry picked from commit ac0ff04) aka ethereum/go-ethereum#29023
core/vm, params: ensure order of forks, prevent overflow (ethereum#29023)
…ereum#29023)" This reverts commit 1c12cf9.
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.
Ko
This PR fixes an overflow which can could happen if inconsistent blockchain rules were configured. Additionally, it tries to prevent such inconsistencies from occurring by making sure that merge cannot be enabled unless previous fork(s) are also enabled.
Without this PR, it is possible to set the overrides on an
eth_call
over RPC and trigger a method-handler crash (not a full node crash).Hat-tip to @pleasew8t for reporting this via the bounty via a very well-written submission.