Clarify --enable-banking-trace with better naming

Author: ryoqun

banking-bench/src/main.rs

@@ -8,7 +8,7 @@ use {
     solana_client::connection_cache::ConnectionCache,
     solana_core::{
         banking_stage::BankingStage,
-        banking_trace::{BankingPacketBatch, BankingTracer, DEFAULT_BANKING_TRACE_SIZE},
+        banking_trace::{BankingPacketBatch, BankingTracer, BANKING_TRACE_DIR_DEFAULT_BYTE_LIMIT},
     },
     solana_gossip::cluster_info::{ClusterInfo, Node},
     solana_ledger::{
@@ -419,7 +419,7 @@ fn main() {
         let banking_tracer = BankingTracer::new(matches.is_present("trace_banking").then_some((
             blockstore.banking_trace_path(),
             exit.clone(),
-            DEFAULT_BANKING_TRACE_SIZE,
+            BANKING_TRACE_DIR_DEFAULT_BYTE_LIMIT,
         )))
         .unwrap();
         let (non_vote_sender, non_vote_receiver) = banking_tracer.create_channel_non_vote();

core/benches/banking_trace.rs

@@ -8,7 +8,7 @@ use {
             drop_and_clean_temp_dir_unless_suppressed, sample_packet_batch, terminate_tracer,
         },
         receiving_loop_with_minimized_sender_overhead, BankingPacketBatch, BankingTracer,
-        TraceError, TracerThreadResult, DEFAULT_BANKING_TRACE_SIZE,
+        TraceError, TracerThreadResult, BANKING_TRACE_DIR_DEFAULT_BYTE_LIMIT,
     },
     std::{
         path::PathBuf,
@@ -61,7 +61,7 @@ fn bench_banking_tracer_main_thread_overhead_under_peak_write(bencher: &mut Benc
     let tracer = BankingTracer::new(Some((
         temp_dir.path().join("banking-trace"),
         exit.clone(),
-        DEFAULT_BANKING_TRACE_SIZE,
+        BANKING_TRACE_DIR_DEFAULT_BYTE_LIMIT,
     )))
     .unwrap();
     let (non_vote_sender, non_vote_receiver) = tracer.create_channel_non_vote();

core/src/banking_trace.rs

@@ -29,6 +29,7 @@ pub type BankingPacketSender = TracedSender;
 pub type BankingPacketReceiver = Receiver<BankingPacketBatch>;
 pub type TracerThreadResult = Result<(), TraceError>;
 pub type TracerThread = Option<JoinHandle<TracerThreadResult>>;
+pub type DirByteLimit = u64;
 
 #[derive(Error, Debug)]
 pub enum TraceError {
@@ -41,17 +42,17 @@ pub enum TraceError {
     #[error("Integer Cast Error: {0}")]
     IntegerCastError(#[from] std::num::TryFromIntError),
 
-    #[error("Trace size is too small (must be larger than {1}): {0}")]
-    TooSmallTraceSize(u64, u64),
+    #[error("dir byte limit is too small (must be larger than {1}): {0}")]
+    TooSmallDirByteLimit(DirByteLimit, DirByteLimit),
 }
 
 const BASENAME: &str = "events";
 const TRACE_FILE_ROTATE_COUNT: u64 = 14; // target 2 weeks retention under normal load
 const TRACE_FILE_WRITE_INTERVAL_MS: u64 = 100;
 const BUF_WRITER_CAPACITY: usize = 10 * 1024 * 1024;
 pub const TRACE_FILE_DEFAULT_ROTATE_BYTE_THRESHOLD: u64 = 1024 * 1024 * 1024;
-pub const EMPTY_BANKING_TRACE_SIZE: u64 = 0;
-pub const DEFAULT_BANKING_TRACE_SIZE: u64 =
+pub const DISABLED_BAKING_TRACE_DIR: DirByteLimit = 0;
+pub const BANKING_TRACE_DIR_DEFAULT_BYTE_LIMIT: DirByteLimit =
     TRACE_FILE_DEFAULT_ROTATE_BYTE_THRESHOLD * TRACE_FILE_ROTATE_COUNT;
 
 #[allow(clippy::type_complexity)]
@@ -168,13 +169,13 @@ pub fn receiving_loop_with_minimized_sender_overhead<T, E, const SLEEP_MS: u64>(
 
 impl BankingTracer {
     pub fn new(
-        maybe_config: Option<(PathBuf, Arc<AtomicBool>, u64)>,
+        maybe_config: Option<(PathBuf, Arc<AtomicBool>, DirByteLimit)>,
     ) -> Result<Arc<Self>, TraceError> {
         let enabled_tracer = maybe_config
             .map(|(path, exit, total_size)| {
                 let rotate_threshold_size = total_size / TRACE_FILE_ROTATE_COUNT;
                 if rotate_threshold_size == 0 {
-                    return Err(TraceError::TooSmallTraceSize(
+                    return Err(TraceError::TooSmallDirByteLimit(
                         total_size,
                         TRACE_FILE_ROTATE_COUNT,
                     ));
@@ -451,7 +452,8 @@ mod tests {
         let temp_dir = TempDir::new().unwrap();
         let path = temp_dir.path().join("banking-trace");
         let exit = Arc::<AtomicBool>::default();
-        let tracer = BankingTracer::new(Some((path, exit.clone(), u64::max_value()))).unwrap();
+        let tracer =
+            BankingTracer::new(Some((path, exit.clone(), DirByteLimit::max_value()))).unwrap();
         let tracer_thread = tracer.take_tracer_thread_join_handle();
         let (non_vote_sender, non_vote_receiver) = tracer.create_channel_non_vote();
 
@@ -489,8 +491,12 @@ mod tests {
         let temp_dir = TempDir::new().unwrap();
         let path = temp_dir.path().join("banking-trace");
         let exit = Arc::<AtomicBool>::default();
-        let tracer =
-            BankingTracer::new(Some((path.clone(), exit.clone(), u64::max_value()))).unwrap();
+        let tracer = BankingTracer::new(Some((
+            path.clone(),
+            exit.clone(),
+            DirByteLimit::max_value(),
+        )))
+        .unwrap();
         let (non_vote_sender, non_vote_receiver) = tracer.create_channel_non_vote();
 
         let dummy_main_thread = thread::spawn(move || {

core/src/validator.rs

@@ -4,7 +4,7 @@ pub use solana_perf::report_target_features;
 use {
     crate::{
         accounts_hash_verifier::AccountsHashVerifier,
-        banking_trace::BankingTracer,
+        banking_trace::{self, BankingTracer},
         broadcast_stage::BroadcastStageType,
         cache_block_meta_service::{CacheBlockMetaSender, CacheBlockMetaService},
         cluster_info_vote_listener::VoteTracker,
@@ -176,7 +176,7 @@ pub struct ValidatorConfig {
     pub ledger_column_options: LedgerColumnOptions,
     pub runtime_config: RuntimeConfig,
     pub replay_slots_concurrently: bool,
-    pub banking_trace_size: u64,
+    pub banking_trace_dir_byte_limit: banking_trace::DirByteLimit,
 }
 
 impl Default for ValidatorConfig {
@@ -238,7 +238,7 @@ impl Default for ValidatorConfig {
             ledger_column_options: LedgerColumnOptions::default(),
             runtime_config: RuntimeConfig::default(),
             replay_slots_concurrently: false,
-            banking_trace_size: 0,
+            banking_trace_dir_byte_limit: 0,
         }
     }
 }
@@ -930,12 +930,13 @@ impl Validator {
             exit.clone(),
         );
 
-        let banking_tracer = BankingTracer::new((config.banking_trace_size > 0).then_some((
-            blockstore.banking_trace_path(),
-            exit.clone(),
-            config.banking_trace_size,
-        )))
-        .map_err(|err| format!("{} [{:?}]", &err, &err))?;
+        let banking_tracer =
+            BankingTracer::new((config.banking_trace_dir_byte_limit > 0).then_some((
+                blockstore.banking_trace_path(),
+                exit.clone(),
+                config.banking_trace_dir_byte_limit,
+            )))
+            .map_err(|err| format!("{} [{:?}]", &err, &err))?;
 
         let (replay_vote_sender, replay_vote_receiver) = unbounded();
         let tvu = Tvu::new(

local-cluster/src/validator_configs.rs

@@ -63,7 +63,7 @@ pub fn safe_clone_config(config: &ValidatorConfig) -> ValidatorConfig {
         ledger_column_options: config.ledger_column_options.clone(),
         runtime_config: config.runtime_config.clone(),
         replay_slots_concurrently: config.replay_slots_concurrently,
-        banking_trace_size: config.banking_trace_size,
+        banking_trace_dir_byte_limit: config.banking_trace_dir_byte_limit,
     }
 }
 

validator/src/cli.rs

@@ -10,7 +10,7 @@ use {
         },
         keypair::SKIP_SEED_PHRASE_VALIDATION_ARG,
     },
-    solana_core::banking_trace::DEFAULT_BANKING_TRACE_SIZE,
+    solana_core::banking_trace::{DirByteLimit, BANKING_TRACE_DIR_DEFAULT_BYTE_LIMIT},
     solana_faucet::faucet::FAUCET_PORT,
     solana_net_utils::{MINIMUM_VALIDATOR_PORT_RANGE_WIDTH, VALIDATOR_PORT_RANGE},
     solana_rpc::{rpc::MAX_REQUEST_BODY_SIZE, rpc_pubsub_service::PubSubConfig},
@@ -1306,13 +1306,22 @@ pub fn app<'a>(version: &'a str, default_args: &'a DefaultArgs) -> App<'a, 'a> {
                 .help("Allow concurrent replay of slots on different forks")
         )
         .arg(
-            Arg::with_name("banking_trace_size")
+            Arg::with_name("banking_trace_dir_byte_limit")
+                // expose friendly alternative name to cli than internal
+                // implementation-oriented one
                 .long("enable-banking-trace")
                 .value_name("MAX_BYTES")
-                .validator(is_parsable::<u64>)
-                .takes_value(true)
-                .default_value(&default_args.banking_trace_size)
-                .help("Write trace files for simulate-leader-blocks, retaining up to the maximum total bytes")
+                .validator(is_parsable::<DirByteLimit>)
+                .takes_value(true)
+                // Firstly, zero limit value causes tracer to be disabled
+                // altogether, intuitively. On the other hand, this non-zero
+                // default doesn't enable banking tracer unless this flag is
+                // explicitly given, similar to --limit-ledger-size.
+                // see configure_banking_trace_dir_byte_limit() for this.
+                .default_value(&default_args.banking_trace_dir_byte_limit)
+                .help("Write trace files for simulate-leader-blocks, retaining \
+                       up to the default or specified total bytes in the \
+                       ledger")
         )
         .args(&get_deprecated_arguments())
         .after_help("The default subcommand is run")
@@ -1656,7 +1665,7 @@ pub struct DefaultArgs {
     pub wait_for_restart_window_min_idle_time: String,
     pub wait_for_restart_window_max_delinquent_stake: String,
 
-    pub banking_trace_size: String,
+    pub banking_trace_dir_byte_limit: String,
 }
 
 impl DefaultArgs {
@@ -1735,7 +1744,7 @@ impl DefaultArgs {
             exit_max_delinquent_stake: "5".to_string(),
             wait_for_restart_window_min_idle_time: "10".to_string(),
             wait_for_restart_window_max_delinquent_stake: "5".to_string(),
-            banking_trace_size: DEFAULT_BANKING_TRACE_SIZE.to_string(),
+            banking_trace_dir_byte_limit: BANKING_TRACE_DIR_DEFAULT_BYTE_LIMIT.to_string(),
         }
     }
 }

validator/src/main.rs

@@ -8,7 +8,7 @@ use {
     rand::{seq::SliceRandom, thread_rng},
     solana_clap_utils::input_parsers::{keypair_of, keypairs_of, pubkey_of, value_of},
     solana_core::{
-        banking_trace::EMPTY_BANKING_TRACE_SIZE,
+        banking_trace::DISABLED_BAKING_TRACE_DIR,
         ledger_cleanup_service::{DEFAULT_MAX_LEDGER_SHREDS, DEFAULT_MIN_MAX_LEDGER_SHREDS},
         system_monitor_service::SystemMonitorService,
         tower_storage,
@@ -410,6 +410,21 @@ fn get_cluster_shred_version(entrypoints: &[SocketAddr]) -> Option<u16> {
     None
 }
 
+fn configure_banking_trace_dir_byte_limit(
+    validator_config: &mut ValidatorConfig,
+    matches: &ArgMatches,
+) {
+    validator_config.banking_trace_dir_byte_limit =
+        if matches.occurrences_of("banking_trace_dir_byte_limit") == 0 {
+            // disable with no explicit flag; then, this effectively becomes `opt-in` even if we're
+            // specifying a default value in clap configuration.
+            DISABLED_BAKING_TRACE_DIR
+        } else {
+            // BANKING_TRACE_DIR_DEFAULT_BYTE_LIMIT or user-supplied override value
+            value_t_or_exit!(matches, "banking_trace_dir_byte_limit", u64)
+        };
+}
+
 pub fn main() {
     let default_args = DefaultArgs::new();
     let solana_version = solana_version::version!();
@@ -1338,14 +1353,7 @@ pub fn main() {
         validator_config.max_ledger_shreds = Some(limit_ledger_size);
     }
 
-    validator_config.banking_trace_size = if matches.occurrences_of("banking_trace_size") == 0 {
-        // disable with no explicit flag; then, this effectively becomes `opt-in` even if we're
-        // specifying a default value in clap configuration.
-        EMPTY_BANKING_TRACE_SIZE
-    } else {
-        // DEFAULT_BANKING_TRACE_SIZE or user-supplied override value
-        value_t_or_exit!(matches, "banking_trace_size", u64)
-    };
+    configure_banking_trace_dir_byte_limit(&mut validator_config, &matches);
 
     validator_config.ledger_column_options = LedgerColumnOptions {
         compression_type: match matches.value_of("rocksdb_ledger_compression") {