Merge pull request #1876 from octol/review-comments-for-conn-req-part-1

Review comment fixes for #1868

src/chain/bls_emu.rs

@@ -154,19 +154,22 @@ mod test {
         id::{FullId, P2pNode},
         ConnectionInfo,
     };
-    use std::net::SocketAddr;
     use unwrap::unwrap;
 
     fn gen_section(size: usize) -> (PublicKeySet, Vec<SecretKeyShare>) {
         let ids: Vec<_> = (0..size).map(|_| FullId::new()).collect();
-        let socket_addr: SocketAddr = unwrap!("127.0.0.1:9999".parse());
-        let connection_info = ConnectionInfo {
-            peer_addr: socket_addr,
-            peer_cert_der: vec![],
-        };
         let p2p_nodes = ids
             .iter()
-            .map(|full_id| P2pNode::new(*full_id.public_id(), connection_info.clone()))
+            .enumerate()
+            .map(|(index, full_id)| {
+                P2pNode::new(
+                    *full_id.public_id(),
+                    ConnectionInfo {
+                        peer_addr: ([127, 0, 0, 1], (index + 9000) as u16).into(),
+                        peer_cert_der: vec![],
+                    },
+                )
+            })
             .collect();
         let elders_info = unwrap!(EldersInfo::new(p2p_nodes, Default::default(), None));
         let pk_set = PublicKeySet::from_elders_info(elders_info);

src/chain/chain.rs

@@ -422,7 +422,7 @@ impl Chain {
             .state
             .our_members
             .entry(pub_id)
-            .or_insert_with(|| MemberInfo::new(p2p_node.connection_info().clone()));
+            .or_insert_with(|| MemberInfo::new(p2p_node.into_connection_info()));
         info.state = MemberState::Joined;
         info.set_age(age);
     }
@@ -464,17 +464,13 @@ impl Chain {
         // We already have the connection info from when it was added online.
         let connection_info = self
             .get_member_connection_info(&pub_id)
-            .ok_or(RoutingError::InvalidStateForOperation)?;
+            .ok_or(RoutingError::PeerNotFound(pub_id))?;
 
         let mut elders_p2p = self.state.new_info.p2p_members().clone();
-        let _ = elders_p2p.insert(P2pNode::new(pub_id, connection_info));
-
-        // WIP: remove me by the end of this PR
-        let mut elders = self.state.new_info.members();
-        let _ = elders.insert(pub_id);
+        let _ = elders_p2p.insert(P2pNode::new(pub_id, connection_info.clone()));
 
         // TODO: the split decision should be based on the number of all members, not just elders.
-        if self.should_split(&elders)? {
+        if self.should_split(&elders_p2p)? {
             let (our_info, other_info) = self.split_self(elders_p2p.clone())?;
             self.state.change = PrefixChange::Splitting;
             return Ok(vec![our_info, other_info]);
@@ -497,8 +493,8 @@ impl Chain {
         let mut elders = self.state.new_info.p2p_members().clone();
         let connection_info = self
             .get_member_connection_info(&pub_id)
-            .ok_or(RoutingError::InvalidStateForOperation)?;
-        let p2p_node = P2pNode::new(pub_id, connection_info);
+            .ok_or(RoutingError::PeerNotFound(pub_id))?;
+        let p2p_node = P2pNode::new(pub_id, connection_info.clone());
         let _ = elders.remove(&p2p_node);
 
         if self.our_id() == &pub_id {
@@ -630,11 +626,11 @@ impl Chain {
     }
 
     /// Returns the `ConnectioInfo` for a member of our section.
-    pub fn get_member_connection_info(&self, pub_id: &PublicId) -> Option<ConnectionInfo> {
+    pub fn get_member_connection_info(&self, pub_id: &PublicId) -> Option<&ConnectionInfo> {
         self.state
             .our_members
             .get(&pub_id)
-            .map(|member_info| member_info.connection_info.clone())
+            .map(|member_info| &member_info.connection_info)
     }
 
     /// Returns a set of elders we should be connected to.
@@ -1010,15 +1006,18 @@ impl Chain {
     }
 
     /// Returns whether we should split into two sections.
-    fn should_split(&self, members: &BTreeSet<PublicId>) -> Result<bool, RoutingError> {
+    fn should_split(&self, members: &BTreeSet<P2pNode>) -> Result<bool, RoutingError> {
         if self.state.change != PrefixChange::None || self.should_vote_for_merge() {
             return Ok(false);
         }
 
         let new_size = members
             .iter()
-            .filter(|id| {
-                self.our_id.name().common_prefix(id.name()) > self.our_prefix().bit_count()
+            .filter(|p2p_node| {
+                self.our_id
+                    .name()
+                    .common_prefix(p2p_node.public_id().name())
+                    > self.our_prefix().bit_count()
             })
             .count();
         let min_split_size = self.min_split_size();

src/chain/elders_info.rs

@@ -14,6 +14,7 @@ use crate::{
     routing_table::Prefix,
     XorName, {QUORUM_DENOMINATOR, QUORUM_NUMERATOR},
 };
+use itertools::Itertools;
 use maidsafe_utilities::serialisation;
 use serde::{de::Error as SerdeDeError, Deserialize, Deserializer, Serialize, Serializer};
 use std::{
@@ -196,16 +197,11 @@ impl Display for EldersInfo {
         writeln!(formatter, "\t\tprefix: {:?},", self.prefix)?;
         writeln!(formatter, "\t\tversion: {:?},", self.version)?;
         writeln!(formatter, "\t\tprev_hash_len: {},", self.prev_hash.len())?;
-        write!(formatter, "\t\tmembers: [")?;
-        for (index, member) in self.members().iter().enumerate() {
-            let comma = if index == self.members.len() - 1 {
-                ""
-            } else {
-                ","
-            };
-            write!(formatter, " {}{}", member.name(), comma)?;
-        }
-        writeln!(formatter, " ]")?;
+        writeln!(
+            formatter,
+            "members: [{}]",
+            self.members.iter().map(|member| member.name()).format(", ")
+        )?;
         writeln!(formatter, "\t}}")
     }
 }

src/chain/shared_state.rs

@@ -699,22 +699,21 @@ mod test {
     use super::*;
     use crate::{chain::EldersInfo, id::P2pNode, ConnectionInfo, FullId, Prefix, XorName};
     use std::collections::BTreeSet;
-    use std::net::SocketAddr;
     use std::str::FromStr;
     use unwrap::unwrap;
 
     fn gen_elders_info(pfx: Prefix<XorName>, version: u64) -> EldersInfo {
         let sec_size = 5;
         let mut members = BTreeSet::new();
-        for _ in 0..sec_size {
-            let id = FullId::within_range(&pfx.range_inclusive());
-            let socket_addr: SocketAddr = unwrap!("127.0.0.1:9999".parse());
-            let connection_info = ConnectionInfo {
-                peer_addr: socket_addr,
-                peer_cert_der: vec![],
-            };
-            let _ = members.insert(P2pNode::new(*id.public_id(), connection_info));
-        }
+        (0..sec_size).for_each(|index| {
+            let _ = members.insert(P2pNode::new(
+                *FullId::within_range(&pfx.range_inclusive()).public_id(),
+                ConnectionInfo {
+                    peer_addr: ([127, 0, 0, 1], 9000 + index).into(),
+                    peer_cert_der: vec![],
+                },
+            ));
+        });
         unwrap!(EldersInfo::new_for_test(members, pfx, version))
     }
 

src/error.rs

@@ -84,8 +84,8 @@ pub enum RoutingError {
     InvalidStateForOperation,
     /// Serialisation Error
     SerialisationError(serialisation::SerialisationError),
-    /// Unknown Connection
-    UnknownConnection(PublicId),
+    /// Peer not found
+    PeerNotFound(PublicId),
     /// Invalid Destination
     InvalidDestination,
     /// Invalid Source

src/id.rs

@@ -223,13 +223,12 @@ fn name_from_key(public_key: &signing::PublicKey) -> XorName {
 /// Network p2p node identity.
 /// When a node knows another node as a `P2pNode` it's implicitly connected to it. This is separate
 /// from being connected at the network layer, which currently is handled by quic-p2p.
-#[derive(Hash, PartialEq, Eq, Clone)]
+#[derive(Hash, PartialEq, Eq, Clone, Serialize, Deserialize)]
 pub struct P2pNode {
     public_id: PublicId,
     connection_info: ConnectionInfo,
 }
 
-#[allow(unused)]
 impl P2pNode {
     /// Creates a new `P2pNode` given a `PublicId` and a `ConnectionInfo`.
     pub fn new(public_id: PublicId, connection_info: ConnectionInfo) -> Self {
@@ -239,6 +238,11 @@ impl P2pNode {
         }
     }
 
+    /// Creates a `ConnectionInfo` from the `P2pNode` instance.
+    pub fn into_connection_info(self) -> ConnectionInfo {
+        self.connection_info
+    }
+
     /// Returns the `PublicId`.
     pub fn public_id(&self) -> &PublicId {
         &self.public_id
@@ -257,7 +261,12 @@ impl P2pNode {
 
 impl Debug for P2pNode {
     fn fmt(&self, formatter: &mut Formatter) -> fmt::Result {
-        write!(formatter, "PublicId(name: {})", self.public_id.name())
+        write!(
+            formatter,
+            "P2pNode({} at {})",
+            self.public_id.name(),
+            self.connection_info.peer_addr,
+        )
     }
 }
 
@@ -267,22 +276,6 @@ impl Display for P2pNode {
     }
 }
 
-impl Serialize for P2pNode {
-    fn serialize<S: Serializer>(&self, serialiser: S) -> Result<S::Ok, S::Error> {
-        (&self.public_id, &self.connection_info).serialize(serialiser)
-    }
-}
-
-impl<'de> Deserialize<'de> for P2pNode {
-    fn deserialize<D: Deserializer<'de>>(deserialiser: D) -> Result<Self, D::Error> {
-        let (public_id, connection_info) = Deserialize::deserialize(deserialiser)?;
-        Ok(P2pNode {
-            public_id,
-            connection_info,
-        })
-    }
-}
-
 impl PartialOrd for P2pNode {
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
         Some(self.cmp(other))

src/states/bootstrapping_peer.rs

@@ -344,14 +344,7 @@ impl Base for BootstrappingPeer {
     ) -> Result<Transition, RoutingError> {
         match msg {
             DirectMessage::BootstrapResponse(BootstrapResponse::Join { prefix, p2p_nodes }) => {
-                let conn_infos: Vec<_> = p2p_nodes
-                    .iter()
-                    .map(|n| n.connection_info().clone())
-                    .collect();
-                info!(
-                    "{} - Joining a section {:?}: {:?}",
-                    self, prefix, conn_infos
-                );
+                info!("{} - Joining a section {:?}: {:?}", self, prefix, p2p_nodes);
                 self.join_section(prefix, p2p_nodes)
             }
             DirectMessage::BootstrapResponse(BootstrapResponse::Rebootstrap(new_conn_infos)) => {

src/states/common/base.rs

@@ -289,7 +289,7 @@ pub trait Base: Display {
                 let connection_info = self
                     .peer_map()
                     .get_connection_info(public_id)
-                    .ok_or(RoutingError::UnknownConnection(public_id))?
+                    .ok_or(RoutingError::PeerNotFound(public_id))?
                     .clone();
                 self.handle_direct_message(msg, P2pNode::new(public_id, connection_info), outbox)
             }

src/states/elder/mod.rs

@@ -689,7 +689,8 @@ impl Elder {
                 LogLevel::Error,
                 "Not connected to the sender of BootstrapRequest."
             );
-            return Err(RoutingError::UnknownConnection(pub_id));
+            // Note: peer_map and this block is scheduled for removal
+            return Err(RoutingError::PeerNotFound(pub_id));
         };
 
         if self.chain.is_peer_our_member(&pub_id) {