fix(eth/downloader): don't mistake a lagging head for a stalling peer

eth/downloader/downloader.go

@@ -1361,6 +1361,11 @@ func (d *Downloader) processHeaders(origin uint64, pivot uint64, td *big.Int) er
 		rollback    []*types.Header
 		rollbackErr error
 		mode        = d.getMode()
+		// Highest header written to the light chain this cycle. Block imports
+		// move the header head back to the inserted block, so CurrentHeader
+		// can trail the headers the peer already delivered. A bailing peer
+		// never advances it, keeping the stalling-peer detection intact.
+		lastInserted *types.Header
 	)
 	defer func() {
 		if len(rollback) > 0 {
@@ -1433,6 +1438,9 @@ func (d *Downloader) processHeaders(origin uint64, pivot uint64, td *big.Int) er
 				// peer gave us something useful, we're already happy/progressed (above check).
 				if mode == FastSync || mode == LightSync {
 					head := d.lightchain.CurrentHeader()
+					if lastInserted != nil && lastInserted.Number.Uint64() > head.Number.Uint64() {
+						head = lastInserted
+					}
 					if td.Cmp(d.lightchain.GetTd(head.Hash(), head.Number.Uint64())) > 0 {
 						return errStallingPeer
 					}
@@ -1483,6 +1491,7 @@ func (d *Downloader) processHeaders(origin uint64, pivot uint64, td *big.Int) er
 					if len(rollback) > fsHeaderSafetyNet {
 						rollback = append(rollback[:0], rollback[len(rollback)-fsHeaderSafetyNet:]...)
 					}
+					lastInserted = chunk[len(chunk)-1]
 				}
 				// Unless we're doing light chains, schedule the headers for associated content retrieval
 				if mode == FullSync || mode == FastSync {

eth/downloader/downloader_test.go

@@ -75,6 +75,17 @@ type downloadTester struct {
 
 	insertHeaderChainHook func([]*types.Header) error
 
+	// headHeaderCap, when non-zero, caps the height reported by CurrentHeader.
+	// It models the real chain, where importing blocks moves the header head
+	// back to the block being inserted. It must be set below the length of the
+	// peer chain being synced; a value at or above the chain head simply
+	// disables the cap and no longer models the lagging head.
+	//
+	// The cap is static, while the real lag is a transient window around the
+	// block being imported, but the simplified model is enough to reproduce
+	// the stall misdetection in the terminating header batch.
+	headHeaderCap uint64
+
 	// configOverride, when non-nil, is returned by Config() instead of the
 	// default TestChainConfig.  Used by tests that require XDPoS to be active.
 	configOverride *params.ChainConfig
@@ -187,6 +198,9 @@ func (dl *downloadTester) CurrentHeader() *types.Header {
 
 	for i := len(dl.ownHashes) - 1; i >= 0; i-- {
 		if header := dl.ownHeaders[dl.ownHashes[i]]; header != nil {
+			if dl.headHeaderCap != 0 && header.Number.Uint64() > dl.headHeaderCap {
+				continue
+			}
 			return header
 		}
 	}
@@ -1136,6 +1150,33 @@ func testHighTDStarvationAttack(t *testing.T, protocol int, mode SyncMode) {
 	tester.terminate()
 }
 
+// Tests that a header head lagging behind the headers the peer already delivered
+// is not mistaken for a stalling peer. Importing the post-pivot blocks moves the
+// header head back to the block being inserted, so it can trail the synced head
+// while the terminating header batch is processed. Both fast and light sync run
+// the lag-sensitive check, hence both modes are covered.
+func TestFastSyncHeaderHeadLag100(t *testing.T)  { testHeaderHeadLag(t, xdc100, FastSync) }
+func TestFastSyncHeaderHeadLag164(t *testing.T)  { testHeaderHeadLag(t, xdc164, FastSync) }
+func TestFastSyncHeaderHeadLag165(t *testing.T)  { testHeaderHeadLag(t, xdc165, FastSync) }
+func TestLightSyncHeaderHeadLag164(t *testing.T) { testHeaderHeadLag(t, xdc164, LightSync) }
+func TestLightSyncHeaderHeadLag165(t *testing.T) { testHeaderHeadLag(t, xdc165, LightSync) }
+
+func testHeaderHeadLag(t *testing.T, protocol int, mode SyncMode) {
+	t.Parallel()
+
+	tester := newTester()
+	defer tester.terminate()
+
+	chain := testChainBase.shorten(blockCacheMaxItems - 15)
+	tester.headHeaderCap = uint64(chain.len()) - 4
+	tester.newPeer("peer", protocol, chain)
+
+	if err := tester.sync("peer", nil, mode); err != nil {
+		t.Fatalf("failed to synchronise blocks: %v", err)
+	}
+	assertOwnChain(t, tester, chain.len())
+}
+
 // Tests that misbehaving peers are disconnected, whilst behaving ones are not.
 func TestBlockHeaderAttackerDropping100(t *testing.T) { testBlockHeaderAttackerDropping(t, xdc100) }
 func TestBlockHeaderAttackerDropping164(t *testing.T) { testBlockHeaderAttackerDropping(t, xdc164) }