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

app: increase beacon node timeout #42

Merged
merged 3 commits into from
Dec 14, 2023
Merged

Conversation

gsora
Copy link
Collaborator

@gsora gsora commented Dec 14, 2023

Explicitly set beacon node query timeouts to (validatorAmount/50)*20 seconds.

This also sets the amount of validator to query in each chunk -- which is done internally by go-eth2-client -- to 50.

This should allow even the worst-performing beacon nodes to still fetch data.

category: bug
ticket: none

Explicitly set beacon node query timeouts to `(validatorAmount/50)*20 seconds`.

This also sets the amount of validator to query in each chunk -- which is done internally by `go-eth2-client` -- to 50.

This should allow even the worst-performing beacon nodes to still fetch data.
@gsora gsora requested a review from dB2510 December 14, 2023 12:00
commit 686d882
Author: Gianguido Sorà <[email protected]>
Date:   Thu Dec 14 13:00:47 2023 +0100

    *: update to go v1.21.5 (#43)
app/app.go Outdated Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
Co-authored-by: Luke Hackett <[email protected]>
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@@ -16,7 +16,7 @@ jobs:
# Config options can be found in README here: https://github.com/golangci/golangci-lint-action
- uses: actions/setup-go@v4
with:
go-version: '1.21.3'
Copy link

Choose a reason for hiding this comment

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

Thank you for fixing these!

@@ -377,6 +380,15 @@ func eth2Client(ctx context.Context, bnURL string) (eth2wrap.Client, error) {
return eth2wrap.AdaptEth2HTTP(bnClient, maxBeaconNodeTimeout), nil
}

// timeoutByValAmount returns the maximum timeout an eth2http call will have.
// Return a timeout of (valAmount/50)*20, where 20 are the seconds to wait.
func timeoutByValAmount(valAmount uint64) time.Duration {
Copy link
Contributor

@dB2510 dB2510 Dec 14, 2023

Choose a reason for hiding this comment

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

So for 100 validators we will timeout after 40s. Useful in debugging but it should fail fast. Can we make this timeout configurable with a CLI flag instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Useful in debugging but it should fail fast.

Consider that this tool is designed to fetch this data, process and then stop to never be seen again.

Is adding a CLI toggle better than just make it work and forget about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. We can have any timeout as there are no strict timing requirements here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly.

If this was Charon, we could never allow such timeouts.

Copy link
Contributor

@xenowits xenowits left a comment

Choose a reason for hiding this comment

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

LGTM!

I was wondering if the timeout formula (n/50)*20s is the output of an empirical result?

@gsora
Copy link
Collaborator Author

gsora commented Dec 14, 2023

Yes @xenowits, it is the result of testing with Lido operators as well as my local testing.

@gsora gsora merged commit 990beba into main Dec 14, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants