autonat: don't emit reachability changed events on address change (#2092

) * don't emit events unless reachability changed * remove nil address check * fix test assertion * speed up test --------- Co-authored-by: Marten Seemann <[email protected]>
libp2p · Feb 15, 2023 · 84f2278 · 84f2278
1 parent 6e9dd01
commit 84f2278
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 27 deletions.
diff --git a/p2p/host/autonat/autonat.go b/p2p/host/autonat/autonat.go
@@ -286,10 +286,13 @@ func (as *AmbientAutoNAT) scheduleProbe() time.Duration {
 // Update the current status based on an observed result.
 func (as *AmbientAutoNAT) recordObservation(observation autoNATResult) {
 	currentStatus := as.status.Load()
+
 	if observation.Reachability == network.ReachabilityPublic {
 		log.Debugf("NAT status is public")
 		changed := false
 		if currentStatus.Reachability != network.ReachabilityPublic {
+			// Aggressively switch to public from other states ignoring confidence
+
 			// we are flipping our NATStatus, so confidence drops to 0
 			as.confidence = 0
 			if as.service != nil {
@@ -299,21 +302,13 @@ func (as *AmbientAutoNAT) recordObservation(observation autoNATResult) {
 		} else if as.confidence < 3 {
 			as.confidence++
 		}
-		if observation.address != nil {
-			if !changed && currentStatus.address != nil && !observation.address.Equal(currentStatus.address) {
-				as.confidence--
-			}
-			if currentStatus.address == nil || !observation.address.Equal(currentStatus.address) {
-				changed = true
-			}
-			as.status.Store(&observation)
-		}
-		if observation.address != nil && changed {
+		as.status.Store(&observation)
+		if changed {
 			as.emitStatus()
 		}
 	} else if observation.Reachability == network.ReachabilityPrivate {
 		log.Debugf("NAT status is private")
-		if currentStatus.Reachability == network.ReachabilityPublic {
+		if currentStatus.Reachability != network.ReachabilityPrivate {
 			if as.confidence > 0 {
 				as.confidence--
 			} else {
@@ -328,9 +323,6 @@ func (as *AmbientAutoNAT) recordObservation(observation autoNATResult) {
 		} else if as.confidence < 3 {
 			as.confidence++
 			as.status.Store(&observation)
-			if currentStatus.Reachability != network.ReachabilityPrivate {
-				as.emitStatus()
-			}
 		}
 	} else if as.confidence > 0 {
 		// don't just flip to unknown, reduce confidence first

diff --git a/p2p/host/autonat/autonat_test.go b/p2p/host/autonat/autonat_test.go
@@ -209,19 +209,6 @@ func TestAutoNATObservationRecording(t *testing.T) {
 		t.Fatalf("failed to subscribe to event EvtLocalRoutabilityPublic, err=%s", err)
 	}
 
-	// pubic observation without address should be ignored.
-	an.recordObservation(autoNATResult{network.ReachabilityPublic, nil})
-	if an.Status() != network.ReachabilityUnknown {
-		t.Fatalf("unexpected transition")
-	}
-
-	select {
-	case <-s.Out():
-		t.Fatal("not expecting a public reachability event")
-	default:
-		// expected
-	}
-
 	addr, _ := ma.NewMultiaddr("/ip4/127.0.0.1/udp/1234")
 	an.recordObservation(autoNATResult{network.ReachabilityPublic, addr})
 	if an.Status() != network.ReachabilityPublic {
@@ -252,6 +239,17 @@ func TestAutoNATObservationRecording(t *testing.T) {
 		t.Fatalf("too-extreme private transition.")
 	}
 
+	// don't emit events on observed address change
+	newAddr, _ := ma.NewMultiaddr("/ip4/127.0.0.1/udp/12345")
+	an.recordObservation(autoNATResult{network.ReachabilityPublic, newAddr})
+	if an.Status() != network.ReachabilityPublic {
+		t.Fatalf("reachability should stay public")
+	}
+	select {
+	case <-s.Out():
+		t.Fatal("received event without state transition")
+	case <-time.After(300 * time.Millisecond):
+	}
 }
 
 func TestStaticNat(t *testing.T) {