Max reconstruction delay as a function of slot time #8067
Conversation
PoulavBhowmick03
force-pushed
the
delay/slot_duration
branch
from
25ab98c to
8012a09
Compare
| } | ||
| InboundEvent::Msg(DelayColumnReconstruction(request)) => { | ||
| let mut reconstruction_delay = QUEUED_RECONSTRUCTION_DELAY; | ||
| let slot_duration = self.slot_clock.slot_duration().as_millis() as f64; |
There was a problem hiding this comment.
We avoid doing any float arithmetic due to rounding issues and general flakiness. Integer arithmetic in milliseconds would be my preference
There was a problem hiding this comment.
yep, might be worth adding this preference to CLAUDE.md too
There was a problem hiding this comment.
We avoid doing any float arithmetic due to rounding issues and general flakiness. Integer arithmetic in milliseconds would be my preference
got it. will make that change
There was a problem hiding this comment.
We avoid doing any float arithmetic due to rounding issues and general flakiness. Integer arithmetic in milliseconds would be my preference
in that case, i was initially having RECONSTRUCTION_DEADLINE as a fraction (1/4) and then multiplying the slot_duration in millis with the numerator and denominator of the same. should i keep that approach?
let reconstruction_deadline_millis =
(slot_duration_millis * RECONSTRUCTION_DEADLINE.0)
/ RECONSTRUCTION_DEADLINE.1;
let reconstruction_deadline = Duration::from_millis(reconstruction_deadline_millis);There was a problem hiding this comment.
I think the simplest is probably something like slot_duration_ms * reconstruction_deadline / 100. Or just divide by 4. Seems a bit unnecessary to multiply by one
michaelsproul
added
the
waiting-on-author
PoulavBhowmick03
force-pushed
the
delay/slot_duration
branch
from
8012a09 to
9d896af
Compare
jimmygchen
added
ready-for-review
eserilev
left a comment
eserilev
left a comment
There was a problem hiding this comment.
Not the biggest fan of the tuple "fraction" but I think this fine and we need it for the upcoming release. Once we merge this #7944 we may be able use similar logic to calculate the max reconstruction deadline and clean this up a bit
michaelsproul
left a comment
michaelsproul
left a comment
There was a problem hiding this comment.
Looks good to me, but we should test on some live networks to make sure this behaves as expected (could merge to unstable first I guess)
jimmygchen
added
v8.0.0
| (slot_duration * RECONSTRUCTION_DEADLINE.0) / RECONSTRUCTION_DEADLINE.1; | ||
| let reconstruction_deadline = Duration::from_millis(reconstruction_deadline_millis); | ||
| if let Some(seconds_from_current_slot) = | ||
| self.slot_clock.seconds_from_current_slot_start() |
There was a problem hiding this comment.
This function only gives us granuarity to the seconds I believe
lighthouse/common/slot_clock/src/lib.rs
Lines 108 to 110 in 522bd9e
So on a network like gnosis (5s slot times), reconstruction is only triggered immediately if seconds_from_current_slot_start is 2s
…adline (#8246) This recent PR below changes the max reconstruction delay to be a function of slot time. However it uses `seconds_from_slot_start` when comparing (and dropping `nano`), so it might delay reconstruction on networks where the slot time isn’t a multiple of 4, e.g. on gnosis this only happens at 2s instead of 1.25s.: - #8067 (comment) Use `millis_from_slot_start` when comparing against reconstruction deadline Also added some tests for reconstruction delay. Co-Authored-By: Jimmy Chen <[email protected]>
Fixes sigp#8054 Co-Authored-By: PoulavBhowmick03 <[email protected]>
…adline (sigp#8246) This recent PR below changes the max reconstruction delay to be a function of slot time. However it uses `seconds_from_slot_start` when comparing (and dropping `nano`), so it might delay reconstruction on networks where the slot time isn’t a multiple of 4, e.g. on gnosis this only happens at 2s instead of 1.25s.: - sigp#8067 (comment) Use `millis_from_slot_start` when comparing against reconstruction deadline Also added some tests for reconstruction delay. Co-Authored-By: Jimmy Chen <[email protected]>
4 participants
Issue Addressed
Fixes #8054