Rationale
Currently, addnode has a couple of corner cases that allow it to either connect to the same peer more than once, hence wasting outbound connection slots, or add redundant information to m_added_nodes, hence making Bitcoin iterate through useless data on a regular basis.
Connecting to the same node more than once
In general, connecting to the same node more than once is something we should try to prevent. Currently, this is possible via addnode in two different ways:
- Calling
addnodemore than once in a short time period, using two equivalent but distinct addresses - Calling
addnode addusing an IP, andaddnode onetryafter with an address that resolved to the same IP
For the former, the issue boils down to CConnman::ThreadOpenAddedConnections calling CConnman::GetAddedNodeInfo once, and iterating over the result to open connections (CConman::OpenNetworkConnection) on the same loop for all addresses.CConnman::ConnectNode only checks a single address, at random, when resolving from a hostname, and uses it to check whether we are already connected to it.
An example to test this would be calling:
0bitcoin-cli addnode "127.0.0.1:port" add
1bitcoin-cli addnode "localhost:port" add
And check how it allows us to perform both connections some times, and some times it fails.
The latter boils down to the same issue, but takes advantage of onetry bypassing the CConnman::ThreadOpenAddedConnections logic and calling CConnman::OpenNetworkConnection straightaway. A way to test this would be:
0bitcoin-cli addnode "127.0.0.1:port" add
1bitcoin-cli addnode "localhost:port" onetry
Adding the same peer with two different, yet equivalent, addresses
The current implementation of addnode is pretty naive when checking what data is added to m_added_nodes. Given the collection stores strings, the checks at CConnman::AddNode() basically check wether the exact provided string is already in the collection. If so, the data is rejected, otherwise, it is accepted. However, ips can be formatted in several ways that would bypass those checks.
Two examples would be 127.0.0.1 being equal to 127.1 and [::1] being equal to [0:0:0:0:0:0:0:1]. Adding any pair of these will be allowed by the rpc command, and both will be reported as connected by getaddednodeinfo, given they map to the same CService.
This is less severe than the previous issue, since even tough both nodes are reported as connected by getaddednodeinfo, there is only a single connection to them (as properly reported by getpeerinfo). However, this adds redundant data to m_added_nodes, which is undesirable.
Parametrize CConnman::GetAddedNodeInfo
Finally, this PR also parametrizes CConnman::GetAddedNodeInfo so it returns either all added nodes info, or only info about the nodes we are not connected to. This method is used both for rpc, in getaddednodeinfo, in which we are reporting all data to the user, so the former applies, and to check what nodes we are not connected to, in CConnman::ThreadOpenAddedConnections, in which we are currently returning more data than needed and then actively filtering using CService.fConnected()
