Updated validator config according to suggestions.

- Directory structure changed slightly
 - Uses a filter_map instead of a for loop.
 - All errors reading files does not prevent others from being read.
 - The accounts manager needs to generate files first, with the same structure.

account_manager/src/main.rs

@@ -2,7 +2,7 @@ use bls::Keypair;
 use clap::{App, Arg, SubCommand};
 use slog::{debug, info, o, Drain};
 use std::path::PathBuf;
-use validator_client::config::ValidatorClientConfig;
+use validator_client::Config as ValidatorClientConfig;
 
 fn main() {
     // Logging
@@ -31,7 +31,7 @@ fn main() {
         )
         .get_matches();
 
-    let config = ValidatorClientConfig::build_config(&matches)
+    let config = ValidatorClientConfig::parse_args(&matches, &log)
         .expect("Unable to build a configuration for the account manager.");
 
     // Log configuration

validator_client/README.md

@@ -65,7 +65,6 @@ with `--datadir`.
 The configuration directory structure looks like:
 ```
 ~/.lighthouse-validator
-└── validators
     ├── 3cf4210d58ec
     │   └── private.key
     ├── 9b5d8b5be4e7

validator_client/src/config.rs

@@ -5,119 +5,129 @@ use std::fs;
 use std::fs::File;
 use std::io::{Error, ErrorKind};
 use std::path::PathBuf;
+use slog::{debug, info, error};
 use types::ChainSpec;
 
 /// Stores the core configuration for this validator instance.
 #[derive(Clone)]
-pub struct ValidatorClientConfig {
+pub struct Config {
     /// The data directory, which stores all validator databases
     pub data_dir: PathBuf,
-    /// The directory where the individual validator configuration directories are stored.
-    pub validator_dir: PathBuf,
     /// The server at which the Beacon Node can be contacted
     pub server: String,
     /// The chain specification that we are connecting to
     pub spec: ChainSpec,
 }
 
-const DEFAULT_VALIDATOR_DATADIR: &str = ".lighthouse-validator";
-const DEFAULT_VALIDATORS_SUBDIR: &str = "validators";
 const DEFAULT_PRIVATE_KEY_FILENAME: &str = "private.key";
 
-impl ValidatorClientConfig {
-    /// Build a new configuration from defaults, which are overrided by arguments provided.
-    pub fn build_config(arguments: &ArgMatches) -> Result<Self, Error> {
-        // Use the specified datadir, or default in the home directory
-        let data_dir: PathBuf = match arguments.value_of("datadir") {
-            Some(path) => PathBuf::from(path.to_string()),
-            None => {
-                let home = dirs::home_dir().ok_or_else(|| {
-                    Error::new(ErrorKind::NotFound, "Unable to determine home directory.")
-                })?;
-                home.join(DEFAULT_VALIDATOR_DATADIR)
-            }
+impl Default for Config {
+    fn default() -> Self {
+        let data_dir = {
+            let home = dirs::home_dir().expect("Unable to determine home directory.");
+            home.join(".lighthouse-validator")
         };
-        fs::create_dir_all(&data_dir)?;
+        fs::create_dir_all(&data_dir)
+            .unwrap_or_else(|_| panic!("Unable to create {:?}", &data_dir));
 
-        let validator_dir = data_dir.join(DEFAULT_VALIDATORS_SUBDIR);
-        fs::create_dir_all(&validator_dir)?;
+        let server = "localhost:50051".to_string();
 
-        let server: String = match arguments.value_of("server") {
-            Some(srv) => {
-                //TODO: I don't think this parses correctly a server & port combo
-                srv.parse::<u16>()
-                    .map_err(|e| Error::new(ErrorKind::InvalidInput, e))?
-                    .to_string()
+        let spec = ChainSpec::foundation();
+
+        Self {
+            data_dir,
+            server,
+            spec,
         }
-            None => "localhost:50051".to_string(),
+    }
+}
+
+impl Config {
+    /// Build a new configuration from defaults, which are overrided by arguments provided.
+    pub fn parse_args(args: &ArgMatches, log: &slog::Logger) -> Result<Self, Error> {
+        let mut config = Config::default();
+
+        // Use the specified datadir, or default in the home directory
+        if let Some(datadir) = args.value_of("datadir") {
+            config.data_dir = PathBuf::from(datadir);
+            fs::create_dir_all(&config.data_dir)
+                .unwrap_or_else(|_| panic!("Unable to create {:?}", &config.data_dir));
+            info!(log, "Using custom data dir: {:?}", &config.data_dir);
+        };
+
+        if let Some(srv) = args.value_of("server") {
+            //TODO: I don't think this parses correctly a server & port combo
+            config.server = srv.to_string();
+            info!(log, "Using custom server: {:?}", &config.server);
         };
 
         // TODO: Permit loading a custom spec from file.
-        let spec: ChainSpec = match arguments.value_of("spec") {
-            Some(spec_str) => {
-                match spec_str {
+        if let Some(spec_str) = args.value_of("spec") {
+            info!(log, "Using custom spec: {:?}", spec_str);
+            config.spec = match spec_str {
                 "foundation" => ChainSpec::foundation(),
                 "few_validators" => ChainSpec::few_validators(),
                 // Should be impossible due to clap's `possible_values(..)` function.
                 _ => unreachable!(),
-                }
-            }
-            None => ChainSpec::foundation(),
+            };
         };
 
-        Ok(Self {
-            data_dir,
-            validator_dir,
-            server,
-            spec,
-        })
+        Ok(config)
     }
 
     /// Try to load keys from validator_dir, returning None if none are found or an error.
-    pub fn fetch_keys(&self) -> Result<Option<Vec<Keypair>>, Error> {
-        let mut validator_dirs = fs::read_dir(&self.validator_dir)?.peekable();
+    pub fn fetch_keys(&self, log: &slog::Logger) -> Option<Vec<Keypair>> {
 
-        // There are no validator directories.
-        if validator_dirs.peek().is_none() {
-            return Ok(None);
+        let key_pairs: Vec<Keypair> = fs::read_dir(&self.data_dir)
+            .unwrap()
+            .filter_map( |validator_dir| {
+
+                let validator_dir = validator_dir.ok()?;
+
+                if !(validator_dir.file_type().ok()?.is_dir()) {
+                    // Skip non-directories (i.e. no files/symlinks)
+                    return None;
                 }
 
-        let mut key_pairs: Vec<Keypair> = Vec::new();
+                let key_filename = validator_dir.path().join(DEFAULT_PRIVATE_KEY_FILENAME);
 
-        for validator_dir_result in validator_dirs {
-            let validator_dir = validator_dir_result?;
-
-            // Try to open the key file directly
-            // TODO skip keyfiles that are not found, and log the error instead of returning it.
-            let mut key_file = File::open(validator_dir.path().join(DEFAULT_PRIVATE_KEY_FILENAME))?;
-
-            let key: Keypair = bincode::deserialize_from(&mut key_file)
-                .map_err(|e| Error::new(ErrorKind::InvalidData, e))?;
-
-            // TODO skip keyfile if it's not matched, and log the error instead of returning it.
-            let validator_directory_name =
-                validator_dir.file_name().into_string().map_err(|_| {
-                    Error::new(
-                        ErrorKind::InvalidData,
-                        "The filename cannot be parsed to a string.",
-                    )
-                })?;
-            if key.identifier() != validator_directory_name {
-                return Err(Error::new(
-                    ErrorKind::InvalidData,
-                    "The validator directory ID did not match the key found inside.",
-                ));
+                if !(key_filename.is_file()) {
+                    info!(log, "Private key is not a file: {:?}", key_filename.to_str());
+                    return None;
                 }
 
-            key_pairs.push(key);
+                debug!(log, "Deserializing private key from file: {:?}", key_filename.to_str());
+
+                let mut key_file = File::open(key_filename.clone()).ok()?;
+
+                let key: Keypair = if let Ok(key_ok) = bincode::deserialize_from(&mut key_file) {
+                        key_ok
+                    } else {
+                        error!(log, "Unable to deserialize the private key file: {:?}", key_filename);
+                        return None;
+                    };
+
+                let ki = key.identifier();
+                if ki != validator_dir.file_name().into_string().ok()? {
+                    error!(log, "The validator key ({:?}) did not match the directory filename {:?}.", ki, &validator_dir.path().to_string_lossy());
+                    return None;
+                }
+                Some(key)
+            })
+            .collect();
+
+        // Check if it's an empty vector, and return none.
+        if key_pairs.is_empty() {
+            None
+        } else {
+            Some(key_pairs)
         }
 
-        Ok(Some(key_pairs))
     }
 
     /// Saves a keypair to a file inside the appropriate validator directory. Returns the saved path filename.
     pub fn save_key(&self, key: &Keypair) -> Result<PathBuf, Error> {
-        let validator_config_path = self.validator_dir.join(key.identifier());
+        let validator_config_path = self.data_dir.join(key.identifier());
         let key_path = validator_config_path.join(DEFAULT_PRIVATE_KEY_FILENAME);
 
         fs::create_dir_all(&validator_config_path)?;

validator_client/src/lib.rs

@@ -1,3 +1,3 @@
 pub mod config;
 
-pub use crate::config::ValidatorClientConfig;
+pub use crate::config::Config;

validator_client/src/main.rs

@@ -1,14 +1,14 @@
 use self::block_producer_service::{BeaconBlockGrpcClient, BlockProducerService};
 use self::duties::{DutiesManager, DutiesManagerService, EpochDutiesMap};
-use crate::config::ValidatorClientConfig;
+use crate::config::Config;
 use block_proposer::{test_utils::LocalSigner, BlockProducer};
 use clap::{App, Arg};
 use grpcio::{ChannelBuilder, EnvBuilder};
 use protos::services_grpc::{BeaconBlockServiceClient, ValidatorServiceClient};
-use slog::{error, info, o, Drain};
+use slog::{info, o, Drain};
 use slot_clock::SystemTimeSlotClock;
 use std::sync::Arc;
-use std::{process, thread, time};
+use std::thread;
 
 mod block_producer_service;
 mod config;
@@ -52,7 +52,7 @@ fn main() {
         )
         .get_matches();
 
-    let config = ValidatorClientConfig::build_config(&matches)
+    let config = Config::parse_args(&matches, &log)
         .expect("Unable to build a configuration for the validator client.");
 
     // Log configuration
@@ -91,15 +91,8 @@ fn main() {
     let poll_interval_millis = spec.seconds_per_slot * 1000 / 10; // 10% epoch time precision.
     info!(log, "Starting block producer service"; "polls_per_epoch" => spec.seconds_per_slot * 1000 / poll_interval_millis);
 
-    let keypairs = config
-        .fetch_keys()
-        .expect("Encountered an error while fetching saved keys.")
-        .unwrap_or_else(|| {
-            error!(log, "No key pairs found in configuration, they must first be generated with: account_manager generate.");
-            // give the logger a chance to flush the error before exiting.
-            thread::sleep(time::Duration::from_millis(500));
-            process::exit(1)
-        });
+    let keypairs = config.fetch_keys(&log)
+        .expect("No key pairs found in configuration, they must first be generated with: account_manager generate.");
 
     /*
      * Start threads.