autonat: don't emit reachability changed events on address change #2092
Conversation
sukunrt
changed the title
sukunrt
force-pushed
the
fix-autonat-event
branch
from
97c8a2b
to
e7c7980
Compare
sukunrt
force-pushed
the
fix-autonat-event
branch
from
e7c7980
to
d3d3173
Compare
p2p/host/autonat/autonat.go
Outdated
| @@ -286,10 +286,18 @@ 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() | |||
|
|
|||
| // Ignore public observations with no address | |||
| if observation.Reachability == network.ReachabilityPublic && observation.address == nil { | |||
There was a problem hiding this comment.
If I understand correctly, this can't really happen. A successful observation will always contain a multiaddress, and if the peer sends a multiaddress that we can't decode, we'd record this as a ReachabilityUnknown observation. We shouldn't do that either, we should discard that measurement.
There was a problem hiding this comment.
I checked. It cannot happen. cli.DialBack(ctx, pi.ID) ensures that address is not nil when err is nil.
I added this because TestAutoNATObservationRecording specifically has a check for this case.
I'll remove both.
It's used in the PublicAddr method which is used by host.AllAddrs. |
p2p/host/autonat/autonat_test.go
Outdated
| } | ||
| select { | ||
| case e := <-s.Out(): | ||
| _, ok := e.(event.EvtLocalReachabilityChanged) |
There was a problem hiding this comment.
Nit: remove the ok assertion here. It's ok to panic (especially since it's a test), and would make the test more robust.
There was a problem hiding this comment.
So I removed the assertion completely. The subscription is for reachability change, any event received here is an error in this case.
fixes: #2046
@marten-seemann I've changed: