Update discv5 to use non deprecated ENR calls and simplify code (#710)

And due to avoiding an extra PublicKey.fromRaw call we get a
little performance boost also.

eth/p2p/discoveryv5/encoding.nim

@@ -481,7 +481,7 @@ proc decodeHandshakePacket(c: var Codec, fromAddr: Address, nonce: AESGCMNonce,
       # Signature check of record happens in decode.
       record = Opt.some(rlp.decode(authdata.toOpenArray(recordPos, authdata.high),
         enr.Record))
-    except RlpError, ValueError:
+    except RlpError:
       return err("Invalid encoded ENR")
 
   var pubkey: PublicKey
@@ -490,7 +490,7 @@ proc decodeHandshakePacket(c: var Codec, fromAddr: Address, nonce: AESGCMNonce,
   # need the pubkey and the nodeid
   if record.isSome():
     # Node returned might not have an address or not a valid address.
-    let node = ? newNode(record.get())
+    let node = Node.fromRecord(record.value)
     if node.id != srcId:
       return err("Invalid node id: does not match node id of ENR")
 

eth/p2p/discoveryv5/enr.nim

@@ -57,7 +57,7 @@ type
     ## must remain sorted and without duplicate keys. Use the insert func to
     ## ensure this.
     raw*: seq[byte] ## RLP encoded record
-    publicKey: PublicKey ## Public key of the record
+    publicKey*: PublicKey ## Public key of the record
 
   EnrUri* = distinct string
 
@@ -67,7 +67,7 @@ type
   # gets renamed.
   TypedRecord* = object
     id*: string
-    secp256k1*: Opt[array[33, byte]]
+    secp256k1*: Opt[array[33, byte]] # compressed secp256k1 public key
     ip*: Opt[array[4, byte]]
     ip6*: Opt[array[16, byte]]
     tcp*: Opt[int]
@@ -192,6 +192,7 @@ func makeEnrAux(
     Field(kind: kBytes, bytes: @(pubkey.toRawCompressed()))))
 
   record.raw = ? makeEnrRaw(seqNum, pk, record.pairs)
+  record.publicKey = pubkey
   ok(record)
 
 macro initRecord*(

eth/p2p/discoveryv5/node.nim

@@ -34,29 +34,24 @@ func toNodeId*(pk: PublicKey): NodeId =
   ## Convert public key to a node identifier.
   # Keccak256 hash is used as defined in ENR spec for scheme v4:
   # https://github.com/ethereum/devp2p/blob/master/enr.md#v4-identity-scheme
+  # The raw key used is the uncompressed public key.
   readUintBE[256](keccak256.digest(pk.toRaw()).data)
 
-func newNode*(r: Record): Result[Node, cstring] =
+func fromRecord*(T: type Node, r: Record): T =
   ## Create a new `Node` from a `Record`.
-  # TODO: Handle IPv6
-
-  let pk = r.get(PublicKey)
-  # This check is redundant for a properly created record as the deserialization
-  # of a record will fail at `verifySignature` if there is no public key.
-  if pk.isNone():
-    return err("Could not recover public key from ENR")
-
-  # Also this can not fail for a properly created record as id is checked upon
-  # deserialization.
-  let tr = ? r.toTypedRecord()
+  let tr = TypedRecord.fromRecord(r)
   if tr.ip.isSome() and tr.udp.isSome():
     let a = Address(ip: ipv4(tr.ip.get()), port: Port(tr.udp.get()))
 
-    ok(Node(id: pk.get().toNodeId(), pubkey: pk.get(), record: r,
-       address: Opt.some(a)))
+    Node(id: r.publicKey.toNodeId(), pubkey: r.publicKey, record: r,
+       address: Opt.some(a))
   else:
-    ok(Node(id: pk.get().toNodeId(), pubkey: pk.get(), record: r,
-       address: Opt.none(Address)))
+    Node(id: r.publicKey.toNodeId(), pubkey: r.publicKey, record: r,
+       address: Opt.none(Address))
+
+func newNode*(r: Record): Result[Node, cstring] {.deprecated: "Use TypedRecord.fromRecord instead".} =
+  ## Create a new `Node` from a `Record`.
+  ok(Node.fromRecord(r))
 
 func update*(n: Node, pk: PrivateKey, ip: Opt[IpAddress],
     tcpPort: Opt[Port] = Opt.none(Port),

eth/p2p/discoveryv5/nodes_verification.nim

@@ -56,33 +56,31 @@ proc verifyNodesRecords(
 
     count.inc()
 
-    let node = newNode(r)
-    if node.isOk():
-      let n = node.get()
-      # Check for duplicates in the nodes reply. Duplicates are checked based
-      # on node id.
-      if n in seen:
-        trace "Duplicate node ids", record = n.record.toURI, id = n.id
+    let n = Node.fromRecord(r)
+    # Check for duplicates in the nodes reply. Duplicates are checked based
+    # on node id.
+    if n in seen:
+      trace "Duplicate node ids", record = n.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(src.address.get().ip, n.address.get().ip):
+      trace "Invalid ip-address", record = n.record.toURI, node = n
+      continue
+    # Check if returned node has one of the requested distances.
+    if distances.isSome():
+      # TODO: This is incorrect for custom distances
+      if (not distances.get().contains(logDistance(n.id, src.id))):
+        debug "Incorrect distance", record = n.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(src.address.get().ip, n.address.get().ip):
-        trace "Invalid ip-address", record = n.record.toURI, node = n
-        continue
-      # Check if returned node has one of the requested distances.
-      if distances.isSome():
-        # TODO: This is incorrect for custom distances
-        if (not distances.get().contains(logDistance(n.id, src.id))):
-          debug "Incorrect distance", record = n.record.toURI
-          continue
 
-      # No check on UDP port and thus any port is allowed, also the so called
-      # "well-known" ports.
+    # No check on UDP port and thus any port is allowed, also the so called
+    # "well-known" ports.
 
-      seen.incl(n)
-      result.add(n)
+    seen.incl(n)
+    result.add(n)
 
 proc verifyNodesRecords*(
     enrs: openArray[Record], src: Node, nodesLimit: int): seq[Node] =

eth/p2p/discoveryv5/protocol.nim

@@ -196,19 +196,17 @@ proc addNode*(d: Protocol, r: Record): bool =
   ##
   ## Returns false only if no valid `Node` can be created from the `Record` or
   ## on the conditions of `addNode` from a `Node`.
-  let node = newNode(r)
-  if node.isOk():
-    return d.addNode(node[])
+  d.addNode(Node.fromRecord(r))
 
-proc addNode*(d: Protocol, enr: EnrUri): bool =
+proc addNode*(d: Protocol, uri: string): bool =
   ## Add `Node` from a ENR URI to discovery routing table.
   ##
   ## Returns false if no valid ENR URI, or on the conditions of `addNode` from
   ## an `Record`.
-  var r: Record
-  let res = r.fromURI(enr)
-  if res:
-    return d.addNode(r)
+  let r = enr.Record.fromURI(uri).valueOr:
+    return false
+
+  d.addNode(r)
 
 func getNode*(d: Protocol, id: NodeId): Opt[Node] =
   ## Get the node with id from the routing table.
@@ -1010,7 +1008,7 @@ proc newProtocol*(
     else:
       warn "No external IP provided for the ENR, this node will not be discoverable"
 
-  let node = newNode(record).expect("Properly initialized record")
+  let node = Node.fromRecord(record)
 
   # TODO Consider whether this should be a Defect
   doAssert rng != nil, "RNG initialization failed"
@@ -1069,7 +1067,7 @@ proc newProtocol*(
       warn "No external IP provided for the ENR, this node will not be " &
            "discoverable"
 
-  let node = newNode(record).expect("Properly initialized record")
+  let node = Node.fromRecord(record)
 
   doAssert not(isNil(rng)), "RNG initialization failed"
 

tests/fuzzing/discoveryv5/fuzz_decode_message.nim

@@ -3,14 +3,14 @@ import
   ../../../eth/p2p/discoveryv5/messages_encoding
 
 test:
-  block:
+  block testBlock:
     let decoded = decodeMessage(payload)
 
     if decoded.isOk():
       let message = decoded.get()
       var encoded: seq[byte]
       case message.kind
-      of unused: break
+      of unused: break testBlock
       of ping: encoded = encodeMessage(message.ping, message.reqId)
       of pong: encoded = encodeMessage(message.pong, message.reqId)
       of findNode: encoded = encodeMessage(message.findNode, message.reqId)
@@ -18,7 +18,7 @@ test:
       of talkReq: encoded = encodeMessage(message.talkReq, message.reqId)
       of talkResp: encoded = encodeMessage(message.talkResp, message.reqId)
       of regTopic, ticket, regConfirmation, topicQuery:
-        break
+        break testBlock
 
       # This will hit assert because of issue:
       # https://github.com/status-im/nim-eth/issues/255

tests/fuzzing/discoveryv5/fuzz_decode_packet.nim

@@ -15,12 +15,12 @@ init:
     enrRecA = enr.Record.init(1, privKeyA,
       Opt.some(parseIpAddress("127.0.0.1")), Opt.some(Port(9000)),
       Opt.some(Port(9000))).expect("Properly initialized private key")
-    nodeA = newNode(enrRecA).expect("Properly initialized record")
+    nodeA = Node.fromRecord(enrRecA)
 
     enrRecB = enr.Record.init(1, privKeyB,
       Opt.some(parseIpAddress("127.0.0.1")), Opt.some(Port(9000)),
       Opt.some(Port(9000))).expect("Properly initialized private key")
-    nodeB = newNode(enrRecB).expect("Properly initialized record")
+    nodeB = Node.fromRecord(enrRecB)
 
   var codecB = Codec(localNode: nodeB, privKey: privKeyB,
     sessions: Sessions.init(5))

tests/fuzzing/enr/fuzz_enr.nim

@@ -3,7 +3,7 @@ import
   ../../../eth/rlp, ../../../eth/p2p/discoveryv5/enr
 
 test:
-  block:
+  block testBlock:
     # This is fuzzing the full ENR deserialisation. As ENRs contain a signature
     # this will practically always fail. So the second (encoding) steps will
     # never be reached.
@@ -12,16 +12,16 @@ test:
     let decoded = try: rlp.decode(payload, enr.Record)
                   except RlpError as e:
                         debug "decode failed", err = e.msg
-                        break
+                        break testBlock
                   except ValueError as e:
                         debug "decode failed", err = e.msg
-                        break
+                        break testBlock
 
     let encoded = try: rlp.encode(decoded)
                   except RlpError as e:
                     debug "decode failed", err = e.msg
                     doAssert(false, "decoding worked but encoding failed")
-                    break
+                    break testBlock
     if encoded != payload.toOpenArray(0, encoded.len - 1):
         echo "payload: ", toHex(payload.toOpenArray(0, encoded.len - 1))
         echo "encoded: ", toHex(encoded)

tests/p2p/discv5_test_helper.nim

@@ -53,7 +53,7 @@ func generateNode*(privKey: PrivateKey, port: int = 20302,
   let port = Port(port)
   let enr = enr.Record.init(1, privKey, Opt.some(ip),
     Opt.some(port), Opt.some(port), localEnrFields).expect("Properly initialized private key")
-  result = newNode(enr).expect("Properly initialized node")
+  result = Node.fromRecord(enr)
 
 proc generateNRandomNodes*(rng: var HmacDrbgContext, n: int): seq[Node] =
   var res = newSeq[Node]()

tests/p2p/test_discoveryv5.nim

@@ -520,7 +520,7 @@ suite "Discovery v5 Tests":
       srcRecord = enr.Record.init(1, PrivateKey.random(rng[]),
         Opt.some(parseIpAddress("11.12.13.14")),
         Opt.some(port), Opt.some(port))[]
-      srcNode = newNode(srcRecord)[]
+      srcNode = Node.fromRecord(srcRecord)
       pk = PrivateKey.random(rng[])
       targetDistance = @[logDistance(srcNode.id, pk.toPublicKey().toNodeId())]
       limit = 16
@@ -603,7 +603,7 @@ suite "Discovery v5 Tests":
       srcRecord = enr.Record.init(1, PrivateKey.random(rng[]),
         Opt.some(parseIpAddress("127.0.0.0")),
         Opt.some(port), Opt.some(port))[]
-      srcNode = newNode(srcRecord)[]
+      srcNode = Node.fromRecord(srcRecord)
       pk = PrivateKey.random(rng[])
       targetDistance = @[logDistance(srcNode.id, pk.toPublicKey().toNodeId())]
       limit = 16
@@ -630,7 +630,7 @@ suite "Discovery v5 Tests":
       srcRecord = enr.Record.init(1, PrivateKey.random(rng[]),
         Opt.some(parseIpAddress("192.168.1.1")),
         Opt.some(port), Opt.some(port))[]
-      srcNode = newNode(srcRecord)[]
+      srcNode = Node.fromRecord(srcRecord)
       pk = PrivateKey.random(rng[])
       targetDistance = @[logDistance(srcNode.id, pk.toPublicKey().toNodeId())]
       limit = 16
@@ -693,7 +693,7 @@ suite "Discovery v5 Tests":
         enrRec = enr.Record.init(1, privKey,
           Opt.some(parseIpAddress("127.0.0.1")), Opt.some(Port(9000)),
           Opt.some(Port(9000))).expect("Properly initialized private key")
-        sendNode = newNode(enrRec).expect("Properly initialized record")
+        sendNode = Node.fromRecord(enrRec)
       var codec = Codec(localNode: sendNode, privKey: privKey, sessions: Sessions.init(5))
 
       let (packet, _) = encodeMessagePacket(rng[], codec,
@@ -722,7 +722,7 @@ suite "Discovery v5 Tests":
       enrRec = enr.Record.init(1, privKey,
         Opt.some(parseIpAddress("127.0.0.1")), Opt.some(Port(9000)),
         Opt.some(Port(9000))).expect("Properly initialized private key")
-      sendNode = newNode(enrRec).expect("Properly initialized record")
+      sendNode = Node.fromRecord(enrRec)
     var codec = Codec(localNode: sendNode, privKey: privKey, sessions: Sessions.init(5))
     for i in 0 ..< 5:
       let a = localAddress(20303 + i)
@@ -753,7 +753,7 @@ suite "Discovery v5 Tests":
       enrRec = enr.Record.init(1, privKey,
         Opt.some(parseIpAddress("127.0.0.1")), Opt.some(Port(9000)),
         Opt.some(Port(9000))).expect("Properly initialized private key")
-      sendNode = newNode(enrRec).expect("Properly initialized record")
+      sendNode = Node.fromRecord(enrRec)
     var codec = Codec(localNode: sendNode, privKey: privKey, sessions: Sessions.init(5))
 
     var firstRequestNonce: AESGCMNonce

tests/p2p/test_discoveryv5_encoding.nim

@@ -96,10 +96,14 @@ suite "Discovery v5.1 Protocol Message Encodings":
       message.nodes.enrs.len() == 0
 
   test "Nodes Response (multiple)":
-    var e1, e2: Record
-    check e1.fromURI("enr:-HW4QBzimRxkmT18hMKaAL3IcZF1UcfTMPyi3Q1pxwZZbcZVRI8DC5infUAB_UauARLOJtYTxaagKoGmIjzQxO2qUygBgmlkgnY0iXNlY3AyNTZrMaEDymNMrg1JrLQB2KTGtv6MVbcNEVv0AHacwUAPMljNMTg")
-    check e2.fromURI("enr:-HW4QNfxw543Ypf4HXKXdYxkyzfcxcO-6p9X986WldfVpnVTQX1xlTnWrktEWUbeTZnmgOuAY_KUhbVV1Ft98WoYUBMBgmlkgnY0iXNlY3AyNTZrMaEDDiy3QkHAxPyOgWbxp5oF1bDdlYE6dLCUUp8xfVw50jU")
+    let res1 = Record.fromURI("enr:-HW4QBzimRxkmT18hMKaAL3IcZF1UcfTMPyi3Q1pxwZZbcZVRI8DC5infUAB_UauARLOJtYTxaagKoGmIjzQxO2qUygBgmlkgnY0iXNlY3AyNTZrMaEDymNMrg1JrLQB2KTGtv6MVbcNEVv0AHacwUAPMljNMTg")
+    let res2 = Record.fromURI("enr:-HW4QNfxw543Ypf4HXKXdYxkyzfcxcO-6p9X986WldfVpnVTQX1xlTnWrktEWUbeTZnmgOuAY_KUhbVV1Ft98WoYUBMBgmlkgnY0iXNlY3AyNTZrMaEDDiy3QkHAxPyOgWbxp5oF1bDdlYE6dLCUUp8xfVw50jU")
+    check:
+      res1.isOk()
+      res2.isOk()
     let
+      e1 = res1.value
+      e2 = res2.value
       total = 0x1'u32
       n = NodesMessage(total: total, enrs: @[e1, e2])
       reqId = RequestId(id: @[1.byte])
@@ -267,12 +271,12 @@ suite "Discovery v5.1 Packet Encodings Test Vectors":
       enrRecA = enr.Record.init(1, privKeyA,
         Opt.some(parseIpAddress("127.0.0.1")), Opt.some(Port(9000)),
         Opt.some(Port(9000))).expect("Properly initialized private key")
-      nodeA = newNode(enrRecA).expect("Properly initialized record")
+      nodeA = Node.fromRecord(enrRecA)
 
       enrRecB = enr.Record.init(1, privKeyB,
         Opt.some(parseIpAddress("127.0.0.1")), Opt.some(Port(9000)),
         Opt.some(Port(9000))).expect("Properly initialized private key")
-      nodeB = newNode(enrRecB).expect("Properly initialized record")
+      nodeB = Node.fromRecord(enrRecB)
 
     var
       codecA {.used.} = Codec(localNode: nodeA, privKey: privKeyA,
@@ -488,12 +492,12 @@ suite "Discovery v5.1 Additional Encode/Decode":
       enrRecA = enr.Record.init(1, privKeyA,
         Opt.some(parseIpAddress("127.0.0.1")), Opt.some(Port(9000)),
         Opt.some(Port(9000))).expect("Properly initialized private key")
-      nodeA = newNode(enrRecA).expect("Properly initialized record")
+      nodeA = Node.fromRecord(enrRecA)
 
       enrRecB = enr.Record.init(1, privKeyB,
         Opt.some(parseIpAddress("127.0.0.1")), Opt.some(Port(9000)),
         Opt.some(Port(9000))).expect("Properly initialized private key")
-      nodeB = newNode(enrRecB).expect("Properly initialized record")
+      nodeB = Node.fromRecord(enrRecB)
 
     var
       codecA = Codec(localNode: nodeA, privKey: privKeyA, sessions: Sessions.init(5))

tools/dcli.nim

@@ -131,14 +131,11 @@ proc parseCmdArg*(T: type Node, p: string): T {.raises: [ValueError].} =
   if res.isErr:
     raise newException(ValueError, "Invalid ENR:" & $res.error)
 
-  let n = newNode(res.value)
-  if n.isErr:
-    raise newException(ValueError, $n.error)
-
-  if n.value.address.isNone():
+  let n = Node.fromRecord(res.value)
+  if n.address.isNone():
     raise newException(ValueError, "ENR without address")
 
-  n.value
+  n
 
 proc completeCmdArg*(T: type Node, val: string): seq[string] =
   return @[]
@@ -225,12 +222,18 @@ proc run(config: DiscoveryConf) {.raises: [CatchableError].} =
     let
       address = config.metricsAddress
       port = config.metricsPort
-    notice "Starting metrics HTTP server",
       url = "http://" & $address & ":" & $port & "/metrics"
+      server = MetricsHttpServerRef.new($address, port).valueOr:
+        error "Could not instantiate metrics HTTP server", url, error
+        quit QuitFailure
+
+    info "Starting metrics HTTP server", url
     try:
-      chronos_httpserver.startMetricsHttpServer($address, port)
-    except CatchableError as exc: raise exc
-    except Exception as exc: raiseAssert exc.msg # TODO fix metrics
+      waitFor server.start()
+    except MetricsError as exc:
+      fatal "Could not start metrics HTTP server",
+        url, error_msg = exc.msg, error_name = exc.name
+      quit QuitFailure
 
   case config.cmd
   of ping: