Deterministic Round Timeout #1120
Conversation
|
This PR looks good. The only thing I'm missing is a tests that test this functionality. |
protocol/v2/qbft/roundtimer/timer.go
Outdated
| ) | ||
|
|
||
| type RoundTimeoutFunc func(specqbft.Round) time.Duration | ||
| type TimeAtSlotFunc func(slot phase0.Slot) int64 |
There was a problem hiding this comment.
its a bit confusing that we're using slot and height here. lets stick to either height or slot.
There was a problem hiding this comment.
Looks good! I have one comment on naming. this is diverted from spec as we discussed. lets get review from @moshe-blox and @lior-blox and then add it to the spec alignment script as an exception.
operator/validator/controller.go
Outdated
| @@ -809,7 +809,7 @@ func SetupRunners(ctx context.Context, logger *zap.Logger, options validator.Opt | |||
| }, | |||
| Storage: options.Storage.Get(role), | |||
| Network: options.Network, | |||
| Timer: roundtimer.New(ctx, nil), | |||
| Timer: roundtimer.New(ctx, role, options.BeaconNetwork.EstimatedTimeAtSlot, nil), | |||
There was a problem hiding this comment.
| Timer: roundtimer.New(ctx, role, options.BeaconNetwork.EstimatedTimeAtSlot, nil), | |
| Timer: roundtimer.New(ctx, role, func(height specbft.Height) { return options.BeaconNetwork.EstimatedTimeAtSlot(phase.Slot(height)), nil), |
There was a problem hiding this comment.
LGTM, again lets make sure we're ok with the spec divergence @moshe-blox @lior-blox
protocol/v2/qbft/roundtimer/timer.go
Outdated
| func RoundTimeout(timeAtSlotF TimeAtSlotFunc, role spectypes.BeaconRole, height specqbft.Height, round specqbft.Round) time.Duration { | ||
| if round == specqbft.FirstRound && (role == spectypes.BNRoleAttester || role == spectypes.BNRoleSyncCommittee) { | ||
| dutyStartTime := time.Unix(timeAtSlotF(phase0.Slot(height)), 0) | ||
| return time.Until(dutyStartTime.Add(firstRoundTimeout)) |
There was a problem hiding this comment.
what happens if we started the duty very late into the slot (for example second 7) and this is in the past? we would need to start consensus at round 2, no? do we need a test for it?
| @@ -66,7 +66,7 @@ func (i *Instance) Start(logger *zap.Logger, value []byte, height specqbft.Heigh | |||
| i.State.Height = height | |||
| i.metrics.StartStage() | |||
|
|
|||
| i.config.GetTimer().TimeoutForRound(specqbft.FirstRound) | |||
| i.config.GetTimer().TimeoutForRound(height, specqbft.FirstRound) | |||
There was a problem hiding this comment.
i think might have to to start not always at FirstRound but at a func that returns current round according to current time, otherwise if you start duty at second 7 you will start at wrong round
however, would that be a spec change? @lior-blox @GalRogozinski
|
@lior-blox @alonmuroch @y0sher Besides the SIP maybe this should also go inside the spec code imo |
|
First TODO is here |
This PR brings significant updates to the
roundtimerpackage, focusing on improving the round timeout logic and introducing new features for better synchronization and configurability.Key Changes:
Refactored
RoundTimeoutFunction:To ensure synchronized timeouts across instances, the timeout is now calculated based on the duty start time, which is derived from the slot height
Configurable Timeout Options:
TimeoutOptionsstruct has been introduced to make the quick and slow timeout thresholds and durations configurable.BeaconNetworkinterface has been added to abstract the logic for getting slot start times and slot durations. This makes the package more modular and easier to test.TODOs Addressed:
Update SIP for Deterministic Round Timeout:
Decide if to Make the Proposer Timeout Deterministic:
SIP Reference:
For more details, see the related SIP at SIP-22.