More proposer shuffling cleanup

[michaelsproul]

Issue Addressed

Addressing more review comments from:

• Fix bugs in proposer calculation post-Fulu #8101

I've also tweaked a few more things that I think are minor bugs.

Proposed Changes

• Instrument ensure_state_can_determine_proposers_for_epoch
• Fix block_root usage in compute_proposer_duties_from_head. This was a regression introduced in 8101 😬 .
• Update the state_advance_timer to prime the next-epoch proposer cache post-Fulu.

Additional Info

• Add test for next epoch shuffling case in HTTP duties API. This test is proposer_duties_from_head_fulu and I have confirmed it fails when reverting the usage of proposer_shuffling_decision_root_at_epoch with the correct block root.
• Add regression test for bug in try_proposer_duties_from_cache

[michaelsproul]

We had regressed here (in 8101) when I removed this code:

lighthouse/beacon_node/http_api/src/proposer_duties.rs

Lines 114 to 126 in cfb1f73

	let dependent_root = match head_epoch.cmp(&request_epoch) {
	// head_epoch == request_epoch
	Ordering::Equal => head_decision_root,
	// head_epoch < request_epoch
	Ordering::Less => head_block_root,
	// head_epoch > request_epoch
	Ordering::Greater => {
	return Err(warp_utils::reject::custom_server_error(format!(
	"head epoch {} is later than request epoch {}",
	head_epoch, request_epoch
	)))
	}
	};

My intent was to replace the manual calculation of the dependent root with the unified proposer_shuffling_decision_root_at_epoch function, but I mistakenly kept using the proposer_shuffling_decision_root function which computes the decision root for the state's current epoch. At epoch boundaries, this codepath could be called with request_epoch == head_epoch + 1, in which case proposer_shuffling_decision_root would be wrong (it would be the decision block for the current epoch instead of the next).

Using proposer_shuffling_decision_root_at_epoch fixes this, and is generic over pre-Fulu vs post-Fulu. It uses the request epoch to determine the decision slot, and then uses either the passed in head_block_root or state's block roots to compute the correct root.

[michaelsproul]

The impact of this is that in v8.0.0-rc.0 we might return the wrong duties for an epoch, and put a nonsensical entry into the cache. This could lead to a block proposal failure in the worst case (the VC would make a request for a block when it isn't actually the proposer), which would be caught by the proposer index checks in produce_block (which are not buggy). If the same VC holds the key for the true proposer then it might miss the proposal for that true proposer.

I think this is low impact enough that it is OK not to delay v8.0.0-rc.0 over, although we should fix it soon!

[michaelsproul]

For this to return the wrong duties in practice requires that there be an entry for (current_epoch_decision_block_root, request_epoch) in the cache. This should not happen, unless there's another bug somewhere that would add a value for this pair to the cache. As far as I can tell, there isn't (need to check the state advance).

[michaelsproul]

The other way we could end up with an entry for that key is if there's a long skip on an alternate chain using that shuffling. This is also quite unlikely, but I guess could be triggered intentionally by an attacker to force some LH proposers to fail proposing temporarily.

[dapplion]

LGTM

[dapplion]

why not necessary anymore?

[michaelsproul]

Pawan added this to work around the fact that we weren't advancing into the Fulu fork epoch to compute its shuffling (we were wrongly assuming we had lookahead). Now that we've fixed the function to calculate the decision slot, this is no longer a concern.

In this PR we've also started using fork_at_epoch so that the shuffling calculation works at future fork epochs (i.e. Gloas). This could be a good test case as well actually.

Removing the advance means we can take advantage of the lookahead post-Fulu. We avoid doing up to 1 epoch of unnecessary state advance.