Skip to content

Add test for execution payload with wrong withdrawals#4856

Merged
jtraglia merged 5 commits into
ethereum:masterfrom
ensi321:payload-withdrawal-test
Jan 30, 2026
Merged

Add test for execution payload with wrong withdrawals#4856
jtraglia merged 5 commits into
ethereum:masterfrom
ensi321:payload-withdrawal-test

Conversation

@ensi321

@ensi321 ensi321 commented Jan 22, 2026

Copy link
Copy Markdown
Contributor

Noticed spec test is missing a test vector where payload has wrong withdrawals.

Also fix the default value of builder index and one's not provided in prepare_signed_execution_payload_bid

@jtraglia jtraglia added the testing CI, actions, tests, testing infra label Jan 22, 2026

@jtraglia jtraglia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

@jtraglia jtraglia requested a review from leolara January 27, 2026 18:45

@leolara leolara left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!!

The test is good, but I would like the name of the test and the docstring to be more clear (without being excesively long).

I would suggest:

def test_process_execution_payload_missing_expected_withdrawal
  """                                                                                                                                                                                                                                                                                                                                                                                                                
  Verify payload rejected when it omits a withdrawal expected by the state.                                                                                                                                                                                                                                                                                                                                          
  """ 

This will help future test implementers to understand better what has been implemented. For example the withdrawals could be wrong for having something in execution_payload.withdrawals that is not in state.payload_expected_withdrawals. So that would be a different test. Even if in the specs it is clear that will be catched by the same assertion, we are testing this fact for the clients with the test vectors.

@jihoonsong

jihoonsong commented Jan 28, 2026

Copy link
Copy Markdown
Member

I agree that having more explicit function name and docstring is better. Given that other adjacent functions to the newly added one also have "wrong_XYZ" suffix, how about having another PR that improves them, including this one? cc @leolara

@jtraglia jtraglia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks @ensi321! I've renamed the function like @leolara suggested. @leolara you should consider following up with another PR which improves function names/docstrings for other tests in this file.

@jtraglia jtraglia disabled auto-merge January 30, 2026 22:15
@jtraglia jtraglia merged commit 44968d7 into ethereum:master Jan 30, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing CI, actions, tests, testing infra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants