Skip to content

Conversation

@leolara
Copy link
Member

@leolara leolara commented Nov 27, 2025

This PR adds more tests that are needed for get_expected_withdrawals.

This part of the spec is non-trivial and have evolved 3 times. In EIP7732 it is changed significantly. More tests are needed.

Currently they don't produce tests vectors, but this will be archived with #4755

@leolara leolara marked this pull request as ready for review November 28, 2025 09:08
@leolara leolara changed the title Add more get_expected_withdrawals Add more get_expected_withdrawals tests Nov 28, 2025
@leolara leolara requested a review from jtraglia November 28, 2025 21:28
@jtraglia jtraglia added the testing CI, actions, tests label Dec 1, 2025
@jtraglia jtraglia changed the title Add more get_expected_withdrawals tests Add more get_expected_withdrawals tests for Gloas Dec 1, 2025
@jtraglia jtraglia changed the title Add more get_expected_withdrawals tests for Gloas Add more get_expected_withdrawals tests Dec 1, 2025
Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

Hey @leolara I'm not sure unit tests (that don't produce reference tests) are that valuable here.

This function is tested in run_withdrawals_processing helper:

expected_withdrawals = get_expected_withdrawals(spec, state)
assert len(expected_withdrawals) <= spec.MAX_WITHDRAWALS_PER_PAYLOAD
if num_expected_withdrawals is not None:
assert len(expected_withdrawals) == num_expected_withdrawals

It would be better to review/extend the existing withdrawals tests.

For Gloas, maybe help Terence get this PR merged?

@leolara
Copy link
Member Author

leolara commented Dec 2, 2025

I think those are for tests of process_withdrawals not for get_expected_withdrawals that should also be tested in isolation. And will produce vector tests wtih #4755

I don't think to restrict our tests to only check get_expected_withdrawals together with process_withdrawals makes us have a good coverage. We should have tests that test each in isolation and also combined

@IvanAnishchuk
Copy link

Admittedly the tracing framework takes a while to polish and fully integrate into reftests system but we are getting there and it is already basically functional and tracing these tests (for example) is just a matter of adding a single decorator (I tested compatibility, can show some traces)

@leolara leolara closed this Dec 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing CI, actions, tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants