Refactor cli config - #38
Merged
Merged
Conversation
Replace 9 environment variables with structured clap CLI arguments organized in polkadot-sdk-style parameter groups (StorageParams, RpcParams, KeyParams, CheckpointParams, ReplicaSyncParams). Key changes: - New cli.rs with Cli struct using #[clap(flatten)] for param groups - SEED env var replaced by --keyfile (with Unix permission checks) and --dev flag - Non-secret args retain env= fallback for deployment compatibility - main.rs reduced to one-line delegating to cli::run() - run() returns Result, using ? operator instead of process::exit() - Justfile updated to use CLI flags instead of env vars
Contributor
Author
|
hey @bkontur is this roughly similar to what you had in mind? i wasn't exactly sure what Polkadot-SDK style means here |
bkontur
reviewed
Mar 11, 2026
| SEED="//Alice" CHAIN_RPC="ws://127.0.0.1:2222" BIND_ADDR="0.0.0.0:3333" \ | ||
| nohup ./target/release/storage-provider-node --storage-mode inmemory > /tmp/provider.log 2>&1 & | ||
| nohup ./target/release/storage-provider-node \ | ||
| --dev --storage-mode inmemory \ |
Collaborator
There was a problem hiding this comment.
@RafalMirowski1 let's remove --dev and do the same here as for bob with key file
Contributor
Author
There was a problem hiding this comment.
removed --dev flag entirely (or you meant just remove it here but keep as a flag?)
franciscoaguirre
approved these changes
Mar 11, 2026
bkontur
reviewed
Mar 12, 2026
| .key | ||
| .provider_id | ||
| .clone() | ||
| .unwrap_or_else(|| DEFAULT_PROVIDER_ID.to_string()); |
Collaborator
There was a problem hiding this comment.
@RafalMirowski1 do we need this DEFAULT provider? Can we just throw error? Does it make sense to run provider withtou provider id?
bkontur
approved these changes
Mar 12, 2026
4 tasks
bkontur
added a commit
that referenced
this pull request
Aug 14, 2026
* provider-node: make disk the default storage backend `--storage-mode` defaulted to `inmemory` only because that was the behaviour before #33 introduced the flag: the provider called `Storage::new()` unconditionally, disk was bolted on for one persistent Westend demo ("very dummy implementation ... will be reworked later"), and defaulting to inmemory kept every existing caller unchanged. #38 moved the flag into `StorageParams` verbatim; the default itself was never re-decided. It has been wrong for a while. The disk backend is now the production path — RocksDB with four column families, a durable nonce counter, the fs/s3 index layered on top, and its own module doc saying "for production use" — while an operator who passes no storage flags still got a provider that silently drops every byte on restart and can no longer answer challenges for buckets it still holds agreements for. - `StorageMode` derives `Default` with `#[default] Disk`, and the clap arg takes its default from it, so there is one source of truth. - `just start-provider` / `register-then-start-provider` default to `MODE=disk` to match the binary. - Document both backends in provider-node/README.md — the repo had no prose about the storage backend at all, so the old default was invisible. `--storage-mode inmemory` stays as an explicit opt-in for throwaway runs. * ci: cover both storage backends, one provider launcher Since #145 dropped the disk provider from the zombienet job (collateral of trimming that job to a smoke test), every provider in CI ran `--storage-mode inmemory`. Nothing exercised RocksDB, the fs index, or the persistent nonce counter end to end, and nothing ever restarted a provider — so the backend the binary now defaults to had no integration coverage at all. Split the backends across the jobs that already exist instead of adding runs, so both are stretched and no suite runs twice: e2e-integration-tests no --storage-mode: asserts the binary default (disk) integration-tests disk, plus a mid-job restart (persistence) sc-integration-tests inmemory ui-integration-tests inmemory The zombienet job keeps one provider on :3333 because the L0 demo is what registers //Alice on-chain and the L1 fs/s3 demos reuse that registration — they resolve the provider from the justfile's PROVIDER_URL default, so a second provider on another port would need its own registration to be useful. The restart check snapshots /stats after the L0 demo, kills the process, waits for it to actually exit (RocksDB holds a LOCK on the data dir), restarts on the same --storage-path and asserts nodes/bytes are unchanged. It fails loudly if the provider stored nothing, so it cannot pass vacuously. The fs/s3 demos then run against the restarted instance, which also proves a recovered provider is usable and not merely non-empty; they wait on /info readiness first, since /health answers before the chain-derived state (nonce counter, registration) is back. Also: the four copies of the nohup provider block are now one composite action, .github/actions/start-provider, which writes the keyfile, launches the node, fails fast if it exits immediately, and waits for health. Log files are named per provider, and the log artifacts glob /tmp/provider-*.log — the zombienet job had been uploading /tmp/provider.log while the step wrote /tmp/provider-inmemory.log, so its provider log was never collected. Cost: no extra suite runs and no extra chain. The disk job adds one process restart plus two readiness waits. * ci, provider-node: simplify the storage-backend work Cleanup pass over the previous two commits. wait-for-provider-health now owns what "up" means: a `wait-for: health|ready` input picks the endpoint and the jq predicate, so the restart's bespoke readiness loop in integration-tests.yml is gone. It was a copy of that action's poll loop differing only in path and filter, and the race it worked around (/health answers before the nonce counter and on-chain registration are loaded) exists on every cold start, not just this restart — any job can now opt into the stronger wait. That also drops a redundant /health poll on the restart path: /info cannot answer unless the listener is bound, so waiting for both was two grids for one fact. Restart timeout goes to 20 attempts, since the chain is already up by then; worst case on a red run falls by ~200s. Smaller cuts: - start-provider builds the optional flag with ${MODE:+--storage-mode "$MODE"} instead of an array plus a `${arr[@]+...}` splat and the comment explaining it. - Call sites stop restating action defaults (seed, port, and storage-path where it is not the point) — 13 lines of `with:` that said what the action already says. storage-path stays on the disk pair, where path identity is the invariant under test. - The restart check compares one `jq -c '{total_nodes, total_bytes}'` blob through one GITHUB_ENV var instead of two scalars through two, and drops `// 0` fallbacks that could not fire (StatsResponse has non-optional u64s and curl -sf already fails the step on a non-2xx). The pkill pattern is a variable rather than three copies of the same string. - StorageMode drops the Default derive: StorageMode::default() had exactly one caller — the clap attribute — so `default_value_t = StorageMode::Disk` says it in one place instead of two. Variant docs go back to one line each; they are --help text, where clap already appends [default: disk], and the rationale lives in provider-node/README.md. - Trimmed the prose added by the previous commits (README, workflow comments, action descriptions). * ci: run the restart-check steps under bash Workflow `run:` steps get `sh -e {0}` in the CI container — only steps that declare `shell:` (and composite-action steps, which must) get bash. The two inline steps added with the restart check opened with `set -euo pipefail`, so dash killed the first one before it ran: /__w/_temp/….sh: 1: set: Illegal option -o pipefail Declare `shell: bash` on both rather than dropping to POSIX: -u and pipefail are what make the snapshot fail loudly instead of comparing empty strings later. Nothing else in the diff was affected — start-provider and wait-for-provider-health already declare `shell: bash`, and both ran fine. * ci: drop the restart-persistence check Removes the three zombienet steps that snapshotted /stats, killed the provider, restarted it on the same data dir and compared — plus the plumbing that existed only to serve them: the `wait-for: health|ready` input on wait-for-provider-health (that action is now byte-identical to dev again) and the `wait-attempts` passthrough on start-provider. The zombienet job still runs on disk, so the L0 and L1 fs/s3 demos exercise the backend the binary now defaults to; what is gone is the assertion that data survives a process restart. Nothing else referenced the removed inputs. * provider-node: run the HTTP integration tests on both backends The suites covered one backend each: api/fs/s3_integration ran in-memory only, and disk_integration was a hand-renamed partial copy of the api suite (disk_check_exists ↔ test_check_exists, and so on) covering ~30% of the API surface and nothing from fs/s3. Two files to keep in sync, and disk coverage that drifts behind whatever the api suite grows. Parameterize instead. `common::backend_tests!` declares a test once and emits `<name>::in_memory` and `<name>::disk`, so a failure names the backend it happened on; `common::storage_for(backend)` builds the storage plus the TempDir RocksDB needs kept alive. Each suite's TestServer takes a Backend and holds `_dir: Option<TempDir>`. 71 tests now run on both backends (142 cases), up from 57 tests on one backend each. disk_integration.rs is deleted: every scenario it had is covered by the parameterized suites, except `/mmr_proof`, which it alone exercised — that one is ported into api_integration.rs, so it now runs in-memory too. Test bodies are unchanged apart from indentation and taking the server from `TestServer::new(backend)`; `git diff -w` shows the real change. Not parameterized: auth_integration (membership/signature logic, backend-blind) and negotiate_integration, which already covers the one genuinely disk-specific behaviour in `with_store_counter_persists_on_next` — the nonce counter surviving a store reopen. * provider-storage: one place that builds a backend Three places knew how to turn "which storage mode" into a backend: the clap enum, the match in command.rs, and (since the test parameterization) a test-only Backend enum. The test copy had already drifted — every disk test paired DiskStorage with NullNonceStore, so the pairing the binary actually uses was exercised nowhere. StorageBackendSpec in provider-storage owns construction: pub enum StorageBackendSpec { InMemory, Disk { path: PathBuf } } pub fn build(&self) -> Result<(Arc<dyn StorageBackend>, Arc<dyn NonceStore>), Error> The path now lives on the variant that uses it, which is what a plain `Disk(RocksDB)` on the CLI enum was reaching for — clap value-enums must be unit variants, and `--storage-path` arrives as a separate flag anyway, so the rich type belongs next to the backends rather than in the parse surface. - command.rs drops its match for `cli.storage.spec().build()?`, and logs the spec via Display instead of a per-variant message. - StorageParams::spec() is the only place --storage-path is read, making "disk-only" explicit rather than a silently ignored combination. - Tests drop their Backend enum for the node's own cli::StorageMode and go through build(), so disk tests now run against the RocksDB-backed nonce store like the binary does. StorageMode keeps its CLI shape and its --storage-mode values; only what it maps to changed. * provider-node: name the backends after their engine StorageMode said which *mode*, not which backend, and its variants named a property (`Disk`) and mis-cased the other (`Inmemory`). Follow what the SDK does for the same problem — sc_client_db::DatabaseSource carries `RocksDb { path, cache_size }` / `ParityDb { path }`, and the CLI-facing sc_cli::arg_enums::Database is the flat `RocksDb` / `ParityDb` / `Auto`: StorageMode { Inmemory, Disk } -> StorageBackendKind { InMemory, RocksDb } StorageBackendSpec::Disk { path } -> StorageBackendSpec::RocksDb { path } Naming the engine rather than the medium leaves an obvious place for a second one (ParityDb, S3, …) instead of a second thing called "disk". The CLI surface does not change: `#[value(name = "inmemory", alias = "in-memory")]` keeps the string CI, the justfile and the docs already pass, and `#[value(alias = "disk")]` keeps every existing `--storage-mode disk` working. A test walks all four spellings so the aliases cannot rot. Repo-side strings move to the canonical name — justfile MODE, the workflow's `mode:`/`label:`, the composite action's docs, provider-node/README.md — so the codebase says one thing while operators' scripts keep working. * provider-node: drop the CLI aliases, take the engine names as-is `#[value(rename_all = "lower")]` on the enum is what sc_cli::arg_enums::Database uses to publish `rocksdb`/`paritydb`, and it gives the same result here without per-variant names: `rocksdb` and `inmemory`. Clap's default would be kebab-case (`rocks-db`, `in-memory`), which is the only reason the previous commit reached for explicit names. No back-compat alias for `disk`: the repo passes the canonical values everywhere, and the test now asserts `disk`, `rocks-db` and `in-memory` are rejected so the surface stays exactly the two engine names. * provider-node: one test harness, and stop re-deriving //Alice Cleanup pass over the parameterization. The three suites each carried their own TestServer: fs and s3 were byte-identical and api differed only in which ProviderState constructor it called, so the storage_for 3-tuple, the `_dir: Option<TempDir>` field and its comment were written out three times. TestServer and TestBackend now live in tests/common; each suite keeps a two-line `new` naming its identity, and eight imports per suite go away. Test bodies are untouched. TestBackend also replaces the cli::StorageBackendKind selector the tests were importing. The kind is the arg parser's surface — its value is the strings `rocksdb`/`inmemory`, which only the binary's callers use — and taking it forced storage_for to re-implement the kind→spec mapping that StorageParams::spec already owns. sc-client-db does the same thing: its test helper (`new_test_with_tx_storage_source`) takes DatabaseSource, not sc_cli::arg_enums::Database. test_member_pair now derives once per test thread. `//Alice` is a dev phrase, so every call ran PBKDF2 2048 times — ~37ms in a debug build, two or three times per case across 142 cases. That was a bigger cost than everything the RocksDB half of the parameterization added. Also: - negotiate_integration builds its disk nonce store through StorageBackendSpec::build instead of pairing DiskStorage + nonce_store by hand — the one call site left doing what build() exists to prevent. - The backend_tests! attribute capture is gone: no call site used a real attribute, and a captured `#[ignore]` would have landed on the private helper rather than the generated tests, silently doing nothing. - The CLI value test asserts on the built spec, which covers the mapping and --storage-path in one go, and drops both the mem::discriminant dance and the now-redundant inmemory_is_opt_in. - Trimmed the prose these commits added: the lib crate no longer documents the CLI's clap constraints, and the reviewer-facing justifications are gone. * provider-node: keep the storage doc comments to what the code does Drops the cross-reference to sc_cli::arg_enums::Database, the two comments that restated their own asserts, and the mode name from the --storage-path help, which reads for any persistent engine. * provider-node: rename the flag to --storage-backend The flag was the last thing still calling it a mode: the enum is StorageBackendKind, the configured form is StorageBackendSpec, and the values are engine names. No alias — the repo passes the new spelling everywhere. Renames with it: StorageParams::storage_backend, the start-provider action's `backend` input, and the justfile's BACKEND parameter. * provider-node: drop the in-memory storage backend Removes the InMemory variant from StorageBackendKind and StorageBackendSpec and everything that selected it: the build()/Display arms, the `inmemory` CLI value, TestBackend::in_memory, the macro's second arm, the sc/ui jobs' `backend:` input, the justfile's conditional --storage-path, and the README paragraph. A provider that forgets its data cannot answer challenges for buckets it still holds agreements for, so it was never an option an operator should have. Both enums stay as the extension point, and the prose around them stays generic: adding an engine means a variant carrying its own configuration, an arm in build(), an arm in the test macro — no doc rewrites. Also fixes the two CI failures on 459f92b, both in backend/mod.rs: - clippy type_complexity on build()'s return (the repo runs -D warnings). Factored into `OpenedBackend`, which is what the lint suggests and leaves callers' tuple destructuring untouched. - Patch coverage 61%: Display was reachable only from command.rs at runtime, so 5 of 13 new lines never executed. It now has a test in its own crate, which also pins what build() exists for — persist a nonce, drop both halves, reopen the same directory, watermark still there. The suites now run 71 cases instead of 142, each against a real database. in_memory.rs and NullNonceStore stay: no longer reachable through the spec, but auth/negotiate/chain_state/coordinators and clients/storage construct them directly. * provider-storage: the in-memory backend is a test helper It has no production caller now that the spec only builds RocksDB — every remaining use is a #[cfg(test)] module or a tests/ suite. Rather than delete it and push ~40 fast RAM-backed tests onto real databases, gate it behind a `test-helpers` feature (off by default) and have provider-node and clients/storage switch it on in dev-dependencies only. Same shape sc-client-db uses for its test-only constructors. A release build of the provider now cannot contain a backend an operator has no way to select. NullNonceStore stays ungated: NonceCounter::new and ChainStateCoordinator use it in production as the no-op store. Also shows BACKEND alongside STORAGE_PATH in the justfile examples. * provider-storage: export the in_memory module, not its type Drops both gated `pub use ... Storage` lines. The module is the export, so the cfg lives in one place — `pub mod in_memory` — instead of being repeated on a re-export in backend/mod.rs and another in lib.rs. Callers now say provider_storage::backend::in_memory::Storage, which also reads as what it is at every use site. The test-helpers feature carries no comment: the cfg on the module says where it applies. * provider-storage: delete the in-memory backend and the no-op nonce store Neither had a production caller once the spec built only RocksDB: in_memory.rs was reachable only from tests, and NullNonceStore only from the two constructors that existed for it — NonceCounter::new and ChainState::default, both documented as the in-memory path. Keeping a second StorageBackend implementation meant 601 lines and 19 trait methods maintained in parallel with the one that ships, and this branch already showed what that costs: every disk test had been paired with NullNonceStore, so the pairing the binary uses was exercised nowhere. Gone: - crates/providers/storage/src/backend/in_memory.rs and NullNonceStore - the test-helpers feature that briefly gated the first of those - NonceCounter::new (with_store is the constructor) and ChainState::default (with_nonce_store is the builder) Tests now build the real thing. Each fixture opens a RocksDB on a TempDir and keeps the guard for as long as the state lives — a `_dir` field on the server structs, an extra tuple element out of the coordinator fixtures. Two exceptions, both where the server outlives any guard a caller could hold: clients/storage's start_test_provider persists its directory, and negotiate_integration keeps a ForgetfulStore for the NonceCounter unit tests, which exercise the atomic and not persistence — a no-op store is a test double, so it lives with the tests. * provider-storage: temp_rocksdb(), so tests stop hand-rolling scratch dirs Removing the in-memory backend left seven files opening a TempDir, building a RocksDB spec and threading the guard around by hand — provider-node's src tests, four test suites, the coordinator fixtures and clients/storage's helper. One function does it now: let (storage, nonce_store, _dir) = temp_rocksdb(); Keep the guard for as long as the backend is in use; dropping it takes the database with it. The suites that hold a server keep it in a `_dir` field, and clients/storage — whose server outlives any guard a caller could hold — calls `dir.keep()` at the one site where that is true. It lives behind `test-helpers` (off by default, pulls tempfile only when on), so a release build of the provider does not carry it. provider-node and clients/storage switch it on in dev-dependencies. Also drops the deprecated TempDir::into_path() — tempfile renamed it to keep(), and -D warnings would have caught it. * clients/storage: one provider-storage dev-dependency, not two The test-helpers entry landed above the existing plain one instead of replacing it, and a duplicate key stops cargo metadata dead — which took out check-fmt, Cargo check and cargo-deny before any of them got to do their job. * provider-node: fix what the first real compile caught Three things the rewrite missed, all in tests: - coordinators/challenge.rs destructured test_state_with_data() into two elements; `mut challenge` is why the scripted pass skipped this one. - chain_state_integration built ChainState with `{ nonce_store, ..Default }`, which was the long way of saying with_nonce_store — now the only way. - common/ is compiled into every test binary, so the backend_tests re-export is unused in the suites that build their own servers, and -D warnings counts it. * provider-node: one allow for the shared test module, not two clippy caught the sibling of the last fix: the macro definition is unused in the suites that never call it, same as its re-export and the helpers around it. All three are the same per-crate-compilation effect the file header already describes, so they belong in that allow rather than scattered over the lines that happen to trip first. * provider-node: check init_bucket's Result, name temp_rocksdb's return Behind `Arc<dyn StorageBackend>` init_bucket returns a Result the coordinator fixtures were dropping, which -D warnings rejects. Three sites now expect(), which is what a fixture wants: a backend that will not initialise fails the test. temp_rocksdb's return gets a `TempBackend` alias — clippy's type_complexity rejected the same tuple-of-trait-object-Arcs shape on `build()`. The tuple stays flat, so no call site changes. * review fixes: one kind->spec mapping, and no stale in-memory references `StorageBackendKind::spec(path)` is now the only place a CLI value becomes a `StorageBackendSpec`; `StorageParams::spec()` and the integration harness both call it, so a second engine is one arm in one file rather than three matches that have to agree. The rest is what the in-memory backend left behind: `--storage-path` claimed to be ignored by a backend that no longer exists, `backend/mod.rs`'s module doc still introduced the trait through `Storage` (a broken intra-doc link now), and the photos design doc pointed at `backend/in_memory.rs`. Also, so a leaked scratch directory can be found: `temp_rocksdb` names them with `TEMP_DIR_PREFIX`, which is what `clients/storage` leaves behind when it keeps one for a server that outlives the guard. In CI, `start-provider` derives the data directory from the label, so two providers in one job cannot silently take the same one — RocksDB only lets the first in. The extra `unused_imports`/`unused_macros` allows in the shared test module go back to a bare `dead_code`: dead code is still name-resolved, so the helpers no suite calls keep their imports live. README gains the fsync caveat that `DiskNonceStore` already documents, since persistence is now the only mode. * provider-node: the test harness owns its scratch dir, and allow only the macro `unused_macros`/`unused_imports` are back, but on `backend_tests!` and its re-export rather than the whole module: the suites that never parameterize by backend compile `common/mod.rs` too, and a `pub(crate) use` does not count as a use in a crate that never expands the macro. Blanketing the module hid stale imports in the rest of the file, which is what the previous commit was after. `negotiate_integration`'s `serve` now takes the directory, so the field is a `TempDir` instead of an `Option` set by mutation after construction, and the ten call sites hand theirs over instead of holding a binding alive by hand. The three lines that publish a bootstrapped counter became `publish_nonce_counter`, and `chain_state_integration` gets `counter_for` -- each of its tests bootstraps differently, so only the constructor is shared. Its `test_chain_state` also moves out from between two `use` lines. `test_delete_happy_path` asserts `leaf_count` again: `disk_delete_before` checked it before that suite was folded in, and nothing else covers it. `register-then-start-provider` registers first. `start-provider` runs in the foreground and never returns, so `register-provider` was unreachable; registration is a chain-only extrinsic, so the order is free to swap. --------- Co-authored-by: Andrii <ndk@parity.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#35
main.rsturned into clap args (but kept env var fallback)--keyfilewith proper permissions can be used for seed or--devflag which sets it to"//Alice"cli.rscommand.rs