From 5f3c58f1de9ef91dd22b632e6bcb559081d56e1d Mon Sep 17 00:00:00 2001
From: jwasinger <j-wasinger@hotmail.com>
Date: Wed, 24 Apr 2024 00:07:39 -0700
Subject: [PATCH] eth/downloader: fix case where skeleton reorgs below the
 filled block (#29358)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 | 81 +++++++++++++++++++++++++++++++
 eth/downloader/skeleton.go        | 10 ++++
 2 files changed, 91 insertions(+)

diff --git a/eth/downloader/downloader_test.go b/eth/downloader/downloader_test.go
index 0fdc7ead9c3b0..c810518d56ae0 100644
--- a/eth/downloader/downloader_test.go
+++ b/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")
+	}
+}
diff --git a/eth/downloader/skeleton.go b/eth/downloader/skeleton.go
index 873ee950b66c6..04421a2bf5cab 100644
--- a/eth/downloader/skeleton.go
+++ b/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