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.

Sign up for GitHub

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

Jump to bottom

staking: 🎏 remove validator_definitions method and refactor rpc::validator_info to use consensus_set_stream #3846

Closed
3 tasks done
erwanor opened this issue Feb 19, 2024 · 5 comments
Closed
3 tasks done

staking: 🎏 remove validator_definitions method and refactor rpc::validator_info to use consensus_set_stream #3846

erwanor opened this issue Feb 19, 2024 · 5 comments
Assignees
@cratelyn
Labels
A-client Area: Design and implementation for client functionality A-staking Area: Design and implementation of staking and delegation _P-high High priority _P-V1 Priority: slated for V1 release
Milestone
Sprint 5

Comments

@erwanor
Copy link
Member

erwanor commented Feb 19, 2024 •
edited by cratelyn
Loading

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.

A better solution is to instead use ConsensusIndexRead and filter through validator definitions that are in the consensus set (i.e. validators that are in the active set, or eligible to be promoted into it).

On the other hand, this method is also used by validators to check that their submissions have been successfully recorded by the chain. To address this valid use case, we could create a pcli query validator status that Defined validator can use to inquire about their validator state.

☑️ to do:
@erwanor erwanor added A-staking Area: Design and implementation of staking and delegation A-client Area: Design and implementation for client functionality labels Feb 19, 2024
@github-project-automation github-project-automation bot moved this to 🗄️ Backlog in Penumbra Feb 19, 2024
@erwanor
Copy link
Member Author

erwanor commented Feb 19, 2024

We must make this change transparent to consumers of the service (web extension).

@erwanor erwanor mentioned this issue Feb 26, 2024
Closed
3 tasks
@hdevalence
Copy link
Member

We should not aim to preserve API compatibility, we should instead just make it do the right thing, restricting to the list of validators above the minimum limit.

@erwanor erwanor added _P-V1 Priority: slated for V1 release _P-high High priority labels Apr 8, 2024
@aubrika aubrika added this to the Sprint 4 milestone Apr 8, 2024
@aubrika aubrika moved this from Backlog to Todo in Penumbra Apr 8, 2024
@erwanor erwanor mentioned this issue Apr 8, 2024
Merged
1 task
@cratelyn
Copy link
Contributor

cratelyn commented Apr 9, 2024

following up on this, after it came up in #4173:

validator_definitions returning a vector of every definition is useful (and sometimes required) for test logic, and saves test cases the need to repeatedly deal with polling an asynchronous stream to get a &[Validator] or &[IdentityKey] to make assertions upon.

i don't have strong opinions about how we present this information via RPC, but i'd hope we can preserve the friendly ergonomics in tests!

@erwanor
Copy link
Member Author

erwanor commented Apr 9, 2024

We should assign this during sprint planning @cratelyn

@erwanor erwanor removed their assignment Apr 9, 2024
@cratelyn
Copy link
Contributor

cratelyn commented Apr 9, 2024

We should assign this during sprint planning @cratelyn

ah. it sounds like this shouldn't be included in this sprint! pardon the noise. 👍

@cratelyn cratelyn removed this from the Sprint 4 milestone Apr 9, 2024
@aubrika aubrika changed the title staking: remove validator_definitions method staking: remove validator_definitions method and refactor rpc::validator_info to use consensus_set_stream Apr 22, 2024
@cratelyn cratelyn self-assigned this Apr 22, 2024
@cratelyn cratelyn added this to the Sprint 5 milestone Apr 22, 2024
@cratelyn cratelyn changed the title staking: remove validator_definitions method and refactor rpc::validator_info to use consensus_set_stream staking: 🎏 remove validator_definitions method and refactor rpc::validator_info to use consensus_set_stream Apr 22, 2024
@cratelyn cratelyn moved this from Todo to In progress in Penumbra Apr 23, 2024
cratelyn added a commit that referenced this issue Apr 23, 2024
@cratelyn
staking: 🥞 get_validator_info() uses consensus set
c4398af 
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.
@cratelyn cratelyn mentioned this issue Apr 23, 2024
Merged
1 task
cratelyn added a commit that referenced this issue Apr 23, 2024
@cratelyn
staking: 🥞 get_validator_info() uses consensus set
693e273 
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.
cratelyn added a commit that referenced this issue Apr 23, 2024
@cratelyn
staking: 🥞 get_validator_info() uses consensus set
c13600f 
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.
cratelyn added a commit that referenced this issue Apr 24, 2024
@cratelyn
pcli: 😯 query validator definition only fetches one validator
caef469 
see #3846.

this uses the `get_validator_info` so that we only need to get a single
validator definition, without having to scan through the list of all
known validators.
cratelyn added a commit that referenced this issue Apr 24, 2024
@cratelyn
pcli: 🛻 query validator status fetches validator status
4b64879 
see #3846.

this builds upon the previous commit, and adds a `status` subcommand
that will print the status of a validator to stdout.
@cratelyn cratelyn mentioned this issue Apr 24, 2024
Merged
1 task
cratelyn added a commit that referenced this issue Apr 29, 2024
@cratelyn
staking: 🥞 get_validator_info() uses consensus set
066501a 
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.
cratelyn added a commit that referenced this issue Apr 29, 2024
@cratelyn
staking: 🥞 get_validator_info() uses consensus set
2ff80aa 
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.
cratelyn added a commit that referenced this issue Apr 29, 2024
@cratelyn
pcli: 😯 query validator definition only fetches one validator
ea234a0 
see #3846.

this uses the `get_validator_info` so that we only need to get a single
validator definition, without having to scan through the list of all
known validators.
cratelyn added a commit that referenced this issue Apr 29, 2024
@cratelyn
pcli: 🛻 query validator status fetches validator status
3cee16f 
see #3846.

this builds upon the previous commit, and adds a `status` subcommand
that will print the status of a validator to stdout.
cratelyn added a commit that referenced this issue Apr 29, 2024
@cratelyn
pcli: 🏓 query validator status prints a table
e86dea3 
this commit addresses @erwanor's review comment:
#4263 (comment)

> Frankly I would prefer that we do this in this pr, and finish this
> ticket. The cumulative voting share does not matter. The reason this
> command is important is that we have no way to observe validator
> identities that are not part of the main staking indexed since we
> merged #3846. My suggestion was about rendering the data in a table
> rather than pretty-printing a toml. The command is useful if we just
> render:
>
>     * validator info (name / IK)
>
>     * state
>
>     * validator exchange rate
>
>     * bonding state
>
>     * voting power
>
>
> The rest is bonus and completely auxiliary. What matters is that
> there's some way to observe validator state. What we should tackle in
> a separate issue is discoverability of validator identities.

this commit prints the validator information in a table, including:
* Voting Power
* Commission
* State
* Bonding State
* Exchange Rate
* Identity Key
* Name
cratelyn added a commit that referenced this issue Apr 29, 2024
@cratelyn
pcli: 😯 query validator definition only fetches one validator
5c96e60 
see #3846.

this uses the `get_validator_info` so that we only need to get a single
validator definition, without having to scan through the list of all
known validators.
cratelyn added a commit that referenced this issue Apr 29, 2024
@cratelyn
pcli: 🛻 query validator status fetches validator status
1595489 
see #3846.

this builds upon the previous commit, and adds a `status` subcommand
that will print the status of a validator to stdout.
cratelyn added a commit that referenced this issue Apr 29, 2024
@cratelyn
pcli: 🏓 query validator status prints a table
1b6c612 
this commit addresses @erwanor's review comment:
#4263 (comment)

> Frankly I would prefer that we do this in this pr, and finish this
> ticket. The cumulative voting share does not matter. The reason this
> command is important is that we have no way to observe validator
> identities that are not part of the main staking indexed since we
> merged #3846. My suggestion was about rendering the data in a table
> rather than pretty-printing a toml. The command is useful if we just
> render:
>
>     * validator info (name / IK)
>
>     * state
>
>     * validator exchange rate
>
>     * bonding state
>
>     * voting power
>
>
> The rest is bonus and completely auxiliary. What matters is that
> there's some way to observe validator state. What we should tackle in
> a separate issue is discoverability of validator identities.

this commit prints the validator information in a table, including:
* Voting Power
* Commission
* State
* Bonding State
* Exchange Rate
* Identity Key
* Name
cratelyn added a commit that referenced this issue Apr 29, 2024
@cratelyn
pcli: 😯 query validator definition only fetches one validator
fd690d3 
see #3846.

this uses the `get_validator_info` so that we only need to get a single
validator definition, without having to scan through the list of all
known validators.
cratelyn added a commit that referenced this issue Apr 29, 2024
@cratelyn
pcli: 🛻 query validator status fetches validator status
f7539e8 
see #3846.

this builds upon the previous commit, and adds a `status` subcommand
that will print the status of a validator to stdout.
cratelyn added a commit that referenced this issue Apr 29, 2024
@cratelyn
pcli: 🏓 query validator status prints a table
c84338f 
this commit addresses @erwanor's review comment:
#4263 (comment)

> Frankly I would prefer that we do this in this pr, and finish this
> ticket. The cumulative voting share does not matter. The reason this
> command is important is that we have no way to observe validator
> identities that are not part of the main staking indexed since we
> merged #3846. My suggestion was about rendering the data in a table
> rather than pretty-printing a toml. The command is useful if we just
> render:
>
>     * validator info (name / IK)
>
>     * state
>
>     * validator exchange rate
>
>     * bonding state
>
>     * voting power
>
>
> The rest is bonus and completely auxiliary. What matters is that
> there's some way to observe validator state. What we should tackle in
> a separate issue is discoverability of validator identities.

this commit prints the validator information in a table, including:
* Voting Power
* Commission
* State
* Bonding State
* Exchange Rate
* Identity Key
* Name
@erwanor erwanor closed this as completed Apr 29, 2024
@github-project-automation github-project-automation bot moved this from In progress to Done in Penumbra Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cratelyn cratelyn

Labels
A-client Area: Design and implementation for client functionality A-staking Area: Design and implementation of staking and delegation _P-high High priority _P-V1 Priority: slated for V1 release
Projects
Penumbra
Archived in project
Milestone
Sprint 5
Development

No branches or pull requests
4 participants
@hdevalence @erwanor @cratelyn @aubrika