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

fix: passing of timeouts in chunkedValidators #142

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

y0sher
Copy link

@y0sher y0sher commented May 30, 2024

Timeouts weren't being passed to chunked validators request, causing deadline exceeds on big queries.
All credits goes to @moshe-blox which is on vacation, just opening the PR to allow faster resolve on main branch.

@mcdee
Copy link
Contributor

mcdee commented May 31, 2024

I think this is a little more subtle than it appears at first glance. For example, say you send a request to obtain validators with a timeout of 2s. The number of validators requested means that there are 10 chunked requests to make, and each takes 1.5s to return. In this situation the call will take 15s, even though a timeout of 2s was passed to it.

One option would be to calculate the number of chunks, and split the timeout by that number, (e.g. if there were 10 chunks and a top-level timeout of 20s then each chunk's timeout would be 2s) but it may be that the first request takes longer than the others, if it's fetching data from an older state which is then cached for subsequent chunks. So there is no ideal solution with the current setup.

That said, not passing any timeout to the chunked requests is probably the worst situation so I'm inclined to take this and accept the rough edge.

The is a newer POST endpoint for validators; last time I looked it wasn't fully supported by all clients but I'll take another to see if it is now available. That would address this issue and allow us to remove the whole chunking code, which falls into the "unwanted but necessary" category.

@y0sher
Copy link
Author

y0sher commented Jun 2, 2024

@mcdee I agree the passed timeout should probably include all requests. We don't want to consumer to have longer timeout than what passed. But as you mentioned, currently no timeout passed at all caused for us issues where we couldn't pass big requests at all.
I used that endpoint in Prysm and LH for sure.

@mcdee
Copy link
Contributor

mcdee commented Jun 3, 2024

Yeah I think that I'm going to accept this as-is, and accept that there is a weakness here that will be addressed by using the POST variant once it is fully supported.

@mcdee mcdee merged commit 23b592c into attestantio:master Jun 3, 2024
3 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.

3 participants