Don't ban nodes on timeout.

eth/p2p/discoveryv5/protocol.nim

@@ -126,7 +126,6 @@ const
 
   ## Ban durations for banned nodes in the routing table
   NodeBanDurationInvalidResponse = 15.minutes
-  NodeBanDurationNoResponse = 5.minutes
 
 type
   OptAddress* = object
@@ -438,6 +437,15 @@ proc sendWhoareyou(d: Protocol, toId: NodeId, a: Address,
   else:
     debug "Node with this id already has ongoing handshake, ignoring packet"
 
+proc replaceNode(d: Protocol, n: Node) =
+  if n.record notin d.bootstrapRecords:
+    d.routingTable.replaceNode(n)
+  else:
+    # For now we never remove bootstrap nodes. It might make sense to actually
+    # do so and to retry them only in case we drop to a really low amount of
+    # peers in the routing table.
+    debug "Message request to bootstrap node failed", enr = toURI(n.record)
+
 proc banNode(d: Protocol, n: Node, banPeriod: chronos.Duration) =
   if n.record notin d.bootstrapRecords:
     if d.banNodes:
@@ -575,7 +583,7 @@ proc waitNodes(d: Protocol, fromNode: Node, reqId: RequestId):
       discovery_message_requests_outgoing.inc(labelValues = ["invalid_response"])
       return err("Invalid response to find node message")
   else:
-    d.banNode(fromNode, NodeBanDurationNoResponse)
+    d.replaceNode(fromNode)
     discovery_message_requests_outgoing.inc(labelValues = ["no_response"])
     return err("Nodes message not received in time")
 
@@ -618,7 +626,7 @@ proc ping*(d: Protocol, toNode: Node):
       discovery_message_requests_outgoing.inc(labelValues = ["invalid_response"])
       return err("Invalid response to ping message")
   else:
-    d.banNode(toNode, NodeBanDurationNoResponse)
+    d.replaceNode(toNode)
     discovery_message_requests_outgoing.inc(labelValues = ["no_response"])
     return err("Pong message not received in time")
 
@@ -664,7 +672,7 @@ proc talkReq*(d: Protocol, toNode: Node, protocol, request: seq[byte]):
       discovery_message_requests_outgoing.inc(labelValues = ["invalid_response"])
       return err("Invalid response to talk request message")
   else:
-    d.banNode(toNode, NodeBanDurationNoResponse)
+    d.replaceNode(toNode)
     discovery_message_requests_outgoing.inc(labelValues = ["no_response"])
     return err("Talk response message not received in time")
 

tests/p2p/test_discoveryv5_bannodes.nim

@@ -225,7 +225,7 @@ suite "Discovery v5 Ban Nodes Enabled Tests":
         n.isSome()
         n.get().id == targetId
         n.get().record.seqNum == targetSeqNum
-    # Node will be banned because of failed findNode request.
+    # Node will be removed because of failed findNode request.
 
     # Bring target back online, update seqNum in ENR, check if we get the
     # updated ENR.
@@ -236,7 +236,9 @@ suite "Discovery v5 Ban Nodes Enabled Tests":
       # session will still be in the LRU cache.
       let nodes = await mainNode.findNode(targetNode.localNode, @[0'u16])
       check:
-        nodes.isErr() # Node is banned
+        nodes.isOk()
+        nodes[].len == 1
+        mainNode.addNode(nodes[][0])
 
       targetSeqNum.inc()
       # need to add something to get the enr sequence number incremented
@@ -245,15 +247,42 @@ suite "Discovery v5 Ban Nodes Enabled Tests":
 
       var n = mainNode.getNode(targetId)
       check:
-        n.isNone() # Node was removed when banned
+        n.isSome()
+        n.get().id == targetId
+        n.get().record.seqNum == targetSeqNum - 1
 
       n = await mainNode.resolve(targetId)
       check:
-        n.isNone() # Node is banned
+        n.isSome()
+        n.get().id == targetId
+        n.get().record.seqNum == targetSeqNum
+
+      # Add the updated version
+      discard mainNode.addNode(n.get())
+
+    # Update seqNum in ENR again, ping lookupNode to be added in routing table,
+    # close targetNode, resolve should lookup, check if we get updated ENR.
+    block:
+      targetSeqNum.inc()
+      let update = targetNode.updateRecord({"addsomefield": @[byte 2]})
+      check update.isOk()
+
+      # ping node so that its ENR gets added
+      check (await targetNode.ping(lookupNode.localNode)).isOk()
+      # ping node so that it becomes "seen" and thus will be forwarded on a
+      # findNode request
+      check (await lookupNode.ping(targetNode.localNode)).isOk()
+      await targetNode.closeWait()
+
+      check mainNode.addNode(lookupNode.localNode.record)
+      let n = await mainNode.resolve(targetId)
+      check:
+        n.isSome()
+        n.get().id == targetId
+        n.get().record.seqNum == targetSeqNum
 
     await mainNode.closeWait()
     await lookupNode.closeWait()
-    await targetNode.closeWait()
 
   asyncTest "Random nodes with enr field filter":
     let