diff --git a/eth/protocols/snap/syncv2.go b/eth/protocols/snap/syncv2.go index 630a9d1b75..5ee97bd452 100644 --- a/eth/protocols/snap/syncv2.go +++ b/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. diff --git a/eth/protocols/snap/syncv2_test.go b/eth/protocols/snap/syncv2_test.go index 577504152f..d085176ad2 100644 --- a/eth/protocols/snap/syncv2_test.go +++ b/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) {