fix: ensure leader sends vote after proposal #219
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the leader's voting behavior in the consensus protocol by ensuring the leader sends a vote after broadcasting a proposal. The fix distinguishes between when the current node is the next leader versus when it needs to send its vote to a different next leader.
- Reordered proposal broadcasting to happen before vote handling
- Added logic to determine the next leader and conditionally send votes
- Updated function documentation to reflect the new dual responsibility
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
protocol/comm/clique.go
Outdated
| } | ||
|
|
||
| // Disseminate stores a vote for the proposal and broadcasts the proposal. | ||
| // Disseminate broadcasts the proposal and aggregate my vote for this proposal |
There was a problem hiding this comment.
The doc comment is a bit confusing. What about this:
// Disseminate broadcasts the proposal to the other replicas and records it locally, bypassing the network stack.There was a problem hiding this comment.
no, the point is that the vote is also broadcast or submitted locally, depending on where is the next leader.
There was a problem hiding this comment.
Now it is:
Disseminate broadcasts the proposal and aggregates vote for this proposal
There was a problem hiding this comment.
Hmm. I still think it is unclear. Since we explicitly separated aggregation from dissemination with two methods, I feel a more precise explanation is warranted. There is also no doc comment explaining this in relation to the next leader (could be a new sentence). If we keep the current phrasing, it must be ... aggregates my vote for this proposal. (as in the original).
Nit: Why removed the .?
There was a problem hiding this comment.
I was not aware of the details. But do you want to explain aggregation in the doc for dissemination?
e.g.
// Disseminate broadcasts the proposal and aggregates my vote for this proposals.
// Aggregating sends the vote or stores it if I am leader in the next view.
func (hs *Clique) Disseminate(proposal *hotstuff.ProposeMsg, pc hotstuff.PartialCert) error {
There was a problem hiding this comment.
What about this:
// Disseminate broadcasts the proposal and immediately aggregates my vote for this proposals.
| // reusing the previous partial cert | ||
| // aggregating for view 2 to change the leader to 2 so clique will aggregate instead | ||
| // of storing the vote internally | ||
| err = clique.Aggregate(1, nil, pc) |
There was a problem hiding this comment.
Is this no longer necessary? Then we don't have any tests that involve calls to Aggregate. (I've actually updated this code in #220.)
There was a problem hiding this comment.
I noticed this as well. Check the new version
2 participants
No description provided.