From 3486d6a80958f56ee4ca2cbb465d12befa81de4d Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 26 Nov 2020 11:25:46 +0000 Subject: [PATCH] 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 | 2 +- Cargo.lock | 22 +++ Cargo.toml | 1 + account_manager/src/validator/list.rs | 24 ++- common/eth2_wallet_manager/Cargo.toml | 1 + .../eth2_wallet_manager/src/locked_wallet.rs | 25 +-- .../eth2_wallet_manager/src/wallet_manager.rs | 14 +- common/lockfile/Cargo.toml | 11 ++ common/lockfile/src/lib.rs | 133 +++++++++++++ common/validator_dir/Cargo.toml | 2 + common/validator_dir/src/lib.rs | 3 - common/validator_dir/src/manager.rs | 175 ------------------ common/validator_dir/src/validator_dir.rs | 80 ++------ validator_client/Cargo.toml | 3 +- validator_client/src/cli.rs | 7 +- validator_client/src/config.rs | 12 +- .../src/http_api/create_validator.rs | 6 +- validator_client/src/http_api/mod.rs | 5 +- validator_client/src/http_api/tests.rs | 1 - .../src/initialized_validators.rs | 165 +++++------------ validator_client/src/lib.rs | 1 - 21 files changed, 282 insertions(+), 411 deletions(-) create mode 100644 common/lockfile/Cargo.toml create mode 100644 common/lockfile/src/lib.rs delete mode 100644 common/validator_dir/src/manager.rs diff --git a/.dockerignore b/.dockerignore index 37c2b8932..d9b57b657 100644 --- a/.dockerignore +++ b/.dockerignore @@ -1,4 +1,4 @@ -tests/ef_tests/eth2.0-spec-tests +testing/ef_tests/eth2.0-spec-tests target/ *.data *.tar.gz diff --git a/Cargo.lock b/Cargo.lock index 45250ffa5..0e02e4091 100644 --- a/Cargo.lock +++ b/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", diff --git a/Cargo.toml b/Cargo.toml index 2e30af450..f08276104 100644 --- a/Cargo.toml +++ b/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", diff --git a/account_manager/src/validator/list.rs b/account_manager/src/validator/list.rs index 600fa420f..60173ec5e 100644 --- a/account_manager/src/validator/list.rs +++ b/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) - .map_err(|e| format!("Unable to read --{}: {:?}", VALIDATOR_DIR_FLAG, e))?; + let validator_definitions = ValidatorDefinitions::open(&validator_dir).map_err(|e| { + format!( + "No validator definitions found in {:?}: {:?}", + validator_dir, e + ) + })?; - for (name, _path) in mgr - .directory_names() - .map_err(|e| format!("Unable to list validators: {:?}", e))? - { - println!("{}", name) + for def in validator_definitions.as_slice() { + println!( + "{} ({})", + def.voting_public_key, + if def.enabled { "enabled" } else { "disabled" } + ); } Ok(()) diff --git a/common/eth2_wallet_manager/Cargo.toml b/common/eth2_wallet_manager/Cargo.toml index 7f3b6b545..0e38c8732 100644 --- a/common/eth2_wallet_manager/Cargo.toml +++ b/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" diff --git a/common/eth2_wallet_manager/src/locked_wallet.rs b/common/eth2_wallet_manager/src/locked_wallet.rs index 029901a53..a77f9bd78 100644 --- a/common/eth2_wallet_manager/src/locked_wallet.rs +++ b/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); - if lockfile.exists() { - return Err(Error::WalletIsLocked(wallet_dir)); - } else { - OpenOptions::new() - .write(true) - .create_new(true) - .open(lockfile) - .map_err(Error::UnableToCreateLockfile)?; - } + let _lockfile = Lockfile::new(wallet_dir.join(LOCK_FILE))?; 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); - } - } -} diff --git a/common/eth2_wallet_manager/src/wallet_manager.rs b/common/eth2_wallet_manager/src/wallet_manager.rs index df30cfe4c..57773595b 100644 --- a/common/eth2_wallet_manager/src/wallet_manager.rs +++ b/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 for Error { @@ -45,6 +45,12 @@ impl From for Error { } } +impl From 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"), }; diff --git a/common/lockfile/Cargo.toml b/common/lockfile/Cargo.toml new file mode 100644 index 000000000..7300520cc --- /dev/null +++ b/common/lockfile/Cargo.toml @@ -0,0 +1,11 @@ +[package] +name = "lockfile" +version = "0.1.0" +authors = ["Michael Sproul "] +edition = "2018" + +[dependencies] +fs2 = "0.4.3" + +[dev-dependencies] +tempdir = "0.3.7" diff --git a/common/lockfile/src/lib.rs b/common/lockfile/src/lib.rs new file mode 100644 index 000000000..433d4426e --- /dev/null +++ b/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 { + 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(..) + )); + } +} diff --git a/common/validator_dir/Cargo.toml b/common/validator_dir/Cargo.toml index df8eed3d0..f993197c6 100644 --- a/common/validator_dir/Cargo.toml +++ b/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" diff --git a/common/validator_dir/src/lib.rs b/common/validator_dir/src/lib.rs index 875901ded..a39d32283 100644 --- a/common/validator_dir/src/lib.rs +++ b/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}; diff --git a/common/validator_dir/src/manager.rs b/common/validator_dir/src/manager.rs deleted file mode 100644 index a2a50d106..000000000 --- a/common/validator_dir/src/manager.rs +++ /dev/null @@ -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>(dir: P) -> Result { - 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, 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>(&self, path: P) -> Result { - 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, 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, 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, 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, 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)) - }), - )) - } -} diff --git a/common/validator_dir/src/validator_dir.rs b/common/validator_dir/src/validator_dir.rs index 26783de01..fb2e06bd4 100644 --- a/common/validator_dir/src/validator_dir.rs +++ b/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), - UnableToCreateLockfile(io::Error), + LockfileError(LockfileError), 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>(dir: P) -> Result { 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); - if lockfile.exists() { - return Err(Error::DirectoryLocked(dir)); - } else { - OpenOptions::new() - .write(true) - .create_new(true) - .open(lockfile) - .map_err(Error::UnableToCreateLockfile)?; - } + // Lock the keystore file that *might* be in this directory. + // This is not ideal, see: https://github.com/sigp/lighthouse/issues/1978 + let lockfile_path = dir.join(format!("{}.lock", VOTING_KEYSTORE_FILE)); + let lockfile = Lockfile::new(lockfile_path).map_err(Error::LockfileError)?; - Ok(Self { dir }) - } - - /// 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>(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)) + Ok(Self { dir, lockfile }) } /// 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>( keystore_path: &PathBuf, diff --git a/validator_client/Cargo.toml b/validator_client/Cargo.toml index 382105ff5..3dd8aa800 100644 --- a/validator_client/Cargo.toml +++ b/validator_client/Cargo.toml @@ -33,7 +33,8 @@ slog-term = "2.6.0" tokio = { version = "0.2.22", features = ["time"] } futures = { version = "0.3.5", features = ["compat"] } dirs = "3.0.1" -directory = {path = "../common/directory"} +directory = { path = "../common/directory" } +lockfile = { path = "../common/lockfile" } logging = { path = "../common/logging" } environment = { path = "../lighthouse/environment" } parking_lot = "0.11.0" diff --git a/validator_client/src/cli.rs b/validator_client/src/cli.rs index bdc5e7d05..5cc5ab60a 100644 --- a/validator_client/src/cli.rs +++ b/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. \ - 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." + "DEPRECATED. This flag does nothing and will be removed in a future release." ) ) .arg( diff --git a/validator_client/src/config.rs b/validator_client/src/config.rs index 13a9a9de5..a222258ed 100644 --- a/validator_client/src/config.rs +++ b/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"); diff --git a/validator_client/src/http_api/create_validator.rs b/validator_client/src/http_api/create_validator.rs index b84f9d614..a186d4e98 100644 --- a/validator_client/src/http_api/create_validator.rs +++ b/validator_client/src/http_api/create_validator.rs @@ -125,9 +125,13 @@ pub fn create_validators, 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, )) diff --git a/validator_client/src/http_api/mod.rs b/validator_client/src/http_api/mod.rs index 6dde635c9..9172cee5b 100644 --- a/validator_client/src/http_api/mod.rs +++ b/validator_client/src/http_api/mod.rs @@ -352,11 +352,14 @@ pub fn serve( )) })?; + // 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, )) diff --git a/validator_client/src/http_api/tests.rs b/validator_client/src/http_api/tests.rs index fd7f3d6ed..594b82e27 100644 --- a/validator_client/src/http_api/tests.rs +++ b/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 diff --git a/validator_client/src/initialized_validators.rs b/validator_client/src/initialized_validators.rs index 67ee77641..47e8c6763 100644 --- a/validator_client/src/initialized_validators.rs +++ b/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), - /// There was a filesystem error when creating the lockfile. - UnableToCreateLockfile(io::Error), + LockfileError(LockfileError), /// The voting public key in the definition did not match the one in the keystore. VotingPublicKeyMismatch { definition: Box, @@ -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 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 { 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 { }) } -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, ) -> Result { @@ -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 { - data: T, - lock_path: PathBuf, -} - -impl LockedData { - fn new(data: T, lock_path: PathBuf) -> Self { - Self { data, lock_path } - } -} - -impl Deref for LockedData { - type Target = T; - - fn deref(&self) -> &Self::Target { - &self.data - } -} - -impl DerefMut for LockedData { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.data - } -} - -impl Drop for LockedData { - 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 { 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(|e| { - 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 cache = + KeyCache::open_or_create(&self.validators_dir).map_err(Error::UnableToOpenKeyCache)?; + let mut key_cache = self.decrypt_key_cache(cache, &mut key_stores).await?; 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!( diff --git a/validator_client/src/lib.rs b/validator_client/src/lib.rs index 06350f2f9..e495d7e49 100644 --- a/validator_client/src/lib.rs +++ b/validator_client/src/lib.rs @@ -138,7 +138,6 @@ impl ProductionValidatorClient { let validators = InitializedValidators::from_definitions( validator_defs, config.validator_dir.clone(), - config.delete_lockfiles, log.clone(), ) .await