From 0eb62f8e0d4f469b90f5252a391025f7f923b7d1 Mon Sep 17 00:00:00 2001 From: yihau Date: Tue, 25 Jun 2024 13:07:43 +0800 Subject: [PATCH 1/4] check whether accounts and snapshots are using the same path --- Cargo.lock | 3 +++ validator/Cargo.toml | 3 +++ validator/src/main.rs | 21 +++++++++++++++------ validator/tests/cli.rs | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 6 deletions(-) create mode 100644 validator/tests/cli.rs diff --git a/Cargo.lock b/Cargo.lock index 8d76491cac02a3..5288fb235584ed 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -219,6 +219,7 @@ name = "agave-validator" version = "2.0.0" dependencies = [ "agave-geyser-plugin-interface", + "assert_cmd", "chrono", "clap 2.33.3", "console", @@ -236,6 +237,7 @@ dependencies = [ "libloading", "log", "num_cpus", + "predicates", "rand 0.8.5", "rayon", "serde", @@ -277,6 +279,7 @@ dependencies = [ "solana-vote-program", "spl-token-2022", "symlink", + "tempfile", "thiserror", "tikv-jemallocator", "tokio", diff --git a/validator/Cargo.toml b/validator/Cargo.toml index 6222435906a31d..39e5c4d1b19d0e 100644 --- a/validator/Cargo.toml +++ b/validator/Cargo.toml @@ -69,10 +69,13 @@ thiserror = { workspace = true } tokio = { workspace = true } [dev-dependencies] +assert_cmd = { workspace = true } +predicates = { workspace = true } solana-account-decoder = { workspace = true } solana-inline-spl = { workspace = true } solana-runtime = { workspace = true, features = ["dev-context-only-utils"] } spl-token-2022 = { workspace = true, features = ["no-entrypoint"] } +tempfile = { workspace = true } [target.'cfg(not(target_env = "msvc"))'.dependencies] jemallocator = { workspace = true } diff --git a/validator/src/main.rs b/validator/src/main.rs index a87f6b3f488d9c..0d3c7c10f5eb5d 100644 --- a/validator/src/main.rs +++ b/validator/src/main.rs @@ -1549,6 +1549,21 @@ pub fn main() { } else { vec![ledger_path.join("accounts")] }; + + let snapshots_dir = if let Some(snapshots) = matches.value_of("snapshots") { + Path::new(snapshots) + } else { + &ledger_path + }; + + // they have the same subdirectory name, "snapshot". try to catch the directory collision error earlier + for account_path in account_paths.iter() { + if account_path.as_path() == snapshots_dir { + eprintln!("Please provide different values for --accounts and --snapshots"); + exit(1); + } + } + let account_paths = create_and_canonicalize_directories(account_paths).unwrap_or_else(|err| { eprintln!("Unable to access account path: {err}"); exit(1); @@ -1589,12 +1604,6 @@ pub fn main() { let maximum_snapshot_download_abort = value_t_or_exit!(matches, "maximum_snapshot_download_abort", u64); - let snapshots_dir = if let Some(snapshots) = matches.value_of("snapshots") { - Path::new(snapshots) - } else { - &ledger_path - }; - let bank_snapshots_dir = snapshots_dir.join("snapshots"); fs::create_dir_all(&bank_snapshots_dir).unwrap_or_else(|err| { eprintln!( diff --git a/validator/tests/cli.rs b/validator/tests/cli.rs new file mode 100644 index 00000000000000..fc9168f0acbcd5 --- /dev/null +++ b/validator/tests/cli.rs @@ -0,0 +1,36 @@ +use { + assert_cmd::prelude::*, + solana_sdk::signer::keypair::{write_keypair_file, Keypair}, + std::process::Command, + tempfile::TempDir, +}; + +#[test] +fn test_use_the_same_path_for_accounts_and_snapshots() { + let temp_dir = TempDir::new().unwrap(); + let temp_dir_path = temp_dir.path(); + + let id_json_path = temp_dir_path.join("id.json"); + let id_json_str = id_json_path.to_str().unwrap(); + + let keypair = Keypair::new(); + write_keypair_file(&keypair, id_json_str).unwrap(); + + let temp_dir_str = temp_dir_path.to_str().unwrap(); + + let mut cmd = Command::cargo_bin(env!("CARGO_PKG_NAME")).unwrap(); + cmd.args([ + "--identity", + id_json_str, + "--log", + "-", + "--no-voting", + "--accounts", + temp_dir_str, + "--snapshots", + temp_dir_str, + ]); + cmd.assert().failure().stderr(predicates::str::contains( + "Please provide different values for --accounts and --snapshots", + )); +} From d9fc8012a456336467cb8cb64ec5921d5f01a693 Mon Sep 17 00:00:00 2001 From: yihau Date: Wed, 26 Jun 2024 15:17:34 +0800 Subject: [PATCH 2/4] patch and update test --- validator/src/main.rs | 64 +++++++++++++++++++++++------------------- validator/tests/cli.rs | 2 +- 2 files changed, 36 insertions(+), 30 deletions(-) diff --git a/validator/src/main.rs b/validator/src/main.rs index 0d3c7c10f5eb5d..8193b90251b1bf 100644 --- a/validator/src/main.rs +++ b/validator/src/main.rs @@ -1549,21 +1549,6 @@ pub fn main() { } else { vec![ledger_path.join("accounts")] }; - - let snapshots_dir = if let Some(snapshots) = matches.value_of("snapshots") { - Path::new(snapshots) - } else { - &ledger_path - }; - - // they have the same subdirectory name, "snapshot". try to catch the directory collision error earlier - for account_path in account_paths.iter() { - if account_path.as_path() == snapshots_dir { - eprintln!("Please provide different values for --accounts and --snapshots"); - exit(1); - } - } - let account_paths = create_and_canonicalize_directories(account_paths).unwrap_or_else(|err| { eprintln!("Unable to access account path: {err}"); exit(1); @@ -1604,6 +1589,30 @@ pub fn main() { let maximum_snapshot_download_abort = value_t_or_exit!(matches, "maximum_snapshot_download_abort", u64); + let snapshots_dir = if let Some(snapshots) = matches.value_of("snapshots") { + Path::new(snapshots) + } else { + &ledger_path + }; + + let snapshots_dir = fs::canonicalize(snapshots_dir).unwrap_or_else(|err| { + eprintln!( + "Failed to canonicalize snapshots path '{}': {err}", + snapshots_dir.display(), + ); + exit(1); + }); + if account_paths + .iter() + .any(|account_path| account_path == &snapshots_dir) + { + eprintln!( + "Failed: The --accounts and --snapshots paths must be unique since they \ + both create 'snapshots' subdirectories, otherwise there may be collisions", + ); + exit(1); + } + let bank_snapshots_dir = snapshots_dir.join("snapshots"); fs::create_dir_all(&bank_snapshots_dir).unwrap_or_else(|err| { eprintln!( @@ -1613,13 +1622,12 @@ pub fn main() { exit(1); }); - let full_snapshot_archives_dir = PathBuf::from( + let full_snapshot_archives_dir = if let Some(full_snapshot_archive_path) = matches.value_of("full_snapshot_archive_path") { - Path::new(full_snapshot_archive_path) + PathBuf::from(full_snapshot_archive_path) } else { - snapshots_dir - }, - ); + snapshots_dir.clone() + }; fs::create_dir_all(&full_snapshot_archives_dir).unwrap_or_else(|err| { eprintln!( "Failed to create full snapshot archives directory '{}': {err}", @@ -1628,15 +1636,13 @@ pub fn main() { exit(1); }); - let incremental_snapshot_archives_dir = PathBuf::from( - if let Some(incremental_snapshot_archive_path) = - matches.value_of("incremental_snapshot_archive_path") - { - Path::new(incremental_snapshot_archive_path) - } else { - snapshots_dir - }, - ); + let incremental_snapshot_archives_dir = if let Some(incremental_snapshot_archive_path) = + matches.value_of("incremental_snapshot_archive_path") + { + PathBuf::from(incremental_snapshot_archive_path) + } else { + snapshots_dir.clone() + }; fs::create_dir_all(&incremental_snapshot_archives_dir).unwrap_or_else(|err| { eprintln!( "Failed to create incremental snapshot archives directory '{}': {err}", diff --git a/validator/tests/cli.rs b/validator/tests/cli.rs index fc9168f0acbcd5..3757df96df1d8d 100644 --- a/validator/tests/cli.rs +++ b/validator/tests/cli.rs @@ -31,6 +31,6 @@ fn test_use_the_same_path_for_accounts_and_snapshots() { temp_dir_str, ]); cmd.assert().failure().stderr(predicates::str::contains( - "Please provide different values for --accounts and --snapshots", + "The --accounts and --snapshots paths must be unique", )); } From 76a86d080190629967626c93f7f7e77fd4d4bbec Mon Sep 17 00:00:00 2001 From: yihau Date: Wed, 26 Jun 2024 22:34:46 +0800 Subject: [PATCH 3/4] remove new line --- validator/src/main.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/validator/src/main.rs b/validator/src/main.rs index 8193b90251b1bf..0b1877329408ec 100644 --- a/validator/src/main.rs +++ b/validator/src/main.rs @@ -1594,7 +1594,6 @@ pub fn main() { } else { &ledger_path }; - let snapshots_dir = fs::canonicalize(snapshots_dir).unwrap_or_else(|err| { eprintln!( "Failed to canonicalize snapshots path '{}': {err}", From 94ccec054e6fc1a96efe1ff29c61e0505f096834 Mon Sep 17 00:00:00 2001 From: yihau Date: Wed, 26 Jun 2024 22:35:52 +0800 Subject: [PATCH 4/4] align --- validator/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/validator/src/main.rs b/validator/src/main.rs index 0b1877329408ec..82de6c8e9a4288 100644 --- a/validator/src/main.rs +++ b/validator/src/main.rs @@ -1607,7 +1607,7 @@ pub fn main() { { eprintln!( "Failed: The --accounts and --snapshots paths must be unique since they \ - both create 'snapshots' subdirectories, otherwise there may be collisions", + both create 'snapshots' subdirectories, otherwise there may be collisions", ); exit(1); }