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

Fix for issue 4860 - Added in process_justification_and_finalization #4877

Merged

Conversation

zack-scott
Copy link
Contributor

Issue Addressed

This PR addresses the issue of compute_attestation_rewards_altair not taking into account the justified epochs attestation rewards when coming out of an inactivity leak.

Which issue # does this PR address?
#4860

Proposed Changes

Added in process_justification_and_finalization to the compute_attestation_rewards_altair function as suggested in issue.

Please list or describe the changes introduced by this PR.
The changes allow for calculation of attestation rewards when coming out of an inactivity leak.

Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.

Added in process_justification_and_finalization to compute_attestation_rewards_altair to take into account justified attestations when coming out of inactivity leak. Also added in test to check for this edge case.
@chong-he chong-he added bug Something isn't working work-in-progress PR is a work-in-progress HTTP-API labels Oct 24, 2023
@zack-scott zack-scott marked this pull request as ready for review October 24, 2023 21:24
@zack-scott
Copy link
Contributor Author

@jimmygchen hey, I added in the logic process_justification_and_finalization for the compute_attestation_rewards_altair function, but I wasn't sure if we also needed to add in the process_justification_and_finalization to the phase0 logic compute_attestation_rewards_base as well?

@chong-he chong-he added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Oct 25, 2023
@michaelsproul
Copy link
Member

I wasn't sure if we also needed to add in the process_justification_and_finalization to the phase0 logic compute_attestation_rewards_base as well?

Good point. I think we do, as phase0 rewards calculation also uses is_in_inactivity_leak.

@zack-scott
Copy link
Contributor Author

zack-scott commented Oct 25, 2023

I wasn't sure if we also needed to add in the process_justification_and_finalization to the phase0 logic compute_attestation_rewards_base as well?

Good point. I think we do, as phase0 rewards calculation also uses is_in_inactivity_leak.

@michaelsproul sounds good, I will get the logic added for phase0 and tested as well

@zack-scott zack-scott marked this pull request as draft October 25, 2023 14:40
@zack-scott zack-scott marked this pull request as ready for review October 25, 2023 21:38
@zack-scott
Copy link
Contributor Author

@michaelsproul I have added in the logic for phase0 to process the justification and finalization and added in tests to ensure it is working as expected.

@zack-scott
Copy link
Contributor Author

@michaelsproul @jimmygchen this PR is ready for review

@jimmygchen jimmygchen self-requested a review November 9, 2023 03:46
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @zack-scott

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Nov 10, 2023
@jimmygchen jimmygchen merged commit e181741 into sigp:unstable Nov 16, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working HTTP-API ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants