Doppelganger Protection Changes

[paulhauner]

Overview

Heyo 👋 Whilst reviewing sigp#2230 I wanted to make a few changes and I thought it would be easier for everyone to do that myself than to suggest in comments. However, once I started I got carried away and ended up with a rather large diff. That being said, I think the changes I've added here are a net benefit.

I've made this PR against your realbigsean/lighthouse fork. My intention is that you could review this PR (comment and changes) and then merge it into your PR. Then, we can finish off in sigp#2230 and merge that.

I've structured the comment here in two sections:

• "Potential Issues...": these are things in your original PR which I think might need to change. I've detailed them so that you can correct me if I'm wrong or gain feedback if I'm right. You don't need to address them, I've already got to them in my changes 🙂
• "Changes Introduced...": a summary of the changes I made in this PR. It should help with your review and to see if I've made mistakes.

Potential Issues in Existing Implementation

Doppelganger check happens at the start of the slot

In this update loop we're triggering doppelganger protection at the very start of the slot:

lighthouse/validator_client/src/doppelganger_service.rs

Lines 47 to 62 in e39feee

	async move {
	loop {
	if let Some(duration) = doppelganger_service.slot_clock.duration_to_next_slot()
	{
	sleep(duration).await;
	} else {
	// Just sleep for one slot if we are unable to read the system clock, this gives
	// us an opportunity for the clock to eventually come good.
	sleep(doppelganger_service.slot_clock.slot_duration()).await;
	continue;
	}
	if let Err(e) = doppelganger_service.detect_doppelgangers().await {
	error!(log,"Error during doppelganger detection"; "error" => ?e);
	}
	}

Then, we run this code:

lighthouse/validator_client/src/doppelganger_service.rs

Lines 151 to 157 in e39feee

	// If we are in the first slot of epoch N, consider checks in epoch N-1 completed.
	if slot == epoch.start_slot(E::slots_per_epoch()) {
	self.validator_store
	.initialized_validators()
	.write()
	.complete_doppelganger_detection_in_epoch(epoch.saturating_sub(Epoch::new(1)));
	}

So, this means that in the first instant of epoch N, we consider N - 1 to be safe. However, attestations from the last slot of epoch N - 1 are only first allowed to appear in blocks in epoch N. Since we do the doppelganger check so early in the first slot of epoch N we don't allow any time for a block to arrive with N - 1 attestations.

break instead of continue

Here there's a break which I think should be a continue:

lighthouse/validator_client/src/doppelganger_service.rs

Line 90 in e39feee

break;

The break would cancel all epochs instead of just skipping empty ones.

Enabling doppelganger for all validators if one BN call fails:

lighthouse/validator_client/src/doppelganger_service.rs

Lines 143 to 146 in e39feee

	self.validator_store
	.initialized_validators()
	.write()
	.update_all_initialization_epochs(current_epoch, genesis_epoch);

This seems safe, but perhaps overly safe. It might be the case that we've had a few validators running for a while and then added one more validator via the API. If that new validator triggers a failure from the BN then we'd enable doppleganger for all validators.

Completing detection even if call to BN fails

Regardless of if the liveness_response is Ok or Err we still consider the doppelganger check a success:

lighthouse/validator_client/src/doppelganger_service.rs

Lines 151 to 157 in e39feee

	// If we are in the first slot of epoch N, consider checks in epoch N-1 completed.
	if slot == epoch.start_slot(E::slots_per_epoch()) {
	self.validator_store
	.initialized_validators()
	.write()
	.complete_doppelganger_detection_in_epoch(epoch.saturating_sub(Epoch::new(1)));
	}

I didn't follow this all the way through to ensure it's an issue, but it seems suspicious.

Changes Introduced in this PR

Firstly, I merged in unstable which has caused a lot of noise in this diff. You might want to merge it into your branch to quieten down the noise.

Changes include:

• I've also added doppelganger checks to the signing functions as a second-line-of-defense against accidental signing.
• I lifted doppelganger logic out of InitializedValidators and collected it in ValidatorStore. This meant I could remove current_epoch, genesis_epoch from all the initialization calls which reduced the scope of the change.
• I modified the Doppelganger service to make it a bit easier to unit test. I also added a bunch of unit tests to it.
  • This resulted in a decent refactor. Sorry about that, I had a large break during the middle of this project and when I came back to it I needed to dig into it a bit more to ensure I didn't miss anything.
• We now run the doppelganger check when we're 75% through the slot to give us a chance to see the blocks/attestations in that slot.
• I changed it so that we only consider epoch N - 1 to be safe once we're in the last slot of epoch N. The reasoning here is that once we're 75% through the last slot of epoch N then we should have seen all blocks that could include attestations from epoch N - 1. This means we miss a few more epochs due to doppelganger, but it should be safer. Presently, if you start in epoch 1 then the first epoch you'll be able to participate in will be epoch 5. We could reduce DEFAULT_REMAINING_DETECTION_EPOCHS by 1 to make this epochs 1-4 if we wanted.

Additional Info

There's still a few open questions/issues remaining. Perhaps we just merge this PR and then copy them over to sigp#2230 and deal with them there.

• The doppelganger tests you wrote seem to be failing. I had to deal with some merge conflicts, I might have broken something sorry.
• I'm not sure we have tests that demonstrate the "everything went well" flow of starting post-genesis, being blocked by doppelganger, not seeing any doppels and then starting to sign. I think it would be cool to have tests like this, the idea of validators failing to start is the sort of thing that could keep us up at night.
• I get the feeling we should disable this by default initially. This if for three reasons: (1) we've seen some pushback on [Merged by Bors] - Doppelganger detection sigp/lighthouse#2230 comments, (2) Infura isn't going to support this until we can get the liveness endpoint into Teku and (3) it might be nice to get a bit of real-world testing before we push it on everyone by default. I'm quite scared of this change cause it adds a point of failure in front of performing duties.
• Should we reduce DEFAULT_REMAINING_DETECTION_EPOCHS to 1?