Replace INTERVALS_PER_SLOT with explicit slot component times
#7944
Conversation
eserilev
added
spec_change
|
Should this change consider that the slot time will change in the future? Or that's to be handled in a future PR |
I think we should handle that in a future PR to keep this diff here as small as possible |
|
Fixed the above comments |
eserilev
added
ready-for-review
|
Thanks for addressing the feedback! The fixes look good |
|
This pull request has merge conflicts. Could you please resolve them @eserilev? 🙏 |
mergify
bot
added
waiting-on-author
eserilev
added
ready-for-review
|
|
||
| let unaggregated_attestation_due = self | ||
| .chain_spec | ||
| .get_unaggregated_attestation_due() |
There was a problem hiding this comment.
shouldn't function calls like these take in current epoch? - this way the gloas versions of timing vars can fetched when needed
There was a problem hiding this comment.
thats out of scope for this PR
|
This pull request has merge conflicts. Could you please resolve them @eserilev? 🙏 |
mergify
bot
added
waiting-on-author
eserilev
added
ready-for-review
|
|
||
| // Test unaggregated attestation (3333 bps = 33.33% of 12s = 4s) | ||
| let unagg_due = spec.get_unaggregated_attestation_due().unwrap(); | ||
| assert_eq!(unagg_due, Duration::from_millis(3999)); // 12000 * 3333 / 10000 |
There was a problem hiding this comment.
Is it intentional that the attestation deadline is 1ms earlier?
There was a problem hiding this comment.
yeah these deadlines are percentages now, e.g. the spec defines the unagg attestation deadline as 33.33% into the slot which is 3.99 seconds
|
|
||
| /// Get the duration into a slot in which an unaggregated attestation is due | ||
| pub fn get_unaggregated_attestation_due(&self) -> Result<Duration, ArithError> { | ||
| self.get_slot_component_duration(self.attestation_due_bps) |
There was a problem hiding this comment.
Is it worth pre-computing these so we don't have to compute and handle error at runtime? similar to the "Networking Derived" config above
There was a problem hiding this comment.
Yeah i was a bit on the fence about this. My reasoning for not pre-computing is that when we move to 6 second slot times, we'll have to compute this at runtime. Also in epbs the attestation times change so this will also need to be computed at runtime. We'll need to add a slot as an argument to this function
| #[serde(with = "serde_utils::quoted_u64")] | ||
| seconds_per_slot: u64, | ||
| #[serde(with = "serde_utils::quoted_u64")] | ||
| slot_duration_ms: u64, |
There was a problem hiding this comment.
Needs a default for backwards compat with BNs that don't have this field
jimmygchen
added
waiting-on-author
6 participants
Issue Addressed
ethereum/consensus-specs#4476
Additional Info
This change affects many critical paths in lighthouse and should not be merged until after
8.0.0the Ethereum kurtosis package is working to support this change here: ethpandaops/ethereum-package#1168