Make AddrMan support multiple ports per IP

[sipa]

For a long part of Bitcoin's history, this codebase has aggressively avoided making automatic connections to anything but nodes running on port 8333. I'd like to propose changing that, and this is a first PR necessary for that.

The folklore justification (eventually actually added as a comment to the codebase in #20668) is that this is to prevent the Bitcoin P2P network from being leveraged to perform a DoS attack on other services, if their IP/port would get rumoured. It appears, at least the current network scale - and probably significantly larger - that the impact is very low at best (see calculations by vasild in #5150 (comment) e.g.). Another possible justification would be a risk that treating different IP:port combinations separately would help perform Eclipse attacks (by an attacker rumouring their own IP with many ports). This concern is (a) no different than what is possible with IPv6 (where large ranges of IP addresses are very cheaply available), and (b) already hopefully sufficiently addressed by addrman's design (which limits access through based selected based on network groups).

And this policy has downsides too; in particular, a fixed port is easy to detect, and a very obvious sign a Bitcoin node is running there.

One obstacle in moving away from a default port that is the fact that addrman is currently restricted to a single entry per IP address. If ports are no longer expected to be generally always the default one, we need to deal with the case where conflicting information is relayed. It turns out there is a very natural solution to this: treat (IP,port) combination exactly as we're treating IPs now; this automatically means that the same IP may appear with multiple ports, simply because those would be distinct entries. Given that indexing into addrman's bucket already uses the port number, the only change required is making all addrman lookup be (IP,port) (aka CService) based, rather than IP (aka CNetAddr) based.

This PR doesn't include any change to the actual outbound connection preference logic, as perhaps that's something that we want to phase in more gradually.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #23077 (Full CJDNS support by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[ghost]

For a long part of Bitcoin's history, this codebase has aggressively avoided making automatic connections to anything but nodes running on port 8333. I'd like to propose changing that, and this is a first PR necessary for that.

Concept ACK for allowing automatic connections to nodes which are not using default ports

This concern is (a) no different than what is possible with IPv6 (where large ranges of IP addresses are very cheaply available), and (b) already hopefully sufficiently addressed by addrman's design (which limits access through based selected based on network groups).

Agree

And this policy has downsides too; in particular, a fixed port is easy to detect, and a very obvious sign a Bitcoin node is running there.

I found UA string as the easiest way to look for Bitcoin nodes while using shodan in which I could also see few nodes using non-default ports:

    "addr": "193.32.127.161:58477",
    "addrbind": "192.168.1.4:60877",
    "network": "ipv4",

Ignore if its not related: Few users use Tor hidden services and different ports for RPC as explained in Learning-Bitcoin-from-the-Command-Line/Verifying_Your_Tor_Setup.md

[dhruv]

Concept ACK

For a long part of Bitcoin's history, this codebase has aggressively avoided making automatic connections to anything but nodes running on port 8333. I'd like to propose changing that, and this is a first PR necessary for that.

Port 8333 is the cheapest fingerprint for a Bitcoin node. The possibility of port randomization is quite exciting as we will be able to use the raised floor of fingerprinting cost from BIP324.

[jamesob]

Concept ACK. Changes look pretty straightforward.

[lsilva01]

Tested ACK 5697d60 on Ubuntu 20.04.

CNetAddr has been replaced by CService in mapAddr in this change.
CService is a subclass of CNetAddr that has the port attribute.
Started node with an existing peers.dat file and with no file. It had no impact on serialization / unserialization as this is done using AddrInfo.

[practicalswift]

The folklore justification (eventually actually added as a comment to the codebase in #20668) is that this is to prevent the Bitcoin P2P network from being leveraged to perform a DoS attack on other services, if their IP/port would get rumoured. […]  Another possible justification would be a risk that treating different IP:port combinations separately would help perform Eclipse attacks (by an attacker rumouring their own IP with many ports).

A third concern (which I think can be addressed: see below) could be that Bitcoin Core users would risk having to deal with bogus complaints along the lines of "why am I seeing a failed SSH-login to my server on port 22 from your IP-address w.x.y.z?" if we allowed for automatic outgoing connections on all ports by default.

When opening up for additional port numbers we might want to look at how browsers are tackling "bad ports":

• kRestrictedPorts in Chromium
• gBadPortList in Firefox
• Definition of "bad port" in whatwg's Fetch standard

Avoiding "bad ports" (or a subset of these ports) by default is probably a cheap way to reduce the likelihood of Bitcoin Core users having to deal with these types of bogus complaints.

An alternative route would be to explicitly whitelist (by default) a set of "good ports" which a.) have low likelihood of causing said type of complaints, and b.) are unlikely to be firewalled.

[vasild]

ACK 5697d60

TODO: once this PR and #22839 are merged, ensure that all logging of addresses from within addrman include the port (i.e. avoid ToStringIP()).

[naumenkogs]

Concept ACK.
Interested in addressing the point by practicalswift (either dismissing or accepting, but with rationale).

Otherwise, I agree this change is beneficial and I don't see any real harm.

[mzumsande]

Concept ACK

In four different places in Addrman, there is the pattern

    AddrInfo* pinfo = Find(addr);

    // if not found, bail out
    if (!pinfo)
        return;

    AddrInfo& info = *pinfo;

    // check whether we are talking about the exact same CService (including same port)
    if (info != addr)
        return;

I think the final check for info != addr could be removed now, since with this PR, Find() only returns an entry if there is a match including the port (and the first return would be hit if not).

[sipa]

@practicalswift That's a reasonable concern. I think as a first step, the policy could e.g. still maintain the deprioritization of connections to ports numbered < 1024.

[vasild]

I like the idea of "bad ports", especially if popular softwares like Firefox and Chromium agree on what is a "bad port". Trying to open http://bitcoin.org:22/ in Firefox gives me "This address is restricted" error.

This discussion belongs to the next PR, which removes the 8333 preference:

bitcoin/src/net.cpp

Line 2070 in 9469ffc

if (addr.GetPort() != Params().GetDefaultPort(addr.GetNetwork()) && nTries < 50) {

Maybe when removing that, in the same PR, we should introduce the concept for "bad ports". I wonder if it would be as easy as adding CService::IsValid() which calls the parent's CNetAddr::IsValid() and does an extra check like:

switch (port) {
case 1: return false; // tcpmux
case 7: return false; // echo
case 9: return false; // discard
...
}
return true;

[jnewbery]

Concept ACK. Couple of minor comments inline.

I agree with @mzumsande about removing the // check whether we are talking about the exact same CService (including same port) logic from _Good(), _Attempt(), _Connected() and _SetServices().

[laanwj]

Concept ACK, I think this makes sense. More flexibility with regard to ports is better, allows people to accept incoming connections on their node even if they can't bind to specific port 8333 (e.g. on a shared router). Also, it allows to route around the dumbest kind of ISP firewall blocking.

@practicalswift That's a reasonable concern. I think as a first step, the policy could e.g. still maintain the deprioritization of connections to ports numbered < 1024.

I like this idea. Labeling specific "bad ports" could work too, but I don't see any problems with de-prioritizing the entire <1024 range, after all, to use them one would have to run bitcoind as root on many operating systems, which is inadvisable.

[practicalswift]

@sipa

A third concern (which I think can be addressed: see below) could be that Bitcoin Core users would risk having to deal with bogus complaints along the lines of "why am I seeing a failed SSH-login to my server on port 22 from your IP-address w.x.y.z?" if we allowed for automatic outgoing connections on all ports by default.

@practicalswift That's a reasonable concern. I think as a first step, the policy could e.g. still maintain the deprioritization of connections to ports numbered < 1024.

Sounds resonable. That should resolve this concern. Perhaps port 80 and 443 could be allowed too (in addition to >= 1024) in order to accommodate users behind very strict firewalls.

FWIW, IETF Network Working Group's "Implications of running Internet over ports 80 and 443": "Users are often connected to Internet with very few outgoing ports available, such as only port 80 and 443 over TCP. This situation has many implications on designing, deploying and using IETF protocols, such as encaspulating protocols within HTTP, difficulty to do traffic engineering, quality of service, peer-to-peer, multi-channel protocols or deploying new transport protocols. This document describes the situation and its implications."

From the same paper:

• "A trend started many years ago has been to provide Internet access to end-users with limited outgoing ports. The most constraint but common case is to only have outgoing TCP port 80 and port 443 opened."
• "A consequence of this trend is that Internet statistics show [Labovitz] that a majority of the Internet traffic is over port 80 and 443. And the concentration on these ports are further increasing every year."

[vasild]

There are good ports <1024 (e.g. 80, 443) and bad ports >1024 (e.g. 6000). I think that if we are going to do if (port < 1024 && port != 80 && port != 443) (poor man bad ports approximation) we might as well do the proper if (port == 1 || port == 7 || ...) (or the switch from above).

[jnewbery]

Code review ACK 92617b7

[sipa]

There is plenty to discuss still on exactly how the relax the peer port preference policy practically, which is probably best left for an actual PR that implements that.

[naumenkogs]

ACK 92617b7

[vasild]

ACK 92617b7

[ajtowns]

ACK 92617b7

If I'm reading it right, it seems like ~5% of addresses in my addrman use non-default ports (and none of them seem to be bad ports like ssh or ircd), so moving towards more egalitarian treatment of those nodes seems like a good thing to me. I agree we should have some check for bad ports as part of the next PR that moves towards enabling that.

[sipa]

Mental note: investigate what happens when downgrading, and you have a peers.dat with multiple entries for the same IP?

[maflcko]

Looks like a downgrade will fail with:

Error: Invalid or corrupt peers.dat (Corrupt data. Consistency check failed with code -5: iostream error). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/tmp/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.

[vasild]

A followup to this, that removes the strong preference for 8333 is in #23542.