Repository navigation
Conversation
- `--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
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Reviewer: GPT-6.1 Sol
Reviewed head 48a34da54e8b0b9a226e7104409e2916dab12a3c against base 9e6d01f8bbefd126d87be6325e8cfc1f09825ae9.
-
[P2] Check for a real RocksDB before resolving
Auto— client/cli/src/config.rs:245–248. An existing ParityDb node that used--database autocan already have a leftoverdb/fulldirectory: the old backend's non-creating RocksDB probe creates that directory even when no RocksDB exists, then correctly opens ParityDb. The newexists()check selects RocksDB for the same layout, andBackend::newcreates 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: oldDatabaseSource::Autoreopened ParityDb and leftdb/fullwithoutCURRENT; 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. -
[P2] Preserve archive-canonical pruning in the migration command — README.md:173–177. The documented
import-blockscommand 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 forcesArchiveCanonicaland fails with incompatible pruning modes; replaying a large export also cannot preserve the intended historical state. Add--state-pruning archive-canonicalto 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 --checkand locked offline Cargo metadata passed.scripts/fmt.sh -p sc-cli -p quantus-node -p quantus-runtime -- --checkpassed.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.
- `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.
|
Both addressed in 4e0a104. 1. Leftover Reproduced against the real backend with a debug build: imported 2,000 mainnet blocks into a ParityDb base path, created 2. Import pruning mode. README command now includes |
n13
left a comment
There was a problem hiding this comment.
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, andscripts/fmt.sh -p sc-cli -p quantus-node -p quantus-runtime -- --checkpassed.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.
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.
|
Addressed in HEAD.
Reproduced with the built node: |
n13
left a comment
There was a problem hiding this comment.
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, andscripts/fmt.sh -p sc-cli -p quantus-node -p quantus-runtime -- --checkpassed.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-featurescompiled 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.
|
Addressed in HEAD: both probe-error tests are now gated on |
n13
left a comment
There was a problem hiding this comment.
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, andscripts/fmt.sh -p sc-cli -p quantus-node -p quantus-runtime -- --checkpassed.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.
…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.
|
Took a different approach in HEAD to stop reimplementing the backend's probe: Trade-off: the startup banner reads 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 |
n13
left a comment
There was a problem hiding this comment.
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, andscripts/fmt.sh -p sc-cli -p quantus-node -p quantus-runtime -- --checkpassed.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 warningspassed.- 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.
Summary
Makes ParityDb the node's default database backend and prices the runtime's
DbWeightfor it.--databasenow defaults toauto. An existing RocksDB database under<base>/chains/<id>/dbis reused; otherwise ParityDb is created under<base>/chains/<id>/paritydb.Autois resolved insidesc-cli(database_config) so the startup banner and every subcommand see the concrete backend instead of "Auto".frame_system::Config::DbWeightswitches fromRocksDbWeight(25 µs read / 100 µs write) toParityDbWeight(8 µs / 50 µs). Nospec_versionbump per repo convention.export-blocks→import-blocks --database paritydbmigration.Why
Storage access is the dominant cost in the wormhole settlement path (see #714).
benchmark storageagainst a mainnet snapshot (block 161,984, 693k keys, MacBook Air NVMe,--state-version 1):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 storagerun on a representative VPS before release would confirm the margin.Behavior matrix (smoke-tested with a debug build against real base paths)
💾 Database: ParityDb at …/paritydb/fulldb/(RocksDB), with or without strayparitydb/💾 Database: RocksDb at …/db/full+ warningparitydb/only💾 Database: ParityDb at …/paritydb/fullExplicit
--database rocksdb/--database paritydbare unchanged.Checks
scripts/fmt.shSKIP_WASM_BUILD=1 cargo clippy --locked -p sc-cli -p quantus-node -p quantus-runtime -- -D warningscleancargo test -p quantus-runtime(139 passed)