wip

ipn/ipnlocal/local.go

@@ -6373,11 +6373,11 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand
 	candidatesByRegion := make(map[int][]tailcfg.NodeView, len(netMap.DERPMap.Regions))
 	var preferredDERP *tailcfg.DERPRegion = netMap.DERPMap.Regions[report.PreferredDERP]
 	var minDistance float64 = math.MaxFloat64
-	type candidateDistance struct {
+	type nodeDistance struct {
 		nv       tailcfg.NodeView
 		distance float64 // in meters, approximately
 	}
-	candidateDistances := make([]candidateDistance, len(candidates))
+	distances := make([]nodeDistance, len(candidates))
 	for _, c := range candidates {
 		if !c.Valid() {
 			continue
@@ -6397,9 +6397,12 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand
 			}
 			continue
 		}
+		if len(candidatesByRegion) > 0 {
+			// Since a candidate exists that does have a DERP home, skip this candidate. We never select
+			// a candidate without a DERP home if there is a candidate available with a DERP home.
+			continue
+		}
 		// This candidate does not have a DERP home.
-		// Since a candidate exists that does have a DERP home, skip this candidate. We never select
-		// a candidate without a DERP home if there is a candidate available with a DERP home.
 		// Use geographic distance from our DERP home to estimate how good this candidate is.
 		hi := c.Hostinfo()
 		if !hi.Valid() {
@@ -6409,11 +6412,11 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand
 		if loc == nil {
 			continue
 		}
-		distance := longLatDistance(preferredDERP.Latitude, preferredDERP.Longitude, c.Hostinfo().Location().Latitude, c.Hostinfo().Location().Longitude)
+		distance := longLatDistance(preferredDERP.Latitude, preferredDERP.Longitude, loc.Latitude, loc.Longitude)
 		if distance < minDistance {
 			minDistance = distance
 		}
-		candidateDistances = append(candidateDistances, candidateDistance{nv: c, distance: distance})
+		distances = append(distances, nodeDistance{nv: c, distance: distance})
 	}
 	// First, try to select an exit node that has the closest DERP home, based on lastReport's DERP latency.
 	if len(candidatesByRegion) > 0 {
@@ -6433,10 +6436,17 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand
 		return res, nil
 	}
 	// None of the candidates have a DERP home, so proceed to select based on geographical distance from our preferred DERP region.
-	const minDistanceEpsilon = 100000 // consider peers at most 100km further away than the minimum distance found
+	// allowanceMeters is the extra distance that will be permitted when considering peers. By this point, there
+	// are multiple approximations taking place (DERP location standing in for this device's location, the peer's
+	// location may only be city granularity, the distance algorithm assumes a spherical planet, etc.) so it is
+	// reasonable to consider peers that are similar distances. Those peers are good enough to be within
+	// measurement error. 100km corresponds to approximately 1ms of additional round trip light
+	// propagation delay in a fiber optic cable and seems like a reasonable heuristic. It may be adjusted in
+	// future.
+	const allowanceMeters = 100000 // consider peers at most 100km further away than the minimum distance found
 	var pickFrom []tailcfg.NodeView
-	for _, candidate := range candidateDistances {
-		if candidate.nv.Valid() && candidate.distance <= minDistance+minDistanceEpsilon {
+	for _, candidate := range distances {
+		if candidate.nv.Valid() && candidate.distance <= minDistance+allowanceMeters {
 			pickFrom = append(pickFrom, candidate.nv)
 		}
 	}
@@ -6482,7 +6492,10 @@ func pickDERPNode(nodes []tailcfg.NodeView, r *rand.Rand) tailcfg.NodeView {
 // If there are no latency values, it returns 0.
 func minLatencyDERPRegion(regions []int, report *netcheck.Report) int {
 	return slices.MinFunc(regions, func(i, j int) int {
-		return cmp.Compare(report.RegionLatency[i], report.RegionLatency[j])
+		if c := cmp.Compare(report.RegionLatency[i], report.RegionLatency[j]); c != 0 {
+			return c
+		}
+		return cmp.Compare(i, j)
 	})
 }
 

ipn/ipnlocal/local_test.go

@@ -3197,8 +3197,20 @@ func TestSuggestExitNode(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			r := rand.New(rand.NewSource(100))
 			got, err := suggestExitNode(&tt.lastReport, &tt.netMap, r)
-			if !reflect.DeepEqual(got.Name, tt.wantName) || !reflect.DeepEqual(got.ID, tt.wantID) || !errors.Is(err, tt.wantError) || !reflect.DeepEqual(got.Location, tt.wantLocation) {
-				t.Errorf("got name %v id %v location %v error %v want name %v id %v location %v error %v", got.Name, got.ID, got.Location, err, tt.wantName, tt.wantID, tt.wantLocation, tt.wantError)
+			if got.Name != tt.wantName {
+				t.Errorf("name=%v, want %v", got.Name, tt.wantName)
+			}
+			if got.ID != tt.wantID {
+				t.Errorf("ID=%v, want %v", got.ID, tt.wantID)
+			}
+			if tt.wantError == nil && err != nil {
+				t.Errorf("err=%v, want no error", err)
+			}
+			if tt.wantError != nil && !errors.Is(err, tt.wantError) {
+				t.Errorf("err=%v, want %v", err, tt.wantError)
+			}
+			if !reflect.DeepEqual(got.Location, tt.wantLocation) {
+				t.Errorf("location=%v, want %v", got.Location, tt.wantLocation)
 			}
 		})
 	}
@@ -3331,7 +3343,7 @@ func TestSuggestExitNodeLongLatDistance(t *testing.T) {
 			fromLong:      -73.935242,
 			toLat:         37.7749,
 			toLong:        -122.4194,
-			distanceValue: 4132000,
+			distanceValue: 4134926,
 		},
 		{
 			name:          "valid values, locations in north and south of equator",
@@ -3343,11 +3355,11 @@ func TestSuggestExitNodeLongLatDistance(t *testing.T) {
 		},
 	}
 	// Check if the absolute value of the difference between the calculated distance value
-	// and the hardcoded distance value is greater than 100km.
+	// and the hardcoded distance value is greater than 1km.
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			got := longLatDistance(tt.fromLat, tt.fromLong, tt.toLat, tt.toLong)
-			if math.Abs(got-tt.distanceValue) > 100000 {
+			if math.Abs(got-tt.distanceValue) > 1000 {
 				t.Errorf("got value %v distance value %v absolute value difference %v", got, tt.distanceValue, math.Abs(got-tt.distanceValue))
 			}
 		})