-
Notifications
You must be signed in to change notification settings - Fork 946
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
Add invalid large withdrawable epoch test #2887
Add invalid large withdrawable epoch test #2887
Conversation
What's the value of this test if it's impossible to occur in any practical situation? |
I believe there is value in having all of the clients behave the same even in impossible situations. The spec states that "state transitions that cause a uint64 overflow or underflow are also considered invalid." This test is just verifying that the clients consider this state transition as invalid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test looks good although I'd suggest tightening the scope of what we are testing
the logic you target is in initiate_validator_exit
so we can test that function instead of the broader process_registry_updates
I'd suggest doing the same setup you have here but testing just that one function (and moving this code to reflect the corresponding spec structure)
Good idea. I agree with tightening scope. Where do you suggest I move this test to? I was thinking |
Moved it to |
I'm sligntly inclined to (ii) because it may be fragmental to create a new test format only for |
I would prefer to keep this as testing the epoch processing functions directly to be able to build tests in a generic way. You can also add a unit test (or a few) for |
yeah I agree that making a new test format is a lot of overhead and I thought we would have already exposed this function to the testgen infra so in my original comment I thought it would be a much simpler change :) in this case I think we can either revert to the original |
@hwwhww Thank you for your review. Feel free to make changes. Do I need to |
0ac25bc
to
89e7a10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Thank you! |
PR Description
I believe this is an impossible situation to be in, but this PR tests an edge case in
initiate_validator_exit
. It is possible to cause an overflow when calculating a new withdrawable epoch. When fuzzing state transitions, we discovered that one of the clients would crash in this situation; so we should add a test.