diff --git a/eth/p2p/discoveryv5/encoding.nim b/eth/p2p/discoveryv5/encoding.nim index 866a334..9d470b1 100644 --- a/eth/p2p/discoveryv5/encoding.nim +++ b/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 - newNode = ? newNode(authResp.record).mapErrTo(HandshakeError) - if newNode.id != fromId: - return err(HandshakeError) + var pubKey: PublicKey + if authResp.record.isSome(): + # Node returned might not have an address or not a valid address. + 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) diff --git a/eth/p2p/discoveryv5/enr.nim b/eth/p2p/discoveryv5/enr.nim index 04aac32..c93dd68 100644 --- a/eth/p2p/discoveryv5/enr.nim +++ b/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: - first = false - else: - result &= ", " + result &= ", " result &= k result &= ": " result &= $v diff --git a/eth/p2p/discoveryv5/protocol.nim b/eth/p2p/discoveryv5/protocol.nim index bc94706..6b2d744 100644 --- a/eth/p2p/discoveryv5/protocol.nim +++ b/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 - ## contact if for newer information. If node is not known or it does not - ## reply, a lookup is done to see if it can find a (newer) record of the node - ## on the network. + ## This will first look in the own routing table. If the node is known, it + ## will try to contact if for newer information. If node is not known or it + ## does not reply, a lookup is done to see if it can find a (newer) record of + ## 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 - discard + # Request new ENR + 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. diff --git a/eth/p2p/discoveryv5/routing_table.nim b/eth/p2p/discoveryv5/routing_table.nim index 6bd7dfd..ddd2bbe 100644 --- a/eth/p2p/discoveryv5/routing_table.nim +++ b/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) - k.replacementCache.add(n) + if k.replacementCache[nodeIdx].record.seqNum <= n.record.seqNum: + # 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: diff --git a/eth/p2p/discoveryv5/types.nim b/eth/p2p/discoveryv5/types.nim index 0c700f4..74631ed 100644 --- a/eth/p2p/discoveryv5/types.nim +++ b/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) diff --git a/tests/p2p/test_discoveryv5.nim b/tests/p2p/test_discoveryv5.nim index a03da9f..5275df0 100644 --- a/tests/p2p/test_discoveryv5.nim +++ b/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 - # if resolve works (only local lookup) + # Populate routing table with target through a ping. Next, close target and + # 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) diff --git a/tests/p2p/test_discv5_encoding.nim b/tests/p2p/test_discv5_encoding.nim index 5b48e2d..5c153f0 100644 --- a/tests/p2p/test_discv5_encoding.nim +++ b/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) - var rlp = rlpFromBytes(auth) - let authHeader = rlp.read(AuthHeader) - var newNode: Node - let secrets = c.decodeAuthResp(privKey.toPublicKey().toNodeId(), - authHeader, whoareyou, newNode) + block: # With ENR + let + whoareyou = Whoareyou(idNonce: idNonce, recordSeq: 0, 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) + + 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 diff --git a/tests/p2p/test_enr.nim b/tests/p2p/test_enr.nim index ff9dce2..b1eeb57 100644 --- a/tests/p2p/test_enr.nim +++ b/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(