Userland clean up (#1277)

* Improve logging, remove unused CLI and move discovery

* Correct tests

* Handle flag correctly

beacon_node/eth2_libp2p/src/config.rs

@@ -39,12 +39,6 @@ pub struct Config {
     /// Target number of connected peers.
     pub max_peers: usize,
 
-    /// A secp256k1 secret key, as bytes in ASCII-encoded hex.
-    ///
-    /// With or without `0x` prefix.
-    #[serde(skip)]
-    pub secret_key_hex: Option<String>,
-
     /// Gossipsub configuration parameters.
     #[serde(skip)]
     pub gs_config: GossipsubConfig,
@@ -64,11 +58,6 @@ pub struct Config {
 
     /// List of extra topics to initially subscribe to as strings.
     pub topics: Vec<GossipKind>,
-
-    /// Introduces randomization in network propagation of messages. This should only be set for
-    /// testing purposes and will likely be removed in future versions.
-    // TODO: Remove this functionality for mainnet
-    pub propagation_percentage: Option<u8>,
 }
 
 impl Default for Config {
@@ -131,14 +120,12 @@ impl Default for Config {
             enr_udp_port: None,
             enr_tcp_port: None,
             max_peers: 50,
-            secret_key_hex: None,
             gs_config,
             discv5_config,
             boot_nodes: vec![],
             libp2p_nodes: vec![],
             client_version: version::version(),
             topics,
-            propagation_percentage: None,
         }
     }
 }

beacon_node/eth2_libp2p/src/discovery/enr_ext.rs

@@ -15,6 +15,9 @@ pub trait EnrExt {
 
     /// Returns the multiaddr with the `PeerId` prepended.
     fn multiaddr_p2p(&self) -> Vec<Multiaddr>;
+
+    /// Returns any multiaddrs that contain the TCP protocol.
+    fn multiaddr_tcp(&self) -> Vec<Multiaddr>;
 }
 
 /// Extend ENR CombinedPublicKey for libp2p types.
@@ -107,6 +110,27 @@ impl EnrExt for Enr {
         }
         multiaddrs
     }
+
+    /// Returns a list of multiaddrs if the ENR has an `ip` and either a `tcp` or `udp` key **or** an `ip6` and either a `tcp6` or `udp6`.
+    /// The vector remains empty if these fields are not defined.
+    fn multiaddr_tcp(&self) -> Vec<Multiaddr> {
+        let mut multiaddrs: Vec<Multiaddr> = Vec::new();
+        if let Some(ip) = self.ip() {
+            if let Some(tcp) = self.tcp() {
+                let mut multiaddr: Multiaddr = ip.into();
+                multiaddr.push(Protocol::Tcp(tcp));
+                multiaddrs.push(multiaddr);
+            }
+        }
+        if let Some(ip6) = self.ip6() {
+            if let Some(tcp6) = self.tcp6() {
+                let mut multiaddr: Multiaddr = ip6.into();
+                multiaddr.push(Protocol::Tcp(tcp6));
+                multiaddrs.push(multiaddr);
+            }
+        }
+        multiaddrs
+    }
 }
 
 impl CombinedKeyPublicExt for CombinedPublicKey {

beacon_node/eth2_libp2p/src/discovery/mod.rs

@@ -188,7 +188,7 @@ impl<TSpec: EthSpec> Discovery<TSpec> {
                 "tcp" => format!("{:?}", bootnode_enr.tcp())
             );
             let _ = discv5.add_enr(bootnode_enr).map_err(|e| {
-                warn!(
+                debug!(
                     log,
                     "Could not add peer to the local routing table";
                     "error" => format!("{}", e)

beacon_node/eth2_libp2p/src/lib.rs

@@ -7,6 +7,7 @@ extern crate lazy_static;
 
 pub mod behaviour;
 mod config;
+pub mod discovery;
 mod metrics;
 mod peer_manager;
 pub mod rpc;
@@ -16,15 +17,11 @@ pub mod types;
 pub use crate::types::{error, Enr, GossipTopic, NetworkGlobals, PubsubMessage};
 pub use behaviour::{BehaviourEvent, PeerRequestId, Request, Response};
 pub use config::Config as NetworkConfig;
+pub use discovery::{CombinedKeyExt, EnrExt, Eth2Enr};
 pub use discv5;
 pub use libp2p::gossipsub::{MessageId, Topic, TopicHash};
 pub use libp2p::{core::ConnectedPoint, PeerId, Swarm};
 pub use libp2p::{multiaddr, Multiaddr};
 pub use metrics::scrape_discovery_metrics;
-pub use peer_manager::discovery;
+pub use peer_manager::{client::Client, PeerDB, PeerInfo, PeerSyncStatus, SyncInfo};
-pub use peer_manager::{
-    client::Client,
-    discovery::{CombinedKeyExt, EnrExt, Eth2Enr},
-    PeerDB, PeerInfo, PeerSyncStatus, SyncInfo,
-};
 pub use service::{Libp2pEvent, Service, NETWORK_KEY_FILENAME};

beacon_node/eth2_libp2p/src/peer_manager/mod.rs

@@ -1,6 +1,7 @@
 //! Implementation of a Lighthouse's peer management system.
 
 pub use self::peerdb::*;
+use crate::discovery::{Discovery, DiscoveryEvent};
 use crate::rpc::{MetaData, Protocol, RPCError, RPCResponseErrorCode};
 use crate::{error, metrics};
 use crate::{Enr, EnrExt, NetworkConfig, NetworkGlobals, PeerId};
@@ -23,13 +24,10 @@ use types::{EthSpec, SubnetId};
 pub use libp2p::core::{identity::Keypair, Multiaddr};
 
 pub mod client;
-pub mod discovery;
 mod peer_info;
 mod peer_sync_status;
 mod peerdb;
 
-use discovery::{Discovery, DiscoveryEvent};
-
 pub use peer_info::{PeerConnectionStatus::*, PeerInfo};
 pub use peer_sync_status::{PeerSyncStatus, SyncInfo};
 /// The minimum reputation before a peer is disconnected.

beacon_node/eth2_libp2p/src/service.rs

@@ -94,11 +94,7 @@ impl<TSpec: EthSpec> Service<TSpec> {
         trace!(log, "Libp2p Service starting");
 
         // initialise the node's ID
-        let local_keypair = if let Some(hex_bytes) = &config.secret_key_hex {
+        let local_keypair = load_private_key(config, &log);
-            keypair_from_hex(hex_bytes)?
-        } else {
-            load_private_key(config, &log)
-        };
 
         // Create an ENR or load from disk if appropriate
         let enr =
@@ -344,7 +340,6 @@ impl<TSpec: EthSpec> Service<TSpec> {
                             debug!(self.log, "Listener error"; "error" => format!("{:?}", error.to_string()))
                         }
                         SwarmEvent::Dialing(peer_id) => {
-                            debug!(self.log, "Dialing peer"; "peer" => peer_id.to_string());
                             self.swarm.peer_manager().dialing_peer(&peer_id);
                         }
                     }
@@ -425,6 +420,8 @@ fn build_transport(
     Ok(transport)
 }
 
+// Useful helper functions for debugging. Currently not used in the client.
+#[allow(dead_code)]
 fn keypair_from_hex(hex_bytes: &str) -> error::Result<Keypair> {
     let hex_bytes = if hex_bytes.starts_with("0x") {
         hex_bytes[2..].to_string()
@@ -437,6 +434,7 @@ fn keypair_from_hex(hex_bytes: &str) -> error::Result<Keypair> {
         .and_then(keypair_from_bytes)
 }
 
+#[allow(dead_code)]
 fn keypair_from_bytes(mut bytes: Vec<u8>) -> error::Result<Keypair> {
     libp2p::core::identity::secp256k1::SecretKey::from_bytes(&mut bytes)
         .map(|secret| {

beacon_node/eth2_libp2p/tests/common/mod.rs

@@ -71,11 +71,7 @@ pub fn unused_port(transport: &str) -> Result<u16, String> {
     Ok(local_addr.port())
 }
 
-pub fn build_config(
+pub fn build_config(port: u16, mut boot_nodes: Vec<Enr>) -> NetworkConfig {
-    port: u16,
-    mut boot_nodes: Vec<Enr>,
-    secret_key: Option<String>,
-) -> NetworkConfig {
     let mut config = NetworkConfig::default();
     let path = TempDir::new(&format!("libp2p_test{}", port)).unwrap();
 
@@ -85,7 +81,6 @@ pub fn build_config(
     config.enr_udp_port = Some(port);
     config.enr_address = Some("127.0.0.1".parse().unwrap());
     config.boot_nodes.append(&mut boot_nodes);
-    config.secret_key_hex = secret_key;
     config.network_dir = path.into_path();
     // Reduce gossipsub heartbeat parameters
     config.gs_config.heartbeat_initial_delay = Duration::from_millis(500);
@@ -93,13 +88,9 @@ pub fn build_config(
     config
 }
 
-pub fn build_libp2p_instance(
+pub fn build_libp2p_instance(boot_nodes: Vec<Enr>, log: slog::Logger) -> Libp2pInstance {
-    boot_nodes: Vec<Enr>,
-    secret_key: Option<String>,
-    log: slog::Logger,
-) -> Libp2pInstance {
     let port = unused_port("tcp").unwrap();
-    let config = build_config(port, boot_nodes, secret_key);
+    let config = build_config(port, boot_nodes);
     // launch libp2p service
 
     let (signal, exit) = exit_future::signal();
@@ -123,7 +114,7 @@ pub fn get_enr(node: &LibP2PService<E>) -> Enr {
 #[allow(dead_code)]
 pub fn build_full_mesh(log: slog::Logger, n: usize) -> Vec<Libp2pInstance> {
     let mut nodes: Vec<_> = (0..n)
-        .map(|_| build_libp2p_instance(vec![], None, log.clone()))
+        .map(|_| build_libp2p_instance(vec![], log.clone()))
         .collect();
     let multiaddrs: Vec<Multiaddr> = nodes
         .iter()
@@ -150,8 +141,8 @@ pub async fn build_node_pair(log: &slog::Logger) -> (Libp2pInstance, Libp2pInsta
     let sender_log = log.new(o!("who" => "sender"));
     let receiver_log = log.new(o!("who" => "receiver"));
 
-    let mut sender = build_libp2p_instance(vec![], None, sender_log);
+    let mut sender = build_libp2p_instance(vec![], sender_log);
-    let mut receiver = build_libp2p_instance(vec![], None, receiver_log);
+    let mut receiver = build_libp2p_instance(vec![], receiver_log);
 
     let receiver_multiaddr = receiver.swarm.local_enr().multiaddr()[1].clone();
 
@@ -192,7 +183,7 @@ pub async fn build_node_pair(log: &slog::Logger) -> (Libp2pInstance, Libp2pInsta
 #[allow(dead_code)]
 pub fn build_linear(log: slog::Logger, n: usize) -> Vec<Libp2pInstance> {
     let mut nodes: Vec<_> = (0..n)
-        .map(|_| build_libp2p_instance(vec![], None, log.clone()))
+        .map(|_| build_libp2p_instance(vec![], log.clone()))
         .collect();
     let multiaddrs: Vec<Multiaddr> = nodes
         .iter()

beacon_node/eth2_libp2p/tests/noise.rs

@@ -135,7 +135,7 @@ async fn test_secio_noise_fallback() {
     let log = common::build_log(log_level, enable_logging);
 
     let port = common::unused_port("tcp").unwrap();
-    let noisy_config = common::build_config(port, vec![], None);
+    let noisy_config = common::build_config(port, vec![]);
     let (_signal, exit) = exit_future::signal();
     let executor =
         environment::TaskExecutor::new(tokio::runtime::Handle::current(), exit, log.clone());
@@ -144,7 +144,7 @@ async fn test_secio_noise_fallback() {
         .1;
 
     let port = common::unused_port("tcp").unwrap();
-    let secio_config = common::build_config(port, vec![common::get_enr(&noisy_node)], None);
+    let secio_config = common::build_config(port, vec![common::get_enr(&noisy_node)]);
 
     // Building a custom Libp2pService from outside the crate isn't possible because of
     // private fields in the Libp2pService struct. A swarm is good enough for testing

beacon_node/network/src/service.rs

@@ -48,8 +48,6 @@ pub struct NetworkService<T: BeaconChainTypes> {
     next_fork_update: Option<Delay>,
     /// The logger for the network service.
     log: slog::Logger,
-    /// A probability of propagation.
-    propagation_percentage: Option<u8>,
 }
 
 impl<T: BeaconChainTypes> NetworkService<T> {
@@ -67,8 +65,6 @@ impl<T: BeaconChainTypes> NetworkService<T> {
         // get a reference to the beacon chain store
         let store = beacon_chain.store.clone();
 
-        let propagation_percentage = config.propagation_percentage;
-
         // build the current enr_fork_id for adding to our local ENR
         let enr_fork_id = beacon_chain.enr_fork_id();
 
@@ -79,8 +75,14 @@ impl<T: BeaconChainTypes> NetworkService<T> {
         let (network_globals, mut libp2p) =
             LibP2PService::new(executor.clone(), config, enr_fork_id, &network_log)?;
 
-        for enr in load_dht::<T::EthSpec, T::HotStore, T::ColdStore>(store.clone()) {
+        // Repopulate the DHT with stored ENR's.
-            libp2p.swarm.add_enr(enr);
+        let enrs_to_load = load_dht::<T::EthSpec, T::HotStore, T::ColdStore>(store.clone());
+        debug!(
+            network_log,
+            "Loading peers into the routing table"; "peers" => enrs_to_load.len()
+        );
+        for enr in enrs_to_load {
+            libp2p.swarm.add_enr(enr.clone());
         }
 
         // launch derived network services
@@ -110,7 +112,6 @@ impl<T: BeaconChainTypes> NetworkService<T> {
             network_globals: network_globals.clone(),
             next_fork_update,
             log: network_log,
-            propagation_percentage,
         };
 
         spawn_service(executor, network_service)?;
@@ -174,20 +175,6 @@ fn spawn_service<T: BeaconChainTypes>(
                             propagation_source,
                             message_id,
                         } => {
-                            // TODO: Remove this for mainnet
-                            // randomly prevents propagation
-                            let mut should_send = true;
-                            if let Some(percentage) = service.propagation_percentage {
-                                // not exact percentage but close enough
-                                let rand = rand::random::<u8>() % 100;
-                                if rand > percentage {
-                                    // don't propagate
-                                    should_send = false;
-                                }
-                            }
-                            if !should_send {
-                                info!(service.log, "Random filter did not propagate message");
-                            } else {
                                 trace!(service.log, "Propagating gossipsub message";
                                     "propagation_peer" => format!("{:?}", propagation_source),
                                     "message_id" => message_id.to_string(),
@@ -197,22 +184,7 @@ fn spawn_service<T: BeaconChainTypes>(
                                     .swarm
                                     .propagate_message(&propagation_source, message_id);
                         }
-                        }
                         NetworkMessage::Publish { messages } => {
-                            // TODO: Remove this for mainnet
-                            // randomly prevents propagation
-                            let mut should_send = true;
-                            if let Some(percentage) = service.propagation_percentage {
-                                // not exact percentage but close enough
-                                let rand = rand::random::<u8>() % 100;
-                                if rand > percentage {
-                                    // don't propagate
-                                    should_send = false;
-                                }
-                            }
-                            if !should_send {
-                                info!(service.log, "Random filter did not publish messages");
-                            } else {
                                 let mut topic_kinds = Vec::new();
                                 for message in &messages {
                                     if !topic_kinds.contains(&message.kind()) {
@@ -228,7 +200,6 @@ fn spawn_service<T: BeaconChainTypes>(
                                 expose_publish_metrics(&messages);
                                 service.libp2p.swarm.publish(messages);
                         }
-                        }
                         NetworkMessage::Disconnect { peer_id } => {
                             service.libp2p.disconnect_and_ban_peer(
                                 peer_id,

beacon_node/src/cli.rs

@@ -61,8 +61,8 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
                 .takes_value(true),
         )
         .arg(
-            Arg::with_name("maxpeers")
+            Arg::with_name("max_peers")
-                .long("maxpeers")
+                .long("max-peers")
                 .help("The maximum number of peers.")
                 .default_value("50")
                 .takes_value(true),
@@ -125,24 +125,6 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
                        without an ENR.")
                 .takes_value(true),
         )
-        .arg(
-            Arg::with_name("p2p-priv-key")
-                .long("p2p-priv-key")
-                .value_name("HEX")
-                .help("A secp256k1 secret key, represented as ASCII-encoded hex bytes (with or \
-                       without 0x prefix). Default is either loaded from disk or generated \
-                       automatically.")
-                .takes_value(true),
-        )
-        .arg(
-            Arg::with_name("random-propagation")
-                .long("random-propagation")
-                .value_name("INTEGER")
-                .takes_value(true)
-                .help("Specifies (as a percentage) the likelihood of propagating blocks and \
-                       attestations. This should only be used for testing networking elements. The \
-                       value must like in the range 1-100. Default is 100.")
-        )
         /* REST API related arguments */
         .arg(
             Arg::with_name("http")

beacon_node/src/config.rs

@@ -102,7 +102,7 @@ pub fn get_config<E: EthSpec>(
         client_config.network.listen_address = listen_address;
     }
 
-    if let Some(max_peers_str) = cli_args.value_of("maxpeers") {
+    if let Some(max_peers_str) = cli_args.value_of("max-peers") {
         client_config.network.max_peers = max_peers_str
             .parse::<usize>()
             .map_err(|_| format!("Invalid number of max peers: {}", max_peers_str))?;
@@ -208,24 +208,6 @@ pub fn get_config<E: EthSpec>(
         client_config.network.discv5_config.enr_update = false;
     }
 
-    if let Some(p2p_priv_key) = cli_args.value_of("p2p-priv-key") {
-        client_config.network.secret_key_hex = Some(p2p_priv_key.to_string());
-    }
-
-    // Define a percentage of messages that should be propogated, useful for simulating bad network
-    // conditions.
-    //
-    // WARNING: setting this to anything less than 100 will cause bad behaviour.
-    if let Some(propagation_percentage_string) = cli_args.value_of("random-propagation") {
-        let percentage = propagation_percentage_string
-            .parse::<u8>()
-            .map_err(|_| "Unable to parse the propagation percentage".to_string())?;
-        if percentage > 100 {
-            return Err("Propagation percentage greater than 100".to_string());
-        }
-        client_config.network.propagation_percentage = Some(percentage);
-    }
-
     /*
      * Http server
      */