From cca931d0b59edeb7cbe6d9e3be37b794cf2e4a16 Mon Sep 17 00:00:00 2001 From: Zahary Karadjov Date: Tue, 18 Feb 2020 00:47:13 +0200 Subject: [PATCH 1/2] Reduce the use of the general Exception type and improve the exception tarcking in protocol.receive --- eth/p2p/discoveryv5/discovery_db.nim | 27 ++++++++++++++++++--------- eth/p2p/discoveryv5/encoding.nim | 22 ++++++++++------------ eth/p2p/discoveryv5/enr.nim | 10 +++++++--- eth/p2p/discoveryv5/protocol.nim | 28 ++++++++++++++++++++-------- eth/p2p/discoveryv5/types.nim | 5 +++-- eth/trie/db.nim | 16 ++++++++++++---- 6 files changed, 70 insertions(+), 38 deletions(-) diff --git a/eth/p2p/discoveryv5/discovery_db.nim b/eth/p2p/discoveryv5/discovery_db.nim index 0b02180..bb9681d 100644 --- a/eth/p2p/discoveryv5/discovery_db.nim +++ b/eth/p2p/discoveryv5/discovery_db.nim @@ -27,15 +27,24 @@ proc makeKey(id: NodeId, address: Address): array[keySize, byte] = copyMem(addr result[sizeof(id) + 1], unsafeAddr address.ip.address_v6, sizeof(address.ip.address_v6)) copyMem(addr result[sizeof(id) + 1 + sizeof(address.ip.address_v6)], unsafeAddr address.udpPort, sizeof(address.udpPort)) -method storeKeys*(db: DiscoveryDB, id: NodeId, address: Address, r, w: array[16, byte]) = - var value: array[sizeof(r) + sizeof(w), byte] - value[0 .. 15] = r - value[16 .. ^1] = w - db.backend.put(makeKey(id, address), value) +method storeKeys*(db: DiscoveryDB, id: NodeId, address: Address, r, w: array[16, byte]): bool {.raises: [Defect].} = + try: + var value: array[sizeof(r) + sizeof(w), byte] + value[0 .. 15] = r + value[16 .. ^1] = w + db.backend.put(makeKey(id, address), value) + return true + except CatchableError: + return false -method loadKeys*(db: DiscoveryDB, id: NodeId, address: Address, r, w: var array[16, byte]): bool = - let res = db.backend.get(makeKey(id, address)) - if res.len == sizeof(r) + sizeof(w): +method loadKeys*(db: DiscoveryDB, id: NodeId, address: Address, r, w: var array[16, byte]): bool {.raises: [Defect].} = + try: + let res = db.backend.get(makeKey(id, address)) + if res.len != sizeof(r) + sizeof(w): + return false copyMem(addr r[0], unsafeAddr res[0], sizeof(r)) copyMem(addr w[0], unsafeAddr res[sizeof(r)], sizeof(w)) - result = true + return true + except CatchableError: + return false + diff --git a/eth/p2p/discoveryv5/encoding.nim b/eth/p2p/discoveryv5/encoding.nim index f804428..5c14caf 100644 --- a/eth/p2p/discoveryv5/encoding.nim +++ b/eth/p2p/discoveryv5/encoding.nim @@ -32,13 +32,14 @@ type ephemeralKey*: array[64, byte] response*: seq[byte] + RandomSourceDepleted* = object of CatchableError const gcmTagSize = 16 proc randomBytes(v: var openarray[byte]) = if nimcrypto.randomBytes(v) != v.len: - raise newException(Exception, "Could not randomize bytes") # TODO: + raise newException(RandomSourceDepleted, "Could not randomize bytes") proc idNonceHash(nonce, ephkey: openarray[byte]): array[32, byte] = var ctx: sha256 @@ -50,12 +51,12 @@ proc idNonceHash(nonce, ephkey: openarray[byte]): array[32, byte] = proc signIDNonce(c: Codec, idNonce, ephKey: openarray[byte]): SignatureNR = if signRawMessage(idNonceHash(idNonce, ephKey), c.privKey, result) != EthKeysStatus.Success: - raise newException(Exception, "Could not sign idNonce") + raise newException(EthKeysException, "Could not sign idNonce") proc deriveKeys(n1, n2: NodeID, priv: PrivateKey, pub: PublicKey, challenge: Whoareyou, result: var HandshakeSecrets) = var eph: SharedSecretFull if ecdhAgree(priv, pub, eph) != EthKeysStatus.Success: - raise newException(Exception, "ecdhAgree failed") + raise newException(EthKeysException, "ecdhAgree failed") # TODO: Unneeded allocation here var info = newSeqOfCap[byte](idNoncePrefix.len + 32 * 2) @@ -85,16 +86,12 @@ proc makeAuthHeader(c: Codec, toNode: Node, nonce: array[gcmNonceSize, byte], if challenge.recordSeq < ln.record.seqNum: resp.record = ln.record - var remotePubkey: PublicKey - if not toNode.record.get(remotePubkey): - raise newException(Exception, "Could not get public key from remote ENR") # Should not happen! - let ephKey = newPrivateKey() let ephPubkey = ephKey.getPublicKey().getRaw resp.signature = c.signIDNonce(challenge.idNonce, ephPubkey).getRaw - deriveKeys(ln.id, toNode.id, ephKey, remotePubkey, challenge, handhsakeSecrets) + deriveKeys(ln.id, toNode.id, ephKey, toNode.node.pubKey, challenge, handhsakeSecrets) let respRlp = rlp.encode(resp) @@ -134,8 +131,8 @@ proc encodeEncrypted*(c: Codec, toNode: Node, packetData: seq[byte], challenge: headEnc = c.makeAuthHeader(toNode, nonce, sec, challenge) writeKey = sec.writeKey - - c.db.storeKeys(toNode.id, toNode.address, sec.readKey, sec.writeKey) + # TODO: is it safe to ignore the error here? + discard c.db.storeKeys(toNode.id, toNode.address, sec.readKey, sec.writeKey) var body = packetData let tag = packetTag(toNode.id, c.localNode.id) @@ -228,7 +225,8 @@ proc decodeEncrypted*(c: var Codec, fromId: NodeID, fromAddr: Address, input: se # Swap keys to match remote swap(sec.readKey, sec.writeKey) - c.db.storeKeys(fromId, fromAddr, sec.readKey, sec.writeKey) + # TODO: is it safe to ignore the error here? + discard c.db.storeKeys(fromId, fromAddr, sec.readKey, sec.writeKey) readKey = sec.readKey else: @@ -249,7 +247,7 @@ proc decodeEncrypted*(c: var Codec, fromId: NodeID, fromAddr: Address, input: se proc newRequestId*(): RequestId = if randomBytes(addr result, sizeof(result)) != sizeof(result): - raise newException(Exception, "Could not randomize bytes") # TODO: + raise newException(RandomSourceDepleted, "Could not randomize bytes") # TODO: proc numFields(T: typedesc): int = for k, v in fieldPairs(default(T)): inc result diff --git a/eth/p2p/discoveryv5/enr.nim b/eth/p2p/discoveryv5/enr.nim index 2b904e1..6fc7b8a 100644 --- a/eth/p2p/discoveryv5/enr.nim +++ b/eth/p2p/discoveryv5/enr.nim @@ -84,7 +84,7 @@ proc makeEnrAux(seqNum: uint64, pk: PrivateKey, pairs: openarray[(string, Field) var sig: SignatureNR if signRawMessage(keccak256.digest(toSign).data, pk, sig) != EthKeysStatus.Success: - raise newException(Exception, "Could not sign ENR (internal error)") + raise newException(EthKeysException, "Could not sign ENR (internal error)") result.raw = block: var w = initRlpList(result.pairs.len * 2 + 2) @@ -136,6 +136,10 @@ proc get*(r: Record, key: string, T: type): T = elif T is string: requireKind(f, kString) return f.str + elif T is PublicKey: + requireKind(f, kBytes) + if recoverPublicKey(f.bytes, result) != EthKeysStatus.Success: + raise newException(ValueError, "Invalid public key") elif T is array: when type(result[0]) is byte: requireKind(f, kBytes) @@ -156,11 +160,11 @@ proc get*(r: Record, pubKey: var PublicKey): bool = proc tryGet*(r: Record, key: string, T: type): Option[T] = try: - return some r.get(key, T) + return some get(r, key, T) except CatchableError: discard -func toTypedRecord*(r: Record): Option[TypedRecord] = +proc toTypedRecord*(r: Record): Option[TypedRecord] = let id = r.tryGet("id", string) if id.isSome: var tr: TypedRecord diff --git a/eth/p2p/discoveryv5/protocol.nim b/eth/p2p/discoveryv5/protocol.nim index abc5110..7d6db55 100644 --- a/eth/p2p/discoveryv5/protocol.nim +++ b/eth/p2p/discoveryv5/protocol.nim @@ -63,7 +63,7 @@ proc send(d: Protocol, n: Node, data: seq[byte]) = proc randomBytes(v: var openarray[byte]) = if nimcrypto.randomBytes(v) != v.len: - raise newException(Exception, "Could not randomize bytes") # TODO: + raise newException(RandomSourceDepleted, "Could not randomize bytes") # TODO: proc `xor`[N: static[int], T](a, b: array[N, T]): array[N, T] = for i in 0 .. a.high: @@ -123,9 +123,18 @@ proc handleFindNode(d: Protocol, fromNode: Node, fn: FindNodePacket, reqId: Requ let distance = min(fn.distance, 256) d.sendNodes(fromNode, reqId, d.routingTable.neighboursAtDistance(distance)) -proc receive(d: Protocol, a: Address, msg: Bytes) - {.gcsafe, raises:[RlpError, Exception].} = - # TODO: figure out where the general exception comes from and clean it up +proc receive(d: Protocol, a: Address, msg: Bytes) {.gcsafe, + raises: [ + Defect, + # TODO This is now coming from Chronos's callSoon + Exception, + # TODO All of these should probably be handled here + RlpError, + IOError, + TransportAddressError, + EthKeysException, + Secp256k1Exception, + ].} = if msg.len < 32: return # Invalid msg @@ -136,9 +145,12 @@ proc receive(d: Protocol, a: Address, msg: Bytes) var pr: PendingRequest if d.pendingRequests.take(whoareyou.authTag, pr): let toNode = pr.node - - let (data, _) = d.codec.encodeEncrypted(toNode, pr.packet, challenge = whoareyou) - d.send(toNode, data) + try: + let (data, _) = d.codec.encodeEncrypted(toNode, pr.packet, challenge = whoareyou) + d.send(toNode, data) + except RandomSourceDepleted as err: + debug "Failed to respond to a who-you-are msg " & + "due to randomness source depletion." else: var tag: array[32, byte] @@ -169,7 +181,7 @@ proc receive(d: Protocol, a: Address, msg: Bytes) if d.awaitedPackets.take((node, packet.reqId), waiter): waiter.complete(packet.some) else: - debug "TODO: handle packet: ", packet = packet.kind, origin = node + debug "TODO: handle packet: ", packet = packet.kind, origin = $node else: debug "Could not decode, respond with whoareyou" diff --git a/eth/p2p/discoveryv5/types.nim b/eth/p2p/discoveryv5/types.nim index 9363ea7..41d0720 100644 --- a/eth/p2p/discoveryv5/types.nim +++ b/eth/p2p/discoveryv5/types.nim @@ -69,8 +69,9 @@ template packetKind*(T: typedesc[SomePacket]): PacketKind = elif T is FindNodePacket: findNode elif T is NodesPacket: nodes -method storeKeys*(db: Database, id: NodeId, address: Address, r, w: array[16, byte]) {.base.} = discard -method loadKeys*(db: Database, id: NodeId, address: Address, r, w: var array[16, byte]): bool {.base.} = discard +method storeKeys*(db: Database, id: NodeId, address: Address, r, w: array[16, byte]): bool {.base, raises: [Defect].} = discard + +method loadKeys*(db: Database, id: NodeId, address: Address, r, w: var array[16, byte]): bool {.base, raises: [Defect].} = discard proc toBytes*(id: NodeId): array[32, byte] {.inline.} = id.toByteArrayBE() diff --git a/eth/trie/db.nim b/eth/trie/db.nim index c0418d7..a9ed8fa 100644 --- a/eth/trie/db.nim +++ b/eth/trie/db.nim @@ -21,10 +21,18 @@ type contains(DB, KeccakHash) is bool # XXX: poor's man vtref types - PutProc = proc (db: RootRef, key, val: openarray[byte]) {.gcsafe.} - GetProc = proc (db: RootRef, key: openarray[byte]): Bytes {.gcsafe.} # Must return empty seq if not found - DelProc = proc (db: RootRef, key: openarray[byte]) {.gcsafe.} - ContainsProc = proc (db: RootRef, key: openarray[byte]): bool {.gcsafe.} + PutProc = proc (db: RootRef, key, val: openarray[byte]) {. + gcsafe, raises: [Defect, CatchableError] .} + + GetProc = proc (db: RootRef, key: openarray[byte]): Bytes {. + gcsafe, raises: [Defect, CatchableError] .} + ## The result will be empty seq if not found + + DelProc = proc (db: RootRef, key: openarray[byte]) {. + gcsafe, raises: [Defect, CatchableError] .} + + ContainsProc = proc (db: RootRef, key: openarray[byte]): bool {. + gcsafe, raises: [Defect, CatchableError] .} TrieDatabaseRef* = ref object obj: RootRef From cfdb26db4097b078f02db172534977bd47e0e6c3 Mon Sep 17 00:00:00 2001 From: Zahary Karadjov Date: Tue, 18 Feb 2020 01:07:23 +0200 Subject: [PATCH 2/2] [discv5] Advertise the LibP2P TCP port properly --- eth/p2p/discoveryv5/protocol.nim | 10 +++++++--- tests/p2p/p2p_test_helper.nim | 5 +++-- tests/p2p/test_discoveryv5.nim | 4 ++-- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/eth/p2p/discoveryv5/protocol.nim b/eth/p2p/discoveryv5/protocol.nim index 7d6db55..ac14a75 100644 --- a/eth/p2p/discoveryv5/protocol.nim +++ b/eth/p2p/discoveryv5/protocol.nim @@ -33,9 +33,12 @@ proc whoareyouMagic(toNode: NodeId): array[32, byte] = for i, c in prefix: data[sizeof(toNode) + i] = byte(c) sha256.digest(data).data -proc newProtocol*(privKey: PrivateKey, db: Database, port: Port): Protocol = +proc newProtocol*(privKey: PrivateKey, + db: Database, + tcpPort, udpPort: Port): Protocol = result = Protocol(privateKey: privKey, db: db) - let a = Address(ip: parseIpAddress("127.0.0.1"), udpPort: port) + let a = Address(ip: parseIpAddress("127.0.0.1"), + tcpPort: tcpPort, udpPort: udpPort) result.localNode = newNode(initENode(result.privateKey.getPublicKey(), a)) result.localNode.record = enr.Record.init(12, result.privateKey, a) @@ -364,7 +367,8 @@ when isMainModule: else: pk = newPrivateKey() - let d = newProtocol(pk, DiscoveryDB.init(newMemoryDB()), Port(12001 + i)) + let d = newProtocol(pk, DiscoveryDB.init(newMemoryDB()), + Port(12001 + i), Port(12001 + i)) d.open() result.add(d) diff --git a/tests/p2p/p2p_test_helper.nim b/tests/p2p/p2p_test_helper.nim index 7a1087c..e75e64c 100644 --- a/tests/p2p/p2p_test_helper.nim +++ b/tests/p2p/p2p_test_helper.nim @@ -6,7 +6,8 @@ var nextPort = 30303 proc localAddress*(port: int): Address = let port = Port(port) - result = Address(udpPort: port, tcpPort: port, ip: parseIpAddress("127.0.0.1")) + result = Address(udpPort: port, tcpPort: port, + ip: parseIpAddress("127.0.0.1")) proc startDiscoveryNode*(privKey: PrivateKey, address: Address, bootnodes: seq[ENode]): Future[DiscoveryProtocol] {.async.} = @@ -47,4 +48,4 @@ proc recvMsgMock*(msg: openArray[byte]): tuple[msgId: int, msgData: Rlp] = var rlp = rlpFromBytes(@msg.toRange) let msgId = rlp.read(int32) - return (msgId.int, rlp) \ No newline at end of file + return (msgId.int, rlp) diff --git a/tests/p2p/test_discoveryv5.nim b/tests/p2p/test_discoveryv5.nim index 4e2776c..7998833 100644 --- a/tests/p2p/test_discoveryv5.nim +++ b/tests/p2p/test_discoveryv5.nim @@ -6,9 +6,9 @@ import ./p2p_test_helper proc startDiscoveryv5Node*(privKey: PrivateKey, address: Address, - bootnodes: seq[Record]): discv5_protocol.Protocol = + bootnodes: seq[Record]): discv5_protocol.Protocol = var db = DiscoveryDB.init(newMemoryDB()) - result = newProtocol(privKey, db, address.udpPort) + result = newProtocol(privKey, db, address.tcpPort, address.udpPort) for node in bootnodes: result.addNode(node)