reuse beacon_node methods for initializing network configs in boot_node (#1520)

## Issue Addressed

#1378

## Proposed Changes

Boot node reuses code from beacon_node to initialize network config. This also enables using the network directory to store/load the enr and the private key.

## Additional Info

Note that before this PR the port cli arguments were off (the argument was named `enr-port` but used as `boot-node-enr-port`).
Therefore as port always the cli port argument was used (for both enr and listening). Now the enr-port argument can be used to overwrite the listening port as the public port others should connect to.

Last but not least note, that this restructuring reuses `ethlibp2p::NetworkConfig` that has many more options than the ones used in the boot node. For example the network config has an own `discv5_config` field that gets never used in the boot node and instead another `Discv5Config` gets created later in the boot node process.

Co-authored-by: Age Manning <Age@AgeManning.com>

beacon_node/eth2_libp2p/src/discovery/enr.rs

@@ -6,6 +6,7 @@ use super::enr_ext::CombinedKeyExt;
 use super::ENR_FILENAME;
 use crate::types::{Enr, EnrBitfield};
 use crate::NetworkConfig;
+use discv5::enr::EnrKey;
 use libp2p::core::identity::Keypair;
 use slog::{debug, warn};
 use ssz::{Decode, Encode};
@@ -48,6 +49,56 @@ impl Eth2Enr for Enr {
     }
 }
 
+/// Either use the given ENR or load an ENR from file if it exists and matches the current NodeId
+/// and sequence number.
+/// If an ENR exists, with the same NodeId, this function checks to see if the loaded ENR from
+/// disk is suitable to use, otherwise we increment the given ENR's sequence number.
+pub fn use_or_load_enr(
+    enr_key: &CombinedKey,
+    local_enr: &mut Enr,
+    config: &NetworkConfig,
+    log: &slog::Logger,
+) -> Result<(), String> {
+    let enr_f = config.network_dir.join(ENR_FILENAME);
+    if let Ok(mut enr_file) = File::open(enr_f.clone()) {
+        let mut enr_string = String::new();
+        match enr_file.read_to_string(&mut enr_string) {
+            Err(_) => debug!(log, "Could not read ENR from file"),
+            Ok(_) => {
+                match Enr::from_str(&enr_string) {
+                    Ok(disk_enr) => {
+                        // if the same node id, then we may need to update our sequence number
+                        if local_enr.node_id() == disk_enr.node_id() {
+                            if compare_enr(&local_enr, &disk_enr) {
+                                debug!(log, "ENR loaded from disk"; "file" => format!("{:?}", enr_f));
+                                // the stored ENR has the same configuration, use it
+                                *local_enr = disk_enr;
+                                return Ok(());
+                            }
+
+                            // same node id, different configuration - update the sequence number
+                            // Note: local_enr is generated with default(0) attnets value,
+                            // so a non default value in persisted enr will also update sequence number.
+                            let new_seq_no = disk_enr.seq().checked_add(1).ok_or_else(|| "ENR sequence number on file is too large. Remove it to generate a new NodeId")?;
+                            local_enr.set_seq(new_seq_no, enr_key).map_err(|e| {
+                                format!("Could not update ENR sequence number: {:?}", e)
+                            })?;
+                            debug!(log, "ENR sequence number increased"; "seq" =>  new_seq_no);
+                        }
+                    }
+                    Err(e) => {
+                        warn!(log, "ENR from file could not be decoded"; "error" => format!("{:?}", e));
+                    }
+                }
+            }
+        }
+    }
+
+    save_enr_to_disk(&config.network_dir, &local_enr, log);
+
+    Ok(())
+}
+
 /// Loads an ENR from file if it exists and matches the current NodeId and sequence number. If none
 /// exists, generates a new one.
 ///
@@ -65,51 +116,11 @@ pub fn build_or_load_enr<T: EthSpec>(
     let enr_key = CombinedKey::from_libp2p(&local_key)?;
     let mut local_enr = build_enr::<T>(&enr_key, config, enr_fork_id)?;
 
-    let enr_f = config.network_dir.join(ENR_FILENAME);
-    if let Ok(mut enr_file) = File::open(enr_f.clone()) {
-        let mut enr_string = String::new();
-        match enr_file.read_to_string(&mut enr_string) {
-            Err(_) => debug!(log, "Could not read ENR from file"),
-            Ok(_) => {
-                match Enr::from_str(&enr_string) {
-                    Ok(disk_enr) => {
-                        // if the same node id, then we may need to update our sequence number
-                        if local_enr.node_id() == disk_enr.node_id() {
-                            if compare_enr(&local_enr, &disk_enr) {
-                                debug!(log, "ENR loaded from disk"; "file" => format!("{:?}", enr_f));
-                                // the stored ENR has the same configuration, use it
-                                return Ok(disk_enr);
-                            }
-
-                            // same node id, different configuration - update the sequence number
-                            // Note: local_enr is generated with default(0) attnets value,
-                            // so a non default value in persisted enr will also update sequence number.
-                            let new_seq_no = disk_enr.seq().checked_add(1).ok_or_else(|| "ENR sequence number on file is too large. Remove it to generate a new NodeId")?;
-                            local_enr.set_seq(new_seq_no, &enr_key).map_err(|e| {
-                                format!("Could not update ENR sequence number: {:?}", e)
-                            })?;
-                            debug!(log, "ENR sequence number increased"; "seq" =>  new_seq_no);
-                        }
-                    }
-                    Err(e) => {
-                        warn!(log, "ENR from file could not be decoded"; "error" => format!("{:?}", e));
-                    }
-                }
-            }
-        }
-    }
-
-    save_enr_to_disk(&config.network_dir, &local_enr, log);
-
+    use_or_load_enr(&enr_key, &mut local_enr, config, log)?;
     Ok(local_enr)
 }
 
-/// Builds a lighthouse ENR given a `NetworkConfig`.
-pub fn build_enr<T: EthSpec>(
-    enr_key: &CombinedKey,
-    config: &NetworkConfig,
-    enr_fork_id: EnrForkId,
-) -> Result<Enr, String> {
+pub fn create_enr_builder_from_config<T: EnrKey>(config: &NetworkConfig) -> EnrBuilder<T> {
     let mut builder = EnrBuilder::new("v4");
     if let Some(enr_address) = config.enr_address {
         builder.ip(enr_address);
@@ -120,7 +131,17 @@ pub fn build_enr<T: EthSpec>(
     // we always give it our listening tcp port
     // TODO: Add uPnP support to map udp and tcp ports
     let tcp_port = config.enr_tcp_port.unwrap_or_else(|| config.libp2p_port);
-    builder.tcp(tcp_port);
+    builder.tcp(tcp_port).tcp(config.libp2p_port);
+    builder
+}
+
+/// Builds a lighthouse ENR given a `NetworkConfig`.
+pub fn build_enr<T: EthSpec>(
+    enr_key: &CombinedKey,
+    config: &NetworkConfig,
+    enr_fork_id: EnrForkId,
+) -> Result<Enr, String> {
+    let mut builder = create_enr_builder_from_config(config);
 
     // set the `eth2` field on our ENR
     builder.add_value(ETH2_ENR_KEY.into(), enr_fork_id.as_ssz_bytes());
@@ -131,7 +152,6 @@ pub fn build_enr<T: EthSpec>(
     builder.add_value(BITFIELD_ENR_KEY.into(), bitfield.as_ssz_bytes());
 
     builder
-        .tcp(config.libp2p_port)
         .build(enr_key)
         .map_err(|e| format!("Could not build Local ENR: {:?}", e))
 }

beacon_node/eth2_libp2p/src/discovery/mod.rs

@@ -3,7 +3,7 @@ pub(crate) mod enr;
 pub mod enr_ext;
 
 // Allow external use of the lighthouse ENR builder
-pub use enr::{build_enr, CombinedKey, Eth2Enr};
+pub use enr::{build_enr, create_enr_builder_from_config, use_or_load_enr, CombinedKey, Eth2Enr};
 pub use enr_ext::{CombinedKeyExt, EnrExt};
 pub use libp2p::core::identity::Keypair;
 

beacon_node/eth2_libp2p/src/lib.rs

@@ -26,4 +26,4 @@ pub use metrics::scrape_discovery_metrics;
 pub use peer_manager::{
     client::Client, score::PeerAction, PeerDB, PeerInfo, PeerSyncStatus, SyncInfo,
 };
-pub use service::{Libp2pEvent, Service, NETWORK_KEY_FILENAME};
+pub use service::{load_private_key, Libp2pEvent, Service, NETWORK_KEY_FILENAME};

beacon_node/eth2_libp2p/src/service.rs

@@ -357,7 +357,7 @@ fn keypair_from_bytes(mut bytes: Vec<u8>) -> error::Result<Keypair> {
 /// generated and is then saved to disk.
 ///
 /// Currently only secp256k1 keys are allowed, as these are the only keys supported by discv5.
-fn load_private_key(config: &NetworkConfig, log: &slog::Logger) -> Keypair {
+pub fn load_private_key(config: &NetworkConfig, log: &slog::Logger) -> Keypair {
     // check for key from disk
     let network_key_f = config.network_dir.join(NETWORK_KEY_FILENAME);
     if let Ok(mut network_key_file) = File::open(network_key_f.clone()) {

beacon_node/src/config.rs

@@ -2,7 +2,7 @@ use beacon_chain::builder::PUBKEY_CACHE_FILENAME;
 use clap::ArgMatches;
 use clap_utils::BAD_TESTNET_DIR_MESSAGE;
 use client::{config::DEFAULT_DATADIR, ClientConfig, ClientGenesis};
-use eth2_libp2p::{multiaddr::Protocol, Enr, Multiaddr};
+use eth2_libp2p::{multiaddr::Protocol, Enr, Multiaddr, NetworkConfig};
 use eth2_testnet_config::Eth2TestnetConfig;
 use slog::{crit, info, Logger};
 use ssz::Encode;
@@ -75,148 +75,13 @@ pub fn get_config<E: EthSpec>(
     /*
      * Networking
      */
-    // If a network dir has been specified, override the `datadir` definition.
-    if let Some(dir) = cli_args.value_of("network-dir") {
-        client_config.network.network_dir = PathBuf::from(dir);
-    } else {
-        client_config.network.network_dir = client_config.data_dir.join(NETWORK_DIR);
-    };
-
-    if let Some(listen_address_str) = cli_args.value_of("listen-address") {
-        let listen_address = listen_address_str
-            .parse()
-            .map_err(|_| format!("Invalid listen address: {:?}", listen_address_str))?;
-        client_config.network.listen_address = listen_address;
-    }
-
-    if let Some(target_peers_str) = cli_args.value_of("target-peers") {
-        client_config.network.target_peers = target_peers_str
-            .parse::<usize>()
-            .map_err(|_| format!("Invalid number of target peers: {}", target_peers_str))?;
-    }
-
-    if let Some(port_str) = cli_args.value_of("port") {
-        let port = port_str
-            .parse::<u16>()
-            .map_err(|_| format!("Invalid port: {}", port_str))?;
-        client_config.network.libp2p_port = port;
-        client_config.network.discovery_port = port;
-    }
-
-    if let Some(port_str) = cli_args.value_of("discovery-port") {
-        let port = port_str
-            .parse::<u16>()
-            .map_err(|_| format!("Invalid port: {}", port_str))?;
-        client_config.network.discovery_port = port;
-    }
-
-    if let Some(boot_enr_str) = cli_args.value_of("boot-nodes") {
-        let mut enrs: Vec<Enr> = vec![];
-        let mut multiaddrs: Vec<Multiaddr> = vec![];
-        for addr in boot_enr_str.split(',') {
-            match addr.parse() {
-                Ok(enr) => enrs.push(enr),
-                Err(_) => {
-                    // parsing as ENR failed, try as Multiaddr
-                    let multi: Multiaddr = addr
-                        .parse()
-                        .map_err(|_| format!("Not valid as ENR nor Multiaddr: {}", addr))?;
-                    if !multi.iter().any(|proto| matches!(proto, Protocol::Udp(_))) {
-                        slog::error!(log, "Missing UDP in Multiaddr {}", multi.to_string());
-                    }
-                    if !multi.iter().any(|proto| matches!(proto, Protocol::P2p(_))) {
-                        slog::error!(log, "Missing P2P in Multiaddr {}", multi.to_string());
-                    }
-                    multiaddrs.push(multi);
-                }
-            }
-        }
-        client_config.network.boot_nodes_enr = enrs;
-        client_config.network.boot_nodes_multiaddr = multiaddrs;
-    }
-
-    if let Some(libp2p_addresses_str) = cli_args.value_of("libp2p-addresses") {
-        client_config.network.libp2p_nodes = libp2p_addresses_str
-            .split(',')
-            .map(|multiaddr| {
-                multiaddr
-                    .parse()
-                    .map_err(|_| format!("Invalid Multiaddr: {}", multiaddr))
-            })
-            .collect::<Result<Vec<Multiaddr>, _>>()?;
-    }
-
-    if let Some(enr_udp_port_str) = cli_args.value_of("enr-udp-port") {
-        client_config.network.enr_udp_port = Some(
-            enr_udp_port_str
-                .parse::<u16>()
-                .map_err(|_| format!("Invalid discovery port: {}", enr_udp_port_str))?,
-        );
-    }
-
-    if let Some(enr_tcp_port_str) = cli_args.value_of("enr-tcp-port") {
-        client_config.network.enr_tcp_port = Some(
-            enr_tcp_port_str
-                .parse::<u16>()
-                .map_err(|_| format!("Invalid ENR TCP port: {}", enr_tcp_port_str))?,
-        );
-    }
-
-    if cli_args.is_present("enr-match") {
-        // set the enr address to localhost if the address is 0.0.0.0
-        if client_config.network.listen_address
-            == "0.0.0.0".parse::<IpAddr>().expect("valid ip addr")
-        {
-            client_config.network.enr_address =
-                Some("127.0.0.1".parse::<IpAddr>().expect("valid ip addr"));
-        } else {
-            client_config.network.enr_address = Some(client_config.network.listen_address);
-        }
-        client_config.network.enr_udp_port = Some(client_config.network.discovery_port);
-    }
-
-    if let Some(enr_address) = cli_args.value_of("enr-address") {
-        let resolved_addr = match enr_address.parse::<IpAddr>() {
-            Ok(addr) => addr, // // Input is an IpAddr
-            Err(_) => {
-                let mut addr = enr_address.to_string();
-                // Appending enr-port to the dns hostname to appease `to_socket_addrs()` parsing.
-                // Since enr-update is disabled with a dns address, not setting the enr-udp-port
-                // will make the node undiscoverable.
-                if let Some(enr_udp_port) = client_config.network.enr_udp_port {
-                    addr.push_str(&format!(":{}", enr_udp_port.to_string()));
-                } else {
-                    return Err(
-                        "enr-udp-port must be set for node to be discoverable with dns address"
-                            .into(),
-                    );
-                }
-                // `to_socket_addr()` does the dns resolution
-                // Note: `to_socket_addrs()` is a blocking call
-                let resolved_addr = if let Ok(mut resolved_addrs) = addr.to_socket_addrs() {
-                    // Pick the first ip from the list of resolved addresses
-                    resolved_addrs
-                        .next()
-                        .map(|a| a.ip())
-                        .ok_or_else(|| "Resolved dns addr contains no entries".to_string())?
-                } else {
-                    return Err(format!("Failed to parse enr-address: {}", enr_address));
-                };
-                client_config.network.discv5_config.enr_update = false;
-                resolved_addr
-            }
-        };
-        client_config.network.enr_address = Some(resolved_addr);
-    }
-
-    if cli_args.is_present("disable_enr_auto_update") {
-        client_config.network.discv5_config.enr_update = false;
-    }
-
-    if cli_args.is_present("disable-discovery") {
-        client_config.network.disable_discovery = true;
-        slog::warn!(log, "Discovery is disabled. New peers will not be found");
-    }
+    set_network_config(
+        &mut client_config.network,
+        cli_args,
+        &client_config.data_dir,
+        &log,
+        false,
+    )?;
 
     /*
      * Http server
@@ -399,6 +264,163 @@ pub fn get_config<E: EthSpec>(
     Ok(client_config)
 }
 
+/// Sets the network config from the command line arguments
+pub fn set_network_config(
+    config: &mut NetworkConfig,
+    cli_args: &ArgMatches,
+    data_dir: &PathBuf,
+    log: &Logger,
+    use_listening_port_as_enr_port_by_default: bool,
+) -> Result<(), String> {
+    // If a network dir has been specified, override the `datadir` definition.
+    if let Some(dir) = cli_args.value_of("network-dir") {
+        config.network_dir = PathBuf::from(dir);
+    } else {
+        config.network_dir = data_dir.join(NETWORK_DIR);
+    };
+
+    if let Some(listen_address_str) = cli_args.value_of("listen-address") {
+        let listen_address = listen_address_str
+            .parse()
+            .map_err(|_| format!("Invalid listen address: {:?}", listen_address_str))?;
+        config.listen_address = listen_address;
+    }
+
+    if let Some(target_peers_str) = cli_args.value_of("target-peers") {
+        config.target_peers = target_peers_str
+            .parse::<usize>()
+            .map_err(|_| format!("Invalid number of target peers: {}", target_peers_str))?;
+    }
+
+    if let Some(port_str) = cli_args.value_of("port") {
+        let port = port_str
+            .parse::<u16>()
+            .map_err(|_| format!("Invalid port: {}", port_str))?;
+        config.libp2p_port = port;
+        config.discovery_port = port;
+    }
+
+    if let Some(port_str) = cli_args.value_of("discovery-port") {
+        let port = port_str
+            .parse::<u16>()
+            .map_err(|_| format!("Invalid port: {}", port_str))?;
+        config.discovery_port = port;
+    }
+
+    if let Some(boot_enr_str) = cli_args.value_of("boot-nodes") {
+        let mut enrs: Vec<Enr> = vec![];
+        let mut multiaddrs: Vec<Multiaddr> = vec![];
+        for addr in boot_enr_str.split(',') {
+            match addr.parse() {
+                Ok(enr) => enrs.push(enr),
+                Err(_) => {
+                    // parsing as ENR failed, try as Multiaddr
+                    let multi: Multiaddr = addr
+                        .parse()
+                        .map_err(|_| format!("Not valid as ENR nor Multiaddr: {}", addr))?;
+                    if !multi.iter().any(|proto| matches!(proto, Protocol::Udp(_))) {
+                        slog::error!(log, "Missing UDP in Multiaddr {}", multi.to_string());
+                    }
+                    if !multi.iter().any(|proto| matches!(proto, Protocol::P2p(_))) {
+                        slog::error!(log, "Missing P2P in Multiaddr {}", multi.to_string());
+                    }
+                    multiaddrs.push(multi);
+                }
+            }
+        }
+        config.boot_nodes_enr = enrs;
+        config.boot_nodes_multiaddr = multiaddrs;
+    }
+
+    if let Some(libp2p_addresses_str) = cli_args.value_of("libp2p-addresses") {
+        config.libp2p_nodes = libp2p_addresses_str
+            .split(',')
+            .map(|multiaddr| {
+                multiaddr
+                    .parse()
+                    .map_err(|_| format!("Invalid Multiaddr: {}", multiaddr))
+            })
+            .collect::<Result<Vec<Multiaddr>, _>>()?;
+    }
+
+    if let Some(enr_udp_port_str) = cli_args.value_of("enr-udp-port") {
+        config.enr_udp_port = Some(
+            enr_udp_port_str
+                .parse::<u16>()
+                .map_err(|_| format!("Invalid discovery port: {}", enr_udp_port_str))?,
+        );
+    }
+
+    if let Some(enr_tcp_port_str) = cli_args.value_of("enr-tcp-port") {
+        config.enr_tcp_port = Some(
+            enr_tcp_port_str
+                .parse::<u16>()
+                .map_err(|_| format!("Invalid ENR TCP port: {}", enr_tcp_port_str))?,
+        );
+    }
+
+    if cli_args.is_present("enr-match") {
+        // set the enr address to localhost if the address is 0.0.0.0
+        if config.listen_address == "0.0.0.0".parse::<IpAddr>().expect("valid ip addr") {
+            config.enr_address = Some("127.0.0.1".parse::<IpAddr>().expect("valid ip addr"));
+        } else {
+            config.enr_address = Some(config.listen_address);
+        }
+        config.enr_udp_port = Some(config.discovery_port);
+    }
+
+    if let Some(enr_address) = cli_args.value_of("enr-address") {
+        let resolved_addr = match enr_address.parse::<IpAddr>() {
+            Ok(addr) => addr, // // Input is an IpAddr
+            Err(_) => {
+                let mut addr = enr_address.to_string();
+                // Appending enr-port to the dns hostname to appease `to_socket_addrs()` parsing.
+                // Since enr-update is disabled with a dns address, not setting the enr-udp-port
+                // will make the node undiscoverable.
+                if let Some(enr_udp_port) = config.enr_udp_port.or_else(|| {
+                    if use_listening_port_as_enr_port_by_default {
+                        Some(config.discovery_port)
+                    } else {
+                        None
+                    }
+                }) {
+                    addr.push_str(&format!(":{}", enr_udp_port.to_string()));
+                } else {
+                    return Err(
+                        "enr-udp-port must be set for node to be discoverable with dns address"
+                            .into(),
+                    );
+                }
+                // `to_socket_addr()` does the dns resolution
+                // Note: `to_socket_addrs()` is a blocking call
+                let resolved_addr = if let Ok(mut resolved_addrs) = addr.to_socket_addrs() {
+                    // Pick the first ip from the list of resolved addresses
+                    resolved_addrs
+                        .next()
+                        .map(|a| a.ip())
+                        .ok_or_else(|| "Resolved dns addr contains no entries".to_string())?
+                } else {
+                    return Err(format!("Failed to parse enr-address: {}", enr_address));
+                };
+                config.discv5_config.enr_update = false;
+                resolved_addr
+            }
+        };
+        config.enr_address = Some(resolved_addr);
+    }
+
+    if cli_args.is_present("disable_enr_auto_update") {
+        config.discv5_config.enr_update = false;
+    }
+
+    if cli_args.is_present("disable-discovery") {
+        config.disable_discovery = true;
+        slog::warn!(log, "Discovery is disabled. New peers will not be found");
+    }
+
+    Ok(())
+}
+
 /// Gets the datadir which should be used.
 pub fn get_data_dir(cli_args: &ArgMatches) -> PathBuf {
     // Read the `--datadir` flag.

beacon_node/src/lib.rs

@@ -7,7 +7,7 @@ mod config;
 pub use beacon_chain;
 pub use cli::cli_app;
 pub use client::{Client, ClientBuilder, ClientConfig, ClientGenesis};
-pub use config::{get_data_dir, get_eth2_testnet_config};
+pub use config::{get_data_dir, get_eth2_testnet_config, set_network_config};
 pub use eth2_config::Eth2Config;
 
 use beacon_chain::events::TeeEventHandler;