Ignore unknown answers (#831)

Co-authored-by: Tanguy <tanguy@status.im>

libp2p/services/autonatservice.nim

@@ -75,9 +75,11 @@ proc callHandler(self: AutonatService) {.async.} =
 
 proc handleAnswer(self: AutonatService, ans: NetworkReachability) {.async.} =
 
+  if ans == Unknown:
+    return
+
   if self.answers.len == self.maxQueueSize:
     self.answers.popFirst()
-
   self.answers.addLast(ans)
 
   self.networkReachability = Unknown

tests/stubs/autonatstub.nim

@@ -12,11 +12,16 @@ import ../../libp2p/multiaddress
 
 type
   AutonatStub* = ref object of Autonat
-    returnSuccess*: bool
+    answer*: Answer
     dials: int
     expectedDials: int
     finished*: Future[void]
 
+  Answer* = enum
+    Reachable,
+    NotReachable,
+    Unknown
+
 proc new*(T: typedesc[AutonatStub], expectedDials: int): T =
   return T(dials: 0, expectedDials: expectedDials, finished: newFuture[void]())
 
@@ -30,7 +35,10 @@ method dialMe*(
 
     if self.dials == self.expectedDials:
       self.finished.complete()
-    if self.returnSuccess:
+    case self.answer:
-      return MultiAddress.init("/ip4/0.0.0.0/tcp/0").tryGet()
+      of Reachable:
-    else:
+        return MultiAddress.init("/ip4/0.0.0.0/tcp/0").tryGet()
-      raise newException(AutonatUnreachableError, "")
+      of NotReachable:
+        raise newException(AutonatUnreachableError, "")
+      of Unknown:
+        raise newException(AutonatError, "")

tests/testautonatservice.nim

@@ -34,10 +34,10 @@ suite "Autonat Service":
   teardown:
     checkTrackers()
 
-  asyncTest "Autonat Service Private Reachability test":
+  asyncTest "Peer must be not reachable":
 
     let autonatStub = AutonatStub.new(expectedDials = 3)
-    autonatStub.returnSuccess = false
+    autonatStub.answer = NotReachable
 
     let autonatService = AutonatService.new(autonatStub, newRng())
 
@@ -65,10 +65,10 @@ suite "Autonat Service":
     await allFuturesThrowing(
       switch1.stop(), switch2.stop(), switch3.stop(), switch4.stop())
 
-  asyncTest "Autonat Service Public Reachability test":
+  asyncTest "Peer must be reachable":
 
     let autonatStub = AutonatStub.new(expectedDials = 3)
-    autonatStub.returnSuccess = true
+    autonatStub.answer = Reachable
 
     let autonatService = AutonatService.new(autonatStub, newRng(), some(1.seconds))
 
@@ -96,10 +96,10 @@ suite "Autonat Service":
     await allFuturesThrowing(
       switch1.stop(), switch2.stop(), switch3.stop(), switch4.stop())
 
-  asyncTest "Autonat Service Full Reachability test":
+  asyncTest "Peer must be not reachable and then reachable":
 
     let autonatStub = AutonatStub.new(expectedDials = 6)
-    autonatStub.returnSuccess = false
+    autonatStub.answer = NotReachable
 
     let autonatService = AutonatService.new(autonatStub, newRng(), some(1.seconds))
 
@@ -113,7 +113,7 @@ suite "Autonat Service":
     proc statusAndConfidenceHandler(networkReachability: NetworkReachability, confidence: Option[float]) {.gcsafe, async.} =
       if networkReachability == NetworkReachability.NotReachable and confidence.isSome() and confidence.get() >= 0.3:
         if not awaiter.finished:
-          autonatStub.returnSuccess = true
+          autonatStub.answer = Reachable
           awaiter.complete()
 
     check autonatService.networkReachability() == NetworkReachability.Unknown
@@ -141,15 +141,60 @@ suite "Autonat Service":
 
     await allFuturesThrowing(switch1.stop(), switch2.stop(), switch3.stop(), switch4.stop())
 
-asyncTest "Autonat Service setup and stop twice":
+  asyncTest "Unknown answers must be ignored":
 
-  let switch = createSwitch()
+    let autonatStub = AutonatStub.new(expectedDials = 6)
-  let autonatService = AutonatService.new(AutonatStub.new(expectedDials = 0), newRng(), some(1.seconds))
+    autonatStub.answer = NotReachable
 
-  check (await autonatService.setup(switch)) == true
+    let autonatService = AutonatService.new(autonatStub, newRng(), some(1.seconds), maxQueueSize = 3)
-  check (await autonatService.setup(switch)) == false
 
-  check (await autonatService.stop(switch)) == true
+    let switch1 = createSwitch(autonatService)
-  check (await autonatService.stop(switch)) == false
+    let switch2 = createSwitch()
+    let switch3 = createSwitch()
+    let switch4 = createSwitch()
 
-  await allFuturesThrowing(switch.stop())
+    let awaiter = newFuture[void]()
+
+    proc statusAndConfidenceHandler(networkReachability: NetworkReachability, confidence: Option[float]) {.gcsafe, async.} =
+      if networkReachability == NetworkReachability.NotReachable and confidence.isSome() and confidence.get() >= 0.3:
+        if not awaiter.finished:
+          autonatStub.answer = Unknown
+          awaiter.complete()
+
+    check autonatService.networkReachability() == NetworkReachability.Unknown
+
+    autonatService.statusAndConfidenceHandler(statusAndConfidenceHandler)
+
+    await switch1.start()
+    await switch2.start()
+    await switch3.start()
+    await switch4.start()
+
+    await switch1.connect(switch2.peerInfo.peerId, switch2.peerInfo.addrs)
+    await switch1.connect(switch3.peerInfo.peerId, switch3.peerInfo.addrs)
+    await switch1.connect(switch4.peerInfo.peerId, switch4.peerInfo.addrs)
+
+    await awaiter
+
+    check autonatService.networkReachability() == NetworkReachability.NotReachable
+    check libp2p_autonat_reachability_confidence.value(["NotReachable"]) == 1/3
+
+    await autonatStub.finished
+
+    check autonatService.networkReachability() == NetworkReachability.NotReachable
+    check libp2p_autonat_reachability_confidence.value(["NotReachable"]) == 1/3
+
+    await allFuturesThrowing(switch1.stop(), switch2.stop(), switch3.stop(), switch4.stop())
+
+  asyncTest "Calling setup and stop twice must work":
+
+    let switch = createSwitch()
+    let autonatService = AutonatService.new(AutonatStub.new(expectedDials = 0), newRng(), some(1.seconds))
+
+    check (await autonatService.setup(switch)) == true
+    check (await autonatService.setup(switch)) == false
+
+    check (await autonatService.stop(switch)) == true
+    check (await autonatService.stop(switch)) == false
+
+    await allFuturesThrowing(switch.stop())