[Refactor] Tighten up high-level connection/peer handling

[ljedrz]

We currently have several high-level connection/peer-related collections, which provides us with a good overview when reading the objects, but isn't without potential issues: it involves multiple lock operations whenever a peer is added or removed, which can easily lead to race conditions.

This PR is an attempt to get rid of these issues altogether by collapsing several collections of simple items in favor of a single collection of more complex ones. The basic summary of the changes is this:

# InnerRouter<N: Network> {
#    (...)
-    /// The map of connected peer IPs to their peer handlers.
-    connected_peers: RwLock<HashMap<SocketAddr, Peer<N>>>,
-    /// The set of handshaking peers. While `Tcp` already recognizes the connecting IP addresses
-    /// and prevents duplicate outbound connection attempts to the same IP address, it is unable to
-    /// prevent simultaneous "two-way" connections between two peers (i.e. both nodes simultaneously
-    /// attempt to connect to each other). This set is used to prevent this from happening.
-    connecting_peers: Mutex<HashMap<SocketAddr, Option<Peer<N>>>>,
-    /// The set of candidate peer IPs.
-    candidate_peers: RwLock<HashSet<SocketAddr>>,
-    /// The set of restricted peer IPs.
-    restricted_peers: RwLock<HashMap<SocketAddr, Instant>>,
+    /// The collection of both candidate and connected peers.
+    peer_pool: RwLock<HashMap<SocketAddr, Peer<N>>>,

A short summary of the other changes involved:

• the Peer becomes an enum with 3 possible variants: Candidate, Connecting, and Connected
• the Peer is no longer Clone (to avoid any possibility of unintentional duplication)
• new concrete types, CandidatePeer and ConnectedPeer, are isolated for added type safety
• the completion of the high-level network handshake and the high-level disconnect now only involve accessing a single collection
• the Resolver is only ever modified indirectly, through the Peer
• the Resolver no longer needs to map from the listener address to the connected address (the peer_pool can do it)

I tried to limit the scope of the logic changes to minimum, and confirmed that they are working by running all the tests locally, plus the devnet script.

Fixes #3705.

[vicsn]

Triggered prerelease tests against this.

EDIT: Nevermind, still adding more clients to prerelease tests so it more accurately reflects the load.

[vicsn]

Connections on a 40 validator 80 client network look healthy. The visible drops to 0 connections are due to expected restarts.

Can you evaluate if the logs look good to you too @ljedrz ?
https://drive.google.com/file/d/1WZcRemR1vUWhWkeLEsdsl-Pp3c_HK1tl/view?usp=sharing

[kaimast]

These changes are exciting as they improve the code quality and maintainability a lot.
I left a few comments, and we can hopefully get this merged soon :)

[ljedrz]

@vicsn The logs look ok to me 👍.

[ljedrz]

The CI failure is unrelated:

    Scanning Cargo.lock for vulnerabilities (603 crate dependencies)
Crate:     curve25519-dalek
Version:   4.2.0
Warning:   yanked
Dependency tree:
curve25519-dalek 4.2.0
└── ed25519-dalek 2.2.0
    └── zipsign-api 0.1.5
        └── self_update 0.42.0
            └── snarkos-cli 3.8.0
                └── snarkos 4.0.0

error: 1 denied warning found!

[kaimast]

I had the audit issue too. cargo update should downgrade that package and fix it.

[kaimast]

This should be good to merge now 🥳