-
Notifications
You must be signed in to change notification settings - Fork 973
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
update penalty params for Merge #2698
Conversation
4ffd780
to
78040ac
Compare
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.
LGTM, but needs some minor comment updates in preset files.
@@ -223,6 +245,62 @@ def compute_timestamp_at_slot(state: BeaconState, slot: Slot) -> uint64: | |||
return uint64(state.genesis_time + slots_since_genesis * SECONDS_PER_SLOT) | |||
``` | |||
|
|||
### Beacon state accessors |
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.
Altair is frozen, but it might make sense to go for code-reuse anyway by not copying it over. That said, I'm also kind of happy we can easily extract a version of the spec that doesn't contain any legacy preset/config options, for new client implementations to join.
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.
by instead providing a config getter function -- e.g. get_inactivity_penalty_quotient(epoch)
or something?
yeah, I see the argument on both sides ..
Co-authored-by: Diederik Loerakker <proto@protolambda.com>
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.
Looks good.
In Altair beacon-chain.md "Introduction" section, there is a note of penalty parameter updates towards their planned maximally punitive values
. I'd say let's add some description to Merge beacon-chain.md introduction as well.
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.
lgtm
Co-authored-by: Diederik Loerakker <proto@protolambda.com>
As described in EIP-2982, punitive penalty params will be adjusted upward to maximum security for the Merge.
(note, these were partially increased during Altair