feat: restart aware doppelganger protection (#6012)

* Skip doppelganger detection if validator attested in previous epoch * Async initialization of ValidatorStore and Validator class * Add attested in prev epoch doppelganger unit test * Fix typos in doppelganger * Update doppelganger service register validator logs * Use jsdoc to document doppelganger statuses * Use prettyBytes to print out pubkeys * Add comment about doppelganger and signer registration order * Print out validator pubkey first * Update validator client jsdoc * Revise doppelganger registration logs * Truncate and format bytes as 0x123456789abc * Add reference to github issue * Revise logs final * Update tests to ensure correct epoch is provided
ChainSafe · Oct 17, 2023 · 3a6702e · 3a6702e
1 parent a05e1b3
commit 3a6702e
Show file tree

Hide file tree

Showing 16 changed files with 332 additions and 125 deletions.
diff --git a/packages/cli/src/cmds/validator/keymanager/impl.ts b/packages/cli/src/cmds/validator/keymanager/impl.ts
@@ -149,7 +149,7 @@ export class KeymanagerApi implements Api {
 
         decryptKeystores.queue(
           {keystoreStr, password},
-          (secretKeyBytes: Uint8Array) => {
+          async (secretKeyBytes: Uint8Array) => {
             const secretKey = bls.SecretKey.fromBytes(secretKeyBytes);
 
             // Persist the key to disk for restarts, before adding to in-memory store
@@ -165,7 +165,7 @@ export class KeymanagerApi implements Api {
             });
 
             // Add to in-memory store to start validating immediately
-            this.validator.validatorStore.addSigner({type: SignerType.Local, secretKey});
+            await this.validator.validatorStore.addSigner({type: SignerType.Local, secretKey});
 
             statuses[i] = {status: ImportStatus.imported};
           },
@@ -292,7 +292,7 @@ export class KeymanagerApi implements Api {
   async importRemoteKeys(
     remoteSigners: Pick<SignerDefinition, "pubkey" | "url">[]
   ): ReturnType<Api["importRemoteKeys"]> {
-    const results = remoteSigners.map(({pubkey, url}): ResponseStatus<ImportRemoteKeyStatus> => {
+    const importPromises = remoteSigners.map(async ({pubkey, url}): Promise<ResponseStatus<ImportRemoteKeyStatus>> => {
       try {
         if (!isValidatePubkeyHex(pubkey)) {
           throw Error(`Invalid pubkey ${pubkey}`);
@@ -308,7 +308,7 @@ export class KeymanagerApi implements Api {
 
         // Else try to add it
 
-        this.validator.validatorStore.addSigner({type: SignerType.Remote, pubkey, url});
+        await this.validator.validatorStore.addSigner({type: SignerType.Remote, pubkey, url});
 
         this.persistedKeysBackend.writeRemoteKey({
           pubkey,
@@ -325,7 +325,7 @@ export class KeymanagerApi implements Api {
     });
 
     return {
-      data: results,
+      data: await Promise.all(importPromises),
     };
   }
 

diff --git a/packages/utils/src/format.ts b/packages/utils/src/format.ts
@@ -17,3 +17,13 @@ export function prettyBytesShort(root: Uint8Array | string): string {
   const str = typeof root === "string" ? root : toHexString(root);
   return `${str.slice(0, 6)}…`;
 }
+
+/**
+ * Truncate and format bytes as `0x123456789abc`
+ * 6 bytes is sufficient to avoid collisions and it allows to easily look up
+ * values on explorers like beaconcha.in while improving readability of logs
+ */
+export function truncBytes(root: Uint8Array | string): string {
+  const str = typeof root === "string" ? root : toHexString(root);
+  return str.slice(0, 14);
+}
diff --git a/packages/validator/src/services/doppelgangerService.ts b/packages/validator/src/services/doppelgangerService.ts
@@ -1,7 +1,9 @@
+import {fromHexString} from "@chainsafe/ssz";
 import {Epoch, ValidatorIndex} from "@lodestar/types";
 import {Api, ApiError, routes} from "@lodestar/api";
-import {Logger, sleep} from "@lodestar/utils";
+import {Logger, sleep, truncBytes} from "@lodestar/utils";
 import {computeStartSlotAtEpoch} from "@lodestar/state-transition";
+import {ISlashingProtection} from "../slashingProtection/index.js";
 import {ProcessShutdownCallback, PubkeyHex} from "../types.js";
 import {IClock} from "../util/index.js";
 import {Metrics} from "../metrics.js";
@@ -10,7 +12,8 @@ import {IndicesService} from "./indices.js";
 // The number of epochs that must be checked before we assume that there are
 // no other duplicate validators on the network
 const DEFAULT_REMAINING_DETECTION_EPOCHS = 1;
-const REMAINING_EPOCHS_IF_DOPPLEGANGER = Infinity;
+const REMAINING_EPOCHS_IF_DOPPELGANGER = Infinity;
+const REMAINING_EPOCHS_IF_SKIPPED = 0;
 
 /** Liveness responses for a given epoch */
 type EpochLivenessData = {
@@ -24,13 +27,13 @@ export type DoppelgangerState = {
 };
 
 export enum DoppelgangerStatus {
-  // This pubkey is known to the doppelganger service and has been verified safe
+  /** This pubkey is known to the doppelganger service and has been verified safe */
   VerifiedSafe = "VerifiedSafe",
-  // This pubkey is known to the doppelganger service but has not been verified safe
+  /** This pubkey is known to the doppelganger service but has not been verified safe */
   Unverified = "Unverified",
-  // This pubkey is unknown to the doppelganger service
+  /** This pubkey is unknown to the doppelganger service */
   Unknown = "Unknown",
-  // This pubkey has been detected to be active on the network
+  /** This pubkey has been detected to be active on the network */
   DoppelgangerDetected = "DoppelgangerDetected",
 }
 
@@ -42,6 +45,7 @@ export class DoppelgangerService {
     private readonly clock: IClock,
     private readonly api: Api,
     private readonly indicesService: IndicesService,
+    private readonly slashingProtection: ISlashingProtection,
     private readonly processShutdownCallback: ProcessShutdownCallback,
     private readonly metrics: Metrics | null
   ) {
@@ -54,16 +58,41 @@ export class DoppelgangerService {
     this.logger.info("Doppelganger protection enabled", {detectionEpochs: DEFAULT_REMAINING_DETECTION_EPOCHS});
   }
 
-  registerValidator(pubkeyHex: PubkeyHex): void {
+  async registerValidator(pubkeyHex: PubkeyHex): Promise<void> {
     const {currentEpoch} = this.clock;
     // Disable doppelganger protection when the validator was initialized before genesis.
     // There's no activity before genesis, so doppelganger is pointless.
-    const remainingEpochs = currentEpoch <= 0 ? 0 : DEFAULT_REMAINING_DETECTION_EPOCHS;
+    let remainingEpochs = currentEpoch <= 0 ? REMAINING_EPOCHS_IF_SKIPPED : DEFAULT_REMAINING_DETECTION_EPOCHS;
     const nextEpochToCheck = currentEpoch + 1;
 
     // Log here to alert that validation won't be active until remainingEpochs == 0
     if (remainingEpochs > 0) {
-      this.logger.info("Registered validator for doppelganger", {remainingEpochs, nextEpochToCheck, pubkeyHex});
+      const previousEpoch = currentEpoch - 1;
+      const attestedInPreviousEpoch = await this.slashingProtection.hasAttestedInEpoch(
+        fromHexString(pubkeyHex),
+        previousEpoch
+      );
+
+      if (attestedInPreviousEpoch) {
+        // It is safe to skip doppelganger detection
+        // https://github.com/ChainSafe/lodestar/issues/5856
+        remainingEpochs = REMAINING_EPOCHS_IF_SKIPPED;
+        this.logger.info("Doppelganger detection skipped for validator because restart was detected", {
+          pubkey: truncBytes(pubkeyHex),
+          previousEpoch,
+        });
+      } else {
+        this.logger.info("Registered validator for doppelganger detection", {
+          pubkey: truncBytes(pubkeyHex),
+          remainingEpochs,
+          nextEpochToCheck,
+        });
+      }
+    } else {
+      this.logger.info("Doppelganger detection skipped for validator initialized before genesis", {
+        pubkey: truncBytes(pubkeyHex),
+        currentEpoch,
+      });
     }
 
     this.doppelgangerStateByPubkey.set(pubkeyHex, {
@@ -180,7 +209,7 @@ export class DoppelgangerService {
         }
 
         if (state.nextEpochToCheck <= epoch) {
-          // Doppleganger detected
+          // Doppelganger detected
           violators.push(response.index);
         }
       }
@@ -189,7 +218,7 @@ export class DoppelgangerService {
     if (violators.length > 0) {
       // If a single doppelganger is detected, enable doppelganger checks on all validators forever
       for (const state of this.doppelgangerStateByPubkey.values()) {
-        state.remainingEpochs = Infinity;
+        state.remainingEpochs = REMAINING_EPOCHS_IF_DOPPELGANGER;
       }
 
       this.logger.error(
@@ -225,9 +254,9 @@ export class DoppelgangerService {
 
           const {remainingEpochs, nextEpochToCheck} = state;
           if (remainingEpochs <= 0) {
-            this.logger.info("Doppelganger detection complete", {index: response.index});
+            this.logger.info("Doppelganger detection complete", {index: response.index, epoch: currentEpoch});
           } else {
-            this.logger.info("Found no doppelganger", {remainingEpochs, nextEpochToCheck, index: response.index});
+            this.logger.info("Found no doppelganger", {index: response.index, remainingEpochs, nextEpochToCheck});
           }
         }
       }
@@ -253,7 +282,7 @@ function getStatus(state: DoppelgangerState | undefined): DoppelgangerStatus {
     return DoppelgangerStatus.Unknown;
   } else if (state.remainingEpochs <= 0) {
     return DoppelgangerStatus.VerifiedSafe;
-  } else if (state.remainingEpochs === REMAINING_EPOCHS_IF_DOPPLEGANGER) {
+  } else if (state.remainingEpochs === REMAINING_EPOCHS_IF_DOPPELGANGER) {
     return DoppelgangerStatus.DoppelgangerDetected;
   } else {
     return DoppelgangerStatus.Unverified;

diff --git a/packages/validator/src/services/validatorStore.ts b/packages/validator/src/services/validatorStore.ts
@@ -97,6 +97,14 @@ export type ValidatorProposerConfig = {
   defaultConfig: ProposerConfig;
 };
 
+export type ValidatorStoreModules = {
+  config: BeaconConfig;
+  slashingProtection: ISlashingProtection;
+  indicesService: IndicesService;
+  doppelgangerService: DoppelgangerService | null;
+  metrics: Metrics | null;
+};
+
 /**
  * This cache stores SignedValidatorRegistrationV1 data for a validator so that
  * we do not create and send new registration objects to avoid DOSing the builder
@@ -130,21 +138,25 @@ export const defaultOptions = {
  * Service that sets up and handles validator attester duties.
  */
 export class ValidatorStore {
+  private readonly config: BeaconConfig;
+  private readonly slashingProtection: ISlashingProtection;
+  private readonly indicesService: IndicesService;
+  private readonly doppelgangerService: DoppelgangerService | null;
+  private readonly metrics: Metrics | null;
+
   private readonly validators = new Map<PubkeyHex, ValidatorData>();
   /** Initially true because there are no validators */
   private pubkeysToDiscover: PubkeyHex[] = [];
   private readonly defaultProposerConfig: DefaultProposerConfig;
 
-  constructor(
-    private readonly config: BeaconConfig,
-    private readonly slashingProtection: ISlashingProtection,
-    private readonly indicesService: IndicesService,
-    private readonly doppelgangerService: DoppelgangerService | null,
-    private readonly metrics: Metrics | null,
-    initialSigners: Signer[],
-    valProposerConfig: ValidatorProposerConfig = {defaultConfig: {}, proposerConfig: {}},
-    private readonly genesisValidatorRoot: Root
-  ) {
+  constructor(modules: ValidatorStoreModules, valProposerConfig: ValidatorProposerConfig) {
+    const {config, slashingProtection, indicesService, doppelgangerService, metrics} = modules;
+    this.config = config;
+    this.slashingProtection = slashingProtection;
+    this.indicesService = indicesService;
+    this.doppelgangerService = doppelgangerService;
+    this.metrics = metrics;
+
     const defaultConfig = valProposerConfig.defaultConfig;
     this.defaultProposerConfig = {
       graffiti: defaultConfig.graffiti ?? "",
@@ -157,15 +169,26 @@ export class ValidatorStore {
       },
     };
 
-    for (const signer of initialSigners) {
-      this.addSigner(signer, valProposerConfig);
-    }
-
     if (metrics) {
       metrics.signers.addCollect(() => metrics.signers.set(this.validators.size));
     }
   }
 
+  /**
+   * Create a validator store with initial signers
+   */
+  static async init(
+    modules: ValidatorStoreModules,
+    initialSigners: Signer[],
+    valProposerConfig: ValidatorProposerConfig = {defaultConfig: {}, proposerConfig: {}}
+  ): Promise<ValidatorStore> {
+    const validatorStore = new ValidatorStore(modules, valProposerConfig);
+
+    await Promise.all(initialSigners.map((signer) => validatorStore.addSigner(signer, valProposerConfig)));
+
+    return validatorStore;
+  }
+
   /** Return all known indices from the validatorStore pubkeys */
   getAllLocalIndices(): ValidatorIndex[] {
     return this.indicesService.getAllLocalIndices();
@@ -282,18 +305,19 @@ export class ValidatorStore {
     return proposerConfig;
   }
 
-  addSigner(signer: Signer, valProposerConfig?: ValidatorProposerConfig): void {
+  async addSigner(signer: Signer, valProposerConfig?: ValidatorProposerConfig): Promise<void> {
     const pubkey = getSignerPubkeyHex(signer);
     const proposerConfig = (valProposerConfig?.proposerConfig ?? {})[pubkey];
 
     if (!this.validators.has(pubkey)) {
+      // Doppelganger registration must be done before adding validator to signers
+      await this.doppelgangerService?.registerValidator(pubkey);
+
       this.pubkeysToDiscover.push(pubkey);
       this.validators.set(pubkey, {
         signer,
         ...proposerConfig,
       });
-
-      this.doppelgangerService?.registerValidator(pubkey);
     }
   }
 

diff --git a/packages/validator/src/slashingProtection/attestation/index.ts b/packages/validator/src/slashingProtection/attestation/index.ts
@@ -1,4 +1,4 @@
-import {BLSPubkey} from "@lodestar/types";
+import {BLSPubkey, Epoch} from "@lodestar/types";
 import {isEqualNonZeroRoot, minEpoch} from "../utils.js";
 import {MinMaxSurround, SurroundAttestationError, SurroundAttestationErrorCode} from "../minMaxSurround/index.js";
 import {SlashingProtectionAttestation} from "../types.js";
@@ -133,6 +133,13 @@ export class SlashingProtectionAttestationService {
     await this.minMaxSurround.insertAttestation(pubKey, attestation);
   }
 
+  /**
+   * Retrieve an attestation from the slashing protection database for a given `pubkey` and `epoch`
+   */
+  async getAttestationForEpoch(pubkey: BLSPubkey, epoch: Epoch): Promise<SlashingProtectionAttestation | null> {
+    return this.attestationByTarget.get(pubkey, epoch);
+  }
+
   /**
    * Interchange import / export functionality
    */

diff --git a/packages/validator/src/slashingProtection/index.ts b/packages/validator/src/slashingProtection/index.ts
@@ -1,5 +1,5 @@
 import {toHexString} from "@chainsafe/ssz";
-import {BLSPubkey, Root} from "@lodestar/types";
+import {BLSPubkey, Epoch, Root} from "@lodestar/types";
 import {Logger} from "@lodestar/utils";
 import {LodestarValidatorDatabaseController} from "../types.js";
 import {uniqueVectorArr} from "../slashingProtection/utils.js";
@@ -56,6 +56,10 @@ export class SlashingProtection implements ISlashingProtection {
     await this.attestationService.checkAndInsertAttestation(pubKey, attestation);
   }
 
+  async hasAttestedInEpoch(pubKey: BLSPubkey, epoch: Epoch): Promise<boolean> {
+    return (await this.attestationService.getAttestationForEpoch(pubKey, epoch)) !== null;
+  }
+
   async importInterchange(interchange: Interchange, genesisValidatorsRoot: Root, logger?: Logger): Promise<void> {
     const {data} = parseInterchange(interchange, genesisValidatorsRoot);
     for (const validator of data) {

diff --git a/packages/validator/src/slashingProtection/interface.ts b/packages/validator/src/slashingProtection/interface.ts
@@ -1,4 +1,4 @@
-import {BLSPubkey, Root} from "@lodestar/types";
+import {BLSPubkey, Epoch, Root} from "@lodestar/types";
 import {Logger} from "@lodestar/utils";
 import {Interchange, InterchangeFormatVersion} from "./interchange/types.js";
 import {SlashingProtectionBlock, SlashingProtectionAttestation} from "./types.js";
@@ -13,6 +13,11 @@ export interface ISlashingProtection {
    */
   checkAndInsertAttestation(pubKey: BLSPubkey, attestation: SlashingProtectionAttestation): Promise<void>;
 
+  /**
+   * Check whether a validator as identified by `pubKey` has attested in the specified `epoch`
+   */
+  hasAttestedInEpoch(pubKey: BLSPubkey, epoch: Epoch): Promise<boolean>;
+
   importInterchange(interchange: Interchange, genesisValidatorsRoot: Uint8Array | Root, logger?: Logger): Promise<void>;
   exportInterchange(
     genesisValidatorsRoot: Uint8Array | Root,