Skip to content

fix: fixed unsafe modification order in MarkBallotExpired nand MarkBa… #217

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

Merged
0xNilesh merged 1 commit into from
May 1, 2026
+29 −25
Merged

fix: fixed unsafe modification order in MarkBallotExpired nand MarkBa… #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions x/uvalidator/keeper/ballot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,12 @@ func (k Keeper) DeleteBallot(ctx context.Context, id string) error {
return nil
}

// MarkBallotExpired moves a ballot from active to expired
// MarkBallotExpired moves a ballot from active to expired.
// Side-effect ordering: secondary indexes are updated before the canonical
// ballot record is rewritten, so the status field is only persisted once the
// active/expired set membership is in its final shape (defensive CEI-style
// ordering; collections.KeySet.Remove is a no-op on absent keys, so retries
// remain safe).
func (k Keeper) MarkBallotExpired(ctx context.Context, id string) error {
ballot, err := k.Ballots.Get(ctx, id)
if err != nil {
Expand All @@ -122,18 +127,20 @@ func (k Keeper) MarkBallotExpired(ctx context.Context, id string) error {
"expiry_height", ballot.BlockHeightExpiry,
)

ballot.Status = types.BallotStatus_BALLOT_STATUS_EXPIRED
if err := k.Ballots.Set(ctx, id, ballot); err != nil {
if err := k.ActiveBallotIDs.Remove(ctx, id); err != nil {
return err
}

if err := k.ActiveBallotIDs.Remove(ctx, id); err != nil {
if err := k.ExpiredBallotIDs.Set(ctx, id); err != nil {
return err
}
return k.ExpiredBallotIDs.Set(ctx, id)

ballot.Status = types.BallotStatus_BALLOT_STATUS_EXPIRED
return k.Ballots.Set(ctx, id, ballot)
}

// MarkBallotFinalized moves a ballot from active to finalized (PASSED or REJECTED)
// MarkBallotFinalized moves a ballot from active to finalized (PASSED or REJECTED).
// Side-effect ordering matches MarkBallotExpired: secondary indexes are
// updated before the canonical ballot record is rewritten with its final status.
func (k Keeper) MarkBallotFinalized(ctx context.Context, id string, status types.BallotStatus) error {
if status != types.BallotStatus_BALLOT_STATUS_PASSED && status != types.BallotStatus_BALLOT_STATUS_REJECTED {
return fmt.Errorf("invalid finalization status: %v", status)
Expand All @@ -149,15 +156,15 @@ func (k Keeper) MarkBallotFinalized(ctx context.Context, id string, status types
"final_status", status.String(),
)

ballot.Status = status
if err := k.Ballots.Set(ctx, id, ballot); err != nil {
if err := k.ActiveBallotIDs.Remove(ctx, id); err != nil {
return err
}

if err := k.ActiveBallotIDs.Remove(ctx, id); err != nil {
if err := k.FinalizedBallotIDs.Set(ctx, id); err != nil {
return err
}
return k.FinalizedBallotIDs.Set(ctx, id)

ballot.Status = status
return k.Ballots.Set(ctx, id, ballot)
}

// ExpireBallotsBeforeHeight checks active ballots and marks expired ones.
Expand Down
23 changes: 10 additions & 13 deletions x/uvalidator/keeper/voting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,22 +171,14 @@ func (k Keeper) VoteOnBallot(
if err != nil {
return ballot, false, false, err
}
if isFinalizing {
k.Logger().Debug("ballot finalized",
"ballot_id", id,
"ballot_status", ballot.Status.String(),
)
if err := k.ActiveBallotIDs.Remove(ctx, id); err != nil {
return ballot, false, isNew, errors.Wrap(err, "failed removing from active ballots")
}
if err := k.FinalizedBallotIDs.Set(ctx, id); err != nil {
return ballot, false, isNew, errors.Wrap(err, "failed adding to finalized ballots")
}
}

return ballot, isFinalizing, isNew, nil
}

// CheckIfFinalizingVote inspects whether the just-cast vote pushes the ballot
// over its threshold and, if so, drives the finalization through
// MarkBallotFinalized — the single canonical write path for terminal status
// transitions, which applies CEI-style ordering on the secondary indexes.
func (k Keeper) CheckIfFinalizingVote(ctx context.Context, b types.Ballot) (types.Ballot, bool, error) {
ballot, isFinalizing := b.IsFinalizingVote()
if !isFinalizing {
Expand All @@ -198,8 +190,13 @@ func (k Keeper) CheckIfFinalizingVote(ctx context.Context, b types.Ballot) (type
"ballot_status", ballot.Status.String(),
)

if err := k.SetBallot(ctx, ballot); err != nil {
if err := k.MarkBallotFinalized(ctx, ballot.Id, ballot.Status); err != nil {
return ballot, false, errors.Wrap(err, "failed updating finalized ballot")
}

k.Logger().Debug("ballot finalized",
"ballot_id", ballot.Id,
"ballot_status", ballot.Status.String(),
)
return ballot, true, nil
}
Loading