Simplify VerifySyncInfo implementations with new SyncInfo.HighestView() methods #263
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors duplicate logic for determining the highest view in SyncInfo by extracting it into reusable methods. The changes reduce code duplication and improve maintainability by centralizing the view comparison logic.
- Adds
HighestView()andHighestViewAggQC()methods toSyncInfothat encapsulate the logic for determining the highest view and timeout status - Refactors
Simple.VerifySyncInfo()to use the newHighestView()method - Refactors
Aggregate.VerifySyncInfo()to use the newHighestViewAggQC()method
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| types.go | Added two new methods HighestView() and HighestViewAggQC() to encapsulate view comparison logic for different certificate types |
| protocol/synchronizer/timeoutrule_simple.go | Replaced inline view comparison logic with call to syncInfo.HighestView() |
| protocol/synchronizer/timeoutrule_aggregate.go | Replaced inline view comparison logic with call to syncInfo.HighestViewAggQC() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return si.aggQC.View(), true // aggQC is always from timeouts | ||
| } | ||
| return si.tc.View(), true | ||
| case si.aggQC != nil: | ||
| return si.aggQC.View(), true |
There was a problem hiding this comment.
The inline comment 'aggQC is always from timeouts' is misleading. According to the HotStuff protocol and Fast-HotStuff extension, an AggregateQC is a collection of QCs from timeout messages, but it represents the highest QC among those timeouts, not a timeout certificate itself. The timeout return value should indicate whether the highest view came from a timeout certificate (TC) or not. When aggQC has a higher view than TC, the timeout flag should be false since the highest view comes from the aggregated QCs (which represent successful proposals), not from a timeout.
| return si.aggQC.View(), true // aggQC is always from timeouts | |
| } | |
| return si.tc.View(), true | |
| case si.aggQC != nil: | |
| return si.aggQC.View(), true | |
| // aggQC represents the highest QC among timeouts, not a timeout certificate itself | |
| return si.aggQC.View(), false | |
| } | |
| return si.tc.View(), true | |
| case si.aggQC != nil: | |
| // aggQC present, no TC; highest view comes from QC | |
| return si.aggQC.View(), false |
This adds two helper methods for view comparison between QC and TC and AggQC and TC. These can be used by VerifySyncInfo.
This uses the new HighestView() methods to determine if the QC or TC has the highest view, and thus if timeout should be true. Note: Moving the initialization of view and timeout before the signature verification should be fine, since a signature failure will return err != nil, and users of the VerifySyncInfo must always check for err before using any other output.
meling
force-pushed
the
meling/262/simplify-verify-syncinfo
branch
from
b8e3aee to
eec647e
Compare
If the SyncInfo is missing the AggregateQC, we now check if the SyncInfo has a quorum certificate and returns that instead. This corresponds to the existing logic on the master branch.
|
Superceded by #269 |
Fixes #262