-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Spec 6s slot times (EIP-7782) #4484
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
base: master
Are you sure you want to change the base?
Conversation
are we putting the change to rewards in another PR or are we looking at that in this PR too? |
I think it's already covered by adjusting |
# Calculate the time when EIP-7782 fork occurs | ||
if EIP7782_FORK_EPOCH == FAR_FUTURE_EPOCH: | ||
# If EIP-7782 is not scheduled, use standard slot duration | ||
return (store_time_ms - genesis_time_ms) // SLOT_DURATION_MS |
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 don't believe this will actually work here. In our test framework, genesis starts with EIP-7782 even if EIP7782_FORK_EPOCH
is still FAR_FUTURE_EPOCH
. So this wouldn't work properly until we schedule the fork (ie assign a real epoch) which won't happen until a couple months before. The way the testing framework was designed doesn't handle this situation well.
Ok great - had missed that but yes. |
""" | ||
churn = max( | ||
MIN_PER_EPOCH_CHURN_LIMIT_EIP7782, | ||
get_total_active_balance(state) // CHURN_LIMIT_QUOTIENT, |
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.
Do we need to double CHURN_LIMIT_QUOTIENT
?
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.
Weak subjectivity is epoch based so this is possibly ok, but may ask @mkalinin
I think |
MIN_PER_EPOCH_CHURN_LIMIT_EIP7782: 64000000000 | ||
# 2**7 * 10**9 (= 128,000,000,000) Gwei | ||
MAX_PER_EPOCH_ACTIVATION_EXIT_CHURN_LIMIT_EIP7782: 128000000000 | ||
|
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.
might also need to double MIN_EPOCHS_FOR_DATA_COLUMN_SIDECARS_REQUESTS
, otherwise the data availability window goes down from ~18 days to just ~9 days
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 was just thinking about this one :)
where do we stand on sync committee period? there was a desire originally to have it fairly stable, so it only rotated once a day... Is that still desirable? if so we should probably double that too.. |
Yes, agreed, I think sync periods should stay the same, missed that one. Thanks! |
probably also worth considering halving the |
On top of |
PAYLOAD_ATTESTATION_DUE_BPS: 7500 | ||
|
||
# EIP-7782 | ||
# [customized] 3000 milliseconds, 3 seconds |
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.
# [customized] 3000 milliseconds, 3 seconds | |
# [customized] 6000 milliseconds, 6 seconds |
```python | ||
def get_attestation_due_ms(epoch: Epoch) -> uint64: | ||
""" | ||
Return the attestation due time in milliseconds for the given epoch. | ||
""" | ||
if epoch >= EIP7782_FORK_EPOCH: | ||
return ATTESTATION_DUE_BPS_EIP7782 * SLOT_DURATION_MS_EIP7782 // BASIS_POINTS | ||
else: | ||
return ATTESTATION_DUE_BPS * SLOT_DURATION_MS // BASIS_POINTS | ||
``` |
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.
Hey @dankrad these modifications don't match the way we do this elsewhere. Like:
consensus-specs/specs/gloas/fork-choice.md
Lines 440 to 444 in f0b3bca
def get_attestation_due_ms(epoch: Epoch) -> uint64: | |
# [New in Gloas] | |
if epoch >= GLOAS_FORK_EPOCH: | |
return get_slot_component_duration_ms(ATTESTATION_DUE_BPS_GLOAS) | |
return get_slot_component_duration_ms(ATTESTATION_DUE_BPS) |
For this function, it should be:
def get_attestation_due_ms(epoch: Epoch) -> uint64:
# [New in EIP7782]
if epoch >= EIP7782_FORK_EPOCH:
get_slot_component_duration_ms(ATTESTATION_DUE_BPS_EIP7782)
return get_slot_component_duration_ms(ATTESTATION_DUE_BPS)
And we need to update get_slot_component_duration_ms
to be:
def get_slot_component_duration_ms(basis_points: uint64) -> uint64:
"""
Calculate the duration of a slot component in milliseconds.
"""
return basis_points * SLOT_DURATION_MS_EIP7782 // BASIS_POINTS
At the fork, the updated get_slot_component_duration_ms
will be used. It doesn't need to know the epoch.
Spec for 6s slot times. Based on #4476 .
TODOs:
get_forkchoice_store
on_tick
get_slots_since_genesis
logicFailing tests