fix(blocksync): prevent maxPeerHeight poisoning (#5803)

blocksync/pool.go

@@ -254,6 +254,9 @@ func (pool *BlockPool) PopRequest() {
 	delete(pool.requesters, pool.height)
 	pool.height++
 
+	// Re-evaluate maxPeerHeight: peers whose pruned base was just beyond the previous pool.height may now be able to contribute
+	pool.updateMaxPeerHeight()
+
 	// Notify the next minBlocksForSingleRequest requesters about new height, so
 	// they can potentially request a block from the second peer.
 	for i := int64(0); i < minBlocksForSingleRequest && i < int64(len(pool.requesters)); i++ {
@@ -359,6 +362,16 @@ func (pool *BlockPool) SetPeerRange(peerID p2p.ID, base int64, height int64) {
 	pool.mtx.Lock()
 	defer pool.mtx.Unlock()
 
+	// A peer whose own reported base exceeds its own height is structurally impossible and treated as malicious.
+	if base > height {
+		pool.Logger.Info("Peer reporting base greater than height", "peer", peerID, "base", base, "height", height)
+		if _, exists := pool.peers[peerID]; exists {
+			pool.removePeer(peerID)
+		}
+		pool.banPeer(peerID)
+		return
+	}
+
 	peer := pool.peers[peerID]
 	if peer != nil {
 		if base < peer.base || height < peer.height {
@@ -386,9 +399,7 @@ func (pool *BlockPool) SetPeerRange(peerID p2p.ID, base int64, height int64) {
 		pool.sortedPeers = append([]*bpPeer{peer}, pool.sortedPeers...)
 	}
 
-	if height > pool.maxPeerHeight {
-		pool.maxPeerHeight = height
-	}
+	pool.updateMaxPeerHeight()
 }
 
 // RemovePeer removes the peer with peerID from the pool. If there's no peer
@@ -429,10 +440,16 @@ func (pool *BlockPool) removePeer(peerID p2p.ID) {
 	}
 }
 
-// If no peers are left, maxPeerHeight is set to 0.
+// updateMaxPeerHeight sets maxPeerHeight to the highest height among peers.
 func (pool *BlockPool) updateMaxPeerHeight() {
 	var max int64
 	for _, peer := range pool.peers {
+		if pool.height > 0 && peer.base > pool.height {
+			// Blocks a malicious peer from poisoning maxPeerHeight with an
+			// inflated base/height pair no peer can actually serve, which
+			// would stall IsCaughtUp forever.
+			continue
+		}
 		if peer.height > max {
 			max = peer.height
 		}

blocksync/pool_test.go

@@ -187,7 +187,8 @@
 	// Introduce each peer.
 	go func() {
 		for _, peer := range peers {
-			pool.SetPeerRange(peer.id, peer.base, peer.height)
+			// Force uniform base so every peer contributes to maxPeerHeight at pool startup.
+			pool.SetPeerRange(peer.id, start, peer.height)
 		}
 	}()
 
@@ -497,3 +498,53 @@
 		}
 	}
 }
+
+// TestBlockPoolBansPeerWithBaseGreaterThanHeight verifies that a peer whose self-reported base
+// exceeds its own height (a structurally impossible state) is banned.
+func TestBlockPoolBansPeerWithBaseGreaterThanHeight(t *testing.T) {
+	requestsCh := make(chan BlockRequest, 10)
+	errorsCh := make(chan peerError, 10)
+
+	pool := NewBlockPool(1, requestsCh, errorsCh)
+	pool.SetLogger(log.TestingLogger())
+
+	badID := p2p.ID("bad")
+	pool.SetPeerRange(badID, 500, 100)
+
+	require.True(t, pool.IsPeerBanned(badID), "peer reporting base > height must be banned")
+	require.EqualValues(t, 0, pool.MaxPeerHeight(), "banned peer must not raise maxPeerHeight")
+}
+
+// TestBlockPoolMaxPeerHeightRefreshesOnPopRequest covers:
+//  1. A peer whose base is ahead of pool.height must not contribute to maxPeerHeight
+//  2. When pool.height advances past a pruned peer's base, maxPeerHeight is re-evaluated.
+func TestBlockPoolMaxPeerHeightRefreshesOnPopRequest(t *testing.T) {
+	requestsCh := make(chan BlockRequest, 10)
+	errorsCh := make(chan peerError, 10)
+
+	pool := NewBlockPool(10, requestsCh, errorsCh)
+	pool.SetLogger(log.TestingLogger())
+
+	// Peer A's range covers pool.height, so it contributes to maxPeerHeight.
+	pool.SetPeerRange(p2p.ID("A"), 1, 20)
+	// Peer B is pruned ahead of pool.height and must be excluded until the
+	// pool advances past its base.
+	pool.SetPeerRange(p2p.ID("B"), 15, 100)
+	require.EqualValues(t, 20, pool.MaxPeerHeight(),
+		"peer B is pruned ahead of pool.height and must not contribute yet")
+
+	// Advance pool.height from 10 to 15 via PopRequest. Install a dummy
+	// requester at each height so PopRequest has something to pop; the
+	// requester is never started, so Stop() is a logged no-op.
+	for h := int64(10); h < 15; h++ {
+		pool.mtx.Lock()
+		pool.requesters[h] = newBPRequester(pool, h)
+		pool.mtx.Unlock()
+		pool.PopRequest()
+	}
+
+	// pool.height is now 15, so B (base=15) becomes eligible and must lift
+	// maxPeerHeight to its advertised height without B re-sending status.
+	require.EqualValues(t, 100, pool.MaxPeerHeight(),
+		"peer B must contribute to maxPeerHeight once pool.height reaches its base")
+}