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

eth/catalyst: prefix payload id with version #28246

Merged
merged 3 commits into from
Jan 24, 2024

Conversation

lightclient
Copy link
Member

this PR is built on top of #28230

Technically, GetPayloadVX should only return payloads which match its version. GetPayloadV2 is a special snowflake that supports v1 and v2 payloads. Originally I thought to either i) plumb the version through the entire building process so when the payload was retrieved in the API request, it would be possible to determine if the call was valid or ii) to ascertain from the payload itself which version it would have been requested through.

@holiman had a much better idea, which is to prefix the payload id with the version number. This involved minimal plumbing and pretty straightforward integration in GetPayloadVX.

eth/catalyst/api.go Outdated Show resolved Hide resolved
@lightclient lightclient force-pushed the prefix-payload-id branch 3 times, most recently from 89d0560 to 05d844c Compare October 16, 2023 19:41
@holiman
Copy link
Contributor

holiman commented Nov 28, 2023

Why is this still draft? Want to get it merged?

@lightclient
Copy link
Member Author

With this PR applied:

Screenshot 2024-01-22 at 14 32 45

@holiman
Copy link
Contributor

holiman commented Jan 23, 2024

Please rebase now that #28230 is merged

@lightclient lightclient force-pushed the prefix-payload-id branch 2 times, most recently from 0ea3c04 to 9a9f549 Compare January 23, 2024 17:00
@lightclient lightclient marked this pull request as ready for review January 23, 2024 17:50
@lightclient
Copy link
Member Author

rebased!

@holiman holiman added this to the 1.13.11 milestone Jan 23, 2024
@holiman
Copy link
Contributor

holiman commented Jan 23, 2024

--- FAIL: TestEth2PrepareAndGetPayload (0.13s)
    api_test.go:216: error getting payload, err=Unsupported fork
--- FAIL: TestNewPayloadOnInvalidChain (0.08s)
    api_test.go:636: can't get payload: Unsupported fork
--- FAIL: TestWithdrawals (0.03s)
    api_test.go:1082: error getting payload, err=Unsupported fork
--- FAIL: TestNilWithdrawals (0.03s)
    api_test.go:1268: error getting payload, err=Unsupported fork
...

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM once the testcases become 🟢

@lightclient lightclient force-pushed the prefix-payload-id branch 2 times, most recently from 519ce52 to 190e40a Compare January 23, 2024 21:18
@lightclient
Copy link
Member Author

Ah good catch, should be fixed now!

@holiman holiman merged commit a8a8758 into ethereum:master Jan 24, 2024
2 of 3 checks passed
Dergarcon pushed a commit to specialmechanisms/mev-geth-0x2mev that referenced this pull request Jan 31, 2024
GetPayloadVX should only return payloads which match its version. GetPayloadV2 is a special snowflake that supports v1 and v2 payloads. This change uses a a version-specific prefix within in the payload id, basically a namespace for the version number.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants