Use OS file locks in validator client (#1958)

## Issue Addressed

Closes #1823

## Proposed Changes

* Use OS-level file locking for validator keystores, eliminating problems with lockfiles lingering after ungraceful shutdowns (`SIGKILL`, power outage). I'm using the `fs2` crate because it's cross-platform (unlike `file-lock`), and it seems to have the most downloads on crates.io.
* Deprecate + disable `--delete-lockfiles` CLI param, it's no longer necessary
* Delete the `validator_dir::Manager`, as it was mostly dead code and was only used in the `validator list` command, which has been rewritten to read the validator definitions YAML instead.

## Additional Info

Tested on:

- [x] Linux
- [x] macOS
- [x] Docker Linux
- [x] Docker macOS
- [ ] Windows

.dockerignore

@@ -1,4 +1,4 @@
-tests/ef_tests/eth2.0-spec-tests
+testing/ef_tests/eth2.0-spec-tests
 target/
 *.data
 *.tar.gz

Cargo.lock

@@ -2074,6 +2074,7 @@ version = "0.1.0"
 dependencies = [
  "eth2_keystore",
  "eth2_wallet",
+ "lockfile",
  "tempfile",
 ]
 
@@ -2273,6 +2274,16 @@ dependencies = [
  "percent-encoding 2.1.0",
 ]
 
+[[package]]
+name = "fs2"
+version = "0.4.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "9564fc758e15025b46aa6643b1b77d047d1a56a1aea6e01002ac0c7026876213"
+dependencies = [
+ "libc",
+ "winapi 0.3.9",
+]
+
 [[package]]
 name = "fuchsia-cprng"
 version = "0.1.1"
@@ -3791,6 +3802,14 @@ dependencies = [
  "scopeguard",
 ]
 
+[[package]]
+name = "lockfile"
+version = "0.1.0"
+dependencies = [
+ "fs2",
+ "tempdir",
+]
+
 [[package]]
 name = "log"
 version = "0.3.9"
@@ -7387,6 +7406,7 @@ dependencies = [
  "libsecp256k1",
  "lighthouse_metrics",
  "lighthouse_version",
+ "lockfile",
  "logging",
  "parking_lot 0.11.1",
  "rand 0.7.3",
@@ -7419,8 +7439,10 @@ version = "0.1.0"
 dependencies = [
  "bls",
  "deposit_contract",
+ "derivative",
  "eth2_keystore",
  "hex",
+ "lockfile",
  "rand 0.7.3",
  "rayon",
  "slog",

Cargo.toml

@@ -30,6 +30,7 @@ members = [
     "common/hashset_delay",
     "common/lighthouse_metrics",
     "common/lighthouse_version",
+    "common/lockfile",
     "common/logging",
     "common/lru_cache",
     "common/remote_signer_consumer",

account_manager/src/validator/list.rs

@@ -1,24 +1,28 @@
-use crate::VALIDATOR_DIR_FLAG;
+use account_utils::validator_definitions::ValidatorDefinitions;
 use clap::App;
 use std::path::PathBuf;
-use validator_dir::Manager as ValidatorManager;
 
 pub const CMD: &str = "list";
 
 pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
-    App::new(CMD).about("Lists the names of all validators.")
+    App::new(CMD).about("Lists the public keys of all validators.")
 }
 
 pub fn cli_run(validator_dir: PathBuf) -> Result<(), String> {
     eprintln!("validator-dir path: {:?}", validator_dir);
-    let mgr = ValidatorManager::open(&validator_dir)
+    let validator_definitions = ValidatorDefinitions::open(&validator_dir).map_err(|e| {
-        .map_err(|e| format!("Unable to read --{}: {:?}", VALIDATOR_DIR_FLAG, e))?;
+        format!(
+            "No validator definitions found in {:?}: {:?}",
+            validator_dir, e
+        )
+    })?;
 
-    for (name, _path) in mgr
+    for def in validator_definitions.as_slice() {
-        .directory_names()
+        println!(
-        .map_err(|e| format!("Unable to list validators: {:?}", e))?
+            "{} ({})",
-    {
+            def.voting_public_key,
-        println!("{}", name)
+            if def.enabled { "enabled" } else { "disabled" }
+        );
     }
 
     Ok(())

common/eth2_wallet_manager/Cargo.toml

@@ -9,6 +9,7 @@ edition = "2018"
 [dependencies]
 eth2_keystore = { path = "../../crypto/eth2_keystore" }
 eth2_wallet = { path = "../../crypto/eth2_wallet" }
+lockfile = { path = "../lockfile" }
 
 [dev-dependencies]
 tempfile = "3.1.0"

common/eth2_wallet_manager/src/locked_wallet.rs

@@ -3,7 +3,7 @@ use crate::{
     Error,
 };
 use eth2_wallet::{Uuid, ValidatorKeystores, Wallet};
-use std::fs::{remove_file, OpenOptions};
+use lockfile::Lockfile;
 use std::path::{Path, PathBuf};
 
 pub const LOCK_FILE: &str = ".lock";
@@ -26,6 +26,7 @@ pub const LOCK_FILE: &str = ".lock";
 pub struct LockedWallet {
     wallet_dir: PathBuf,
     wallet: Wallet,
+    _lockfile: Lockfile,
 }
 
 impl LockedWallet {
@@ -49,20 +50,12 @@ impl LockedWallet {
             return Err(Error::MissingWalletDir(wallet_dir));
         }
 
-        let lockfile = wallet_dir.join(LOCK_FILE);
+        let _lockfile = Lockfile::new(wallet_dir.join(LOCK_FILE))?;
-        if lockfile.exists() {
-            return Err(Error::WalletIsLocked(wallet_dir));
-        } else {
-            OpenOptions::new()
-                .write(true)
-                .create_new(true)
-                .open(lockfile)
-                .map_err(Error::UnableToCreateLockfile)?;
-        }
 
         Ok(Self {
             wallet: read(&wallet_dir, uuid)?,
             wallet_dir,
+            _lockfile,
         })
     }
 
@@ -99,13 +92,3 @@ impl LockedWallet {
         Ok(keystores)
     }
 }
-
-impl Drop for LockedWallet {
-    /// Clean-up the lockfile.
-    fn drop(&mut self) {
-        let lockfile = self.wallet_dir.clone().join(LOCK_FILE);
-        if let Err(e) = remove_file(&lockfile) {
-            eprintln!("Unable to remove {:?}: {:?}", lockfile, e);
-        }
-    }
-}

common/eth2_wallet_manager/src/wallet_manager.rs

@@ -3,6 +3,7 @@ use crate::{
     LockedWallet,
 };
 use eth2_wallet::{bip39::Mnemonic, Error as WalletError, Uuid, Wallet, WalletBuilder};
+use lockfile::LockfileError;
 use std::collections::HashMap;
 use std::ffi::OsString;
 use std::fs::{create_dir_all, read_dir, OpenOptions};
@@ -21,10 +22,9 @@ pub enum Error {
     WalletNameUnknown(String),
     WalletDirExists(PathBuf),
     IoError(io::Error),
-    WalletIsLocked(PathBuf),
     MissingWalletDir(PathBuf),
-    UnableToCreateLockfile(io::Error),
     UuidMismatch((Uuid, Uuid)),
+    LockfileError(LockfileError),
 }
 
 impl From<io::Error> for Error {
@@ -45,6 +45,12 @@ impl From<FilesystemError> for Error {
     }
 }
 
+impl From<LockfileError> for Error {
+    fn from(e: LockfileError) -> Error {
+        Error::LockfileError(e)
+    }
+}
+
 /// Defines the type of an EIP-2386 wallet.
 ///
 /// Presently only `Hd` wallets are supported.
@@ -358,7 +364,7 @@ mod tests {
         );
 
         match LockedWallet::open(&base_dir, &uuid_a) {
-            Err(Error::WalletIsLocked(_)) => {}
+            Err(Error::LockfileError(_)) => {}
             _ => panic!("did not get locked error"),
         };
 
@@ -368,7 +374,7 @@ mod tests {
             .expect("should open wallet a after previous instance is dropped");
 
         match LockedWallet::open(&base_dir, &uuid_b) {
-            Err(Error::WalletIsLocked(_)) => {}
+            Err(Error::LockfileError(_)) => {}
             _ => panic!("did not get locked error"),
         };
 

common/lockfile/Cargo.toml

@@ -0,0 +1,11 @@
+[package]
+name = "lockfile"
+version = "0.1.0"
+authors = ["Michael Sproul <michael@sigmaprime.io>"]
+edition = "2018"
+
+[dependencies]
+fs2 = "0.4.3"
+
+[dev-dependencies]
+tempdir = "0.3.7"

common/lockfile/src/lib.rs

@@ -0,0 +1,133 @@
+use fs2::FileExt;
+use std::fs::{self, File, OpenOptions};
+use std::io::{self, ErrorKind};
+use std::path::{Path, PathBuf};
+
+/// Cross-platform file lock that auto-deletes on drop.
+///
+/// This lockfile uses OS locking primitives (`flock` on Unix, `LockFile` on Windows), and will
+/// only fail if locked by another process. I.e. if the file being locked already exists but isn't
+/// locked, then it can still be locked. This is relevant if an ungraceful shutdown (SIGKILL, power
+/// outage) caused the lockfile not to be deleted.
+#[derive(Debug)]
+pub struct Lockfile {
+    file: File,
+    path: PathBuf,
+    file_existed: bool,
+}
+
+#[derive(Debug)]
+pub enum LockfileError {
+    FileLocked(PathBuf, io::Error),
+    IoError(PathBuf, io::Error),
+    UnableToOpenFile(PathBuf, io::Error),
+}
+
+impl Lockfile {
+    /// Obtain an exclusive lock on the file at `path`, creating it if it doesn't exist.
+    pub fn new(path: PathBuf) -> Result<Self, LockfileError> {
+        let file_existed = path.exists();
+        let file = if file_existed {
+            File::open(&path)
+        } else {
+            OpenOptions::new()
+                .read(true)
+                .write(true)
+                .create_new(true)
+                .open(&path)
+        }
+        .map_err(|e| LockfileError::UnableToOpenFile(path.clone(), e))?;
+
+        file.try_lock_exclusive().map_err(|e| match e.kind() {
+            ErrorKind::WouldBlock => LockfileError::FileLocked(path.clone(), e),
+            _ => LockfileError::IoError(path.clone(), e),
+        })?;
+        Ok(Self {
+            file,
+            path,
+            file_existed,
+        })
+    }
+
+    /// Return `true` if the lockfile existed when the lock was created.
+    ///
+    /// This could indicate another process that isn't aware of the OS lock using the file,
+    /// or an ungraceful shutdown that caused the file not to be deleted.
+    pub fn file_existed(&self) -> bool {
+        self.file_existed
+    }
+
+    /// The path of the lockfile.
+    pub fn path(&self) -> &Path {
+        &self.path
+    }
+}
+
+impl Drop for Lockfile {
+    fn drop(&mut self) {
+        let _ = fs::remove_file(&self.path);
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use tempdir::TempDir;
+
+    #[cfg(unix)]
+    use std::{fs::Permissions, os::unix::fs::PermissionsExt};
+
+    #[test]
+    fn new_lock() {
+        let temp = TempDir::new("lock_test").unwrap();
+        let path = temp.path().join("lockfile");
+
+        let _lock = Lockfile::new(path.clone()).unwrap();
+        assert!(matches!(
+            Lockfile::new(path).unwrap_err(),
+            LockfileError::FileLocked(..)
+        ));
+    }
+
+    #[test]
+    fn relock_after_drop() {
+        let temp = TempDir::new("lock_test").unwrap();
+        let path = temp.path().join("lockfile");
+
+        let lock1 = Lockfile::new(path.clone()).unwrap();
+        drop(lock1);
+        let lock2 = Lockfile::new(path.clone()).unwrap();
+        assert!(!lock2.file_existed());
+        drop(lock2);
+
+        assert!(!path.exists());
+    }
+
+    #[test]
+    fn lockfile_exists() {
+        let temp = TempDir::new("lock_test").unwrap();
+        let path = temp.path().join("lockfile");
+
+        let _lockfile = File::create(&path).unwrap();
+
+        let lock = Lockfile::new(path.clone()).unwrap();
+        assert!(lock.file_existed());
+    }
+
+    #[test]
+    #[cfg(unix)]
+    fn permission_denied_create() {
+        let temp = TempDir::new("lock_test").unwrap();
+        let path = temp.path().join("lockfile");
+
+        let lockfile = File::create(&path).unwrap();
+        lockfile
+            .set_permissions(Permissions::from_mode(0o000))
+            .unwrap();
+
+        assert!(matches!(
+            Lockfile::new(path).unwrap_err(),
+            LockfileError::UnableToOpenFile(..)
+        ));
+    }
+}

common/validator_dir/Cargo.toml

@@ -19,6 +19,8 @@ rayon = "1.4.1"
 tree_hash = "0.1.1"
 slog = { version = "2.5.2", features = ["max_level_trace", "release_max_level_trace"] }
 hex = "0.4.2"
+derivative = "2.1.1"
+lockfile = { path = "../lockfile" }
 
 [dev-dependencies]
 tempfile = "3.1.0"

common/validator_dir/src/lib.rs

@@ -2,14 +2,12 @@
 //!
 //! - `ValidatorDir`: manages a directory containing validator keypairs, deposit info and other
 //! things.
-//! - `Manager`: manages a directory that contains multiple `ValidatorDir`.
 //!
 //! This crate is intended to be used by the account manager to create validators and the validator
 //! client to load those validators.
 
 mod builder;
 pub mod insecure_keys;
-mod manager;
 mod validator_dir;
 
 pub use crate::validator_dir::{
@@ -20,4 +18,3 @@ pub use builder::{
     Builder, Error as BuilderError, ETH1_DEPOSIT_DATA_FILE, VOTING_KEYSTORE_FILE,
     WITHDRAWAL_KEYSTORE_FILE,
 };
-pub use manager::{Error as ManagerError, Manager};

common/validator_dir/src/manager.rs

@@ -1,175 +0,0 @@
-use crate::{Error as ValidatorDirError, ValidatorDir};
-use bls::Keypair;
-use rayon::prelude::*;
-use slog::{info, warn, Logger};
-use std::collections::BTreeMap;
-use std::fs::read_dir;
-use std::io;
-use std::iter::FromIterator;
-use std::path::{Path, PathBuf};
-
-#[derive(Debug)]
-pub enum Error {
-    DirectoryDoesNotExist(PathBuf),
-    UnableToReadBaseDir(io::Error),
-    UnableToReadFile(io::Error),
-    ValidatorDirError(ValidatorDirError),
-}
-
-/// Manages a directory containing multiple `ValidatorDir` directories.
-///
-/// ## Example
-///
-/// ```ignore
-/// validators
-/// └── 0x91494d3ac4c078049f37aa46934ba8cdf5a9cca6e1b9a9e12403d69d8a2c43a25a7f576df2a5a3d7cb3f45e6aa5e2812
-///     ├── eth1_deposit_data.rlp
-///     ├── deposit-tx-hash.txt
-///     ├── voting-keystore.json
-///     └── withdrawal-keystore.json
-/// ```
-pub struct Manager {
-    dir: PathBuf,
-}
-
-impl Manager {
-    /// Open a directory containing multiple validators.
-    ///
-    /// Pass the `validators` director as `dir` (see struct-level example).
-    pub fn open<P: AsRef<Path>>(dir: P) -> Result<Self, Error> {
-        let dir: PathBuf = dir.as_ref().into();
-
-        if dir.exists() {
-            Ok(Self { dir })
-        } else {
-            Err(Error::DirectoryDoesNotExist(dir))
-        }
-    }
-
-    /// Iterate the nodes in `self.dir`, filtering out things that are unlikely to be a validator
-    /// directory.
-    fn iter_dir(&self) -> Result<Vec<PathBuf>, Error> {
-        read_dir(&self.dir)
-            .map_err(Error::UnableToReadBaseDir)?
-            .map(|file_res| file_res.map(|f| f.path()))
-            // We use `map_or` with `true` here to ensure that we always fail if there is any
-            // error.
-            .filter(|path_res| path_res.as_ref().map_or(true, |p| p.is_dir()))
-            .map(|res| res.map_err(Error::UnableToReadFile))
-            .collect()
-    }
-
-    /// Open a `ValidatorDir` at the given `path`.
-    ///
-    /// ## Note
-    ///
-    /// It is not enforced that `path` is contained in `self.dir`.
-    pub fn open_validator<P: AsRef<Path>>(&self, path: P) -> Result<ValidatorDir, Error> {
-        ValidatorDir::open(path).map_err(Error::ValidatorDirError)
-    }
-
-    /// Opens all the validator directories in `self`.
-    ///
-    /// ## Errors
-    ///
-    /// Returns an error if any of the directories is unable to be opened, perhaps due to a
-    /// file-system error or directory with an active lockfile.
-    pub fn open_all_validators(&self) -> Result<Vec<ValidatorDir>, Error> {
-        self.iter_dir()?
-            .into_iter()
-            .map(|path| ValidatorDir::open(path).map_err(Error::ValidatorDirError))
-            .collect()
-    }
-
-    /// Opens all the validator directories in `self` and decrypts the validator keypairs,
-    /// regardless if a lockfile exists or not.
-    ///
-    /// If `log.is_some()`, an `info` log will be generated for each decrypted validator.
-    /// Additionally, a warning log will be created if a lockfile existed already.
-    ///
-    /// ## Errors
-    ///
-    /// Returns an error if any of the directories is unable to be opened.
-    pub fn force_decrypt_all_validators(
-        &self,
-        secrets_dir: PathBuf,
-        log_opt: Option<&Logger>,
-    ) -> Result<Vec<(Keypair, ValidatorDir)>, Error> {
-        self.iter_dir()?
-            .into_par_iter()
-            .map(|path| {
-                ValidatorDir::force_open(path)
-                    .and_then(|(v, existed)| {
-                        v.voting_keypair(&secrets_dir).map(|kp| (kp, v, existed))
-                    })
-                    .map(|(kp, v, lockfile_existed)| {
-                        if let Some(log) = log_opt {
-                            info!(
-                                log,
-                                "Decrypted validator keystore";
-                                "voting_pubkey" => kp.pk.to_hex_string()
-                            );
-                            if lockfile_existed {
-                                warn!(
-                                    log,
-                                    "Lockfile already existed";
-                                    "msg" => "ensure no other validator client is running on this host",
-                                    "voting_pubkey" => kp.pk.to_hex_string()
-                                );
-                            }
-                        }
-                        (kp, v)
-                    })
-                    .map_err(Error::ValidatorDirError)
-            })
-            .collect()
-    }
-
-    /// Opens all the validator directories in `self` and decrypts the validator keypairs.
-    ///
-    /// If `log.is_some()`, an `info` log will be generated for each decrypted validator.
-    ///
-    /// ## Errors
-    ///
-    /// Returns an error if any of the directories is unable to be opened.
-    pub fn decrypt_all_validators(
-        &self,
-        secrets_dir: PathBuf,
-        log_opt: Option<&Logger>,
-    ) -> Result<Vec<(Keypair, ValidatorDir)>, Error> {
-        self.iter_dir()?
-            .into_par_iter()
-            .map(|path| {
-                ValidatorDir::open(path)
-                    .and_then(|v| v.voting_keypair(&secrets_dir).map(|kp| (kp, v)))
-                    .map(|(kp, v)| {
-                        if let Some(log) = log_opt {
-                            info!(
-                                log,
-                                "Decrypted validator keystore";
-                                "voting_pubkey" => kp.pk.to_hex_string()
-                            )
-                        }
-                        (kp, v)
-                    })
-                    .map_err(Error::ValidatorDirError)
-            })
-            .collect()
-    }
-
-    /// Returns a map of directory name to full directory path. E.g., `myval -> /home/vals/myval`.
-    /// Filters out nodes in `self.dir` that are unlikely to be a validator directory.
-    ///
-    /// ## Errors
-    ///
-    /// Returns an error if a directory is unable to be read.
-    pub fn directory_names(&self) -> Result<BTreeMap<String, PathBuf>, Error> {
-        Ok(BTreeMap::from_iter(
-            self.iter_dir()?.into_iter().filter_map(|path| {
-                path.file_name()
-                    .and_then(|os_string| os_string.to_str().map(|s| s.to_string()))
-                    .map(|filename| (filename, path))
-            }),
-        ))
-    }
-}

common/validator_dir/src/validator_dir.rs

@@ -3,24 +3,22 @@ use crate::builder::{
     WITHDRAWAL_KEYSTORE_FILE,
 };
 use deposit_contract::decode_eth1_tx_data;
+use derivative::Derivative;
 use eth2_keystore::{Error as KeystoreError, Keystore, PlainText};
-use std::fs::{read, remove_file, write, OpenOptions};
+use lockfile::{Lockfile, LockfileError};
+use std::fs::{read, write, OpenOptions};
 use std::io;
 use std::path::{Path, PathBuf};
 use tree_hash::TreeHash;
 use types::{DepositData, Hash256, Keypair};
 
-/// The file used for indicating if a directory is in-use by another process.
-const LOCK_FILE: &str = ".lock";
-
 /// The file used to save the Eth1 transaction hash from a deposit.
 pub const ETH1_DEPOSIT_TX_HASH_FILE: &str = "eth1-deposit-tx-hash.txt";
 
 #[derive(Debug)]
 pub enum Error {
     DirectoryDoesNotExist(PathBuf),
-    DirectoryLocked(PathBuf),
+    LockfileError(LockfileError),
-    UnableToCreateLockfile(io::Error),
     UnableToOpenKeystore(io::Error),
     UnableToReadKeystore(KeystoreError),
     UnableToOpenPassword(io::Error),
@@ -58,19 +56,22 @@ pub struct Eth1DepositData {
 
 /// Provides a wrapper around a directory containing validator information.
 ///
-/// Creates/deletes a lockfile in `self.dir` to attempt to prevent concurrent access from multiple
+/// Holds a lockfile in `self.dir` to attempt to prevent concurrent access from multiple
 /// processes.
-#[derive(Debug, PartialEq)]
+#[derive(Debug, Derivative)]
+#[derivative(PartialEq)]
 pub struct ValidatorDir {
     dir: PathBuf,
+    #[derivative(PartialEq = "ignore")]
+    lockfile: Lockfile,
 }
 
 impl ValidatorDir {
-    /// Open `dir`, creating a lockfile to prevent concurrent access.
+    /// Open `dir`, obtaining a lockfile to prevent concurrent access.
     ///
     /// ## Errors
     ///
-    /// If there is a filesystem error or if a lockfile already exists.
+    /// If there is a filesystem error or if the lockfile is locked by another process.
     pub fn open<P: AsRef<Path>>(dir: P) -> Result<Self, Error> {
         let dir: &Path = dir.as_ref();
         let dir: PathBuf = dir.into();
@@ -79,49 +80,12 @@ impl ValidatorDir {
             return Err(Error::DirectoryDoesNotExist(dir));
         }
 
-        let lockfile = dir.join(LOCK_FILE);
+        // Lock the keystore file that *might* be in this directory.
-        if lockfile.exists() {
+        // This is not ideal, see: https://github.com/sigp/lighthouse/issues/1978
-            return Err(Error::DirectoryLocked(dir));
+        let lockfile_path = dir.join(format!("{}.lock", VOTING_KEYSTORE_FILE));
-        } else {
+        let lockfile = Lockfile::new(lockfile_path).map_err(Error::LockfileError)?;
-            OpenOptions::new()
-                .write(true)
-                .create_new(true)
-                .open(lockfile)
-                .map_err(Error::UnableToCreateLockfile)?;
-        }
 
-        Ok(Self { dir })
+        Ok(Self { dir, lockfile })
-    }
-
-    /// Open `dir`, regardless or not if a lockfile exists.
-    ///
-    /// Returns `(validator_dir, lockfile_existed)`, where `lockfile_existed == true` if a lockfile
-    /// was already present before opening. Creates a lockfile if one did not already exist.
-    ///
-    /// ## Errors
-    ///
-    /// If there is a filesystem error.
-    pub fn force_open<P: AsRef<Path>>(dir: P) -> Result<(Self, bool), Error> {
-        let dir: &Path = dir.as_ref();
-        let dir: PathBuf = dir.into();
-
-        if !dir.exists() {
-            return Err(Error::DirectoryDoesNotExist(dir));
-        }
-
-        let lockfile = dir.join(LOCK_FILE);
-
-        let lockfile_exists = lockfile.exists();
-
-        if !lockfile_exists {
-            OpenOptions::new()
-                .write(true)
-                .create_new(true)
-                .open(lockfile)
-                .map_err(Error::UnableToCreateLockfile)?;
-        }
-
-        Ok((Self { dir }, lockfile_exists))
     }
 
     /// Returns the `dir` provided to `Self::open`.
@@ -238,18 +202,6 @@ impl ValidatorDir {
     }
 }
 
-impl Drop for ValidatorDir {
-    fn drop(&mut self) {
-        let lockfile = self.dir.clone().join(LOCK_FILE);
-        if let Err(e) = remove_file(&lockfile) {
-            eprintln!(
-                "Unable to remove validator lockfile {:?}: {:?}",
-                lockfile, e
-            );
-        }
-    }
-}
-
 /// Attempts to load and decrypt a Keypair given path to the keystore.
 pub fn unlock_keypair<P: AsRef<Path>>(
     keystore_path: &PathBuf,

validator_client/Cargo.toml

@@ -34,6 +34,7 @@ tokio = { version = "0.2.22", features = ["time"] }
 futures = { version = "0.3.5", features = ["compat"] }
 dirs = "3.0.1"
 directory = { path = "../common/directory" }
+lockfile = { path = "../common/lockfile" }
 logging = { path = "../common/logging" }
 environment = { path = "../lighthouse/environment" }
 parking_lot = "0.11.0"

validator_client/src/cli.rs

@@ -57,12 +57,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
             Arg::with_name("delete-lockfiles")
             .long("delete-lockfiles")
             .help(
-                "If present, ignore and delete any keystore lockfiles encountered during start up. \
+                "DEPRECATED. This flag does nothing and will be removed in a future release."
-                This is useful if the validator client did not exit gracefully on the last run. \
-                WARNING: lockfiles help prevent users from accidentally running the same validator \
-                using two different validator clients, an action that likely leads to slashing. \
-                Ensure you are certain that there are no other validator client instances running \
-                that might also be using the same keystores."
             )
         )
         .arg(

validator_client/src/config.rs

@@ -29,8 +29,6 @@ pub struct Config {
     /// If true, the validator client will still poll for duties and produce blocks even if the
     /// beacon node is not synced at startup.
     pub allow_unsynced_beacon_node: bool,
-    /// If true, delete any validator keystore lockfiles that would prevent starting.
-    pub delete_lockfiles: bool,
     /// If true, don't scan the validators dir for new keystores.
     pub disable_auto_discover: bool,
     /// If true, re-register existing validators in definitions.yml for slashing protection.
@@ -59,7 +57,6 @@ impl Default for Config {
             secrets_dir,
             beacon_node: DEFAULT_BEACON_NODE.to_string(),
             allow_unsynced_beacon_node: false,
-            delete_lockfiles: false,
             disable_auto_discover: false,
             init_slashing_protection: false,
             graffiti: None,
@@ -123,8 +120,15 @@ impl Config {
             config.beacon_node = server;
         }
 
+        if cli_args.is_present("delete-lockfiles") {
+            warn!(
+                log,
+                "The --delete-lockfiles flag is deprecated";
+                "msg" => "it is no longer necessary, and no longer has any effect",
+            );
+        }
+
         config.allow_unsynced_beacon_node = cli_args.is_present("allow-unsynced");
-        config.delete_lockfiles = cli_args.is_present("delete-lockfiles");
         config.disable_auto_discover = cli_args.is_present("disable-auto-discover");
         config.init_slashing_protection = cli_args.is_present("init-slashing-protection");
 

validator_client/src/http_api/create_validator.rs

@@ -125,9 +125,13 @@ pub fn create_validators<P: AsRef<Path>, T: 'static + SlotClock, E: EthSpec>(
             )));
         }
 
+        // Drop validator dir so that `add_validator_keystore` can re-lock the keystore.
+        let voting_keystore_path = validator_dir.voting_keystore_path();
+        drop(validator_dir);
+
         tokio::runtime::Handle::current()
             .block_on(validator_store.add_validator_keystore(
-                validator_dir.voting_keystore_path(),
+                voting_keystore_path,
                 voting_password_string,
                 request.enable,
             ))

validator_client/src/http_api/mod.rs

@@ -352,11 +352,14 @@ pub fn serve<T: 'static + SlotClock + Clone, E: EthSpec>(
                             ))
                         })?;
 
+                    // Drop validator dir so that `add_validator_keystore` can re-lock the keystore.
+                    let voting_keystore_path = validator_dir.voting_keystore_path();
+                    drop(validator_dir);
                     let voting_password = body.password.clone();
 
                     let validator_def = tokio::runtime::Handle::current()
                         .block_on(validator_store.add_validator_keystore(
-                            validator_dir.voting_keystore_path(),
+                            voting_keystore_path,
                             voting_password,
                             body.enable,
                         ))

validator_client/src/http_api/tests.rs

@@ -49,7 +49,6 @@ impl ApiTester {
         let initialized_validators = InitializedValidators::from_definitions(
             validator_defs,
             validator_dir.path().into(),
-            false,
             log.clone(),
         )
         .await

validator_client/src/initialized_validators.rs

@@ -14,16 +14,16 @@ use account_utils::{
     ZeroizeString,
 };
 use eth2_keystore::Keystore;
+use lockfile::{Lockfile, LockfileError};
 use slog::{debug, error, info, warn, Logger};
 use std::collections::{HashMap, HashSet};
-use std::fs::{self, File, OpenOptions};
+use std::fs::File;
 use std::io;
 use std::path::PathBuf;
 use types::{Keypair, PublicKey};
 
 use crate::key_cache;
 use crate::key_cache::KeyCache;
-use std::ops::{Deref, DerefMut};
 
 // Use TTY instead of stdin to capture passwords from users.
 const USE_STDIN: bool = false;
@@ -32,9 +32,7 @@ const USE_STDIN: bool = false;
 pub enum Error {
     /// Refused to open a validator with an existing lockfile since that validator may be in-use by
     /// another process.
-    LockfileExists(PathBuf),
+    LockfileError(LockfileError),
-    /// There was a filesystem error when creating the lockfile.
-    UnableToCreateLockfile(io::Error),
     /// The voting public key in the definition did not match the one in the keystore.
     VotingPublicKeyMismatch {
         definition: Box<PublicKey>,
@@ -61,12 +59,16 @@ pub enum Error {
     UnableToReadPasswordFromUser(String),
     /// There was an error running a tokio async task.
     TokioJoin(tokio::task::JoinError),
-    /// There was a filesystem error when deleting a lockfile.
-    UnableToDeleteLockfile(io::Error),
     /// Cannot initialize the same validator twice.
     DuplicatePublicKey,
 }
 
+impl From<LockfileError> for Error {
+    fn from(error: LockfileError) -> Self {
+        Self::LockfileError(error)
+    }
+}
+
 /// A method used by a validator to sign messages.
 ///
 /// Presently there is only a single variant, however we expect more variants to arise (e.g.,
@@ -75,7 +77,7 @@ pub enum SigningMethod {
     /// A validator that is defined by an EIP-2335 keystore on the local filesystem.
     LocalKeystore {
         voting_keystore_path: PathBuf,
-        voting_keystore_lockfile_path: PathBuf,
+        voting_keystore_lockfile: Lockfile,
         voting_keystore: Keystore,
         voting_keypair: Keypair,
     },
@@ -86,6 +88,18 @@ pub struct InitializedValidator {
     signing_method: SigningMethod,
 }
 
+impl InitializedValidator {
+    /// Return a reference to this validator's lockfile if it has one.
+    pub fn keystore_lockfile(&self) -> Option<&Lockfile> {
+        match self.signing_method {
+            SigningMethod::LocalKeystore {
+                ref voting_keystore_lockfile,
+                ..
+            } => Some(voting_keystore_lockfile),
+        }
+    }
+}
+
 fn open_keystore(path: &PathBuf) -> Result<Keystore, Error> {
     let keystore_file = File::open(path).map_err(Error::UnableToOpenVotingKeystore)?;
     Keystore::from_json_reader(keystore_file).map_err(Error::UnableToParseVotingKeystore)
@@ -102,43 +116,6 @@ fn get_lockfile_path(file_path: &PathBuf) -> Option<PathBuf> {
         })
 }
 
-fn create_lock_file(
-    file_path: &PathBuf,
-    delete_lockfiles: bool,
-    log: &Logger,
-) -> Result<(), Error> {
-    if file_path.exists() {
-        if delete_lockfiles {
-            warn!(
-                log,
-                "Deleting validator lockfile";
-                "file" => format!("{:?}", file_path)
-            );
-
-            fs::remove_file(file_path).map_err(Error::UnableToDeleteLockfile)?;
-        } else {
-            return Err(Error::LockfileExists(file_path.clone()));
-        }
-    }
-    // Create a new lockfile.
-    OpenOptions::new()
-        .write(true)
-        .create_new(true)
-        .open(file_path)
-        .map_err(Error::UnableToCreateLockfile)?;
-    Ok(())
-}
-
-fn remove_lock(lock_path: &PathBuf) {
-    if lock_path.exists() {
-        if let Err(e) = fs::remove_file(&lock_path) {
-            eprintln!("Failed to remove {:?}: {:?}", lock_path, e)
-        }
-    } else {
-        eprintln!("Lockfile missing: {:?}", lock_path)
-    }
-}
-
 impl InitializedValidator {
     /// Instantiate `self` from a `ValidatorDefinition`.
     ///
@@ -150,8 +127,6 @@ impl InitializedValidator {
     /// If the validator is unable to be initialized for whatever reason.
     async fn from_definition(
         def: ValidatorDefinition,
-        delete_lockfiles: bool,
-        log: &Logger,
         key_cache: &mut KeyCache,
         key_stores: &mut HashMap<PathBuf, Keystore>,
     ) -> Result<Self, Error> {
@@ -182,7 +157,7 @@ impl InitializedValidator {
                     // to keep if off the core executor. This also has the fortunate effect of
                     // interrupting the potentially long-running task during shut down.
                     let (password, keypair) = tokio::task::spawn_blocking(move || {
-                        Ok(
+                        Result::<_, Error>::Ok(
                             match (voting_keystore_password_path, voting_keystore_password) {
                                 // If the password is supplied, use it and ignore the path
                                 // (if supplied).
@@ -226,15 +201,15 @@ impl InitializedValidator {
                 }
 
                 // Append a `.lock` suffix to the voting keystore.
-                let voting_keystore_lockfile_path = get_lockfile_path(&voting_keystore_path)
+                let lockfile_path = get_lockfile_path(&voting_keystore_path)
                     .ok_or_else(|| Error::BadVotingKeystorePath(voting_keystore_path.clone()))?;
 
-                create_lock_file(&voting_keystore_lockfile_path, delete_lockfiles, &log)?;
+                let voting_keystore_lockfile = Lockfile::new(lockfile_path)?;
 
                 Ok(Self {
                     signing_method: SigningMethod::LocalKeystore {
                         voting_keystore_path,
-                        voting_keystore_lockfile_path,
+                        voting_keystore_lockfile,
                         voting_keystore: voting_keystore.clone(),
                         voting_keypair,
                     },
@@ -258,20 +233,6 @@ impl InitializedValidator {
     }
 }
 
-/// Custom drop implementation to allow for `LocalKeystore` to remove lockfiles.
-impl Drop for InitializedValidator {
-    fn drop(&mut self) {
-        match &self.signing_method {
-            SigningMethod::LocalKeystore {
-                voting_keystore_lockfile_path,
-                ..
-            } => {
-                remove_lock(voting_keystore_lockfile_path);
-            }
-        }
-    }
-}
-
 /// Try to unlock `keystore` at `keystore_path` by prompting the user via `stdin`.
 fn unlock_keystore_via_stdin_password(
     keystore: &Keystore,
@@ -316,8 +277,6 @@ fn unlock_keystore_via_stdin_password(
 ///
 /// Forms the fundamental list of validators that are managed by this validator client instance.
 pub struct InitializedValidators {
-    /// If `true`, delete any validator keystore lockfiles that would prevent starting.
-    delete_lockfiles: bool,
     /// A list of validator definitions which can be stored on-disk.
     definitions: ValidatorDefinitions,
     /// The directory that the `self.definitions` will be saved into.
@@ -328,47 +287,14 @@ pub struct InitializedValidators {
     log: Logger,
 }
 
-pub struct LockedData<T> {
-    data: T,
-    lock_path: PathBuf,
-}
-
-impl<T> LockedData<T> {
-    fn new(data: T, lock_path: PathBuf) -> Self {
-        Self { data, lock_path }
-    }
-}
-
-impl<T> Deref for LockedData<T> {
-    type Target = T;
-
-    fn deref(&self) -> &Self::Target {
-        &self.data
-    }
-}
-
-impl<T> DerefMut for LockedData<T> {
-    fn deref_mut(&mut self) -> &mut Self::Target {
-        &mut self.data
-    }
-}
-
-impl<T> Drop for LockedData<T> {
-    fn drop(&mut self) {
-        remove_lock(&self.lock_path);
-    }
-}
-
 impl InitializedValidators {
     /// Instantiates `Self`, initializing all validators in `definitions`.
     pub async fn from_definitions(
         definitions: ValidatorDefinitions,
         validators_dir: PathBuf,
-        delete_lockfiles: bool,
         log: Logger,
     ) -> Result<Self, Error> {
         let mut this = Self {
-            delete_lockfiles,
             validators_dir,
             definitions,
             validators: HashMap::default(),
@@ -566,23 +492,11 @@ impl InitializedValidators {
         let key_cache_path = KeyCache::cache_file_path(&self.validators_dir);
         let cache_lockfile_path = get_lockfile_path(&key_cache_path)
             .ok_or_else(|| Error::BadKeyCachePath(key_cache_path))?;
-        create_lock_file(&cache_lockfile_path, self.delete_lockfiles, &self.log)?;
+        let _cache_lockfile = Lockfile::new(cache_lockfile_path)?;
 
-        let mut key_cache = LockedData::new(
+        let cache =
-            {
+            KeyCache::open_or_create(&self.validators_dir).map_err(Error::UnableToOpenKeyCache)?;
-                let cache = KeyCache::open_or_create(&self.validators_dir).map_err(|e| {
+        let mut key_cache = self.decrypt_key_cache(cache, &mut key_stores).await?;
-                    remove_lock(&cache_lockfile_path);
-                    Error::UnableToOpenKeyCache(e)
-                })?;
-                self.decrypt_key_cache(cache, &mut key_stores)
-                    .await
-                    .map_err(|e| {
-                        remove_lock(&cache_lockfile_path);
-                        e
-                    })?
-            },
-            cache_lockfile_path,
-        );
 
         let mut disabled_uuids = HashSet::new();
         for def in self.definitions.as_slice() {
@@ -602,14 +516,18 @@ impl InitializedValidators {
 
                         match InitializedValidator::from_definition(
                             def.clone(),
-                            self.delete_lockfiles,
-                            &self.log,
                             &mut key_cache,
                             &mut key_stores,
                         )
                         .await
                         {
                             Ok(init) => {
+                                let existing_lockfile_path = init
+                                    .keystore_lockfile()
+                                    .as_ref()
+                                    .filter(|l| l.file_existed())
+                                    .map(|l| l.path().to_owned());
+
                                 self.validators
                                     .insert(init.voting_public_key().clone(), init);
                                 info!(
@@ -617,6 +535,17 @@ impl InitializedValidators {
                                     "Enabled validator";
                                     "voting_pubkey" => format!("{:?}", def.voting_public_key)
                                 );
+
+                                if let Some(lockfile_path) = existing_lockfile_path {
+                                    warn!(
+                                        self.log,
+                                        "Ignored stale lockfile";
+                                        "path" => lockfile_path.display(),
+                                        "cause" => "Ungraceful shutdown (harmless) OR \
+                                                    non-Lighthouse client using this keystore \
+                                                    (risky)"
+                                    );
+                                }
                             }
                             Err(e) => {
                                 error!(

validator_client/src/lib.rs

@@ -138,7 +138,6 @@ impl<T: EthSpec> ProductionValidatorClient<T> {
         let validators = InitializedValidators::from_definitions(
             validator_defs,
             config.validator_dir.clone(),
-            config.delete_lockfiles,
             log.clone(),
         )
         .await