agave-validator: add args tests for run (part 2) #6667

yihau · 2025-06-20T12:28:34Z

Problem

#4082

Summary of Changes

--no-genesis-fetch

extract to ./validator/src/commands/run/args/rpc_bootstrap_config.rs

--no-snapshot-fetch

extract to ./validator/src/commands/run/args/rpc_bootstrap_config.rs

--entrypoint

split the method chain and use IndexSet in the middle steps for ensuring the input order

--check-vote-account

extract to ./validator/src/commands/run/args/rpc_bootstrap_config.rs

--known-validator

port validators_set without an exit func

--only-known-rpc

extract to ./validator/src/commands/run/args/rpc_bootstrap_config.rs

--max-genesis-archive-unpacked-size

extract to ./validator/src/commands/run/args/rpc_bootstrap_config.rs

--no-incremental-snapshots

extract to ./validator/src/commands/run/args/rpc_bootstrap_config.rs

misc

introduce pretty_assertions

codecov-commenter · 2025-06-20T15:08:11Z

Codecov Report

Attention: Patch coverage is 97.58242% with 11 lines in your changes missing coverage. Please review.

Project coverage is 83.3%. Comparing base (9595527) to head (8716726).
Report is 36 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #6667    +/-   ##
========================================
  Coverage    83.3%    83.3%            
========================================
  Files         853      854     +1     
  Lines      378266   378684   +418     
========================================
+ Hits       315409   315813   +404     
- Misses      62857    62871    +14

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

validator/src/commands/run/args.rs

steviez

Finally got to an actual full review on this

validator/src/commands/run/execute.rs

validator/src/commands/run/args.rs

validator/src/commands/run/execute.rs

steviez · 2025-07-01T17:40:02Z

validator/src/commands/mod.rs

@@ -46,7 +46,7 @@ pub mod tests {
    {
        let matches = app.get_matches_from(vec);
        let result = T::from_clap_arg_match(&matches);
-        assert_eq!(result.unwrap(), expected_arg);
+        pretty_assertions::assert_eq!(result.unwrap(), expected_arg);


I'm guessing this makes debugging things easier ?

yeah, it makes the diff much clearer

steviez · 2025-07-01T17:50:00Z

validator/src/commands/run/args.rs

+        let entrypoints = values_t!(matches, "entrypoint", String).unwrap_or_default();
+        let mut parsed_entrypoints = IndexSet::new();
+        for entrypoint in entrypoints {
+            let parsed = solana_net_utils::parse_host_port(&entrypoint).map_err(|err| {
+                Box::<dyn std::error::Error>::from(format!(
+                    "failed to parse entrypoint address: {err}"
+                ))
+            })?;
+            parsed_entrypoints.insert(parsed);
+        }
+        let entrypoints = parsed_entrypoints.into_iter().collect();


For testability, we just need consistent ordering. How about this ?

let mut entrypoints = values_t!(matches, "entrypoint", String).unwrap_or_default(); // sort() + dedup() to yield a vector of unique elements entrypoints.sort(); entrypoints.dedup(); let entrypoints = entrypoints.into_iter().map(|entrypoint| solana_net_utils::parse_host_port(...).map_err(...)).collect();

Technically may be less efficient, but this is a one-time-cost at startup and avoid bringing in another dependency

validator/src/commands/run/args/rpc_bootstrap_config.rs

validator/src/commands/run/args.rs

steviez · 2025-07-01T18:06:44Z

validator/src/commands/run/args/rpc_bootstrap_config.rs

+#[derive(Debug, PartialEq, Clone)]
+pub struct RpcBootstrapConfig {
+    pub no_genesis_fetch: bool,
+    pub no_snapshot_fetch: bool,
+    pub check_vote_account: Option<String>,
+    pub only_known_rpc: bool,
+    pub max_genesis_archive_unpacked_size: u64,
+    pub no_incremental_snapshots: bool,
+}


I'm debating whether we actually need this type or not. It looks just about identical to:

agave/validator/src/bootstrap.rs

Lines 59 to 67 in 898b76e

#[derive(Debug)]

pub struct RpcBootstrapConfig {

pub no_genesis_fetch: bool,

pub no_snapshot_fetch: bool,

pub only_known_rpc: bool,

pub max_genesis_archive_unpacked_size: u64,

pub check_vote_account: Option<String>,

pub incremental_snapshot_fetch: bool,

}

The only difference is the final field, no_incremental_snapshots vs. incremental_snapshot_fetch. IMO, the struct should yield incremental_snapshot_fetch and do the below "conversion" in FromClapArgMatches impl:

incremental_snapshot_fetch: !run_args.rpc_bootstrap_config.no_incremental_snapshots,

If we make that change, we can seemingly just implement FromClapArgMatches on the existing struct directly

my idea is that all RunArgs related structs should be used strictly for input purposes. this ensures that changes to input won’t affect the core logic, and vice versa. if we use a shared argument struct for both input and core functionality, we’ll eventually run into issues where changes are only intended for one side. 🤔

my idea is that all RunArgs related structs should be used strictly for input purposes. this ensures that changes to input won’t affect the core logic, and vice versa

I'm struggling to think of a scenario where we'd want or need this separation. Do you see any situations in our codebase today that would necessitate the separation ?

if we use a shared argument struct for both input and core functionality, we’ll eventually run into issues where changes are only intended for one side

I think of the pre existing struct that I linked is the "input" struct. More so, I think that this config struct should be very simple and have the sole purpose of passing config down to the relevant service. And I have this belief for all the similar structs such as BlockstoreOptions, SnapshotConfig, etc.

okay. I moved RpcBootstrapConfig into args.rs and removed the existing one. 754e36d just want to emphasize that this is intended as an input argument specifically for the run command

okay. I moved RpcBootstrapConfig into args.rs and removed the existing one

Did it have to move ? I thought we could implement the trait on it while keeping it in the original location. I don't see us moving BlockstoreOptions or AccountsDbConfig to be defined in agave-validator crate

just want to emphasize that this is intended as an input argument specifically for the run command

Let's hop on a call to get on the same page here, that will probably be quicker than more GH comments back and forth

This reverts commit 754e36d.

… new introduced one

steviez

Thanks for pushing this one through; think the subsequent ones will be easier now that we have decided on a "pattern"

yihau force-pushed the add-args-run-part-2-new branch 2 times, most recently from 48ea50a to 0b8c2be Compare June 20, 2025 13:56

yihau marked this pull request as ready for review June 20, 2025 15:14

yihau requested a review from steviez June 20, 2025 15:14

steviez reviewed Jun 26, 2025

View reviewed changes

validator/src/commands/run/args.rs Outdated Show resolved Hide resolved

steviez reviewed Jul 1, 2025

View reviewed changes

yihau added 12 commits July 3, 2025 20:38

--no-genesis-fetch

76e0844

--no-snapshot-fetch

ed3f273

use pretty_assertions

7d765ac

--entrypoint

5b7c5ae

--check-vote-account

3b827da

--known-validator

c2c1f2c

--only-known-rpc

9228047

--max-genesis-archive-unpacked-size

ef0e83d

--no-incremental-snapshots

7ec4931

use run_args.known_validators directly

a910aa6

remove indexmap dep

d3e3803

test only default

23b13ed

yihau force-pushed the add-args-run-part-2-new branch from 0b8c2be to 23b13ed Compare July 3, 2025 13:24

yihau added 3 commits July 8, 2025 21:43

move RpcBootstrapConfig from bootstarp.rs to args.rs

754e36d

Revert "move RpcBootstrapConfig from bootstarp.rs to args.rs"

fe31cb0

This reverts commit 754e36d.

impl FromClapArgMatches for existing RpcBootstrapConfg and remove the…

8716726

… new introduced one

steviez approved these changes Jul 9, 2025

View reviewed changes

yihau merged commit 2546ea6 into anza-xyz:master Jul 10, 2025
52 checks passed

yihau deleted the add-args-run-part-2-new branch July 10, 2025 04:07

	#[derive(Debug)]
	pub struct RpcBootstrapConfig {
	pub no_genesis_fetch: bool,
	pub no_snapshot_fetch: bool,
	pub only_known_rpc: bool,
	pub max_genesis_archive_unpacked_size: u64,
	pub check_vote_account: Option<String>,
	pub incremental_snapshot_fetch: bool,
	}

agave-validator: add args tests for run (part 2) #6667

agave-validator: add args tests for run (part 2) #6667

Uh oh!

Conversation

yihau commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

--no-genesis-fetch

--no-snapshot-fetch

--entrypoint

--check-vote-account

--known-validator

--only-known-rpc

--max-genesis-archive-unpacked-size

--no-incremental-snapshots

misc

Uh oh!

codecov-commenter commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

steviez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steviez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yihau commented Jun 20, 2025 •

edited

Loading

codecov-commenter commented Jun 20, 2025 •

edited

Loading