multi-client: failover enhancements - #11
Merged
Merged
Conversation
Author
|
@greptileai could you review this PR for me pls ? |
nkryuchkov
reviewed
Jul 28, 2025
nkryuchkov
previously approved these changes
Jul 31, 2025
nkryuchkov
left a comment
There was a problem hiding this comment.
good job, let's make sure this is tested well enough with real nodes
nkryuchkov
approved these changes
Aug 4, 2025
y0sher
approved these changes
Aug 7, 2025
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is meant to improve multi-client failover capabilities, previous implementation is using a simple heuristic of "client being synced" as the best way to predict if the client can serve API request or not (aka "active" client) + deactivating client under certain failure scenarios during API request execution, the issue is - that behavior doesn't cover some of the cases we've encountered in practice (eg. fetch-duties call was just timing out due to ~12s deadline we set, but at the same time we weren't trying to switch to another client presumably because the one we were trying reported it's synced).
Client being synced doesn't guarantee it's gonna serve API requests faithfully in a stable manner, hence a better heuristic is needed. A simple scoring system implemented in this PR (as seamlessly as possible on top of the previous sync-based approach to keep the code well-aligned with upstream) would do a better job at differentiating between good & bad clients, now failover works roughly like this:
endpoint1call has the same effect on client score as failingendpoint2call)Besides the added scoring mechanism this PR is also addressing the following edge-cases:
context.DeadlineExceedederror the previous implementation will simply keep trying that very same client every time without any failover, this is not ideal because this can very well happen regardless of what timeout(deadline) value the caller chooses due to variety of reasons: this particular client being overloaded at the moment, temporary network issue or crash while sync status hasn't updated yet, etc. - there is no reason not to try another client in that scenario4**type error response the previous implementation will simply keep trying that very same client every time without any failover - while the request itself is the most likely problem in this case, there is also a chance a bug in the client is the root cause for the request failing - so I'm not 100% we want to "trust" single client response (in SSV setting it would probably be better to fail over to another client just in case it's a client-bug, but I'm not changing this behavior for now)I think this change should also remove the need for implementing things like ssvlabs/ssv#2339 (@nkryuchkov do you think we should partially-revert stuff added in ssvlabs/ssv#2339 ?).
Note, this PR is complementary to attestantio#192 but instead of "trying to make sync reporting more reliable" it targets the failover algorithm itself to make it more generic & robust.