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

Low amounts of collective penalties during slashing are ignored. #1322

Closed
ralexstokes opened this issue Jul 26, 2019 · 3 comments
Closed

Low amounts of collective penalties during slashing are ignored. #1322

ralexstokes opened this issue Jul 26, 2019 · 3 comments
Labels
post-freeze (substantive) Substantive consensus change non-critical for long-lived cross-client testnets

Comments

@ralexstokes
Copy link
Member

ralexstokes commented Jul 26, 2019

After the change in #1284, we can get into a situation where the collective penalty calculated in process_slashings can be 0, even when there are a non-zero amount of per-epoch penalties in state.slashings.

For example:

Validator count: 10, each at 32 ETH effective balance.
sum(state.slashings): 64 ETH (let's say 2 validators who are slashed).
min(sum(state.slashings) * 3, total_balance): 192 ETH
min(...) // total_balance: 192 ETH // 256 ETH
=> gives a collective penalty of 0, ignoring the 64 ETH of slashings. I'd imagine we want to include these penalties otherwise there is an implicit floor on the amount of slashings needed before any collective penalty kicks in.

@djrtwo
Copy link
Contributor

djrtwo commented Jul 26, 2019

Nice find! damn integer division...

I supposed we can normalize all the balance calculations here :/

@vbuterin
Copy link
Contributor

vbuterin commented Jul 27, 2019

The actual expression is:

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

validator.effective_balance // increment equals 32, and it gets multiplied by min(sum(state.slashings) * 3, total_balance) before being divided by the total balance. So it seems like the penalty would be zero when the total slashings are under 1/96 of the total balance, though note that there's still a 1 ETH base penalty that slashed validators would have to pay in this context.

So I actually think it's fine as is, all it means is that penalties are rounded down to the next whole ETH, but if we want we could decrease increment from 1 ETH to 0.01 ETH to get more accuracy.

@JustinDrake JustinDrake added the post-freeze (substantive) Substantive consensus change non-critical for long-lived cross-client testnets label Aug 20, 2019
@djrtwo
Copy link
Contributor

djrtwo commented Dec 12, 2019

As vitalik noted, this rounding down will be fine in practice, and in most cases make punishment of accidental slashings less aggressive which is good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
post-freeze (substantive) Substantive consensus change non-critical for long-lived cross-client testnets
Projects
None yet
Development

No branches or pull requests

4 participants