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

Division by zero in get_attestation_deltas or wrong min value of get_total_balance #1663

Closed
MrChico opened this issue Mar 16, 2020 · 5 comments

Comments

@MrChico
Copy link
Member

MrChico commented Mar 16, 2020

A comment in get_total_balance reads that the function returns a minimum of 1 Gwei to avoid divisions by zero.

But since #1635, total_balance must be at least EFFECTIVE_BALANCE_INCREMENT in order to avoid a division by zero in this line of get_attestation_deltas:

rewards[index] = reward_numerator // (total_balance // increment)

It seems that either the minimum of total_balance should be raised, or the reward calculation be adjusted to avoid this edge case.

@MrChico
Copy link
Member Author

MrChico commented Mar 16, 2020

In a similar vein, if a slashed validators effective balance drops under EFFECTIVE_BALANCE_INCREMENT, it seems they will not suffer the anti-correlation penalty applied in process_slashings, as the expression:

penalty_numerator = validator.effective_balance // increment * min(sum(state.slashings) * 3, total_balance)

will evaluate to 0.

@djrtwo
Copy link
Contributor

djrtwo commented Mar 16, 2020

Right. The only way their effective balance could drop below EFFECTIVE_BALANCE_INCREMENT is if it went to 0 (because effective balance is only changed on that increment) which would give them zero penalty regardless.

That said, maybe penalty_numerator here should be in terms of balance rather than effective_balance

@djrtwo
Copy link
Contributor

djrtwo commented Mar 16, 2020

@MrChico I addressed the first issue here #1664

Eating lunch, thinking about the second

@MrChico
Copy link
Member Author

MrChico commented Mar 16, 2020

Ah, yes, I see now that the second concern is not an issue.
It seems that all other reward calculations are being weighted by total_balances and the validators effective balance, so I don't see why this one should be different?

@djrtwo
Copy link
Contributor

djrtwo commented Mar 17, 2020

Yeah, no need for this to be different.

Just an orthogonal thought, and one that I don't think I agree with.

@djrtwo djrtwo closed this as completed Mar 26, 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

No branches or pull requests

2 participants