multi: add automatic network address discovery. #1522
Conversation
There was a problem hiding this comment.
So, I haven't reviewed this yet, but I will say that if it's doing what the title says, we definitely do not want that. You should never be trusting inbound peers to tell you what your IP is. That is too easy to DoS. On the other hand, what has been discussed, and we do want, is some weighted discovery based on what outbound peers (those you connect to), tell you they think you are.
Based on prior discussions, the following situations need to be handled as well:
Add a flag called --nodiscoverip which allows the user to disable automatic address discovery via this mechanism. That implies it should be discovering by default. However, the discovery should also automatically be disabled in the following cases:
- There is a proxy set (e.g.
--proxyand--onion)- It's generally pointless in this case due to the fact that network is probably manually configured in such a way that auto discover is very unlikely to work, and, more importantly, it's needed to protect privacy when using Tor
- There is an
--externalipexplicitly set- When the user is specifically telling the software what to use, don't fight it!
- Listening is disabled for whatever reason (e.g.
--nolistenis specified, listening was disabled because of--connect, etc)- When not listening, there is no way you could accept connections anyway, so it doesn't make sense to try and discover your external ip and erroneously report it as if it accepts connections
Another consideration is that addresses discovered via --upnp should take precedence over addresses discovered from remote peers. For now, discovery could be disabled when --upnp is specified, much like the other aforementioned cases, however, ideally, it would also randomly report the discovered address to deal with some home providers that have connections whose IP address changes periodically and improperly configured UPnP/NAT-PMP routers.
dnldd
changed the title
dnldd
force-pushed
the
public_ip
branch
2 times, most recently
from
15c5ecc to
0493e60
Compare
dnldd
force-pushed
the
public_ip
branch
3 times, most recently
from
c797ab7 to
44e7879
Compare
There was a problem hiding this comment.
This somewhat works, but honestly, I don't think the approach is the right one at all and the current incarnation is also wrong in many ways.
This is not an exhaustive list of the incorrect things I see since I don't want to spend any more time on it in the current state, but a few examples are:
- Improperly uses the
Youfield on inbound conns - Doesn't considering routability of the reported address which could be incorrect
- Doesn't consider
simnetproperly - No tor support
In terms of the approach, running a separate goroutine that is constantly iterating all of the current peers is wasteful as it results in unnecessary work when the peers haven't even changed and loses all history due to the transient nature of peers. Instead, I think it really should keep a map protected by a mutex that is updated when outbound connections are successfully made and all of the other conditions are met which warrant the address being added as a candidate for the best external address. For example discovery is enabled, the address is routable, the address is reachable (e.g. if you connected to the remote peer via IPv4, but it's reporting you connected from an IPv6 address, it's not reachable), etc.
|
Updated the PR per the issues raised. Tested via Tor and IPv4. Will need help with IPv6 tests since I don't have an IPv6 address (disabled by ISP). |
dnldd
force-pushed
the
public_ip
branch
3 times, most recently
from
dc23944 to
4f3437d
Compare
There was a problem hiding this comment.
I've tested this pretty thoroughly and added some debugging prints to ensure proper behavior. It appears to be working properly with both IPv4 and IPv6. I do have some code quality comments inline.
I also tested the local discovery is disabled in the following cases:
--nolisten--externalipspecified--proxyspecified--onionspecified--nodiscoveripspecified--upnpspecified--regnetspecified--simnetspecified
This discovers the network address(es) of the daemon through connected outbound peers. The address(es) discovered are advertised to subsequent connecting peers.
This discovers the network address of the daemon through connected outbound peers. The address is advertised to subsequent connecting peers if automatic network address discovery is not disabled.