Faster Doppelganger Check

[nisdas]

What type of PR is this?

Feature Improvement

What does this PR do? Why is it needed?

In the event all the keys provided to the rpc method have been recently observed, we choose to exit
the method early. The reason this is done is because performing state regeneration is expensive
and doing it for 2 states can take time. This allows validators to perform quicker restarts which is
required when updating the validator client software.

Additionally this PR also modifies a test case to check for this particular situation.

Which issues(s) does this PR fix?

N.A

Other notes for review

[potuz]

Re reading the PR, the test is not for the condition I read originally.

I think this check misses the case of a validator that has is actually recent like this is it's first epoch, second or third. Currently we can check for the second case since we can check if the balance at the start of the current epoch is larger than the balance at the previous epoch. So a validator with v.Epoch + 2 = currentEpoch is covered. In addition to this, when we fix the doppelganger for Altair, this method will also have to go, since in Altair we can check for flags in the current epoch state. So I feel that including this would be detrimental only covering a few cases of validators deposited between the next release and the one where we fix for Altair

[potuz]

I suggest

if v.Epoch+2 < headEpoch {
    validatorsAreRecent = false
    break
 }
 resp.Responses....

[nisdas]

This is much cleaner

[potuz]

Suggested change

	func (vs *Server) checkValidatorsAreRecent(headEpoch types.Epoch, req *ethpb.DoppelGangerRequest) (bool, *ethpb.DoppelGangerResponse) {
	func checkValidatorsAreRecent(headEpoch types.Epoch, req *ethpb.DoppelGangerRequest) (bool, *ethpb.DoppelGangerResponse) {

[potuz]

Optionally we may want to leave the number of states we check as a configurable parameter. These constants v.Epoch + 2 >= headEpoch should probably be something like v.Epoch + doppelgangerCheckedStates >= headEpoch. Because when Altair comes, I claim that we get the same level of security with one less state. Or perhaps highly complicated setups may want to do checks for longer.

[nisdas]

@potuz We can potentially add that in a new PR. But given that we would have to change the schema of our request type for it, we would need to see how that would work out. Some users don't always update the beacon node and validator in lockstep.

[potuz]

@potuz We can potentially add that in a new PR. But given that we would have to change the schema of our request type for it, we would need to see how that would work out. Some users don't always update the beacon node and validator in lockstep.

I meant to have a constant in our configuration, no need to have it configurable by API now.

[potuz]

Do we want to to also set the response to nil here? so that we make sure that we are not using it by mistake?