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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4000 +/- ##
==========================================
- Coverage 61.34% 57.98% -3.37%
==========================================
Files 2163 2206 +43
Lines 188757 184972 -3785
==========================================
- Hits 115792 107250 -8542
- Misses 62256 67906 +5650
+ Partials 10709 9816 -893
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 Incremental tally persists validator snapshots, tally parameters, results, and a vote cursor across blocks; processed votes move into round-specific archives (expedited vs regular). New votes are rejected after voting ends, during backfill, or while tallying. Tallying uses frozen per-voter delegation snapshots (maintained at vote time and refreshed via staking hooks until tally starts), with genesis consensus version 4, new store keys, Migrate3to4 backfill for pre-upgrade votes, and export/import of snapshots for unfinished tallies. Reviewed by Cursor Bugbot for commit c537ff0. 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).
| iterator := sdk.KVStorePrefixIterator(store, types.VotesKeyPrefix) | ||
| defer func() { _ = iterator.Close() }() | ||
|
|
||
| for ; iterator.Valid(); iterator.Next() { |
There was a problem hiding this comment.
[blocker] This migration is unbounded over exactly the state this PR exists to bound, and it runs in a single block at a height that is public well in advance.
Migrate3to4 iterates every record under VotesKeyPrefix and, per vote, calls initializeVoteDelegationTracking → snapshotVoteDelegations → sk.IterateDelegations plus two store.Sets. Votes only survive for proposals still in their voting period, and the PR's own threat model is that a proposal can accumulate an arbitrary number of cheap zero-power votes.
Failure scenario: upgrade height H is announced. Before H an attacker submits a proposal (or votes on an existing one) with N zero-power voters, the same spam this PR bounds to 1,000 records/block in EndBlocker. At H, RunMigrations executes in x/upgrade's BeginBlocker and does O(N) unmetered store work with no budget and no cursor — for large N the upgrade block never finishes in a reasonable time and the chain stalls at the upgrade rather than at proposal expiry. The rest of the PR caps that same N at 1,000 records/block; this path reintroduces the whole set in one block.
Options: give the backfill a per-block budget with a persisted cursor (the same shape as cleanupProposalTallyVotes, gated so tallying cannot start for a proposal whose backfill is incomplete), or bound it by only backfilling votes on proposals whose voting period is still open and refusing to start a tally on a proposal that still has unbackfilled votes. Either way the migration needs a bound, since its input size is attacker-chosen.
There was a problem hiding this comment.
Fixed in c537ff0.
Migrate3to4 is now O(1): it records the first post-upgrade proposal ID and performs no vote or delegation iteration. Legacy proposals backfill delegation snapshots and voter indexes under a persisted per-proposal cursor when tallying reaches them. Every scanned vote consumes the same MaxVotesProcessedPerBlock budget shared with tally and archive cleanup, and tally progress is not initialized until that backfill completes.
The backfill cursor also locks further votes once scanning starts, while AddVote now rejects strictly after VotingEndTime so queued expired proposals cannot accumulate more work while an earlier proposal consumes the budget. Votes at the exact expiry block remain valid.
The new EndBlock regression covers 1,001 legacy votes plus an existing cleanup backlog and verifies the 1,000-record cap across cleanup, backfill, and tally over multiple blocks. It also covers restartable cursor behavior, post-upgrade proposals, delegation changes between batches, and read-only tally/export during backfill.
| } | ||
|
|
||
| snapshot := keeper.snapshotVoteDelegationsExcept(ctx, 0, voter, excludedValidator) | ||
| for ; iterator.Valid(); iterator.Next() { |
There was a problem hiding this comment.
[suggestion] Repeating the previous round's finding (Codex raises it too): every delegation change fans out over all active proposals the delegator voted on and rewrites a full snapshot for each, at O(#delegations + #votedActiveProposals) store writes per hook call.
On MsgDelegate/MsgUndelegate/MsgBeginRedelegate this is gas-metered and self-inflicted. It also fires from the slashing path — SlashRedelegation/SlashUnbondingDelegation call Unbond, which triggers AfterDelegationModified/BeforeDelegationRemoved for each affected delegator — and that runs in Begin/EndBlock unmetered. Distribution's hook already makes that path per-delegator work, so this multiplies existing unbounded work rather than introducing a new class, which is why it stays a suggestion; but it does erode the per-block guarantee this PR establishes.
Bounding the fan-out (cap the number of active proposals indexed per address, or store one shared versioned snapshot per voter instead of one copy per proposal) would keep that guarantee intact.
Smaller point on the same function: when every indexed proposal is tallying, the loop still pays for a full snapshotVoteDelegationsExcept before skipping them all — the snapshot could be computed lazily on the first non-tallying proposal.
|
|
||
| complete, processed = keeper.processTallyVotes(ctx, proposal.ProposalId, &progress, maxVotes) | ||
| if !complete { | ||
| keeper.setTallyProgress(ctx, proposal.ProposalId, progress) |
There was a problem hiding this comment.
[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. 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. Splitting the immutable snapshot from the mutable accumulator (or keying per-validator deductions separately) would rewrite only what changed. (Raised in the previous three rounds; Codex raises it too.)
| } | ||
|
|
||
| func (keeper Keeper) setTallyProgress(ctx sdk.Context, proposalID uint64, progress tallyProgress) { | ||
| bz, err := json.Marshal(progress) |
There was a problem hiding this comment.
[suggestion] tallyProgress is consensus-critical state but is serialized with encoding/json, while every other record in x/gov goes through keeper.cdc. The app hash now depends on Go's JSON encoder for sdk.Dec/sdk.Int/proto structs, and simulation/decoder.go can 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 three rounds; still unaddressed.)
|
|
||
| complete = !iterator.Valid() | ||
| if complete { | ||
| store.Delete(types.TallyCleanupKey(proposalID, expedited)) |
There was a problem hiding this comment.
[suggestion] Ownership of the cleanup cursor key is still split across two functions: cleanupProposalTallyVotes deletes TallyCleanupKey itself here when it finishes, and its only caller deletes or rewrites the same key immediately afterwards (CleanupTallyVotes, lines 118-123). 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.)
| if proposal.Status != types.StatusVotingPeriod { | ||
| return sdkerrors.Wrapf(types.ErrInactiveProposal, "%d", proposalID) | ||
| } | ||
| if keeper.IsTallying(ctx, proposalID) { |
There was a problem hiding this comment.
[suggestion] While a tally is in progress 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 this window is now observable for many blocks. A distinct error or exposing IsTallying on the proposal query would make it diagnosable. (Raised in the previous three rounds; still unaddressed.)
|
|
||
| case bytes.Equal(kvA.Key[:1], types.TallyProgressKeyPrefix), | ||
| bytes.Equal(kvA.Key[:1], types.TallyCleanupKeyPrefix), | ||
| bytes.Equal(kvA.Key[:1], types.VoteDelegationsKeyPrefix), |
There was a problem hiding this comment.
[suggestion] VoteDelegationsKeyPrefix and TallyVoteDelegationsKeyPrefix values are proto VoteDelegationSnapshot records that cdc.MustUnmarshal can decode, unlike the JSON tallyProgress blob they are grouped with here. Decoding them in their own case would make store diffs for this app-hash-breaking change readable at no cost. (Raised in the previous round.)
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