-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix: enforce unsupported fork rules on get_payload_v3 #4562
fix: enforce unsupported fork rules on get_payload_v3 #4562
Conversation
Codecov Report
... and 12 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more.
|
// were to resolve the payload here, and it is before cancun, then it's possible a | ||
// subsequent call to `get_payload_v3` would return `UnknownPayload` because the payload | ||
// may no longer exist in the payload store. |
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 don't understand this,
The timestamp is fixed, why would we expect a subsequent call to get payload?
it's a bit weird that we're effectively calling a getter twice.
we could add a new function to the payloadstore that returns the payloadattributes that would be cleaner than returning the block twice
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.
We typically wouldn't expect another call, but this is how one of the hive tests works unfortunately
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.
Just added another method to the payloadstore that returns the payloadattributes
return Err(EngineApiError::UnsupportedFork) | ||
} | ||
|
||
// Now resolve the payload |
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.
tbh I would consider this a CL bug
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 like this
lgtm
@Rjected somethings needs to be updated in docs
We did not enforce the following rule for returned payloads in
engine_getPayloadV3
:This adds an additional check, causing the
GetPayloadV3 To Request Shanghai Payload
cancun hive test to pass.