Revert "Do not consider blocked networks local"

dergoegge commented at 3:50 pm on April 12, 2022: member

This reverts commit 1653f97c8f3c37cd96e03cf397c31c5caf81af08.

The output of bitcoin-cli -netinfo does not show local addresses for networks that the node itself would not make connections to, however the node might still receive incoming connections on addresses not shown by -netinfo. I would have expected -netinfo to list all local addresses that can receive incoming connections.

This can be observed when running a node that listens on multiple networks but also uses -onlynet. The local addresses of the networks not specified by -onlynet will be excluded from the output of -netinfo.

This reverts commit 1653f97c8f3c37cd96e03cf397c31c5caf81af08.

51a2043e36

fanquake added the label P2P on Apr 12, 2022

laanwj commented at 2:45 pm on April 13, 2022: member

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

dergoegge commented at 9:12 am on April 14, 2022: member

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

fanquake commented at 10:14 am on April 20, 2022: member

@mzumsande @jnewbery @sipa any opinions here?

mzumsande commented at 11:36 pm on April 24, 2022: contributor

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 https://github.com/bitcoin/bitcoin/commit/1653f97c8f3c37cd96e03cf397c31c5caf81af08 - 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.

MarcoFalke commented at 7:03 am on April 25, 2022: member

It would be good to have more info on the rationale of https://github.com/bitcoin/bitcoin/commit/1653f97c8f3c37cd96e03cf397c31c5caf81af08 - was there a PR for this that I’m unable to find?

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

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

vasild commented at 12:45 pm on April 25, 2022: contributor

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:

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?)

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.

dergoegge commented at 9:06 am on April 26, 2022: member

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.

mzumsande commented at 12:59 pm on April 27, 2022: contributor

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).

sipa commented at 1:28 pm on April 27, 2022: member

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

DrahtBot commented at 8:52 pm on June 2, 2022: contributor

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

Conflicts

No conflicts as of last run.

dergoegge commented at 12:18 pm on June 29, 2022: member

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.

dergoegge closed this on Jun 29, 2022

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

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:

#25669
#25336
the display issue described in the OP of this PR

A duplicate of this PR:

https://github.com/bitcoin/bitcoin/pull/25690

bitcoin locked this on Jul 28, 2023

Revert “Do not consider blocked networks local” #24835

Conflicts