fix(gov): bound EndBlock vote tally work - #4000
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4000 +/- ##
==========================================
- Coverage 61.34% 58.46% -2.89%
==========================================
Files 2163 2211 +48
Lines 188757 188803 +46
==========================================
- Hits 115792 110378 -5414
- Misses 62256 68463 +6207
+ Partials 10709 9962 -747
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview App / ABCI: Registers Upgrade & state: Adds EVM RPC: Historical trace/simulation replays run governance Reviewed by Cursor Bugbot for commit 88ad05c. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7c3f7c7. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Spreading the gov tally across blocks is the right fix for the unbounded EndBlock work, but freezing the validator/bonded-token snapshot at tally start while still reading delegations live in later blocks makes the tally arithmetic unsound (deductions can exceed snapshot shares, yielding negative validator power). Several secondary gaps — archived votes invisible to vote queries, tally progress dropped on genesis export, and an unbounded (now heavier) Tally on the public query path — should also be addressed.
Findings: 1 blocking | 7 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] Cleanup only receives the budget left over after tallying (
CleanupTallyVotes(ctx, remainingVotes)inabci.go). While a large tally repeatedly consumes the full 1,000-record budget, archived rounds from earlier proposals are never reclaimed and the0x31archive grows. Consider reserving a slice of the budget for cleanup, or draining cleanup before tallying, so archive removal cannot be starved indefinitely. - [suggestion] Test gap: there is no multi-block regression test for the expedited→regular conversion path (an expedited tally that spans several blocks, converts, then re-tallies with both round archives live), nor for a delegation change occurring between two blocks of the same tally. Both are the cases the new persisted-snapshot design actually changes; the current tests only exercise single-round, single-block-delta scenarios.
- 4 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
grpc_query.go:275/querier.go:177compute a provisional tally for a voting-period proposal by callingTally, which iterates every vote record. This is already an unbounded, publicly reachable query independent of this PR.
|
Addressed the remaining review-summary items in d67cd15:
Validation completed:
|
|
@seidroid review |
There was a problem hiding this comment.
Spreading the gov tally across blocks with a persisted accumulator, round-scoped vote archives, a merged read view, and a shared cleanup budget is a solid structure, and the previous round's findings on vote visibility, query mutation, genesis round-trip, cleanup starvation, and spec layout are genuinely addressed. The remaining blocker is the one Codex also flags: mixing a frozen validator snapshot with live delegation reads is still unsound — the greedy per-voter clamp bounds each validator's total but makes a legitimate voter's counted power depend on other delegators' post-deadline stake movements and on vote-key ordering, and can silently drop their vote entirely.
Findings: 1 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
tallyProgressis consensus-critical state but is serialized withencoding/json, while every other record inx/govgoes throughkeeper.cdc. The app hash now depends on Go's JSON encoder forsdk.Dec/sdk.Int/proto structs, and the simulation decoder can only hex-dump the blob. A proto message would keep the module consistent and give the store diff a readable decoder. - [suggestion] The whole progress blob is re-encoded and re-written to IAVL on every block a tally advances, including the immutable part of the snapshot (validator address, bonded tokens, snapshot shares — roughly 200 bytes per bonded validator). For a long spam-induced tally that is the full validator set written per block. Consider splitting the immutable snapshot from the mutable accumulator, or keying deductions per validator, so only what changed is rewritten.
- [suggestion]
if !complete { return true }inEndBlockerstopsIterateActiveProposalsQueueentirely, so a single vote-spammed proposal defers finalization of every other expired proposal until it drains at ~900 records/block. Work per block is correctly bounded, but head-of-line blocking across proposals is a new liveness property worth stating in the spec (or working around by continuing to the next proposal when the leader is out of budget). - [suggestion] While a tally is in progress
AddVotereturnsErrInactiveProposal, butQueryProposalstill reportsPROPOSAL_STATUS_VOTING_PERIOD, so a client cannot distinguish "voting closed, tallying" from a genuine status mismatch. Consider a distinct error or exposingIsTallyingon the proposal query. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
grpc_query.go:274andquerier.go:177compute a provisional tally for a voting-period proposal by callingTally, which iterates every vote record with no bound. This is a publicly reachable unbounded query independent of this PR (and this PR makes it cheaper than the previous revision by removing the writes).
|
@seidroid review |
There was a problem hiding this comment.
The incremental tally structure — persisted accumulator, round-scoped archives, merged read view, shared cleanup budget — is sound and the budget arithmetic checks out against the new tests. The remaining blocker, which Codex also flags, is that mixing a frozen validator snapshot with live delegation reads still lets post-deadline stake movement change a proposal's outcome; the PR's own TestTallyIncrementalScalesDelegationsAddedAfterSnapshot asserts a case where an account with zero stake at the deadline captures 80% of a validator's voting power.
Findings: 1 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
tallyProgressis consensus-critical state but is serialized withencoding/json(tally.go:334-340), while every other record inx/govgoes throughkeeper.cdc. The app hash now depends on Go's JSON encoder forsdk.Dec/sdk.Int/proto structs, andsimulation/decoder.gocan only hex-dump the blob. A proto message would keep the module consistent and give the store diff a readable decoder. (Raised in the previous round; still unaddressed.) - [suggestion] The whole progress blob — including the immutable snapshot (validator address, bonded tokens, delegator shares, ~200 bytes per bonded validator) — is re-marshalled and re-written to IAVL on every block a tally advances. For the long spam-induced tally this PR is designed to absorb, that is the full validator set written per block, which partly undoes the bounding. Consider splitting the immutable snapshot from the mutable accumulator. (Raised in the previous round; still unaddressed.)
- [suggestion] While a tally is in progress
AddVotereturnsErrInactiveProposal(vote.go:23) butQueryProposalstill reportsPROPOSAL_STATUS_VOTING_PERIOD, so a client cannot distinguish "voting closed, tallying" from a genuine status mismatch. A distinct error or exposingIsTallyingon the proposal query would make this diagnosable. (Raised in the previous round; still unaddressed.) - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
keeper/grpc_query.go:274andkeeper/querier.go:177compute a provisional tally for a voting-period proposal by callingTally, which iterates every vote record with no bound. This is a publicly reachable unbounded query independent of this PR (which does make it cheaper than before by removing the writes).
|
@seidroid review |
There was a problem hiding this comment.
The incremental tally machinery (persisted accumulator, round-scoped archives, merged read view, shared cleanup budget) is well-structured and the per-block budget arithmetic checks out against the new tests. The remaining blocker — which Codex also raises — is that voting power is now frozen when the vote is cast rather than at the voting deadline, which lets stake moved before voting closes capture (and nullify) a validator's snapshotted voting power.
Findings: 1 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
tallyProgressis consensus-critical state but is serialized withencoding/json(keeper/tally.go:356-368), while every other record inx/govgoes throughkeeper.cdc. The app hash now depends on Go's JSON encoder forsdk.Dec/sdk.Int/proto structs, andsimulation/decoder.gocan only hex-dump the blob. A proto message would keep the module consistent and give the store diff a readable decoder. (Raised in the two previous rounds; still unaddressed.) - [suggestion] The whole progress blob — including the immutable part of the snapshot (validator address, bonded tokens, delegator shares, ~200 bytes per bonded validator) — is re-marshalled and re-written to IAVL on every block a tally advances (
keeper/tally.go:70). For the long spam-induced tally this PR is designed to absorb, that is the full validator set written per block, which partly undoes the bounding. Consider splitting the immutable snapshot from the mutable accumulator. (Raised in the two previous rounds; still unaddressed.) - [suggestion] While a tally is in progress
AddVotereturnsErrInactiveProposal(keeper/vote.go:25) butQueryProposalstill reportsPROPOSAL_STATUS_VOTING_PERIOD, so a client cannot distinguish "voting closed, tallying" from a genuine status mismatch. A distinct error or exposingIsTallyingon the proposal query would make this diagnosable. (Raised in the two previous rounds; still unaddressed.) - [suggestion] Ownership of the cleanup cursor key is split across two functions:
cleanupProposalTallyVotesdeletesTallyCleanupKeyitself when it finishes (keeper/tally.go:408-410), and its only caller deletes or rewrites the same key immediately afterwards (keeper/tally.go:120-125). One of the two should own the key so a future change cannot leave the cursor and the archive out of step. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
keeper/grpc_query.go:274andkeeper/querier.go:177compute a provisional tally for a voting-period proposal by callingTally, which iterates every vote record with no bound. This is a publicly reachable unbounded query independent of this PR (which does make it cheaper than before by removing the writes it used to perform).
|
@seidroid review |
There was a problem hiding this comment.
The staking-hook approach genuinely closes the previous round's vote-time/tally-time snapshot mismatch, and the head-of-line and key-encoding threads are addressed. One blocker remains: votes recorded before this binary ships have no delegation snapshot and no VoterProposals index entry, so processTallyVotes falls back to a live delegation read against a frozen validator snapshot — reintroducing the exact power-capture attack for any proposal whose voting period spans the upgrade.
Findings: 1 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
tallyProgressis consensus-critical state but is serialized withencoding/json(keeper/tally.go:357,364), while every other record inx/govgoes throughkeeper.cdc. The app hash now depends on Go's JSON encoder forsdk.Dec/sdk.Int/proto structs, andsimulation/decoder.gocan only hex-dump the blob. A proto message would keep the module consistent and give the store diff a readable decoder. (Raised in the three previous rounds; still unaddressed. Codex raises it too.) - [suggestion] The whole progress blob — including the immutable snapshot (validator address, bonded tokens, delegator shares, ~200 bytes per bonded validator) — is re-marshalled and re-written to IAVL on every block a tally advances (
keeper/tally.go:70). For the long spam-induced tally this PR is designed to absorb, that is the full validator set written per block, which partly undoes the bounding. Consider splitting the immutable snapshot from the mutable accumulator. (Raised in the three previous rounds; still unaddressed. Codex raises it too.) - [suggestion] While a tally is in progress
AddVotereturnsErrInactiveProposal(keeper/vote.go:25) butQueryProposalstill reportsPROPOSAL_STATUS_VOTING_PERIOD, so a client cannot distinguish "voting closed, tallying" from a genuine status mismatch. A distinct error or exposingIsTallyingon the proposal query would make this diagnosable. (Raised in the three previous rounds; still unaddressed. Codex raises it too.) - [suggestion] Ownership of the cleanup cursor key is still split across two functions:
cleanupProposalTallyVotesdeletesTallyCleanupKeyitself when it finishes (keeper/tally.go:409-411), and its only caller deletes or rewrites the same key immediately afterwards (keeper/tally.go:120-125). One of the two should own the key so a future change cannot leave the cursor and the archive out of step. (Raised in the previous round.) - [suggestion]
simulation/decoder.go:53-56hex-dumpsVoteDelegationsKeyPrefixandTallyVoteDelegationsKeyPrefixvalues, but unliketallyProgressthese are protoVoteDelegationSnapshotrecords thatcdc.MustUnmarshalcan decode. Decoding them would make store diffs for this app-hash-breaking change readable at no cost. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
keeper/grpc_query.go:274andkeeper/querier.go:177compute a provisional tally for a voting-period proposal by callingTally, which iterates every vote record with no bound. This is a publicly reachable unbounded query independent of this PR (which does make it cheaper than before by removing the writes it used to perform).
|
@seidroid review |
There was a problem hiding this comment.
The vote-delegation snapshot design now closes the snapshot-consistency hole the last four rounds blocked on: snapshots are maintained at the staking-hook choke point, frozen at tally start, archived with the vote, and backfilled for pre-upgrade votes, with the live-read fallbacks removed. The one remaining blocker is that the new Migrate3to4 backfill is itself unbounded over the same attacker-stageable vote set this PR exists to bound, concentrating that work in the upgrade block.
Findings: 1 blocking | 7 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- None at the file/PR level.
- 6 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
keeper/grpc_query.go:274andkeeper/querier.go:177compute a provisional tally for a voting-period proposal by callingTally, which iterates every vote record with no bound. This is a publicly reachable unbounded query independent of this PR (which does make it cheaper than before by removing the writes it used to perform).
|
@seidroid review |
|
@seidroid review |
|
@seidrod review |
|
@seidroid review |
There was a problem hiding this comment.
The redesign around frozen "tally boundaries" (gap/exact electorate captures at BeginBlock/EndBlock, vote-delegation snapshots maintained at the staking-hook choke point and deferred for the slash path, plus an O(1) Migrate3to4 with a bounded per-proposal backfill) genuinely closes the snapshot-consistency and unbounded-migration blockers from the previous five rounds — voter shares and the validator electorate are now frozen from the same state, so ObservedDelegatorShares <= DelegatorShares holds by construction. What remains are the long-standing non-blocking items (JSON encoding of consensus state, whole-blob progress rewrites, cleanup-cursor ownership, tallying-status observability, simulation decoder) plus a new liveness coupling between the global delegation-update queue and every proposal's tally.
Findings: 0 blocking | 8 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 7 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
keeper/grpc_query.go(QueryTallyResult) andkeeper/querier.gocompute a provisional tally for a voting-period proposal by callingTally, which iterates every vote record with no bound. This is a publicly reachable unbounded query independent of this PR; this PR leaves it read-only (no writes) but adds a full bonded-validator electorate snapshot to the same path when no tally progress exists yet.
Superseded: latest AI review found no blocking issues.
| } | ||
|
|
||
| func removeFutureGovernanceActivation(ctx sdk.Context, a *app.App, version string) { | ||
| if semver.Compare(version, "v6.7") < 0 { |
There was a problem hiding this comment.
We are planning to ship this in 6.8 so this needs to change unless I have missed something.
|
:seidroid review |
|
@seidroid review |
There was a problem hiding this comment.
The latest revision splits the tally accumulator out of the progress blob into per-validator proto records, moves tally progress/boundaries/deferred updates onto keeper.cdc, gives CleanupTallyVotes sole ownership of the cleanup cursor, and documents the global update-queue ordering — all of which resolve the prior round's inline findings, and I found no new correctness problems in the accumulator split (load/flush/cleanup are consistent across single-block, multi-block, and expedited→regular rounds). Three non-blocking items remain: two carried over from the previous review (tallying state is not observable on queries, and the genesis electorate panic message covers three distinct failures) plus a nil-Dec panic in the new genesis validation.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The per-validator accumulator work is not counted against
MaxVotesProcessedPerBlock:flushTallyValidatorAccumulatorswrites one record per touched validator on every non-final block, andfinishTallyreads one record per bonded validator on the completing block (keeper/tally.go:518-547,559-570). It is bounded by the validator set so it does not undo the guarantee, but since the whole point of the budget is that EndBlock work is accounted for, it is worth either folding these into the record count or noting the extra per-block term inspec/02_state.md. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
- 2 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
keeper/grpc_query.go(QueryTallyResult) andkeeper/querier.gocompute a provisional tally for a voting-period proposal by callingTally, which iterates every vote record with no bound. This is a publicly reachable unbounded query independent of this PR; this PR leaves it write-free but adds a bonded-validator electorate snapshot to the same path when no tally progress exists yet. - [suggestion]
ValidateGenesis->validateTallyParams(x/gov/types/params.go:154-200) callsIsNegative/IsPositive/GTonTallyParamsdecimals without a nil check, so a genesis file omitting e.g.quorumpanics rather than returning a validation error. Pre-existing on the base branch; the new electorate validation reuses the same function.
| } | ||
| for _, electorate := range data.TallyElectorates { | ||
| proposal, found := proposalsByID[electorate.ProposalId] | ||
| if !found || proposal.Status != types.StatusVotingPeriod || proposal.VotingEndTime.After(ctx.BlockTime()) { |
There was a problem hiding this comment.
[suggestion] Still a single panic message covering three distinct failures — the proposal is missing, it is not in the voting period, or its voting end time is still in the future — but only the third is described, so two of the three produce a misleading halt during genesis import. (Codex raises this too.)
The third condition is also the one an operator is most likely to hit accidentally: ValidateGenesis cannot check it (validateTallyElectorates has no block time), so a state export replayed with a genesis_time earlier than an exported electorate's proposal VotingEndTime panics the node here with text pointing at the wrong thing. Splitting the conditions into distinct messages — and deciding deliberately whether the future-end-time case should be a validation error rather than a panic — would make a failed restart diagnosable.
| if proposal.VotingEndTime.Before(ctx.BlockTime()) { | ||
| return false, sdkerrors.Wrapf(types.ErrInactiveProposal, "%d", proposalID) | ||
| } | ||
| if keeper.voteDelegationSnapshotFrozen(ctx, proposal) || keeper.IsVoteDelegationBackfillInProgress(ctx, proposalID) { |
There was a problem hiding this comment.
[suggestion] While the electorate is frozen (or a backfill is running) AddVote returns ErrInactiveProposal, but QueryProposal still reports PROPOSAL_STATUS_VOTING_PERIOD, so a client cannot distinguish "voting closed, tallying" from a genuine status mismatch — and with a multi-block tally plus a multi-block legacy backfill this window is observable for many blocks. AddDeposit (deposit.go:119-123) has the same behaviour, and nothing in grpc_query.go exposes the tallying/backfill state. A distinct error or a tallying flag on the proposal query would make it diagnosable. (Raised in the previous five rounds; Codex raises it too.)
| if _, err := sdk.ValAddressFromBech32(delegation.Validator); err != nil { | ||
| return fmt.Errorf("invalid vote delegation snapshot validator %q: %w", delegation.Validator, err) | ||
| } | ||
| if !delegation.Shares.IsPositive() { |
There was a problem hiding this comment.
[suggestion] delegation.Shares is a non-nullable customtype Dec, so a genesis file whose vote_delegation_snapshots[].delegations[] entry omits shares leaves it as the zero Dec with a nil *big.Int, and IsPositive() (sdk.Dec.IsPositive is (d.i).Sign() == 1) nil-derefs — validate-genesis panics instead of reporting the bad field.
The surrounding new validation already guards this: validateTallyElectorates checks BondedTokens.IsNil() and DelegatorShares.IsNil() before comparing (lines 263-274). Adding the same if delegation.Shares.IsNil() guard here keeps the new validators consistent.
2 participants
Describe your changes and provide context
Governance proposal finalization previously iterated and deleted every stored vote in one
EndBlock, allowing a proposal with many zero-power voters to concentrate unbounded work in its expiry block.This change:
EndBlockProposal execution, deposit handling, hooks, and final events remain deferred until every vote has been processed.
Testing performed to validate your change
go test ./sei-cosmos/x/gov/... -count=1go test -race ./sei-cosmos/x/gov ./sei-cosmos/x/gov/keeper -count=1go vet ./sei-cosmos/x/gov/...make fmtcheckwith golangci-lint v2.8.0 rebuilt for Go 1.25./sei-cosmos/x/gov/...— 0 issues