[network] Use NetworkId instead of RoleType to identify networks

RoleType was representing what kind of node the code is running, and the
type of networks, which was making it very confusing to tell when there
were "full node" networks on validators.  RoleType is now properly set
for the network context, and is not used to make Network decisions, just
for logging.

There was additionally a gap in logic for default network configs used
for tests and the network configs loaded in real situations.  This
ensures all are properly handled the same.

Closes: aptos-labs#7604

config/management/operational/src/network_checker.rs

@@ -117,29 +117,31 @@ fn parse_hex(src: &str) -> Result<Key, Error> {
 
 impl CheckValidatorSetEndpoints {
     pub fn execute(self) -> Result<String, Error> {
+        let is_validator = self.role.is_validator();
+
         // Following unwraps shouldn't fail as it is in memory
         let mut encryptor = Encryptor::new(Storage::InMemoryStorage(InMemoryStorage::new()));
         encryptor.initialize().unwrap();
-        let encryptor = match self.role {
-            RoleType::FullNode => encryptor,
-            RoleType::Validator => {
-                encryptor
-                    .add_key(self.version.unwrap(), self.key.unwrap())
-                    .unwrap();
-                encryptor
-                    .set_current_version(self.version.unwrap())
-                    .unwrap();
-                encryptor
-            }
+        let encryptor = if is_validator {
+            encryptor
+                .add_key(self.version.unwrap(), self.key.unwrap())
+                .unwrap();
+            encryptor
+                .set_current_version(self.version.unwrap())
+                .unwrap();
+            encryptor
+        } else {
+            encryptor
         };
 
         let client = JsonRpcClientWrapper::new(self.json_server);
         let validator_set = crate::validator_set::decode_validator_set(encryptor, client, None)?;
 
         for info in validator_set {
-            let address = match self.role {
-                RoleType::FullNode => info.fullnode_network_address.clone(),
-                RoleType::Validator => info.validator_network_address.clone(),
+            let address = if is_validator {
+                info.fullnode_network_address.clone()
+            } else {
+                info.validator_network_address.clone()
             };
             let check_endpoint = CheckEndpoint { address };
             match check_endpoint.execute() {

config/src/config/mod.rs

@@ -228,27 +228,37 @@ impl NodeConfig {
     /// Paths used in the config are either absolute or relative to the config location
     pub fn load<P: AsRef<Path>>(input_path: P) -> Result<Self, Error> {
         let mut config = Self::load_config(&input_path)?;
-        if config.base.role.is_validator() {
+
+        let input_dir = RootPath::new(input_path);
+        config.execution.load(&input_dir)?;
+
+        let mut config = config.validate_network_configs()?;
+        config.set_data_dir(config.data_dir().clone());
+        Ok(config)
+    }
+
+    /// Checks `NetworkConfig` setups so that they exist on proper networks
+    /// Additionally, handles any strange missing default cases
+    fn validate_network_configs(mut self) -> Result<NodeConfig, Error> {
+        if self.base.role.is_validator() {
             invariant(
-                config.validator_network.is_some(),
+                self.validator_network.is_some(),
                 "Missing a validator network config for a validator node".into(),
             )?;
         } else {
             invariant(
-                config.validator_network.is_none(),
+                self.validator_network.is_none(),
                 "Provided a validator network config for a full_node node".into(),
             )?;
         }
 
         let mut network_ids = HashSet::new();
-        let input_dir = RootPath::new(input_path);
-        config.execution.load(&input_dir)?;
-        if let Some(network) = &mut config.validator_network {
-            network.load(RoleType::Validator)?;
+        if let Some(network) = &mut self.validator_network {
+            network.load_validator_network()?;
             network_ids.insert(network.network_id.clone());
         }
-        for network in &mut config.full_node_networks {
-            network.load(RoleType::FullNode)?;
+        for network in &mut self.full_node_networks {
+            network.load_fullnode_network()?;
 
             // Check a validator network is not included in a list of full-node networks
             let network_id = &network.network_id;
@@ -258,8 +268,7 @@ impl NodeConfig {
             )?;
             network_ids.insert(network_id.clone());
         }
-        config.set_data_dir(config.data_dir().clone());
-        Ok(config)
+        Ok(self)
     }
 
     pub fn save<P: AsRef<Path>>(&mut self, output_path: P) -> Result<(), Error> {
@@ -332,22 +341,26 @@ impl NodeConfig {
         self.test = Some(test);
     }
 
+    fn default_config(serialized: &str, path: &'static str) -> Self {
+        let config = Self::parse(serialized).unwrap_or_else(|e| panic!("Error in {}: {}", path, e));
+        config
+            .validate_network_configs()
+            .unwrap_or_else(|e| panic!("Error in {}: {}", path, e))
+    }
+
     pub fn default_for_public_full_node() -> Self {
         let contents = std::include_str!("test_data/public_full_node.yaml");
-        let path = "default_for_public_full_node";
-        Self::parse(&contents).unwrap_or_else(|e| panic!("Error in {}: {}", path, e))
+        Self::default_config(contents, "default_for_public_full_node")
     }
 
     pub fn default_for_validator() -> Self {
         let contents = std::include_str!("test_data/validator.yaml");
-        let path = "default_for_validator";
-        Self::parse(&contents).unwrap_or_else(|e| panic!("Error in {}: {}", path, e))
+        Self::default_config(contents, "default_for_validator")
     }
 
     pub fn default_for_validator_full_node() -> Self {
         let contents = std::include_str!("test_data/validator_full_node.yaml");
-        let path = "default_for_validator_full_node";
-        Self::parse(&contents).unwrap_or_else(|e| panic!("Error in {}: {}", path, e))
+        Self::default_config(contents, "default_for_validator_full_node")
     }
 }
 

config/src/config/network_config.rs

@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use crate::{
-    config::{Error, RoleType, SecureBackend},
+    config::{Error, SecureBackend},
     keys::ConfigKey,
     network_id::NetworkId,
     utils,
@@ -167,20 +167,28 @@ impl NetworkConfig {
         }
     }
 
-    pub fn load(&mut self, role: RoleType) -> Result<(), Error> {
+    /// Per convenience, so that NetworkId isn't needed to be specified for `validator_networks`
+    pub fn load_validator_network(&mut self) -> Result<(), Error> {
+        self.network_id = NetworkId::Validator;
+        self.load()
+    }
+
+    pub fn load_fullnode_network(&mut self) -> Result<(), Error> {
+        if self.network_id.is_validator_network() {
+            return Err(Error::InvariantViolation(format!(
+                "Set {} network for a non-validator network",
+                self.network_id
+            )));
+        }
+        self.load()
+    }
+
+    fn load(&mut self) -> Result<(), Error> {
         if self.listen_address.to_string().is_empty() {
             self.listen_address = utils::get_local_ip()
                 .ok_or_else(|| Error::InvariantViolation("No local IP".to_string()))?;
         }
 
-        if role == RoleType::Validator {
-            self.network_id = NetworkId::Validator;
-        } else if self.network_id == NetworkId::Validator {
-            return Err(Error::InvariantViolation(
-                "Set NetworkId::Validator network for a non-validator network".to_string(),
-            ));
-        }
-
         self.prepare_identity();
         Ok(())
     }

config/src/network_id.rs

@@ -10,9 +10,10 @@ use std::fmt;
 /// for logging accordingly.
 #[derive(Clone, Eq, PartialEq, Serialize)]
 pub struct NetworkContext {
+    /// The type of node
+    role: RoleType,
     #[serde(serialize_with = "NetworkId::serialize_str")]
     network_id: NetworkId,
-    role: RoleType,
     peer_id: PeerId,
 }
 
@@ -27,48 +28,48 @@ impl fmt::Display for NetworkContext {
         write!(
             f,
             "[{},{},{}]",
-            self.network_id.as_str(),
             self.role,
+            self.network_id.as_str(),
             self.peer_id.short_str(),
         )
     }
 }
 
 impl NetworkContext {
-    pub fn new(network_id: NetworkId, role: RoleType, peer_id: PeerId) -> NetworkContext {
+    pub fn new(role: RoleType, network_id: NetworkId, peer_id: PeerId) -> NetworkContext {
         NetworkContext {
-            network_id,
             role,
+            network_id,
             peer_id,
         }
     }
 
-    pub fn network_id(&self) -> &NetworkId {
-        &self.network_id
-    }
-
     pub fn role(&self) -> RoleType {
         self.role
     }
 
+    pub fn network_id(&self) -> &NetworkId {
+        &self.network_id
+    }
+
     pub fn peer_id(&self) -> PeerId {
         self.peer_id
     }
 
     #[cfg(any(test, feature = "testing", feature = "fuzzing"))]
     pub fn mock_with_peer_id(peer_id: PeerId) -> std::sync::Arc<Self> {
         std::sync::Arc::new(Self::new(
-            NetworkId::Validator,
             RoleType::Validator,
+            NetworkId::Validator,
             peer_id,
         ))
     }
 
     #[cfg(any(test, feature = "testing", feature = "fuzzing"))]
     pub fn mock() -> std::sync::Arc<Self> {
         std::sync::Arc::new(Self::new(
-            NetworkId::Validator,
             RoleType::Validator,
+            NetworkId::Validator,
             PeerId::random(),
         ))
     }
@@ -147,6 +148,10 @@ impl NetworkId {
         matches!(self, NetworkId::Private(network) if network == VFN_NETWORK)
     }
 
+    pub fn is_validator_network(&self) -> bool {
+        matches!(self, NetworkId::Validator)
+    }
+
     pub fn as_str(&self) -> &str {
         match self {
             NetworkId::Validator => "Validator",
@@ -194,12 +199,13 @@ mod test {
 
     #[test]
     fn test_network_context_serialization() {
-        let role = RoleType::Validator;
         let peer_id = PeerId::random();
-        let context = NetworkContext::new(NetworkId::vfn_network(), role, peer_id);
+        let context = NetworkContext::new(RoleType::Validator, NetworkId::vfn_network(), peer_id);
         let expected = format!(
-            "---\nnetwork_id: {}\nrole: {}\npeer_id: {}\n",
-            VFN_NETWORK, role, peer_id
+            "---\nrole: {}\nnetwork_id: {}\npeer_id: {}\n",
+            RoleType::Validator,
+            VFN_NETWORK,
+            peer_id
         );
         assert_eq!(expected, serde_yaml::to_string(&context).unwrap());
     }

diem-node/src/lib.rs

@@ -5,7 +5,7 @@ use backup_service::start_backup_service;
 use consensus::{consensus_provider::start_consensus, gen_consensus_reconfig_subscription};
 use debug_interface::node_debug_service::NodeDebugService;
 use diem_config::{
-    config::{NetworkConfig, NodeConfig, RoleType},
+    config::{NetworkConfig, NodeConfig},
     network_id::NodeNetworkId,
     utils::get_genesis_txn,
 };
@@ -316,23 +316,22 @@ pub fn setup_environment(node_config: &NodeConfig, logger: Option<Arc<Logger>>)
     }
 
     // Gather all network configs into a single vector.
-    // TODO:  consider explicitly encoding the role in the NetworkConfig
-    let mut network_configs: Vec<(RoleType, &NetworkConfig)> = node_config
-        .full_node_networks
-        .iter()
-        .map(|network_config| (RoleType::FullNode, network_config))
-        .collect();
+    let mut network_configs: Vec<&NetworkConfig> = node_config.full_node_networks.iter().collect();
     if let Some(network_config) = node_config.validator_network.as_ref() {
-        network_configs.push((RoleType::Validator, network_config));
+        network_configs.push(network_config);
     }
 
     let mut network_builders = Vec::new();
 
     // Instantiate every network and collect the requisite endpoints for state_sync, mempool, and consensus.
-    for (idx, (role, network_config)) in network_configs.into_iter().enumerate() {
+    for (idx, network_config) in network_configs.into_iter().enumerate() {
         // Perform common instantiation steps
-        let mut network_builder =
-            NetworkBuilder::create(chain_id, role, network_config, TimeService::real());
+        let mut network_builder = NetworkBuilder::create(
+            chain_id,
+            node_config.base.role,
+            network_config,
+            TimeService::real(),
+        );
         let network_id = network_config.network_id.clone();
 
         // Create the endpoints to connect the Network to State Sync.
@@ -349,27 +348,23 @@ pub fn setup_environment(node_config: &NodeConfig, logger: Option<Arc<Logger>>)
             diem_mempool::network::network_endpoint_config(MEMPOOL_NETWORK_CHANNEL_BUFFER_SIZE),
         );
         mempool_network_handles.push((
-            NodeNetworkId::new(network_id, idx),
+            NodeNetworkId::new(network_id.clone(), idx),
             mempool_sender,
             mempool_events,
         ));
 
-        match role {
-            // Perform steps relevant specifically to Validator networks.
-            RoleType::Validator => {
-                // A valid config is allowed to have at most one ValidatorNetwork
-                // TODO:  `expect_none` would be perfect here, once it is stable.
-                if consensus_network_handles.is_some() {
-                    panic!("There can be at most one validator network!");
-                }
-
-                consensus_network_handles =
-                    Some(network_builder.add_protocol_handler(
-                        consensus::network_interface::network_endpoint_config(),
-                    ));
+        // Perform steps relevant specifically to Validator networks.
+        if network_id.is_validator_network() {
+            // A valid config is allowed to have at most one ValidatorNetwork
+            // TODO:  `expect_none` would be perfect here, once it is stable.
+            if consensus_network_handles.is_some() {
+                panic!("There can be at most one validator network!");
             }
-            // Currently no FullNode network specific steps.
-            RoleType::FullNode => (),
+
+            consensus_network_handles = Some(
+                network_builder
+                    .add_protocol_handler(consensus::network_interface::network_endpoint_config()),
+            );
         }
 
         reconfig_subscriptions.append(network_builder.reconfig_subscriptions());
@@ -382,11 +377,7 @@ pub fn setup_environment(node_config: &NodeConfig, logger: Option<Arc<Logger>>)
         let network_context = network_builder.network_context();
         debug!("Creating runtime for {}", network_context);
         let runtime = Builder::new_multi_thread()
-            .thread_name(format!(
-                "network-{}-{}",
-                network_context.role(),
-                network_context.network_id()
-            ))
+            .thread_name(format!("network-{}", network_context.network_id()))
             .enable_all()
             .build()
             .expect("Failed to start runtime. Won't be able to start networking.");

network/builder/src/builder.rs

@@ -179,8 +179,8 @@ impl NetworkBuilder {
         };
 
         let network_context = Arc::new(NetworkContext::new(
-            config.network_id.clone(),
             role,
+            config.network_id.clone(),
             peer_id,
         ));
 
@@ -340,7 +340,8 @@ impl NetworkBuilder {
         channel_size: usize,
     ) -> &mut Self {
         let pm_conn_mgr_notifs_rx = self.add_connection_event_listener();
-        let outbound_connection_limit = if let RoleType::FullNode = self.network_context.role() {
+        let outbound_connection_limit = if !self.network_context.network_id().is_validator_network()
+        {
             Some(max_outbound_connections)
         } else {
             None