Midpoint slashing amount calculation truncates < 1 ETH penalties

pietjepuk2 commented 3 years ago

What happened

The first validator to be slashed on mainnet (validator 20075) reached the point where it was supposed to be getting an additional penalty at epoch 4304. Looking at the decrease of balance over time however, there was no significant deviation visible.

Looking into a bit further, it seems that the current calculation for the midpoint slashing penalty only results in integer increments of ETH (or Gwei multiples of 1E9). So the penalty for validator 20075 turned out to be 0.

I think this was (unintentionally) caused by #1286 . Although there have been a few restructurings in that piece of code since.

Example

To illustrate with the example on mainnet for validator 20075:

There were about 15 validators slashed at the time of epoch 4304 so let's say adjusted_total_slashing_balance = 15 * 32E9 (maybe some had 31 ETH effective balance, but that does not matter for this example).
About 1.2M ETH staked, so total_balance = 1.2E6 * 1E9

Before #1286 the calculation was (paraphrased, because of other changes since):

penalty = validator.effective_balance * adjusted_total_slashing_balance // total_balance
# penalty = 12400000.0

Currently the calculation is:

increment = EFFECTIVE_BALANCE_INCREMENT  # Factored out from penalty numerator to avoid uint64 overflow
penalty_numerator = validator.effective_balance // increment * adjusted_total_slashing_balance
penalty = penalty_numerator // total_balance * increment
# penalty = 0

Proposed fix

I would suggest to instead do penalty_numerator // (total_balance // increment) (i.e. change the order of operations)

increment = EFFECTIVE_BALANCE_INCREMENT  # Factored out from penalty numerator to avoid uint64 overflow
penalty_numerator = validator.effective_balance // increment * adjusted_total_slashing_balance
penalty = penalty_numerator // (total_balance // increment)
# penalty = 12400000.0

or to make order of operations a bit clearer without the use of parentheses (e.g. for implementations using "safe divide" type of constructs):

increment = EFFECTIVE_BALANCE_INCREMENT  # Factored out from penalty numerator to avoid uint64 overflow
penalty_numerator = validator.effective_balance // increment * adjusted_total_slashing_balance
penalty_denominator = total_balance // increment
penalty = penalty_numerator // penalty_denominator
# penalty = 12400000.0

Side note: Penalties were supposed to be lower in the first few months anyway, so I guess this is an unexpected benefit for those slashed.

ralexstokes commented 3 years ago

One thing to consider here is that this mechanism implements the "collective slashing penalty" function with the higher-level goal to discourage large-scale slashable behavior occurring across (many) validators contemporaneously.

If we mainly care about discourage large-scale attacks here, then it is ok to lose sub-ETH resolution in the penalty amount (because attacks of sufficient scale will not have sub-ETH penalties so the issue does not apply). We still have individual slashing penalties that are strong disincentives for a single validator to attack in the first place so we aren't really losing any protocol security.

Moreover, we make it easier to achieve consensus across clients via reduction of possibility of overflow (a la #1286). So while we do lose some fidelity in penalties, the trade-off to make it easier to reach consensus is worthwhile and we have penalties in other parts of the protocol so we aren't leaving an attack vector wide open.

pietjepuk2 commented 3 years ago

I agree that for large scale slashing penalties it would not matter much. The reason I opened this issue is that I could not find any reasoning on making this midpoint penalty integer-only (in ETH), and that upon closer inspection it looks like it's the unintended result of a PR fixing the possible overflow.

As far as consensus is concerned, I don't think that changing the order of operations has any impact there (the risk of overflow that is). Any change to the spec does inherently come with consensus risks of course.

Either way, if this behavior is the way we want it to be, then so be it (although the process_slashing function could do with a comment stating as such). That said, I personally dislike discontinuous behavior where it can be avoided. With that I mean that right now ~1200 slashed validators (in a 36 day window) would not result in any penalization, but 1201 slashed validators would "all of a sudden" result in each receiving a 1 ETH penalty.

Neurone commented 3 years ago

I agree with @pietjepuk2 and because we already use other lower then 1 ETH penalties (i.e. the epoch leaking) it seems to me a good choice to reduce that 1 ETH increment, if not now at least in the next phase.

This issue was opened more than one year ago here #1322 just by @ralexstokes and also @vbuterin had thought about a reduction to 0.01 ETH.

metatron1973 commented 3 years ago

Ok cool dual shields honest OATH2 key to success of ETH2 is honesty good post thanks .

ethereum / consensus-specs