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

Randomize validator index in partial withdrawal test #3892

Merged
merged 3 commits into from
Aug 29, 2024
Merged

Conversation

ensi321
Copy link
Contributor

@ensi321 ensi321 commented Aug 21, 2024

Background

Raised by @pk910 in discord that Lodestar forked off in devnet 2 on partial withdrawals. After debugging, it was because Lodestar incorrectly checks validator with index state.next_withdrawal_index instead of withdrawal.index for partial withdrawal eligibility in get_expected_withdrawals.

See discord discussion for detail.

This bug is not caught by partial withdrawal spec test because every test case uses fixed validator index
validator_index = spec.get_active_validator_indices(state, current_epoch)[0]
which is 0 and it coincides with the default value of state.next_withdrawal_index which is 0 as well.

Proposal

Make each test pick validator index at random ie.

validator_index = rng(spec.get_active_validator_indices(state, current_epoch))

@mkalinin
Copy link
Collaborator

Do we leave any test with 0 index? Might be helpful for testing as it is one of the boundary values

@ensi321
Copy link
Contributor Author

ensi321 commented Aug 22, 2024

Do we leave any test with 0 index? Might be helpful for testing as it is one of the boundary values

That's a good point. Updated.

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

makes sense!

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

lgtm. Thank you @ensi321!

@hwwhww hwwhww merged commit daf1002 into ethereum:dev Aug 29, 2024
26 checks passed
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.

5 participants