Merge pull request #266 from status-im/enr-update

Enr updating

eth/p2p/discoveryv5/enr.nim

@@ -1,4 +1,4 @@
-# ENR implemetation according to spec:
+# ENR implementation according to specification in EIP-778:
 # https://github.com/ethereum/EIPs/blob/master/EIPS/eip-778.md
 
 import
@@ -11,8 +11,9 @@ export options
 {.push raises: [Defect].}
 
 const
-  maxEnrSize = 300
+  maxEnrSize = 300  ## Maximum size of an encoded node record, in bytes.
-  minRlpListLen = 4 # for signature, seqId, "id" key, id
+  minRlpListLen = 4 ## Minimum node record RLP list has: signature, seqId,
+  ## "id" key and value.
 
 type
   FieldPair* = (string, Field)
@@ -63,8 +64,51 @@ template toField[T](v: T): Field =
   else:
     {.error: "Unsupported field type".}
 
+proc `==`(a, b: Field): bool =
+  if a.kind == b.kind:
+    case a.kind
+    of kString:
+      return a.str == b.str
+    of kNum:
+      return a.num == b.num
+    of kBytes:
+      return a.bytes == b.bytes
+  else:
+    return false
+
+proc cmp(a, b: FieldPair): int = cmp(a[0], b[0])
+
+proc makeEnrRaw(seqNum: uint64, pk: PrivateKey,
+    pairs: openarray[FieldPair]): EnrResult[seq[byte]] =
+  proc append(w: var RlpWriter, seqNum: uint64,
+      pairs: openarray[FieldPair]): seq[byte] =
+    w.append(seqNum)
+    for (k, v) in pairs:
+      w.append(k)
+      case v.kind
+      of kString: w.append(v.str)
+      of kNum: w.append(v.num)
+      of kBytes: w.append(v.bytes)
+    w.finish()
+
+  let toSign = block:
+    var w = initRlpList(pairs.len * 2 + 1)
+    w.append(seqNum, pairs)
+
+  let sig = signNR(pk, toSign)
+
+  var raw = block:
+    var w = initRlpList(pairs.len * 2 + 2)
+    w.append(sig.toRaw())
+    w.append(seqNum, pairs)
+
+  if raw.len > maxEnrSize:
+    err("Record exceeds maximum size")
+  else:
+    ok(raw)
+
 proc makeEnrAux(seqNum: uint64, pk: PrivateKey,
-    pairs: openarray[(string, Field)]): EnrResult[Record] =
+    pairs: openarray[FieldPair]): EnrResult[Record] =
   var record: Record
   record.pairs = @pairs
   record.seqNum = seqNum
@@ -76,35 +120,19 @@ proc makeEnrAux(seqNum: uint64, pk: PrivateKey,
     Field(kind: kBytes, bytes: @(pubkey.toRawCompressed()))))
 
   # Sort by key
-  record.pairs.sort() do(a, b: (string, Field)) -> int:
+  record.pairs.sort(cmp)
-    cmp(a[0], b[0])
+  # TODO: Should deduplicate on keys here also. Should we error on that or just
-
+  # deal with it?
-  proc append(w: var RlpWriter, seqNum: uint64,
-      pairs: openarray[(string, Field)]): seq[byte] =
-    w.append(seqNum)
-    for (k, v) in pairs:
-      w.append(k)
-      case v.kind
-      of kString: w.append(v.str)
-      of kNum: w.append(v.num)
-      of kBytes: w.append(v.bytes)
-    w.finish()
-
-  let toSign = block:
-    var w = initRlpList(record.pairs.len * 2 + 1)
-    w.append(seqNum, record.pairs)
-
-  let sig = signNR(pk, toSign)
-
-  record.raw = block:
-    var w = initRlpList(record.pairs.len * 2 + 2)
-    w.append(sig.toRaw())
-    w.append(seqNum, record.pairs)
 
+  record.raw = ? makeEnrRaw(seqNum, pk, record.pairs)
   ok(record)
 
 macro initRecord*(seqNum: uint64, pk: PrivateKey,
     pairs: untyped{nkTableConstr}): untyped =
+  ## Initialize a `Record` with given sequence number, private key and k:v
+  ## pairs.
+  ##
+  ## Can fail in case the record exceeds the `maxEnrSize`.
   for c in pairs:
     c.expectKind(nnkExprColonExpr)
     c[1] = newCall(bindSym"toField", c[1])
@@ -115,14 +143,8 @@ macro initRecord*(seqNum: uint64, pk: PrivateKey,
 template toFieldPair*(key: string, value: auto): FieldPair =
   (key, toField(value))
 
-proc init*(T: type Record, seqNum: uint64,
+proc addAddress(fields: var seq[FieldPair], ip: Option[ValidIpAddress],
-                           pk: PrivateKey,
+    tcpPort, udpPort: Port) =
-                           ip: Option[ValidIpAddress],
-                           tcpPort, udpPort: Port,
-                           extraFields: openarray[FieldPair] = []):
-                           EnrResult[T] =
-  var fields = newSeq[FieldPair]()
-
   if ip.isSome():
     let
       ipExt = ip.get()
@@ -136,6 +158,19 @@ proc init*(T: type Record, seqNum: uint64,
     fields.add(("tcp", tcpPort.uint16.toField))
     fields.add(("udp", udpPort.uint16.toField))
 
+proc init*(T: type Record, seqNum: uint64,
+                           pk: PrivateKey,
+                           ip: Option[ValidIpAddress],
+                           tcpPort, udpPort: Port,
+                           extraFields: openarray[FieldPair] = []):
+                           EnrResult[T] =
+  ## Initialize a `Record` with given sequence number, private key, optional
+  ## ip address, tcp port, udp port, and optional custom k:v pairs.
+  ##
+  ## Can fail in case the record exceeds the `maxEnrSize`.
+  var fields = newSeq[FieldPair]()
+
+  fields.addAddress(ip, tcpPort, udpPort)
   fields.add extraFields
   makeEnrAux(seqNum, pk, fields)
 
@@ -190,6 +225,75 @@ proc get*(r: Record, T: type PublicKey): Option[T] =
     if pk.isOk:
       return some pk[]
 
+proc find(r: Record, key: string): Option[int] =
+  ## Search for key in record key:value pairs.
+  ##
+  ## Returns some(index of key) if key is found in record. Else return none.
+  for i, (k, v) in r.pairs:
+    if k == key:
+      return some(i)
+
+proc update*(record: var Record, pk: PrivateKey,
+    fieldPairs: openarray[FieldPair]): EnrResult[void] =
+  ## Update a `Record` k:v pairs.
+  ##
+  ## In case any of the k:v pairs is updated or added (new), the sequence number
+  ## of the `Record` will be incremented and a new signature will be applied.
+  ##
+  ## Can fail in case of wrong `PrivateKey`, if the size of the resulting record
+  ## exceeds `maxEnrSize` or if maximum sequence number is reached. The `Record`
+  ## will not be altered in these cases.
+  var r = record
+
+  let pubkey = r.get(PublicKey)
+  if pubkey.isNone() or pubkey.get() != pk.toPublicKey():
+    return err("Public key does not correspond with given private key")
+
+  var updated = false
+  for fieldPair in fieldPairs:
+    let index = r.find(fieldPair[0])
+    if(index.isSome()):
+      if r.pairs[index.get()][1] == fieldPair[1]:
+        # Exact k:v pair is already in record, nothing to do here.
+        continue
+      else:
+        # Need to update the value.
+        r.pairs[index.get()] = fieldPair
+        updated = true
+    else:
+      # Add new k:v pair.
+      r.pairs.insert(fieldPair, lowerBound(r.pairs, fieldPair, cmp))
+      updated = true
+
+  if updated:
+    if r.seqNum == high(r.seqNum): # highly unlikely
+      return err("Maximum sequence number reached")
+    r.seqNum.inc()
+    r.raw = ? makeEnrRaw(r.seqNum, pk, r.pairs)
+    record = r
+
+  ok()
+
+proc update*(r: var Record, pk: PrivateKey,
+                            ip: Option[ValidIpAddress],
+                            tcpPort, udpPort: Port,
+                            extraFields: openarray[FieldPair] = []):
+                            EnrResult[void] =
+  ## Update a `Record` with given ip address, tcp port, udp port and optional
+  ## custom k:v pairs.
+  ##
+  ## In case any of the k:v pairs is updated or added (new), the sequence number
+  ## of the `Record` will be incremented and a new signature will be applied.
+  ##
+  ## Can fail in case of wrong `PrivateKey`, if the size of the resulting record
+  ## exceeds `maxEnrSize` or if maximum sequence number is reached. The `Record`
+  ## will not be altered in these cases.
+  var fields = newSeq[FieldPair]()
+
+  fields.addAddress(ip, tcpPort, udpPort)
+  fields.add extraFields
+  r.update(pk, fields)
+
 proc tryGet*(r: Record, key: string, T: type): Option[T] =
   try:
     return some get(r, key, T)

eth/p2p/discoveryv5/protocol.nim

@@ -73,7 +73,7 @@
 ## This might be a concern for mobile devices.
 
 import
-  std/[tables, sets, options, math, random], bearssl,
+  std/[tables, sets, options, math, random, sequtils], bearssl,
   stew/shims/net as stewNet, json_serialization/std/net,
   stew/[byteutils, endians2], chronicles, chronos, stint,
   eth/[rlp, keys, async_utils], types, encoding, node, routing_table, enr
@@ -172,6 +172,16 @@ proc nodesDiscovered*(d: Protocol): int {.inline.} = d.routingTable.len
 func privKey*(d: Protocol): lent PrivateKey =
   d.privateKey
 
+func getRecord*(d: Protocol): Record =
+  d.localNode.record
+
+proc updateRecord*(
+    d: Protocol, enrFields: openarray[(string, seq[byte])]): DiscResult[void] =
+  let fields = mapIt(enrFields, toFieldPair(it[0], it[1]))
+  d.localNode.record.update(d.privateKey, fields)
+  # TODO: Would it make sense to actively ping ("broadcast") to all the peers
+  # we stored a handshake with in order to get that ENR updated?
+
 proc send(d: Protocol, a: Address, data: seq[byte]) =
   let ta = initTAddress(a.ip, a.port)
   try:
@@ -691,8 +701,9 @@ proc lookupLoop(d: Protocol) {.async, raises: [Exception, Defect].} =
 
 proc newProtocol*(privKey: PrivateKey, db: Database,
                   externalIp: Option[ValidIpAddress], tcpPort, udpPort: Port,
-                  localEnrFields: openarray[FieldPair] = [],
+                  localEnrFields: openarray[(string, seq[byte])] = [],
                   bootstrapRecords: openarray[Record] = [],
+                  previousRecord = none[enr.Record](),
                   bindIp = IPv4_any(), rng = newRng()):
                   Protocol {.raises: [Defect].} =
   # TODO: Tried adding bindPort = udpPort as parameter but that gave
@@ -700,10 +711,19 @@ proc newProtocol*(privKey: PrivateKey, db: Database,
   # Anyhow, nim-beacon-chain would also require some changes to support port
   # remapping through NAT and this API is also subject to change once we
   # introduce support for ipv4 + ipv6 binding/listening.
-  let
+  let extraFields = mapIt(localEnrFields, toFieldPair(it[0], it[1]))
-    enrRec = enr.Record.init(1, privKey, externalIp, tcpPort, udpPort,
+  # TODO:
-      localEnrFields).expect("Properly intialized private key")
+  # - Defect as is now or return a result for enr errors?
-    node = newNode(enrRec).expect("Properly initialized node")
+  # - In case incorrect key, allow for new enr based on new key (new node id)?
+  var record: Record
+  if previousRecord.isSome():
+    record = previousRecord.get()
+    record.update(privKey, externalIp, tcpPort, udpPort,
+      extraFields).expect("Record within size limits and correct key")
+  else:
+    record = enr.Record.init(1, privKey, externalIp, tcpPort, udpPort,
+     extraFields).expect("Record within size limits")
+  let node = newNode(record).expect("Properly initialized record")
 
   # TODO Consider whether this should be a Defect
   doAssert rng != nil, "RNG initialization failed"

tests/p2p/discv5_test_helper.nim

@@ -7,16 +7,19 @@ import
 proc localAddress*(port: int): Address =
   Address(ip: ValidIpAddress.init("127.0.0.1"), port: Port(port))
 
-proc initDiscoveryNode*(rng: ref BrHmacDrbgContext, privKey: PrivateKey, address: Address,
+proc initDiscoveryNode*(rng: ref BrHmacDrbgContext, privKey: PrivateKey,
+                        address: Address,
                         bootstrapRecords: openarray[Record] = [],
-                        localEnrFields: openarray[FieldPair] = []):
+                        localEnrFields: openarray[(string, seq[byte])] = [],
+                        previousRecord = none[enr.Record]()):
                         discv5_protocol.Protocol =
   var db = DiscoveryDB.init(newMemoryDB())
   result = newProtocol(privKey, db,
                        some(address.ip),
                        address.port, address.port,
                        bootstrapRecords = bootstrapRecords,
-                       localEnrFields = localEnrFields, rng = rng)
+                       localEnrFields = localEnrFields,
+                       previousRecord = previousRecord, rng = rng)
 
   result.open()
 

tests/p2p/test_discoveryv5.nim

@@ -1,7 +1,7 @@
 import
   chronos, chronicles, tables, stint, testutils/unittests,
-  stew/shims/net, eth/keys, bearssl,
+  stew/shims/net, eth/[keys, trie/db], bearssl,
-  eth/p2p/discoveryv5/[enr, node, types, routing_table, encoding],
+  eth/p2p/discoveryv5/[enr, node, types, routing_table, encoding, discovery_db],
   eth/p2p/discoveryv5/protocol as discv5_protocol,
   ./discv5_test_helper
 
@@ -54,7 +54,6 @@ procSuite "Discovery v5 Tests":
 
     await node1.closeWait()
 
-
   asyncTest "Handshake cleanup":
     let node = initDiscoveryNode(
       rng, PrivateKey.random(rng[]), localAddress(20302))
@@ -334,11 +333,12 @@ procSuite "Discovery v5 Tests":
       # resolve in previous test block
       let pong = await targetNode.ping(mainNode.localNode)
       check pong.isOk()
-      # TODO: need to add some logic to update ENRs properly
+
       targetSeqNum.inc()
-      let r = enr.Record.init(targetSeqNum, targetKey,
+      # need to add something to get the enr sequence number incremented
-        some(targetAddress.ip), targetAddress.port, targetAddress.port)[]
+      let update = targetNode.updateRecord({"addsomefield": @[byte 1]})
-      targetNode.localNode.record = r
+      check update.isOk()
+
       let n = await mainNode.resolve(targetId)
       check:
         n.isSome()
@@ -349,9 +349,7 @@ procSuite "Discovery v5 Tests":
     # close targetNode, resolve should lookup, check if we get updated ENR.
     block:
       targetSeqNum.inc()
-      let r = enr.Record.init(targetSeqNum, targetKey, some(targetAddress.ip),
+      let update = targetNode.updateRecord({"addsomefield": @[byte 2]})
-        targetAddress.port, targetAddress.port)[]
-      targetNode.localNode.record = r
 
       # ping node so that its ENR gets added
       check (await targetNode.ping(lookupNode.localNode)).isOk()
@@ -394,3 +392,29 @@ procSuite "Discovery v5 Tests":
     check discoveredFiltered.len == 1 and discoveredFiltered.contains(targetNode)
 
     await lookupNode.closeWait()
+
+  test "New protocol with enr":
+    let
+      privKey = PrivateKey.random(rng[])
+      ip = some(ValidIpAddress.init("127.0.0.1"))
+      port = Port(20301)
+      db = DiscoveryDB.init(newMemoryDB())
+      node = newProtocol(privKey, db, ip, port, port, rng = rng)
+      noUpdatesNode = newProtocol(privKey, db, ip, port, port, rng = rng,
+        previousRecord = some(node.getRecord()))
+      updatesNode = newProtocol(privKey, db, ip, port, Port(20302), rng = rng,
+        previousRecord = some(noUpdatesNode.getRecord()))
+      moreUpdatesNode = newProtocol(privKey, db, ip, port, port, rng = rng,
+        localEnrFields = {"addfield": @[byte 0]},
+        previousRecord = some(updatesNode.getRecord()))
+    check:
+      node.getRecord().seqNum == 1
+      noUpdatesNode.getRecord().seqNum == 1
+      updatesNode.getRecord().seqNum == 2
+      moreUpdatesNode.getRecord().seqNum == 3
+
+    # Defect (for now?) on incorrect key use
+    expect ResultDefect:
+      let incorrectKeyUpdates = newProtocol(PrivateKey.random(rng[]),
+        db, ip, port, port, rng = rng,
+        previousRecord = some(updatesNode.getRecord()))

tests/p2p/test_enr.nim

@@ -1,5 +1,5 @@
 import
-  unittest, options,
+  unittest, options, sequtils,
   nimcrypto/utils, stew/shims/net,
   eth/p2p/enode, eth/p2p/discoveryv5/enr, eth/keys
 
@@ -35,14 +35,15 @@ suite "ENR":
 
   test "Create from ENode address":
     let
-      keys = KeyPair.random(rng[])
+      keypair = KeyPair.random(rng[])
       ip = ValidIpAddress.init("10.20.30.40")
-      enr = Record.init(100, keys.seckey, some(ip), Port(9000), Port(9000), @[])[]
+      enr = Record.init(
+        100, keypair.seckey, some(ip), Port(9000), Port(9000),@[])[]
       typedEnr = get enr.toTypedRecord()
 
     check:
       typedEnr.secp256k1.isSome()
-      typedEnr.secp256k1.get == keys.pubkey.toRawCompressed()
+      typedEnr.secp256k1.get == keypair.pubkey.toRawCompressed()
 
       typedEnr.ip.isSome()
       typedEnr.ip.get() == [byte 10, 20, 30, 40]
@@ -55,13 +56,14 @@ suite "ENR":
 
   test "ENR without address":
     let
-      keys = KeyPair.random(rng[])
+      keypair = KeyPair.random(rng[])
-      enr = Record.init(100, keys.seckey, none(ValidIpAddress), Port(9000), Port(9000))[]
+      enr = Record.init(
+        100, keypair.seckey, none(ValidIpAddress), Port(9000), Port(9000))[]
       typedEnr = get enr.toTypedRecord()
 
     check:
       typedEnr.secp256k1.isSome()
-      typedEnr.secp256k1.get() == keys.pubkey.toRawCompressed()
+      typedEnr.secp256k1.get() == keypair.pubkey.toRawCompressed()
 
       typedEnr.ip.isNone()
       typedEnr.tcp.isSome()
@@ -73,3 +75,104 @@ suite "ENR":
       typedEnr.ip6.isNone()
       typedEnr.tcp6.isNone()
       typedEnr.udp6.isNone()
+
+  test "ENR init size too big":
+    let pk = PrivateKey.fromHex(
+      "5d2908f3f09ea1ff2e327c3f623159639b00af406e9009de5fd4b910fc34049d")[]
+    block: # This gives ENR of 300 bytes encoded
+      let r = initRecord(1, pk, {"maxvalue": repeat(byte 2, 169),})
+      check r.isOk()
+
+    block: # This gives ENR of 301 bytes encoded
+      let r = initRecord(1, pk, {"maxplus1": repeat(byte 2, 170),})
+      check r.isErr()
+
+  test "ENR update":
+    let
+      pk = PrivateKey.fromHex(
+        "5d2908f3f09ea1ff2e327c3f623159639b00af406e9009de5fd4b910fc34049d")[]
+      newField = toFieldPair("test", 123'u)
+    var r = Record.init(1, pk, none(ValidIpAddress), Port(9000), Port(9000))[]
+
+    block: # Insert new k:v pair, update of seqNum should occur.
+      let updated = r.update(pk, [newField])
+      check updated.isOk()
+      check:
+        r.get("test", uint) == 123
+        r.seqNum == 2
+
+    block: # Insert same k:v pair, no update of seqNum should occur.
+      let updated = r.update(pk, [newField])
+      check updated.isOk()
+      check:
+        r.get("test", uint) == 123
+        r.seqNum == 2
+
+    block: # Insert k:v pair with changed value, update of seqNum should occur.
+      let updatedField = toFieldPair("test", 1234'u)
+      let updated = r.update(pk, [updatedField])
+      check updated.isOk()
+      check:
+        r.get("test", uint) == 1234
+        r.seqNum == 3
+
+  test "ENR update sorted":
+    let pk = PrivateKey.fromHex(
+      "5d2908f3f09ea1ff2e327c3f623159639b00af406e9009de5fd4b910fc34049d")[]
+    var r = initRecord(123, pk, {"abc": 1234'u,
+                                 "z": [byte 0],
+                                 "123": "abc",
+                                 "a12": 1'u})[]
+    check $r == """(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)"""
+
+  test "ENR update size too big":
+    let pk = PrivateKey.fromHex(
+      "5d2908f3f09ea1ff2e327c3f623159639b00af406e9009de5fd4b910fc34049d")[]
+
+    var r = initRecord(1, pk, {"maxvalue": repeat(byte 2, 169),})
+    check r.isOk()
+
+    let newField = toFieldPair("test", 123'u)
+    let updated = r[].update(pk, [newField])
+    check updated.isErr()
+
+  test "ENR update invalid key":
+    let pk = PrivateKey.fromHex(
+      "5d2908f3f09ea1ff2e327c3f623159639b00af406e9009de5fd4b910fc34049d")[]
+
+    var r = initRecord(1, pk, {"abc": 1'u,})
+    check r.isOk()
+
+    let
+      wrongPk = PrivateKey.random(rng[])
+      newField = toFieldPair("test", 123'u)
+      updated = r[].update(wrongPk, [newField])
+    check updated.isErr()
+
+  test "ENR update address":
+    let
+      pk = PrivateKey.fromHex(
+        "5d2908f3f09ea1ff2e327c3f623159639b00af406e9009de5fd4b910fc34049d")[]
+    var r = Record.init(1, pk, none(ValidIpAddress), Port(9000), Port(9000))[]
+
+    block:
+      let updated = r.update(pk, none(ValidIpAddress), Port(9000), Port(9000))
+      check updated.isOk()
+      check:
+        r.get("tcp", uint) == 9000
+        r.get("udp", uint) == 9000
+        r.seqNum == 1
+
+    block:
+      let updated = r.update(pk, none(ValidIpAddress), Port(9001), Port(9002))
+      check updated.isOk()
+      check:
+        r.get("tcp", uint) == 9001
+        r.get("udp", uint) == 9002
+        r.seqNum == 2