-
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
eth/catalyst: fix engine API #28135
eth/catalyst: fix engine API #28135
Conversation
return fmt.Errorf("invalid parent beacon block root: %w", err) | ||
} | ||
return nil | ||
} | ||
|
||
func checkAttribute(active func(*big.Int, uint64) bool, exists bool, time uint64) error { | ||
if active(common.Big0, time) && !exists { | ||
func checkAttribute(active func(*big.Int, uint64) bool, exists bool, block *big.Int, time uint64) error { |
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.
The london block is used to align with line https://github.com/ethereum/go-ethereum/pull/28135/files#diff-96840e4882901b94cef9f51a992a44e64df659335ff13dd011be8c1ffa097135R179
Although I don't think london block is correct here. The block number to generate is the one should pass, IIUC.
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.
The original code passed the weird london block because in merge mode we know that we already passed london. So instead of complicating everything with passing a block number, it was a hacky way to hard code "always london".
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
This reverts commit 15b7c0b.
This reverts commit 15b7c0b.
eth/catalyst: fix engine API (ethereum#28135) Conflicts: eth/catalyst/api.go Conflicts due to us adding arbosversion to checkAttribute. Accepted both. Test fails: metrics/influxdb tests fail here, but they also fail on the merged upstream commit
Fixes #28134