gnosis/lighthouse
1
0
Fork 0
mirror of https://github.com/sigp/lighthouse.git synced 2026-03-22 22:34:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

Write validator definitions atomically (#2338)

Browse Source 
## Issue Addressed

Closes https://github.com/sigp/lighthouse/issues/2159

## Proposed Changes

Rather than trying to write the validator definitions to disk directly, use a temporary file called `.validator_defintions.yml.tmp` and then atomically rename it to `validator_definitions.yml`. This avoids truncating the primary file, which can cause permanent damage when the disk is full.

The same treatment is also applied to the validator key cache, although the situation is less dire if it becomes corrupted because it can just be deleted without the user having to reimport keys or resupply passwords.

## Additional Info

* `File::create` truncates upon opening: https://doc.rust-lang.org/std/fs/struct.File.html#method.create
* `fs::rename` uses `rename` on UNIX and `MoveFileEx` on Windows: https://doc.rust-lang.org/std/fs/fn.rename.html
* UNIX `rename` call is atomic: https://unix.stackexchange.com/questions/322038/is-mv-atomic-on-my-fs
* Windows `MoveFileEx` is _not_ atomic in general, and Windows lacks any clear API for atomic file renames :(
   https://stackoverflow.com/questions/167414/is-an-atomic-file-rename-with-overwrite-possible-on-windows

## Further Work

* Consider whether we want to try a different Windows syscall as part of #2333. The `rust-atomicwrites` crate seems promising, but actually uses the same syscall under the hood presently: https://github.com/untitaker/rust-atomicwrites/issues/27.
This commit is contained in:
Michael Sproul
2021-05-12 02:04:44 +00:00
parent 480b247828
commit 58e52f8f40
3 changed files with 57 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
common/account_utils/src/validator_definitions.rs
View File

@@ -3,7 +3,7 @@
//! Serves as the source-of-truth of which validators this validator client should attempt (or not
//! attempt) to load into the `crate::intialized_validators::InitializedValidators` struct.
use crate::{create_with_600_perms, default_keystore_password_path, ZeroizeString};
use crate::{default_keystore_password_path, write_file_via_temporary, ZeroizeString};
use directory::ensure_dir_exists;
use eth2_keystore::Keystore;
use regex::Regex;
@@ -19,6 +19,12 @@ use validator_dir::VOTING_KEYSTORE_FILE;
/// The file name for the serialized `ValidatorDefinitions` struct.
pub const CONFIG_FILENAME: &str = "validator_definitions.yml";
/// The temporary file name for the serialized `ValidatorDefinitions` struct.
///
/// This is used to achieve an atomic update of the contents on disk, without truncation.
/// See: https://github.com/sigp/lighthouse/issues/2159
pub const CONFIG_TEMP_FILENAME: &str = ".validator_definitions.yml.tmp";
#[derive(Debug)]
pub enum Error {
/// The config file could not be opened.
@@ -29,7 +35,7 @@ pub enum Error {
UnableToSearchForKeystores(io::Error),
/// The config file could not be serialized as YAML.
UnableToEncodeFile(serde_yaml::Error),
/// The config file could not be written to the filesystem.
/// The config file or temp file could not be written to the filesystem.
UnableToWriteFile(io::Error),
/// The public key from the keystore is invalid.
InvalidKeystorePubkey,
@@ -249,19 +255,19 @@ impl ValidatorDefinitions {
Ok(new_defs_count)
}
/// Encodes `self` as a YAML string it writes it to the `CONFIG_FILENAME` file in the
/// `validators_dir` directory.
/// Encodes `self` as a YAML string and atomically writes it to the `CONFIG_FILENAME` file in
/// the `validators_dir` directory.
///
/// Will create a new file if it does not exist or over-write any existing file.
/// Will create a new file if it does not exist or overwrite any existing file.
pub fn save<P: AsRef<Path>>(&self, validators_dir: P) -> Result<(), Error> {
let config_path = validators_dir.as_ref().join(CONFIG_FILENAME);
let temp_path = validators_dir.as_ref().join(CONFIG_TEMP_FILENAME);
let bytes = serde_yaml::to_vec(self).map_err(Error::UnableToEncodeFile)?;
if config_path.exists() {
fs::write(config_path, &bytes).map_err(Error::UnableToWriteFile)
} else {
create_with_600_perms(&config_path, &bytes).map_err(Error::UnableToWriteFile)
}
write_file_via_temporary(&config_path, &temp_path, &bytes)
.map_err(Error::UnableToWriteFile)?;
Ok(())
}
/// Adds a new `ValidatorDefinition` to `self`.
@@ -392,7 +398,7 @@ mod tests {
#[test]
fn graffiti_checks() {
let no_graffiti = r#"---
let no_graffiti = r#"---
description: ""
enabled: true
type: local_keystore
@@ -402,7 +408,7 @@ mod tests {
let def: ValidatorDefinition = serde_yaml::from_str(&no_graffiti).unwrap();
assert!(def.graffiti.is_none());
let invalid_graffiti = r#"---
let invalid_graffiti = r#"---
description: ""
enabled: true
type: local_keystore
@@ -414,7 +420,7 @@ mod tests {
let def: Result<ValidatorDefinition, _> = serde_yaml::from_str(&invalid_graffiti);
assert!(def.is_err());
let valid_graffiti = r#"---
let valid_graffiti = r#"---
description: ""
enabled: true
type: local_keystore