Skip to content

Default node to ParityDb and price runtime DbWeight for it - #715

Open
illuzen wants to merge 5 commits into
mainfrom
perf/paritydb-default
Open

illuzen wants to merge 5 commits into
mainfrom
perf/paritydb-default

Conversation

@illuzen

@illuzen illuzen commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Makes ParityDb the node's default database backend and prices the runtime's DbWeight for it.

  • Node: --database now defaults to auto. An existing RocksDB database under <base>/chains/<id>/db is reused; otherwise ParityDb is created under <base>/chains/<id>/paritydb. Auto is resolved inside sc-cli (database_config) so the startup banner and every subcommand see the concrete backend instead of "Auto".
  • Runtime: frame_system::Config::DbWeight switches from RocksDbWeight (25 µs read / 100 µs write) to ParityDbWeight (8 µs / 50 µs). No spec_version bump per repo convention.
  • Operators: nodes still on RocksDB get a startup warning with migration steps; README documents the export-blocks → import-blocks --database paritydb migration.

Why

Storage access is the dominant cost in the wormhole settlement path (see #714). benchmark storage against a mainnet snapshot (block 161,984, 693k keys, MacBook Air NVMe, --state-version 1):

backend read avg read p99 write avg (batch 500) write p99
RocksDB 10.9 µs 18.2 µs 18.5 µs 25.8 µs
ParityDb 2.6 µs 6.1 µs 6.5 µs 8.8 µs

The SDK's ParityDbWeight (8/50) is still ~3x–7x above the measured ParityDb numbers, so it stays conservative on this hardware. Applied to the post-#714 public-batch accounting (2,236 r / 3,350 w settlement + 2×372 pre-validation reads) the storage share of a public batch drops from ~410 ms to ~191 ms and the total from ~449 ms to ~231 ms. That moves the all-public-batch path from weight-limited (10/block) to block-length-limited (17/block at 223,616 B each; weight alone would allow 19).

Caveat: these are local-NVMe numbers. The fleet is mostly VPS; a benchmark storage run on a representative VPS before release would confirm the margin.

Behavior matrix (smoke-tested with a debug build against real base paths)

base path contents result
empty 💾 Database: ParityDb at …/paritydb/full
db/ (RocksDB), with or without stray paritydb/ 💾 Database: RocksDb at …/db/full + warning
paritydb/ only 💾 Database: ParityDb at …/paritydb/full

Explicit --database rocksdb / --database paritydb are unchanged.

Checks

  • scripts/fmt.sh
  • SKIP_WASM_BUILD=1 cargo clippy --locked -p sc-cli -p quantus-node -p quantus-runtime -- -D warnings clean
  • cargo test -p quantus-runtime (139 passed)

- `--database` now defaults to `auto`: reuse an existing RocksDB database,
  otherwise create ParityDb. `Auto` is resolved in sc-cli so the startup
  banner and subcommands see the concrete backend.
- Runtime `DbWeight` switches from `RocksDbWeight` (25us/100us) to
  `ParityDbWeight` (8us/50us).
- Nodes still on RocksDB get a startup warning with migration steps;
  README documents the export-blocks/import-blocks migration.

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict: Request changes
Reviewer: GPT-6.1 Sol

Reviewed head 48a34da54e8b0b9a226e7104409e2916dab12a3c against base 9e6d01f8bbefd126d87be6325e8cfc1f09825ae9.

  1. [P2] Check for a real RocksDB before resolving Auto — client/cli/src/config.rs:245–248. An existing ParityDb node that used --database auto can already have a leftover db/full directory: the old backend's non-creating RocksDB probe creates that directory even when no RocksDB exists, then correctly opens ParityDb. The new exists() check selects RocksDB for the same layout, and Backend::new creates a fresh RocksDB alongside the populated ParityDb. On upgrade, the node ignores its saved chain and starts syncing from genesis. I reproduced this with the actual backend: old DatabaseSource::Auto reopened ParityDb and left db/full without CURRENT; the PR's resolver then selected and created RocksDB. Preserve detection of an actual existing database, including the old probe's missing-database handling, and cover ParityDb with a leftover RocksDB directory in regression tests.

  2. [P2] Preserve archive-canonical pruning in the migration command — README.md:173–177. The documented import-blocks command omits --state-pruning archive-canonical. The node's enforced pruning configuration is applied only in the normal startup branch, so import initializes the fresh database with the backend's default constrained pruning. The subsequent normal startup forces ArchiveCanonical and fails with incompatible pruning modes; replaying a large export also cannot preserve the intended historical state. Add --state-pruning archive-canonical to the import command and verify import followed by normal startup. A focused backend reproduction confirmed the failure and that explicitly selecting archive-canonical allows reopening.

Validation:

  • git diff --check and locked offline Cargo metadata passed.
  • scripts/fmt.sh -p sc-cli -p quantus-node -p quantus-runtime -- --check passed.
  • SKIP_WASM_BUILD=1 CARGO_BUILD_JOBS=8 cargo test --locked --offline -p sc-cli -p quantus-runtime --lib: 127 passed (50 CLI, 77 runtime).
  • Five temporary checks against the compiled PR CLI and real database backend passed, reproducing both findings and covering fresh/existing ParityDb, existing RocksDB, and the corrected pruning option. No tracked PR files were changed.
  • GitHub formatting, dependency checks, and Clippy/rustdoc passed. Linux/macOS build and test jobs were still in progress at review time. Local runtime tests used SKIP_WASM_BUILD=1; a full local node/Wasm build was not run.

@n13 n13 removed the bot-review label Oct 23, 2026
- `Auto` now selects RocksDB only when `db/full/CURRENT` exists. The old
  backend probe leaves an empty `db/full` (with `db_version`) next to a live
  ParityDb; a bare directory check would create a fresh RocksDB and restart
  such nodes from genesis. Unit tests cover fresh, leftover-dir and real
  RocksDB layouts.
- README/startup warning: `import-blocks` needs
  `--state-pruning archive-canonical`, otherwise the imported database is
  created with constrained pruning and the node refuses to reopen it.
@illuzen

illuzen commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Both addressed in 4e0a104.

1. Leftover db/full next to a live ParityDb. Auto is now resolved by resolve_auto_database, which selects RocksDB only when db/full/CURRENT exists (the manifest pointer every real RocksDB has; the backend's non-creating probe only ever leaves db_version). Unit tests in client/cli/src/config.rs cover: fresh base path → ParityDb; db/full containing only db_version → ParityDb; db/full/CURRENT present → RocksDB.

Reproduced against the real backend with a debug build: imported 2,000 mainnet blocks into a ParityDb base path, created db/full/db_version by hand, started the node with no --database flag → 💾 Database: ParityDb at …/paritydb/full, Highest known block at #2000, and db/full still contains only db_version afterwards.

2. Import pruning mode. README command now includes --state-pruning archive-canonical, with a sentence on why, and the RocksDB startup warning carries the same flag. Verified the full sequence: export-blocks --database rocksdb --to 2000 --binary → import-blocks --database paritydb --state-pruning archive-canonical --binary → plain quantus-node --chain mainnet --base-path <new> reopens and continues syncing.

@illuzen
illuzen requested a review from n13 October 23, 2026 10:57

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict: Request changes
Reviewer: GPT-6.1 Sol

Reviewed head 4e0a10429b6a1c5c93050a73afd4a4688f730270 against base 9e6d01f8bbefd126d87be6325e8cfc1f09825ae9. The earlier leftover-directory and migration-pruning findings are addressed, but one database-selection regression remains.

[P2] Propagate filesystem errors when detecting RocksDB — client/cli/src/config.rs:741–742. Path::is_file() returns false when metadata access fails, including PermissionDenied. If an existing RocksDB's db/full directory is inaccessible to the node user while the surrounding base path remains writable, the new resolver treats that database as absent and selects ParityDb. Startup then creates a fresh database and ignores the saved chain instead of reporting the access failure. The RocksDB warning is also bypassed because the selected backend is now ParityDb.

I reproduced this against the real backend: create RocksDB with a persisted auxiliary marker, remove access to db/full with mode 000, then resolve/open the default database. Reading CURRENT metadata fails with PermissionDenied; the previous backend Auto fails with Database version cannot be read from existing db_version file, whereas this resolver successfully opens a new ParityDb without the marker. Restore permissions afterward and the original RocksDB remains intact. Use a fallible filesystem probe, select ParityDb only for genuine absence, and propagate other I/O errors through database_config. Add a regression test for an unreadable existing database.

Validation:

  • git diff --check, locked offline Cargo metadata, and scripts/fmt.sh -p sc-cli -p quantus-node -p quantus-runtime -- --check passed.
  • SKIP_WASM_BUILD=1 CARGO_BUILD_JOBS=8 cargo test --locked --offline -p sc-cli -p quantus-runtime --lib: 130 passed (53 CLI, 77 runtime), including all three added selection tests.
  • Six temporary checks using the exact resolver functions extracted from this head and the real database backend passed: fresh/existing ParityDb, the old probe's leftover RocksDB directory, RocksDB preference when both databases exist, archive-canonical reopening, incompatible default pruning, and the access-error reproduction above. No tracked PR files were modified.
  • GitHub formatting and dependency checks passed. Linux/macOS build/test and Clippy/rustdoc jobs were still running at review time. A full local node/Wasm build was not run.

@n13 n13 removed the bot-review label Oct 23, 2026
Only a missing CURRENT manifest selects ParityDb; any other metadata
failure (e.g. unreadable db/full) is returned from database_config so an
existing RocksDB is never shadowed by a fresh ParityDb.
@illuzen

illuzen commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in HEAD.

rocksdb_exists now returns Result<bool>: fs::metadata(db/full/CURRENT) → Ok(is_file), NotFound → Ok(false), anything else is propagated through resolve_auto_database and database_config. Two regression tests added: auto_propagates_probe_errors_other_than_not_found (db/full is a regular file → NotADirectory, deterministic including as root) and auto_refuses_to_shadow_unreadable_rocksdb (db/full mode 000, cfg(unix), self-skips when permissions aren't enforced).

Reproduced with the built node: db/full/CURRENT present, chmod 000 db/full, plain start → Error: Io(Os { code: 13, kind: PermissionDenied }), no paritydb/ created. Clippy -D warnings clean for sc-cli with and without the rocksdb feature; sc-cli --lib 55 tests pass.

@illuzen
illuzen requested a review from n13 October 23, 2026 11:29

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict: Request changes
Reviewer: GPT-6.1 Sol

Reviewed head f894179addd1a208f348a7eccde1a9386b18eac0 against base 9e6d01f8bbefd126d87be6325e8cfc1f09825ae9. The earlier leftover-directory, migration-pruning, and swallowed-I/O-error findings are addressed. One test regression remains in the supported build without RocksDB.

[P2] Gate the resolver error tests on the RocksDB feature — client/cli/src/config.rs:783–790, with the same issue in auto_refuses_to_shadow_unreadable_rocksdb at lines 793–810. Both new tests unconditionally assert that resolve_auto_database returns an error for an invalid/inaccessible RocksDB path. With rocksdb disabled, that function intentionally skips the RocksDB probe and returns ParityDb, so these assertions fail. I reproduced this with CARGO_BUILD_JOBS=8 cargo test --locked --offline -p sc-cli --no-default-features --lib: 52 passed, 2 failed, at lines 790 and 810. Add #[cfg(feature = "rocksdb")] to both resolver-error tests (combine it with the existing Unix condition for the permission test), or make their resolver expectations feature-aware. Keep feature-independent coverage of rocksdb_exists separately if desired.

Validation:

  • git diff --check, locked offline Cargo metadata, and scripts/fmt.sh -p sc-cli -p quantus-node -p quantus-runtime -- --check passed.
  • SKIP_WASM_BUILD=1 CARGO_BUILD_JOBS=8 cargo test --locked --offline -p sc-cli -p quantus-runtime --lib: 132 passed (55 CLI, 77 runtime), including all five database-selection tests.
  • The CLI suite with --no-default-features compiled successfully, then failed the two tests described above; the other 52 passed.
  • GitHub dependency checks passed; the formatting job was still in progress at review time. A full local node/Wasm build was not run. No tracked PR files were modified.

@n13 n13 removed the bot-review label Oct 23, 2026
@illuzen

illuzen commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in HEAD: both probe-error tests are now gated on feature = "rocksdb" (the permission test on all(unix, feature = "rocksdb")). cargo test -p sc-cli --lib: 55 pass with default features, 52 pass with --no-default-features.

@illuzen
illuzen requested a review from n13 October 28, 2026 01:07

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict: Approve
Reviewer: GPT-6.1 Sol

Reviewed head 4d37c83c75b0a85c78bed3c65001a46ea04754a9 against base 9e6d01f8bbefd126d87be6325e8cfc1f09825ae9, including the complete five-file diff and the fixes discussed in the previous reviews. No blocking findings remain.

The latest commit correctly gates both RocksDB probe-error tests on the RocksDB feature. The default-feature suite still exercises those regressions, and the supported build without RocksDB now passes. The resolver preserves existing RocksDB databases, ignores the old probe's leftover directory, and propagates filesystem errors. The migration instructions and startup warning include --state-pruning archive-canonical. The runtime uses the existing SDK ParityDb weights consistently with the configured pallet weights and transaction extensions; leaving spec_version unchanged follows the documented release workflow.

Validation:

  • git diff --check, locked offline Cargo metadata, and scripts/fmt.sh -p sc-cli -p quantus-node -p quantus-runtime -- --check passed.
  • SKIP_WASM_BUILD=1 CARGO_BUILD_JOBS=8 cargo test --locked --offline -p sc-cli -p quantus-runtime --lib: 132 passed (55 CLI, 77 runtime), including all five database-selection tests.
  • CARGO_BUILD_JOBS=8 cargo test --locked --offline -p sc-cli --no-default-features --lib: 52 passed, with no failures.
  • GitHub formatting and dependency checks passed. Linux/macOS build/test and Clippy/rustdoc jobs were still running at review time. A full local node/Wasm build and production VPS storage benchmark were not run. The representative-VPS benchmark remains a release follow-up, as documented in the PR.

No tracked PR files were modified.

@n13 n13 removed the bot-review label Oct 28, 2026
…opens

Drops the sc-cli resolver and its tests. sc-client-db's own Auto probe
picks the backend; the node checks for a RocksDB manifest once the
database is open and warns if that is what was selected.
@illuzen

illuzen commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

Took a different approach in HEAD to stop reimplementing the backend's probe: sc-cli no longer resolves Auto at all. --database still defaults to auto, and sc-client-db's existing Auto handling picks the backend exactly as upstream does. After new_partial has opened the database, service::new_full checks for a RocksDB manifest (db/full/CURRENT) and logs the migration warning if that is what was opened. The resolver, rocksdb_exists, and the five selection tests are gone; client/cli/src/config.rs is back to a one-line default change.

Trade-off: the startup banner reads 💾 Database: Auto at …/paritydb/full for auto (upstream behaviour); the warning line identifies RocksDB when relevant.

Re-ran the layouts against the real backend with a debug build: fresh base path → ParityDb; existing RocksDB → opens RocksDB, warning logged; ParityDb with a leftover db/full (LOCK/LOG from the probe, no CURRENT) → continues on ParityDb, no warning; db/full mode 000 → backend fails with "Database version cannot be read from existing db_version file", no ParityDb created. Clippy -D warnings clean on sc-cli, quantus-node, quantus-runtime; sc-cli --lib 50 pass.

@illuzen
illuzen requested a review from n13 October 28, 2026 04:27

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict: Request changes
Reviewer: GPT-6.1 Sol

Reviewed head 4116269ff147778744f404924cfd222ef6f88edb against base 9e6d01f8bbefd126d87be6325e8cfc1f09825ae9, including the complete five-file diff and the latest removal of the custom resolver. Startup now correctly delegates backend selection to the existing database implementation, but the new default regresses a command that does not open the backend.

[P2] Resolve the effective backend before purging the default database — client/cli/src/config.rs:493–495. The new default passes an unresolved DatabaseSource::Auto to PurgeChainCmd::run. That command removes database_config.path().parent(), while Auto.path() always returns the ParityDb path, even when startup selects an existing RocksDB. Consequently, plain quantus-node purge-chain --chain <chain> --base-path <base> -y now returns success without removing an existing RocksDB; restarting retains the chain state the operator intended to purge. If both backends exist, it deletes the unused ParityDb and leaves the active RocksDB intact.

I reproduced both layouts using this head's compiled create_configuration and PurgeChainCmd with real databases: a persisted RocksDB auxiliary marker survives the default purge and is readable after reopening with Auto; explicitly selecting --database rocksdb removes it. Ensure filesystem-only commands select the same effective backend as startup before choosing their target, preserving probe-error handling, and add regressions for RocksDB-only and mixed-backend layouts.

Validation:

  • git diff --check, locked offline Cargo metadata, and scripts/fmt.sh -p sc-cli -p quantus-node -p quantus-runtime -- --check passed.
  • SKIP_WASM_BUILD=1 CARGO_BUILD_JOBS=8 cargo test --locked --offline -p sc-cli -p quantus-runtime --lib: 127 passed (50 CLI, 77 runtime).
  • CARGO_BUILD_JOBS=8 cargo test --locked --offline -p sc-cli --no-default-features --lib: 50 passed.
  • SKIP_WASM_BUILD=1 CARGO_BUILD_JOBS=8 cargo clippy --locked --offline -p sc-cli -p quantus-node -p quantus-runtime -- -D warnings passed.
  • Eight temporary compiled CLI/backend probes passed, confirming the purge regression and covering fresh/existing ParityDb, old probe leftovers, RocksDB preference, inaccessible RocksDB errors, archive-canonical reopening, and the default-pruning incompatibility. No tracked PR files were modified.
  • GitHub formatting, dependency checks, and Clippy/rustdoc passed. Linux/macOS build/test jobs were still running at review time. A full local node/Wasm build and production VPS storage benchmark were not run.

@n13 n13 removed the bot-review label Oct 28, 2026

This branch has not been deployed

No deployments
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.

2 participants