[Merged by Bors] - Retry registration timeout fix

[ConvallariaMaj]

Motivation

Fix an issue #6035

Description

Closes #6035 `

Fix first half of issue: established separate parameter for submit challenge timeout. If timeout is not set, submitting challenge will continue till poet round starts.
Added tests and comments to config parameters.

Second half of issue is fixed within #6116

Test Plan

Tested with unit tests

TODO

• Explain motivation or link existing issue(s)
• Test changes and document test plan
• Update documentation as needed
• Update changelog as needed

[fasmat]

I don't quite understand what the purpose of "Default Timeout" is, when it is only used for a single type of request and for others individual timeouts have to be specified.

[fasmat]

The changes seem to also have broken other tests that now hang when executed.

[poszu]

Removing the timeout is not enough. It will now retry to submit MaxRequestRetries times (handled by the go-retryablehttp library), but no more. We need to manually retry submitting till the end of the cycle gap (probably the best place to do this is in the PoetService::Submit() method.

Please, also add a test checking that it retries submitting up until the cycle gap.

[fasmat]

Removing the timeout is not enough. It will now retry to submit MaxRequestRetries times (handled by the go-retryablehttp library), but no more. We need to manually retry submitting till the end of the cycle gap (probably the best place to do this is in the PoetService::Submit() method.

Please, also add a test checking that it retries submitting up until the cycle gap.

I agree that this might need to be changed but I don't agree with thee suggested approach of retrying the failed retryablehttp.Client request. Instead the Submit shouldn't have an "unlimited" number (math.MaxInt) of retries instead.

The HTTPPoetClient can have 2 retryablehttp.Clients inside - one with the limited number for proof fetches and info request and one with "unlimited" retries for submit.

Wdyt?

[poszu]

I agree that this might need to be changed but I don't agree with thee suggested approach of retrying the failed retryablehttp.Client request. Instead the Submit shouldn't have an "unlimited" number (math.MaxInt) of retries instead.

The HTTPPoetClient can have 2 retryablehttp.Clients inside - one with the limited number for proof fetches and info request and one with "unlimited" retries for submit.

Wdyt?

Retrying via the mechanism built in the retryablehttp.Client has the benefit (I think?) of giving up after non-retryable failures so it makes sense. Perhaps it's possible to adjust retry mechanism on per-request basis instead of keeping 2 clients around?

[fasmat]

Perhaps it's possible to adjust retry mechanism on per-request basis instead of keeping 2 clients around?

Maybe, I didn't check the full documentation but even if not, an http client isn't very expensive to keep around - from the users perspective its still the PoetClient with a Submit and Proof method, but internally it uses 2 http clients; I don't see an issue with this 🙂

[fasmat]

I think we are using wrong backoff functions. For the client I think we should keep using retryablehttp.LinearJitterBackoff. It uses a linear backoff that increases the time between retries by numAttempts * rand(min, max). Given that we have 10 seconds as min and 20 seconds as max this will take at least 550 and at most 1100 seconds (which is our timeout) with increasing delays between requests (to avoid putting to much load on an already struggling PoET).

For submit we also want some jitter to not have all nodes query a poet at exactly the same time, but we do not want to increase the time between requests by too much. I believe a backoff function like this can be used:

// customJitterBackoff always waits between `min` and `max` before retrying
func customJitterBackoff(min, max time.Duration, attemptNum int, resp *http.Response) time.Duration {
	if max <= min {
		return min
	}

	return min + rand.N(max-min)
}

[ConvallariaMaj]

bors merge

[spacemesh-bors]

Pull request successfully merged into develop.

Build succeeded:

• ci-status
• systest-status