From 8b7d41f596b38d22efbc7c391efb351be81e1b9c Mon Sep 17 00:00:00 2001
From: Kim De Mey <kim.demey@gmail.com>
Date: Tue, 21 Dec 2021 15:09:32 +0100
Subject: [PATCH] Don't use exceptions for enr get call (#453)

The ENR code used to be solely exception based, and these
exceptions where a left-over of that. They are useless as later
calls use Result anyhow.

Additionally, they cause quite the performance loss because they
are used in the "common path" for the toTypedRecord call, e.g.
when reading the fields of ip6, tcp6 and udp6.
---
 eth/p2p/discoveryv5/enr.nim | 57 ++++++++++++++++++++-----------------
 tests/p2p/test_enr.nim      |  6 ++--
 2 files changed, 34 insertions(+), 29 deletions(-)

diff --git a/eth/p2p/discoveryv5/enr.nim b/eth/p2p/discoveryv5/enr.nim
index baeb9d9..acb50fe 100644
--- a/eth/p2p/discoveryv5/enr.nim
+++ b/eth/p2p/discoveryv5/enr.nim
@@ -12,10 +12,10 @@
 
 import
   std/[strutils, macros, algorithm, options],
-  stew/shims/net, stew/base64, nimcrypto,
+  stew/shims/net, stew/[base64, results], nimcrypto,
   ".."/../[rlp, keys]
 
-export options
+export options, results, keys
 
 const
   maxEnrSize = 300  ## Maximum size of an encoded node record, in bytes.
@@ -207,43 +207,47 @@ proc getField(r: Record, name: string, field: var Field): bool =
       field = v
       return true
 
-proc requireKind(f: Field, kind: FieldKind) {.raises: [ValueError].} =
+proc requireKind(f: Field, kind: FieldKind): EnrResult[void] =
   if f.kind != kind:
-    raise newException(ValueError, "Wrong field kind")
+    err("Wrong field kind")
+  else:
+    ok()
 
-proc get*(r: Record, key: string, T: type): T {.raises: [ValueError, Defect].} =
+proc get*(r: Record, key: string, T: type): EnrResult[T] =
   ## Get the value from the provided key.
-  ## Throw `KeyError` if key does not exist.
-  ## Throw `ValueError` if the value is invalid according to type `T`.
   var f: Field
   if r.getField(key, f):
     when T is SomeInteger:
-      requireKind(f, kNum)
-      return T(f.num)
+      ? requireKind(f, kNum)
+      ok(T(f.num))
     elif T is seq[byte]:
-      requireKind(f, kBytes)
-      return f.bytes
+      ? requireKind(f, kBytes)
+      ok(f.bytes)
     elif T is string:
-      requireKind(f, kString)
-      return f.str
+      ? requireKind(f, kString)
+      ok(f.str)
     elif T is PublicKey:
-      requireKind(f, kBytes)
+      ? requireKind(f, kBytes)
       let pk = PublicKey.fromRaw(f.bytes)
       if pk.isErr:
-        raise newException(ValueError, "Invalid public key")
-      return pk[]
+        err("Invalid public key")
+      else:
+        ok(pk[])
     elif T is array:
-      when type(result[0]) is byte:
-        requireKind(f, kBytes)
-        if f.bytes.len != result.len:
-          raise newException(ValueError, "Invalid byte blob length")
-        copyMem(addr result[0], addr f.bytes[0], result.len)
+      when type(default(T)[low(T)]) is byte:
+        ? requireKind(f, kBytes)
+        if f.bytes.len != T.len:
+          err("Invalid byte blob length")
+        else:
+          var res: T
+          copyMem(addr res[0], addr f.bytes[0], res.len)
+          ok(res)
       else:
         {.fatal: "Unsupported output type in enr.get".}
     else:
       {.fatal: "Unsupported output type in enr.get".}
   else:
-    raise newException(KeyError, "Key not found in ENR: " & key)
+    err("Key not found in ENR")
 
 proc get*(r: Record, T: type PublicKey): Option[T] =
   ## Get the `PublicKey` from provided `Record`. Return `none` when there is
@@ -328,10 +332,11 @@ proc tryGet*(r: Record, key: string, T: type): Option[T] =
   ## Get the value from the provided key.
   ## Return `none` if the key does not exist or if the value is invalid
   ## according to type `T`.
-  try:
-    return some get(r, key, T)
-  except ValueError:
-    discard
+  let val = get(r, key, T)
+  if val.isOk():
+    some(val.get())
+  else:
+    none(T)
 
 proc toTypedRecord*(r: Record): EnrResult[TypedRecord] =
   let id = r.tryGet("id", string)
diff --git a/tests/p2p/test_enr.nim b/tests/p2p/test_enr.nim
index de7e6e4..a80167d 100644
--- a/tests/p2p/test_enr.nim
+++ b/tests/p2p/test_enr.nim
@@ -123,14 +123,14 @@ suite "ENR":
       let updated = r.update(pk, [newField])
       check updated.isOk()
       check:
-        r.get("test", uint) == 123
+        r.get("test", uint).get() == 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.get("test", uint).get() == 123
         r.seqNum == 2
 
     block: # Insert k:v pair with changed value, update of seqNum should occur.
@@ -138,7 +138,7 @@ suite "ENR":
       let updated = r.update(pk, [updatedField])
       check updated.isOk()
       check:
-        r.get("test", uint) == 1234
+        r.get("test", uint).get() == 1234
         r.seqNum == 3
 
   test "ENR update sorted":