bug: timeoutrule_aggregate.go:VerifySyncInfo() always returns timeout = true (unless SyncInfo is empty)

[meling]

See my TODO/NOTE comments below. This code will always return true for the timeout, unless syncInfo is missing both TC and AggQC (or the signatures fail to verify). This can't be right, no?

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 // TODO(meling): is this correct? See similar logic in timeoutrule_simple.go
		}
		return &highQC, view, timeout, nil // NOTE(meling): Timeout will always be true here
	}
	return nil, view, timeout, nil // aggregate quorum certificate not present, so no high QC available
}

[meling]

I now think I recall that we discussed the timeout = true issue before, but since it is confusing every time I see it, we should add a comment to clarify it. Or better yet, I think we can move some of the view and timeout checking as a method on SyncInfo:

// 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) {}

// 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) {}

These methods will make the logic in VerifySyncInfo easier to understand.