eth/downloader: fix case where skeleton reorgs below the filled block (…

…#29358)

This change adds a testcase and fixes a corner-case in the skeleton sync.

With this change, when doing the skeleton cleanup, we check if the filled header is acually within the range of what we were meant to backfill. If not, it means the backfill was a noop (possibly because we started and stopped it so quickly that it didn't have time to do any meaningful work). In that case, just don't clean up anything.

---------

Co-authored-by: Péter Szilágyi <peterke@gmail.com>

eth/downloader/downloader_test.go

@@ -1311,3 +1311,84 @@ func testBeaconSync(t *testing.T, protocol uint, mode SyncMode) {
 		})
 	}
 }
+
+// Tests that synchronisation progress (origin block number and highest block
+// number) is tracked and updated correctly in case of manual head reversion
+func TestBeaconForkedSyncProgress68Full(t *testing.T) {
+	testBeaconForkedSyncProgress(t, eth.ETH68, FullSync)
+}
+func TestBeaconForkedSyncProgress68Snap(t *testing.T) {
+	testBeaconForkedSyncProgress(t, eth.ETH68, SnapSync)
+}
+func TestBeaconForkedSyncProgress68Light(t *testing.T) {
+	testBeaconForkedSyncProgress(t, eth.ETH68, LightSync)
+}
+
+func testBeaconForkedSyncProgress(t *testing.T, protocol uint, mode SyncMode) {
+	success := make(chan struct{})
+	tester := newTesterWithNotification(t, func() {
+		success <- struct{}{}
+	})
+	defer tester.terminate()
+
+	chainA := testChainForkLightA.shorten(len(testChainBase.blocks) + MaxHeaderFetch)
+	chainB := testChainForkLightB.shorten(len(testChainBase.blocks) + MaxHeaderFetch)
+
+	// Set a sync init hook to catch progress changes
+	starting := make(chan struct{})
+	progress := make(chan struct{})
+
+	tester.downloader.syncInitHook = func(origin, latest uint64) {
+		starting <- struct{}{}
+		<-progress
+	}
+	checkProgress(t, tester.downloader, "pristine", ethereum.SyncProgress{})
+
+	// Synchronise with one of the forks and check progress
+	tester.newPeer("fork A", protocol, chainA.blocks[1:])
+	pending := new(sync.WaitGroup)
+	pending.Add(1)
+	go func() {
+		defer pending.Done()
+		if err := tester.downloader.BeaconSync(mode, chainA.blocks[len(chainA.blocks)-1].Header(), nil); err != nil {
+			panic(fmt.Sprintf("failed to beacon sync: %v", err))
+		}
+	}()
+
+	<-starting
+	progress <- struct{}{}
+	select {
+	case <-success:
+		checkProgress(t, tester.downloader, "initial", ethereum.SyncProgress{
+			HighestBlock: uint64(len(chainA.blocks) - 1),
+			CurrentBlock: uint64(len(chainA.blocks) - 1),
+		})
+	case <-time.NewTimer(time.Second * 3).C:
+		t.Fatalf("Failed to sync chain in three seconds")
+	}
+
+	// Set the head to a second fork
+	tester.newPeer("fork B", protocol, chainB.blocks[1:])
+	pending.Add(1)
+	go func() {
+		defer pending.Done()
+		if err := tester.downloader.BeaconSync(mode, chainB.blocks[len(chainB.blocks)-1].Header(), nil); err != nil {
+			panic(fmt.Sprintf("failed to beacon sync: %v", err))
+		}
+	}()
+
+	<-starting
+	progress <- struct{}{}
+
+	// reorg below available state causes the state sync to rewind to genesis
+	select {
+	case <-success:
+		checkProgress(t, tester.downloader, "initial", ethereum.SyncProgress{
+			HighestBlock:  uint64(len(chainB.blocks) - 1),
+			CurrentBlock:  uint64(len(chainB.blocks) - 1),
+			StartingBlock: 0,
+		})
+	case <-time.NewTimer(time.Second * 3).C:
+		t.Fatalf("Failed to sync chain in three seconds")
+	}
+}

eth/downloader/skeleton.go

@@ -1132,6 +1132,16 @@ func (s *skeleton) cleanStales(filled *types.Header) error {
 	if number+1 == s.progress.Subchains[0].Tail {
 		return nil
 	}
+	// If the latest fill was on a different subchain, it means the backfiller
+	// was interrupted before it got to do any meaningful work, no cleanup
+	header := rawdb.ReadSkeletonHeader(s.db, filled.Number.Uint64())
+	if header == nil {
+		log.Debug("Filled header outside of skeleton range", "number", number, "head", s.progress.Subchains[0].Head, "tail", s.progress.Subchains[0].Tail)
+		return nil
+	} else if header.Hash() != filled.Hash() {
+		log.Debug("Filled header on different sidechain", "number", number, "filled", filled.Hash(), "skeleton", header.Hash())
+		return nil
+	}
 	var (
 		start uint64
 		end   uint64