Shift IP-related ENR fields as optional#1668
Conversation
|
So the point here is that some peers will be behind a NAT and thus their ENR's won't have some of these fields. This isn't necessarily a problem when it comes to using ENRs for DHT discovery because you couldn't discover such peers anyway. But it is a problem if we use ENRs for other usecases such as option 2 in #1637 ^ that all check out? |
|
Yeah, I guess this is more of a discv5 implementation issue for whether it effects DHT discovery. A chat with felix prompted me that ENR's could not specify IPs. If a node connects to you and asks you for peers and you don't know this node. You get it's ENR. Depending on the implementation, you need to know whether to add this new node to your routing table or not. If the connecting node doesn't have an IP field you just ignore it. I think it helps in general if node's haven't specified a specific IP, we can assume they are non-contactable, rather than attempting to connect and finding out after a timeout. For example, you start a new node without specifying a contactable IP, and try and connect to it's ENR. We can now have specific errors such as: "this node has no contactable address" rather than "could not connect to node for some reason". It gives us an extra state to work with. I've found issues with setting the IP to whatever the local listening address is. In general its non-contactable and in these cases I think it's better to just not have an IP field. |
|
I see, thanks for the clarification. Any other comments? @arnetheduck @mkalinin @nisdas @fjl |
|
+1 from us (cc @kdeme) |
|
I think this change is good. |
|
@AgeManning Can you rebase to |
|
this looks good to me 👍 |
Description
In Lighthouse we had these fields always defined as specified. However in a recent update to our discv5 which removes NAT'd peers from the DHT we realised it makes more sense to have nodes that have not sorted out their NAT traversal (i.e do not know or do not have external IP/PORTS that other peers can connect on) that their ENR's do not contain any IP/PORTs.
Nodes without IP/PORTs should still be able to connect and discover other peers, but it will be less likely (depending on implementations) that these peers would be added to a local node routing table.
It makes it clearer that nodes without an IP/PORT do not have a contactable address and will save us time trying to connect to a local IP/PORT which is not globally contactable.
Lighthouse by default, will have ENRs without any IP/PORT field (unless loaded from a previous state), although we set a TCP port (as this is not update-able from discovery). As they discover peers and their peers inform the node that they are contactable on a specific IP/PORT, the node will update it's ENR with IP/PORT field and get added to the DHT. This also ensures that the ENR doesn't get updated unless it is contactable from a wider audience.