fix NewSlotTickerWithIntervals Panics

[v4lproik]

What type of PR is this?

Bug fix: #13280

There's several issues highlighted by this bug which are:

• The tests do not properly check the panic message
• One of the tests checking if zero genesis error message is never reached
• receive_attestation.spawnProcessAttestationsRoutine() should check if the reorgInterval := time.Second*time.Duration(params.BeaconConfig().SecondsPerSlot) - reorgLateBlockCountAttestations can be of a negative value and saturate to 0 if it's the case.

[CLAassistant]

All committers have signed the CLA.

[nisdas]

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

[potuz]

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.

[nisdas]

any update on this @v4lproik ?

[v4lproik]

Sorry @nisdas, I thought since you weren't entirely certain whether it was the right approach or not, you might discuss it internally.

In my opinion, based on what I have observed in other CL implementations and specifically in LH, attempting to handle specific configurations depending on a value that is not expected initially is counterintuitive and prone to misunderstanding. I believe that calculating an interval or distance that directly depends on slots and/or durations should only make sense within the realm of the chain specs. Here, the value doesn't make sense as we have a negative value for the reorg interval, meaning that several options are available in order to tackle this issue (from least favorable to best in my opinion):

1/ We disable the feature for that particular case. I think this approach won't pay off in the long run, as it would be misleading trying to understand which values could potentially be accepted. For example, if the slot duration is 2, then the value is not negative but equal to 0, what if it's 3 then? What are the impact on other features in the code with those values? What's acceptable? It sounds like a rabbit hole.

2/ We do not run the service at all if we have a negative interval value. This is critical enough to prevent the service from running as distances and intervals cannot be negative. (As you can see, I added a check in this PR and a test).

3/ We saturate the value so it cannot be negative, we throw a log error as it’s critical enough informing the user that it's not the expected configuration, surely won't work as intended and should only be used at their own risk. Happy to add the error log to this PR if that's the consensus.

Let me know your thoughts!