feat: restart aware doppelganger protection #6012
Merged
Conversation
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
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
commented
Oct 1, 2023
| /** | ||
| * 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)
nflaig
force-pushed
the
nflaig/zero-downtime-doppelganger
branch
from
e5b9887
to
79a89a2
Compare
nflaig
force-pushed
the
nflaig/zero-downtime-doppelganger
branch
from
79a89a2
to
d40f1cd
Compare
nflaig
force-pushed
the
nflaig/zero-downtime-doppelganger
branch
from
b6c3016
to
0fa1682
Compare
nflaig
commented
Oct 2, 2023
| 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.
It also assumes slashing protection was exported / imported within short time period, likely in an automated setup which aims to achieve no downtime when switching clients and therefore doppelganger protection would not be enabled anyways.
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
nflaig
commented
Oct 2, 2023
nflaig
changed the title
dapplion
reviewed
Oct 12, 2023
| @@ -149,7 +149,7 @@ export class KeymanagerApi implements Api { | |||
|
|
|||
| decryptKeystores.queue( | |||
| {keystoreStr, password}, | |||
| (secretKeyBytes: Uint8Array) => { | |||
| async (secretKeyBytes: Uint8Array) => { | |||
There was a problem hiding this comment.
decryptKeystores.queue does 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.
decryptKeystores.queue does no handle promise rejections.
That's not an issue due to how promise chaining works. In any case sync or async this will be handled by onError handler
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.
Why not decrypt all keystores first, accumulate the result of this callbacks to the queue
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.
| previousEpoch | ||
| ); | ||
|
|
||
| if (attestedInPreviousEpoch) { |
There was a problem hiding this comment.
consider committing this explanation into the code
|
🎉 This PR is included in v1.12.0 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
Closes #5856
Description
Skip doppelganger detection for validator if client restart is detected by checking if an attestation from previous epoch exists for this validator in the slashing protection database. This means there is now almost no downside to using doppelganger protection while still getting all security benefits. The only time doppelganger detection will run is if validator is imported for the first time or has not been active for more than one epoch due to a longer downtime / maintenance.