Update config and args #669

bewakes · 2025-02-13T10:00:33Z

Description

This PR does the following:

Remove a lot of config args from strata-client's Args struct. It now only accepts --datadir, --config, --rollup-params, --rpc-host, --rpc-port and generic override param -o.
The usage of -o is something like ./strata-client --config configfile.toml -o bitcoind.rpc_user=user -o btcio.reader.client_poll_dur_ms=1000 -o exec.reth.rpc_url=http://reth
Anything passed via -o will override the contents from config file and the default values.
Add factory/config.py module corresponding to strata_config::Config with default values set and conversion to toml.

Note that the previous args passing no longer works and this has impacts on deployment too. Possibly need to update docker stuffs as well.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactor
New or updated tests
Dependency Update

Notes to Reviewers

Checklist

I have performed a self-review of my code.
I have commented my code where necessary.
I have updated the documentation if needed.
My changes do not introduce new warnings.
I have added (where necessary) tests that prove my changes are effective or that my feature works.
New and existing tests pass with my changes.

Related Issues

codecov · 2025-02-13T10:08:18Z

Codecov Report

Attention: Patch coverage is 54.79452% with 99 lines in your changes missing coverage. Please review.

Project coverage is 54.72%. Comparing base (403b15a) to head (5cd74a7).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
bin/strata-client/src/main.rs	0.00%	43 Missing ⚠️
bin/strata-client/src/helpers.rs	0.00%	33 Missing ⚠️
bin/strata-client/src/args.rs	86.25%	18 Missing ⚠️
crates/config/src/config.rs	58.33%	5 Missing ⚠️

@@            Coverage Diff             @@
##             main     #669      +/-   ##
==========================================
+ Coverage   54.05%   54.72%   +0.67%     
==========================================
  Files         320      320              
  Lines       34158    34490     +332     
==========================================
+ Hits        18464    18876     +412     
+ Misses      15694    15614      -80

Files with missing lines	Coverage Δ
crates/config/src/btcio.rs	`100.00% <ø> (+27.77%)`	⬆️
crates/config/src/config.rs	`90.90% <58.33%> (-5.17%)`	⬇️
bin/strata-client/src/args.rs	`86.80% <86.25%> (+86.80%)`	⬆️
bin/strata-client/src/helpers.rs	`0.00% <0.00%> (ø)`
bin/strata-client/src/main.rs	`0.00% <0.00%> (ø)`

... and 12 files with indirect coverage changes

github-actions · 2025-02-13T10:34:03Z

Commit: 2947eeb

SP1 Performance Test Results

program	cycles	success
BTC_BLOCKSPACE	30,370,501	✅
EL_BLOCK	97,878	✅
CL_BLOCK	93,599	✅
L1_BATCH	30,419,221	✅
L2_BATCH	3,432	✅
CHECKPOINT	25,769	✅

storopoli

minor nits.
These are awesome, thanks! Nice work.

bin/strata-client/src/args.rs

crates/config/src/config.rs

delbonis

As discussed in DMs, the round trip through json and the weird special casing of strings is a problem. This forces that the config be well-formed before applying the overrides, which would breaks use cases like where the user has an incomplete config wants to "finish" it by setting overrides at run time. Maybe that specific scenario isn't particularly common, but it makes it generally more flexible if we apply the overrides on the toml parsing first before trying to interpret it concretely.

delbonis · 2025-02-18T17:15:59Z

bin/strata-client/src/args.rs

+/// Parse valid overrides. This first splits the entries by '=' to get key and value and then splits
+/// the key by '.' which is the update path.
+fn parse_overrides(overrides: &[String]) -> anyhow::Result<Vec<Override>> {
+    let mut result = Vec::new();
+    for item in overrides {
+        let (key, value) = item
+            .split_once("=")
+            .ok_or(anyhow!("Invalid override: must be in 'key=value' format"))?;
+        let path: Vec<_> = key.split(".").map(|x| x.to_string()).collect();
+        result.push((path, value.to_string()));
+    }
+    Ok(result)
+}


Functional programming heuristic: The way this is written, it's not possible to parse just a single override string. You always have to parse a list of override strings. The body of the loop is where the hard part is happening, and then we don't need a function just to transform elements of a list.

delbonis · 2025-02-18T17:20:35Z

bin/strata-client/src/args.rs

+        [key] => {
+            config[key] = parse_value(str_value)?;
+        }
+        [key, other @ ..] => {
+            apply_override(other, str_value, &mut config[key])?;
+        }


The config[key] bit panics if config isn't a json dict. The snippet I sent gives a safer way to handle this that gives smarter error handling.

delbonis · 2025-02-18T17:23:35Z

bin/strata-client/src/args.rs

+    match serde_json::from_str(str_value) {
+        Ok(v) => Ok(v),
+        Err(e) => match serde_json::from_str::<T>(&format!("\"{str_value}\"")) {
+            Ok(v) => Ok(v),
+            Err(_) => Err(e),


This is messy because it's trying to parse all possible json values. Don't try to, constrain behavior. It's only reasonable right now to support bools, ints, and strings. When the user passes something they expect to be a string then it implicitly needs to be a valid "the chars between the quotes" of a json string token, so it'd break unexpectedly if they pass something that has a \ in it, like maybe someone running Windows would.

delbonis · 2025-02-18T17:24:25Z

bin/strata-client/src/args.rs

+    pub fn override_config(&self, config: &mut Config) -> anyhow::Result<bool> {
+        // Override using -o params.
+        let mut overridden = self.override_generic(config)?;
+
+        // Override by explicitly parsed args like datadir, rpc_host and rpc_port.
+        if let Some(datadir) = &self.datadir {
+            config.client.datadir = datadir.into();
+            overridden = true
+        }
+        if let Some(rpc_host) = &self.rpc_host {
+            config.client.rpc_host = rpc_host.to_string();
+            overridden = true
+        }
+        if let Some(rpc_port) = &self.rpc_port {
+            config.client.rpc_port = *rpc_port;
+            overridden = true
+        }


These can be lowered to just being overrides instead of having to maintain additional special casing for these.

fn extract_cmdline_override_strs(args: &Args) -> anyhow::Result<Vec<Override>> { let mut overrides = Vec::new(); if let Some(dd) = &self.datadir { overrides.push(Override::new("client.datadir", Value::String(dd.to_owned())); } // the rest Ok(overrides) }

So really you'd have some .get_overrides() fn that just returns a list of overrides, the ones explicitly added and then the ones generated from the other options like -d and -p. Also I think this is missing a few options.

This pattern also makes it easy when we want to parse values from the env, it's all just the override system.

delbonis · 2025-02-18T17:45:11Z

bin/strata-client/src/args.rs

+        }
+        // Convert back to json.
+        *config = from_value(json_config)
+            .context("Should be able to create Config from serde json Value")?;


These contexts are for adding more information about what the program was trying to do when the fn was called, not just literally what went wrong if there were to be an error. That would be the job of the inner error type.

delbonis · 2025-02-18T17:46:57Z

functional-tests/factory/factory.py

+            # bitcoind
+            "-o", f"bitcoind.rpc_url={bitcoind_config["bitcoind_sock"]}",
+            "-o", f"bitcoind.rpc_user={bitcoind_config["bitcoind_user"]}",
+            "-o", f"bitcoind.rpc_password={bitcoind_config["bitcoind_pass"]}",
+            "-o", "bitcoind.network=regtest",
+
+            # reth
+            "-o", f"exec.reth.rpc_url={reth_config["reth_socket"]}",
+            "-o", f"exec.reth.secret={reth_config["reth_secret_path"]}",
+            "--sequencer"
+
+            # client
+            "-o", f"client.sync_endpoint={sequencer_rpc}",


This is good to see here, but really we should be writing all of these to the config file instead of just building the whole configuration as overrides (and same in above case). It's also less likely to silently break if we use the nice types you also added.

Yeah good point. Changed.

delbonis · 2025-02-18T17:48:22Z

functional-tests/factory/config.py

+    client: ClientConfig
+    bitcoind: BitcoindConfig
+    btcio: BtcioConfig
+    sync: SyncConfig
+    exec: ExecConfig
+    relayer: RelayerConfig


Can't we also use field(default_factory=...) for these?

We can totally! changed.

storopoli

ACK 7e15fca

crates/config/src/config.rs

bewakes · 2025-02-19T13:32:08Z

@delbonis , made the changes. Seems like I was wrong about using json would let us dynamically parse stuffs. Removed all the json manipulation now.

delbonis · 2025-02-19T21:48:53Z

bin/strata-client/src/args.rs

-    #[argh(
-        option,
-        description = "max interval between bitcoin RPC retries in ms (default: 1000)"
-    )]
-    pub bitcoind_retry_interval: Option<u64>,
+type Override = (Vec<String>, String);

-    #[argh(option, short = 'n', description = "L1 network to run on")]
-    pub network: Option<Network>,
+/// Parses an overrides This first splits the string by '=' to get key and value and then splits
+/// the key by '.' which is the update path.
+pub fn parse_override(override_str: &str) -> Result<Override, ConfigError> {
+    let (key, value) = override_str
+        .split_once("=")
+        .ok_or(ConfigError::InvalidOverride(override_str.to_string()))?;
+    let path: Vec<_> = key.split(".").map(|x| x.to_string()).collect();
+    Ok((path, value.to_string()))
+}

-    #[argh(switch, description = "start sequencer bookkeeping tasks")]
-    pub sequencer: bool,
+pub fn apply_overrides(overrides: Vec<String>, table: &mut Table) -> Result<Config, ConfigError> {
+    for res in overrides.iter().map(String::as_str).map(parse_override) {
+        let (path, val) = res?;
+        apply_override(&path, &val, table)?;
+    }

-    #[argh(option, description = "sequencer rpc host:port")]
-    pub sequencer_rpc: Option<String>,
+    toml::Value::Table(table.clone())
+        .try_into()
+        .map_err(|_| ConfigError::ConfigNotParseable)
+}


Why not go the rest of the way with this and have a

struct Override { key: String, // (using `.split_once` to traverse the path) value: toml::Value }

So that we can complain about parse failures before we try to manipulate the configuration? This more cleanly separates the stages since it's not having to do more parsing in the override phase, making it more functional and easier to reason about.

Also the way the apply_overrides function works is weird. It mutates its input and then parses and it and returns that? Same thing where it's merging phases into a conglomeration that's hard to reason about. Also the error reporting here is weak, it throws away all the information the user might need to figure out what they did wrong.

Pseudocode for making it 3 distinct phases:

let abstract_config = ...; let overrides = override_strs.map(parse_override); overrides.for_each(apply_override(abstract_config)); let final_config = parse(abstract_config);

Yeah, this makes sense. Agree that the apply_overrides is a bit weird. changed.

delbonis · 2025-02-19T21:55:40Z

bin/strata-client/src/args.rs

+        .parse::<i64>()
+        .map(toml::Value::Integer)
+        .or_else(|_| str_value.parse::<bool>().map(toml::Value::Boolean))
+        .unwrap_or(toml::Value::String(str_value.to_string()))


Use unwrap_or_else. The way it's written now always constructs the value even if it parses successfully as a string or bool beforehand.

delbonis · 2025-02-19T21:57:26Z

bin/strata-client/src/helpers.rs

-    let conf =
-        toml::from_str::<Config>(&config_str).map_err(|err| SerdeError::new(config_str, err))?;
-    Ok(conf)
+    Ok(toml::from_str(&config_str).map_err(|_| ConfigError::ConfigNotParseable)?)


Just return the toml error! It has moderately detailed error reporting!

delbonis · 2025-02-19T21:57:57Z

bin/strata-client/src/main.rs

@@ -137,9 +140,9 @@ fn main_inner(args: Args) -> anyhow::Result<()> {

    let mut methods = jsonrpsee::Methods::new();

-    match &config.client.client_mode {
+    match &args.sequencer {


This should be pulling from the parsed config instead of args.

Also, if?

delbonis · 2025-02-19T22:00:17Z

bin/strata-client/src/args.rs

+        let mut overrides = Vec::new();
+        if let Some(datadir) = &self.datadir {
+            overrides.push(format!("client.datadir={}", datadir.to_string_lossy()));
+        }
+        if let Some(rpc_host) = &self.rpc_host {
+            overrides.push(format!("client.rpc_host={}", rpc_host));
+        }
+        if let Some(rpc_port) = &self.rpc_port {
+            overrides.push(format!("client.rpc_port={}", rpc_port));
+        }

-    /// Max retries for Bitcoin RPC calls.
-    #[argh(option, description = "max retries for bitcoin RPC (default: 3)")]
-    pub bitcoind_retry_count: Option<u8>,
+        overrides


You can do format!("foo.bar={bar}") instead of having to put it at the end.

delbonis

Looks mostly good but there's some minor tweaks here.

bin/strata-client/src/args.rs

bin/strata-client/src/errors.rs

bewakes marked this pull request as ready for review February 18, 2025 11:44

bewakes requested review from a team as code owners February 18, 2025 11:44

bewakes requested a review from delbonis February 18, 2025 11:48

storopoli requested changes Feb 18, 2025

View reviewed changes

bewakes force-pushed the STR-408-enrich-client-configuration branch from 8cd663b to bd24021 Compare February 18, 2025 12:56

bewakes requested a review from storopoli February 18, 2025 16:43

delbonis requested changes Feb 18, 2025

View reviewed changes

storopoli previously approved these changes Feb 19, 2025

View reviewed changes

crates/config/src/config.rs Outdated Show resolved Hide resolved

bewakes dismissed storopoli’s stale review via d980806 February 19, 2025 12:07

bewakes force-pushed the STR-408-enrich-client-configuration branch 3 times, most recently from e0a8f83 to 72f0863 Compare February 19, 2025 13:28

bewakes requested a review from delbonis February 19, 2025 13:31

delbonis requested changes Feb 19, 2025

View reviewed changes

bewakes requested a review from delbonis February 20, 2025 06:38

delbonis requested changes Feb 21, 2025

View reviewed changes

bin/strata-client/src/args.rs Outdated Show resolved Hide resolved

bin/strata-client/src/args.rs Outdated Show resolved Hide resolved

bin/strata-client/src/errors.rs Outdated Show resolved Hide resolved

Bibek Pandey added 8 commits February 24, 2025 11:00

Update config and args

9ca1c9e

Add some generic overrdiing code

54e5fa6

Remove ClientMode from config

f9fbb1d

Rename bitcoind_rpc to bitcoind

fb911ca

Fix parsing json while overriding

2f94826

func_tests: Update config in factory

a8a0c04

Fix review comments; better docstrings

0b4d02c

args: Remove json conversion and directly apply overrides on toml

b224e6b

Bibek Pandey added 3 commits February 24, 2025 11:00

Cleanup configs around func tests

cb66164

Make overrides parsing/applying cleaner

b4172e0

Fix some func tests config

1fd93be

bewakes force-pushed the STR-408-enrich-client-configuration branch from a14ac35 to ffde02e Compare February 24, 2025 05:37

Bibek Pandey added 2 commits February 24, 2025 12:05

Enhance error messages

dac66b5

(func-test): Set default value for relay_misc in config to be true

40f8dbc

bewakes force-pushed the STR-408-enrich-client-configuration branch from ffde02e to 40f8dbc Compare February 24, 2025 07:25

(func-test): Fix some func tests

7bf93c2

bewakes force-pushed the STR-408-enrich-client-configuration branch from 284faf1 to 7bf93c2 Compare February 24, 2025 10:51

bewakes requested review from delbonis and sapinb February 25, 2025 06:19

Add extra validation to config

5cd74a7

sapinb approved these changes Feb 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update config and args #669

Update config and args #669

bewakes commented Feb 13, 2025 •

edited

Loading

codecov bot commented Feb 13, 2025 •

edited

Loading

github-actions bot commented Feb 13, 2025 •

edited

Loading

storopoli left a comment

delbonis left a comment

delbonis Feb 18, 2025

bewakes Feb 19, 2025

delbonis Feb 18, 2025

delbonis Feb 18, 2025

delbonis Feb 18, 2025

delbonis Feb 18, 2025

delbonis Feb 18, 2025

bewakes Feb 19, 2025

delbonis Feb 18, 2025

bewakes Feb 19, 2025

storopoli left a comment

bewakes commented Feb 19, 2025

delbonis Feb 19, 2025

bewakes Feb 20, 2025

delbonis Feb 19, 2025

delbonis Feb 19, 2025

delbonis Feb 19, 2025

delbonis Feb 19, 2025

delbonis left a comment

Update config and args #669

Are you sure you want to change the base?

Update config and args #669

Conversation

bewakes commented Feb 13, 2025 • edited Loading

Description

Type of Change

Notes to Reviewers

Checklist

Related Issues

codecov bot commented Feb 13, 2025 • edited Loading

Codecov Report

github-actions bot commented Feb 13, 2025 • edited Loading

storopoli left a comment

Choose a reason for hiding this comment

delbonis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storopoli left a comment

Choose a reason for hiding this comment

bewakes commented Feb 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delbonis left a comment

Choose a reason for hiding this comment

bewakes commented Feb 13, 2025 •

edited

Loading

codecov bot commented Feb 13, 2025 •

edited

Loading

github-actions bot commented Feb 13, 2025 •

edited

Loading