From 74df90e16d1a71134bee25e505d2a2cd12ff3588 Mon Sep 17 00:00:00 2001 From: kdeme Date: Thu, 30 Apr 2020 00:11:03 +0200 Subject: [PATCH] discv5: further prepping for results error handling --- eth/p2p/discoveryv5/encoding.nim | 2 +- eth/p2p/discoveryv5/enr.nim | 84 ++++++++++++++++----------- eth/p2p/discoveryv5/node.nim | 39 ++++++------- eth/p2p/discoveryv5/protocol.nim | 3 +- eth/p2p/discoveryv5/routing_table.nim | 41 +++++++------ tests/p2p/test_discoveryv5.nim | 6 +- tests/p2p/test_enr.nim | 6 +- 7 files changed, 97 insertions(+), 84 deletions(-) diff --git a/eth/p2p/discoveryv5/encoding.nim b/eth/p2p/discoveryv5/encoding.nim index d79cc9a..128245f 100644 --- a/eth/p2p/discoveryv5/encoding.nim +++ b/eth/p2p/discoveryv5/encoding.nim @@ -170,7 +170,7 @@ proc encodePacket*(c: Codec, ok((packet, nonce)) proc decryptGCM*(key: AesKey, nonce, ct, authData: openarray[byte]): - Option[seq[byte]] {.raises:[].} = + Option[seq[byte]] = if ct.len <= gcmTagSize: debug "cipher is missing tag", len = ct.len return diff --git a/eth/p2p/discoveryv5/enr.nim b/eth/p2p/discoveryv5/enr.nim index 7c43937..21351f8 100644 --- a/eth/p2p/discoveryv5/enr.nim +++ b/eth/p2p/discoveryv5/enr.nim @@ -8,6 +8,8 @@ import export options +{.push raises: [Defect].} + const maxEnrSize = 300 minRlpListLen = 4 # for signature, seqId, "id" key, id @@ -47,6 +49,8 @@ type of kBytes: bytes: seq[byte] + EnrResult*[T] = Result[T, cstring] + template toField[T](v: T): Field = when T is string: Field(kind: kString, str: v) @@ -59,20 +63,24 @@ template toField[T](v: T): Field = else: {.error: "Unsupported field type".} -proc makeEnrAux(seqNum: uint64, pk: PrivateKey, pairs: openarray[(string, Field)]): Record = - result.pairs = @pairs - result.seqNum = seqNum +proc makeEnrAux(seqNum: uint64, pk: PrivateKey, + pairs: openarray[(string, Field)]): EnrResult[Record] = + var record: Record + record.pairs = @pairs + record.seqNum = seqNum - let pubkey = pk.toPublicKey().tryGet() + let pubkey = ? pk.toPublicKey() - result.pairs.add(("id", Field(kind: kString, str: "v4"))) - result.pairs.add(("secp256k1", Field(kind: kBytes, bytes: @(pubkey.toRawCompressed())))) + record.pairs.add(("id", Field(kind: kString, str: "v4"))) + record.pairs.add(("secp256k1", + Field(kind: kBytes, bytes: @(pubkey.toRawCompressed())))) # Sort by key - result.pairs.sort() do(a, b: (string, Field)) -> int: + record.pairs.sort() do(a, b: (string, Field)) -> int: cmp(a[0], b[0]) - proc append(w: var RlpWriter, seqNum: uint64, pairs: openarray[(string, Field)]): seq[byte] = + proc append(w: var RlpWriter, seqNum: uint64, + pairs: openarray[(string, Field)]): seq[byte] {.raises: [].} = w.append(seqNum) for (k, v) in pairs: w.append(k) @@ -83,19 +91,20 @@ proc makeEnrAux(seqNum: uint64, pk: PrivateKey, pairs: openarray[(string, Field) w.finish() let toSign = block: - var w = initRlpList(result.pairs.len * 2 + 1) - w.append(seqNum, result.pairs) + var w = initRlpList(record.pairs.len * 2 + 1) + w.append(seqNum, record.pairs) - let sig = signNR(pk, toSign) - if sig.isErr: - raise newException(CatchableError, "Could not sign ENR (internal error)") + let sig = ? signNR(pk, toSign) - result.raw = block: - var w = initRlpList(result.pairs.len * 2 + 2) - w.append(sig[].toRaw()) - w.append(seqNum, result.pairs) + record.raw = block: + var w = initRlpList(record.pairs.len * 2 + 2) + w.append(sig.toRaw()) + w.append(seqNum, record.pairs) -macro initRecord*(seqNum: uint64, pk: PrivateKey, pairs: untyped{nkTableConstr}): untyped = + ok(record) + +macro initRecord*(seqNum: uint64, pk: PrivateKey, + pairs: untyped{nkTableConstr}): untyped = for c in pairs: c.expectKind(nnkExprColonExpr) c[1] = newCall(bindSym"toField", c[1]) @@ -110,7 +119,8 @@ proc init*(T: type Record, seqNum: uint64, pk: PrivateKey, ip: Option[IpAddress], tcpPort, udpPort: Port, - extraFields: openarray[FieldPair] = []): T = + extraFields: openarray[FieldPair] = []): + EnrResult[T] = var fields = newSeq[FieldPair]() if ip.isSome(): @@ -129,7 +139,7 @@ proc init*(T: type Record, seqNum: uint64, fields.add extraFields makeEnrAux(seqNum, pk, fields) -proc getField(r: Record, name: string, field: var Field): bool = +proc getField(r: Record, name: string, field: var Field): bool {.raises: [].} = # It might be more correct to do binary search, # as the fields are sorted, but it's unlikely to # make any difference in reality. @@ -138,11 +148,11 @@ proc getField(r: Record, name: string, field: var Field): bool = field = v return true -proc requireKind(f: Field, kind: FieldKind) = +proc requireKind(f: Field, kind: FieldKind) {.raises: [ValueError].} = if f.kind != kind: raise newException(ValueError, "Wrong field kind") -proc get*(r: Record, key: string, T: type): T = +proc get*(r: Record, key: string, T: type): T {.raises: [ValueError].} = var f: Field if r.getField(key, f): when T is SomeInteger: @@ -180,7 +190,7 @@ proc get*(r: Record, T: type PublicKey): Option[T] {.raises: [Defect].} = if pk.isOk: return some pk[] -proc tryGet*(r: Record, key: string, T: type): Option[T] = +proc tryGet*(r: Record, key: string, T: type): Option[T] {.raises: [].} = try: return some get(r, key, T) except CatchableError: @@ -205,7 +215,8 @@ proc toTypedRecord*(r: Record): Option[TypedRecord] = return some(tr) -proc verifySignatureV4(r: Record, sigData: openarray[byte], content: seq[byte]): bool = +proc verifySignatureV4(r: Record, sigData: openarray[byte], content: seq[byte]): + bool = let publicKey = r.get(PublicKey) if publicKey.isSome: let sig = SignatureNR.fromRaw(sigData) @@ -213,7 +224,7 @@ proc verifySignatureV4(r: Record, sigData: openarray[byte], content: seq[byte]): var h = keccak256.digest(content) return verify(sig[], h, publicKey.get) -proc verifySignature(r: Record): bool = +proc verifySignature(r: Record): bool {.raises: [RlpError, Defect].} = var rlp = rlpFromBytes(r.raw) let sz = rlp.listLen if not rlp.enterList: @@ -236,7 +247,7 @@ proc verifySignature(r: Record): bool = # Unknown Identity Scheme discard -proc fromBytesAux(r: var Record): bool = +proc fromBytesAux(r: var Record): bool {.raises: [RlpError, Defect].} = if r.raw.len > maxEnrSize: return false @@ -275,7 +286,7 @@ proc fromBytesAux(r: var Record): bool = verifySignature(r) proc fromBytes*(r: var Record, s: openarray[byte]): bool = - # Loads ENR from rlp-encoded bytes, and validated the signature. + ## Loads ENR from rlp-encoded bytes, and validated the signature. r.raw = @s try: result = fromBytesAux(r) @@ -283,7 +294,8 @@ proc fromBytes*(r: var Record, s: openarray[byte]): bool = discard proc fromBase64*(r: var Record, s: string): bool = - # Loads ENR from base64-encoded rlp-encoded bytes, and validated the signature. + ## Loads ENR from base64-encoded rlp-encoded bytes, and validated the + ## signature. try: r.raw = Base64Url.decode(s) result = fromBytesAux(r) @@ -291,7 +303,8 @@ proc fromBase64*(r: var Record, s: string): bool = discard proc fromURI*(r: var Record, s: string): bool = - # Loads ENR from its text encoding: base64-encoded rlp-encoded bytes, prefixed with "enr:". + ## Loads ENR from its text encoding: base64-encoded rlp-encoded bytes, + ## prefixed with "enr:". const prefix = "enr:" if s.startsWith(prefix): result = r.fromBase64(s[prefix.len .. ^1]) @@ -299,12 +312,12 @@ proc fromURI*(r: var Record, s: string): bool = template fromURI*(r: var Record, url: EnrUri): bool = fromURI(r, string(url)) -proc toBase64*(r: Record): string = +proc toBase64*(r: Record): string {.raises: [].} = result = Base64Url.encode(r.raw) -proc toURI*(r: Record): string = "enr:" & r.toBase64 +proc toURI*(r: Record): string {.raises: [].} = "enr:" & r.toBase64 -proc `$`(f: Field): string = +proc `$`(f: Field): string {.raises: [].} = case f.kind of kNum: $f.num @@ -313,7 +326,7 @@ proc `$`(f: Field): string = of kString: "\"" & f.str & "\"" -proc `$`*(r: Record): string = +proc `$`*(r: Record): string {.raises: [].} = result = "(" var first = true for (k, v) in r.pairs: @@ -328,10 +341,11 @@ proc `$`*(r: Record): string = proc `==`*(a, b: Record): bool = a.raw == b.raw -proc read*(rlp: var Rlp, T: typedesc[Record]): T {.inline.} = +proc read*(rlp: var Rlp, T: typedesc[Record]): + T {.inline, raises:[RlpError, ValueError, Defect].} = if not result.fromBytes(rlp.rawData): raise newException(ValueError, "Could not deserialize") rlp.skipElem() -proc append*(rlpWriter: var RlpWriter, value: Record) = +proc append*(rlpWriter: var RlpWriter, value: Record) {.raises: [].} = rlpWriter.appendRawBytes(value.raw) diff --git a/eth/p2p/discoveryv5/node.nim b/eth/p2p/discoveryv5/node.nim index 76c2c7e..d32dd68 100644 --- a/eth/p2p/discoveryv5/node.nim +++ b/eth/p2p/discoveryv5/node.nim @@ -2,6 +2,8 @@ import std/[net, hashes], nimcrypto, stint, chronicles, types, enr, eth/keys, ../enode +{.push raises: [Defect].} + type Node* = ref object node*: ENode @@ -11,21 +13,13 @@ type proc toNodeId*(pk: PublicKey): NodeId = readUintBE[256](keccak256.digest(pk.toRaw()).data) -proc newNode*(enode: ENode): Node = - Node(node: enode, - id: enode.pubkey.toNodeId()) - +# TODO: Lets not allow to create a node where enode info is not in sync with the +# record proc newNode*(enode: ENode, r: Record): Node = Node(node: enode, id: enode.pubkey.toNodeId(), record: r) -proc newNode*(uriString: string): Node = - newNode ENode.fromString(uriString).tryGet() - -proc newNode*(pk: PublicKey, address: Address): Node = - newNode ENode(pubkey: pk, address: address) - proc newNode*(r: Record): Node = # TODO: Handle IPv6 var a: Address @@ -37,28 +31,33 @@ proc newNode*(r: Record): Node = a = Address(ip: IpAddress(family: IpAddressFamily.IPv4, address_v4: ipBytes), udpPort: Port udpPort) - except KeyError: + except KeyError, ValueError: # TODO: This will result in a 0.0.0.0 address. Might introduce more bugs. # Maybe we shouldn't allow the creation of Node from Record without IP. # Will need some refactor though. discard - let pk = PublicKey.fromRaw(r.get("secp256k1", seq[byte])) - if pk.isErr: - warn "Could not recover public key", err = pk.error + let pk = r.get(PublicKey) + if pk.isNone(): + warn "Could not recover public key from ENR" return - result = newNode(ENode(pubkey: pk[], address: a)) - result.record = r + let enode = ENode(pubkey: pk.get(), address: a) + result = Node(node: enode, + id: enode.pubkey.toNodeId(), + record: r) proc hash*(n: Node): hashes.Hash = hash(n.node.pubkey.toRaw) -proc `==`*(a, b: Node): bool = (a.isNil and b.isNil) or (not a.isNil and not b.isNil and a.node.pubkey == b.node.pubkey) +proc `==`*(a, b: Node): bool {.raises: [].} = + (a.isNil and b.isNil) or + (not a.isNil and not b.isNil and a.node.pubkey == b.node.pubkey) -proc address*(n: Node): Address {.inline.} = n.node.address +proc address*(n: Node): Address {.inline, raises: [].} = n.node.address -proc updateEndpoint*(n: Node, a: Address) {.inline.} = n.node.address = a +proc updateEndpoint*(n: Node, a: Address) {.inline, raises: [].} = + n.node.address = a -proc `$`*(n: Node): string = +proc `$`*(n: Node): string {.raises: [].} = if n == nil: "Node[local]" else: diff --git a/eth/p2p/discoveryv5/protocol.nim b/eth/p2p/discoveryv5/protocol.nim index a1c7720..2c69cd3 100644 --- a/eth/p2p/discoveryv5/protocol.nim +++ b/eth/p2p/discoveryv5/protocol.nim @@ -585,7 +585,8 @@ proc newProtocol*(privKey: PrivateKey, db: Database, a = Address(ip: externalIp.get(IPv4_any()), tcpPort: tcpPort, udpPort: udpPort) enode = ENode(pubkey: privKey.toPublicKey().tryGet(), address: a) - enrRec = enr.Record.init(1, privKey, externalIp, tcpPort, udpPort, localEnrFields) + enrRec = enr.Record.init(1, privKey, externalIp, tcpPort, udpPort, + localEnrFields).expect("Properly intialized private key") node = newNode(enode, enrRec) result = Protocol( diff --git a/eth/p2p/discoveryv5/routing_table.nim b/eth/p2p/discoveryv5/routing_table.nim index b1b633b..14f3092 100644 --- a/eth/p2p/discoveryv5/routing_table.nim +++ b/eth/p2p/discoveryv5/routing_table.nim @@ -3,6 +3,8 @@ import stint, chronicles, types, node +{.push raises: [Defect].} + type RoutingTable* = object thisNode: Node @@ -19,11 +21,11 @@ const BITS_PER_HOP = 8 ID_SIZE = 256 -proc distanceTo(n: Node, id: NodeId): UInt256 = +proc distanceTo(n: Node, id: NodeId): UInt256 {.raises: [].} = ## Calculate the distance to a NodeId. n.id xor id -proc logDist*(a, b: NodeId): uint32 = +proc logDist*(a, b: NodeId): uint32 {.raises: [].} = ## Calculate the logarithmic distance between two `NodeId`s. ## ## According the specification, this is the log base 2 of the distance. But it @@ -42,7 +44,7 @@ proc logDist*(a, b: NodeId): uint32 = break return uint32(a.len * 8 - lz) -proc newKBucket(istart, iend: NodeId): KBucket = +proc newKBucket(istart, iend: NodeId): KBucket {.raises: [].} = result.new() result.istart = istart result.iend = iend @@ -53,13 +55,13 @@ proc midpoint(k: KBucket): NodeId = k.istart + (k.iend - k.istart) div 2.u256 proc distanceTo(k: KBucket, id: NodeId): UInt256 = k.midpoint xor id -proc nodesByDistanceTo(k: KBucket, id: NodeId): seq[Node] = +proc nodesByDistanceTo(k: KBucket, id: NodeId): seq[Node] {.raises: [].} = sortedByIt(k.nodes, it.distanceTo(id)) -proc len(k: KBucket): int {.inline.} = k.nodes.len -proc head(k: KBucket): Node {.inline.} = k.nodes[0] +proc len(k: KBucket): int {.inline, raises: [].} = k.nodes.len +proc head(k: KBucket): Node {.inline, raises: [].} = k.nodes[0] -proc add(k: KBucket, n: Node): Node = +proc add(k: KBucket, n: Node): Node {.raises: [].} = ## Try to add the given node to this bucket. ## If the node is already present, it is moved to the tail of the list, and we return nil. @@ -82,7 +84,7 @@ proc add(k: KBucket, n: Node): Node = return k.head return nil -proc removeNode(k: KBucket, n: Node) = +proc removeNode(k: KBucket, n: Node) {.raises: [].} = let i = k.nodes.find(n) if i != -1: k.nodes.delete(i) @@ -98,12 +100,10 @@ proc split(k: KBucket): tuple[lower, upper: KBucket] = let bucket = if node.id <= splitid: result.lower else: result.upper bucket.replacementCache.add(node) -proc inRange(k: KBucket, n: Node): bool {.inline.} = +proc inRange(k: KBucket, n: Node): bool {.inline, raises: [].} = k.istart <= n.id and n.id <= k.iend -proc isFull(k: KBucket): bool = k.len == BUCKET_SIZE - -proc contains(k: KBucket, n: Node): bool = n in k.nodes +proc contains(k: KBucket, n: Node): bool {.raises: [].} = n in k.nodes proc binaryGetBucketForNode(buckets: openarray[KBucket], id: NodeId): KBucket {.inline.} = @@ -116,8 +116,10 @@ proc binaryGetBucketForNode(buckets: openarray[KBucket], if bucket.istart <= id and id <= bucket.iend: result = bucket + # TODO: Is this really an error that should occur? Feels a lot like a work- + # around to another problem. Set to Defect for now. if result.isNil: - raise newException(ValueError, "No bucket found for node with id " & $id) + raise (ref Defect)(msg: "No bucket found for node with id " & $id) proc computeSharedPrefixBits(nodes: openarray[Node]): int = ## Count the number of prefix bits shared by all nodes. @@ -138,7 +140,7 @@ proc computeSharedPrefixBits(nodes: openarray[Node]): int = doAssert(false, "Unable to calculate number of shared prefix bits") -proc init*(r: var RoutingTable, thisNode: Node) {.inline.} = +proc init*(r: var RoutingTable, thisNode: Node) {.inline, raises: [].} = r.thisNode = thisNode r.buckets = @[newKBucket(0.u256, high(Uint256))] randomize() # for later `randomNodes` selection @@ -186,9 +188,6 @@ proc contains*(r: RoutingTable, n: Node): bool = n in r.bucketForNode(n.id) proc bucketsByDistanceTo(r: RoutingTable, id: NodeId): seq[KBucket] = sortedByIt(r.buckets, it.distanceTo(id)) -proc notFullBuckets(r: RoutingTable): seq[KBucket] = - r.buckets.filterIt(not it.isFull) - proc neighbours*(r: RoutingTable, id: NodeId, k: int = BUCKET_SIZE): seq[Node] = ## Return up to k neighbours of the given node. result = newSeqOfCap[Node](k * 2) @@ -205,7 +204,7 @@ proc neighbours*(r: RoutingTable, id: NodeId, k: int = BUCKET_SIZE): seq[Node] = if result.len > k: result.setLen(k) -proc idAtDistance*(id: NodeId, dist: uint32): NodeId = +proc idAtDistance*(id: NodeId, dist: uint32): NodeId {.raises: [].} = ## Calculate the "lowest" `NodeId` for given logarithmic distance. ## A logarithmic distance obviously covers a whole range of distances and thus ## potential `NodeId`s. @@ -220,10 +219,10 @@ proc neighboursAtDistance*(r: RoutingTable, distance: uint32, # that are exactly the requested distance. keepIf(result, proc(n: Node): bool = logDist(n.id, r.thisNode.id) == distance) -proc len*(r: RoutingTable): int = +proc len*(r: RoutingTable): int {.raises: [].} = for b in r.buckets: result += b.len -proc moveRight[T](arr: var openarray[T], a, b: int) {.inline.} = +proc moveRight[T](arr: var openarray[T], a, b: int) {.inline, raises: [].} = ## In `arr` move elements in range [a, b] right by 1. var t: T shallowCopy(t, arr[b + 1]) @@ -241,7 +240,7 @@ proc setJustSeen*(r: RoutingTable, n: Node) = b.nodes[0] = n b.lastUpdated = epochTime() -proc nodeToRevalidate*(r: RoutingTable): Node {.raises:[].} = +proc nodeToRevalidate*(r: RoutingTable): Node {.raises: [].} = var buckets = r.buckets shuffle(buckets) # TODO: Should we prioritize less-recently-updated buckets instead? diff --git a/tests/p2p/test_discoveryv5.nim b/tests/p2p/test_discoveryv5.nim index 055a0d6..de00ed8 100644 --- a/tests/p2p/test_discoveryv5.nim +++ b/tests/p2p/test_discoveryv5.nim @@ -35,7 +35,7 @@ proc randomPacket(tag: PacketTag): seq[byte] = proc generateNode(privKey = PrivateKey.random()[], port: int = 20302): Node = let port = Port(port) let enr = enr.Record.init(1, privKey, some(parseIpAddress("127.0.0.1")), - port, port) + port, port).expect("Properly intialized private key") result = newNode(enr) proc nodeAtDistance(n: Node, d: uint32): Node = @@ -344,7 +344,7 @@ suite "Discovery v5 Tests": # TODO: need to add some logic to update ENRs properly targetSeqNum.inc() let r = enr.Record.init(targetSeqNum, targetKey, - some(targetAddress.ip), targetAddress.tcpPort, targetAddress.udpPort) + some(targetAddress.ip), targetAddress.tcpPort, targetAddress.udpPort)[] targetNode.localNode.record = r targetNode.open() let n = await mainNode.resolve(targetId) @@ -358,7 +358,7 @@ suite "Discovery v5 Tests": block: targetSeqNum.inc() let r = enr.Record.init(3, targetKey, some(targetAddress.ip), - targetAddress.tcpPort, targetAddress.udpPort) + targetAddress.tcpPort, targetAddress.udpPort)[] targetNode.localNode.record = r let pong = await targetNode.ping(lookupNode.localNode) require pong.isSome() diff --git a/tests/p2p/test_enr.nim b/tests/p2p/test_enr.nim index 9edb39b..7edac0d 100644 --- a/tests/p2p/test_enr.nim +++ b/tests/p2p/test_enr.nim @@ -7,7 +7,7 @@ suite "ENR": test "Serialization": var pk = PrivateKey.fromHex( "5d2908f3f09ea1ff2e327c3f623159639b00af406e9009de5fd4b910fc34049d")[] - var r = initRecord(123, pk, {"udp": 1234'u, "ip": [byte 5, 6, 7, 8]}) + var r = initRecord(123, pk, {"udp": 1234'u, "ip": [byte 5, 6, 7, 8]})[] doAssert($r == """(id: "v4", ip: 0x05060708, secp256k1: 0x02E51EFA66628CE09F689BC2B82F165A75A9DDECBB6A804BE15AC3FDF41F3B34E7, udp: 1234)""") let uri = r.toURI() var r2: Record @@ -35,7 +35,7 @@ suite "ENR": let keys = KeyPair.random()[] ip = parseIpAddress("10.20.30.40") - enr = Record.init(100, keys.seckey, some(ip), Port(9000), Port(9000), @[]) + enr = Record.init(100, keys.seckey, some(ip), Port(9000), Port(9000), @[])[] typedEnr = get enr.toTypedRecord() check: @@ -54,7 +54,7 @@ suite "ENR": test "ENR without address": let keys = KeyPair.random()[] - enr = Record.init(100, keys.seckey, none(IpAddress), Port(9000), Port(9000)) + enr = Record.init(100, keys.seckey, none(IpAddress), Port(9000), Port(9000))[] typedEnr = get enr.toTypedRecord() check: