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

More capella tests #3227

Merged
merged 6 commits into from
Jan 27, 2023
Merged

More capella tests #3227

merged 6 commits into from
Jan 27, 2023

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Jan 25, 2023

Add tests of mixing top-ups and withdrawals, rename old test case

@hwwhww hwwhww marked this pull request as ready for review January 26, 2023 09:37
@hwwhww hwwhww requested a review from djrtwo January 26, 2023 09:37
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

nice!

yield 'blocks', [signed_block]
yield 'post', state

validator = state.validators[validator_index]
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe write a comment that withdrawals happen before deposits so no partial withdrawal is made in the prior transition (just to make explicit)

state.validators[validator_index], state.balances[validator_index])

# Getting attester rewards and getting partial withdrawals
for _ in range(2):
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the 2 magic number? to go through all validators and thus partially withdraw this validator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of 1 epoch for balance updates, 1 epoch for withdrawals + balance updates.

pre_tag,
post_tag,
operation_type=OperationType.BLS_TO_EXECUTION_CHANGE,
operation_at_slot=fork_epoch * spec.SLOTS_PER_EPOCH - 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this fail because you can't include BLS_TO_EXECUTION_CHANGE operations before the fork?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point, it should have been in eip4844 folder for testing capella -> eip4844 fork transition.

@hwwhww hwwhww force-pushed the more-capella-tests branch from 8e0bd44 to a87f727 Compare January 27, 2023 10:05
@hwwhww hwwhww force-pushed the more-capella-tests branch from a87f727 to 9ab1478 Compare January 27, 2023 10:16
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

nice!

@hwwhww hwwhww merged commit 26d233a into dev Jan 27, 2023
@hwwhww hwwhww deleted the more-capella-tests branch January 27, 2023 10:24
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.

2 participants