Discovery ENR extension - server part #5571
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5571 +/- ##
==========================================
+ Coverage 62.23% 62.23% +<.01%
==========================================
Files 347 347
Lines 29136 29228 +92
Branches 3295 3306 +11
==========================================
+ Hits 18132 18190 +58
- Misses 9826 9848 +22
- Partials 1178 1190 +12 |
| auto* ctx = getCtx(); | ||
|
|
||
| secp256k1_pubkey rawPubkey; | ||
| if (!secp256k1_ec_pubkey_parse( |
There was a problem hiding this comment.
geth's similar code for reference: https://github.com/ethereum/go-ethereum/blob/c4109d790ffd26d67feb7745d4af8a8bc5090bd9/crypto/secp256k1/ext.h#L76-L90
|
Not a part of your changes, but all discovery packet types can be constexpr (e.g. PingNode::type, PongNode::type) |
|
Should we also add tests for unexpected ENRRequests (e.g. we haven't completed the endpoint proof for the node, or the node isn't in our node table)? |
|
|
||
| auto enrResponse = dynamic_cast<ENRResponse const&>(*datagram); | ||
| auto const& keyValuePairs = enrResponse.enr->keyValuePairs(); | ||
| BOOST_REQUIRE_EQUAL(RLP{keyValuePairs.at("id")}.toString(), "v4"); |
There was a problem hiding this comment.
(Nit) Instead of hard-coding v4 here should we maybe pull the protocol version from the node table?
Should we also do the same for the key names?
There was a problem hiding this comment.
Do you mean using the constants from the implementation instead of string literals here?
| return sourceNodeEntry; | ||
| } | ||
|
|
||
| std::shared_ptr<NodeEntry> NodeTable::handleENRResponse( |
There was a problem hiding this comment.
We should verify the hash in the response against the request (or are you planning on doing that in another change)?
There was a problem hiding this comment.
Yes, I planned to add the validation when implementing doing something useful with these responses (the last "ENR Client" part in #5551)
But now I also think there's not much useful we can actually do with them in v4 discovery, so maybe I'll leave it just ignoring ENRResponses
There was a problem hiding this comment.
If we listen for responses I think we should validate them so there's 1 less thing to do when we want to actually do something useful with them.
The reason is the test vectors there contain RLP-list in place of additional field in Ping and Pong packets, and the parsing code for ENR extension expects an integer sequence number there.
Yeah would be good, but I'm a bit lazy for that 😜 |
Support ENRRequest / ENRResponse discovery packets.
This doesn't add any change in the Discovery algorithm itself yet, just adds the ability for the node to return its current ENR to peers.
I've decided not to add CLI option to enable this and have it always enabled, as it should be fully backwards-compatible, nodes not supporting ENR extension shouldn't notice any difference.
Also this adds a function to libdevcrypto for converting compressed public key to uncompressed (used only in test for now)
Also renames
DiscoveryDatagram::ts=>DiscoveryDatagram::expirationcc @fjl