-
Notifications
You must be signed in to change notification settings - Fork 784
Cache validator sets retrieved by client #4170
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
base: master
Are you sure you want to change the base?
Conversation
78aad7c
to
ac137d6
Compare
3d2946e
to
5e4bf8a
Compare
return validatorSets, nil | ||
} | ||
|
||
func (c *CachedState) GetValidatorSet( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case that the State
is a wrapped state like tracedState
, this doesn't actually call the underlying GetValidatorSet
method - will that cause any problems for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tracedState
is a wrapper that needs access to a different state implementation which would presumably make the relevant calls itself at a level no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the case you wrap CachedState -> TracedState -> State
, if you call GetValidatorSet
the trace would be tagged with GetAllValidatorSets
because that would be what you'd be calling under the hood.
If you wrapped TracedState -> CachedState -> State
then the trace would be tagged (correctly?) with GetValidatorSet
Don't think it's a big deal either way though
chains/manager.go
Outdated
@@ -1115,6 +1115,9 @@ func (m *manager) createSnowmanChain( | |||
return nil, fmt.Errorf("expected validators.State but got %T", vm) | |||
} | |||
|
|||
// Wrap the validator state with a cached state | |||
m.validatorState = validators.NewCachedState(m.validatorState) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be valState
or else it just gets overridden below it seems.
Why this should be merged
Closes #4239 (comment)
How this works
Creates a new grpc method
GetAllValidatorSets
that returns all validators for all subnets.Creates a caching wrapper for validator
State
that overridesGetValidatorSet
to check the cache for the validator set. If it is missing, then callGetAllValidatorSets
and cache all the results.TODO check grpc max message sizeUsing current three fields returned the size of the serialized message remains under the default 4MB limit with the maximum 20k L1 validators each validating a separate subnet and an additional 20k primary network validators. Additionally we override/disable the maximum gRPC message size when dialing already.
How this was tested
Need to be documented in RELEASES.md?