-
Notifications
You must be signed in to change notification settings - Fork 302
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
staking: 🥞 get_validator_info()
uses consensus set
#4262
Conversation
❓ question for reviewersthere is a third to-do item mentioned in #3846, regarding a is that reasonable, or is the need to inquire about validator state acute enough that i should do that here? |
31b9672
to
2e829f4
Compare
2e829f4
to
254a78c
Compare
i've opened #4263. the changes to pcli ended up being a straightforward change. it is still kept distinct from the core changes here to facilitate review. |
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.
🥖 breadcrumbs for review.
crates/core/component/stake/src/component/validator_handler/validator_store.rs
Show resolved
Hide resolved
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.
Awesome
see #3846. > The staking component query service provides a ValidatorInfo RPC that > returns the list of registered validators, optionally filtering > Inactive validators. > > The way this method is implemented is by using an internal > ValidatorDataRead::validator_definitions methods which returns every > single definition ever submitted to the chain. This is problematic > because this list will inevitably grow quite large, turning usage of a > benign-looking method into an onerous source of I/O and expensive > rendering. this refactors the `validator_definitions` method to only return validators included within the consensus set. this will prevent the list returned from becoming too large over time.
these methods are tremendously useful for tests, because they save us the trouble of repeatedly hassling with a `Stream`. to discourage future use of them in production code however, we can define these in a test-specific extension. this commit introduces `ValidatorDataReadExt`. `validator_identity_keys` is also moved, as it is subject to the same performance considerations.
254a78c
to
35fca7c
Compare
rebased onto main at dc8eff9 ♻️ |
see #3846.
🔍 changes
NB: this branch performs a refactor away from
validator_definitions()
and subsequently moves the definition of that method. this is done in distinct commits, which reviewers are encouraged to examine independently.staking: 🥞
get_validator_info()
uses consensus setthis refactors the
validator_definitions()
method to only return validatorsincluded within the consensus set. this will prevent the list returned from
becoming too large over time.
app: 🧙
ValidatorDataReadExt::validator_definitions()
these methods are tremendously useful for tests, because they save us the
trouble of repeatedly hassling with a
Stream
. to discourage future use ofthem in production code however, we can define these in a test-specific
extension.
this commit introduces
ValidatorDataReadExt
.validator_identity_keys()
isalso moved, as it is subject to the same performance considerations.
checklist before requesting a review
if this code contains consensus-breaking changes, i have added the "consensus-breaking" label. otherwise, i declare my belief that there are not consensus-breaking changes, for the following reason: