fix(slashing-protection): limit min-max surround epoch lookback to 4096 #5454
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
nflaig
commented
May 2, 2023
| * Number of epochs in the past to check for surrounding attestations. | ||
| * | ||
| * This value can be limited to a reasonable high amount as Lodestar does not solely rely on this strategy but also | ||
| * implements the minimal strategy which has been formally proven to be safe (https://github.com/michaelsproul/slashing-proofs). |
There was a problem hiding this comment.
Minimal strategy refers to conditions 4 and 5 from EIP-3076 and is implemented in Lodestar
lodestar/packages/validator/src/slashingProtection/attestation/index.ts
Lines 99 to 121 in 3ed8d04
| * on first startup with an empty db. See https://github.com/ChainSafe/lodestar/issues/5356 for more details on the issue. | ||
| * | ||
| * The value 4096 has been chosen as it is the default used by slashers (https://lighthouse-book.sigmaprime.io/slasher.html#history-length) | ||
| * and is generally higher than the weak subjectivity period. However, it would still be risky if we just relied on min-max surround |
There was a problem hiding this comment.
Right now, the weak subjectivity period for goerli is 3686 epochs and for mainnet 3442 epochs.
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
force-pushed
the
nflaig/limit-max-epoch-lookback
branch
2 times, most recently
from
842dbea to
f738b3c
Compare
nflaig
force-pushed
the
nflaig/limit-max-epoch-lookback
branch
from
f738b3c to
d997504
Compare
wemeetagain
approved these changes
May 2, 2023
dapplion
approved these changes
May 3, 2023
|
🎉 This PR is included in v1.9.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 #5356
Description
Limits number of epochs in the past to check for surrounding attestations (min-span) to 4096.
DB size
Running with 2k keys, the DB size was initially 4GB and is now reduced to 100MB.
Metrics
These are the metrics when running a VC with 2k keys and an empty db.
There are still quite a lot of read/writes but it goes down very fast compared to initially and does no longer cause instability in VC.
Request times do not show any I/O lag, previously was running into timeouts (10 seconds).
For the first 1-2 minutes there is still a increased step times diff but it goes down very fast, after a few minutes instead of hours as before.
Metrics look good to me now, there are no big issues and also need to keep in mind that those metrics are collected running 2k validators which is not a normal setup. This also only happens if the db is empty, meaning for most setups only on initial installation.