Fix bugs in rebasing of states prior to finalization

[michaelsproul]

Issue Addressed

Attempt to fix this error reported by beaconcha.in on their Hoodi archive nodes:

{"code":500,"message":"UNHANDLED_ERROR: DBError(CacheBuildError(BeaconState(MilhouseError(OutOfBoundsIterFrom { index: 1199549, len: 1060000 }))))","stacktraces":[]}

Proposed Changes

There are only a handful of places where we call iter_from.

This one is safe by construction (the check immediately prior ensures self.pubkeys.len() is not out of bounds):

lighthouse/beacon_node/beacon_chain/src/validator_pubkey_cache.rs

Lines 84 to 90 in cfb1f73

	if state.validators().len() > self.pubkeys.len() {
	self.import(
	state
	.validators()
	.iter_from(self.pubkeys.len())?
	.map(|v| v.pubkey),
	)

This one should also be safe, and the indexes used here would not be as large as the ones in the reported error:

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

Lines 365 to 368 in cfb1f73

	state
	.pending_deposits()?
	.iter_from(ctxt.next_deposit_index)?
	.cloned(),

Which leaves one remaining usage which must be the culprit:

lighthouse/consensus/types/src/beacon_state.rs

Lines 2109 to 2113 in cfb1f73

	pub fn update_pubkey_cache(&mut self) -> Result<(), Error> {
	let mut pubkey_cache = mem::take(self.pubkey_cache_mut());
	let start_index = pubkey_cache.len();
	for (i, validator) in self.validators().iter_from(start_index)?.enumerate() {

This indexing relies on the invariant that self.pubkey_cache().len() <= self.validators.len(). We mostly maintain that invariant, except for in rebase_caches_on (fixed in this PR).

The other bug, is that we were calling rebase_on_finalized for all "hot" states, which post-v7.1.0 includes states prior to the split which are required by the hdiff grid. This is how we end up calling something like genesis_state.rebase_on(&split_state), which then corrupts the pubkey cache of the genesis state using the newer pubkey cache from the split state.

[jimmygchen]

Good catch! the fix makes sense to me.

[michaelsproul]

Will merge once we get a +1 from beaconcha.in, but I'm fairly sure this is good.

[dapplion]

Even if the base cache is not superior is it worth it to use it to save memory? i.e. if it's missing just 1 pubkey it's okay to also re-use it?

[michaelsproul]

oh yeah maybe

[michaelsproul]

Beaconcha.in have tested this PR and confirmed it fixes the issue, so gonna merge as-is. We can come back for this optimisation later if we like (it is orthogonal to fixing the bug).