-
Notifications
You must be signed in to change notification settings - Fork 623
fix: parentBeaconBlockRoot fallback in engine_newPayloadV4 #10072
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
base: master
Are you sure you want to change the base?
fix: parentBeaconBlockRoot fallback in engine_newPayloadV4 #10072
Conversation
| if (parentBeaconBlockRoot is null) | ||
| // Use parentBeaconBlockRoot parameter if provided, otherwise fall back to executionPayload.ParentBeaconBlockRoot | ||
| // This handles cases where op-node sends valid requests with parentBeaconBlockRoot in the payload itself | ||
| Hash256? finalParentBeaconBlockRoot = parentBeaconBlockRoot ?? executionPayload.ParentBeaconBlockRoot; |
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.
but normally executionPayload.ParentBeaconBlockRoot is null and is set few lines below. Unless the mechanism is different in Optimism and it comes from PayloadDecoder?
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've been taking a look at our implementation of the OP-CL and also in OP-node, and in both cases the ParentBeaconBlockRoot sent as parameter is the same in the executionPayload.ParentBeaconBlockRoot parameter, so the assignment is a no-op (this is the case for all post-Ecotone blocks).
The proposed fix most likely does not work as intended. Have you tested this @forkfury ?
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.
@emlautarom1 What about others op-cl implementations?
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.
AFAIK there are only two implementations: op-node (official CL) and ours.
|
We are looking into this as a potential fix but we're not fully sure that it's the correct approach or even if it will have any semantic effect. Could you add the following logs:
|
LukaszRozmej
left a comment
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.
@forkfury please add above logging
Fix
Fixes #9921 - Occasional "Parent beacon block root must be set" error when processing
engine_newPayloadV4requests from op-node.Changes
ExecutionPayloadParams.ValidateParams()to fall back toexecutionPayload.ParentBeaconBlockRootwhen theparentBeaconBlockRootparameter is nullparentBeaconBlockRootin the payload object instead of as a method parameter