Multiple connections to same i2p address #26537

<!-- This issue tracker is only for technical issues related to Bitcoin Core. General bitcoin questions and/or support requests are best directed to the Bitcoin StackExchange at https://bitcoin.stackexchange.com. For reporting security issues, please read instructions at https://bitcoincore.org/en/contact/. If the node is "stuck" during sync or giving "block checksum mismatch" errors, please ensure your hardware is stable by running memtest and observe CPU temperature with a load-test tool such as linpack before creating an issue! -->

<!-- Describe the issue -->

This behavior is okay for ipv4 based on https://bitcoin.stackexchange.com/questions/87739/can-a-bitcoin-node-create-an-outgoing-connection-to-a-inbound-node

However, making multiple connections to same i2p peer makes no sense IMO

Expected behavior

No connections (outbound/inbound) should be initiated with i2p address that already exists as one of the peer.

Actual behavior

To reproduce

• Setup 3 nodes such that node2 and node3 works with i2p
• Connect node2 manually with addnode in config to node1
• Connect node3 manually with node2 using i2p address
• Observe result for getpeerinfo in a few minutes or check peers list in bitcoin-qt

System information

Bitcoin Core Master Branch, v24.0rc2 and v24.0rc4

There is already a check to avoid connecting to an already connected address. See:

#22559 (comment) and https://bitcoin.stackexchange.com/a/116097/129003

What you are observing is probably due to the race in that check described above. For the second screenshot I guess what happened is that while you initiated the manual connection, after the connection attempt started, but before it completed an automatic outbound was initiated to the same address and later that address connected to us for the inbound :disappointed:. This is not I2P specific, but more likely to be seen with I2P because establishing a connection takes longer.

What I think can be improved:

• After accepting an incoming connection, drop it if we already have a connection to the same address (ignore the port).
• Fix the race by maintaining a list of "attempting to connect to" addresses and do not initiate more connections to them.
• Or maybe, periodically, e.g. every 5 minutes, check for duplicates in CConnman::m_nodes and drop the redundant ones, maybe ignoring the port.

Preventing incoming connections from an address we already have a connection to could block legit cases where multiple distinct nodes run behind a common address. At first I thought that this is ok, since the disconnected node will find another random peer to connect to. But then, having multiple connections with the same address, even if it is the same node, is a minor issue too. Maybe not worth the effort to change that. Or do only for I2P.

For the second screenshot I guess what happened is that while you initiated the manual connection, after the connection attempt started, but before it completed an automatic outbound was initiated to the same address and later that address connected to us for the inbound 😞

I will try to reproduce this with deubg=net although I agree something similar happened as you described.

Fix the race by maintaining a list of "attempting to connect to" addresses and do not initiate more connections to them.

This makes sense.

For the second screenshot I guess what happened is that while you initiated the manual connection, after the connection attempt started, but before it completed an automatic outbound was initiated to the same address and later that address connected to us for the inbound 😞

I will try to reproduce this with deubg=net although I agree something similar happened as you described.

Issue is intermittent and I couldn't collect any useful logs.