init: deduplicate added connections #27804

Fixes #5299

Previously connections may be added to both m_added_nodes (via -addnode) and m_specified_outgoing (via -connect) if duplicated in config.

During init this can see duplicate connections made to the same outbound node as ThreadOpenConnections and ThreadOpenAddedConnections will both try to make the same connection at the same time.

Deduplicate the two vectors as a basic sanity check before starting connection manager.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

There are ways to bypass this simple de-duplication(e.g. using different hostnames for the same host) as it relies on string comparison, but it gets us most of the way.

Looking for approach ACKs.

I did consider more robust ways of testing for duplicates, but of course we already do this before making a new outbound and that doesn't help the startup race condition this (mostly) resolves.

Another option would be to de-duplicate after resolving hostnames, but that seems a bit overkill to me?

<details> <summary>Before and after connection summaries</summary> .

Before, both nodes have 3 connections. Node 2 exhibits the race and makes two outbound connections to Node 1, one via addnode and one via connect.

# Node 1
/home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/node1-datadir getpeerinfo | jq '[.[] | {id: .id, addr: .addr, type: .connection_type}]'
[
  {
    "id": 0,
    "addr": "127.0.0.1:60216",
    "type": "inbound"
  },
  {
    "id": 1,
    "addr": "127.0.0.1:60218",
    "type": "inbound"
  },
  {
    "id": 2,
    "addr": "127.0.0.1:18400",
    "type": "manual"
  }
]

# Node 2
/home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/node2-datadir -rpcport=18300 getpeerinfo | jq '[.[] | {id: .id, addr: .addr, type: .connection_type}]'
[
  {
    "id": 0,
    "addr": "127.0.0.1:18445",
    "type": "manual"
  },
  {
    "id": 1,
    "addr": "127.0.0.1:18445",
    "type": "manual"
  },
  {
    "id": 2,
    "addr": "127.0.0.1:38596",
    "type": "inbound"
  }
]

After, nodes have the expected two connections.

# Node 1
/home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/node1-datadir getpeerinfo | jq '[.[] | {id: .id, addr: .addr, type: .connection_type}]'
[
  {
    "id": 0,
    "addr": "127.0.0.1:59196",
    "type": "inbound"
  },
  {
    "id": 1,
    "addr": "127.0.0.1:18400",
    "type": "manual"
  }
]


# Node 2
/home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/node2-datadir -rpcport=18300 getpeerinfo | jq '[.[] | {id: .id, addr: .addr, type: .connection_type}]'
[
  {
    "id": 0,
    "addr": "127.0.0.1:18445",
    "type": "manual"
  },
  {
    "id": 1,
    "addr": "127.0.0.1:51594",
    "type": "inbound"
  }
]

</details>

Weak approach NACK. This feels like a hacky workaround for what is ultimately a race condition. Rather we just address the cause of the issue, instead of introducing weird variables (eg, what happens if the -connection is lost, and the user use the addnode RPC right at the correct moment?).

Thanks Luke, I mostly agree (that this is a bit hacky). Implemented here currently is the "simplest" (could also read "dumbest") fix to the issue, whilst trying to avoid any using more mutexes and possibly introducing new thread safety issues etc.

As I see it the root cause is that we use two threads to make outbound connections simultaneously with no way of deduplicating except after the connection is successful and the node is pushed onto m_nodes. This allows the race on node connections provided to the two startup options.

One alternative could be to introduce a new mutex-guarded datastructure on CConnman to store nodes while they are being tried in OpenNetworkConnection() (called by both ThreadOpenConnections() and ThreadOpenAddedConnections()). They can then be removed when the function exits, whether successful or not.

I implemented the latter using a new RAII helper class in this branch: https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:dedup-added-conns-alt and would be curious if you think this approach is better? It's not totally airtight as I am matching on either CAddress or pszDest but as the the two types are not mixed between ThreadOpenConnections() adding -connect nodes and ThreadOpenAddedConnections() adding -addnode nodes (both use c strings with an empty CAddress object), so matching will work against the startup options at least.

I think this method would also protect against a theoretical RPC race that you mention, although I find the idea of that occurring quite unlikely.

Personally I like the simplicity of the arg list de-duplication, but open to being persuaded in other directions...

this can see duplicate connections made to the same outbound node as ThreadOpenConnections and ThreadOpenAddedConnections will both try to make the same connection at the same time.

This is resolved, in favor of the latter thread, by the commit p2p, bugfix: do not make automatic outbound connections to addnode peers in #28248.

Great thanks @jonatack , I'll close this one then.