Add a new slot ticker and use it on attestation aggregation #12377
Conversation
potuz
changed the title
|
Blocking for Preston to take a look, would like to deploy this 24 hours in the Prater fleet |
| slotDuration := time.Duration(params.BeaconConfig().SecondsPerSlot) * time.Second | ||
| // Adjust intervals for networks with a lower slot duration (Hive, e2e, etc) | ||
| for { | ||
| if intervals[len(intervals)-1] >= slotDuration { |
There was a problem hiding this comment.
Optional: This will panic if len(intervals) == 0. That might be OK, but consider a meaningful panic like
if len(intervals) == 0 {
panic("unable to create timers for attestation aggregation with no intervals provided")
}In my opinion, that is better than index out of range panic. Ideally, we never panic and simply return an error here though
If this code is temporary for the feature flag, then it might be OK as is. I'm thinking about future proofing so if there is not much future for this then you can dismiss this suggestion.
There was a problem hiding this comment.
panic is good here IMO since the intervals are hardcoded configuration constants with defaults. Someone would have to remove the code with the configuration constants for this to panic.
| for { | ||
| if intervals[len(intervals)-1] >= slotDuration { | ||
| for i, offset := range intervals { | ||
| intervals[i] = offset / 2 |
There was a problem hiding this comment.
because the intervals are set by default to be something that works for 12 seconds per slot, but special networks use other parameters. We are halving them to deal with the most common scenario of 6 seconds per slot in Hive. If someone wants to try 4 seconds per slot (which doesn't work even on interop) at least this prevents the panic
time/slots/slotticker.go
Outdated
| func NewSlotTickerWithIntervals(genesisTime time.Time, intervals []time.Duration) *SlotTicker { | ||
| if genesisTime.Unix() == 0 { | ||
| panic("zero genesis time") | ||
| } | ||
| if len(intervals) == 0 { | ||
| panic("at least one interval has to be entered") | ||
| } | ||
| slotDuration := time.Duration(params.BeaconConfig().SecondsPerSlot) * time.Second | ||
| lastOffset := time.Duration(0) | ||
| for _, offset := range intervals { | ||
| if offset < lastOffset { | ||
| panic("invalid decreasing offsets") | ||
| } | ||
| if offset >= slotDuration { | ||
| panic("invalid ticker offset") | ||
| } | ||
| } | ||
| ticker := &SlotTicker{ | ||
| c: make(chan primitives.Slot), | ||
| done: make(chan struct{}), | ||
| } | ||
| ticker.startWithIntervals(genesisTime, prysmTime.Until, time.After, intervals) | ||
| return ticker | ||
| } |
There was a problem hiding this comment.
Consider returning an error instead of panicking.
Could you add tests for the input validation? Or is this more temporary feature testing logic?
| @@ -51,10 +53,24 @@ func NewService(ctx context.Context, cfg *Config) (*Service, error) { | |||
|
|
|||
| // Start an attestation pool service's main event loop. | |||
| func (s *Service) Start() { | |||
| if err := s.waitForSync(s.cfg.InitialSyncComplete); err != nil { | |||
| log.WithError(err) | |||
There was a problem hiding this comment.
This won't log anything. log.WithError(err) is an alias for log.WithField("error", err), you still have to call log.WithError(err).Error("Failed to wait for sync") or some other log method.
There was a problem hiding this comment.
yeah sorry, forgot to add the message, pushing the fix
potuz
added
OK to merge
and removed
Blocked
This PR adds:
SlotTickerWithIntervalsthat accepts a series of intervals between 0 and 12 seconds (not inclusive) so that the ticker ticks every slot at those offsets.