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

chore(payload): fix withdrawals field pre-shanghai in Ethereum payload #12828

Merged
merged 1 commit into from
Nov 24, 2024

Conversation

fgimenez
Copy link
Member

We were getting a hive error Unexpected Failures: ['GetPayloadBodiesByRange (Empty Transactions/Withdrawals) (Paris) (reth)'] in the ethereum-withdrawals suite after #12780, like in https://github.com/paradigmxyz/reth/actions/runs/11982076699/job/33409959229, withdrawals field should be None pre-shanghai

@fgimenez fgimenez added A-block-building Related to block building C-hivetest Used for labelling automated issues relating to hive test failures labels Nov 24, 2024
@fgimenez
Copy link
Member Author

successful hive run from this branch https://github.com/paradigmxyz/reth/actions/runs/11995148078

@fgimenez fgimenez added this pull request to the merge queue Nov 24, 2024
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ah sigh, I missed this in #12780

Merged via the queue into main with commit e020eb7 Nov 24, 2024
55 checks passed
@fgimenez fgimenez deleted the fgimenez/fix-hive-get-payloads-withdrawals-field branch November 24, 2024 10:45
@hai-rise
Copy link
Contributor

hai-rise commented Nov 25, 2024

Totally missed commit_withdrawals returning None pre-Shanghai. Sorry guys 🫠.

Also surprised Hive would expect an overwritten None when clients like Geth reject such a payload with an error. Will have a look at Hive if I can 🙏.

@fgimenez
Copy link
Member Author

Totally missed commit_withdrawals returning None pre-Shanghai. Sorry guys 🫠.

sure np at all, thanks for your contributions!

Also surprised Hive would expect an overwritten None when clients like Geth reject such a payload with an error. Will have a look at Hive if I can 🙏.

in fact the test is exercising engine_getPayloadBodiesByRangeV1, the error we were getting was 2024-11-24T00:36:49.8832431Z FAIL (GetPayloadBodiesByRange (Empty Transactions/Withdrawals)): Unexpected payload body on EngineGetPayloadBodiesV1 at index 0: incorrect withdrawals: want: null, got: [] the test is defined here if you want to take a look https://github.com/ethereum/hive/blob/master/simulators/ethereum/engine/suites/withdrawals/tests.go#L678-L709

@hai-rise
Copy link
Contributor

hai-rise commented Nov 25, 2024

@fgimenez Thank you for the info and fixing the regression!

I think I understand my confusion now. In Geth Withdrawals can be nil in both new payload requests and built block bodies (both use ExecutableData):
https://github.com/ethereum-optimism/op-geth/blob/23fc61420c904ae9d7b8e5978b39bedd469cd283/beacon/engine/types.go#L77-L92

That's why they confidently forward it from the payload attributes to the built block body:
https://github.com/ethereum-optimism/op-geth/blob/23fc61420c904ae9d7b8e5978b39bedd469cd283/beacon/engine/types.go#L313

However, in Alloy, we parse null withdrawals as an empty Vec in new payload requests:
https://github.com/alloy-rs/alloy/blob/1ef28dfb6b97c580a08e5750569e385555c0747c/crates/rpc-types-engine/src/payload.rs#L214

While Reth does Option<Withdrawals> for BlockBody.

pub withdrawals: Option<Withdrawals>,

Ideally, ExecutionPayloadV2 would also be Option for us to forward and match the value between the payload request and the built block as Geth does. So we'd forward a None withdrawals correctly in the above test, instead of parsing null input as [] then need to check Shanghai to revert to null.

A change in Alloy is very noisy so may not worth it to be honest 😅. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-block-building Related to block building C-hivetest Used for labelling automated issues relating to hive test failures
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants