Make on-disk storage the provider's backend, and cover it in CI - #348
Merged
Conversation
`--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.
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.
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).
bkontur
marked this pull request as ready for review
August 13, 2026 16:48
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.
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.
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.
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.
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.
`#[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.
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.
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.
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.
bkontur
enabled auto-merge
August 13, 2026 22:20
Member
|
I remember that we did have both storage backends in CI, but then we decided to prune one of them. now we add it back? |
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.
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.
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.
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.
…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.
Collaborator
Author
@danielbui12 thank you, agree, you right, I just removed InMemory support and the code around, for test that storage does not matter can use |
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.
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.
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.
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.
bkontur
changed the title
`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.
…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.
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.
Summary
The provider defaulted to in-memory storage only because that was the behaviour
before #33 added the flag, and the default was never revisited — a provider
started with no flags silently dropped everything on restart. CI had drifted the
same way: since #145 every provider in CI ran
inmemory, leaving the on-diskbackend and the persistent nonce counter with no end-to-end coverage.
So the on-disk backend is now what the provider runs, and the in-memory
implementation is gone rather than kept alive as a second, untested store.
Changes
--storage-mode→--storage-backend,StorageMode→StorageBackendKind, whose values are engine names (rocksdb) and whose solevariant is the default. It stays an enum so another engine is a variant, not a
reshape.
StorageBackendSpec(inprovider-storage, next to the backends) ownsconstruction and pairs the backend with a matching nonce store;
command.rsis now
cli.storage.spec().build()?.StorageBackendKind::spec(path)is theonly place a CLI value becomes a spec — the flags and the test harness both go
through it.
NullNonceStoreis gone from the library too,but not from the tree: the counter tests keep a local
ForgetfulStore, so ano-op store exists only where a test asks for one and production cannot reach
for it by accident. Tests that want a real store call
provider_storage::temp_rocksdb().TestServermoved intotests/common, replacingthree near-identical copies, and the api/fs/s3 suites are generated by a
backend_tests!macro that takes aStorageBackendKind— so a failure reads<test>::rocksdb, and a second engine is one arm inspec()plus one line inthe macro.
disk_integration.rsis deleted: it was a partial copy of the apisuite run against a disk-backed server, which is now what every suite does.
Its one unique test (
/mmr_proof) moved over..github/actions/start-provider,which derives its data directory from the provider's label so two in one job
cannot collide on one RocksDB. The zombienet job pins the backend explicitly;
the other jobs pass no flag, so each run also asserts the binary's default.
Notes
stress-test upload#220 (storage-cli+stress-test upload): that PR should be rebasedon this one and drop its standalone
utils-integration-testsjob. Thestress-test step belongs in the zombienet smoke test, which already has a
chain, a provider started through
.github/actions/start-provider, and a//Bobbucket + agreement fromjust demofor the CLI to upload into. Aseparate job pays ~2.5 min of fixed setup for well under a minute of work.
provider that remembers.
just start-chaingives new chain state every runwhile
just start-providerkeeps./provider-data, andinit_bucketreturnsearly for a bucket it already has — so a second run's bucket 1 inherits the
first run's MMR. Wants a way to wipe the directory (CI is unaffected: fresh
container per job).
FsIndexManagerandS3IndexManagerhavewith_persistence(), butProviderState::from_partsalways builds the non-persistent variants, so L1drive/bucket metadata is still lost on restart.
case is in
negotiate_integration::with_store_counter_persists_on_next; anend-to-end version would reopen a
TestServeron the same path.