review suggestions

sync/metrics.go

@@ -22,6 +22,7 @@ type metrics struct {
 	trustedPeersOutOfSync metric.Int64Counter
 	outdatedHeader        metric.Int64Counter
 	subjectiveInit        metric.Int64Counter
+	failedAgainstSubjHead metric.Int64Counter
 
 	subjectiveHead atomic.Int64
 
@@ -71,6 +72,16 @@ func newMetrics() (*metrics, error) {
 		return nil, err
 	}
 
+	failedAgainstSubjHead, err := meter.Int64Counter(
+		"hdr_sync_subj_validation_failed",
+		metric.WithDescription(
+			"tracks how many times validation against subjective head failed",
+		),
+	)
+	if err != nil {
+		return nil, err
+	}
+
 	subjectiveHead, err := meter.Int64ObservableGauge(
 		"hdr_sync_subjective_head_gauge",
 		metric.WithDescription("subjective head height"),
@@ -112,6 +123,7 @@ func newMetrics() (*metrics, error) {
 		trustedPeersOutOfSync: trustedPeersOutOfSync,
 		outdatedHeader:        outdatedHeader,
 		subjectiveInit:        subjectiveInit,
+		failedAgainstSubjHead: failedAgainstSubjHead,
 		syncLoopDurationHist:  syncLoopDurationHist,
 		syncLoopRunningInst:   syncLoopRunningInst,
 		requestRangeTimeHist:  requestRangeTimeHist,
@@ -186,6 +198,12 @@ func (m *metrics) newSubjectiveHead(ctx context.Context, height uint64, timestam
 	})
 }
 
+func (m *metrics) failedValidationAgainstSubjHead(ctx context.Context) {
+	m.observe(ctx, func(ctx context.Context) {
+		m.failedAgainstSubjHead.Add(ctx, 1)
+	})
+}
+
 func (m *metrics) rangeRequestStart() {
 	if m == nil {
 		return

sync/sync_head.go

@@ -177,8 +177,7 @@ func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) {
 	if errors.As(err, &verErr) {
 		if verErr.SoftFailure {
 			err := s.verifySkipping(ctx, sbjHead, newHead)
-			var errValSet *NewValidatorSetCantBeTrustedError
-			return errors.As(err, &errValSet), err
+			return err != nil, err
 		}
 
 		logF := log.Warnw
@@ -243,19 +242,10 @@ func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHead, networkHeader
 		diff = networkHeader.Height() - subjHeight
 	}
 
-	return &NewValidatorSetCantBeTrustedError{
-		NetHeadHeight: networkHeader.Height(),
-		NetHeadHash:   networkHeader.Hash(),
-	}
-}
-
-type NewValidatorSetCantBeTrustedError struct {
-	NetHeadHeight uint64
-	NetHeadHash   []byte
-}
+	s.metrics.failedValidationAgainstSubjHead(ctx)
+	log.Warnw("sync: header validation against subjHead", "height", networkHeader.Height(), "hash", networkHeader.Hash().String())
 
-func (e *NewValidatorSetCantBeTrustedError) Error() string {
-	return fmt.Sprintf("sync: new validator set cant be trusted: head %d, attempted %x", e.NetHeadHeight, e.NetHeadHash)
+	return fmt.Errorf("sync: header validation against subjHead height:%d hash:%x", networkHeader.Height(), networkHeader.Hash().String())
 }
 
 // isExpired checks if header is expired against trusting period.

sync/sync_head_test.go

@@ -139,13 +139,12 @@ func TestSyncer_HeadWithNotEnoughValidators(t *testing.T) {
 	require.True(t, wrappedGetter.withTrustedHead)
 }
 
+// Test will simulate a case with upto `iters` failures before we will get to
+// the header that can be verified against subjectiveHead.
 func TestSyncer_verifySkippingSuccess(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
 	t.Cleanup(cancel)
 
-	const total = 1000
-	const badHeaderHeight = total + 1
-
 	suite := headertest.NewTestSuite(t)
 	head := suite.Head()
 
@@ -178,12 +177,18 @@ func TestSyncer_verifySkippingSuccess(t *testing.T) {
 		require.NoError(t, err)
 	})
 
+	// when
+	const total = 1000
+	const badHeaderHeight = total + 1 // make the last header bad
 	const iters = 4
 
 	headers := suite.GenDummyHeaders(total)
 	err = remoteStore.Append(ctx, headers...)
 	require.NoError(t, err)
 
+	// configure header verification method is such way
+	// that the first [iters] verification will fail
+	// but all other will be ok.
 	var verifyCounter atomic.Int32
 	for i := range total {
 		headers[i].VerifyFn = func(hdr *headertest.DummyHeader) error {
@@ -213,13 +218,12 @@ func TestSyncer_verifySkippingSuccess(t *testing.T) {
 	require.NoError(t, err)
 }
 
+// Test will simulate a case with upto `iters` failures before we will get to
+// the header that can be verified against subjectiveHead.
 func TestSyncer_verifySkippingSuccessWithBadCandidates(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
 	t.Cleanup(cancel)
 
-	const total = 1000
-	const badHeaderHeight = total + 1
-
 	suite := headertest.NewTestSuite(t)
 	head := suite.Head()
 
@@ -252,12 +256,17 @@ func TestSyncer_verifySkippingSuccessWithBadCandidates(t *testing.T) {
 		require.NoError(t, err)
 	})
 
+	const total = 1000
+	const badHeaderHeight = total + 1
 	const iters = 4
 
 	headers := suite.GenDummyHeaders(total)
 	err = remoteStore.Append(ctx, headers...)
 	require.NoError(t, err)
 
+	// configure header verification method is such way
+	// that the first [iters] verification will fail
+	// but all other will be ok.
 	var verifyCounter atomic.Int32
 	for i := range total {
 		headers[i].VerifyFn = func(hdr *headertest.DummyHeader) error {
@@ -266,13 +275,13 @@ func TestSyncer_verifySkippingSuccessWithBadCandidates(t *testing.T) {
 			}
 
 			verifyCounter.Add(1)
-			if verifyCounter.Load() <= iters {
-				return &header.VerifyError{
-					Reason:      headertest.ErrDummyVerify,
-					SoftFailure: hdr.SoftFailure,
-				}
+			if verifyCounter.Load() > iters {
+				return nil
+			}
+			return &header.VerifyError{
+				Reason:      headertest.ErrDummyVerify,
+				SoftFailure: hdr.SoftFailure,
 			}
-			return nil
 		}
 	}
 
@@ -286,13 +295,12 @@ func TestSyncer_verifySkippingSuccessWithBadCandidates(t *testing.T) {
 	require.NoError(t, err)
 }
 
+// Test will simulate a case when no headers can be verified against subjectiveHead.
+// As a result the [NewValidatorSetCantBeTrustedError] error will be returned.
 func TestSyncer_verifySkippingCannotVerify(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
 	t.Cleanup(cancel)
 
-	const total = 1000
-	const badHeaderHeight = total + 1
-
 	suite := headertest.NewTestSuite(t)
 	head := suite.Head()
 
@@ -325,6 +333,9 @@ func TestSyncer_verifySkippingCannotVerify(t *testing.T) {
 		require.NoError(t, err)
 	})
 
+	const total = 1000
+	const badHeaderHeight = total + 1
+
 	headers := suite.GenDummyHeaders(total)
 	err = remoteStore.Append(ctx, headers...)
 	require.NoError(t, err)
@@ -349,8 +360,7 @@ func TestSyncer_verifySkippingCannotVerify(t *testing.T) {
 	require.NoError(t, err)
 
 	err = syncer.verifySkipping(ctx, subjHead, headers[total-1])
-	var verErr *NewValidatorSetCantBeTrustedError
-	assert.ErrorIs(t, err, verErr, "%T", err)
+	assert.Error(t, err)
 }
 
 type wrappedGetter struct {