-
-
Notifications
You must be signed in to change notification settings - Fork 289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: only check doppelganger liveness for relevant epochs #5991
Conversation
currentEpoch: this.clock.currentEpoch, | ||
detectionEpochs: DEFAULT_REMAINING_DETECTION_EPOCHS, | ||
}); | ||
this.logger.info("Doppelganger protection enabled", {detectionEpochs: DEFAULT_REMAINING_DETECTION_EPOCHS}); |
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.
I added epoch to the log in #5987 but noticed we only want the epoch information when registering a validator. Better to only log doppelganger related configuration here.
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
@@ -103,7 +100,7 @@ export class DoppelgangerService { | |||
const indicesToCheckMap = new Map<ValidatorIndex, PubkeyHex>(); | |||
|
|||
for (const [pubkeyHex, state] of this.doppelgangerStateByPubkey.entries()) { | |||
if (state.remainingEpochs > 0) { | |||
if (state.remainingEpochs > 0 && state.nextEpochToCheck <= currentEpoch) { |
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.
Is it not redundant condition? Is notremainingEpochs
already related to currentEpoch
?
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.
There is a difference, if a validator is registered first time state.remainingEpochs
will always be > 0 (unless pre-genesis)
const remainingEpochs = currentEpoch <= 0 ? 0 : DEFAULT_REMAINING_DETECTION_EPOCHS; |
and state.nextEpochToCheck
will always be current epoch + 1.
const nextEpochToCheck = currentEpoch + 1; |
This means that the first time we check liveness for a validator it is for a epoch that is not relevant because it will be skipped anyways (see PR description).
The main goal of checking the epoch here is to skip earlier, this avoids unnecessary liveness requests to the beacon node.
26cc38e
to
4ad6603
Compare
🎉 This PR is included in v1.12.0 🎉 |
Motivation
Noticed that we run doppelganger liveness check for a validator even if next epoch to check is current epoch + 1.
The doppelganger check would be skipped either way but we can avoid the liveness request to the beacon node
lodestar/packages/validator/src/services/doppelgangerService.ts
Line 185 in f5e2c3a
lodestar/packages/validator/src/services/doppelgangerService.ts
Line 224 in f5e2c3a
Description
Only run doppelganger liveness check for validators with
nextEpochToCheck <= currentEpoch