Merge 62b1bca4b4 into dddbaa4bf3

eth/protocols/snap/syncv2.go

@@ -607,6 +607,14 @@ func (s *syncerV2) Sync(target *types.Header, cancel chan struct{}) error {
 			// instead of starting a catch-up doomed to stall.
 			log.Warn("Catch-up gap exceeds BAL retention, restarting snap sync from scratch", "oldnumber", prevPivot.Number, "newnumber", target.Number, "gap", new(big.Int).Sub(target.Number, prevPivot.Number), "limit", maxCatchUpBlocks)
 			s.resetSyncState()
+		case gapStartsPreAmsterdam(s.db, prevPivot):
+			// No peer will ever serve a non-empty access list for a
+			// pre-Amsterdam block, so BAL catch-up can never bridge this gap.
+			// Discard the stale progress and resync from scratch via the
+			// state trie instead of starting a catch-up doomed to fail with
+			// errAccessListUnavailable.
+			log.Warn("Catch-up gap predates Amsterdam, restarting snap sync from scratch", "oldnumber", prevPivot.Number, "newnumber", target.Number)
+			s.resetSyncState()
 		default:
 			// An unclean shutdown may have left flushed snapshot data the journal
 			// doesn't cover.
@@ -793,6 +801,31 @@ func catchUpExceedsRetention(prev, curr *types.Header) bool {
 	return gap.Cmp(big.NewInt(maxCatchUpBlocks)) > 0
 }
 
+// gapStartsPreAmsterdam reports whether the first block of the catch-up gap
+// (prev, curr] predates the Amsterdam fork. Block access lists only exist
+// for Amsterdam and later blocks (EIP-7928); peers correctly respond to BAL
+// requests for earlier blocks with an empty entry (see
+// ServiceGetAccessListsQuery), which the fetcher cannot distinguish from a
+// genuinely unavailable access list. Left alone, this makes the fetcher
+// eventually treat every peer as having "refused" the block and fail with
+// errAccessListUnavailable. If the gap dips into pre-Amsterdam territory,
+// catch-up can never complete it, so the caller should wipe the stale
+// progress and resync via the state trie instead.
+func gapStartsPreAmsterdam(db ethdb.Database, prev *types.Header) bool {
+	num := prev.Number.Uint64() + 1
+
+	header := rawdb.ReadSkeletonHeader(db, num)
+	if header == nil {
+		if hash := rawdb.ReadCanonicalHash(db, num); hash != (common.Hash{}) {
+			header = rawdb.ReadHeader(db, hash, num)
+		}
+	}
+	// If the header isn't available locally yet, let the regular catch-up
+	// path surface that as a lookup failure; only short-circuit here when
+	// we can positively confirm the gap starts before Amsterdam.
+	return header != nil && header.BlockAccessListHash == nil
+}
+
 // catchUp runs the BAL catch-up. When the pivot has moved, it fetches BALs
 // for the gap blocks, verifies them against block headers, and applies the
 // diffs to roll flat state forward.

eth/protocols/snap/syncv2_test.go

@@ -1378,6 +1378,68 @@ func TestIsPivotReorged(t *testing.T) {
 	})
 }
 
+// TestGapStartsPreAmsterdam verifies the four conditions gapStartsPreAmsterdam
+// covers: a post-Amsterdam header found via the skeleton store, a
+// pre-Amsterdam header found via the skeleton store, a pre-Amsterdam header
+// found only via the canonical chain (skeleton already pruned), and a header
+// that isn't available locally yet (conservatively not flagged).
+func TestGapStartsPreAmsterdam(t *testing.T) {
+	t.Parallel()
+
+	// PostAmsterdam: the first gap block carries a BlockAccessListHash, so
+	// catch-up can proceed normally.
+	t.Run("PostAmsterdam", func(t *testing.T) {
+		db := rawdb.NewMemoryDatabase()
+		prev := mkPivot(100, common.HexToHash("0xaaaa"))
+		balHash := common.HexToHash("0xbeef")
+		next := &types.Header{Number: big.NewInt(101), Difficulty: common.Big0, BlockAccessListHash: &balHash}
+		rawdb.WriteSkeletonHeader(db, next)
+
+		if gapStartsPreAmsterdam(db, prev) {
+			t.Fatal("should not flag a gap whose first block already has a BAL")
+		}
+	})
+
+	// PreAmsterdam_Skeleton: the first gap block predates Amsterdam and is
+	// still available from the skeleton store.
+	t.Run("PreAmsterdam_Skeleton", func(t *testing.T) {
+		db := rawdb.NewMemoryDatabase()
+		prev := mkPivot(100, common.HexToHash("0xaaaa"))
+		next := &types.Header{Number: big.NewInt(101), Difficulty: common.Big0}
+		rawdb.WriteSkeletonHeader(db, next)
+
+		if !gapStartsPreAmsterdam(db, prev) {
+			t.Fatal("expected a pre-Amsterdam gap to be detected via the skeleton header")
+		}
+	})
+
+	// PreAmsterdam_Canonical: the skeleton entry is gone (e.g. pruned after
+	// the block was fully imported), but the header is still reachable via
+	// the canonical chain.
+	t.Run("PreAmsterdam_Canonical", func(t *testing.T) {
+		db := rawdb.NewMemoryDatabase()
+		prev := mkPivot(100, common.HexToHash("0xaaaa"))
+		next := &types.Header{Number: big.NewInt(101), Difficulty: common.Big0}
+		rawdb.WriteHeader(db, next)
+		rawdb.WriteCanonicalHash(db, next.Hash(), next.Number.Uint64())
+
+		if !gapStartsPreAmsterdam(db, prev) {
+			t.Fatal("expected a pre-Amsterdam gap to be detected via the canonical header")
+		}
+	})
+
+	// MissingHeader: nothing is known locally about the first gap block yet.
+	// Don't guess; let the regular catch-up path surface the lookup failure.
+	t.Run("MissingHeader", func(t *testing.T) {
+		db := rawdb.NewMemoryDatabase()
+		prev := mkPivot(100, common.HexToHash("0xaaaa"))
+
+		if gapStartsPreAmsterdam(db, prev) {
+			t.Fatal("should not flag a gap whose first block header is unknown locally")
+		}
+	})
+}
+
 // TestSyncDetectsPivotReorged exercises the reorg-handling branch in Sync
 // end-to-end.
 //
@@ -1465,6 +1527,73 @@ func TestSyncDetectsPivotReorged(t *testing.T) {
 	}
 }
 
+// TestSyncCatchUpGapPreAmsterdam reproduces
+// https://github.com/ethereum/go-ethereum/issues/35325: the pivot moves from
+// a low block (as happens right after a fresh node's first, early handshake
+// pivot) to a much later one whose gap starts before Amsterdam activated.
+//
+// Setup: persisted progress points at block 1. The chain's block 2 predates
+// Amsterdam (no BlockAccessListHash) and, matching real peer behavior for
+// pre-Amsterdam blocks, the test peer holds no BAL data at all — so any BAL
+// request for that range is guaranteed to be refused by every peer, exactly
+// as the source correctly does for pre-Amsterdam blocks in the wild.
+//
+// If gapStartsPreAmsterdam works, Sync detects this upfront, discards the
+// stale progress, and completes via a normal state download without ever
+// invoking catchUp. If it doesn't fire, catchUp tries to fetch BALs for the
+// unreachable range and Sync fails with errAccessListUnavailable.
+func TestSyncCatchUpGapPreAmsterdam(t *testing.T) {
+	t.Parallel()
+
+	nodeScheme, sourceAccountTrie, elems := makeAccountTrieNoStorage(100, rawdb.HashScheme)
+	root := sourceAccountTrie.Hash()
+
+	db := rawdb.NewMemoryDatabase()
+
+	// Persist progress against a pivot at block 1, as if this were the
+	// syncer's very first (early-handshake) pivot.
+	prevPivot := mkPivot(1, common.HexToHash("0x01"))
+	seed := newSyncerV2(db, nodeScheme)
+	seed.pivot = prevPivot
+	seed.saveSyncStatus()
+	rawdb.WriteHeader(db, prevPivot)
+	rawdb.WriteCanonicalHash(db, prevPivot.Hash(), prevPivot.Number.Uint64())
+
+	// Block 2 — the first block of the catch-up gap — predates Amsterdam:
+	// no BlockAccessListHash, matching a pre-fork header.
+	preAmsterdam := &types.Header{Number: big.NewInt(2), Difficulty: common.Big0}
+	rawdb.WriteSkeletonHeader(db, preAmsterdam)
+
+	// The new pivot is far ahead, well past Amsterdam activation.
+	target := mkPivot(100, root)
+
+	var (
+		once   sync.Once
+		cancel = make(chan struct{})
+		term   = func() { once.Do(func() { close(cancel) }) }
+	)
+	syncer := newSyncerV2(db, nodeScheme)
+	// The peer has no BAL data whatsoever, so it correctly refuses every BAL
+	// request — mirroring how peers respond for pre-Amsterdam blocks.
+	src := newTestPeerV2("source", t, term)
+	src.accountTrie = sourceAccountTrie.Copy()
+	src.accountValues = elems
+	syncer.Register(src)
+	src.remote = syncer
+
+	if err := syncer.Sync(target, cancel); err != nil {
+		t.Fatalf("sync failed (pre-Amsterdam gap detection likely broken): %v", err)
+	}
+	loader := newSyncerV2(db, nodeScheme)
+	loader.loadSyncStatus()
+	if loader.getPhase() != phaseComplete {
+		t.Fatal("sync status should reach the complete phase after successful completion")
+	}
+	if loader.pivot == nil || loader.pivot.Hash() != target.Hash() {
+		t.Fatalf("expected persisted pivot to match new pivot")
+	}
+}
+
 // TestInterruptedDownloadRecovery verifies that partially completed download
 // state is persisted and resumed on restart.
 func TestInterruptedDownloadRecovery(t *testing.T) {