p2p: peer connection bug fixes #28248

Due to their potentially different port numbers, inbound peers can be overlooked in CConnman::GetAddedNodeInfo() when comparing CServices, i.e. both addr and port. Fix this by comparing CNetAddr instances, i.e. addr only, for inbound peers. Co-authored-by: Jon Atack <jon@atack.com>

Addnode (manual) peers connected to us via the cjdns network are currently not detected by CConnman::GetAddedNodeInfo(), i.e. fConnected is always false. This causes the following issues: - RPC `getaddednodeinfo` incorrectly shows them as not connected - CConnman::ThreadOpenAddedConnections() continually retries to connect them

Currently ThreadOpenConnections() does not detect inbound cjdns peers that we are already connected to when it calls AlreadyConnectedToAddress(). Fix this by calling MaybeFlipIPv6toCJDNS before the search.

Dereferencing the pointer result after FindNode* has released m_nodes_mutex is unsafe, so we currently have to add unneeded recursive locks before calling FindNode*. Fix this by acquiring the mutex before calling FindNode* rather than inside them. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

…dress() using the following logic: If we are about to open an outbound connection - for all outbound connections, compare the peer's address and port - for all inbound connections and provided we're not making a manual connection, compare only the address and ignore the port If we are about to accept an inbound connection - for all connections, both inbound and outbound, compare only the address and ignore the port Fix the logging to be actually useful by stating which peer is already connected, and further improve it to provide full details for debugging purposes. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

by upgrading the checks from FindNode() to AlreadyConnectedToAddress().

as creating I2P tunnels is expensive. These duplicate connection requests come most frequently from inbound peers. The other fixes in this PR may mitigate this behavior by our peers, but not for nodes running earlier versions of Bitcoin Core. Extend a related functional test.

instead of a std::string (and rename its argument and that of CConnman::AddNode() to "params"). Passing AddedNodeParams makes the two member functions AddNode and RemoveAddedNode consistent, and is needed in order to refactor `m_added_node_params` from a vector to an unordered set in the next commit.

which simplifies and speeds up the AddNode() and RemoveAddedNode() methods, as well as the next commit, "p2p: do not make automatic outbound connections to addnode peers", which checks if an address is already in m_added_node_params. This refactoring is covered by functional tests in rpc_net.py. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

to allocate our limited outbound slots correctly, and to ensure addnode connections benefit from their intended protections. Our addnode logic usually connects the addnode peers before the automatic outbound logic does, but not always, as a connection race can occur. For instance, our internet connection or router (or the addnode peer) could be temporarily offline, and then return online during the automatic outbound thread. Or we could add a new manual peer using the `addnode` RPC at that time. The race can be more apparent when our node doesn't know many peers, or with networks like cjdns that currently have few bitcoin peers. Examples on mainnet using logging added in the same pull request: 2023-08-12T14:51:05.681743Z [opencon] [net.cpp:1949] [ThreadOpenConnections] [net:debug] Not making automatic network-specific outbound-full-relay connection to i2p peer selected for manual (addnode) connection: [geh...odq.b32.i2p]:0 2023-08-13T03:59:28.050853Z [opencon] [net.cpp:1949] [ThreadOpenConnections] [net:debug] Not making automatic block-relay-only connection to onion peer selected for manual (addnode) connection: kpg...aid.onion:8333 2023-08-13T16:21:26.979052Z [opencon] [net.cpp:1949] [ThreadOpenConnections] [net:debug] Not making automatic network-specific outbound-full-relay connection to cjdns peer selected for manual (addnode) connection: [fcc...8ce]:8333 2023-08-14T20:43:53.401271Z [opencon] [net.cpp:1949] [ThreadOpenConnections] [net:debug] Not making automatic network-specific outbound-full-relay connection to cjdns peer selected for manual (addnode) connection: [fc7...59e]:8333 2023-08-15T00:10:01.894147Z [opencon] [net.cpp:1949] [ThreadOpenConnections] [net:debug] Not making automatic feeler connection to i2p peer selected for manual (addnode) connection: geh...odq.b32.i2p:8333 When an addnode peer is connected as an automatic outbound peer and is the only connection we have to a network, it can be protected by our new outbound eviction logic and persist in the "wrong role". Finally, there does not seem to be a reason to make block-relay or short-lived feeler connections to addnode peers, as the addnode logic will ensure we connect to them if they are up, within the addnode connection limit. Fix these issues by checking if the address is an addnode peer in our automatic outbound connection logic. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

By moving the "Trying connection" log entry in ConnectNode to after the address resolution, we can: - not needlessly log the attempt until after checking if we are already connected to it - know the network the peer connected through, i.e. for cjdns peers - add useful peer info to the log entry for debugging purposes This change makes the feeler log entry in CConnman::ThreadOpenConnections() redundant... 2023-09-06T01:11:42.614464Z Making feeler connection to dft...dyd.onion:8333 2023-09-06T01:11:42.614722Z Trying feeler connection to net=onion, peeraddr=dft...dyd.onion:8333, lastseen=4.2hrs ...so remove it.

This logging has been helpful for observing and debugging our peer connection and eviction behavior. Also improve related log entries for - eviction of an inbound peer - protecting a peer from eviction for a bad or slow chain

…optional and make it a const class method, and add Clang thread-safety analysis annotation and related assertions.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p2p: peer connection bug fixes #28248

p2p: peer connection bug fixes #28248

Commits on Oct 24, 2023