feat: replace LastVote with expanded StopVoting method #221
Conversation
This adds a bool value to the StopVoting method so that we can avoid accessing the LastVote value only for checking if the prev and current view are different outside the voter.
meling
changed the title
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the voting mechanism by removing the LastVote method and extending StopVoting to return a boolean indicating whether the view was advanced. This allows callers to determine if a view change occurred without needing to access the previous vote state externally.
- Replaced
LastVote()method with a boolean return value fromStopVoting() - Simplified the logging logic in the synchronizer by using the return value
- Added comprehensive test coverage for the new
StopVotingbehavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| protocol/synchronizer/synchronizer.go | Simplified timeout handling by using StopVoting's return value for conditional logging |
| protocol/consensus/voter.go | Modified StopVoting to return boolean and removed LastVote method |
| protocol/consensus/voter_test.go | Added comprehensive tests for StopVoting behavior and updated existing test |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| s.logger.Debugf("Stopped voting in view %d and changed to view %d", prev, view) | ||
|
|
||
| if s.voter.StopVoting(view) { | ||
| s.logger.Debugf("Stopped voting for view %d", view) |
There was a problem hiding this comment.
Maybe this should be: Advanced to view %d?
There was a problem hiding this comment.
No, at this point, the replica is actually in between views.
It stopped voting in one view but did not start the next view.
It will only start the next view once it has sufficient timeouts.
We should create an issue to examine this further. I remember a related discussion on PBFT, where PBFT stopped voting once a view timed out but later work only stopped voting once a viewChange message is sent.
Does the synchronizer algorithm implemented here specify that we have to stop voting on a local timeout?
There was a problem hiding this comment.
I don't know the answer to that question, but that's what it does.
| s.logger.Debugf("Stopped voting in view %d and changed to view %d", prev, view) | ||
|
|
||
| if s.voter.StopVoting(view) { | ||
| s.logger.Debugf("Stopped voting for view %d", view) |
There was a problem hiding this comment.
No, at this point, the replica is actually in between views.
It stopped voting in one view but did not start the next view.
It will only start the next view once it has sufficient timeouts.
We should create an issue to examine this further. I remember a related discussion on PBFT, where PBFT stopped voting once a view timed out but later work only stopped voting once a viewChange message is sent.
Does the synchronizer algorithm implemented here specify that we have to stop voting on a local timeout?
Share the blockView variable instead of calling block.View() several times.
2 participants
This adds a bool value to the StopVoting method so that we can avoid accessing the LastVote value only for checking if the prev and current view are different outside the voter.