net: Add addresses to local addr map even if their network is unreachable

fjahr commented at 8:27 pm on July 24, 2022: contributor

Closes #25669 and possibly also addresses #25336

Local addresses specified with externalip are currently not added to mapLocalHost if their network is set unreachable. This does not seem to be necessary because typically the network is set unreachable if outbound connections via the network are disabled via onion=0, onlynet etc. On the inbound side this does not necessarily mean there are any restrictions in place. For the use cases that Matt and kroese seem to have there rather seems to be a deliberate choice that a net is not reachable outbound although it is available inbound.

From my understanding mapLocalHost is used to prevent the node from connecting to itself and getting a local address to announce to a peer. For the first use case this change may mean that there are some addresses in the map that are never checked because there are no outbound connections on that net. But that doesn’t seem to be an issue. For the second use case the upside is that the address actually gets announced to peers. In the case of the open issues mentioned above this does seem to be the intention of the users. On the downside this may also mean that the address is announced even when this particular network is blocked in some way. This seems to have been the original intention by sipa when adding this check but also this was in 2012 and I am sure some of the context was different. In this case it makes more sense to me to assume that the users know what they are doing rather than assuming there is some network issue the user not aware of.

Additionally the change means that these addresses now appear in getnetworkinfo.

I have considered and drafted other ways of addressing this issue but this seems to be the most straight forward approach unless I am overlooking something and the downside outlined above is too big of an issue.

net: Add addresses to local hosts even if their network is unreachable 5910c2f4e3

fanquake added the label P2P on Jul 24, 2022

mzumsande commented at 8:43 pm on July 24, 2022: contributor

Just for reference, #24835 (which recently got closed) attempted the same, although for another reason. There is some more relevant discussion there.

I think that this this from my comment still applies

If removing the IsReachable call from AddLocal(), I think that IsPeerAddrLocalGood() should be adjusted too

fjahr commented at 9:51 pm on July 24, 2022: contributor

Just for reference, #24835 (which recently got closed) attempted the same, although for another reason. There is some more relevant discussion there.

I think that this this from my comment still applies

If removing the IsReachable call from AddLocal(), I think that IsPeerAddrLocalGood() should be adjusted too

Thanks! I missed this but the discussion there seems to be in line with what I found. Since there was no clear support for this approach I will close this here and maybe think about suggesting another solution.

fjahr closed this on Jul 24, 2022

mzumsande commented at 10:22 pm on July 24, 2022: contributor

I think it’s not ideal that the -onlynet help says that Inbound and manual connections are not affected, when they are in reality (by not advertising own addresses to these networks, making it unlikely for others to connect to us unless they knew our address before). So, I’m not against this change, but I hadn’t come to a final opinion either, because I think it might also be weird for a user to activate -onlynet and keep on advertising their clearnet address to the network, without a clear option to disable this or disable incoming connections from these networks. Maybe ideally, we’d have a way of specifying -onlynet separately for outbounds/inbounds/both, depending on what the user wants? It might be good to discuss the broader direction in an issue, considering how frequently it comes up.

fjahr commented at 10:59 pm on July 26, 2022: contributor

I think it’s not ideal that the -onlynet help says that Inbound and manual connections are not affected, when they are in reality (by not advertising own addresses to these networks, making it unlikely for others to connect to us unless they knew our address before). So, I’m not against this change, but I hadn’t come to a final opinion either, because I think it might also be weird for a user to activate -onlynet and keep on advertising their clearnet address to the network, without a clear option to disable this or disable incoming connections from these networks. Maybe ideally, we’d have a way of specifying -onlynet separately for outbounds/inbounds/both, depending on what the user wants? It might be good to discuss the broader direction in an issue, considering how frequently it comes up.

Yeah, I had been thinking about fixing this with a new config option as well, although I went into a different direction than you had in mind. I would be happy if you could give your thoughts on my draft in #25718.

vasild commented at 11:26 am on July 28, 2022: contributor

it might also be weird for a user to activate -onlynet and keep on advertising their clearnet address to the network, without a clear option to disable this or disable incoming connections from these networks.

There is a way to restrict incoming connections by network type - use -bind. If none are desired, then use -listen=0. For example if the user has connectivity to both IPv4 and IPv6 but wants to use only IPv4 for outgoing and incoming connections, they need to use -onlynet=ipv4 together with -bind=1.2.3.4:8333.

Or -onlynet=onion -bind=127.0.0.1:8334=onion to disable incoming connections via clearnet (assuming 127.0.0.1 is not reachable from the internet).

bitcoin locked this on Jul 28, 2023

net: Add addresses to local addr map even if their network is unreachable #25690