Skip to content

fix NewSlotTickerWithIntervals Panics #13307

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

fix NewSlotTickerWithIntervals Panics #13307

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
685b45e
fix tests
v4lproik Dec 8, 2023
3be6b98
fix frozen clock in tests
v4lproik Dec 8, 2023
1e34f1c
fix invalid decreasing offsets
v4lproik Dec 8, 2023
e75bbf5
adding negative test value
v4lproik Dec 8, 2023
1320335
adding specific error if offset is negative
v4lproik Dec 8, 2023
576b23a
saturating sub
v4lproik Dec 8, 2023
32c19f1
to seconds
v4lproik Dec 8, 2023
e02f7c0
Merge branch 'develop' into fix-13280-issue
v4lproik Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion beacon-chain/blockchain/receive_attestation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func (s *Service) spawnProcessAttestationsRoutine() {
return
}

reorgInterval := time.Second*time.Duration(params.BeaconConfig().SecondsPerSlot) - reorgLateBlockCountAttestations
reorgInterval := saturatingReorgInterval()
ticker := slots.NewSlotTickerWithIntervals(s.genesisTime, []time.Duration{0, reorgInterval})
for {
select {
Expand All @@ -111,6 +111,16 @@ func (s *Service) spawnProcessAttestationsRoutine() {
}()
}

// saturatingReorgInterval makes sure that the reorg interval cannot be a negative number
// it should be saturated to 0 if it's the case
func saturatingReorgInterval() time.Duration {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than setting this as zero, it might be better to set the following below as a dynamic value:

const reorgLateBlockCountAttestations = 2 * time.Second

Ex: If 2 seconds maps to 12 second slots, then for 1 second slots it should be 160ms. Otherwise the reorg interval starts breaking for non-standard configs.

cc @potuz might have some thoughts since this was introduced by him

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we had discussed that this constant would be irrelevant on non-mainnet networks because we wouldn't be reorging late blocks, but now this constant becomes a higher issue if we're removing the flag to disable this behavior. I am not sure what is the right approach here. Computing this to be SecondsPerSlot / 2 / intervalsPerSlot would produce the right value for mainnet. I suspect that any network that tries to have short blocks would be very broken by the reorg feature.

reorgInterval := time.Second*time.Duration(params.BeaconConfig().SecondsPerSlot) - reorgLateBlockCountAttestations
if reorgInterval.Seconds() < 0 {
reorgInterval = 0
}
return reorgInterval
}

// UpdateHead updates the canonical head of the chain based on information from fork-choice attestations and votes.
// The caller of this function MUST hold a lock in forkchoice
func (s *Service) UpdateHead(ctx context.Context, proposingSlot primitives.Slot) {
Expand Down
5 changes: 4 additions & 1 deletion time/slots/slotticker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ func (s *SlotIntervalTicker) startWithIntervals(
// Caller is responsible to input the intervals in increasing order and none bigger or equal than
// SecondsPerSlot
func NewSlotTickerWithIntervals(genesisTime time.Time, intervals []time.Duration) *SlotIntervalTicker {
if genesisTime.Unix() == 0 {
if genesisTime.Unix() <= 0 {
panic("zero genesis time")
}
if len(intervals) == 0 {
Expand All @@ -186,6 +186,9 @@ func NewSlotTickerWithIntervals(genesisTime time.Time, intervals []time.Duration
slotDuration := time.Duration(params.BeaconConfig().SecondsPerSlot) * time.Second
lastOffset := time.Duration(0)
for _, offset := range intervals {
if offset.Seconds() < 0 {
panic("invalid negative offset")
}
if offset < lastOffset {
panic("invalid decreasing offsets")
}
Expand Down
12 changes: 7 additions & 5 deletions time/slots/slotticker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,15 +172,17 @@ func TestSlotTickerWithIntervalsInputValidation(t *testing.T) {
panicCall := func() {
NewSlotTickerWithIntervals(genesisTime, intervals)
}
require.Panics(t, panicCall, "zero genesis time")
require.PanicsWithValue(t, "zero genesis time", panicCall)
genesisTime = time.Now()
require.Panics(t, panicCall, "at least one interval has to be entered")
require.PanicsWithValue(t, "at least one interval has to be entered", panicCall)
intervals = []time.Duration{offset, -offset}
require.PanicsWithValue(t, "invalid negative offset", panicCall)
intervals = []time.Duration{2 * offset, offset}
require.Panics(t, panicCall, "invalid decreasing offsets")
require.PanicsWithValue(t, "invalid decreasing offsets", panicCall)
intervals = []time.Duration{offset, 4 * offset}
require.Panics(t, panicCall, "invalid ticker offset")
require.PanicsWithValue(t, "invalid ticker offset", panicCall)
intervals = []time.Duration{4 * offset, offset}
require.Panics(t, panicCall, "invalid ticker offset")
require.PanicsWithValue(t, "invalid ticker offset", panicCall)
intervals = []time.Duration{offset, 2 * offset}
require.NotPanics(t, panicCall)
}