Merge pull request #283 from status-im/enr-updating

Enr updating

eth/p2p/discoveryv5/encoding.nim

@@ -22,7 +22,7 @@ type
   AuthResponse = object
     version: int
     signature: array[64, byte]
-    record: Record
+    record: Option[enr.Record]
 
   Codec* = object
     localNode*: Node
@@ -103,22 +103,26 @@ proc encodeAuthHeader*(rng: var BrHmacDrbgContext,
   var resp = AuthResponse(version: 5)
   let ln = c.localNode
 
-  # TODO: What goes over the wire now in case of no updated ENR?
   if challenge.recordSeq < ln.record.seqNum:
-    resp.record = ln.record
+    resp.record = some(ln.record)
+  else:
+    resp.record = none(enr.Record)
 
   let ephKeys = KeyPair.random(rng)
   let signature = signIDNonce(c.privKey, challenge.idNonce,
     ephKeys.pubkey.toRaw)
   resp.signature = signature.toRaw
 
-  let secrets = deriveKeys(ln.id, toId, ephKeys.seckey, challenge.pubKey,
+  # Calling `encodePacket` for handshake should always be with a challenge
+  # with the pubkey of the node we are targetting.
+  doAssert(challenge.pubKey.isSome())
+  let secrets = deriveKeys(ln.id, toId, ephKeys.seckey, challenge.pubKey.get(),
     challenge.idNonce)
 
   let respRlp = rlp.encode(resp)
 
   var zeroNonce: array[gcmNonceSize, byte]
-  let respEnc = encryptGCM(secrets.authRespKey, zeroNonce, respRLP, [])
+  let respEnc = encryptGCM(secrets.authRespKey, zeroNonce, respRlp, [])
 
   let header = AuthHeader(auth: nonce, idNonce: challenge.idNonce,
     scheme: authSchemeName, ephemeralKey: ephKeys.pubkey.toRaw,
@@ -248,7 +252,8 @@ proc decodeMessage(body: openarray[byte]): DecodeResult[Message] =
 proc decodeAuthResp*(c: Codec, fromId: NodeId, head: AuthHeader,
     challenge: Whoareyou, newNode: var Node): DecodeResult[HandshakeSecrets] =
   ## Decrypts and decodes the auth-response, which is part of the auth-header.
-  ## Requiers the id-nonce from the WHOAREYOU packet that was send.
+  ## Requires the id-nonce from the WHOAREYOU packet that was send.
+  ## newNode can be nil in case node was already known (no was ENR send).
   if head.scheme != authSchemeName:
     warn "Unknown auth scheme"
     return err(HandshakeError)
@@ -269,19 +274,26 @@ proc decodeAuthResp*(c: Codec, fromId: NodeId, head: AuthHeader,
     authResp = rlp.decode(respData.get(), AuthResponse)
   except RlpError, ValueError:
     return err(HandshakeError)
-  # TODO:
-  # Should allow for not having an ENR included, solved for now by sending
-  # whoareyou with always recordSeq of 0
 
-  # Node returned might not have an address or not a valid address
+  var pubKey: PublicKey
-  newNode = ? newNode(authResp.record).mapErrTo(HandshakeError)
+  if authResp.record.isSome():
-  if newNode.id != fromId:
+    # Node returned might not have an address or not a valid address.
-    return err(HandshakeError)
+    newNode = ? newNode(authResp.record.get()).mapErrTo(HandshakeError)
+    if newNode.id != fromId:
+      return err(HandshakeError)
+
+    pubKey = newNode.pubKey
+  else:
+    if challenge.pubKey.isSome():
+      pubKey = challenge.pubKey.get()
+    else:
+      # We should have received a Record in this case.
+      return err(HandshakeError)
 
   # Verify the id-nonce-sig
   let sig = ? SignatureNR.fromRaw(authResp.signature).mapErrTo(HandshakeError)
   let h = idNonceHash(head.idNonce, head.ephemeralKey)
-  if verify(sig, SkMessage(h.data), newNode.pubkey):
+  if verify(sig, SkMessage(h.data), pubkey):
     ok(secrets)
   else:
     err(HandshakeError)

eth/p2p/discoveryv5/enr.nim

@@ -450,12 +450,9 @@ proc `$`(f: Field): string =
 
 proc `$`*(r: Record): string =
   result = "("
-  var first = true
+  result &= $r.seqNum
   for (k, v) in r.pairs:
-    if first:
+    result &= ", "
-      first = false
-    else:
-      result &= ", "
     result &= k
     result &= ": "
     result &= $v

eth/p2p/discoveryv5/protocol.nim

@@ -249,7 +249,12 @@ proc decodeWhoAreYou(d: Protocol, packet: openArray[byte]):
 proc sendWhoareyou(d: Protocol, address: Address, toNode: NodeId,
     authTag: AuthTag): DiscResult[void] {.raises: [Exception, Defect].} =
   trace "sending who are you", to = $toNode, toAddress = $address
-  let challenge = Whoareyou(authTag: authTag, recordSeq: 0)
+  let n = d.getNode(toNode)
+  let challenge = if n.isSome():
+                    Whoareyou(authTag: authTag, recordSeq: n.get().record.seqNum,
+                      pubKey: some(n.get().pubkey))
+                  else:
+                    Whoareyou(authTag: authTag, recordSeq: 0)
   brHmacDrbgGenerate(d.rng[], challenge.idNonce)
 
   # If there is already a handshake going on for this nodeid then we drop this
@@ -353,7 +358,7 @@ proc receive*(d: Protocol, a: Address, packet: openArray[byte]) {.gcsafe,
     var pr: PendingRequest
     if d.pendingRequests.take(whoareyou.authTag, pr):
       let toNode = pr.node
-      whoareyou.pubKey = toNode.pubkey # TODO: Yeah, rather ugly this.
+      whoareyou.pubKey = some(toNode.pubkey) # TODO: Yeah, rather ugly this.
       doAssert(toNode.address.isSome())
       let (data, _) = encodePacket(d.rng[], d.codec, toNode.id, toNode.address.get(),
         pr.message, challenge = whoareyou)
@@ -694,10 +699,10 @@ proc resolve*(d: Protocol, id: NodeId): Future[Option[Node]]
     {.async, raises: [Exception, Defect].} =
   ## Resolve a `Node` based on provided `NodeId`.
   ##
-  ## This will first look in the own DHT. If the node is known, it will try to
+  ## This will first look in the own routing table. If the node is known, it
-  ## contact if for newer information. If node is not known or it does not
+  ## will try to contact if for newer information. If node is not known or it
-  ## reply, a lookup is done to see if it can find a (newer) record of the node
+  ## does not reply, a lookup is done to see if it can find a (newer) record of
-  ## on the network.
+  ## the node on the network.
 
   let node = d.getNode(id)
   if node.isSome():
@@ -710,8 +715,6 @@ proc resolve*(d: Protocol, id: NodeId): Future[Option[Node]]
   let discovered = await d.lookup(id)
   for n in discovered:
     if n.id == id:
-      # TODO: Not getting any new seqNum here as in a lookup nodes in table with
-      # new seqNum don't get replaced.
       if node.isSome() and node.get().record.seqNum >= n.record.seqNum:
         return node
       else:
@@ -725,8 +728,10 @@ proc revalidateNode*(d: Protocol, n: Node)
 
   if pong.isOK():
     if pong.get().enrSeq > n.record.seqNum:
-      # TODO: Request new ENR
+      # Request new ENR
-      discard
+      let nodes = await d.findNode(n, 0)
+      if nodes.isOk() and nodes[].len > 0:
+        discard d.addNode(nodes[][0])
 
 proc revalidateLoop(d: Protocol) {.async, raises: [Exception, Defect].} =
   # TODO: General Exception raised.

eth/p2p/discoveryv5/routing_table.nim

@@ -110,6 +110,9 @@ proc add(k: KBucket, n: Node): Node =
   k.lastUpdated = epochTime()
   let nodeIdx = k.nodes.find(n)
   if nodeIdx != -1:
+    if k.nodes[nodeIdx].record.seqNum < n.record.seqNum:
+      # In case of a newer record, it gets replaced.
+      k.nodes[nodeIdx].record = n.record
     return nil
   elif k.len < BUCKET_SIZE:
     k.nodes.add(n)
@@ -126,8 +129,11 @@ proc addReplacement(k: KBucket, n: Node) =
   ## to the tail.
   let nodeIdx = k.replacementCache.find(n)
   if nodeIdx != -1:
-    k.replacementCache.delete(nodeIdx)
+    if k.replacementCache[nodeIdx].record.seqNum <= n.record.seqNum:
-    k.replacementCache.add(n)
+      # In case the record sequence number is higher or the same, the node gets
+      # moved to the tail.
+      k.replacementCache.delete(nodeIdx)
+      k.replacementCache.add(n)
   else:
     doAssert(k.replacementCache.len <= REPLACEMENT_CACHE_SIZE)
     if k.replacementCache.len == REPLACEMENT_CACHE_SIZE:

eth/p2p/discoveryv5/types.nim

@@ -23,7 +23,7 @@ type
     authTag*: AuthTag
     idNonce*: IdNonce
     recordSeq*: uint64
-    pubKey* {.rlpIgnore.}: PublicKey
+    pubKey* {.rlpIgnore.}: Option[PublicKey]
 
   Whoareyou* = ref WhoareyouObj
 
@@ -107,3 +107,23 @@ proc hash*(address: Address): Hash {.inline.} =
 proc hash*(key: HandshakeKey): Hash =
   result = key.nodeId.hash !& key.address.hash
   result = !$result
+
+proc read*(rlp: var Rlp, O: type Option[Record]): O
+    {.raises: [ValueError, RlpError, Defect].} =
+  mixin read
+  if not rlp.isList:
+    raise newException(
+      ValueError, "Could not deserialize optional ENR, expected list")
+
+  # The discovery specification states that in case no ENR is send in the
+  # handshake, an empty rlp list instead should be send.
+  if rlp.listLen == 0:
+    none(Record)
+  else:
+    some(read(rlp, Record))
+
+proc append*(writer: var RlpWriter, value: Option[Record]) =
+  if value.isSome:
+    writer.append value.get
+  else:
+    writer.startList(0)

tests/p2p/test_discoveryv5.nim

@@ -313,8 +313,8 @@ procSuite "Discovery v5 Tests":
 
     var targetSeqNum = targetNode.localNode.record.seqNum
 
-    # Populate DHT with target through a ping. Next, close target and see
+    # Populate routing table with target through a ping. Next, close target and
-    # if resolve works (only local lookup)
+    # see if resolve works (only local getNode).
     block:
       let pong = await targetNode.ping(mainNode.localNode)
       check pong.isOk()
@@ -324,13 +324,14 @@ procSuite "Discovery v5 Tests":
         n.isSome()
         n.get().id == targetId
         n.get().record.seqNum == targetSeqNum
+    # Node will be removed because of failed findNode request.
 
     # Bring target back online, update seqNum in ENR, check if we get the
     # updated ENR.
     block:
       targetNode.open()
       # ping to node again to add as it was removed after failed findNode in
-      # resolve in previous test block
+      # resolve in previous test block.
       let pong = await targetNode.ping(mainNode.localNode)
       check pong.isOk()
 
@@ -339,13 +340,22 @@ procSuite "Discovery v5 Tests":
       let update = targetNode.updateRecord({"addsomefield": @[byte 1]})
       check update.isOk()
 
-      let n = await mainNode.resolve(targetId)
+      var n = mainNode.getNode(targetId)
+      check:
+        n.isSome()
+        n.get().id == targetId
+        n.get().record.seqNum == targetSeqNum - 1
+
+      n = await mainNode.resolve(targetId)
       check:
         n.isSome()
         n.get().id == targetId
         n.get().record.seqNum == targetSeqNum
 
-    # Update seqNum in ENR again, ping lookupNode to be added in DHT,
+      # Add the updated version
+      check 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()
@@ -357,11 +367,8 @@ procSuite "Discovery v5 Tests":
       # findNode request
       check (await lookupNode.ping(targetNode.localNode)).isOk()
       await targetNode.closeWait()
-      # TODO: This step should eventually not be needed and ENRs with new seqNum
-      # should just get updated in the lookup.
-      await mainNode.revalidateNode(targetNode.localNode)
 
-      check mainNode.addNode(lookupNode.localNode)
+      check mainNode.addNode(lookupNode.localNode.record)
       let n = await mainNode.resolve(targetId)
       check:
         n.isSome()
@@ -419,6 +426,73 @@ procSuite "Discovery v5 Tests":
         db, ip, port, port, rng = rng,
         previousRecord = some(updatesNode.getRecord()))
 
+  asyncTest "Update node record with revalidate":
+    let
+      mainNode =
+        initDiscoveryNode(rng, PrivateKey.random(rng[]), localAddress(20301))
+      testNode =
+        initDiscoveryNode(rng, PrivateKey.random(rng[]), localAddress(20302))
+      testNodeId = testNode.localNode.id
+
+    check:
+      # Get node with current ENR in routing table.
+      # Handshake will get done here.
+      (await testNode.ping(mainNode.localNode)).isOk()
+      testNode.updateRecord({"test" : @[byte 1]}).isOk()
+      testNode.localNode.record.seqNum == 2
+
+    # Get the node from routing table, seqNum should still be 1.
+    var n = mainNode.getNode(testNodeId)
+    check:
+      n.isSome()
+      n.get.record.seqNum == 1
+
+    # This should not do a handshake and thus the new ENR must come from the
+    # findNode(0)
+    await mainNode.revalidateNode(n.get)
+
+    # Get the node from routing table, and check if record got updated.
+    n = mainNode.getNode(testNodeId)
+    check:
+      n.isSome()
+      n.get.record.seqNum == 2
+
+    await mainNode.closeWait()
+    await testNode.closeWait()
+
+  asyncTest "Update node record with handshake":
+    let
+      mainNode =
+        initDiscoveryNode(rng, PrivateKey.random(rng[]), localAddress(20301))
+      testNode =
+        initDiscoveryNode(rng, PrivateKey.random(rng[]), localAddress(20302))
+      testNodeId = testNode.localNode.id
+
+    # Add the node (from the record, so new node!) so no handshake is done yet.
+    check: mainNode.addNode(testNode.localNode.record)
+
+    check:
+      testNode.updateRecord({"test" : @[byte 1]}).isOk()
+      testNode.localNode.record.seqNum == 2
+
+    # Get the node from routing table, seqNum should still be 1.
+    var n = mainNode.getNode(testNodeId)
+    check:
+      n.isSome()
+      n.get.record.seqNum == 1
+
+    # This should do a handshake and update the ENR through that.
+    check (await testNode.ping(mainNode.localNode)).isOk()
+
+    # Get the node from routing table, and check if record got updated.
+    n = mainNode.getNode(testNodeId)
+    check:
+      n.isSome()
+      n.get.record.seqNum == 2
+
+    await mainNode.closeWait()
+    await testNode.closeWait()
+
   test "Verify records of nodes message":
     let
       port = Port(9000)

tests/p2p/test_discv5_encoding.nim

@@ -241,15 +241,27 @@ suite "Discovery v5 Additional":
       nodeId = pubKey.toNodeId()
       idNonce = hexToByteArray[idNonceSize](
         "0xa77e3aa0c144ae7c0a3af73692b7d6e5b7a2fdc0eda16e8d5e6cb0d08e88dd04")
-      whoareyou = Whoareyou(idNonce: idNonce, recordSeq: 0, pubKey: pubKey)
       c = Codec(localNode: node, privKey: privKey)
 
-    let (auth, _) = encodeAuthHeader(rng[], c, nodeId, nonce, whoareyou)
+    block: # With ENR
-    var rlp = rlpFromBytes(auth)
+      let
-    let authHeader = rlp.read(AuthHeader)
+        whoareyou = Whoareyou(idNonce: idNonce, recordSeq: 0, pubKey: some(pubKey))
-    var newNode: Node
+        (auth, _) = encodeAuthHeader(rng[], c, nodeId, nonce, whoareyou)
-    let secrets = c.decodeAuthResp(privKey.toPublicKey().toNodeId(),
+      var rlp = rlpFromBytes(auth)
-      authHeader, whoareyou, newNode)
+      let authHeader = rlp.read(AuthHeader)
+      var newNode: Node
+      let secrets = c.decodeAuthResp(privKey.toPublicKey().toNodeId(),
+        authHeader, whoareyou, newNode)
+
+    block: # Without ENR
+      let
+        whoareyou = Whoareyou(idNonce: idNonce, recordSeq: 1, pubKey: some(pubKey))
+        (auth, _) = encodeAuthHeader(rng[], c, nodeId, nonce, whoareyou)
+      var rlp = rlpFromBytes(auth)
+      let authHeader = rlp.read(AuthHeader)
+      var newNode: Node
+      let secrets = c.decodeAuthResp(privKey.toPublicKey().toNodeId(),
+        authHeader, whoareyou, newNode)
 
     # TODO: Test cases with invalid nodeId and invalid signature, the latter
     # is in the current code structure rather difficult and would need some

tests/p2p/test_enr.nim

@@ -10,7 +10,7 @@ suite "ENR":
     var pk = PrivateKey.fromHex(
       "5d2908f3f09ea1ff2e327c3f623159639b00af406e9009de5fd4b910fc34049d")[]
     var r = initRecord(123, pk, {"udp": 1234'u, "ip": [byte 5, 6, 7, 8]})[]
-    check($r == """(id: "v4", ip: 0x05060708, secp256k1: 0x02E51EFA66628CE09F689BC2B82F165A75A9DDECBB6A804BE15AC3FDF41F3B34E7, udp: 1234)""")
+    check($r == """(123, id: "v4", ip: 0x05060708, secp256k1: 0x02E51EFA66628CE09F689BC2B82F165A75A9DDECBB6A804BE15AC3FDF41F3B34E7, udp: 1234)""")
     let uri = r.toURI()
     var r2: Record
     let sigValid = r2.fromURI(uri)
@@ -22,7 +22,7 @@ suite "ENR":
     var pk = PrivateKey.fromHex(
       "5d2908f3f09ea1ff2e327c3f623159639b00af406e9009de5fd4b910fc34049d")[]
     var r = initRecord(123, pk, {"udp": 1234'u, "ip": [byte 5, 6, 7, 8]})[]
-    check($r == """(id: "v4", ip: 0x05060708, secp256k1: 0x02E51EFA66628CE09F689BC2B82F165A75A9DDECBB6A804BE15AC3FDF41F3B34E7, udp: 1234)""")
+    check($r == """(123, id: "v4", ip: 0x05060708, secp256k1: 0x02E51EFA66628CE09F689BC2B82F165A75A9DDECBB6A804BE15AC3FDF41F3B34E7, udp: 1234)""")
     let encoded = rlp.encode(r)
     let decoded = rlp.decode(encoded, enr.Record)
     check($decoded == $r)
@@ -44,7 +44,7 @@ suite "ENR":
     var r: Record
     let sigValid = r.fromBase64("-IS4QHCYrYZbAKWCBRlAy5zzaDZXJBGkcnh4MHcBFZntXNFrdvJjX04jRzjzCBOonrkTfj499SZuOh8R33Ls8RRcy5wBgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQPKY0yuDUmstAHYpMa2_oxVtw0RW_QAdpzBQA8yWM0xOIN1ZHCCdl8")
     check(sigValid)
-    check($r == """(id: "v4", ip: 0x7F000001, secp256k1: 0x03CA634CAE0D49ACB401D8A4C6B6FE8C55B70D115BF400769CC1400F3258CD3138, udp: 30303)""")
+    check($r == """(1, id: "v4", ip: 0x7F000001, secp256k1: 0x03CA634CAE0D49ACB401D8A4C6B6FE8C55B70D115BF400769CC1400F3258CD3138, udp: 30303)""")
 
   test "Bad base64":
     var r: Record
@@ -146,13 +146,13 @@ suite "ENR":
                                  "z": [byte 0],
                                  "123": "abc",
                                  "a12": 1'u})[]
-    check $r == """(123: "abc", a12: 1, abc: 1234, id: "v4", secp256k1: 0x02E51EFA66628CE09F689BC2B82F165A75A9DDECBB6A804BE15AC3FDF41F3B34E7, z: 0x00)"""
+    check $r == """(123, 123: "abc", a12: 1, abc: 1234, id: "v4", secp256k1: 0x02E51EFA66628CE09F689BC2B82F165A75A9DDECBB6A804BE15AC3FDF41F3B34E7, z: 0x00)"""
 
     let newField = toFieldPair("test", 123'u)
     let newField2 = toFieldPair("zzz", 123'u)
     let updated = r.update(pk, [newField, newField2])
     check updated.isOk()
-    check $r == """(123: "abc", a12: 1, abc: 1234, id: "v4", secp256k1: 0x02E51EFA66628CE09F689BC2B82F165A75A9DDECBB6A804BE15AC3FDF41F3B34E7, test: 123, z: 0x00, zzz: 123)"""
+    check $r == """(124, 123: "abc", a12: 1, abc: 1234, id: "v4", secp256k1: 0x02E51EFA66628CE09F689BC2B82F165A75A9DDECBB6A804BE15AC3FDF41F3B34E7, test: 123, z: 0x00, zzz: 123)"""
 
   test "ENR update size too big":
     let pk = PrivateKey.fromHex(