feat: restart aware doppelganger protection #6012
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| import {fromHexString} from "@chainsafe/ssz"; | ||
| import {Epoch, ValidatorIndex} from "@lodestar/types"; | ||
| import {Api, ApiError, routes} from "@lodestar/api"; | ||
| import {Logger, prettyBytes, sleep} 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_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 */ | ||
| VerifiedSafe = "VerifiedSafe", | ||
| /** This pubkey is known to the doppelganger service but has not been verified safe */ | ||
| Unverified = "Unverified", | ||
| /** This pubkey is unknown to the doppelganger service */ | ||
| Unknown = "Unknown", | ||
| /** 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,42 @@ export class DoppelgangerService { | |
| this.logger.info("Doppelganger protection enabled", {detectionEpochs: DEFAULT_REMAINING_DETECTION_EPOCHS}); | ||
| } | ||
|
|
||
| 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. | ||
| 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) { | ||
| const previousEpoch = currentEpoch - 1; | ||
| const attestedInPreviousEpoch = await this.slashingProtection.hasAttestedInEpoch( | ||
| fromHexString(pubkeyHex), | ||
| previousEpoch | ||
| ); | ||
|
|
||
| if (attestedInPreviousEpoch) { | ||
|
There was a problem hiding this comment. There is an edge case when importing a validator via keymanager API, it is possible to also import the slashing protection interchange file. This will create an attestation record in the database and could cause doppelganger protection to be skipped if import happens within in the next epoch. I don't see this as a huge risk because exporting the slashing protection interchange from a running client is only possible while also deleting the key from that client, there is no risk of getting slashing due to doppelganger. Considering this, I think there is no risk in the currently used approach and it is safe to skip doppelganger in any scenario if an attestation from previous epoch exists in the slashing protection db. There was a problem hiding this comment. consider committing this explanation into the code There was a problem hiding this comment. I mostly added this here to get a second opinion on this if my reasoning is sound. But might be a good idea to add this to the issue description for better visibility instead of here. The issue is already referenced in the code lodestar/packages/validator/src/services/doppelgangerService.ts Lines 76 to 78 in eaf74cd Will refine the issue description a bit |
||
| remainingEpochs = REMAINING_EPOCHS_IF_SKIPPED; | ||
| this.logger.info( | ||
| "Doppelganger detection skipped for validator, previous epoch attestation exists in database", | ||
| { | ||
| pubkey: prettyBytes(pubkeyHex), | ||
| previousEpoch, | ||
| } | ||
| ); | ||
| } else { | ||
| this.logger.info("Registered validator for doppelganger", { | ||
| pubkey: prettyBytes(pubkeyHex), | ||
| remainingEpochs, | ||
| nextEpochToCheck, | ||
| }); | ||
| } | ||
| } else { | ||
| this.logger.info("Doppelganger detection skipped for validator, initialized before genesis", { | ||
| pubkey: prettyBytes(pubkeyHex), | ||
| currentEpoch, | ||
| }); | ||
| } | ||
|
|
||
| this.doppelgangerStateByPubkey.set(pubkeyHex, { | ||
|
|
@@ -180,7 +210,7 @@ export class DoppelgangerService { | |
| } | ||
|
|
||
| if (state.nextEpochToCheck <= epoch) { | ||
| // Doppelganger detected | ||
| violators.push(response.index); | ||
| } | ||
| } | ||
|
|
@@ -189,7 +219,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 = REMAINING_EPOCHS_IF_DOPPELGANGER; | ||
| } | ||
|
|
||
| this.logger.error( | ||
|
|
@@ -225,9 +255,9 @@ export class DoppelgangerService { | |
|
|
||
| const {remainingEpochs, nextEpochToCheck} = state; | ||
| if (remainingEpochs <= 0) { | ||
| this.logger.info("Doppelganger detection complete", {index: response.index, epoch: currentEpoch}); | ||
| } else { | ||
| this.logger.info("Found no doppelganger", {index: response.index, remainingEpochs, nextEpochToCheck}); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -253,7 +283,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_DOPPELGANGER) { | ||
| return DoppelgangerStatus.DoppelgangerDetected; | ||
| } else { | ||
| return DoppelgangerStatus.Unverified; | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(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 { | |
| }, | ||
| }; | ||
|
|
||
| if (metrics) { | ||
| metrics.signers.addCollect(() => metrics.signers.set(this.validators.size)); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Create a validator store with initial signers | ||
| */ | ||
| static async init( | ||
|
There was a problem hiding this comment. Validator store initialization is now async, see #5856 (comment) |
||
| 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; | ||
| } | ||
|
|
||
| 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 file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decryptKeystores.queuedoes no handle promise rejections. Why not decrypt all keystores first, accumulate the result of this callbacks to the queue, then on the main function body run the code below that can throw / reject?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not an issue due to how promise chaining works. In any case sync or async this will be handled by
onErrorhandlerlodestar/packages/cli/src/cmds/validator/keymanager/decryptKeystores/threadPool.ts
Line 48 in 1aa6561
I was more concerned about the success case, i.e. does
await decryptKeystores.completed();properly wait for all promises to be resolved but this is the case as well. I tested both those cases manually, also existing tests should cover that.Don't we lose a lot of concurrency by doing that? Decrypt keystores is CPU heavy while write keystores and add signers are pretty lightweight I/O operations, I think it is good that we run that in parallel.
Also if we assume an import of 100+ keys it potentially takes a few minutes, best to add signers to validator store asap.