eth/protocols/snap: added read lock in catchup and some reorg

documentation and additional test coverage for deep reorg

Author: jrhea

core/state/snapshot/snapshot.go

@@ -733,12 +733,10 @@ func (t *Tree) Rebuild(root common.Hash) {
 func (t *Tree) RebuildFromSyncedState(root common.Hash) {
 	t.lock.Lock()
 	defer t.lock.Unlock()
-
 	rawdb.DeleteSnapshotRecoveryNumber(t.diskdb)
 	rawdb.DeleteSnapshotDisabled(t.diskdb)
 	rawdb.WriteSnapshotRoot(t.diskdb, root)
 	journalProgress(t.diskdb, nil, nil)
-
 	log.Info("Setting up snapshot from synced state", "root", root)
 	t.layers = map[common.Hash]snapshot{
 		root: &diskLayer{

eth/downloader/downloader.go

@@ -872,6 +872,11 @@ func (d *Downloader) importBlockResults(results []*fetchResult) error {
 // the old pivot's block number has a different state root, the syncer's flat
 // state is from the old fork and must be wiped. Returns true if a deep reorg
 // was detected.
+//
+// Returns false (no reorg) when the canonical hash or header is missing. This
+// avoids false positives from pruned or not-yet-downloaded data. If the chain
+// really did shorten past the old pivot, sync.catchUp's from > to guard will
+// catch this.
 func checkDeepReorg(db ethdb.Database, oldNumber uint64, oldRoot common.Hash) bool {
 	oldHash := rawdb.ReadCanonicalHash(db, oldNumber)
 	if oldHash == (common.Hash{}) {

eth/downloader/downloader_test.go

@@ -703,34 +703,60 @@ func testSyncProgress(t *testing.T, protocol uint, mode SyncMode) {
 // TestDeepReorgDetection verifies that checkDeepReorg correctly identifies
 // when the canonical chain has reorged past the old pivot.
 func TestDeepReorgDetection(t *testing.T) {
-	db := rawdb.NewMemoryDatabase()
-	pivotNumber := uint64(100)
-	pivotRoot := common.HexToHash("0xaaaa")
-	reorgedRoot := common.HexToHash("0xbbbb")
+	t.Parallel()
+
+	// Case 1: Chain reorged to a shorter fork — old pivot number no longer
+	// has a canonical hash. checkDeepReorg returns false (can't confirm reorg
+	// without data). The syncer's catchUp guard handles this case instead
+	// (see TestCatchUpInvertedRange in eth/protocols/snap/sync_test.go).
+	t.Run("ShorterChain", func(t *testing.T) {
+		db := rawdb.NewMemoryDatabase()
+		// No canonical hash written at block 100 — chain is shorter
+		if checkDeepReorg(db, 100, common.HexToHash("0xaaaa")) {
+			t.Fatal("should not detect reorg when canonical hash is missing")
+		}
+	})
 
-	// Write a header at the pivot number with a different root (simulating reorg)
-	header := &types.Header{
-		Number:     new(big.Int).SetUint64(pivotNumber),
-		Root:       reorgedRoot,
-		Difficulty: common.Big0,
-	}
-	rawdb.WriteHeader(db, header)
-	rawdb.WriteCanonicalHash(db, header.Hash(), pivotNumber)
+	// Case 2: Chain reorged to a same-length (or longer) fork — old pivot
+	// number exists but has a different state root. checkDeepReorg detects it.
+	t.Run("DifferentRoot", func(t *testing.T) {
+		db := rawdb.NewMemoryDatabase()
+		pivotNumber := uint64(100)
+		pivotRoot := common.HexToHash("0xaaaa")
+		reorgedRoot := common.HexToHash("0xbbbb")
+
+		header := &types.Header{
+			Number:     new(big.Int).SetUint64(pivotNumber),
+			Root:       reorgedRoot,
+			Difficulty: common.Big0,
+		}
+		rawdb.WriteHeader(db, header)
+		rawdb.WriteCanonicalHash(db, header.Hash(), pivotNumber)
 
-	// Should detect reorg: canonical root differs from our pivot root
-	if !checkDeepReorg(db, pivotNumber, pivotRoot) {
-		t.Fatal("expected deep reorg detection when roots differ")
-	}
+		if !checkDeepReorg(db, pivotNumber, pivotRoot) {
+			t.Fatal("expected deep reorg detection when roots differ")
+		}
+	})
 
-	// Should NOT detect reorg: canonical root matches our pivot root
-	if checkDeepReorg(db, pivotNumber, reorgedRoot) {
-		t.Fatal("should not detect reorg when roots match")
-	}
+	// Case 3: Same block at old pivot — no reorg at the pivot level.
+	// Blocks above may differ, but catch-up handles that normally.
+	t.Run("SameRoot", func(t *testing.T) {
+		db := rawdb.NewMemoryDatabase()
+		pivotNumber := uint64(100)
+		pivotRoot := common.HexToHash("0xaaaa")
+
+		header := &types.Header{
+			Number:     new(big.Int).SetUint64(pivotNumber),
+			Root:       pivotRoot,
+			Difficulty: common.Big0,
+		}
+		rawdb.WriteHeader(db, header)
+		rawdb.WriteCanonicalHash(db, header.Hash(), pivotNumber)
 
-	// Should NOT detect reorg: no canonical hash at that number
-	if checkDeepReorg(db, 999, pivotRoot) {
-		t.Fatal("should not detect reorg when block number is unknown")
-	}
+		if checkDeepReorg(db, pivotNumber, pivotRoot) {
+			t.Fatal("should not detect reorg when roots match")
+		}
+	})
 }
 
 // TestDeepReorgWipesProgress verifies that when a deep reorg is detected,

eth/protocols/snap/sync.go

@@ -332,10 +332,10 @@ type SyncPeer interface {
 	Log() log.Logger
 }
 
-// Syncer is an Ethereum account and storage trie syncer based on snapshots and
-// the  snap protocol. It's purpose is to download all the accounts and storage
-// slots from remote peers and reassemble chunks of the state trie, on top of
-// which a state sync can be run to fix any gaps / overlaps.
+// Syncer is an Ethereum account and storage trie syncer based on the snap
+// protocol. It downloads all accounts, storage slots, and bytecodes from
+// remote peers as flat state, applies BAL diffs on pivot moves,
+// and triggers a final trie rebuild once flat state is consistent.
 //
 // Every network request has a variety of failure events:
 //   - The peer disconnects after task assignment, failing to send the request
@@ -657,14 +657,21 @@ func (s *Syncer) resetDownload(root common.Hash, number uint64) {
 // root), it fetches BALs for the gap blocks, verifies them against
 // block headers, and applies the diffs to roll flat state forward.
 func (s *Syncer) catchUp(cancel chan struct{}) error {
+	s.lock.RLock()
 	from := s.previousNumber + 1
 	to := s.number
+	s.lock.RUnlock()
 
-	// The new pivot must be ahead of the old one. This can
-	// fail if a reorg replaced the block at the pivot height (same number,
-	// different root) or if a deep reorg shortened the chain past the old
-	// pivot. In either case, catch-up can't roll forward, so wipe progress
-	// and let the caller restart with a fresh sync.
+	// The new pivot must be ahead of the old one. This can fail if a reorg
+	// replaced the block at the pivot height (same number, different root)
+	// or if a deep reorg shortened the chain past the old pivot. In either
+	// case, catch-up can't roll forward, so wipe progress and return an
+	// error so the caller restarts with a fresh sync.
+	//
+	// Note: this check lives here rather than in checkDeepReorg because
+	// catchUp is reached both when the downloader actively moves the pivot
+	// (via restartSnapSync) and when the syncer resumes from persisted
+	// progress after a restart. checkDeepReorg only covers the former.
 	if from > to {
 		log.Warn("Catch-up range inverted, wiping sync progress", "from", from, "to", to)
 		rawdb.WriteSnapshotSyncStatus(s.db, nil)
@@ -1921,7 +1928,7 @@ func (s *Syncer) forwardAccountTask(task *accountTask) {
 
 	// Persist the received account segments. These flat state maybe
 	// outdated during the sync, but it can be fixed later during the
-	// snapshot generation.
+	// trie rebuild.
 	oldAccountBytes := s.accountBytes
 
 	batch := ethdb.HookedBatch{