Improve governance voting logic #3120
Merged
Conversation
This file contains 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
grarco
reviewed
Apr 29, 2024
|
There's a failing unit test and I left a minor comment. Overall it looks good to me but I'd like a second opinion from @brentstone especially for the query and sdk files |
brentstone
force-pushed
the
fraccaman/improve-governance-voting-logic
branch
3 times, most recently
from
be235ac to
9d0a862
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3120 +/- ##
==========================================
+ Coverage 59.40% 59.46% +0.05%
==========================================
Files 298 298
Lines 92326 92332 +6
==========================================
+ Hits 54849 54904 +55
+ Misses 37477 37428 -49 ☔ View full report in Codecov by Sentry. |
|
@Fraccaman @grarco I fixed up the logic in here and also summarized the changes in the PR description. Lmk your thoughts |
brentstone
force-pushed
the
fraccaman/improve-governance-voting-logic
branch
from
d27cd42 to
48aba22
Compare
brentstone
approved these changes
May 2, 2024
brentstone
added a commit
that referenced
this pull request
May 2, 2024
* fraccaman/improve-governance-voting-logic: better documentation of proposal periods keep validator state longer in storage wasm tx: pass current delegations instead of those at proposal start clean up client code bug fix: ensure votes only to explicit active validators are counted revert find_delegation_validators -> no state should gov VP have any delegations logic? turn off wip added changelog
Fraccaman
commented
May 3, 2024
Comment on lines
+306
to
+363
| // Not sure, since ultimately whether the vote counts or not is | ||
| // determined by the validator state at the end epoch, which likely has | ||
| // not occurred yet during VP execution | ||
|
|
||
| // // Check the target validator of the bond used to vote | ||
| // let delegations_targets = find_delegation_validators( | ||
| // &self.ctx.pre(), | ||
| // voter, | ||
| // &pre_voting_end_epoch, | ||
| // ) | ||
| // .unwrap_or_default(); | ||
|
|
||
| // if delegations_targets.is_empty() { | ||
| // return Err(native_vp::Error::new_alloc(format!( | ||
| // "No delegations found for {voter}" | ||
| // )) | ||
| // .into()); | ||
| // } | ||
|
|
||
| // if !delegations_targets.contains(validator) { | ||
| // return Err(native_vp::Error::new_alloc(format!( | ||
| // "The vote key is not valid due to {voter} not having \ | ||
| // delegations to {validator}" | ||
| // )) | ||
| // .into()); | ||
| // } | ||
|
|
||
| // let validator_state = read_validator_state(&self.ctx.pre(), | ||
| // validator, &pre_voting_end_epoch)?; if !matches! | ||
| // (validator_state, ValidatorState::Consensus | | ||
| // ValidatorState::BelowCapacity | ValidatorState::BelowThreshold) { | ||
| // if validator_state.is_none() { | ||
| // return Err(native_vp::Error::new_alloc(format!( | ||
| // "The vote key is invalid because the validator | ||
| // {validator} is not in \ the active set (jailed | ||
| // or inactive)" )) | ||
| // .into()); | ||
| // } else { | ||
| // return Err(native_vp::Error::new_alloc(format!( | ||
| // "The vote key is invalid because the validator | ||
| // {validator} is not in \ the active set (jailed | ||
| // or inactive)" )) | ||
| // .into()); | ||
| // } | ||
|
|
||
| // } | ||
|
|
||
| // // this is safe as we check above that validator is part of the | ||
| // hashmap if !matches!( | ||
| // delegations_targets.get(validator).unwrap(), | ||
| // ValidatorState::Consensus | ||
| // ) { | ||
| // return Err(native_vp::Error::new_alloc(format!( | ||
| // "The vote key is not valid due to {validator} being not in \ | ||
| // the active set" | ||
| // )) | ||
| // .into()); | ||
| // } |
brentstone
added a commit
that referenced
this pull request
May 8, 2024
* origin/fraccaman/improve-governance-voting-logic: better documentation of proposal periods keep validator state longer in storage wasm tx: pass current delegations instead of those at proposal start clean up client code bug fix: ensure votes only to explicit active validators are counted revert find_delegation_validators -> no state should gov VP have any delegations logic? turn off wip added changelog
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.
Describe your changes
Closes #2523.
Summary of changes w.r.t the delegation validators and governance voting:
vote_proposal, which writes a vote key to storage for each. Does not filter based on the bond validator's stateis_valid_vote_keydoes not filter based on the bond validator's state.Almost no filtering is done in when a vote tx is submitted and vote keys are written into a storage for a proposal. This is because the state of a validator at the tally epoch, which is the voting end epoch, cannot necessarily be known at the epoch of vote submission. We should allow, for example, a delegator to submit a vote for a bond to an inactive validator, which may be active by the time the tally epoch comes around.
Ultimately in
compute_proposal_votes, a vote will only be counted if the validator is active at the end epoch - in consensus, below-capacity, or below-threshold. We rely on this step to do the proper filtering.Filtering is performed, however, by preventing a delegator with no delegations from submitting a valid vote tx and by preventing a validator that is currently jailed or inactive from submitting a valid vote tx.
Indicate on which release or other PRs this topic is based on
v0.34.0
Checklist before merging to
draft