Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

fjahr
Copy link
Contributor

@fjahr fjahr commented Jul 24, 2022

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.

@fanquake fanquake added the P2P label Jul 24, 2022
mzumsande
mzumsande reviewed Jul 24, 2022
View reviewed changes
Copy link
Contributor

@mzumsande mzumsande left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

fjahr commented Jul 24, 2022

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 fjahr closed this Jul 24, 2022
@mzumsande
Copy link
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 fjahr mentioned this pull request Jul 26, 2022
Closed
@fjahr
Copy link
Contributor Author

fjahr commented Jul 26, 2022

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 vasild mentioned this pull request Jul 28, 2022
Closed
@vasild
Copy link
Contributor

vasild commented Jul 28, 2022

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 bitcoin locked and limited conversation to collaborators Jul 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@mzumsande mzumsande mzumsande left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

externalip=...onion ignored when onion/prox not set
4 participants
@fjahr @mzumsande @vasild @fanquake