feat: update correlation penalty computation #7071
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #7071 +/- ##
============================================
- Coverage 50.82% 48.93% -1.90%
============================================
Files 597 597
Lines 39789 39809 +20
Branches 2059 2055 -4
============================================
- Hits 20224 19481 -743
- Misses 19565 20287 +722
- Partials 0 41 +41 |
Performance Report✔️ no performance regression detected Full benchmark results
|
|
spec tests failing |
ensi321
changed the title
New slashing penalty calculation preserves every digit in gwei whereas old calculation is rounded to the nearest 1ETH. So this change doesn't retrofit pre-electra forks and will need to wait for next release of spec test in order to test the correctness of this PR. Will convert to draft for now |
ensi321
force-pushed
the
nc/slashing-calculation
branch
from
2eb3e38
to
e07ed10
Compare
twoeths
approved these changes
Sep 27, 2024
nflaig
approved these changes
Sep 27, 2024
| const penalties: number[] = []; | ||
|
|
||
| const penaltiesByEffectiveBalanceIncrement = new Map<number, number>(); | ||
| for (const index of cache.indicesToSlash) { | ||
| const effectiveBalanceIncrement = effectiveBalanceIncrements[index]; | ||
| let penalty = penaltiesByEffectiveBalanceIncrement.get(effectiveBalanceIncrement); | ||
| if (penalty === undefined) { | ||
| const penaltyNumeratorByIncrement = effectiveBalanceIncrement * adjustedTotalSlashingBalanceByIncrement; | ||
| penalty = Math.floor(penaltyNumeratorByIncrement / totalBalanceByIncrement) * increment; | ||
| if (fork < ForkSeq.electra) { |
There was a problem hiding this comment.
any reason why the check is reversed here?
There was a problem hiding this comment.
any reason why the check is reversed here?
No particular reason. I don't think there is a convention for fork check to be >= ForkSeq.electra vs < ForkSeq.electra.
Several places use < see:
philknows
pushed a commit
that referenced
this pull request
Oct 18, 2024
* Update slashing calculation to be more aligned with spec style * Update calculation * Lint * Fix rounding issue * Update calculation * Enable slashing spec test * lint
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.
ethereum/consensus-specs#3882 proposes to reformat slashing penalty calculation due to overflow issue.
Lodestar does not have such issue as we've already been tracking everything in increments.
This PR restyle the calculation to be more in line with the spec.
Although spec change supposedly will be live for devnet-4, we can make the code change now for devnet-3 since no functional difference.