Do not write a second empty vote for a round we already timed out on - #486
Merged
Conversation
triggerEmptyBlockNotarization set emptyVotes.timedOut but never read it, and three call sites reach it for the same round. Timing out on a round and then failing to verify its late proposal appended a second empty vote record for that round.
samliok
marked this pull request as ready for review
August 4, 2026 20:50
yacovm
reviewed
Aug 4, 2026
| return | ||
| } | ||
|
|
||
| // Several paths trigger the empty block agreement for the same round, so everything below, |
Collaborator
There was a problem hiding this comment.
production code LGTM, will look at test tomorrow
Signed-off-by: Sam Liokumovich <65994425+samliok@users.noreply.github.com>
yacovm
reviewed
Aug 5, 2026
yacovm
reviewed
Aug 5, 2026
yacovm
reviewed
Aug 5, 2026
| // countEmptyVoteRecords returns how many empty vote records the WAL holds for the round. It | ||
| // returns an error rather than asserting, so it is safe to call from the condition goroutine | ||
| // require.Never spawns. | ||
| func countEmptyVoteRecords(wal *testutil.TestWAL, round uint64) (int, error) { |
Collaborator
There was a problem hiding this comment.
I think that in the test we only have a single round - round 0, so we don't actually need this round parameter here.
Collaborator
Author
There was a problem hiding this comment.
updated, although I think a better solution is to just have our test WAL throw an error whenever we append a duplicate record. Maybe for another pr
yacovm
reviewed
Aug 7, 2026
| }, nodes[0])) | ||
|
|
||
| // The epoch appends to the WAL after Verify returns | ||
| require.Never(t, func() bool { |
Collaborator
There was a problem hiding this comment.
this can succeed if error is always != nil
yacovm
approved these changes
Aug 7, 2026
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
2 participants
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.
triggerEmptyBlockNotarizationis guarded only bye.round > round. It setsemptyVotes.timedOutbut never reads it, even thoughhaveWeAlreadyTimedOutOnThisRoundexists, and three call sites reach it for the same round: a failed block verification, the blacklisted-proposer path inbuildBlock, andproposalWaitTimeExpired.Any two firing for one round append two empty vote records, which is what fails
TestLongRunningReplicationwithduplicate empty vote record for round 95. The test times out on round 0, then delivers a proposal that fails verification, and asserts a second record never appears.