fix(sync): discard headers failed bifurcation (#269)

#219 introduced bifurcation however we missed there an issue with
setting a sync target as discovered in
#268 (comment).
Basically, a soft failed header failing bifurcation would still be used
a sync target. This is wrong and defeats the purpose of bifurcation.

This patch changes this so that bifurcation is used as source of truth
for headers we are unsure about(soft failed). If such header appears, we
bifurcate and get to the bottom whether its valid or not, s.t. the
header never enters syncing pipeline in pending cache and doesn't have
to be treated differently and thus simplify syncing flow.

Besides, this change sets all the discovered which were headers along
the bifurcation as subjective head, facilitating the syncing progress.

Author: Wondertan

sync/sync_head.go

@@ -138,52 +138,46 @@ func (s *Syncer[H]) incomingNetworkHead(ctx context.Context, head H) error {
 	s.incomingMu.Lock()
 	defer s.incomingMu.Unlock()
 
-	softFailure, err := s.verify(ctx, head)
-	if err != nil && !softFailure {
+	err := s.verify(ctx, head)
+	if err != nil {
 		return err
 	}
 
-	// TODO(@Wondertan):
-	//  Implement setSyncTarget and use it for soft failures
 	s.setSubjectiveHead(ctx, head)
 	return err
 }
 
 // verify verifies given network head candidate.
-// bool reports whether the returned error is a soft error.
-func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) {
+func (s *Syncer[H]) verify(ctx context.Context, newHead H) error {
 	sbjHead, err := s.subjectiveHead(ctx)
 	if err != nil {
 		log.Errorw("getting subjective head during validation", "err", err)
-		// local error, so soft
-		return true, &header.VerifyError{Reason: err, SoftFailure: true}
+		return err
 	}
 
 	err = header.Verify(sbjHead, newHead)
 	if err == nil {
-		return false, nil
+		return nil
 	}
 
 	var verErr *header.VerifyError
-	if errors.As(err, &verErr) {
-		if verErr.SoftFailure {
-			err := s.verifyBifurcating(ctx, sbjHead, newHead)
-			return err != nil, err
-		}
+	if errors.As(err, &verErr) && verErr.SoftFailure {
+		// bifurcate for soft failures only
+		return s.verifyBifurcating(ctx, sbjHead, newHead)
+	}
 
-		logF := log.Warnw
-		if errors.Is(err, header.ErrKnownHeader) {
-			logF = log.Debugw
-		}
-		logF("invalid network header",
-			"height_of_invalid", newHead.Height(),
-			"hash_of_invalid", newHead.Hash(),
-			"height_of_subjective", sbjHead.Height(),
-			"hash_of_subjective", sbjHead.Hash(),
-			"reason", verErr.Reason)
+	logF := log.Warnw
+	if errors.Is(err, header.ErrKnownHeader) {
+		logF = log.Debugw
 	}
+	logF("invalid network header",
+		"height_of_invalid", newHead.Height(),
+		"hash_of_invalid", newHead.Hash(),
+		"height_of_subjective", sbjHead.Height(),
+		"hash_of_subjective", sbjHead.Hash(),
+		"reason", verErr.Reason)
 
-	return verErr.SoftFailure, err
+	return err
 }
 
 // verifyBifurcating verifies networkHead against subjHead via the interim headers when direct
@@ -224,6 +218,7 @@ func (s *Syncer[H]) verifyBifurcating(ctx context.Context, subjHead, networkHead
 
 		// candidate was validated properly, update subjHead.
 		subjHead = candidateHeader
+		s.setSubjectiveHead(ctx, subjHead)
 
 		if err := header.Verify(subjHead, networkHead); err == nil {
 			// network head validate properly, return success.

sync/sync_test.go

@@ -294,12 +294,11 @@ func TestSyncerIncomingDuplicate(t *testing.T) {
 	require.NoError(t, err)
 }
 
-// TestSync_InvalidSyncTarget tests the possible case that a sync target
-// passes non-adjacent verification but is actually invalid once it is processed
-// via VerifyAdjacent during sync. The expected behavior is that the syncer would
-// discard the invalid sync target and listen for a new sync target from headersub
-// and sync the valid chain.
-func TestSync_InvalidSyncTarget(t *testing.T) {
+// TestSync_SoftFailureBifurcate asserts that network head soft failure is handled correctly,
+// triggering a bifurcation.
+// The expected behavior is that the syncer discards the invalid sync target if bifurcation confirms
+// its invalidity. Yet it recovers if the new sync target is valid.
+func TestSync_SoftFailureBifurcate(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
 	t.Cleanup(cancel)
 
@@ -323,32 +322,30 @@ func TestSync_InvalidSyncTarget(t *testing.T) {
 	// generate 300 more headers
 	headers := suite.GenDummyHeaders(300)
 	// malform the remote store's head so that it can serve
-	// the syncer a "bad" sync target that passes initial validation,
-	// but not verification.
+	// the syncer a soft failed head
 	maliciousHeader := headers[299]
 	maliciousHeader.VerifyFailure = true
+	maliciousHeader.SoftFailure = true
 	err = remoteStore.Append(ctx, headers...)
 	require.NoError(t, err)
 
 	// start syncer so that it immediately requests the bad
 	// sync target from the remote peer via Head request
 	err = syncer.Start(ctx)
 	require.NoError(t, err)
-
-	// give syncer some time to register the sync job before
-	// we wait for it to "finish syncing"
+	// await for sync to finish
 	time.Sleep(time.Millisecond * 100)
+	err = syncer.SyncWait(ctx)
+	require.NoError(t, err)
+
+	// ensure that syncer progressed even with invalid header
+	// yet excluding the invalid header
+	state := syncer.State()
+	assert.Equal(t, maliciousHeader.Height()-1, state.Height)
+	assert.True(t, state.Finished())
+	assert.EqualValues(t, 2, state.FromHeight)
 
-	// expect syncer to not be able to finish the sync
-	// job as the bad sync target cannot be applied
-	shortCtx, cancel := context.WithTimeout(ctx, time.Millisecond*200)
-	err = syncer.SyncWait(shortCtx)
-	require.ErrorIs(t, err, context.DeadlineExceeded)
-	cancel()
-	// ensure that syncer still expects to sync to the bad sync target's
-	// height
-	require.Equal(t, maliciousHeader.Height(), syncer.State().ToHeight)
-	// ensure syncer could only sync up to one header below the bad sync target
+	// ensure syncer could only sync up to one header below the invalid header
 	h, err := localStore.Head(ctx)
 	require.NoError(t, err)
 	require.Equal(t, maliciousHeader.Height()-1, h.Height())