Skip to content
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

Why ec delay multiplier must be non-zero? #783

Open
masih opened this issue Dec 10, 2024 · 7 comments
Open

Why ec delay multiplier must be non-zero? #783

masih opened this issue Dec 10, 2024 · 7 comments

Comments

@masih
Copy link
Member

masih commented Dec 10, 2024

It is unclear why this condition exists. Looking at the code, zero can also be a valid value. No?

return fmt.Errorf("EC delay multiplier must positive, was %f", e.DelayMultiplier)

@github-project-automation github-project-automation bot moved this to Todo in F3 Dec 10, 2024
@masih masih changed the title Why ec delay multiplier must be positive? Why ec delay multiplier must be non-zero? Dec 10, 2024
@Stebalien
Copy link
Member

If that's zero,

go-f3/host.go

Line 346 in 9c8c852

ecDelay := time.Duration(h.manifest.EC.DelayMultiplier * float64(h.manifest.EC.Period))
becomes zero.

Honestly, I think 1 is the sane minimum in general. Unless I'm missing something.

@masih
Copy link
Member Author

masih commented Dec 10, 2024

This ticket was captured as part of running a passive testing that was attempting to minimise the wait time between instances. Specifically to get ecDelay to indeed be zero. Because, we will always wait for HeadLookback number of epochs anyway.

My question remains: Why ecDelay of zero is invalid?

In terms of importance, maybe this does not matter anymore. Or at least it will not matter once transport is made more efficient such that the actual delta is a value closer to the default of 3 seconds.

@Kubuxu
Copy link
Contributor

Kubuxu commented Dec 11, 2024

If we are in a caught-up regime, if ECDelay is zero, we will start a new instance before new tipsets become available. Plus, base decision behaviour depends on ECDelay being non-zero.

@masih
Copy link
Member Author

masih commented Dec 11, 2024

we will start a new instance before new tipsets become available

Why? Headlookback should avoid this already?

@Kubuxu
Copy link
Contributor

Kubuxu commented Dec 11, 2024

It doesn't headlookback just compensates from headlookback, not for tipsets that get created during the instance.

@masih
Copy link
Member Author

masih commented Dec 11, 2024

Right, but we take the max of 0 and len-headlookback when collecting chain. If we are too close to the head then, would that not end up delaying the instance anyway? i.e. collecting base as the chain which would eventually back off from the start of next instance?

@Kubuxu
Copy link
Contributor

Kubuxu commented Dec 12, 2024

It will be missing one or two epochs, I think two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

No branches or pull requests

3 participants