-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add a new slot ticker and use it on attestation aggregation #12377
Changes from all commits
bc915ed
8824fae
67f2f38
1e984a2
80a9bf2
a00bb32
8b672a2
8b61532
c5d0a3a
2e14d09
e34afe9
3b8bec7
6ebcbc1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,27 +7,39 @@ import ( | |
"time" | ||
|
||
"github.com/prysmaticlabs/go-bitfield" | ||
"github.com/prysmaticlabs/prysm/v4/config/features" | ||
"github.com/prysmaticlabs/prysm/v4/config/params" | ||
"github.com/prysmaticlabs/prysm/v4/crypto/hash" | ||
ethpb "github.com/prysmaticlabs/prysm/v4/proto/prysm/v1alpha1" | ||
attaggregation "github.com/prysmaticlabs/prysm/v4/proto/prysm/v1alpha1/attestation/aggregation/attestations" | ||
"github.com/prysmaticlabs/prysm/v4/time/slots" | ||
"go.opencensus.io/trace" | ||
) | ||
|
||
// Prepare attestations for fork choice three times per slot. | ||
var prepareForkChoiceAttsPeriod = slots.DivideSlotBy(3 /* times-per-slot */) | ||
|
||
// This prepares fork choice attestations by running batchForkChoiceAtts | ||
// every prepareForkChoiceAttsPeriod. | ||
func (s *Service) prepareForkChoiceAtts() { | ||
ticker := time.NewTicker(prepareForkChoiceAttsPeriod) | ||
defer ticker.Stop() | ||
intervals := features.Get().AggregateIntervals | ||
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 { | ||
for i, offset := range intervals { | ||
intervals[i] = offset / 2 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this halved? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
} else { | ||
break | ||
} | ||
} | ||
ticker := slots.NewSlotTickerWithIntervals(time.Unix(int64(s.genesisTime), 0), intervals) | ||
for { | ||
select { | ||
case <-ticker.C: | ||
case <-ticker.C(): | ||
t := time.Now() | ||
if err := s.batchForkChoiceAtts(s.ctx); err != nil { | ||
log.WithError(err).Error("Could not prepare attestations for fork choice") | ||
} | ||
log.WithField("latency", time.Since(t).Milliseconds()).Debug("batched forkchoice attestations") | ||
case <-s.ctx.Done(): | ||
log.Debug("Context closed, exiting routine") | ||
return | ||
|
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.
Optional: This will panic if len(intervals) == 0. That might be OK, but consider a meaningful panic like
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.