Skip to content

Simplify VerifySyncInfo implementations with new SyncInfo.HighestView() methods #263

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

Closed
meling wants to merge 4 commits into from
Closed

Simplify VerifySyncInfo implementations with new SyncInfo.HighestView() methods #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee52f58
feat: add HighestView and HighestViewAggQC methods to SyncInfo
meling Nov 3, 2025
bb188b2
refactor: simplify VerifySyncInfo in Aggregate and Simple timeout rules
meling Nov 3, 2025
eec647e
fix: added missing test cases in TestAdvanceView
meling Nov 3, 2025
6de681b
fix: VerifySyncInfo to verify QC absent aggregate QC
meling Nov 14, 2025
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
8 changes: 7 additions & 1 deletion protocol/synchronizer/synchronizer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,28 +180,32 @@ func TestAdvanceView(t *testing.T) {
{name: "signers=4/Simple___/__/TC/AC", tr: S, qc: F, tc: T, ac: T, firstSignerIdx: 0, wantView: 2},
{name: "signers=4/Simple___/QC/__/__", tr: S, qc: T, tc: F, ac: F, firstSignerIdx: 0, wantView: 2},
{name: "signers=4/Simple___/QC/__/AC", tr: S, qc: T, tc: F, ac: T, firstSignerIdx: 0, wantView: 2},
{name: "signers=4/Simple___/QC/TC/__", tr: S, qc: T, tc: T, ac: F, firstSignerIdx: 0, wantView: 2},
{name: "signers=4/Simple___/QC/TC/AC", tr: S, qc: T, tc: T, ac: T, firstSignerIdx: 0, wantView: 2},
{name: "signers=4/Aggregate/__/__/__", tr: A, qc: F, tc: F, ac: F, firstSignerIdx: 0, wantView: 1}, // empty syncInfo, should not advance view
{name: "signers=4/Aggregate/__/__/AC", tr: A, qc: F, tc: F, ac: T, firstSignerIdx: 0, wantView: 2},
{name: "signers=4/Aggregate/__/TC/__", tr: A, qc: F, tc: T, ac: F, firstSignerIdx: 0, wantView: 2},
{name: "signers=4/Aggregate/__/TC/AC", tr: A, qc: F, tc: T, ac: T, firstSignerIdx: 0, wantView: 2},
{name: "signers=4/Aggregate/QC/__/__", tr: A, qc: T, tc: F, ac: F, firstSignerIdx: 0, wantView: 1}, // aggregate timeout rule ignores quorum cert, will not advance view
{name: "signers=4/Aggregate/QC/__/AC", tr: A, qc: T, tc: F, ac: T, firstSignerIdx: 0, wantView: 2},
{name: "signers=4/Aggregate/QC/TC/__", tr: A, qc: T, tc: T, ac: F, firstSignerIdx: 0, wantView: 2},
{name: "signers=4/Aggregate/QC/TC/AC", tr: A, qc: T, tc: T, ac: T, firstSignerIdx: 0, wantView: 2},
// three signers; quorum reacted, advance view
// three signers; quorum reached, advance view
{name: "signers=3/Simple___/__/__/__", tr: S, qc: F, tc: F, ac: F, firstSignerIdx: 1, wantView: 1}, // empty syncInfo, should not advance view
{name: "signers=3/Simple___/__/__/AC", tr: S, qc: F, tc: F, ac: T, firstSignerIdx: 1, wantView: 1}, // simple timeout rule ignores aggregate timeout cert, will not advance view
{name: "signers=3/Simple___/__/TC/__", tr: S, qc: F, tc: T, ac: F, firstSignerIdx: 1, wantView: 2},
{name: "signers=3/Simple___/__/TC/AC", tr: S, qc: F, tc: T, ac: T, firstSignerIdx: 1, wantView: 2},
{name: "signers=3/Simple___/QC/__/__", tr: S, qc: T, tc: F, ac: F, firstSignerIdx: 1, wantView: 2},
{name: "signers=3/Simple___/QC/__/AC", tr: S, qc: T, tc: F, ac: T, firstSignerIdx: 1, wantView: 2},
{name: "signers=3/Simple___/QC/TC/__", tr: S, qc: T, tc: T, ac: F, firstSignerIdx: 1, wantView: 2},
{name: "signers=3/Simple___/QC/TC/AC", tr: S, qc: T, tc: T, ac: T, firstSignerIdx: 1, wantView: 2},
{name: "signers=3/Aggregate/__/__/__", tr: A, qc: F, tc: F, ac: F, firstSignerIdx: 1, wantView: 1}, // empty syncInfo, should not advance view
{name: "signers=3/Aggregate/__/__/AC", tr: A, qc: F, tc: F, ac: T, firstSignerIdx: 1, wantView: 2},
{name: "signers=3/Aggregate/__/TC/__", tr: A, qc: F, tc: T, ac: F, firstSignerIdx: 1, wantView: 2},
{name: "signers=3/Aggregate/__/TC/AC", tr: A, qc: F, tc: T, ac: T, firstSignerIdx: 1, wantView: 2},
{name: "signers=3/Aggregate/QC/__/__", tr: A, qc: T, tc: F, ac: F, firstSignerIdx: 1, wantView: 1}, // aggregate timeout rule ignores quorum cert, will not advance view
{name: "signers=3/Aggregate/QC/__/AC", tr: A, qc: T, tc: F, ac: T, firstSignerIdx: 1, wantView: 2},
{name: "signers=3/Aggregate/QC/TC/__", tr: A, qc: T, tc: T, ac: F, firstSignerIdx: 1, wantView: 2},
{name: "signers=3/Aggregate/QC/TC/AC", tr: A, qc: T, tc: T, ac: T, firstSignerIdx: 1, wantView: 2},
// only two signers; no quorum reached, should not advance view
{name: "signers=2/Simple___/__/__/__", tr: S, qc: F, tc: F, ac: F, firstSignerIdx: 2, wantView: 1}, // empty syncInfo, should not advance view
Expand All @@ -210,13 +214,15 @@ func TestAdvanceView(t *testing.T) {
{name: "signers=2/Simple___/__/TC/AC", tr: S, qc: F, tc: T, ac: T, firstSignerIdx: 2, wantView: 1},
{name: "signers=2/Simple___/QC/__/__", tr: S, qc: T, tc: F, ac: F, firstSignerIdx: 2, wantView: 1},
{name: "signers=2/Simple___/QC/__/AC", tr: S, qc: T, tc: F, ac: T, firstSignerIdx: 2, wantView: 1},
{name: "signers=2/Simple___/QC/TC/__", tr: S, qc: T, tc: T, ac: F, firstSignerIdx: 2, wantView: 1},
{name: "signers=2/Simple___/QC/TC/AC", tr: S, qc: T, tc: T, ac: T, firstSignerIdx: 2, wantView: 1},
{name: "signers=2/Aggregate/__/__/__", tr: A, qc: F, tc: F, ac: F, firstSignerIdx: 2, wantView: 1}, // empty syncInfo, should not advance view
{name: "signers=2/Aggregate/__/__/AC", tr: A, qc: F, tc: F, ac: T, firstSignerIdx: 2, wantView: 1},
{name: "signers=2/Aggregate/__/TC/__", tr: A, qc: F, tc: T, ac: F, firstSignerIdx: 2, wantView: 1},
{name: "signers=2/Aggregate/__/TC/AC", tr: A, qc: F, tc: T, ac: T, firstSignerIdx: 2, wantView: 1},
{name: "signers=2/Aggregate/QC/__/__", tr: A, qc: T, tc: F, ac: F, firstSignerIdx: 2, wantView: 1},
{name: "signers=2/Aggregate/QC/__/AC", tr: A, qc: T, tc: F, ac: T, firstSignerIdx: 2, wantView: 1},
{name: "signers=2/Aggregate/QC/TC/__", tr: A, qc: T, tc: T, ac: F, firstSignerIdx: 2, wantView: 1},
{name: "signers=2/Aggregate/QC/TC/AC", tr: A, qc: T, tc: T, ac: T, firstSignerIdx: 2, wantView: 1},
}
for _, tt := range tests {
Expand Down
15 changes: 8 additions & 7 deletions protocol/synchronizer/timeoutrule_aggregate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,24 +62,25 @@ func (s *Aggregate) RemoteTimeoutRule(currentView, timeoutView hotstuff.View, ti
}

func (s *Aggregate) VerifySyncInfo(syncInfo hotstuff.SyncInfo) (qc *hotstuff.QuorumCert, view hotstuff.View, timeout bool, err error) {
// get the highest view of the sync info's AggQC and TC
view, timeout = syncInfo.HighestViewAggQC()

if timeoutCert, haveTC := syncInfo.TC(); haveTC {
if err := s.auth.VerifyTimeoutCert(timeoutCert); err != nil {
return nil, 0, timeout, fmt.Errorf("failed to verify timeout certificate: %w", err)
}
view = timeoutCert.View()
timeout = true
}

if aggQC, haveQC := syncInfo.AggQC(); haveQC {
highQC, err := s.auth.VerifyAggregateQC(aggQC)
if err != nil {
return nil, 0, timeout, fmt.Errorf("failed to verify aggregate quorum certificate: %w", err)
}
if aggQC.View() >= view {
view = aggQC.View()
timeout = true
}
return &highQC, view, timeout, nil
} else if qc, haveQC := syncInfo.QC(); haveQC {
if err := s.auth.VerifyQuorumCert(qc); err != nil {
return nil, 0, timeout, fmt.Errorf("failed to verify quorum certificate: %w", err)
}
return &qc, view, timeout, nil
}
return nil, view, timeout, nil // aggregate quorum certificate not present, so no high QC available
}
11 changes: 3 additions & 8 deletions protocol/synchronizer/timeoutrule_simple.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,23 +47,18 @@ func (s *Simple) RemoteTimeoutRule(_, timeoutView hotstuff.View, timeouts []hots
}

func (s *Simple) VerifySyncInfo(syncInfo hotstuff.SyncInfo) (qc *hotstuff.QuorumCert, view hotstuff.View, timeout bool, err error) {
// get the highest view of the sync info's QC and TC
view, timeout = syncInfo.HighestView()

if timeoutCert, haveTC := syncInfo.TC(); haveTC {
if err := s.auth.VerifyTimeoutCert(timeoutCert); err != nil {
return nil, 0, timeout, fmt.Errorf("failed to verify timeout certificate: %w", err)
}
view = timeoutCert.View()
timeout = true
}

if quorumCert, haveQC := syncInfo.QC(); haveQC {
if err := s.auth.VerifyQuorumCert(quorumCert); err != nil {
return nil, 0, timeout, fmt.Errorf("failed to verify quorum certificate: %w", err)
}
// if there is both a TC and a QC, we use the QC if its view is greater or equal to the TC.
if quorumCert.View() >= view {
view = quorumCert.View()
timeout = false
}
return &quorumCert, view, timeout, nil
}
return nil, view, timeout, nil // quorum certificate not present, so no high QC available
Expand Down
36 changes: 36 additions & 0 deletions types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,42 @@ func (si *SyncInfo) SetAggQC(aggQC AggregateQC) {
si.aggQC = &aggQC
}

// HighestView returns the highest view among the QC and TC,
// and whether it came from a timeout.
func (si *SyncInfo) HighestView() (view View, timeout bool) {
switch {
case si.qc != nil && si.tc != nil:
if si.qc.View() >= si.tc.View() {
return si.qc.View(), false
}
return si.tc.View(), true
case si.qc != nil:
return si.qc.View(), false
case si.tc != nil:
return si.tc.View(), true
}
// no QC or TC present
return 0, false
}

// HighestViewAggQC returns the highest view among the AggQC and TC,
// and whether it came from a timeout.
func (si *SyncInfo) HighestViewAggQC() (view View, timeout bool) {
switch {
case si.aggQC != nil && si.tc != nil:
if si.aggQC.View() >= si.tc.View() {
return si.aggQC.View(), true // aggQC is always from timeouts
}
return si.tc.View(), true
case si.aggQC != nil:
return si.aggQC.View(), true
Comment on lines +204 to +208
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
case si.tc != nil:
return si.tc.View(), true
}
// no AggQC or TC present
return 0, false
}

// QC returns the quorum certificate, if present.
func (si SyncInfo) QC() (_ QuorumCert, _ bool) {
if si.qc != nil {
Expand Down