Skip to content

Move VerifySyncInfo from TimeoutRuler to cert.Authority #269

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

Open
meling wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Move VerifySyncInfo from TimeoutRuler to cert.Authority #269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
59e86ba
fix: added missing test cases in TestAdvanceView
meling Nov 14, 2025
cab0a01
refactor: move VerifySyncInfo from TimeoutRuler to cert.Authority
meling Nov 14, 2025
92d073b
fix: update TestAdvanceView test cases to match new VerifySyncInfo
meling Nov 14, 2025
2cccddf
fix: VerifySyncInfo to return the highest QC among AggQC and QC
meling Nov 14, 2025
588bef8
fix: VerifySyncInfo to keep QC as highQC if AggQC didn't provide one
meling Nov 16, 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
2 changes: 1 addition & 1 deletion protocol/synchronizer/synchronizer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ func (s *Synchronizer) OnNewView(newView hotstuff.NewViewMsg) {
func (s *Synchronizer) advanceView(syncInfo hotstuff.SyncInfo) {
s.logger.Debugf("advanceView: %v", syncInfo)

qc, view, timeout, err := s.timeoutRules.VerifySyncInfo(syncInfo)
qc, view, timeout, err := s.auth.VerifySyncInfo(syncInfo)
if err != nil {
s.logger.Infof("advanceView: Failed to verify sync info: %v", err)
return
Expand Down
16 changes: 11 additions & 5 deletions protocol/synchronizer/synchronizer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,33 +176,37 @@ func TestAdvanceView(t *testing.T) {
}{
// four signers; quorum reached, advance view
{name: "signers=4/Simple___/__/__/__", tr: S, qc: F, tc: F, ac: F, firstSignerIdx: 0, wantView: 1}, // empty syncInfo, should not advance view
{name: "signers=4/Simple___/__/__/AC", tr: S, qc: F, tc: F, ac: T, firstSignerIdx: 0, wantView: 1}, // simple timeout rule ignores aggregate timeout cert, will not advance view
{name: "signers=4/Simple___/__/__/AC", tr: S, qc: F, tc: F, ac: T, firstSignerIdx: 0, wantView: 2}, // advance view; simple timeout rule no longer ignores aggregate certs: cert.Authority.VerifySyncInfo()
{name: "signers=4/Simple___/__/TC/__", tr: S, qc: F, tc: T, ac: F, firstSignerIdx: 0, wantView: 2},
{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/__/__", tr: A, qc: T, tc: F, ac: F, firstSignerIdx: 0, wantView: 2}, // advance view; aggregate timeout rule no longer ignores quorum certs: cert.Authority.VerifySyncInfo()
{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___/__/__/AC", tr: S, qc: F, tc: F, ac: T, firstSignerIdx: 1, wantView: 2}, // advance view; simple timeout rule no longer ignores aggregate certs: cert.Authority.VerifySyncInfo()
{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/__/__", tr: A, qc: T, tc: F, ac: F, firstSignerIdx: 1, wantView: 2}, // advance view; aggregate timeout rule no longer ignores quorum certs: cert.Authority.VerifySyncInfo()
{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 @@ -211,13 +215,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
22 changes: 1 addition & 21 deletions protocol/synchronizer/timeoutrule_aggregate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,25 +61,5 @@ func (s *Aggregate) RemoteTimeoutRule(currentView, timeoutView hotstuff.View, ti
return si, nil
}

func (s *Aggregate) VerifySyncInfo(syncInfo hotstuff.SyncInfo) (qc *hotstuff.QuorumCert, view hotstuff.View, timeout bool, err error) {
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
}
return nil, view, timeout, nil // aggregate quorum certificate not present, so no high QC available
}
var _ TimeoutRuler = (*Aggregate)(nil)
23 changes: 1 addition & 22 deletions protocol/synchronizer/timeoutrule_simple.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,25 +46,4 @@ func (s *Simple) RemoteTimeoutRule(_, timeoutView hotstuff.View, timeouts []hots
return hotstuff.NewSyncInfoWith(tc), nil
}

func (s *Simple) VerifySyncInfo(syncInfo hotstuff.SyncInfo) (qc *hotstuff.QuorumCert, view hotstuff.View, timeout bool, err error) {
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
}
var _ TimeoutRuler = (*Simple)(nil)
1 change: 0 additions & 1 deletion protocol/synchronizer/timeoutrules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,4 @@ func NewTimeoutRuler(cfg *core.RuntimeConfig, auth *cert.Authority) TimeoutRuler
type TimeoutRuler interface {
LocalTimeoutRule(hotstuff.View, hotstuff.SyncInfo) (*hotstuff.TimeoutMsg, error)
RemoteTimeoutRule(currentView, timeoutView hotstuff.View, timeouts []hotstuff.TimeoutMsg) (hotstuff.SyncInfo, error)
VerifySyncInfo(hotstuff.SyncInfo) (qc *hotstuff.QuorumCert, view hotstuff.View, timeout bool, err error)
}
39 changes: 39 additions & 0 deletions security/cert/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,3 +209,42 @@ func (c *Authority) VerifyAnyQC(proposal *hotstuff.ProposeMsg) error {
}
return c.VerifyQuorumCert(qc)
}

// VerifySyncInfo verifies the sync info and returns the highest QC found (if any),
// the highest view, whether it was a timeout, and an error if verification failed.
func (c *Authority) VerifySyncInfo(syncInfo hotstuff.SyncInfo) (highQC *hotstuff.QuorumCert, view hotstuff.View, timeout bool, err error) {
if timeoutCert, haveTC := syncInfo.TC(); haveTC {
if err := c.VerifyTimeoutCert(timeoutCert); err != nil {
return nil, 0, timeout, fmt.Errorf("failed to verify timeout certificate: %w", err)
}
view = timeoutCert.View()
timeout = true
}

if aggQC, haveAggQC := syncInfo.AggQC(); haveAggQC {
qc, err := c.VerifyAggregateQC(aggQC)
if err != nil {
return nil, 0, timeout, fmt.Errorf("failed to verify aggregate quorum certificate: %w", err)
}
view = max(view, aggQC.View())
timeout = true // an aggregate QC represents a timeout
highQC = &qc
}

if qc, haveQC := syncInfo.QC(); haveQC {
if err := c.VerifyQuorumCert(qc); err != nil {
return nil, 0, timeout, fmt.Errorf("failed to verify quorum certificate: %w", err)
}
view = max(view, qc.View())
if qc.View() == view {
// QC's view is higher or equal: not a timeout
highQC = &qc
timeout = false
} else if highQC == nil {
// QC's view is lower, but there was no AggQC
Comment on lines +243 to +244
Copy link

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

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

The highQC selection logic doesn't properly handle the case when both AggQC and a plain QC are present, and the plain QC has a higher view than the AggQC's embedded QC.

Example scenario:

  • AggQC.View() = 6, AggQC's embedded highQC.View() = 4
  • Plain QC.View() = 5

Current behavior:

  1. Line 229: view = max(0, 6) = 6, highQC = &qc (view 4 from AggQC)
  2. Line 238: view = max(6, 5) = 6
  3. Line 239: qc.View() == view evaluates to 5 == 6 (false)
  4. Line 243: highQC == nil evaluates to false
  5. Result: Returns AggQC's embedded QC (view 4)

Expected: Should return the plain QC (view 5) since it's higher than the AggQC's embedded QC (view 4). The highQC should represent the highest quorum certificate known, regardless of which certificate determines the view to advance to.

Suggested fix: When qc.View() != view and highQC != nil, compare qc.View() with highQC.View() and update highQC if the plain QC has a higher view:

} else if highQC == nil || qc.View() > highQC.View() {
    highQC = &qc
}
Suggested change
} else if highQC == nil {
// QC's view is lower, but there was no AggQC
} else if highQC == nil || qc.View() > highQC.View() {
// QC's view is lower, but there was no AggQC, or QC is higher than current highQC

Copilot uses AI. Check for mistakes.
highQC = &qc
}
}

return highQC, view, timeout, nil
}
Loading