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

Update reward calculations to v0.11.0 - Handle u64 overflow #921

Merged
merged 1 commit into from Mar 19, 2020
Merged

Update reward calculations to v0.11.0 - Handle u64 overflow #921

merged 1 commit into from Mar 19, 2020

Conversation

ghost
Copy link

@ghost ghost commented Mar 17, 2020

Issue Addressed

#920

Proposed Changes

  • Divides by EFFECTIVE_BALANCE_INCREMENT both dividend and divisor on reward calculations

  • Suffix _ebi on each effected variable (To avoid any future confusion)

Additional Info

@ghost ghost changed the title Update reward calculations to v0.11.0 - Handle u64 overflow (#920) Update reward calculations to v0.11.0 - Handle u64 overflow Mar 17, 2020
@paulhauner
Copy link
Member

Thanks @hermanjunge!

@michaelsproul perhaps you'd like to check over this one as well?

This PR would put us in a middle-ground between spec v0.10.x and v0.11.x, but that's probably not too bad considering we don't expect to see this scenario on testnets. We're going to need to get to v0.11.x before we get to multi-client testnet anyway since that's where the networking spec is functional.

@paulhauner paulhauner added the under-review A reviewer has only partially completed a review. label Mar 17, 2020
@paulhauner
Copy link
Member

Also, @hermanjunge it would be handy for future PRs if you could enable Github Actions on your end then our CI will run :)

@ghost
Copy link
Author

ghost commented Mar 17, 2020

@paulhauner Done. Will squash both commits!

@ghost
Copy link
Author

ghost commented Mar 17, 2020

Jobs completed. I will avoid making a new PR with this other branch. Too much noise. https://github.com/hermanjunge/lighthouse/actions/runs/57159324

Next time PR will be from a branch different than master.

Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

This looks good to me. It doesn't break any functionality of v0.10.x that was worth keeping, so I'm fine to merge and exist "between specs" for a bit.

@paulhauner
Copy link
Member

I've triggered CI on our end :)

@paulhauner paulhauner merged commit 70e39cc into sigp:master Mar 19, 2020
@paulhauner paulhauner added ready-to-squerge and removed under-review A reviewer has only partially completed a review. labels Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants