Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: replace toml with cli config #732

Open
wants to merge 14 commits into
base: next
Choose a base branch
from
Open

feat: replace toml with cli config #732

wants to merge 14 commits into from

Conversation

Mirko-von-Leipzig
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig commented Mar 12, 2025

This PR replaces the node's configuration via toml files to pure clap cli args. We retain the ability to configure via a file using environment variables instead.

Left undone

  • Update github scripts (waiting on an initial review first)

Documentation in readme. I'll skip the readme and add it to the operator docs in a separate PR.

The faucet still uses a toml file. This file is re-used when creating a new account and when running the faucet. We may want to relook at the expected flow here but I didn't want to fiddle with it.

New flow

I've swopped around the subcommand structure so that its now miden-node <component> <action>. This was mostly to "organise" the store commands but I can imagine having additional subcommands for rpc and block-producer in the future.

I wanted to change up the store so that miden-node store bootstrap would also create the database and seed it with the genesis data. In this case the genesis.dat would become ephemeral (though it can be brought back when if we decide we need it). miden-node store start would now never create the db, but instead expect it to exist. I also want to move database migration into a separate subcommand (and not automatically do it on start). I decided not to do this in this PR as it was too much churn.

Simplifications

I've replaced the various store filepaths and directory paths with a single data_directory which will contain the database, blockstore and genesis.dat. I don't see a good reason to modify these at present.

I've also simplified node start by making the internal gRPC servers (block-producer & store) use localhost:0 which assigns a random open port. This means these no longer need to be configured at all. We can somewhat trivially make this overrideable but I don't see the point. If you want that, spawn the components separately imo :)

I'll also note that I was tempted to make the components more robust to startup failures, but it seems like tonic in general does not handle connection failures in many cases - meaning we cannot trivially loop/retry until all required components are ready. This may take a bit more work to achieve.

Usage

miden-node node start [OPTIONS] --rpc.url <URL> --store.data-directory <DIR>
miden-node rpc start [OPTIONS] --rpc.url <URL> --store.url <URL> --block-producer.url <URL>
miden-node block-producer start [OPTIONS] --store.url <STORE_URL> <URL>

miden-node store dump-genesis > genesis.toml
miden-node store bootstrap [OPTIONS] --data-directory <DIR> --accounts-directory <DIR>
miden-node store start [OPTIONS] --url <URL> --data-directory <DIR>

@Mirko-von-Leipzig Mirko-von-Leipzig marked this pull request as ready for review March 12, 2025 12:41
Comment on lines +21 to +46
Start {
/// Url at which to serve the RPC component's gRPC API.
#[arg(long = "rpc.url", env = ENV_RPC_URL, value_name = "URL")]
rpc_url: Url,

/// Directory in which the Store component should store the database and raw block data.
#[arg(long = "store.data-directory", env = ENV_STORE_DIRECTORY, value_name = "DIR")]
store_data_directory: PathBuf,

/// The remote batch prover's gRPC url. If unset, will default to running a prover
/// in-process which is expensive.
#[arg(long = "batch_prover.url", env = ENV_BATCH_PROVER_URL, value_name = "URL")]
batch_prover_url: Option<Url>,

/// The remote block prover's gRPC url. If unset, will default to running a prover
/// in-process which is expensive.
#[arg(long = "block_prover.url", env = ENV_BLOCK_PROVER_URL, value_name = "URL")]
block_prover_url: Option<Url>,

/// Enables the exporting of traces for OpenTelemetry.
///
/// This can be further configured using environment variables as defined in the official
/// OpenTelemetry documentation. See our operator manual for further details.
#[arg(long = "open-telemetry", default_value_t = false, env = ENV_ENABLE_OTEL, value_name = "bool")]
open_telemetry: bool,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we shouldn't use the manual (non-derive) way of building these args. That would let us re-use the same ones and keep consistency.

I'm unsure about the naming scheme as well, and whether it should always remain consistent. I suspect I've done a little of column A and a bit of column B.

e.g. should store.url be used to configure RPC, block-producer AND the store start itself?

Comment on lines +60 to +61
let block_producer_url = format!("http://{block_producer_address}");
let channel = tonic::transport::Endpoint::try_from(block_producer_url)?.connect().await?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit awkward and makes me think I was too hasty in removing our custom Endpoint type in #654.

As a summary:

  • Tonic wants a Uri but accepts strings in the format of http(s)://{SocketAddr}.
  • TcpListener::bind wants a SocketAddr.
  • Uri cannot be trivially converted to a SocketAddr
  • Url can be converted to a SocketAddr so we use it.
  • Clients may want to perform a DNS lookup to the server address of http://rpc.testnet.miden.io

I think its okay to leave as is, but we probably want to revisit this (again :'( ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant