net: cleanup and add documentation

src/net/hosts/mod.rs

@@ -32,15 +32,25 @@
 /// and `ProtocolAddress`.
 pub mod refinery;
 
-/// TODO: update documentation
-/// The main interface for interacting with the hostlist.
+/// The main interface for interacting with the hostlist. Contains the following:
 ///
-/// The hostlist is stored in three sections: white, grey, and anchorlists.
-/// The _whitelist_ contains hosts that have been seen recently.
-/// The _anchorlist_ contains hosts that we have been able to establish a connection to.
-/// The _greylist_ is an intermediary host list of recently received hosts that is
-/// periodically refreshed using the greylist refinery.
+/// `Hosts`: the main parent class that manages HostRegistry and HostContainer. It is also
+///  responsible for filtering addresses before writing to the hostlist.
 ///
-/// `store` contains various methods for reading from, quering and writing to the hostlists.
-/// It is also responsible for filtering addresses and ensuring channel transport validity.
+/// `HostRegistry`: A locked HashMap that maps peer addresses onto mutually exclusive
+///  states (`HostState`). Prevents race conditions by dictating a strict flow of logically
+///  acceptable states.
+///
+/// `HostContainer`: A wrapper for the hostlists. Each hostlist is represented by a `HostColor`,
+///  which can be Grey, White, Gold or Black. Exposes a common interface for hostlist queries and
+///  utilities.
+///
+/// `HostColor`: White hosts have been seen recently. Gold hosts we have been able to establish
+///  a connection to. Grey hosts are recently received hosts that are periodically refreshed
+///  using the greylist refinery. Black hosts are considerede hostile and are strictly avoided
+///  for the duration of the program.
+///
+/// `HostState`: a set of mutually exclusive states that can be Insert, Refine, Connect, Suspend
+///  or Connected. The state is `None` when the corresponding host has been removed from the
+///  HostRegistry.
 pub mod store;

src/net/hosts/store.rs

@@ -47,8 +47,8 @@ pub type HostsPtr = Arc<Hosts>;
 /// a given host.
 pub type HostRegistry = RwLock<HashMap<Url, HostState>>;
 
-/// HostState is a set of mutually exclusive states that can be Pending,
-/// Connected, Disconnected or Refining. The state is `None` when the
+/// HostState is a set of mutually exclusive states that can be Insert,
+/// Refine, Connect, Suspend or Connected. The state is `None` when the
 /// corresponding host has been removed from the HostRegistry.
 /// ```
 ///                                +--------+                       
@@ -72,18 +72,22 @@ pub type HostRegistry = RwLock<HashMap<Url, HostState>>;
 /// ```
 #[derive(Clone, Debug)]
 pub enum HostState {
-    /// TODO: doc
+    /// Hosts that are currently being inserting into the hostlist.
     Insert,
     /// Hosts that are migrating from the greylist to the whitelist or being
     /// removed from the greylist, as defined in `refinery.rs`.
     Refine,
     /// Hosts that are being connected to in Outbound and Manual Session.
     Connect,
-    /// TODO: documentation
+    /// Hosts that we have just failed to connect to. Marking a host
+    /// as Suspend effectively gives it a priority in the refinery,
+    /// since Suspend-> Refine is an accessible state transition.
+    // TODO: We will probably make Suspend a `Red list` instead of a HostState.
     Suspend,
     /// Hosts that have been successfully connected to.
     Connected(ChannelPtr),
-    /// TODO: doc
+    /// Host that are moving between hostlists, implemented in
+    /// store::move_host().
     Move,
 }
 
@@ -141,7 +145,7 @@ impl HostState {
         }
     }
 
-    // Try to change state to move. Only possible if this connection is Connect i.e. if we are
+    // Try to change state to Move. Only possible if this connection is Connect i.e. if we are
     // trying to connect to this host.
     fn try_move(&self) -> Result<Self> {
         match self {
@@ -154,7 +158,8 @@ impl HostState {
         }
     }
 
-    // TODO
+    // Try to change the state to Suspend. Only possible when we are currently moving this host,
+    // since we suspend a host after failing to connect to it and then downgrading in move_host.
     fn try_suspend(&self) -> Result<Self> {
         match self {
             HostState::Insert => Err(Error::StateBlocked(self.to_string())),
@@ -203,10 +208,6 @@ impl TryFrom<usize> for HostColor {
 /// A Container for managing Grey, White, Gold and Black
 /// hostlists. Exposes a common interface for writing to and querying
 /// hostlists.
-// TODO: Currently hosts (aside from hosts on the Black list) are on
-// multiple lists at once. This needs to be reconsidered.
-// Rethink upgrade/ move methods and consider a single method move() which
-// removes from one hostlist and places on another.
 // TODO: Verify the performance overhead of using vectors for hostlists.
 // TODO: Check whether anchorlist (Gold) has a max size in Monero.
 pub struct HostContainer {
@@ -317,14 +318,15 @@ impl HostContainer {
         (entry.clone(), position)
     }
 
-    /// TODO: documentation
-    pub async fn fetch_address(
+    /// Fetch addresses that match the provided transports or acceptable mixed transports.
+    /// Will return an empty Vector if no such addresses were found.
+    pub async fn fetch_addrs(
         &self,
         color: HostColor,
         transports: &[String],
         transport_mixing: bool,
     ) -> Vec<(Url, u64)> {
-        trace!(target: "net::hosts::fetch_address()", "[START] {:?}", color);
+        trace!(target: "net::hosts::fetch_addrs()", "[START] {:?}", color);
         let mut hosts = vec![];
         let index = color as usize;
 
@@ -353,7 +355,7 @@ impl HostContainer {
             hosts.push((addr, last_seen));
         }
 
-        trace!(target: "net::hosts::fetch_address()", "Grabbed hosts, length: {}", hosts.len());
+        trace!(target: "net::hosts::fetch_addrs()", "Grabbed hosts, length: {}", hosts.len());
 
         hosts
     }
@@ -558,7 +560,7 @@ impl HostContainer {
         list.remove(index);
     }
 
-    /// TODO: documentation
+    /// Remove an entry from a hostlist if it exists.
     pub async fn remove_if_exists(&self, color: HostColor, addr: &Url) {
         let index = color.clone() as usize;
         if self.contains(index, addr).await {
@@ -682,33 +684,37 @@ impl HostContainer {
     }
 }
 
-/// TODO: documentation
+/// Main parent class for the management and manipulation of hostlists. Keeps
+/// track of hosts and their current state via the HostRegistry, and stores
+/// hostlists and associated methods in the HostContainer. Also operates
+/// two subscribers to notify other parts of the code base when new channels
+/// have been created or new hosts have been added to the hostlist.
 pub struct Hosts {
-    /// Subscriber for notifications of new channels
-    channel_subscriber: SubscriberPtr<Result<ChannelPtr>>,
-
     /// A registry that tracks hosts and their current state.
     registry: HostRegistry,
 
+    /// Hostlists and associated methods.
+    pub container: HostContainer,
+
     /// Subscriber listening for store updates
     store_subscriber: SubscriberPtr<usize>,
 
+    /// Subscriber for notifications of new channels
+    channel_subscriber: SubscriberPtr<Result<ChannelPtr>>,
+
     /// Pointer to configured P2P settings
     settings: SettingsPtr,
-
-    /// TODO: documentation
-    pub container: HostContainer,
 }
 
 impl Hosts {
     /// Create a new hosts list
     pub fn new(settings: SettingsPtr) -> HostsPtr {
         Arc::new(Self {
-            channel_subscriber: Subscriber::new(),
             registry: RwLock::new(HashMap::new()),
+            container: HostContainer::new(),
             store_subscriber: Subscriber::new(),
+            channel_subscriber: Subscriber::new(),
             settings,
-            container: HostContainer::new(),
         })
     }
 
@@ -779,18 +785,18 @@ impl Hosts {
         }
     }
 
-    // TODO: documentation/ re-evaluate
-    pub async fn check_address(&self, hosts: Vec<(Url, u64)>) -> Option<(Url, u64)> {
-        // Try to find an unused host in the set.
+    // Loop through hosts selected by Outbound Session and see if any of them are
+    // free to connect to.
+    pub async fn check_addrs(&self, hosts: Vec<(Url, u64)>) -> Option<(Url, u64)> {
         for (host, last_seen) in hosts {
-            debug!(target: "net::hosts::check_address()", "Starting checks");
+            debug!(target: "net::hosts::check_addrs()", "Starting checks");
 
             if self.try_register(host.clone(), HostState::Connect).await.is_err() {
                 continue
             }
 
             debug!(
-                target: "net::hosts::check_address()",
+                target: "net::hosts::check_addrs()",
                 "Found valid host {}",
                 host
             );
@@ -987,7 +993,12 @@ impl Hosts {
         ret
     }
 
-    /// TODO: documentation
+    /// 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 the refinery passes successfully: move to white, remove from greylist.
+    /// * When we connect to a peer, move to gold, remove from white or grey.
+    /// * When we add a peer to the black list: move to black, remove from all other lists.
     pub async fn move_host(&self, addr: &Url, last_seen: u64, destination: HostColor) {
         if self.try_register(addr.clone(), HostState::Move).await.is_err() {
             return
@@ -995,13 +1006,13 @@ impl Hosts {
 
         match destination {
             // Downgrade to grey. Remove from white and gold.
-            // TODO: doc
             HostColor::Grey => {
                 self.container.remove_if_exists(HostColor::Gold, addr).await;
                 self.container.remove_if_exists(HostColor::White, addr).await;
                 self.container.store_or_update(HostColor::Grey, addr.clone(), last_seen).await;
 
-                // This should never panic.
+                // We mark this peer as Suspend which means we do not try to connect to it until it
+                // has passed through the refinery. This should never panic.
                 self.try_register(addr.clone(), HostState::Suspend).await.unwrap();
                 return
             }
@@ -1233,127 +1244,4 @@ mod tests {
             }
         });
     }
-
-    #[test]
-    fn test_fetch_address() {
-        smol::block_on(async {
-            let mut hostlist = vec![];
-            let mut grey_urls = vec![];
-            let mut white_urls = vec![];
-            let mut anchor_urls = vec![];
-
-            let ex = Arc::new(Executor::new());
-
-            let settings = Settings { ..Default::default() };
-            let p2p = P2p::new(settings, ex.clone()).await;
-            let hosts = &p2p.hosts().container;
-
-            // Build up a hostlist
-            for i in 0..5 {
-                let last_seen = UNIX_EPOCH.elapsed().unwrap().as_secs();
-                hosts
-                    .store(
-                        HostColor::Grey as usize,
-                        Url::parse(&format!("tcp://greylist{}:123", i)).unwrap(),
-                        last_seen,
-                    )
-                    .await;
-                hosts
-                    .store(
-                        HostColor::White as usize,
-                        Url::parse(&format!("tcp://whitelist{}:123", i)).unwrap(),
-                        last_seen,
-                    )
-                    .await;
-                hosts
-                    .store(
-                        HostColor::Gold as usize,
-                        Url::parse(&format!("tcp://anchorlist{}:123", i)).unwrap(),
-                        last_seen,
-                    )
-                    .await;
-
-                grey_urls
-                    .push((Url::parse(&format!("tcp://greylist{}:123", i)).unwrap(), last_seen));
-                white_urls
-                    .push((Url::parse(&format!("tcp://whitelist{}:123", i)).unwrap(), last_seen));
-                anchor_urls
-                    .push((Url::parse(&format!("tcp://anchorlist{}:123", i)).unwrap(), last_seen));
-            }
-
-            assert!(!hosts.is_empty(HostColor::Grey).await);
-            assert!(!hosts.is_empty(HostColor::White).await);
-            assert!(!hosts.is_empty(HostColor::Gold).await);
-
-            let transports = ["tcp".to_string()];
-            let white_count =
-                p2p.settings().outbound_connections * p2p.settings().white_connection_percent / 100;
-            let localnet = true;
-
-            // Simulate the address selection logic found in outbound_session::fetch_address()
-            for i in 0..8 {
-                if i < p2p.settings().anchor_connection_count {
-                    if !hosts.fetch_address(HostColor::Gold, &transports, localnet).await.is_empty()
-                    {
-                        let addrs =
-                            hosts.fetch_address(HostColor::Gold, &transports, localnet).await;
-                        hostlist.push(addrs);
-                    }
-
-                    if !hosts
-                        .fetch_address(HostColor::White, &transports, localnet)
-                        .await
-                        .is_empty()
-                    {
-                        let addrs =
-                            hosts.fetch_address(HostColor::White, &transports, localnet).await;
-                        hostlist.push(addrs);
-                    }
-
-                    if !hosts.fetch_address(HostColor::Grey, &transports, localnet).await.is_empty()
-                    {
-                        let addrs =
-                            hosts.fetch_address(HostColor::Grey, &transports, localnet).await;
-                        hostlist.push(addrs);
-                    }
-                } else if i < white_count {
-                    if !hosts
-                        .fetch_address(HostColor::White, &transports, localnet)
-                        .await
-                        .is_empty()
-                    {
-                        let addrs =
-                            hosts.fetch_address(HostColor::White, &transports, localnet).await;
-                        hostlist.push(addrs);
-                    }
-
-                    if !hosts.fetch_address(HostColor::Grey, &transports, localnet).await.is_empty()
-                    {
-                        let addrs =
-                            hosts.fetch_address(HostColor::Grey, &transports, localnet).await;
-                        hostlist.push(addrs);
-                    }
-                } else if !hosts
-                    .fetch_address(HostColor::Grey, &transports, localnet)
-                    .await
-                    .is_empty()
-                {
-                    let addrs = hosts.fetch_address(HostColor::Grey, &transports, localnet).await;
-                    hostlist.push(addrs);
-                }
-            }
-
-            // Check we're returning the correct addresses.
-            anchor_urls.sort();
-            white_urls.sort();
-            grey_urls.sort();
-            hostlist[0].sort();
-            hostlist[4].sort();
-            hostlist[7].sort();
-
-            assert!(anchor_urls == hostlist[0]);
-            assert!(white_urls == hostlist[4]);
-            assert!(grey_urls == hostlist[7]);
-        })
-    }
 }

src/net/session/outbound_session.rs

@@ -186,111 +186,124 @@ impl Slot {
         self.process.stop().await
     }
 
-    // TODO: rethink this logic.
-    async fn fetch_address(&self, slot_count: usize, transports: &[String]) -> Option<(Url, u64)> {
-        let hosts = self.p2p().hosts();
-        let connects = self.p2p().settings().outbound_connections;
-        let white_count = connects * self.p2p().settings().white_connection_percent / 100;
-        let transport_mixing = self.p2p().settings().transport_mixing;
-
-        if slot_count < self.p2p().settings().anchor_connection_count {
-            //  Up to anchor_connection_count connections:
-            //  Select from the anchorlist
-            //  If the anchorlist is empty, select from the whitelist
-            //  If the whitelist is empty, select from the greylist
-            //  If the greylist is empty, return None and do peer discovery
-            if !hosts
-                .container
-                .fetch_address(HostColor::Gold, transports, transport_mixing)
-                .await
-                .is_empty()
-            {
-                let addrs = hosts
-                    .container
-                    .fetch_address(HostColor::Gold, transports, transport_mixing)
-                    .await;
-
-                return hosts.check_address(addrs).await
+    // Address selection algorithm that works as follows: up to anchor_count, select from the
+    // anchorlist. Up to white_count, select from the whitelist. For all other slots, select from
+    // the greylist.
+    //
+    // If we didn't find an address with this selection logic, downgrade our preferences. Up to
+    // anchor_count, select from the whitelist, up until white_count, select from the greylist.
+    //
+    // If we still didn't find an address, select from the greylist. In all other cases, return an
+    // empty vector. This will trigger fetch_addrs() to return None and initiate peer discovery.
+    async fn fetch_addrs_with_preference(
+        &self,
+        preference: usize,
+        slot_count: usize,
+        transports: &[String],
+        transport_mixing: bool,
+        white_count: usize,
+        anchor_count: usize,
+    ) -> Vec<(Url, u64)> {
+        let hosts = &self.p2p().hosts().container;
+
+        match preference {
+            // Highest preference that corresponds to the anchor and white count preference set in
+            // Settings.
+            0 => {
+                if slot_count < anchor_count {
+                    hosts.fetch_addrs(HostColor::Gold, transports, transport_mixing).await
+                } else if slot_count < white_count {
+                    hosts.fetch_addrs(HostColor::White, transports, transport_mixing).await
+                } else {
+                    hosts.fetch_addrs(HostColor::Grey, transports, transport_mixing).await
+                }
             }
-
-            if !hosts
-                .container
-                .fetch_address(HostColor::White, transports, transport_mixing)
-                .await
-                .is_empty()
-            {
-                let addrs = hosts
-                    .container
-                    .fetch_address(HostColor::White, transports, transport_mixing)
-                    .await;
-
-                return hosts.check_address(addrs).await
+            // Reduced preference in case we don't have sufficient hosts to satisfy our highest
+            // preference.
+            1 => {
+                if slot_count < anchor_count {
+                    hosts.fetch_addrs(HostColor::White, transports, transport_mixing).await
+                } else if slot_count < white_count {
+                    hosts.fetch_addrs(HostColor::Grey, transports, transport_mixing).await
+                } else {
+                    vec![]
+                }
             }
-
-            if !hosts
-                .container
-                .fetch_address(HostColor::Grey, transports, transport_mixing)
-                .await
-                .is_empty()
-            {
-                let addrs = hosts
-                    .container
-                    .fetch_address(HostColor::Grey, transports, transport_mixing)
-                    .await;
-
-                return hosts.check_address(addrs).await
+            // Lowest preference if we still haven't been able to find a host.
+            2 => {
+                if slot_count < anchor_count {
+                    hosts.fetch_addrs(HostColor::Grey, transports, transport_mixing).await
+                } else {
+                    vec![]
+                }
             }
-        } else if slot_count < white_count {
-            // Up to white_connection_percent connections:
-            //  Select from the whitelist
-            //  If the whitelist is empty, select from the greylist
-            //  If the greylist is empty, return None and do peer discovery
-            if !hosts
-                .container
-                .fetch_address(HostColor::White, transports, transport_mixing)
-                .await
-                .is_empty()
-            {
-                let addrs = hosts
-                    .container
-                    .fetch_address(HostColor::White, transports, transport_mixing)
-                    .await;
-
-                return hosts.check_address(addrs).await
+            _ => {
+                panic!()
             }
+        }
+    }
 
-            if !hosts
-                .container
-                .fetch_address(HostColor::Grey, transports, transport_mixing)
-                .await
-                .is_empty()
-            {
-                let addrs = hosts
-                    .container
-                    .fetch_address(HostColor::Grey, transports, transport_mixing)
-                    .await;
-
-                return hosts.check_address(addrs).await
-            }
-        } else {
-            // All other connections:
-            //  Select from the greylist
-            //  If the greylist is empty, do peer discovery
-            if !hosts
-                .container
-                .fetch_address(HostColor::Grey, transports, transport_mixing)
-                .await
-                .is_empty()
-            {
-                let addrs = hosts
-                    .container
-                    .fetch_address(HostColor::Grey, transports, transport_mixing)
-                    .await;
-
-                return hosts.check_address(addrs).await
-            }
+    // Fetch an address we can connect to acccording to the white and anchor connection counts
+    // configured in Settings.
+    async fn fetch_addrs(&self, slot_count: usize, transports: &[String]) -> Option<(Url, u64)> {
+        let hosts = self.p2p().hosts();
+        let transport_mixing = self.p2p().settings().transport_mixing;
+        let anchor_count = self.p2p().settings().anchor_connection_count;
+        let white_count = slot_count * self.p2p().settings().white_connection_percent / 100;
+
+        // First select an addresses that match our white and anchor requirements configured in
+        // Settings.
+        let preference = 0;
+        let addrs = self
+            .fetch_addrs_with_preference(
+                preference,
+                slot_count,
+                transports,
+                transport_mixing,
+                white_count,
+                anchor_count,
+            )
+            .await;
+
+        if !addrs.is_empty() {
+            return hosts.check_addrs(addrs).await;
+        }
+
+        // If no addresses were returned, go for the second best thing (white and grey).
+        let preference = 1;
+        let addrs = self
+            .fetch_addrs_with_preference(
+                preference,
+                slot_count,
+                transports,
+                transport_mixing,
+                white_count,
+                anchor_count,
+            )
+            .await;
+
+        if !addrs.is_empty() {
+            return hosts.check_addrs(addrs).await;
+        }
+
+        // If we still have no addresses, go for the least favored option.
+        let preference = 2;
+        let addrs = self
+            .fetch_addrs_with_preference(
+                preference,
+                slot_count,
+                transports,
+                transport_mixing,
+                white_count,
+                anchor_count,
+            )
+            .await;
+
+        if !addrs.is_empty() {
+            return hosts.check_addrs(addrs).await;
         }
 
+        // If we still don't have an address, return None and do peer discovery.
         None
     }
 
@@ -328,7 +341,7 @@ impl Slot {
                 continue
             }
 
-            let addr = if let Some(addr) = self.fetch_address(slot_count, transports).await {
+            let addr = if let Some(addr) = self.fetch_addrs(slot_count, transports).await {
                 debug!(target: "net::outbound_session::run()", "Fetched address: {:?}", addr);
                 addr
             } else {
@@ -480,7 +493,10 @@ impl Slot {
     }
 }
 
-/// TODO: doc
+/// PeerDiscoveryBase defines a common interface for multiple peer discovery processes. Currently
+/// only one Peer Discovery implementation exists. Making Peer Discovery generic enables us to
+/// support network swarming, since the peer discovery process will differ depending on whether it
+/// occurs on the overlay network or a subnet.
 #[async_trait]
 pub trait PeerDiscoveryBase {
     async fn start(self: Arc<Self>);
@@ -498,7 +514,8 @@ pub trait PeerDiscoveryBase {
     fn p2p(&self) -> P2pPtr;
 }
 
-/// TODO: doc
+/// Main PeerDiscovery process that loops through connected channels and sends out a `GetAddrs`
+/// when it is active.
 struct PeerDiscovery {
     process: StoppableTaskPtr,
     wakeup_self: CondVar,