net: remove fragile unwrap() from move_host()

Previously, we called unwrap() inside of move_host, which would panic if
the host state is invalid (for example, if we try to move a host that is
currently moving).

This is fragile in async conditions such as the following:

* We are doing a version exchange with a peer called Bob
* Version exchange passes and we move the host to Goldlist
* Bob disconnects while this is ongoing
* We try to move to Greylist -> panic!

Now we simply return an error on the second move, so what should happen
instead is:

* Version exchange passes and we move the host to Goldlist
* Bob disconnects
* We try to move to Greylist -> the move fails
* Bob is still on our Goldlist

This means that Bob's node might be on the goldlist even though it's not
actually online. This will be cleaned up as soon as outbound session
fails to establish a connection to it.

src/net/channel.rs

@@ -455,7 +455,14 @@ impl Channel {
 
         let last_seen = UNIX_EPOCH.elapsed().unwrap().as_secs();
         info!(target: "net::channel::ban()", "Blacklisting peer={}", peer);
-        self.p2p().hosts().move_host(&peer, last_seen, HostColor::Black).unwrap();
+        match self.p2p().hosts().move_host(&peer, last_seen, HostColor::Black) {
+            Ok(()) => {
+                info!(target: "net::channel::ban()", "Peer={} blacklisted successfully", peer);
+            }
+            Err(e) => {
+                warn!(target: "net::channel::ban()", "Could not blacklisted peer={}, err={}", peer, e);
+            }
+        }
         self.stop().await;
         debug!(target: "net::channel::ban()", "STOP {:?}", self);
     }

src/net/hosts.rs

@@ -1404,7 +1404,7 @@ impl Hosts {
         Ok(())
     }
 
-    /// A single atomic function for moving hosts between hostlists. Called on the following occasions:
+    /// A single function for moving hosts between hostlists. Called on the following occasions:
     ///
     /// * When we cannot connect to a peer: move to grey, remove from white and gold.
     /// * When a peer disconnects from us: move to grey, remove from white and gold.
@@ -1426,58 +1426,70 @@ impl Hosts {
         debug!(target: "net::hosts::move_host()", "Trying to move addr={} destination={:?}",
                addr, destination);
 
-        // This should never panic. Failure indicates a misuse of the HostState API.
-        self.try_register(addr.clone(), HostState::Move).unwrap();
+        match self.try_register(addr.clone(), HostState::Move) {
+            Ok(new_state) => {
+                debug!(target: "net::hosts::move_host()", "Moving addr={} destination={:?}, state={:?}",
+                       addr, destination, new_state);
 
-        match destination {
-            // Downgrade to grey. Remove from white and gold.
-            HostColor::Grey => {
-                self.container.remove_if_exists(HostColor::Gold, addr);
-                self.container.remove_if_exists(HostColor::White, addr);
+                match destination {
+                    // Downgrade to grey. Remove from white and gold.
+                    HostColor::Grey => {
+                        self.container.remove_if_exists(HostColor::Gold, addr);
+                        self.container.remove_if_exists(HostColor::White, addr);
 
-                self.container.store_or_update(HostColor::Grey, addr.clone(), last_seen);
-                self.container.sort_by_last_seen(HostColor::Grey as usize);
-                self.container.resize(HostColor::Grey);
-            }
+                        self.container.store_or_update(HostColor::Grey, addr.clone(), last_seen);
+                        self.container.sort_by_last_seen(HostColor::Grey as usize);
+                        self.container.resize(HostColor::Grey);
+                    }
 
-            // Remove from Greylist, add to Whitelist. Called by the Refinery.
-            HostColor::White => {
-                self.container.remove_if_exists(HostColor::Grey, addr);
+                    // Remove from Greylist, add to Whitelist. Called by the Refinery.
+                    HostColor::White => {
+                        self.container.remove_if_exists(HostColor::Grey, addr);
 
-                self.container.store_or_update(HostColor::White, addr.clone(), last_seen);
-                self.container.sort_by_last_seen(HostColor::White as usize);
-                self.container.resize(HostColor::White);
-            }
+                        self.container.store_or_update(HostColor::White, addr.clone(), last_seen);
+                        self.container.sort_by_last_seen(HostColor::White as usize);
+                        self.container.resize(HostColor::White);
+                    }
 
-            // Upgrade to gold. Remove from white or grey.
-            HostColor::Gold => {
-                self.container.remove_if_exists(HostColor::Grey, addr);
-                self.container.remove_if_exists(HostColor::White, addr);
+                    // Upgrade to gold. Remove from white or grey.
+                    HostColor::Gold => {
+                        self.container.remove_if_exists(HostColor::Grey, addr);
+                        self.container.remove_if_exists(HostColor::White, addr);
 
-                self.container.store_or_update(HostColor::Gold, addr.clone(), last_seen);
-                self.container.sort_by_last_seen(HostColor::Gold as usize);
-            }
-
-            // Move to black. Remove from all other lists.
-            HostColor::Black => {
-                // We ignore UNIX sockets here so we will just work
-                // with stuff that has host_str().
-                if addr.host_str().is_some() {
-                    // Localhost connections should never enter the blacklist
-                    // This however allows any Tor and Nym connections.
-                    if self.is_local_host(addr) {
-                        return Ok(());
+                        self.container.store_or_update(HostColor::Gold, addr.clone(), last_seen);
+                        self.container.sort_by_last_seen(HostColor::Gold as usize);
                     }
 
-                    self.container.remove_if_exists(HostColor::Grey, addr);
-                    self.container.remove_if_exists(HostColor::White, addr);
-                    self.container.remove_if_exists(HostColor::Gold, addr);
+                    // Move to black. Remove from all other lists.
+                    HostColor::Black => {
+                        // We ignore UNIX sockets here so we will just work
+                        // with stuff that has host_str().
+                        if addr.host_str().is_some() {
+                            // Localhost connections should never enter the blacklist
+                            // This however allows any Tor and Nym connections.
+                            if self.is_local_host(addr) {
+                                return Ok(());
+                            }
+
+                            self.container.remove_if_exists(HostColor::Grey, addr);
+                            self.container.remove_if_exists(HostColor::White, addr);
+                            self.container.remove_if_exists(HostColor::Gold, addr);
+
+                            self.container.store_or_update(
+                                HostColor::Black,
+                                addr.clone(),
+                                last_seen,
+                            );
+                        }
+                    }
 
-                    self.container.store_or_update(HostColor::Black, addr.clone(), last_seen);
+                    HostColor::Dark => return Err(Error::InvalidHostColor),
                 }
             }
-
-            HostColor::Dark => return Err(Error::InvalidHostColor),
+            Err(e) => {
+                warn!(target: "net::hosts::move_host", "Cannot move host={:?}, err={:?}",
+                    addr.clone(), e);
+            }
         }
 
         Ok(())

src/net/session/mod.rs

@@ -22,7 +22,7 @@ use std::{
 };
 
 use async_trait::async_trait;
-use log::{debug, trace};
+use log::{debug, error, trace};
 use smol::Executor;
 
 use super::{channel::ChannelPtr, hosts::HostColor, p2p::P2pPtr, protocol::ProtocolVersion};
@@ -79,7 +79,10 @@ pub async fn remove_sub_on_stop(
         );
 
         let last_seen = hosts.fetch_last_seen(addr).unwrap();
-        hosts.move_host(addr, last_seen, HostColor::Grey).unwrap();
+        if let Err(e) = hosts.move_host(addr, last_seen, HostColor::Grey) {
+            error!(target: "net::session::remove_sub_on_stop()",
+            "Failed to move host {} to Greylist! Err={}", addr.clone(), e);
+        }
     }
 
     // For all sessions that are not refine sessions, mark this addr as
@@ -189,10 +192,8 @@ pub trait Session: Sync {
                     );
 
                     let last_seen = UNIX_EPOCH.elapsed().unwrap().as_secs();
-                    self.p2p()
-                        .hosts()
-                        .move_host(channel.address(), last_seen, HostColor::Gold)
-                        .unwrap();
+
+                    self.p2p().hosts().move_host(channel.address(), last_seen, HostColor::Gold)?;
                 }
 
                 // Attempt to add channel to registry