Don't send Whoareyou on handshake failure

eth/p2p/discoveryv5/encoding.nim

@@ -34,6 +34,11 @@ type
 
   RandomSourceDepleted* = object of CatchableError
 
+  DecodeStatus* = enum
+    Success,
+    HandshakeError,
+    PacketError
+
 const
   gcmTagSize = 16
 
@@ -221,7 +226,7 @@ proc decodeEncrypted*(c: var Codec,
                       input: seq[byte],
                       authTag: var array[12, byte],
                       newNode: var Node,
-                      packet: var Packet): bool =
+                      packet: var Packet): DecodeStatus =
   let input = input.toRange
   var r = rlpFromBytes(input[32 .. ^1])
   var auth: AuthHeader
@@ -230,24 +235,22 @@ proc decodeEncrypted*(c: var Codec,
 
   if r.isList:
     # Handshake - rlp list indicates auth-header
-
-    # TODO: Auth failure will result in resending whoareyou. Do we really want this?
     auth = r.read(AuthHeader)
     authTag = auth.auth
 
     let challenge = c.handshakes.getOrDefault($fromId)
     if challenge.isNil:
       trace "Decoding failed (no challenge)"
-      return false
+      return HandshakeError
 
     if auth.idNonce != challenge.idNonce:
       trace "Decoding failed (different nonce)"
-      return false
+      return HandshakeError
 
     var sec: HandshakeSecrets
     if not c.decodeAuthResp(fromId, auth, challenge, sec, newNode):
       trace "Decoding failed (bad auth)"
-      return false
+      return HandshakeError
     c.handshakes.del($fromId)
 
     # Swap keys to match remote
@@ -263,20 +266,23 @@ proc decodeEncrypted*(c: var Codec,
     var writeKey: array[16, byte]
     if not c.db.loadKeys(fromId, fromAddr, readKey, writeKey):
       trace "Decoding failed (no keys)"
-      return false
+      return PacketError
       # doAssert(false, "TODO: HANDLE ME!")
 
   let headSize = 32 + r.position
   let bodyEnc = input[headSize .. ^1]
 
-  let body = decryptGCM(readKey, auth.auth, bodyEnc.toOpenArray, input[0 .. 31].toOpenArray)
+  let body = decryptGCM(readKey, auth.auth, bodyEnc.toOpenArray,
+    input[0 .. 31].toOpenArray)
   if body.len > 1:
     let status = decodePacketBody(body[0], body.toOpenArray(1, body.high), packet)
     if status == decodingSuccessful:
-      return true
+      return Success
     else:
       debug "Failed to decode discovery packet", reason = status
-      return false
+      return PacketError
+  else:
+    return PacketError
 
 proc newRequestId*(): RequestId =
   if randomBytes(addr result, sizeof(result)) != sizeof(result):

eth/p2p/discoveryv5/protocol.nim

@@ -90,6 +90,10 @@ proc sendWhoareyou(d: Protocol, address: Address, toNode: NodeId, authTag: array
   trace "sending who are you", to = $toNode, toAddress = $address
   let challenge = Whoareyou(authTag: authTag, recordSeq: 1)
   randomBytes(challenge.idNonce)
+  # In case a handshake is already going on for this node, this will overwrite
+  # that one and an incoming response will fail.
+  # TODO: What is the better approach, overwrite or keep the first one until
+  # purged due to timeout (or keep both by using toNode + idNonce as key)?
   d.codec.handshakes[$toNode] = challenge
   var data = @(whoareyouMagic(toNode))
   data.add(rlp.encode(challenge[]))
@@ -151,6 +155,7 @@ proc receive(d: Protocol, a: Address, msg: Bytes) {.gcsafe,
   # debug "Packet received: ", length = msg.len
 
   if d.isWhoAreYou(msg):
+    trace "Received whoareyou", localNode = d.localNode, address = a
     let whoareyou = d.decodeWhoAreYou(msg)
     var pr: PendingRequest
     if d.pendingRequests.take(whoareyou.authTag, pr):
@@ -171,12 +176,12 @@ proc receive(d: Protocol, a: Address, msg: Bytes) {.gcsafe,
     var authTag: array[12, byte]
     var node: Node
     var packet: Packet
-
+    let decoded = d.codec.decodeEncrypted(sender, a, msg, authTag, node, packet)
-    if d.codec.decodeEncrypted(sender, a, msg, authTag, node, packet):
+    if decoded == DecodeStatus.Success:
       if node.isNil:
         node = d.routingTable.getNode(sender)
       else:
-        debug "Adding new node to routing table"
+        debug "Adding new node to routing table", node, localNode = d.localNode
         discard d.routingTable.addNode(node)
 
       doAssert(not node.isNil, "No node in the routing table (internal error?)")
@@ -192,10 +197,11 @@ proc receive(d: Protocol, a: Address, msg: Bytes) {.gcsafe,
           waiter.complete(packet.some)
         else:
           debug "TODO: handle packet: ", packet = packet.kind, origin = $node
-
+    elif decoded == DecodeStatus.PacketError:
-    else:
+      debug "Could not decode packet, respond with whoareyou",
-      debug "Could not decode, respond with whoareyou"
+        localNode = d.localNode, address = a
       d.sendWhoareyou(a, sender, authTag)
+      # No Whoareyou in case it is a Handshake Failure
 
 proc waitPacket(d: Protocol, fromNode: Node, reqId: RequestId): Future[Option[Packet]] =
   result = newFuture[Option[Packet]]("waitPacket")
@@ -277,13 +283,13 @@ proc lookup*(p: Protocol, target: NodeId): Future[seq[Node]] {.async.} =
         pendingQueries.add(p.lookupWorker(n, target))
       inc i
 
-    debug "discv5 pending queries", total = pendingQueries.len
+    trace "discv5 pending queries", total = pendingQueries.len
 
     if pendingQueries.len == 0:
       break
 
     let idx = await oneIndex(pendingQueries)
-    debug "Got discv5 lookup response", idx
+    trace "Got discv5 lookup response", idx
 
     let nodes = pendingQueries[idx].read
     pendingQueries.del(idx)

tests/p2p/test_discoveryv5.nim

@@ -56,7 +56,7 @@ suite "Discovery v5 Tests":
 
   asyncTest "Lookup targets":
     const
-      nodeCount = 5
+      nodeCount = 17
 
     let
       bootNodeKey = newPrivateKey()
@@ -69,10 +69,6 @@ suite "Discovery v5 Tests":
       nodes.add(initDiscoveryNode(newPrivateKey(), localAddress(20301 + i),
         @[bootNode.localNode.record]))
 
-    # Make sure all random lookups ran once (not guaranteed with the loops)
-    for node in nodes:
-      let discovered = await node.lookupRandom()
-
     for i in 0..<nodeCount-1:
       let target = nodes[i]
       let discovered = await nodes[nodeCount-1].lookup(target.localNode.id)