Merge pull request #1616 from madadam/node-state-refactor-part-two

Refactor Node state: extract EstablishingNode state

src/client.rs

@@ -19,7 +19,7 @@ use crate::messages::{Request, CLIENT_GET_PRIORITY, DEFAULT_PRIORITY};
 use crate::outbox::{EventBox, EventBuf};
 use crate::routing_table::Authority;
 use crate::state_machine::{State, StateMachine};
-use crate::states::{Bootstrapping, BootstrappingTargetState};
+use crate::states::{BootstrappingPeer, TargetState};
 use crate::types::{MessageId, RoutingActionSender};
 use crate::xor_name::XorName;
 use crate::{BootstrapConfig, MIN_SECTION_SIZE};
@@ -72,16 +72,17 @@ impl Client {
 
         StateMachine::new(
             move |action_sender, crust_service, timer, _outbox2| {
-                Bootstrapping::new(
+                BootstrappingPeer::new(
                     action_sender,
                     Box::new(NullCache),
-                    BootstrappingTargetState::Client { msg_expiry_dur },
+                    TargetState::Client { msg_expiry_dur },
                     crust_service,
                     full_id,
                     min_section_size,
                     timer,
                 )
-                .map_or(State::Terminated, State::Bootstrapping)
+                .map(State::BootstrappingPeer)
+                .unwrap_or(State::Terminated)
             },
             pub_id,
             bootstrap_config,

src/mock_parsec/mod.rs

@@ -184,10 +184,14 @@ where
         unimplemented!()
     }
 
+    // TODO: rename this to `has_unpolled_observations`
     pub fn has_unconsensused_observations(&self) -> bool {
-        self.observations
-            .iter()
-            .any(|(_, obs)| obs.state != ConsensusState::Polled)
+        state::with::<T, S::PublicId, _, _>(self.section_hash, |state| {
+            state
+                .observations
+                .iter()
+                .any(|(_, observation_state)| !observation_state.consensused())
+        }) || self.our_unpolled_observations().next().is_some()
     }
 
     pub fn our_unpolled_observations(&self) -> impl Iterator<Item = &Observation<T, S::PublicId>> {

src/mock_parsec/observation.rs

@@ -83,6 +83,10 @@ impl<P: PublicId> ObservationState<P> {
         }
     }
 
+    pub fn consensused(&self) -> bool {
+        self.consensused
+    }
+
     fn compute_consensus<T: NetworkEvent>(
         &mut self,
         peers: &BTreeSet<P>,

src/node.rs

@@ -22,7 +22,7 @@ use crate::messages::{
 use crate::outbox::{EventBox, EventBuf};
 use crate::routing_table::Authority;
 use crate::state_machine::{State, StateMachine};
-use crate::states::{self, Bootstrapping, BootstrappingTargetState};
+use crate::states::{self, BootstrappingPeer, TargetState};
 use crate::types::{MessageId, RoutingActionSender};
 use crate::xor_name::XorName;
 #[cfg(feature = "mock_base")]
@@ -141,34 +141,27 @@ impl NodeBuilder {
         StateMachine::new(
             move |action_sender, crust_service, timer, outbox2| {
                 if self.first {
-                    if let Some(state) = states::Node::first(
-                        self.cache,
-                        crust_service,
-                        full_id,
-                        min_section_size,
-                        timer,
-                    ) {
-                        State::Node(state)
-                    } else {
-                        State::Terminated
-                    }
+                    states::Node::first(self.cache, crust_service, full_id, min_section_size, timer)
+                        .map(State::Node)
+                        .unwrap_or(State::Terminated)
                 } else if !dev_config.allow_multiple_lan_nodes && crust_service.has_peers_on_lan() {
                     error!(
                         "More than one routing node found on LAN. Currently this is not supported."
                     );
                     outbox2.send_event(Event::Terminated);
                     State::Terminated
                 } else {
-                    Bootstrapping::new(
+                    BootstrappingPeer::new(
                         action_sender,
                         self.cache,
-                        BootstrappingTargetState::RelocatingNode,
+                        TargetState::RelocatingNode,
                         crust_service,
                         full_id,
                         min_section_size,
                         timer,
                     )
-                    .map_or(State::Terminated, State::Bootstrapping)
+                    .map(State::BootstrappingPeer)
+                    .unwrap_or(State::Terminated)
                 }
             },
             pub_id,
@@ -616,10 +609,10 @@ impl Node {
     }
 
     /// Indicates if there are any pending observations in the parsec object
-    pub fn has_unconsensused_observations(&self, filter_opaque: bool) -> bool {
+    pub fn has_unpolled_observations(&self, filter_opaque: bool) -> bool {
         self.machine
             .current()
-            .has_unconsensused_observations(filter_opaque)
+            .has_unpolled_observations(filter_opaque)
     }
 
     /// Indicates if a given `PublicId` is in the peer manager as a routing peer

src/parsec.rs

@@ -36,9 +36,11 @@ pub struct ParsecMap {
 
 impl ParsecMap {
     pub fn new(full_id: FullId, gen_pfx_info: &GenesisPfxInfo) -> Self {
-        let parsec = create(full_id, gen_pfx_info);
         let mut map = BTreeMap::new();
-        let _ = map.insert(*gen_pfx_info.first_info.version(), parsec);
+        let _ = map.insert(
+            *gen_pfx_info.first_info.version(),
+            create(full_id, gen_pfx_info),
+        );
 
         Self { map }
     }
@@ -100,11 +102,10 @@ impl ParsecMap {
     }
 
     pub fn create_gossip(&mut self, version: u64, target: &id::PublicId) -> Option<Message> {
-        let parsec = self.map.get_mut(&version)?;
-        parsec
-            .create_gossip(target)
-            .ok()
-            .map(|request| Message::Direct(DirectMessage::ParsecRequest(version, request)))
+        let request = self.map.get_mut(&version)?.create_gossip(target).ok()?;
+        Some(Message::Direct(DirectMessage::ParsecRequest(
+            version, request,
+        )))
     }
 
     pub fn vote_for(&mut self, event: chain::NetworkEvent, log_ident: &str) {
@@ -159,7 +160,7 @@ impl ParsecMap {
     }
 
     #[cfg(feature = "mock_base")]
-    pub fn has_unconsensused_observations(&self, filter_opaque: bool) -> bool {
+    pub fn has_unpolled_observations(&self, filter_opaque: bool) -> bool {
         let parsec = if let Some(parsec) = self.map.values().last() {
             parsec
         } else {
@@ -177,7 +178,7 @@ impl ParsecMap {
 }
 
 /// Create Parsec instance.
-pub fn create(full_id: FullId, gen_pfx_info: &GenesisPfxInfo) -> Parsec {
+fn create(full_id: FullId, gen_pfx_info: &GenesisPfxInfo) -> Parsec {
     if gen_pfx_info
         .first_info
         .members()

src/peer_manager.rs

@@ -308,7 +308,6 @@ pub struct PeerManager {
     our_public_id: PublicId,
     candidate: Candidate,
     disable_client_rate_limiter: bool,
-    established: bool,
 }
 
 impl PeerManager {
@@ -320,7 +319,6 @@ impl PeerManager {
             our_public_id: our_public_id,
             candidate: Candidate::None,
             disable_client_rate_limiter: disable_client_rate_limiter,
-            established: false,
         }
     }
 
@@ -633,12 +631,7 @@ impl PeerManager {
     }
 
     /// Remove and return `PublicId`s of expired peers.
-    /// Will only be active once we are established.
     pub fn remove_expired_peers(&mut self) -> Vec<PublicId> {
-        if !self.established {
-            return vec![];
-        }
-
         let remove_candidate = if self.candidate.is_expired() {
             match self.candidate {
                 Candidate::None => None,
@@ -979,17 +972,6 @@ impl PeerManager {
 
         self.peers.remove(pub_id).is_some() || remove_candidate
     }
-
-    /// Sets this peer as established.
-    /// Expired peers will be purged once established.
-    pub fn set_established(&mut self) {
-        self.established = true;
-    }
-
-    /// Returns whether this peer is established.
-    pub fn is_established(&self) -> bool {
-        self.established
-    }
 }
 
 impl fmt::Display for PeerManager {