Progressive balances optimisation is incorrect with slashed validators

[michaelsproul]

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:

lighthouse/consensus/types/src/beacon_state/progressive_balances_cache.rs

Lines 67 to 74 in 441fc16

	/// When a validator is slashed, we reduce the `current_epoch_target_attesting_balance` by the
	/// validator's effective balance to exclude the validator weight.
	pub fn on_slashing(
	&mut self,
	is_previous_epoch_target_attester: bool,
	is_current_epoch_target_attester: bool,
	effective_balance: u64,
	) -> Result<(), BeaconStateError> {

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:

lighthouse/consensus/state_processing/src/per_epoch_processing/effective_balance_updates.rs

Lines 58 to 66 in 441fc16

	if old_effective_balance != new_effective_balance {
	let is_current_epoch_target_attester =
	participation_cache.is_current_epoch_timely_target_attester(index)?;
	progressive_balances_cache.on_effective_balance_change(
	is_current_epoch_target_attester,
	old_effective_balance,
	new_effective_balance,
	)?;
	}

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:

lighthouse/consensus/state_processing/src/per_epoch_processing/single_pass.rs

Lines 623 to 630 in b77de69

	// Update progressive balances cache for the *current* epoch, which will soon become the
	// previous epoch once the epoch transition completes.
	progressive_balances.on_effective_balance_change(
	validator.slashed(),
	validator_info.current_epoch_participation,
	old_effective_balance,
	new_effective_balance,
	)?;

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).

[michaelsproul]

@jimmygchen correctly identified that this isn't an issue on stable! We already check that the validator is not slashed before calling on_effective_balance_change, implicitly via the participation cache:

lighthouse/consensus/state_processing/src/per_epoch_processing/effective_balance_updates.rs

Lines 59 to 65 in 441fc16

	let is_current_epoch_target_attester =
	participation_cache.is_current_epoch_timely_target_attester(index)?;
	progressive_balances_cache.on_effective_balance_change(
	is_current_epoch_target_attester,
	old_effective_balance,
	new_effective_balance,
	)?;

The participation cache only tracks unslashed attesting indices:

lighthouse/consensus/state_processing/src/per_epoch_processing/altair/participation_cache.rs

Lines 80 to 97 in 441fc16

	/// Returns `true` if `val_index` is active, unslashed and has `flag_index` set.
	///
	/// ## Errors
	///
	/// May return an error if `flag_index` is out-of-bounds.
	fn has_flag(&self, val_index: usize, flag_index: usize) -> Result<bool, Error> {
	let participation_flags = self
	.unslashed_participating_indices
	.get(val_index)
	.ok_or(Error::InvalidValidatorIndex(val_index))?;
	if let Some(participation_flags) = participation_flags {
	participation_flags
	.has_flag(flag_index)
	.map_err(|_| Error::InvalidFlagIndex(flag_index))
	} else {
	Ok(false)
	}
	}

Things went awry on tree-states because I had changed this mechanism, to look only at the participation flags without checking the slashing status.