From 97bf5bda328b84b9f39d6b8efb2f76d9ff99840c Mon Sep 17 00:00:00 2001 From: Greg Nazario Date: Wed, 10 Feb 2021 16:02:36 -0800 Subject: [PATCH] [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: #7604 --- .../operational/src/network_checker.rs | 30 +++++----- config/src/config/mod.rs | 47 ++++++++++------ config/src/config/network_config.rs | 28 ++++++---- config/src/network_id.rs | 34 +++++++----- diem-node/src/lib.rs | 55 ++++++++----------- network/builder/src/builder.rs | 5 +- network/builder/src/dummy.rs | 9 +-- network/simple-onchain-discovery/src/lib.rs | 36 ++++++------ network/src/counters.rs | 2 +- network/src/noise/mod.rs | 4 +- network/src/peer_manager/mod.rs | 4 +- state-sync/tests/test_harness.rs | 2 +- 12 files changed, 137 insertions(+), 119 deletions(-) diff --git a/config/management/operational/src/network_checker.rs b/config/management/operational/src/network_checker.rs index 34c876e3c0f48..7e5e547ec96f9 100644 --- a/config/management/operational/src/network_checker.rs +++ b/config/management/operational/src/network_checker.rs @@ -117,29 +117,31 @@ fn parse_hex(src: &str) -> Result { impl CheckValidatorSetEndpoints { pub fn execute(self) -> Result { + 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() { diff --git a/config/src/config/mod.rs b/config/src/config/mod.rs index f47341de0fdc3..6ad946fd63b60 100644 --- a/config/src/config/mod.rs +++ b/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>(input_path: P) -> Result { 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 { + 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>(&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") } } diff --git a/config/src/config/network_config.rs b/config/src/config/network_config.rs index 62f7c12dd35db..dc45ed4820e74 100644 --- a/config/src/config/network_config.rs +++ b/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(()) } diff --git a/config/src/network_id.rs b/config/src/network_id.rs index 86cb926cdbf16..c413ee0b2977e 100644 --- a/config/src/network_id.rs +++ b/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,30 +28,30 @@ 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 } @@ -58,8 +59,8 @@ impl NetworkContext { #[cfg(any(test, feature = "testing", feature = "fuzzing"))] pub fn mock_with_peer_id(peer_id: PeerId) -> std::sync::Arc { std::sync::Arc::new(Self::new( - NetworkId::Validator, RoleType::Validator, + NetworkId::Validator, peer_id, )) } @@ -67,8 +68,8 @@ impl NetworkContext { #[cfg(any(test, feature = "testing", feature = "fuzzing"))] pub fn mock() -> std::sync::Arc { 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()); } diff --git a/diem-node/src/lib.rs b/diem-node/src/lib.rs index 54c0062ecd094..eccec277c0fa5 100644 --- a/diem-node/src/lib.rs +++ b/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>) } // 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>) 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>) 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."); diff --git a/network/builder/src/builder.rs b/network/builder/src/builder.rs index 2a1294b318664..b155102a49a69 100644 --- a/network/builder/src/builder.rs +++ b/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 diff --git a/network/builder/src/dummy.rs b/network/builder/src/dummy.rs index a70f24b17ce77..1e96d427f2927 100644 --- a/network/builder/src/dummy.rs +++ b/network/builder/src/dummy.rs @@ -111,6 +111,7 @@ pub struct DummyNetwork { /// The following sets up a 2 peer network and verifies connectivity. pub fn setup_network() -> DummyNetwork { let runtime = Runtime::new().unwrap(); + let role = RoleType::Validator; let network_id = NetworkId::Validator; let chain_id = ChainId::default(); let dialer_peer_id = PeerId::random(); @@ -141,8 +142,8 @@ pub fn setup_network() -> DummyNetwork { // Set up the listener network let network_context = Arc::new(NetworkContext::new( + role, network_id.clone(), - RoleType::Validator, listener_peer_id, )); let mut network_builder = NetworkBuilder::new_for_test( @@ -169,11 +170,7 @@ pub fn setup_network() -> DummyNetwork { let authentication_mode = AuthenticationMode::Mutual(dialer_identity_private_key); // Set up the dialer network - let network_context = Arc::new(NetworkContext::new( - network_id, - RoleType::Validator, - dialer_peer_id, - )); + let network_context = Arc::new(NetworkContext::new(role, network_id, dialer_peer_id)); let trusted_peers = Arc::new(RwLock::new(HashMap::new())); diff --git a/network/simple-onchain-discovery/src/lib.rs b/network/simple-onchain-discovery/src/lib.rs index 0d6dc92aec685..f6d232b5b9b78 100644 --- a/network/simple-onchain-discovery/src/lib.rs +++ b/network/simple-onchain-discovery/src/lib.rs @@ -3,7 +3,7 @@ use channel::diem_channel::{self, Receiver}; use diem_config::{ - config::{Peer, PeerRole, RoleType}, + config::{Peer, PeerRole}, network_id::NetworkContext, }; use diem_crypto::x25519::PublicKey; @@ -87,7 +87,7 @@ fn extract_updates( encryptor: &Encryptor, node_set: ValidatorSet, ) -> Vec { - let role = network_context.role(); + let is_validator = network_context.network_id().is_validator_network(); // Decode addresses while ignoring bad addresses let discovered_peers = node_set @@ -96,20 +96,19 @@ fn extract_updates( let peer_id = *info.account_address(); let config = info.into_config(); - let addrs = match role { - RoleType::Validator => { - let result = encryptor.decrypt(&config.validator_network_addresses, peer_id); - if let Err(EncryptorError::StorageError(_)) = result { - panic!(format!( - "Unable to initialize validator network addresses: {:?}", - result - )); - } - result.map_err(anyhow::Error::from) + let addrs = if is_validator { + let result = encryptor.decrypt(&config.validator_network_addresses, peer_id); + if let Err(EncryptorError::StorageError(_)) = result { + panic!(format!( + "Unable to initialize validator network addresses: {:?}", + result + )); } - RoleType::FullNode => config + result.map_err(anyhow::Error::from) + } else { + config .fullnode_network_addresses() - .map_err(anyhow::Error::from), + .map_err(anyhow::Error::from) } .map_err(|err| { inc_by_with_context(&DISCOVERY_COUNTS, &network_context, "read_failure", 1); @@ -123,9 +122,10 @@ fn extract_updates( }) .unwrap_or_default(); - let peer_role = match role { - RoleType::Validator => PeerRole::Validator, - RoleType::FullNode => PeerRole::ValidatorFullNode, + let peer_role = if is_validator { + PeerRole::Validator + } else { + PeerRole::ValidatorFullNode }; (peer_id, Peer::from_addrs(peer_role, addrs)) }) @@ -215,7 +215,7 @@ impl ConfigurationChangeListener { info!( NetworkSchema::new(&self.network_context), "Update {} Network about new Node IDs", - self.network_context.role() + self.network_context.network_id() ); for update in updates { diff --git a/network/src/counters.rs b/network/src/counters.rs index c7aa0683739f4..9df792ec65f39 100644 --- a/network/src/counters.rs +++ b/network/src/counters.rs @@ -60,7 +60,7 @@ pub static DIEM_NETWORK_PEER_CONNECTED: Lazy = Lazy::new(|| { }); pub fn peer_connected(network_context: &NetworkContext, remote_peer_id: &PeerId, v: i64) { - if network_context.role().is_validator() { + if network_context.network_id().is_validator_network() { DIEM_NETWORK_PEER_CONNECTED .with_label_values(&[ network_context.role().as_str(), diff --git a/network/src/noise/mod.rs b/network/src/noise/mod.rs index 87f27433afb2e..c66a58ca7e6ef 100644 --- a/network/src/noise/mod.rs +++ b/network/src/noise/mod.rs @@ -43,16 +43,16 @@ //! //! let client_auth = HandshakeAuthMode::mutual(trusted_peers.clone()); //! let client_context = Arc::new(NetworkContext::new( -//! NetworkId::Validator, //! RoleType::Validator, +//! NetworkId::Validator, //! client_peer_id, //! )); //! let client = NoiseUpgrader::new(client_context, client_private, client_auth); //! //! let server_auth = HandshakeAuthMode::mutual(trusted_peers); //! let server_context = Arc::new(NetworkContext::new( -//! NetworkId::Validator, //! RoleType::Validator, +//! NetworkId::Validator, //! server_peer_id, //! )); //! let server = NoiseUpgrader::new(server_context, server_private, server_auth); diff --git a/network/src/peer_manager/mod.rs b/network/src/peer_manager/mod.rs index 7b92a3bb8cdb3..981941e65c43b 100644 --- a/network/src/peer_manager/mod.rs +++ b/network/src/peer_manager/mod.rs @@ -379,10 +379,10 @@ where .filter(|(_, (metadata, _))| metadata.origin == ConnectionOrigin::Inbound) .count(); let outbound = total.saturating_sub(inbound); - let role = self.network_context.role().as_str(); + // TODO: Work with PEs to update this to be a `NetworkId` rather than `RoleType` counters::DIEM_NETWORK_PEERS - .with_label_values(&[role, "connected"]) + .with_label_values(&[self.network_context.role().as_str(), "connected"]) .set(total as i64); counters::connections(&self.network_context, ConnectionOrigin::Inbound).set(inbound as i64); diff --git a/state-sync/tests/test_harness.rs b/state-sync/tests/test_harness.rs index 363915fdabbc1..df0d26ad5d83d 100644 --- a/state-sync/tests/test_harness.rs +++ b/state-sync/tests/test_harness.rs @@ -374,8 +374,8 @@ impl StateSyncEnvironment { let peer = self.peers[index].borrow(); let auth_mode = AuthenticationMode::Mutual(peer.network_key.clone()); let network_context = Arc::new(NetworkContext::new( + *role, VALIDATOR_NETWORK.clone(), - RoleType::Validator, peer.peer_id, ));