Refactor the findNode code + add tests

eth/p2p/discoveryv5/node.nim

@@ -51,6 +51,9 @@ proc `==`*(a, b: Node): bool =
   (a.isNil and b.isNil) or
     (not a.isNil and not b.isNil and a.pubkey == b.pubkey)
 
+proc `$`*(id: NodeId): string =
+  id.toHex()
+
 proc `$`*(a: Address): string =
   result.add($a.ip)
   result.add(":" & $a.port)

eth/p2p/discoveryv5/protocol.nim

@@ -508,15 +508,47 @@ proc waitMessage(d: Protocol, fromNode: Node, reqId: RequestId):
       res.complete(none(Message)) # TODO: raises: [Exception]
   d.awaitedMessages[key] = result
 
-proc addNodesFromENRs(result: var seq[Node], enrs: openarray[Record])
-    {.raises: [Defect].} =
+proc verifyNodesRecords*(enrs: openarray[Record], fromNode: Node,
+    distance: uint32): seq[Node] {.raises: [Defect].} =
+  ## Verify and convert ENRs to a sequence of nodes. Only ENRs that pass
+  ## verification will be added. ENRs are verified for duplicates, invalid
+  ## addresses and invalid distances.
+  # TODO:
+  # - Should we fail and ignore values on first invalid Node?
+  # - Should we limit the amount of nodes? The discovery v5 specification holds
+  # no limit on the amount that can be returned.
+  var seen: HashSet[Node]
   for r in enrs:
     let node = newNode(r)
     if node.isOk():
-      result.add(node[])
+      let n = node.get()
+      # Check for duplicates in the nodes reply. Duplicates are checked based
+      # on node id.
+      if n in seen:
+        trace "Nodes reply contained records with duplicate node ids",
+          record = n.record.toURI, sender = fromNode.record.toURI, id = n.id
+        continue
+      # Check if the node has an address and if the address is public or from
+      # the same local network or lo network as the sender. The latter allows
+      # for local testing.
+      if not n.address.isSome() or not
+          validIp(fromNode.address.get().ip, n.address.get().ip):
+        trace "Nodes reply contained record with invalid ip-address",
+          record = n.record.toURI, sender = fromNode.record.toURI, node = $n
+        continue
+      # Check if returned node has the requested distance.
+      if logDist(n.id, fromNode.id) != min(distance, 256):
+        warn "Nodes reply contained record with incorrect distance",
+          record = n.record.toURI, sender = fromNode.record.toURI
+        continue
+      # No check on UDP port and thus any port is allowed, also the so called
+      # "well-known" ports.
+
+      seen.incl(n)
+      result.add(n)
 
 proc waitNodes(d: Protocol, fromNode: Node, reqId: RequestId):
-    Future[DiscResult[seq[Node]]] {.async, raises: [Exception, Defect].} =
+    Future[DiscResult[seq[Record]]] {.async, raises: [Exception, Defect].} =
   ## Wait for one or more nodes replies.
   ##
   ## The first reply will hold the total number of replies expected, and based
@@ -525,13 +557,12 @@ proc waitNodes(d: Protocol, fromNode: Node, reqId: RequestId):
   ## Same counts for out of order receival.
   var op = await d.waitMessage(fromNode, reqId)
   if op.isSome and op.get.kind == nodes:
-    var res = newSeq[Node]()
-    res.addNodesFromENRs(op.get.nodes.enrs)
+    var res = op.get.nodes.enrs
     let total = op.get.nodes.total
     for i in 1 ..< total:
       op = await d.waitMessage(fromNode, reqId)
       if op.isSome and op.get.kind == nodes:
-        res.addNodesFromENRs(op.get.nodes.enrs)
+        res.add(op.get.nodes.enrs)
       else:
         # No error on this as we received some nodes.
         break
@@ -577,33 +608,7 @@ proc findNode*(d: Protocol, toNode: Node, distance: uint32):
   let nodes = await d.waitNodes(toNode, reqId)
 
   if nodes.isOk:
-    var res = newSeq[Node]()
-    var seen: HashSet[Node]
-    for n in nodes[]:
-      # Check for duplicates in the nodes reply.
-      if n in seen:
-        trace "Nodes reply contained duplicate record",
-          record = n.record.toURI, sender = toNode.record.toURI
-        continue
-      # Check if the node has an address and if the address is public or from
-      # the same local network or lo network as the sender. The latter allows
-      # for local testing.
-      if not n.address.isSome() or not
-          validIp(toNode.address.get().ip, n.address.get().ip):
-        trace "Nodes reply contained record with invalid ip-address",
-          record = n.record.toURI, sender = toNode.record.toURI
-        continue
-      # Check if returned node has the requested distance.
-      if logDist(n.id, toNode.id) != min(distance, 256):
-        warn "Nodes reply contained record with incorrect distance",
-          record = n.record.toURI, sender = toNode.record.toURI
-        continue
-      # No check on UDP port and thus any port is allowed, also the so called
-      # "well-known" ports.
-
-      seen.incl(n)
-      res.add(n)
-
+    let res = verifyNodesRecords(nodes.get(), toNode, distance)
     d.routingTable.setJustSeen(toNode)
     return ok(res)
   else:

tests/p2p/test_discoveryv5.nim

@@ -418,3 +418,63 @@ procSuite "Discovery v5 Tests":
       let incorrectKeyUpdates = newProtocol(PrivateKey.random(rng[]),
         db, ip, port, port, rng = rng,
         previousRecord = some(updatesNode.getRecord()))
+
+  test "Verify records of nodes message":
+    let
+      port = Port(9000)
+      fromNoderecord = enr.Record.init(1, PrivateKey.random(rng[]),
+        some(ValidIpAddress.init("11.12.13.14")),
+        port, port)[]
+      fromNode = newNode(fromNoderecord)[]
+      pk = PrivateKey.random(rng[])
+      targetDistance = logDist(fromNode.id, pk.toPublicKey().toNodeId())
+
+    block: # Duplicates
+      let
+        record = enr.Record.init(
+          1, pk, some(ValidIpAddress.init("12.13.14.15")), port, port)[]
+
+      # Exact duplicates
+      var records = @[record, record]
+      var nodes = verifyNodesRecords(records, fromNode, targetDistance)
+      check nodes.len == 1
+
+      # Node id duplicates
+      let recordSameId = enr.Record.init(
+        1, pk, some(ValidIpAddress.init("212.13.14.15")), port, port)[]
+      records.add(recordSameId)
+      nodes = verifyNodesRecords(records, fromNode, targetDistance)
+      check nodes.len == 1
+
+    block: # No address
+      let
+        recordNoAddress = enr.Record.init(
+          1, pk, none(ValidIpAddress), port, port)[]
+        records = [recordNoAddress]
+        test = verifyNodesRecords(records, fromNode, targetDistance)
+      check test.len == 0
+
+    block: # Invalid address - site local
+      let
+        recordInvalidAddress = enr.Record.init(
+          1, pk, some(ValidIpAddress.init("10.1.2.3")),
+          port, port)[]
+        records = [recordInvalidAddress]
+        test = verifyNodesRecords(records, fromNode, targetDistance)
+      check test.len == 0
+
+    block: # Invalid address - loopback
+      let
+        recordInvalidAddress = enr.Record.init(
+          1, pk, some(ValidIpAddress.init("127.0.0.1")), port, port)[]
+        records = [recordInvalidAddress]
+        test = verifyNodesRecords(records, fromNode, targetDistance)
+      check test.len == 0
+
+    block: # Invalid distance
+      let
+        recordInvalidDistance = enr.Record.init(
+          1, pk, some(ValidIpAddress.init("12.13.14.15")), port, port)[]
+        records = [recordInvalidDistance]
+        test = verifyNodesRecords(records, fromNode, 0)
+      check test.len == 0