That's a commit from 2012 👀 We probably need some discussion here what would be the impact of this change to behavior other than -netinfo.

I think the meaning of -onlynet (previously -blocknet) changed over the years? In commit 91dace3 listening on blocked networks was disabled (and with that 1653f97 makes sense) but nowadays incoming connections are not affected, which is also stated in the description of -onlynet.

@mzumsande @jnewbery @sipa any opinions here?

I agree with @laanwj that the focus should be on the behavioral changes, not so much -netinfo.
It would be good to have more info on the rationale of 1653f97 - was there a PR for this that I'm unable to find?

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

I'm unsure about this, but remember a discussion about the same topic with @vasild in #22834:
I asked:

IsPeerAddrLocalGood() and AddLocal() both call IsReachable() and therefore might be affected. Considering that they deal with the address used for self-advertising (and thus for getting incoming connections) I don't really understand why they make use of IsReachable() in the first place. Shouldn't this affect only outbound connection logic?

vasild's answer:

I think the logic is this - if a network is not reachable then we cannot accept connections from it. "Reachable" in the broader English word meaning, not the stricter -onlynet meaning. For example, a host/OS may have support for both IPv4 and IPv6 but its ISP may only support IPv4. So, an operator would run -onlynet=ipv4 to avoid attempting connections to IPv6 hosts and from advertising an IPv6 address to which the rest of the world cannot connect.

It would be good to have more info on the rationale of 1653f97 - was there a PR for this that I'm unable to find?

Interesting. Looks like there was no PR. One (or the) descendant commit (5e794a9) with unrelated content is not a merge commit, indicating (but not proving) that a merge commit may not exist.

The commit itself (1653f97) was likely written at 3pm. While an unrelated pull on the same day (#1284) was opened at 11am, possibly rebased(?), merged at 6pm, and included 1653f97 in its base.

if a network is not reachable then we cannot accept connections from it

apparently, this is not the case for @dergoegge. If we bind and listen on an address that is omitted from -onlynet, then there are downsides either way:

1. As it is currently in master:

• we will not show the listening address in -netinfo or in getnetworkinfo (bad if peers can reach that address, good otherwise)
• we will not advertise it to other peers (good?)

2. With this PR:

• we will show the listening address in -netinfo or in getnetworkinfo (good if peers can reach that address, bad otherwise)
• we will advertise it to other peers (bad? or at least a significant change in behavior)

I prefer 1. because the "bad" is only a display issue. Other options could be to include a new section in the display: "listening but not advertised addresses". Or to stop bind/listening on addresses omitted from -onlynet (increase its scope as it currently only affects outgoing connections).

PS: actually, we don't know if peers can even reach our address that is omitted from -onlynet. It could be the case as mentioned above that a host/OS has e.g. IPv6 support (and a configured address for some local purposes, maybe), but ISP does not support IPv6. bitcoind can't distinguish between this case and @dergoegge's case. It assumes the former, which looks reasonable to me.

if a network is not reachable then we cannot accept connections from it
apparently, this is not the case for @dergoegge.

@vasild for context: I was playing around with -onlynet and used -onlynet=ipv4 because i wanted my node to only make outbound ipv4 connections but keep getting Tor, ipv6 and ipv4 inbound connections. I wanted to double check with -netinfo but to my surprise only my ipv4 address was shown. However, I noticed that I was still receiving inbound connections on my onion address (i was previously running the node without -onlynet). That seemed inconsistent to me so I opened this PR.

we will advertise it to other peers (bad? or at least a significant change in behavior)

Would we though? afaict we try to announce the address that is best reachable for our peer. So for example, if we connect to a peer through IPv4 and we have a local IPv4 and IPv6 address, we won't announce the IPv6 address to that peer (See GetLocal and GetReachable, we sort by reachability). TBH it does not seem super obvious to me which address we choose for advertising so i might be wrong.

The description of -onlynet states "Inbound and manual connections are not affected by this option." but not advertising an address does affect inbound connections (you won't get any or only a few if you previously advertised your address).

I think my main problem was the display issue and adding a "listening but not advertised addresses" section seems OK to me.

Would we though? afaict we try to announce the address that is best reachable for our peer.

We would at least for any existing inbound peers. So there could be the effect that if your address that is known to the network and you use -onlynet with another network, you'd keep on advertising it to inbound peers - if you are not known and don't get inbound connections, that won't change.

Current behavior is that once you switch to -onlynet, you won't advertise other addresses at all, so inbound connections could become more infrequent over time (although GETDATA relay of the existing address by others is not affected).

I'm afraid I don't remember 1653f97 or its rationale at all. It appears to even have committed directly to the master branch without going through a pull request...

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

Closing this because, based on the feedback here, it looks like my approach isn't the right one. Addressing the -netinfo display issue (e.g. by adding a "listening but not advertised addresses" section) seems like a better idea.

Concept ACK

Thinking about this more, it looks now to me that it makes sense to remove IsReachable() from both AddLocal() and from IsPeerAddrLocalGood(). Why? Because there is an easy way to restrict where bitcoind is listening using -bind= and thus if your bitcoind is listening on network X, then you want to accept incoming connections on network X (otherwise you would use -bind= to not listen on network X). Thus you also want to advertise your listening address for the network X (because listening on an address that nobody knows because it is not advertised makes little sense).

To answer myself:

I think the logic is this - if a network is not reachable then we cannot accept connections from it. "Reachable" in the broader English word meaning, not the stricter -onlynet meaning. For example, a host/OS may have support for both IPv4 and IPv6 but its ISP may only support IPv4. So, an operator would run -onlynet=ipv4 to avoid attempting connections to IPv6 hosts and from advertising an IPv6 address to which the rest of the world cannot connect.

In this case the operator, in addition to -onlynet=ipv4, should also use -bind=1.2.3.4:8333 to avoid listening on the IPv6 address of his machine.

I guess the initial semantic of -onlynet was to apply for both outgoing and incoming connections. Like "don't touch this network at all". But it never actually did that and morphed into "apply to outgoing only" over time. Maybe it should be renamed.

This PR (plus adjusting IsPeerAddrLocalGood()) would resolve:

1. externalip=...onion ignored when onion/prox not set #25669
2. Onlynet=ipv4 disables Tor advertisements #25336
3. the display issue described in the OP of this PR

A duplicate of this PR:

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