Show config options #346

Dodecahedr0x · 2025-04-15T17:53:48Z

Adds a CLI to start the validator. It has a -h command to display configuration and default values. The original interface is preserved to avoid breaking integrations.

CLOSES: #318

Greptile Summary

Major refactoring to add CLI configuration support using clap, maintaining compatibility with existing config.toml and environment variables. However, maintainers recommend pivoting to a simpler approach.

Added new CLI modules in test-bins/src/cli/ with structured argument parsing for accounts, metrics, RPC, and validator settings
Fixed typo in environment variable name from VALIDATOR_FDQN to VALIDATOR_FQDN in magicblock-config/src/lib.rs
Reduced CHANNEL_CAPACITY_DEFAULT from 250,000 to 1,024 in magicblock-geyser-plugin/src/config.rs
⚠️ Current implementation introduces maintenance burden by duplicating configuration across CLI args, env vars, and TOML
⚠️ Maintainers suggest generating args/env vars from a single source instead of the current approach

_{💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!}

greptile-apps

_{2 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-04-15T17:55:32Z

test-bins/src/rpc.rs

+    #[arg(
+        long,
+        value_name = "COMPONENTS",
+        default_value = "(accounts,transactions)",


logic: default_value is set but the type is Option<String> - this will always contain a value and never be None

greptile-apps · 2025-04-15T17:55:33Z

test-bins/src/rpc.rs

+    // Set environment variables from CLI arguments
+    if let Some(keypair) = cli.keypair {
+        std::env::set_var("VALIDATOR_KEYPAIR", keypair);
+    }
+
+    if let Some(disable_geyser) = cli.disable_geyser {
+        std::env::set_var("GEYSER_DISABLE", disable_geyser);
+    }
+
+    if let Some(disable_cache) = cli.disable_geyser_cache {
+        std::env::set_var("GEYSER_CACHE_DISABLE", disable_cache);
+    }


logic: setting env vars after parsing them from CLI args could cause issues with override_from_envs() later picking up these modified values unexpectedly

greptile-apps

LGTM

_{2 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings | Greptile}

thlorenz

Thank for the PR.
I'm not sure that in the long run this makes sense since we're now starting to duplicate some configs that we can either provide via the config file or env vars.

I thought we agreed at some point to find a way to not have to repeat ourselves, but to generate args/env vars + config from one common source so we can maintain that in one place.
I realize that the issue doesn't explain that part at all, but I don't think it's a good idea to add an args implementation that we'll replace most likely fairly soon.

If you want a stop gap for now I think it'd be more useful to create a heavily documented config toml which also mentions the related env vars.
You could then also print that when the user passes -h.

And for that one arg (-h) we don't need to pull in clap, but can manually parse the args for that.

greptile-apps

_{1 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-04-16T04:08:25Z

test-bins/src/rpc.rs

+    /// Path to the configuration file
+    config: Option<String>,


style: config parameter needs a long flag (--config) and better help text for discoverability

greptile-apps · 2025-04-16T04:08:25Z

test-bins/src/rpc.rs

        Keypair::from_base58_string(&keypair)
    } else {


logic: from_base58_string() can panic - needs error handling for invalid base58 strings

Suggested change

Keypair::from_base58_string(&keypair)

} else {

Keypair::from_base58_string(&keypair).unwrap_or_else(|_| {

panic!("Invalid base58 string provided for validator keypair");

})

} else {

taco-paco · 2025-04-16T04:12:10Z

test-bins/src/rpc.rs

+#[command(about = "Runs a MagicBlock validator node")]
+struct Cli {
+    /// Path to the configuration file
+    config: Option<String>,


Clap shall support PathBuf. Also I would propose to consider config_path.

bmuddha

looks good 👍🏽

Dodecahedr0x · 2025-06-03T08:55:37Z

I kept the argument parsing separate from the actual config to 1) prevent including clap/structop in the config crate 2) reduce changes to the config that could have downstream impacts 3) keep the logic for deserializing the config file separate from the overriding logic.

greptile-apps

_{12 file(s) reviewed, 17 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-06-03T08:56:28Z

test-bins/Cargo.toml

@@ -9,14 +9,21 @@ edition.workspace = true
 default-run = "rpc"

 [dependencies]
+clap = { version = "4.5.36", features = ["derive", "env"] }


style: Consider using clap's workspace dependency pattern to match other dependencies

Yes please do. No dep in any crate should define its own version for compatibility.
There are more cases in this PR which greptile pointed out and the same applies there.

Please follow the patterns we established in the codebase in all cases.

greptile-apps · 2025-06-03T08:56:29Z

test-bins/Cargo.toml

+url = { workspace = true }
+
+[dev-dependencies]
+serial_test = { version = "3.2.0", features = ["logging"] }


style: Consider using workspace dependency pattern for serial_test to maintain consistency

greptile-apps · 2025-06-03T08:56:39Z

test-bins/src/cli/accounts.rs

+        config.accounts.allowed_programs.extend(
+            self.allowed_programs.clone().into_iter().map(|id| {
+                AllowedProgram {
+                    id: Pubkey::from_str_const(&id),


logic: Using from_str_const() for program IDs is unsafe - should use regular from_str() and handle potential errors

Suggested change

id: Pubkey::from_str_const(&id),

id: Pubkey::from_str(&id).map_err(|e| format!("Invalid program ID {}: {}", id, e))?,

greptile-apps · 2025-06-03T08:56:40Z

test-bins/src/cli/accounts.rs

+    /// The payer init balance in lamports.
+    #[arg(
+        long = "accounts-payer-init-lamports",
+        env = "INIT_LAMPORTS",


style: Environment variable INIT_LAMPORTS should be prefixed with ACCOUNTS_ for consistency with other env vars

Suggested change

env = "INIT_LAMPORTS",

env = "ACCOUNTS_INIT_LAMPORTS",

I agree, we need to be consistent, otherwise config in different ways will be very confusing.
Again if this was derived, the exact matching of option naming would come for free.

greptile-apps · 2025-06-03T08:56:41Z

test-bins/src/cli/accounts.rs

+        config.accounts.lifecycle = self
+            .lifecycle
+            .unwrap_or(config.accounts.lifecycle.clone().into())
+            .into();


style: Unnecessary clone() call on lifecycle - LifecycleMode likely implements Copy

greptile-apps · 2025-06-03T08:57:38Z

test-bins/src/cli/mod.rs

+fn keypair_parser(s: &str) -> Result<KeypairWrapper, String> {
+    let keypair = Keypair::from_base58_string(s);
+    Ok(KeypairWrapper(keypair))
+}


logic: No error handling for invalid base58 strings. Should return Err instead of panicking.

Suggested change

fn keypair_parser(s: &str) -> Result<KeypairWrapper, String> {

let keypair = Keypair::from_base58_string(s);

Ok(KeypairWrapper(keypair))

}

fn keypair_parser(s: &str) -> Result<KeypairWrapper, String> {

let keypair = Keypair::from_base58_string(s).map_err(|_| "Invalid base58 string for keypair".to_string())?;

Ok(KeypairWrapper(keypair))

}

If this is only used for the config which is processed during startup then panicking maybe ok.

greptile-apps · 2025-06-03T08:57:39Z

test-bins/src/cli/mod.rs

+        help = "Specifies geyser components to disable. [default: (accounts,transactions)]"
+    )]


style: Help text mentions default value but doesn't specify it in the #[arg] attribute using default_value

I'm not sure if this should be part of the official config. I remember adding those as dev tools, thus that we could turn on/off parts while debugging memory leaks and such.
They were supposed to be only used via env vars and never in prod, as otherwise the validator doesn't fully work.

greptile-apps · 2025-06-03T08:57:46Z

test-bins/src/rpc.rs

    let mut api =
-        MagicValidator::try_from_config(config, validator_keypair).unwrap();
+        MagicValidator::try_from_config(config, cli.keypair.0).unwrap();


logic: unwrap() call could panic - should handle errors gracefully here

Suggested change

let mut api =

MagicValidator::try_from_config(config, validator_keypair).unwrap();

MagicValidator::try_from_config(config, cli.keypair.0).unwrap();

let mut api = MagicValidator::try_from_config(config, cli.keypair.0)

.map_err(|e| {

error!("Failed to create validator: {}", e);

std::process::exit(1);

})?;

greptile-apps · 2025-06-03T08:57:48Z

test-bins/src/cli/validator.rs

+    pub sigverify: Option<bool>,
+
+    /// Fully Qualified Domain Name. If specified it will also register ER on chain
+    #[arg(long = "validator-fdqn", env = "VALIDATOR_FDQN")]


syntax: FQDN is misspelled as 'fdqn' in both the field name and CLI argument. Should be 'fqdn'

Suggested change

#[arg(long = "validator-fdqn", env = "VALIDATOR_FDQN")]

#[arg(long = "validator-fqdn", env = "VALIDATOR_FQDN")]

It was already misspelled, but could be a good time to fix it

greptile-apps · 2025-06-03T08:57:49Z

test-bins/src/cli/validator.rs

+                .millis_per_slot
+                .unwrap_or(config.validator.millis_per_slot),
+            sigverify: self.sigverify.unwrap_or(config.validator.sigverify),
+            fdqn: self.fdqn.clone(),


style: Using clone() on an Option<String> can be replaced with .cloned()

GabrielePicco

LGTM in general! Eventually, I believe delegating the parsing and overrides to Clap would simplify the codebase, but this is already a solid improvement.

Dodecahedr0x · 2025-06-05T08:07:52Z

Rebased the pubsub redesign, fixed the typo, and fixed a few greptile comments

thlorenz · 2025-06-07T01:17:04Z

There are still a lot of very valid greptile comments that are unaddressed.
I am currently reviewing this, but will not repeat greptiles suggestions, thus you could fix those in the meantime please.

thlorenz

I'm sorry, but I believe in order to get args as a feature, we are making the maintenance worse.
We currently have a maintenance problem as we are maintaining a config file + env vars.
However this PR doesn't improve on that issue and instead makes the maintenance problem greater.

I already pointed that out a few months back.

I believe we should take the time to properly derive args and env vars from a toml config file specification or similar. Or we could derive a config file from some clap definitions, whichever is easier.
It shouldn't be too hard to generate code either via macros or at build time from a toml file that specifies all our config options.

Unless this feature is super urgent I'd prefer to get it right the first time even if it takes more time todo instead of creating a lot of future maintenance/debugging etc. for us in the future.

Also there are tons of unaddressed greptile warnings which point out valid issues and I upvoted them in those cases.

I also discovered some naming issues (hard to catch and another indicator that repeating ourselves like this is not the right way to go).

thlorenz · 2025-06-07T01:21:07Z

test-bins/Cargo.toml

@@ -9,14 +9,21 @@ edition.workspace = true
 default-run = "rpc"

 [dependencies]
+clap = { version = "4.5.36", features = ["derive", "env"] }


Yes please do. No dep in any crate should define its own version for compatibility.
There are more cases in this PR which greptile pointed out and the same applies there.

Please follow the patterns we established in the codebase in all cases.

thlorenz · 2025-06-07T01:23:36Z

test-bins/src/cli/accounts.rs

+    /// Use a custom remote URL with WebSocket URL
+    #[arg(
+        long = "accounts-remote-custom-with-ws",
+        env = "ACCOUNTS_REMOTE_WS",


Conflicting args name with env name (not sure what the config name is).
Env name doesn't have custom-with, args name does.

thlorenz · 2025-06-07T01:27:05Z

test-bins/src/cli/accounts.rs

+        conflicts_with = "remote",
+        requires = "remote_custom"
+    )]
+    pub remote_custom_with_ws: Option<String>,


Could you please add empty lines between each arg, this is very hard to read.

thlorenz · 2025-06-07T01:27:36Z

test-bins/src/cli/accounts.rs

+    pub remote_custom_with_ws: Option<String>,
+    #[arg(long = "accounts-lifecycle", env = "ACCOUNTS_LIFECYCLE")]
+    pub lifecycle: Option<LifecycleModeArg>,
+    #[command(flatten)]


Ideally keep the commands separate from plain args for readability

thlorenz · 2025-06-07T01:29:21Z

test-bins/src/cli/accounts.rs

+    ) -> Result<(), String> {
+        if let Some(remote) = self.remote {
+            config.accounts.remote = match remote {
+                RemoteConfigArg::Devnet => RemoteConfig::Devnet,


Here we see the duplication of every single option which we will have to maintain in the future.
It'd be a lot better if we could derive one from the other.

thlorenz · 2025-06-07T02:34:54Z

test-bins/src/cli/mod.rs

+    }
+
+    #[test]
+    #[serial]


I don't understand why this has to run in series?
The test is isolated and doesn't affect the machine environment, nor does it startup a validator.
I believe we can run those in parallel which is faster and thus can also remove the dependency on the crate that runs tests in series.

At this point all tests that start up a validator are part of the test-integration and are instrumented to run in series already, thus if you have a test that requires starting up a validator then add it there.

Again just to test configuration that should not be needed.

thlorenz · 2025-06-07T02:35:29Z

test-bins/src/cli/mod.rs

+    }
+
+    #[test]
+    #[serial]


ditto can run in parallel

thlorenz · 2025-06-07T02:37:35Z

test-bins/src/cli/rpc.rs

+        config.rpc = RpcConfig {
+            addr: self.addr.unwrap_or(config.rpc.addr),
+            port: self.port.unwrap_or(config.rpc.port),
+        }


That is actually a bad idea by greptile as after that change it would not fail to compile if we add future fields, but that's actually what we want to help with the maintenance overhead that we are creating with this PR.

thlorenz · 2025-06-07T02:39:13Z

test-bins/src/cli/validator.rs

+    pub millis_per_slot: Option<u64>,
+
+    /// Whether to verify transaction signatures.
+    #[arg(long = "validator-sigverify", env = "VALIDATOR_SIG_VERIFY")]


arg and env var naming doesn't match sigverify vs SIG_VERIFY (should be sig-verify).

Again those little mistakes will bite us in the future. So the PR itself is already showing what kind of maintenance nightmare we're creating for ourselves here.

thlorenz · 2025-06-07T02:42:48Z

test-bins/src/rpc.rs

-    9, 208, 183, 189, 108, 200, 89, 77, 168, 76, 233, 197, 132, 22, 21, 186,
-    202, 240, 105, 168, 157, 64, 233, 249, 100, 104, 210, 41, 83, 87,
-];
+mod cli;


I highly discourage supporting the validator keypair as a config entry (worst case) or as an arg.

In both cases it will be visible either in a file or in machine logs (args are logged) which is a huge attack vector as it encourages operators to just add them in plain text either to a file or in shell scripts as an arg. Secrets should always only be env vars.

I had good reason to only allow passing this as an env var which are not logged, nor are added to a file.

greptile-apps

_{49 file(s) reviewed, 3 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-06-07T02:54:50Z

test-bins/src/cli/ledger.rs

+            reset: self.reset.unwrap_or(config.ledger.reset),
+            path: self.path.clone().or(config.ledger.path.clone()),
+            size: self.size.unwrap_or(config.ledger.size),


style: Clone is called on Option<String> before or() - more efficient to move and use or_else()

greptile-apps · 2025-06-07T02:55:13Z

test-bins/src/cli/mod.rs

+        assert_eq!(config.ledger.path, Some(ledger_path.to_string()));
+        assert_eq!(config.ledger.size, ledger_size.parse::<u64>().unwrap());
+        assert_eq!(config.programs, vec![]);
+        assert_eq!(config.validator.fdqn, Some(validator_fqdn.to_string()));


syntax: There's a typo in the field name 'fdqn' - should be 'fqdn' (Fully Qualified Domain Name)

greptile-apps · 2025-06-07T02:55:20Z

test-bins/src/cli/validator.rs

+                .millis_per_slot
+                .unwrap_or(config.validator.millis_per_slot),
+            sigverify: self.sigverify.unwrap_or(config.validator.sigverify),
+            fdqn: self.fdqn.clone(),


logic: Unlike other fields, fdqn directly uses clone() without unwrap_or(), breaking the pattern of preserving existing config values when CLI arg is None

greptile-apps

_{13 file(s) reviewed, 1 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-06-07T03:24:24Z

test-bins/src/cli/accounts.rs

+    #[arg(long = "accounts-db-size")]
+    pub db_size: Option<usize>,


style: Missing environment variable support for accounts-db-size. Should add env = "ACCOUNTS_DB_SIZE" for consistency with other params.

Suggested change

#[arg(long = "accounts-db-size")]

pub db_size: Option<usize>,

#[arg(long = "accounts-db-size", env = "ACCOUNTS_DB_SIZE")]

pub db_size: Option<usize>,

Dodecahedr0x · 2025-07-11T06:59:39Z

Superseded by #447

This PR is another attempt to allow layered configuration (default -> file -> args -> env) while providing users with information on what parameters they can adjust. It overrides #346 with the following advantages: - Automatically derives CLI and env var names to avoid repetitions and reduce risk of typos - Single definition of the configuration The weakness of this approach is that it utilizes small procedural macros to avoid repetition, which can compromise readability and make debugging more challenging. It introduces a breaking change in the format of the remote because Clap does not accept an enum variant with complex values.  ## Greptile Summary Major refactoring of configuration management using Clap for CLI support, introducing layered configuration (default -> file -> args -> env) with automatic CLI and environment variable derivation. - Added `magicblock-config-macro` crate with procedural macros `clap_prefix` and `clap_from_serde` for automatic CLI argument generation - Changed remote configuration format from `remote = "devnet"` to `remote.cluster = "devnet"` across all TOML files (breaking change) - Consolidated configuration types from individual crates into centralized `magicblock-config` crate - Fixed FQDN typo in validator config (renamed from `fdqn` to `fqdn`) - Added merge functionality to all config structs for layered configuration support

Dodecahedr0x requested review from thlorenz, GabrielePicco and bmuddha April 15, 2025 17:53

greptile-apps bot reviewed Apr 15, 2025

View reviewed changes

thlorenz requested changes Apr 16, 2025

View reviewed changes

greptile-apps bot reviewed Apr 16, 2025

View reviewed changes

taco-paco reviewed Apr 16, 2025

View reviewed changes

bmuddha approved these changes Apr 16, 2025

View reviewed changes

Dodecahedr0x force-pushed the feat/show-config-options branch 2 times, most recently from 346c8b1 to adec389 Compare June 2, 2025 14:42

Dodecahedr0x requested review from taco-paco, thlorenz and bmuddha June 3, 2025 08:55

Dodecahedr0x changed the title ~~feat(test-bins): show config options~~ Show config options Jun 3, 2025

greptile-apps bot reviewed Jun 3, 2025

View reviewed changes

GabrielePicco approved these changes Jun 4, 2025

View reviewed changes

Dodecahedr0x added 12 commits June 5, 2025 10:06

feat(test-bins): show config options

f75ed2a

style(test-bins): linting greptile recommendation

5770060

feat(test-bins): parse all config parameters

f7cf445

style(test-bins): linting

4ba94ab

refactor(test-bins): split modules

dbdc831

test(test-bins): fix env conflicts

591a79b

feat(test-bins): handle keypair and overidding from cli

a9e3ac7

style(test-bins): linting

a834349

test(test-bins): fix test using clap

458dfb7

chore(test-bins): remove launch.json

398b3ea

fix(test-bins): override relevant args

bb6bd59

feat(config): load config using Path

94c06dc

Dodecahedr0x added 2 commits June 5, 2025 10:06

fix(config): correct typo in validator config fdqn -> fqdn

1d50da0

feat(test-bins): address some greptile comments

50cec8d

Dodecahedr0x force-pushed the feat/show-config-options branch from 09a36f5 to 50cec8d Compare June 5, 2025 08:06

Dodecahedr0x added 2 commits June 5, 2025 16:13

test(test-bins): test parsing without clap

35e1de7

style(test-bins): linting

560c19d

thlorenz requested changes Jun 7, 2025

View reviewed changes

greptile-apps bot reviewed Jun 7, 2025

View reviewed changes

Dodecahedr0x mentioned this pull request Jul 10, 2025

feat: CLI configuration #447

Merged

Dodecahedr0x closed this Jul 11, 2025

bmuddha deleted the feat/show-config-options branch July 12, 2025 09:32

	id: Pubkey::from_str_const(&id),
	id: Pubkey::from_str(&id).map_err(\|e\| format!("Invalid program ID {}: {}", id, e))?,

		help = "Specifies geyser components to disable. [default: (accounts,transactions)]"
		)]

-    let mut api =
-        MagicValidator::try_from_config(config, validator_keypair).unwrap();
-        MagicValidator::try_from_config(config, cli.keypair.0).unwrap();
+    let mut api = MagicValidator::try_from_config(config, cli.keypair.0)
+        .map_err(|e| {
+            error!("Failed to create validator: {}", e);
+            std::process::exit(1);
+        })?;

	#[arg(long = "validator-fdqn", env = "VALIDATOR_FDQN")]
	#[arg(long = "validator-fqdn", env = "VALIDATOR_FQDN")]

		#[arg(long = "accounts-db-size")]
		pub db_size: Option<usize>,

Show config options #346

Show config options #346

Uh oh!

Conversation

Dodecahedr0x commented Apr 15, 2025 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

thlorenz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmuddha left a comment

Choose a reason for hiding this comment

Uh oh!

Dodecahedr0x commented Jun 3, 2025

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dodecahedr0x commented Apr 15, 2025 •

edited by greptile-apps bot

Loading

thlorenz left a comment •

edited

Loading

thlorenz left a comment •

edited

Loading