Description

While testing Deneb + tree-states, I found a bug in our current progressive balances cache implementation: we update the unslashed participating balances incorrectly for slashed validators. First we remove the balance with on_slashing:

Later, when processing effective balance changes, we update the total by the difference in the slashed validator's effective balance, which is incorrect, because the validator's balance has already been removed:

The result is that the total unslashed participating balance can be off by O(num_slashed_validators). This is unlikely to cause issues in practice, because:

• The progressive balances optimisation is disabled by default currently. If this bug were triggered, we'd get an error log and nothing more.
• If the user has opted into --progressive-balances fast, the worst-case is a temporary view split, which would require a lot of validators to be slashed. If the slashing is small, it's unlikely to cause a view split, because the attesting balances will only be off by a small amount.

Version

Lighthouse v4.5.0

Steps to resolve

Guard the on_effective_balance_change call by !validator.slashed(). On tree-states, I've forced the caller to pass the validator's is_slashed status to on_effective_balance_change:

We should make a similar change on unstable, with an accompanying regression test. The conditions necessary to trigger this are:

• Validator's balance gets lowered substantially as a result of the amount they are slashed (e.g. validator gets slashed 1 ETH and goes from balance 32 ETH to 31 ETH).
• Epoch transition occurs after the slashing which applies the balance reduction to their effective balance (e.g. eff. balance 32 -> 31).