Eth2.0-specs: Slashing penalty overflows

[terencechain]

I think it'd be a good idea to make comments on the places which uint64 overflow can happen and big int should be used. Since the default primitive is uint64, expression like:

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

results in a differently because validator.effective_balance * min(sum(state.slashings) * 3, total_balance) overflows uint64

[JustinDrake]

big int should be used

One of the design goals is that big int should never be used. As such, I'm labelling this is a bug.

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

In the worst case validator.effective_balance * min(sum(state.slashings) * 3, total_balance) can indeed overflow.

expression like

Do you have other examples?

[terencechain]

This was actually discovered from the yaml test. I think for python, it just converts to long instead of overflow

Do you have other examples

Not right now, I'll go through the spec today to see if I can spot more offenders

[JustinDrake]

Here's a non-substantive fix which makes use of the fact that validator.effective_balance is a small multiple of GWEI_PER_ETH.

penalty = (validator.effective_balance // GWEI_PER_ETH) * min(sum(state.slashings) * 3, total_balance) // total_balance * GWEI_PER_ETH

This works because (validator.effective_balance // GWEI_PER_ETH) * min(sum(state.slashings) * 3, total_balance) is at most 32 * total_balance, itself at most 32 * 2**22 * 2**5 * 10**9, itself less than 2**62 🎉

[JustinDrake]

If we want to be even more conservative we could use the same trick on min(sum(state.slashings) * 3, total_balance) which is guaranteed to also be a multiple of GWEI_PER_ETH.

[djrtwo]

@JustinDrake This is substantive because validator.effective_balance is not guaranteed to be a multiple of GWEI_PER_ETH

[JustinDrake]

validator.effective_balance is not guaranteed to be a multiple of GWEI_PER_ETH

How?

[djrtwo]

hm, you're right. The tests failed due to this change due to a mismatch in previous calculations and current. digging in

[djrtwo]

If we want to be even more conservative we could use the same trick on min(sum(state.slashings) * 3, total_balance) which is guaranteed to also be a multiple of GWEI_PER_ETH

We can't actually do this due to a sum(state.slashings) * 3 // EFFECTIVE_BALANCE_INCREMENT zeroing out in many cases. That was the issue I found when performing the calculations that led me to my incorrect prior assumption.

Also note that the divisor should be EFFECTIVE_BALANCE_INCREMENT rather than GWEI_PER_ETH

[vbuterin]

penalty = (validator.effective_balance // GWEI_PER_ETH) * min(sum(state.slashings) * 3, total_balance) // total_balance * GWEI_PER_ETH

Seems reasonable to me.

[JustinDrake]

Fixed in #1286.