net: Don't return unreachable local addresses for peer #15671

It is possible for GetLocal to return an unreachable local address if there are no reachable ones. This change makes sure that it doesn't.

I think it would help review if the bugfix came with a test case that failed before the fix

utACK 2cc733d, agree with Marco. Not sure about the TODO comment, maybe something like "// Local address reachability must be at least REACH_DEFAULT (1)"

Neither the commit message nor the PR explain the motivation for the change. It isn't self apparent.

I have some concern that this might make the behaviour incorrect?

Say you only have a RFC1918 address because you have a port forward but don't know your external IP (e.g. not running the UPNP security hole...). https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L181 calls GetLocalAddress, which calls GetLocal, which only finds your RFC1918 address, which is REACH_UNREACH https://github.com/bitcoin/bitcoin/blob/master/src/netaddress.cpp#L428 (pretty much the only case that is!), which gets returned which GetLocalAddress notes "IP may be changed to a useful one" https://github.com/bitcoin/bitcoin/blob/master/src/netaddress.cpp#L428 which happens back in Advertiselocal. https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L193 Also in the case a non-routable address is returned it is always overridden in Advertiselocal.

I believe after this change, this can never happen because the REACH_UNREACH will cause a false result that will propagate up and so a portmapped host will never successfully announce itself on the network... but I could be mistaken, it was just a quick glance and I'm not sure what this PR is actually intending to accomplish.

I have some concern that this might make the behaviour incorrect?

Say you only have a RFC1918 address because you have a port forward but don't know your external IP (e.g. not running the UPNP security hole...). /src/net.cpp@master#L181 calls GetLocalAddress, which calls GetLocal, which only finds your RFC1918 address, which is REACH_UNREACH /src/netaddress.cpp@master#L428 (pretty much the only case that is!), which gets returned which GetLocalAddress notes "IP may be changed to a useful one" /src/netaddress.cpp@master#L428 which happens back in Advertiselocal. /src/net.cpp@master#L193 Also in the case a non-routable address is returned it is always overridden in Advertiselocal.

I believe after this change, this can never happen because the REACH_UNREACH will cause a false result that will propagate up and so a portmapped host will never successfully announce itself on the network... but I could be mistaken, it was just a quick glance and I'm not sure what this PR is actually intending to accomplish.

You are absolutely right. I will note this point in #15672 so it's clearer for future developers.