doc(sync): add more clarifying comments to test

headertest/dummy_header.go

@@ -28,7 +28,9 @@ type DummyHeader struct {
 
 	hash header.Hash
 
-	VerifyFailure bool // TODO
+	// VerifyFailure allows for testing scenarios where a header would fail
+	// verification. When set to true, it forces a failure.
+	VerifyFailure bool
 }
 
 func RandDummyHeader(t *testing.T) *DummyHeader {
@@ -104,7 +106,7 @@ func (d *DummyHeader) IsExpired(period time.Duration) bool {
 
 func (d *DummyHeader) Verify(header header.Header) error {
 	if dummy, _ := header.(*DummyHeader); dummy.VerifyFailure {
-		return fmt.Errorf("bad header") // TODO
+		return fmt.Errorf("header at height %d failed verification", header.Height())
 	}
 
 	epsilon := 10 * time.Second

sync/sync_test.go

@@ -285,28 +285,33 @@ func TestSyncerIncomingDuplicate(t *testing.T) {
 	require.NoError(t, err)
 }
 
-// TestSync_InvalidSyncTarget tests the possible case that an incoming
-// header passes non-adjacent verification and is set as the sync target
-// but is actually invalid once it is processed via VerifyAdjacent during sync
+// 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 behaviour 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) {
 	ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
 	t.Cleanup(cancel)
 
 	suite := headertest.NewTestSuite(t)
 	head := suite.Head()
 
-	remoteStore := headertest.NewStore[*headertest.DummyHeader](t, suite, 100)
+	// create a local store which is initialised at genesis height
 	localStore := store.NewTestStore(ctx, t, head)
+	// create a peer which is already on height 100
+	remoteStore := headertest.NewStore[*headertest.DummyHeader](t, suite, 100)
 
 	syncer, err := NewSyncer[*headertest.DummyHeader](
 		local.NewExchange[*headertest.DummyHeader](remoteStore),
 		localStore,
 		headertest.NewDummySubscriber(),
 		WithTrustingPeriod(time.Second),
-		WithBlockTime(time.Nanosecond),
+		WithBlockTime(time.Nanosecond), // force syncer to request more recent sync target
 	)
 	require.NoError(t, err)
 
+	// 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,
@@ -316,39 +321,43 @@ func TestSync_InvalidSyncTarget(t *testing.T) {
 	err = remoteStore.Append(ctx, headers...)
 	require.NoError(t, err)
 
-	// TODO
-	h, err := syncer.Head(ctx)
-	require.NoError(t, err)
-	require.Equal(t, maliciousHeader.Height(), h.Height())
-
+	// 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)
 
-	time.Sleep(time.Millisecond * 100) // TODO flakey?
+	// give syncer some time to register the sync job before
+	// we wait for it to "finish syncing"
+	time.Sleep(time.Millisecond * 100)
 
+	// 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()
-
-	// TODO
+	// ensure that syncer still expects to sync to the bad sync target's
+	// height
 	require.Equal(t, uint64(maliciousHeader.Height()), syncer.State().ToHeight)
-
-	h, err = localStore.Head(ctx)
+	// ensure syncer could only sync up to one header below the bad sync target
+	h, err := localStore.Head(ctx)
 	require.NoError(t, err)
 	require.Equal(t, maliciousHeader.Height()-1, h.Height())
 
-	// TODO
+	// manually change bad sync target to a good header in remote peer
+	// store so it can re-serve it to syncer once it re-requests the height
 	remoteStore.Headers[maliciousHeader.Height()].VerifyFailure = false
 
-	// generate more headers and trigger sync again
+	// generate more headers
 	err = remoteStore.Append(ctx, suite.GenDummyHeaders(100)...)
 	require.NoError(t, err)
 
-	// pretend new header is received from network
+	// pretend new header is received from network to trigger
+	// a new sync job to a good sync target
 	expectedHead, err := remoteStore.Head(ctx)
 	require.NoError(t, err)
 	syncer.incomingNetworkHead(ctx, expectedHead)
+	// wait for syncer to finish
 	err = syncer.SyncWait(ctx)
 	require.NoError(t, err)
 
@@ -358,13 +367,13 @@ func TestSync_InvalidSyncTarget(t *testing.T) {
 	require.NoError(t, err)
 	require.False(t, rerequested.VerifyFailure)
 
-	gotHead, err := localStore.Head(ctx)
+	// check store head and syncer head
+	storeHead, err := localStore.Head(ctx)
 	require.NoError(t, err)
-
 	syncHead, err := syncer.Head(ctx)
 	require.NoError(t, err)
 
-	require.Equal(t, expectedHead.Height()-1, gotHead.Height())
+	require.Equal(t, expectedHead.Height()-1, storeHead.Height()) // head might not be applied to store yet
 	require.Equal(t, expectedHead.Height(), syncHead.Height())
 }