Re-enable voting power check in tally votes#409
Merged
Conversation
avalkov
suggested changes
Jul 29, 2025
Member
|
Lets also uncomment and fix the |
239bd77 to
3d3f132
Compare
avalkov
reviewed
Aug 1, 2025
avalkov
reviewed
Aug 1, 2025
avalkov
reviewed
Aug 1, 2025
avalkov
reviewed
Aug 1, 2025
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR re-enables voting power validation in the tallyVotes function by implementing proper validator set tracking to handle CometBFT's 2-block delay for validator set changes. The fix addresses a bug where voting power validation was disabled due to validator set synchronization issues.
Key changes:
- Adds tracking of the penultimate (2 blocks ago) validator set to handle CometBFT's validator set change propagation delay
- Re-enables voting power checks in
tallyVoteswith configurable height-based logic - Updates vote extension validation to use the appropriate validator set based on block height
- Introduces height-based flags to control when validator checks should be enabled/disabled
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| x/stake/types/keys.go | Adds key for storing penultimate block validator set |
| x/stake/keeper/validator.go | Implements penultimate validator set storage and retrieval functions |
| x/stake/keeper/abci.go | Updates EndBlocker to use SDK context for validator set updates |
| x/milestone/abci/abci.go | Adds context parameter and conditional validator validation logic |
| x/milestone/abci/abci_test.go | Updates test to provide required context parameter |
| x/chainmanager/types/keys.go | Adds key for storing initial chain height |
| x/chainmanager/keeper/keeper.go | Implements initial chain height storage and retrieval |
| x/chainmanager/keeper/genesis.go | Sets initial chain height during genesis |
| helper/config.go | Adds configuration for tally fix and validator check heights |
| bridge/broadcaster/broadcaster_test.go | Sets chain configuration for test |
| app/vote_ext_utils_test.go | Updates tests to use validator set instead of keeper |
| app/vote_ext_utils.go | Refactors to use validator set parameter and re-enables voting power checks |
| app/abci_test.go | Sets initial chain height for tests |
| app/abci.go | Implements validator set selection logic based on block height |
jmalicevic
reviewed
Aug 4, 2025
sergio-mena
reviewed
Aug 4, 2025
sergio-mena
reviewed
Aug 4, 2025
sergio-mena
reviewed
Aug 4, 2025
sergio-mena
reviewed
Aug 4, 2025
marcello33
reviewed
Aug 5, 2025
sergio-mena
reviewed
Aug 6, 2025
sergio-mena
reviewed
Aug 6, 2025
sergio-mena
reviewed
Aug 6, 2025
sergio-mena
reviewed
Aug 6, 2025
sergio-mena
reviewed
Aug 6, 2025
sergio-mena
reviewed
Aug 6, 2025
sergio-mena
reviewed
Aug 6, 2025
sergio-mena
reviewed
Aug 8, 2025
marcello33
approved these changes
Aug 8, 2025
avalkov
approved these changes
Aug 8, 2025
kamuikatsurgi
approved these changes
Aug 8, 2025
|
Member
|
Squash merge please. |
marcello33
added a commit
that referenced
this pull request
Sep 2, 2025
* Delete spans backfill * Always vote NO on spans backfill * Return type assertion for MsgBackfillSpan * Merge pull request #413 from 0xPolygon/kamui/bump-kurtosis chore: remove validator test case and bump kurtosis * chore: side msg and abci handler metrics (#410) * chore: add side msg metrics * timer metrics for Pre, Begin, and End blocker funcs * chore: nits * abci handler metrics * chore: use Namespace from metrics package * misc: migrated from maticnetwork to 0xPolygon (#420) * misc: migrated from maticnetwork to 0xPolygon * misc: bump cosmos to v0.2.3-polygon * misc: bump bor * misc: addressed comments * chore: bump kurtosis (#422) * chore: bump kurtosis * chore: add milestones test * fix: test case name * chore: update test_runner image * Re-enable voting power check in tally votes (#409) * app,helper,x/stake: add logic to store penultimate valset * app: use correct valset in ValidateVoteExtensions * app: fix if condition * app: add condition for genesis * app,x/chainmanager: track initial chain height for UTs * app,helper,x/milestone: re-enable skipped validator not found errors * app: fix some tests and refactor a bit * app,bridge: fix more tests * app,x/milestone: some cleanup * app: dedup redundant stubs * app: more cleanup * app,helper,x/chainmanager: move setting of initial height outside of x/chainmanager * app: improved error logging * app,x/chainmanager: rm unused key + minor nit * app: deterministically extract max hash and VP from non-rp VE * x/stake: fix lint * helper: refactoring * app: add comment * bump go to 1.24.6 * helper: rm unnecessary todo * fix: build (#423) * Fix generate-keystore command (#424) * cmd: allow generate-keystore to accept private key * cmd: add --generate-new flag * update: cosmos-sdk (#425) * helper: fix conflicts * feat: bump kurtosis and migrate to pos-workflows (#431) * feat: remove matic-cli e2e-tests (#432) * bridge: improvements / app: improvements (#427) * improve bridge, remove bridge cli, minor fixes * validate bor_chain_id in checkpoints flow * test fee-transfer events emissions * remove test block * logs on rest server being unresponsive * improve initRootCmd * don't return on ctx done / restServerTimeOutInMinutes to 1m for tests * improve logs / restore restServerTimeOutInMinutes * sort imports * fix comment * fix tests and linter issues * bump cosmos-sdk dep * move const outside of the func * refactor StartWithCtx function * address comments * log anomaly if account not found with node being in sync * Set voting power and valset check heights for amoy (#435) * helper: set tallyFixHeight and disableVPCheckHeight for amoy * helper: set disableValSetCheckHeight for amoy * helper: set initialHeight for amoy * api,proto,x/stake: add penultimate valset to genesis export + add some tests (#436) * set tallyfix mainnet hf heights --------- Co-authored-by: Angel Valkov <avalkov@polygon.technology> Co-authored-by: Krishang Shah <109511742+kamuikatsurgi@users.noreply.github.com> Co-authored-by: Pratik Patil <pratikspatil024@gmail.com> Co-authored-by: Raneet Debnath <35629432+Raneet10@users.noreply.github.com> Co-authored-by: Raneet Debnath <raneetdebnath10@gmail.com>
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.
Description
This PR re-enables the clause to verify the voting power coming from the vote extensions doesn't exceed the total power of the validator set (stored in stake module) in
tallyVotes. Since CometBFT takes 2 heights for validator set changes to take effect, heimdall needs to keep track of the penultimate valset to tally correctly.Changes
Checklist
Testing