feat(storage): add sled implementation, make RocksDB optional feature
#395
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
WalkthroughIntroduces Sled as the default persistent storage backend, feature-gates RocksDB, and wires backend selection through CLI/config and storage factory. Updates Cargo features, tests, and adds Criterion benchmarks. Adjusts project metadata (homepage, license) and adds a benchmark profile. Minor test string updates in WASM lightclient. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as CLI Args
participant Cfg as Config Loader
participant Factory as Storage Factory
participant DB as Database (Sled/RocksDB/InMemory)
participant DA as DA Layer (Celestia/InMemory)
User->>CLI: Provide command + flags
CLI->>Cfg: load_config(path)
Cfg->>Cfg: merge defaults + file + CLI
Cfg-->>CLI: Config{ db_type, db_path, da_layer, ... }
CLI->>Factory: create_storage(Config.db)
alt db_type = Sled
Factory->>DB: SledConnection::new(path)
else db_type = RocksDB (feature)
Factory->>DB: RocksDBConnection::new(path)
else db_type = InMemory
Factory->>DB: InMemoryDatabase::new()
end
Factory-->>CLI: Arc<Box<dyn Database>>
CLI->>DA: initialize_da_layer(Config.da_layer)
alt Celestia
DA->>DA: retry connect (N times)
DA-->>CLI: DataAvailabilityLayer
else InMemory
DA-->>CLI: InMemoryDA
end
CLI-->>User: Running with selected DB and DA
sequenceDiagram
autonumber
participant Tree as JMT/KeyDirectoryTree
participant Sled as SledConnection
participant J as sled::Tree (JMT nodes)
participant V as sled::Tree (Value history)
Tree->>Sled: write_node_batch(NodeBatch)
Sled->>Sled: transaction begin
Sled->>J: insert encoded nodes (node:<key>)
Sled->>V: append value history (value_history:<key>:<version>)
Sled->>Sled: transaction commit
Sled-->>Tree: Ok
Tree->>Sled: get_value_option(max_version, key_hash)
Sled->>V: range scan value_history:<key>..<=max_version (rev)
V-->>Sled: latest <= max_version
Sled-->>Tree: Option<OwnedValue>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
crates/tests/src/lib.rs (1)
32-34: Test database setup correctly updated for Sled.The
setup_dbfunction properly switches from RocksDB to Sled while maintaining the same interface. The temporary directory usage is appropriate for Sled's file-based storage.Consider parameterizing tests to support multiple storage backends for comprehensive testing:
#[cfg(feature = "rocksdb")] fn setup_rocksdb() -> Arc<Box<dyn Database>> { let temp_dir = TempDir::new().unwrap(); let cfg = RocksDBConfig::new(temp_dir.path().to_str().unwrap()); let db = RocksDBConnection::new(&cfg).unwrap(); Arc::new(Box::new(db) as Box<dyn Database>) } fn setup_sled() -> Arc<Box<dyn Database>> { let temp_dir = TempDir::new().unwrap(); let cfg = SledConfig::new(temp_dir.path().to_str().unwrap()); let db = SledConnection::new(&cfg).unwrap(); Arc::new(Box::new(db) as Box<dyn Database>) }This would allow testing both backends when RocksDB is enabled, ensuring compatibility and performance comparisons.
crates/cli/src/cfg.rs (1)
187-189: Fix variable name and comment inconsistency.The code references
args.database.db_pathbut the comment still mentions "RocksDB directory". This should be updated for consistency.Apply this diff to fix the inconsistency:
- if let Some(rocksdb_path) = &args.database.db_path { - fs::create_dir_all(rocksdb_path).context("Failed to create RocksDB directory")?; + if let Some(db_path) = &args.database.db_path { + fs::create_dir_all(db_path).context("Failed to create database directory")?; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.toml(3 hunks)crates/cli/Cargo.toml(1 hunks)crates/cli/src/cfg.rs(6 hunks)crates/storage/Cargo.toml(2 hunks)crates/storage/src/database.rs(1 hunks)crates/storage/src/lib.rs(1 hunks)crates/storage/src/rocksdb.rs(0 hunks)crates/storage/src/sled.rs(1 hunks)crates/storage/src/tests/mod.rs(4 hunks)crates/tests/src/lib.rs(2 hunks)crates/tree/Cargo.toml(1 hunks)crates/tree/benches/database_comparison.rs(1 hunks)crates/tree/src/tests/mod.rs(4 hunks)verification_keys/keys.json(1 hunks)
💤 Files with no reviewable changes (1)
- crates/storage/src/rocksdb.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (8)
verification_keys/keys.json (1)
Learnt from: sebasti810
PR: deltadevsde/prism#342
File: crates/cli/src/main.rs:152-155
Timestamp: 2025-06-17T07:14:03.223Z
Learning: In the Prism codebase, the verifying_key field in NetworkConfig was changed from Option<VerifyingKey> to VerifyingKey because it's always required by all node types (LightClient, Prover, FullNode) and a sensible default (generated key) can be provided when not explicitly configured.
crates/storage/src/lib.rs (4)
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/*.rs : File organization: Group related functionality in modules
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/*.rs : Documentation: Add comments for public APIs and complex logic
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/{tests,**/*_test}.rs : Include tests for new functionality
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/common/src/test_ops.rs:27-44
Timestamp: 2024-10-29T08:34:32.445Z
Learning: In `crates/common/src/test_ops.rs`, methods in test code are deliberately kept short for brevity, even if this results in abbreviated method names.
crates/tests/src/lib.rs (4)
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/common/src/test_ops.rs:27-44
Timestamp: 2024-10-29T08:34:32.445Z
Learning: In `crates/common/src/test_ops.rs`, methods in test code are deliberately kept short for brevity, even if this results in abbreviated method names.
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/{tests,**/*_test}.rs : Include tests for new functionality
Learnt from: distractedm1nd
PR: deltadevsde/prism#381
File: crates/node_types/prover/src/prover_engine/sp1_prover.rs:146-152
Timestamp: 2025-07-11T11:09:35.000Z
Learning: In the recursive prover in `crates/node_types/prover/src/prover_engine/sp1_prover.rs`, it's correct to use the compressed proof from `prev_epoch.stark` for verification while using the public values from `prev_epoch.snark.public_values` for state continuity in the recursive proof chain. This is intentional design where STARK and SNARK proofs serve different purposes.
Cargo.toml (3)
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/*.rs : Use rustfmt with project settings (merge_imports=true, imports_granularity="Crate", max_width=100)
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/*.rs : Follow Rust Coding Standards
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
crates/tree/src/tests/mod.rs (5)
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/{tests,**/*_test}.rs : Include tests for new functionality
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/common/src/test_ops.rs:27-44
Timestamp: 2024-10-29T08:34:32.445Z
Learning: In `crates/common/src/test_ops.rs`, methods in test code are deliberately kept short for brevity, even if this results in abbreviated method names.
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/*.rs : Follow Rust Coding Standards
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/*.rs : Naming: follow Rust conventions (snake_case for functions/variables, CamelCase for types)
crates/storage/src/tests/mod.rs (3)
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/{tests,**/*_test}.rs : Include tests for new functionality
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/common/src/test_ops.rs:27-44
Timestamp: 2024-10-29T08:34:32.445Z
Learning: In `crates/common/src/test_ops.rs`, methods in test code are deliberately kept short for brevity, even if this results in abbreviated method names.
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in `crates/node_types/prover/src/prover/tests.rs`.
crates/cli/src/cfg.rs (3)
Learnt from: sebasti810
PR: deltadevsde/prism#342
File: crates/cli/src/main.rs:152-155
Timestamp: 2025-06-17T07:14:03.223Z
Learning: In the Prism codebase, the verifying_key field in NetworkConfig was changed from Option<VerifyingKey> to VerifyingKey because it's always required by all node types (LightClient, Prover, FullNode) and a sensible default (generated key) can be provided when not explicitly configured.
Learnt from: distractedm1nd
PR: deltadevsde/prism#150
File: crates/common/src/keys.rs:27-31
Timestamp: 2024-10-29T15:02:10.475Z
Learning: In the Rust codebase of `prism`, within the `VerifyingKey::as_bytes` method in `crates/common/src/keys.rs`, returning a `Vec<u8>` is necessary because the underlying key types (`Secp256k1VerifyingKey` and `Ed25519VerifyingKey`) provide their byte representations by value. Returning a `&[u8]` would involve borrowing from a temporary value, which is not feasible.
Learnt from: distractedm1nd
PR: deltadevsde/prism#381
File: crates/node_types/prover/src/prover_engine/sp1_prover.rs:146-152
Timestamp: 2025-07-11T11:09:35.000Z
Learning: In the recursive prover in `crates/node_types/prover/src/prover_engine/sp1_prover.rs`, it's correct to use the compressed proof from `prev_epoch.stark` for verification while using the public values from `prev_epoch.snark.public_values` for state continuity in the recursive proof chain. This is intentional design where STARK and SNARK proofs serve different purposes.
crates/tree/benches/database_comparison.rs (2)
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/common/src/test_ops.rs:27-44
Timestamp: 2024-10-29T08:34:32.445Z
Learning: In `crates/common/src/test_ops.rs`, methods in test code are deliberately kept short for brevity, even if this results in abbreviated method names.
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/{tests,**/*_test}.rs : Include tests for new functionality
🧬 Code Graph Analysis (3)
crates/storage/src/tests/mod.rs (4)
crates/tests/src/lib.rs (1)
setup_db(30-35)crates/storage/src/rocksdb.rs (4)
new(23-27)new(37-45)get_value_option(206-230)get_latest_epoch_height(173-182)crates/storage/src/sled.rs (4)
new(32-38)new(60-81)get_value_option(226-247)get_latest_epoch_height(186-200)crates/storage/src/database.rs (1)
get_latest_epoch_height(24-24)
crates/tree/benches/database_comparison.rs (3)
crates/tree/src/tests/mod.rs (12)
KeyHash(85-85)KeyHash(196-196)KeyHash(250-250)KeyHash(279-279)KeyHash(337-337)KeyHash(338-338)KeyHash(374-374)KeyHash(375-375)KeyHash(422-422)KeyHash(431-431)KeyHash(432-432)KeyHash(487-487)crates/storage/src/rocksdb.rs (3)
new(23-27)new(37-45)with(64-71)crates/storage/src/sled.rs (2)
new(32-38)new(60-81)
crates/storage/src/sled.rs (2)
crates/storage/src/rocksdb.rs (15)
new(23-27)new(37-45)get_commitment(84-94)set_commitment(96-101)get_last_synced_height(103-112)set_last_synced_height(114-116)get_epoch(118-131)add_epoch(133-171)get_latest_epoch_height(173-182)get_latest_epoch(184-187)flush_database(189-191)get_node_option(195-204)get_value_option(206-230)get_rightmost_leaf(234-236)write_node_batch(240-266)crates/storage/src/database.rs (9)
get_commitment(18-18)set_commitment(19-19)get_last_synced_height(27-27)set_last_synced_height(28-28)get_epoch(21-21)add_epoch(22-22)get_latest_epoch_height(24-24)get_latest_epoch(25-25)flush_database(30-30)
🔇 Additional comments (39)
verification_keys/keys.json (1)
1-1: Verification key update looks structurally correct.The JSON format is valid and the hexadecimal strings are properly formatted. However, ensure that these cryptographic key updates are intentional and properly coordinated with the storage backend changes.
crates/storage/src/lib.rs (1)
3-6: Proper module organization and feature gating.The addition of the
sledmodule and conditional compilation of therocksdbmodule correctly implements the storage backend transition. The organization follows Rust conventions and aligns with the PR objectives of making Sled the default and RocksDB optional.crates/cli/Cargo.toml (1)
15-15: Correct feature gating implementation.The
rocksdbfeature is properly configured to enable optional RocksDB support through the storage crate's feature flag. This correctly implements the architectural change to make RocksDB optional while defaulting to Sled.crates/storage/src/database.rs (1)
10-14: All RocksDB references are properly feature-gatedI’ve reviewed every occurrence of
RocksDB,rocksdb,RocksDBConfig, andRocksDBConnectionacross benches, tests, the storage module, and the CLI. Each reference is either within a#[cfg(feature = "rocksdb")]block or in a feature-gated module (storage/src/rocksdb.rsvialib.rs). No unguarded usages were found.crates/tests/src/lib.rs (1)
21-21: Import update correctly switches to Sled backend.The import change from RocksDB to Sled components is consistent with the storage backend transition.
crates/tree/Cargo.toml (1)
35-46: LGTM! Well-structured benchmarking and feature flag additions.The changes correctly:
- Add criterion for benchmarking with HTML reports feature
- Include jmt as a dev-dependency for benchmarking
- Configure the benchmark with
harness = false(required for criterion)- Add the
rocksdbfeature flag that properly depends onprism-storage/rocksdbThis aligns perfectly with the PR objective of making RocksDB optional while adding benchmarking capabilities.
Cargo.toml (4)
11-11: Homepage update looks good.The domain change from
deltadevs.xyztoprism.rsappears to be a branding update.
13-13: Verify the license change impact.The license has been changed from MIT to AGPL-3.0, which is a significant change that affects how the software can be used and distributed. The AGPL-3.0 is a copyleft license that requires derivative works to also be open source.
Please confirm that this license change has been properly reviewed and approved by all stakeholders, as it fundamentally changes the licensing terms of the project.
85-85: Sled dependency addition aligns with PR objectives.The addition of
sled = "0.34.7"as a workspace dependency correctly supports the transition to Sled as the new default storage backend.
281-284: Benchmark profile configuration is appropriate.The benchmark profile with
opt-level = 3,debug = false, andlto = trueis properly configured for performance benchmarking, ensuring optimized builds for accurate measurements.crates/storage/Cargo.toml (3)
13-15: Excellent feature flag implementation.The features section correctly implements the optional RocksDB support:
- Empty default features ensure Sled is the default
rocksdbfeature uses properdep:rocksdbsyntax for optional dependenciesThis follows Rust best practices for conditional compilation.
19-19: Sled dependency addition is correct.Adding
sled = { workspace = true }as a required dependency aligns with making Sled the new default storage backend.
28-30: Proper optional dependency configuration.The RocksDB dependency is correctly configured as optional with the
multi-threaded-cffeature, which is appropriate for the prism use case.crates/tree/src/tests/mod.rs (5)
12-22: Proper conditional imports and dependencies.The imports are correctly structured:
- Sled imports are unconditional (default backend)
- RocksDB imports are feature-gated
- TempDir import added for temporary directory management in tests
24-30: DBType enum correctly updated.The enum properly reflects the new backend structure:
Sledvariant added unconditionallyRocksDBvariant feature-gated- Maintains existing
InMemoryandMockvariants
36-41: Trait implementations for new backend.The
TreeReadWritertrait implementations forSledConnectionandBox<SledConnection>are correctly added, maintaining consistency with other backend implementations.
54-59: Sled database setup implementation is correct.The setup logic for Sled:
- Uses temporary directory for test isolation
- Follows the same pattern as RocksDB setup
- Proper error handling with
unwrap()(acceptable in tests)
447-461: Test generation macro properly updated.The macro correctly:
- Feature-gates RocksDB tests
- Adds new Sled tests with
Secp256r1algorithm- Maintains existing test patterns
This ensures comprehensive testing across all supported backends.
crates/cli/src/cfg.rs (5)
11-18: Proper conditional imports for backend support.The imports are correctly structured with RocksDB imports feature-gated and Sled imports unconditional, aligning with the new default backend strategy.
142-142: Default storage backend correctly changed to Sled.The initialization now defaults to
StorageBackend::Sledinstead of RocksDB, which aligns with the PR objectives of making Sled the default backend.
158-163: DBValues enum properly updated.The enum correctly:
- Sets
Sledas the default variant- Feature-gates
RocksDBvariant- Maintains
InMemoryoption
167-173: DatabaseArgs updated to be backend-agnostic.The field name change from
rocksdb_pathtodb_pathand updated documentation make the configuration more generic and applicable to both Sled and RocksDB.
324-330: Sled initialization properly implemented.The
initialize_dbfunction correctly handles Sled backend initialization with proper error handling and type conversion, following the same pattern as RocksDB.crates/storage/src/tests/mod.rs (6)
1-18: LGTM! Well-structured multi-backend test setup.The refactored setup function cleanly supports multiple storage backends with appropriate conditional compilation for RocksDB.
20-67: Good refactoring to support multiple backends.The existing tests have been cleanly adapted to work with the parameterized backend setup while maintaining their original test logic.
69-77: Good addition of sync height test coverage.The test properly verifies the sync height persistence functionality across all storage backends.
79-96: Valuable test for batch operation atomicity.This test ensures that multiple values written in a single batch are atomically persisted, which is crucial for data consistency.
98-121: Comprehensive epoch operations test coverage.Good coverage of epoch operations including the important negative test case for non-existent epochs. The test properly validates both storage and retrieval of epoch data.
123-143: Excellent test for versioned value retrieval.This test thoroughly validates the versioned storage functionality, ensuring that
get_value_optioncorrectly returns the highest version less than or equal to the requested max version.crates/tree/benches/database_comparison.rs (6)
1-38: Well-structured benchmark setup with proper resource management.The setup functions correctly create isolated temporary databases and return the TempDir to ensure it remains alive during benchmarks.
40-102: Good benchmark design for direct database operations.The benchmarks properly isolate setup from measurement and use
black_boxto prevent compiler optimizations.
104-178: Comprehensive JMT operation benchmarks.Good coverage of both batch write and individual read operations with appropriate data sizes for meaningful measurements.
180-245: Well-designed benchmarks for tree operations.Good use of
iter_with_setupto ensure each benchmark iteration starts with a clean state, testing realistic transaction processing scenarios.
247-575: Comprehensive benchmark suite with good coverage.The benchmarks cover a wide range of operations including updates, data operations, batch reads, and varying payload sizes. The consistent use of
iter_with_setupensures reliable measurements.
577-588: Standard Criterion configuration properly set up.All benchmark functions are correctly registered with the Criterion framework.
crates/storage/src/sled.rs (4)
1-22: Good key prefix organization matching RocksDB implementation.The constants are well-organized and maintain compatibility with the existing RocksDB key structure.
24-82: Well-designed configuration with flexible options.Good use of the builder pattern for configuration and proper separation of node and value trees. The Arc wrapper enables safe concurrent access.
84-126: Correct implementation of commitment and sync height operations.The implementation properly handles byte conversions with BigEndian encoding and provides descriptive error messages.
216-252: Correct TreeReader implementation with proper versioned value retrieval.The reverse iteration logic for finding the highest version is well-implemented. The unimplemented snapshot restoration method is properly documented.
| fn flush_database(&self) -> Result<()> { | ||
| // sled doesn't have a destroy method, so we need to drop the connection and remove the | ||
| // directory | ||
| drop(self.connection.clone()); | ||
| std::fs::remove_dir_all(&self.path)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Potential issue with flush_database implementation.
Dropping a clone of the Arc connection may not be sufficient if other references exist. This could cause the directory removal to fail.
Consider implementing a more robust shutdown mechanism:
fn flush_database(&self) -> Result<()> {
- // sled doesn't have a destroy method, so we need to drop the connection and remove the
- // directory
- drop(self.connection.clone());
+ // Close the database before removing the directory
+ // Note: This requires ensuring no other references exist
+ self.connection.flush()?;
std::fs::remove_dir_all(&self.path)?;
Ok(())
}Alternatively, document that flush_database requires exclusive access to work correctly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn flush_database(&self) -> Result<()> { | |
| // sled doesn't have a destroy method, so we need to drop the connection and remove the | |
| // directory | |
| drop(self.connection.clone()); | |
| std::fs::remove_dir_all(&self.path)?; | |
| Ok(()) | |
| } | |
| fn flush_database(&self) -> Result<()> { | |
| // Close the database before removing the directory | |
| // Note: This requires ensuring no other references exist | |
| self.connection.flush()?; | |
| std::fs::remove_dir_all(&self.path)?; | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In crates/storage/src/sled.rs around lines 207 to 213, the flush_database method
drops only a clone of the Arc connection, which may not close the database if
other references exist, causing remove_dir_all to fail. To fix this, implement a
proper shutdown mechanism that ensures all references to the connection are
dropped before removing the directory, or clearly document that flush_database
requires exclusive ownership of the connection to work correctly.
| impl TreeWriter for SledConnection { | ||
| fn write_node_batch(&self, node_batch: &NodeBatch) -> Result<()> { | ||
| (&self.node_tree, &self.value_tree) | ||
| .transaction(|(tx_node, tx_values)| { | ||
| for (node_key, node) in node_batch.nodes() { | ||
| let key = node_key.encode_to_bytes().unwrap(); | ||
| let value = node.encode_to_bytes().unwrap(); | ||
| tx_node.insert(key, value.as_slice()).unwrap(); | ||
| } | ||
|
|
||
| for ((version, key_hash), value) in node_batch.values() { | ||
| let value_key = key_hash.0; | ||
| let encoded_value = value | ||
| .as_ref() | ||
| .map(|v| v.encode_to_bytes()) | ||
| .transpose() | ||
| .unwrap() | ||
| .unwrap_or_default(); | ||
| let version_bytes = version.to_be_bytes(); | ||
|
|
||
| let mut fkey = Vec::with_capacity(32 + version_bytes.len()); | ||
| fkey.extend_from_slice(&value_key); | ||
| fkey.extend_from_slice(&version_bytes); | ||
| tx_values.insert(fkey, encoded_value.as_slice()).unwrap(); | ||
| } | ||
|
|
||
| Ok::<(), sled::transaction::ConflictableTransactionError<anyhow::Error>>(()) | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling in write_node_batch.
While the transaction ensures atomicity, the error handling could be more robust.
The current implementation uses unwrap() which could panic. Consider propagating errors properly:
fn write_node_batch(&self, node_batch: &NodeBatch) -> Result<()> {
(&self.node_tree, &self.value_tree)
.transaction(|(tx_node, tx_values)| {
for (node_key, node) in node_batch.nodes() {
- let key = node_key.encode_to_bytes().unwrap();
- let value = node.encode_to_bytes().unwrap();
- tx_node.insert(key, value.as_slice()).unwrap();
+ let key = node_key.encode_to_bytes()
+ .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?;
+ let value = node.encode_to_bytes()
+ .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?;
+ tx_node.insert(key, value.as_slice())
+ .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?;
}
for ((version, key_hash), value) in node_batch.values() {
let value_key = key_hash.0;
let encoded_value = value
.as_ref()
.map(|v| v.encode_to_bytes())
.transpose()
- .unwrap()
+ .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?
.unwrap_or_default();
let version_bytes = version.to_be_bytes();
let mut fkey = Vec::with_capacity(32 + version_bytes.len());
fkey.extend_from_slice(&value_key);
fkey.extend_from_slice(&version_bytes);
- tx_values.insert(fkey, encoded_value.as_slice()).unwrap();
+ tx_values.insert(fkey, encoded_value.as_slice())
+ .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?;
}
Ok::<(), sled::transaction::ConflictableTransactionError<anyhow::Error>>(())
})
- .unwrap();
+ .map_err(|e| match e {
+ sled::transaction::TransactionError::Abort(e) => e,
+ sled::transaction::TransactionError::Storage(e) => e.into(),
+ })?;
Ok(())
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/storage/src/sled.rs between lines 254 and 286, the write_node_batch
method uses unwrap() calls inside the transaction closure, which can cause
panics on errors. To fix this, replace unwrap() with proper error propagation by
using the ? operator on fallible calls like encode_to_bytes() and insert().
Adjust the closure's return type to propagate errors correctly, and ensure the
outer function returns an appropriate Result to handle transaction errors
gracefully instead of panicking.
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
| } | ||
|
|
||
| fn get_epoch(&self, height: &u64) -> anyhow::Result<prism_da::FinalizedEpoch> { | ||
| let key = format!("{}{}", KEY_PREFIX_EPOCHS, height); |
There was a problem hiding this comment.
string keys instead of directly to bytes.
There was a problem hiding this comment.
oh shiettttt
| .context("Failed to ensure config file exists")?; | ||
|
|
||
| if let Some(rocksdb_path) = &args.database.rocksdb_path { | ||
| if let Some(rocksdb_path) = &args.database.db_path { |
There was a problem hiding this comment.
| if let Some(rocksdb_path) = &args.database.db_path { | |
| if let Some(db_path) = &args.database.db_path { |
|
|
||
| if let Some(rocksdb_path) = &args.database.rocksdb_path { | ||
| if let Some(rocksdb_path) = &args.database.db_path { | ||
| fs::create_dir_all(rocksdb_path).context("Failed to create RocksDB directory")?; |
There was a problem hiding this comment.
| fs::create_dir_all(rocksdb_path).context("Failed to create RocksDB directory")?; | |
| fs::create_dir_all(db_path).context("Failed to create DB directory")?; |
we could customise the context comment as well but I don't think it's necessary.
| )?)) | ||
| } | ||
|
|
||
| fn get_latest_epoch(&self) -> anyhow::Result<prism_da::FinalizedEpoch> { |
There was a problem hiding this comment.
we should import prism_da::FinalizedEpoch
| .unwrap_or_default(); | ||
| let version_bytes = version.to_be_bytes(); | ||
|
|
||
| let mut fkey = Vec::with_capacity(32 + version_bytes.len()); |
There was a problem hiding this comment.
let's think of a better variable name here
crates/storage/src/sled.rs
Outdated
| let mut fkey = Vec::with_capacity(32 + version_bytes.len()); | ||
| fkey.extend_from_slice(&value_key); | ||
| fkey.extend_from_slice(&version_bytes); | ||
| tx_values.insert(fkey, encoded_value.as_slice()).unwrap(); |
There was a problem hiding this comment.
do we need as_slice() here?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/storage/src/sled.rs (2)
207-213: Potential issue with flush_database implementation.Dropping a clone of the Arc connection may not be sufficient if other references exist. This could cause the directory removal to fail.
Consider implementing a more robust shutdown mechanism:
fn flush_database(&self) -> Result<()> { - // sled doesn't have a destroy method, so we need to drop the connection and remove the - // directory - drop(self.connection.clone()); + // Close the database before removing the directory + // Note: This requires ensuring no other references exist + self.connection.flush()?; std::fs::remove_dir_all(&self.path)?; Ok(()) }Alternatively, document that
flush_databaserequires exclusive access to work correctly.
254-286: Fix error handling in write_node_batch transaction.The transaction contains multiple
.unwrap()calls that can cause panics. Based on the retrieved learnings, this codebase should use proper error handling with Result types.Apply this fix:
fn write_node_batch(&self, node_batch: &NodeBatch) -> Result<()> { (&self.node_tree, &self.value_tree) .transaction(|(tx_node, tx_values)| { for (node_key, node) in node_batch.nodes() { - let key = node_key.encode_to_bytes().unwrap(); - let value = node.encode_to_bytes().unwrap(); - tx_node.insert(key, value).unwrap(); + let key = node_key.encode_to_bytes() + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; + let value = node.encode_to_bytes() + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; + tx_node.insert(key, value) + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; } for ((version, key_hash), value) in node_batch.values() { let value_key = key_hash.0; let encoded_value = value .as_ref() .map(|v| v.encode_to_bytes()) .transpose() - .unwrap() + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))? .unwrap_or_default(); let version_bytes = version.to_be_bytes(); let mut fkey = Vec::with_capacity(32 + version_bytes.len()); fkey.extend_from_slice(&value_key); fkey.extend_from_slice(&version_bytes); - tx_values.insert(fkey, encoded_value).unwrap(); + tx_values.insert(fkey, encoded_value) + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; } Ok::<(), sled::transaction::ConflictableTransactionError<anyhow::Error>>(()) }) - .unwrap(); + .map_err(|e| match e { + sled::transaction::TransactionError::Abort(e) => e, + sled::transaction::TransactionError::Storage(e) => e.into(), + })?; Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/node_types/wasm-lightclient/src/tests/mod.rs(2 hunks)crates/storage/src/sled.rs(1 hunks)crates/storage/src/tests/mod.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
crates/node_types/wasm-lightclient/src/tests/mod.rs (6)
Learnt from: jns-ps
PR: #147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in crates/node_types/prover/src/prover/tests.rs.
Learnt from: jns-ps
PR: #147
File: crates/common/src/test_ops.rs:27-44
Timestamp: 2024-10-29T08:34:32.445Z
Learning: In crates/common/src/test_ops.rs, methods in test code are deliberately kept short for brevity, even if this results in abbreviated method names.
Learnt from: distractedm1nd
PR: #381
File: crates/node_types/prover/src/prover_engine/sp1_prover.rs:146-152
Timestamp: 2025-07-11T11:09:35.000Z
Learning: In the recursive prover in crates/node_types/prover/src/prover_engine/sp1_prover.rs, it's correct to use the compressed proof from prev_epoch.stark for verification while using the public values from prev_epoch.snark.public_values for state continuity in the recursive proof chain. This is intentional design where STARK and SNARK proofs serve different purposes.
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to /{tests,/*_test}.rs : Include tests for new functionality
Learnt from: distractedm1nd
PR: #146
File: crates/common/src/tree.rs:114-126
Timestamp: 2024-11-01T07:52:07.324Z
Learning: In UpdateProof::verify within crates/common/src/tree.rs, the old_hashchain and hashchain_after_update are different hashchains, so caching their serialized values is not feasible.
Learnt from: sebasti810
PR: #389
File: crates/tests/src/lib.rs:174-175
Timestamp: 2025-07-21T09:28:37.652Z
Learning: In test files (such as crates/tests/src/lib.rs), using .unwrap() is acceptable practice and does not need to be replaced with proper error handling. Panicking behavior in tests is considered acceptable in this codebase.
crates/storage/src/tests/mod.rs (5)
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to /{tests,/*_test}.rs : Include tests for new functionality
Learnt from: jns-ps
PR: #147
File: crates/common/src/test_ops.rs:27-44
Timestamp: 2024-10-29T08:34:32.445Z
Learning: In crates/common/src/test_ops.rs, methods in test code are deliberately kept short for brevity, even if this results in abbreviated method names.
Learnt from: jns-ps
PR: #147
File: crates/node_types/prover/src/prover/tests.rs:120-129
Timestamp: 2024-10-25T08:50:21.262Z
Learning: Avoid including debug print statements in test functions in crates/node_types/prover/src/prover/tests.rs.
Learnt from: sebasti810
PR: #389
File: crates/tests/src/lib.rs:174-175
Timestamp: 2025-07-21T09:28:37.652Z
Learning: In test files (such as crates/tests/src/lib.rs), using .unwrap() is acceptable practice and does not need to be replaced with proper error handling. Panicking behavior in tests is considered acceptable in this codebase.
Learnt from: sebasti810
PR: #389
File: crates/tests/src/lib.rs:208-208
Timestamp: 2025-07-21T09:28:36.395Z
Learning: In test files like crates/tests/src/lib.rs, using .unwrap() for error handling is acceptable as tests should fail fast when unexpected conditions occur, making panics informative for debugging purposes.
crates/storage/src/sled.rs (4)
Learnt from: sebasti810
PR: #389
File: crates/tests/src/lib.rs:208-208
Timestamp: 2025-07-21T09:28:36.395Z
Learning: In test files like crates/tests/src/lib.rs, using .unwrap() for error handling is acceptable as tests should fail fast when unexpected conditions occur, making panics informative for debugging purposes.
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T12:18:09.576Z
Learning: Applies to **/*.rs : Error handling: Use Result types with descriptive error messages
Learnt from: sebasti810
PR: #389
File: crates/tests/src/lib.rs:174-175
Timestamp: 2025-07-21T09:28:37.652Z
Learning: In test files (such as crates/tests/src/lib.rs), using .unwrap() is acceptable practice and does not need to be replaced with proper error handling. Panicking behavior in tests is considered acceptable in this codebase.
Learnt from: distractedm1nd
PR: #150
File: crates/common/src/keys.rs:27-31
Timestamp: 2024-10-29T15:02:10.475Z
Learning: In the Rust codebase of prism, within the VerifyingKey::as_bytes method in crates/common/src/keys.rs, returning a Vec<u8> is necessary because the underlying key types (Secp256k1VerifyingKey and Ed25519VerifyingKey) provide their byte representations by value. Returning a &[u8] would involve borrowing from a temporary value, which is not feasible.
🧬 Code Graph Analysis (1)
crates/storage/src/sled.rs (4)
crates/tree/src/tests/mod.rs (12)
KeyHash(85-85)KeyHash(196-196)KeyHash(250-250)KeyHash(279-279)KeyHash(337-337)KeyHash(338-338)KeyHash(374-374)KeyHash(375-375)KeyHash(422-422)KeyHash(431-431)KeyHash(432-432)KeyHash(487-487)crates/storage/src/rocksdb.rs (14)
new(23-27)new(37-45)get_commitment(84-94)set_commitment(96-101)get_last_synced_height(103-112)set_last_synced_height(114-116)get_epoch(118-131)add_epoch(133-171)get_latest_epoch_height(173-182)get_latest_epoch(184-187)flush_database(189-191)get_node_option(195-204)get_value_option(206-230)write_node_batch(240-266)crates/storage/src/database.rs (9)
get_commitment(18-18)set_commitment(19-19)get_last_synced_height(27-27)set_last_synced_height(28-28)get_epoch(21-21)add_epoch(22-22)get_latest_epoch_height(24-24)get_latest_epoch(25-25)flush_database(30-30)crates/tree/src/proofs.rs (1)
node(245-245)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: clippy
- GitHub Check: wasm-test
- GitHub Check: coverage
- GitHub Check: unused dependencies
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: build-and-push-image
🔇 Additional comments (9)
crates/storage/src/sled.rs (4)
1-23: LGTM: Clean imports and well-organized constants.The import structure is well-organized and the key prefixes follow a consistent naming pattern that aligns with the RocksDB implementation for compatibility.
24-49: LGTM: Well-designed configuration with builder pattern.The
SledConfigstruct provides a clean builder pattern API with sensible defaults, making it easy to configure the sled database instance.
59-82: LGTM: Proper initialization with error handling.The constructor properly applies configuration options and handles potential errors from sled operations. The use of
Arc<Db>allows safe sharing of the connection across threads.
216-252: LGTM: Proper TreeReader implementation with good error handling.The
get_node_optionandget_value_optionmethods correctly handle sled operations and provide proper error propagation. The range iteration logic for versioned values is well-implemented.crates/node_types/wasm-lightclient/src/tests/mod.rs (1)
214-214: LGTM: Clean terminology update in test assertions.The changes from "backwards sync" to "historical sync" properly align the test expectations with the updated terminology used throughout the project. This ensures test consistency with the new event messages.
Also applies to: 320-320
crates/storage/src/tests/mod.rs (4)
3-20: LGTM: Well-structured backend abstraction.The
setup_dbfunction provides a clean abstraction for creating different database backends, properly handling conditional compilation for RocksDB. The imports are well-organized.
22-69: LGTM: Properly updated existing tests.The existing test functions have been correctly refactored to accept a
StorageBackendparameter while maintaining their original test logic and assertions.
71-145: LGTM: Comprehensive new test coverage.The new test functions provide excellent coverage for:
- Sync height operations
- Transaction consistency
- Epoch lifecycle operations
- Range iteration with multiple versions
The test implementations follow good practices and provide meaningful assertions.
147-181: LGTM: Efficient test generation with proper feature gating.The macro efficiently generates backend-specific test functions with proper conditional compilation for RocksDB. The use of
paste!for identifier generation is clean and the test invocations cover all the test functions comprehensively.
| self.connection | ||
| .transaction(|tx| { | ||
| tx.insert(epoch_key.as_bytes(), epoch_data.as_slice())?; | ||
| tx.insert(KEY_LATEST_EPOCH_HEIGHT, &height_bytes)?; | ||
| Ok::<(), sled::transaction::ConflictableTransactionError<anyhow::Error>>(()) | ||
| }) | ||
| .unwrap(); |
There was a problem hiding this comment.
Fix transaction error handling.
The transaction uses .unwrap() which can cause panics on transaction errors. This violates the error handling best practices for this codebase.
Apply this fix:
self.connection
.transaction(|tx| {
tx.insert(epoch_key.as_bytes(), epoch_data.as_slice())?;
tx.insert(KEY_LATEST_EPOCH_HEIGHT, &height_bytes)?;
Ok::<(), sled::transaction::ConflictableTransactionError<anyhow::Error>>(())
})
- .unwrap();
+ .map_err(|e| match e {
+ sled::transaction::TransactionError::Abort(e) => e,
+ sled::transaction::TransactionError::Storage(e) => e.into(),
+ })?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.connection | |
| .transaction(|tx| { | |
| tx.insert(epoch_key.as_bytes(), epoch_data.as_slice())?; | |
| tx.insert(KEY_LATEST_EPOCH_HEIGHT, &height_bytes)?; | |
| Ok::<(), sled::transaction::ConflictableTransactionError<anyhow::Error>>(()) | |
| }) | |
| .unwrap(); | |
| self.connection | |
| .transaction(|tx| { | |
| tx.insert(epoch_key.as_bytes(), epoch_data.as_slice())?; | |
| tx.insert(KEY_LATEST_EPOCH_HEIGHT, &height_bytes)?; | |
| Ok::<(), sled::transaction::ConflictableTransactionError<anyhow::Error>>(()) | |
| }) | |
| .map_err(|e| match e { | |
| sled::transaction::TransactionError::Abort(e) => e, | |
| sled::transaction::TransactionError::Storage(e) => e.into(), | |
| })?; |
🤖 Prompt for AI Agents
In crates/storage/src/sled.rs around lines 175 to 181, the transaction call uses
.unwrap() which can cause panics on errors. Replace .unwrap() with proper error
handling by propagating or returning the error instead of panicking. Modify the
function to return a Result type if needed, and handle the transaction error
gracefully according to the codebase's error handling conventions.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/storage/src/factory.rs (1)
68-73: Fix: pass reference to RocksDBConnection::new (current call moves the value).RocksDBConnection::new takes &RocksDBConfig per rocksdb.rs, but here config is passed by value. This won’t compile. Pass a reference.
Apply this diff:
- DatabaseConfig::RocksDB(config) => { - let db = RocksDBConnection::new(config)?; + DatabaseConfig::RocksDB(config) => { + let db = RocksDBConnection::new(&config)?; Ok(Arc::new(Box::new(db))) }
♻️ Duplicate comments (4)
crates/storage/src/sled.rs (3)
175-181: Don’tunwrap()transaction result — propagate sled transaction errors.Using
.unwrap()can panic on both conflict and storage errors; this is library code and should not panic. MapTransactionErrorvariants into your error type and return.Apply:
- self.connection - .transaction(|tx| { + self.connection + .transaction(|tx| { tx.insert(epoch_key.as_bytes(), epoch_data.as_slice())?; tx.insert(KEY_LATEST_EPOCH_HEIGHT, &height_bytes)?; Ok::<(), sled::transaction::ConflictableTransactionError<anyhow::Error>>(()) - }) - .unwrap(); + }) + .map_err(|e| match e { + sled::transaction::TransactionError::Abort(e) => e, + sled::transaction::TransactionError::Storage(e) => e.into(), + })?;
207-213:flush_databaselikely fails to remove the directory; drop clone does nothing here.Dropping a clone of the Arc doesn’t close the DB;
self.connectionis still alive due to&self. This can causeremove_dir_allto fail (DB still open). At minimum, ensure data is persisted and document exclusivity; ideally, make destruction require ownership.Apply a safer minimal change and document behavior:
fn flush_database(&self) -> Result<()> { - // sled doesn't have a destroy method, so we need to drop the connection and remove the - // directory - drop(self.connection.clone()); - std::fs::remove_dir_all(&self.path)?; - Ok(()) + // sled has no "destroy while open". We flush all trees and the DB. + // Callers must ensure all references are dropped before removing the directory. + self.node_tree.flush()?; + self.value_tree.flush()?; + self.connection.flush()?; + // NOTE: Removing the directory while the DB is still open will fail. + // Prefer removing it in the caller after dropping the connection. + Ok(()) }Follow-up option (separate PR): change the trait to support a consuming destroy (e.g.,
fn destroy(self)) or introduce ashutdown()that returns a guard confirming exclusive ownership before removing the directory.Would you like me to open an issue proposing an API for safe database teardown?
256-283: Don’t useunwrap()in transactional batch writes; propagate errors.Multiple
unwrap()calls can panic on encode/insert failures, violating error-handling standards. Map fallible operations toConflictableTransactionError::Abortand propagate the outer transaction error instead of panicking.Apply:
(&self.node_tree, &self.value_tree) .transaction(|(tx_node, tx_values)| { for (node_key, node) in node_batch.nodes() { - let key = node_key.encode_to_bytes().unwrap(); - let value = node.encode_to_bytes().unwrap(); - tx_node.insert(key, value).unwrap(); + let key = node_key.encode_to_bytes() + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; + let value = node.encode_to_bytes() + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; + tx_node.insert(key, value) + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; } for ((version, key_hash), value) in node_batch.values() { let value_key = key_hash.0; - let encoded_value = value - .as_ref() - .map(|v| v.encode_to_bytes()) - .transpose() - .unwrap() - .unwrap_or_default(); + let encoded_value = value + .as_ref() + .map(|v| v.encode_to_bytes()) + .transpose() + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))? + .unwrap_or_default(); let version_bytes = version.to_be_bytes(); let mut fkey = Vec::with_capacity(32 + version_bytes.len()); fkey.extend_from_slice(&value_key); fkey.extend_from_slice(&version_bytes); - tx_values.insert(fkey, encoded_value).unwrap(); + tx_values.insert(fkey, encoded_value) + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; } Ok::<(), sled::transaction::ConflictableTransactionError<anyhow::Error>>(()) }) - .unwrap(); + .map_err(|e| match e { + sled::transaction::TransactionError::Abort(e) => e, + sled::transaction::TransactionError::Storage(e) => e.into(), + })?;crates/cli/src/cfg.rs (1)
286-296: Fix field name: use db_path (rocksdb_path no longer exists).This still references args.database.rocksdb_path, which was renamed to db_path. It breaks compilation.
Apply this diff:
db: match args.database.db_type { #[cfg(feature = "rocksdb")] DBValues::RocksDB => StorageBackend::RocksDB(RocksDBConfig { - path: args.database.rocksdb_path.unwrap_or_else(|| format!("{}/data", prism_home)), + path: args.database.db_path.unwrap_or_else(|| format!("{}/data", prism_home)), }),
🧹 Nitpick comments (17)
crates/tree/Cargo.toml (2)
31-37: Dev-deps and benches wiring look good; minor cleanup suggestions.
- Good: adding Criterion with
harness = falseand forwardingrocksdbfeature toprism-storagealigns with the PR goal.- Cleanup:
jmtappears under both[dependencies]and[dev-dependencies]. If the bench/tests don’t require an extra feature set, the dev-dep duplication can be dropped to avoid redundant resolution work.Apply this minimal cleanup:
[dev-dependencies] paste = { workspace = true } prism-storage = { workspace = true } tempfile.workspace = true -criterion = { version = "0.5", features = ["html_reports"] } -jmt = { workspace = true } +criterion = { version = "0.5", features = ["html_reports"] }
38-40: Optional: gate the bench on the feature if it hard-requires RocksDB paths.If
benches/database_comparison.rsconditionally compiles RocksDB code with#[cfg(feature = "rocksdb")], you’re fine. If the bench assumes RocksDB is present when comparing backends, consider addingrequired-features = ["rocksdb"]to avoid confusing users who runcargo benchwithout the feature.Would you like me to check the bench for unconditional RocksDB usage and open a follow-up if needed?
Also applies to: 44-44
crates/storage/src/sled.rs (3)
24-29: Make builder methods public and document config knobs.
with_cache_capacityandwith_flush_every_msare notpub, which limits their utility outside this module. Also, brief doc comments help discoverability.Apply:
impl SledConfig { pub fn new(path: &str) -> Self { Self { path: path.to_string(), cache_capacity: None, flush_every_ms: None, } } - const fn with_cache_capacity(mut self, capacity: u64) -> Self { + /// Set the cache capacity (bytes) for sled. + pub const fn with_cache_capacity(mut self, capacity: u64) -> Self { self.cache_capacity = Some(capacity); self } - const fn with_flush_every_ms(mut self, ms: u64) -> Self { + /// Enable periodic flushing every `ms` milliseconds. + pub const fn with_flush_every_ms(mut self, ms: u64) -> Self { self.flush_every_ms = Some(ms); self } }Also applies to: 31-49
226-247: Value reads: reverse scan is OK; minor efficiency/readability nits.
- Using a reverse range with a constructed upper bound is correct for “<= max_version”.
- Optional nit:
scan_prefix(value_key)+ manual upper-bound handling can be clearer, but current approach is fine.- let mut iter = self.value_tree.range(..=max_key).rev(); + // Alternatively: `self.value_tree.scan_prefix(&value_key)` and stop at `> max_version` + let mut iter = self.value_tree.range(..=max_key).rev();
249-251: Unimplementedget_rightmost_leafpanics if called.If this can be triggered (e.g., by future snapshot restore paths), consider returning
Ok(None)and documenting the limitation to avoid a hard panic.- fn get_rightmost_leaf(&self) -> Result<Option<(NodeKey, LeafNode)>> { - unimplemented!("JMT Restoration from snapshot is unimplemented."); - } + fn get_rightmost_leaf(&self) -> Result<Option<(NodeKey, LeafNode)>> { + // JMT restoration from snapshot is currently unsupported. + Ok(None) + }crates/tree/src/tests/mod.rs (1)
54-59: Setup for sled-backed tests mirrors RocksDB/InMemory patterns.Good use of
TempDirper test to isolate state. Consider adding an explicit teardown via a database API if/when sled supports a safe destroy; for now, the tempdir auto-cleanup suffices.crates/cli/src/tests/mod.rs (1)
182-187: Feature-gated RocksDB assertions prevent false failures without the feature.Gating these blocks under
#[cfg(feature = "rocksdb")]is the right move. Optional follow-up: add a sled-path assertion in a separate test to validate default DB when RocksDB is disabled.Also applies to: 286-291
crates/cli/src/apply_args/database.rs (2)
15-24:apply_database_argsbehavior is predictable; minor UX consideration.
- Behavior: When
db_typeisNone, config is untouched — good.- When RocksDB is disabled at compile time but chosen via CLI, this will (correctly) be a “DB type mismatch.” Consider improving the error text to hint that RocksDB support isn’t compiled in.
- _ => anyhow::bail!("DB type mismatch"), + _ => anyhow::bail!("DB type mismatch (hint: was the 'rocksdb' feature enabled at build time?)"),
35-38: Tests gated behind feature are appropriate; consider adding a no-feature smoke test.Current tests verify RocksDB arg application when the feature is enabled. Consider a small test (behind
#[cfg(not(feature = "rocksdb"))]) that ensures pickingRocksDBvia CLI yields an error, so users get an early, clear failure without the feature.I can add that negative test if desired.
Also applies to: 43-65, 66-82
crates/storage/src/factory.rs (2)
62-65: Optional API simplification: avoid Arc<Box>; return Arc.Box inside Arc adds a needless indirection and allocation. Arc is sufficient (Arc supports unsize coercion to trait objects). This is a cross-crate change (CLI and tests also use Arc<Box<...>>), so consider doing it in a follow-up to keep this PR focused.
Illustrative diff for this file:
-pub async fn create_storage( - config: &DatabaseConfig, -) -> Result<Arc<Box<dyn Database>>, DatabaseError> { +pub async fn create_storage( + config: &DatabaseConfig, +) -> Result<Arc<dyn Database>, DatabaseError> { info!("Initializing storage layer..."); match config { - DatabaseConfig::InMemory => Ok(Arc::new(Box::new(InMemoryDatabase::new()))), + DatabaseConfig::InMemory => Ok(Arc::new(InMemoryDatabase::new()) as Arc<dyn Database>), #[cfg(feature = "rocksdb")] DatabaseConfig::RocksDB(config) => { let db = RocksDBConnection::new(&config)?; - Ok(Arc::new(Box::new(db))) + Ok(Arc::new(db) as Arc<dyn Database>) } } }If you want, I can prepare a repo-wide patch (factory, CLI initialize_db, tests) to migrate to Arc.
Also applies to: 67-73
65-73: Improve observability: log the chosen backend (and path when applicable).Right now we only log “Initializing storage layer…”. Emitting which backend was selected helps support/bench runs.
Example tweak:
info!("Initializing storage layer..."); match config { - DatabaseConfig::InMemory => Ok(Arc::new(Box::new(InMemoryDatabase::new()))), + DatabaseConfig::InMemory => { + info!("Using InMemory storage backend"); + Ok(Arc::new(Box::new(InMemoryDatabase::new()))) + } #[cfg(feature = "rocksdb")] DatabaseConfig::RocksDB(config) => { - let db = RocksDBConnection::new(config)?; + info!("Using RocksDB storage backend at {}", config.path); + let db = RocksDBConnection::new(&config)?; Ok(Arc::new(Box::new(db))) } }crates/storage/src/tests/mod.rs (2)
13-19: Use expect with context instead of unwrap in test setup.Small ergonomics: expect provides immediate context if a backend fails to initialize, which speeds up diagnosis across the macro-generated tests.
- StorageBackend::RocksDB(cfg) => Box::new(RocksDBConnection::new(&cfg).unwrap()), - StorageBackend::Sled(cfg) => Box::new(SledConnection::new(&cfg).unwrap()), + StorageBackend::RocksDB(cfg) => { + Box::new(RocksDBConnection::new(&cfg).expect("failed to init RocksDB for tests")) + } + StorageBackend::Sled(cfg) => { + Box::new(SledConnection::new(&cfg).expect("failed to init Sled for tests")) + }
100-123: Epoch operations basic invariants covered.Covers NotFound on empty state, add_epoch, latest height, and retrieval. Consider adding a negative case for a non-existent epoch height as well.
If useful, I can add a tiny test asserting get_epoch(999) returns an error.
crates/tree/benches/database_comparison.rs (2)
41-43: Stabilize benches: set sample size and warm-up for less variance.Criterion’s defaults can be noisy with DB I/O. A small warm-up and a slightly larger sample often helps.
fn bench_direct_operations(c: &mut Criterion) { - let mut group = c.benchmark_group("direct_operations"); + let mut group = c.benchmark_group("direct_operations"); + group.sample_size(40).warm_up_time(std::time::Duration::from_secs(2));Apply similarly to other groups if you see high variance locally.
485-575: Payload-size benches: nice parameterization; consider throughput metrics.To make results comparable across sizes, set a throughput hint so Criterion reports MB/s (or elements).
Example inside the loop, before bench_with_input calls:
- for size in [100, 1000, 10000, 100000].iter() { + for size in [100, 1000, 10_000, 100_000].iter() { + group.throughput(criterion::Throughput::Bytes(*size as u64));crates/cli/src/cfg.rs (2)
187-189: Clarify variable/message: path applies to any disk-backed DB, not just RocksDB.We’re creating the directory for args.database.db_path regardless of backend; the error message says “RocksDB directory”.
- if let Some(rocksdb_path) = &args.database.db_path { - fs::create_dir_all(rocksdb_path).context("Failed to create RocksDB directory")?; + if let Some(db_path) = &args.database.db_path { + fs::create_dir_all(db_path).context("Failed to create DB directory")?; }
156-174: CLI help nit: “rocks-db” vs “rocksdb”.DBValues derives ValueEnum; clap will expect “rocksdb”. The docstring says “rocks-db”. Consider aligning to avoid confusion.
- /// Path to the database, used when `db_type` is `rocks-db` or `sled` + /// Path to the database, used when `db_type` is `rocksdb` or `sled`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.toml(3 hunks)crates/cli/Cargo.toml(1 hunks)crates/cli/src/apply_args/database.rs(4 hunks)crates/cli/src/cfg.rs(1 hunks)crates/cli/src/tests/mod.rs(2 hunks)crates/node_types/wasm-lightclient/src/tests/mod.rs(2 hunks)crates/storage/Cargo.toml(2 hunks)crates/storage/src/database.rs(1 hunks)crates/storage/src/factory.rs(6 hunks)crates/storage/src/lib.rs(1 hunks)crates/storage/src/rocksdb.rs(0 hunks)crates/storage/src/sled.rs(1 hunks)crates/storage/src/tests/mod.rs(4 hunks)crates/tests/src/lib.rs(2 hunks)crates/tree/Cargo.toml(1 hunks)crates/tree/benches/database_comparison.rs(1 hunks)crates/tree/src/tests/mod.rs(4 hunks)
💤 Files with no reviewable changes (1)
- crates/storage/src/rocksdb.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/cli/Cargo.toml
- crates/node_types/wasm-lightclient/src/tests/mod.rs
- crates/storage/Cargo.toml
- Cargo.toml
- crates/storage/src/database.rs
- crates/storage/src/lib.rs
- crates/tests/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use rustfmt with project settings (merge_imports=true, imports_granularity="Crate", max_width=100)
Follow Rust Coding Standards
Naming: follow Rust conventions (snake_case for functions/variables, CamelCase for types)
Documentation: Add comments for public APIs and complex logic
File organization: Group related functionality in modules
Error handling: Use Result types with descriptive error messages
Include tests for new functionality
Avoid allocations in hot paths: Use references and borrowing
Use tokio for I/O-bound operations (Async/Await)
Handle errors properly: Use?operator and proper error types
Group related functionality in modules
Add comments for public APIs and complex logic
Files:
crates/cli/src/tests/mod.rscrates/storage/src/sled.rscrates/storage/src/factory.rscrates/tree/src/tests/mod.rscrates/cli/src/apply_args/database.rscrates/storage/src/tests/mod.rscrates/tree/benches/database_comparison.rscrates/cli/src/cfg.rs
🧠 Learnings (4)
📚 Learning: 2025-08-18T05:30:36.919Z
Learnt from: jns-ps
PR: deltadevsde/prism#406
File: crates/storage/src/factory.rs:47-58
Timestamp: 2025-08-18T05:30:36.919Z
Learning: The codebase uses Arc<Box<dyn Database>> pattern throughout for database trait objects. While Arc<dyn Database> would be more efficient, changing this requires updating all call sites and should be done in a separate focused PR rather than as part of larger refactoring efforts.
Applied to files:
crates/storage/src/sled.rscrates/tree/src/tests/mod.rs
📚 Learning: 2025-07-30T06:42:01.656Z
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-30T06:42:01.656Z
Learning: Applies to **/*.rs : Include tests for new functionality
Applied to files:
crates/tree/src/tests/mod.rscrates/cli/src/apply_args/database.rs
📚 Learning: 2024-10-29T08:34:32.445Z
Learnt from: jns-ps
PR: deltadevsde/prism#147
File: crates/common/src/test_ops.rs:27-44
Timestamp: 2024-10-29T08:34:32.445Z
Learning: In `crates/common/src/test_ops.rs`, methods in test code are deliberately kept short for brevity, even if this results in abbreviated method names.
Applied to files:
crates/tree/src/tests/mod.rscrates/cli/src/apply_args/database.rs
📚 Learning: 2025-07-21T09:28:36.437Z
Learnt from: sebasti810
PR: deltadevsde/prism#389
File: crates/tests/src/lib.rs:208-208
Timestamp: 2025-07-21T09:28:36.437Z
Learning: In test files like `crates/tests/src/lib.rs`, using `.unwrap()` for error handling is acceptable as tests should fail fast when unexpected conditions occur, making panics informative for debugging purposes.
Applied to files:
crates/cli/src/apply_args/database.rs
🧬 Code graph analysis (5)
crates/storage/src/sled.rs (2)
crates/storage/src/rocksdb.rs (15)
new(61-65)new(75-88)get_commitment(127-137)set_commitment(139-144)get_last_synced_height(146-155)set_last_synced_height(157-159)get_epoch(161-174)add_epoch(176-214)get_latest_epoch_height(216-225)get_latest_epoch(227-230)flush_database(232-234)get_node_option(238-247)get_value_option(249-273)get_rightmost_leaf(277-279)write_node_batch(283-309)crates/storage/src/database.rs (9)
get_commitment(18-18)set_commitment(19-19)get_last_synced_height(27-27)set_last_synced_height(28-28)get_epoch(21-21)add_epoch(22-22)get_latest_epoch_height(24-24)get_latest_epoch(25-25)flush_database(30-30)
crates/tree/src/tests/mod.rs (4)
crates/storage/src/tests/mod.rs (1)
setup_db(13-20)crates/tests/src/lib.rs (1)
setup_db(32-37)crates/storage/src/sled.rs (2)
new(32-38)new(60-81)crates/storage/src/rocksdb.rs (2)
new(61-65)new(75-88)
crates/storage/src/tests/mod.rs (3)
crates/storage/src/sled.rs (4)
new(32-38)new(60-81)get_value_option(226-247)get_latest_epoch_height(186-200)crates/storage/src/rocksdb.rs (4)
new(61-65)new(75-88)get_value_option(249-273)get_latest_epoch_height(216-225)crates/storage/src/database.rs (1)
get_latest_epoch_height(24-24)
crates/tree/benches/database_comparison.rs (2)
crates/storage/src/sled.rs (2)
new(32-38)new(60-81)crates/storage/src/rocksdb.rs (3)
new(61-65)new(75-88)with(107-114)
crates/cli/src/cfg.rs (6)
crates/da/src/celestia/light_client.rs (2)
default(203-223)new(342-366)crates/da/src/celestia/full_node.rs (2)
default(119-128)new(160-190)crates/da/src/memory.rs (2)
default(42-44)new(48-69)crates/storage/src/inmemory.rs (2)
default(42-44)new(30-38)crates/storage/src/rocksdb.rs (3)
new(61-65)new(75-88)connection(140-140)crates/keys/src/verifying_keys.rs (1)
from_base64(327-343)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: clippy
- GitHub Check: integration-test
- GitHub Check: unused dependencies
- GitHub Check: coverage
- GitHub Check: build-and-push-image
- GitHub Check: unit-test
🔇 Additional comments (20)
crates/storage/src/sled.rs (7)
60-81: Constructor is solid and feature-aligned.Opening dedicated trees for nodes and value history keeps sled keyspaces simple and avoids prefix-collision edge cases. Good use of
sled::Configoptions fromSledConfig.
85-106: Commitment getters/setters: clear errors and strict length check — LGTM.The explicit 32-byte enforcement and descriptive errors are helpful. No changes needed.
107-121: Sync height I/O: good conversion hygiene.Strict BE length checks and clear messages are on point. Setter uses
insertappropriately. No changes needed.Also applies to: 123-126
128-141: Epoch retrieval: proper not-found and decode errors.The error mapping is consistent with the RocksDB impl. LGTM.
186-200: Latest epoch getters: consistent with other storage backends.Clear error messages and decode path — looks good.
Also applies to: 202-205
216-224: Node reads: correct decoding and Option handling.Straightforward and matches the RocksDB semantics. LGTM.
256-283: Static scan complete: only intendedunwrap()calls exist in sled transactionsThe repository was searched for any
unwrap()calls within.transaction(…)closures. Incrates/storage/src/sled.rs, the only occurrences are those already in the reviewed snippet:
- Line 259:
node_key.encode_to_bytes().unwrap()- Line 260:
node.encode_to_bytes().unwrap()- Line 261:
tx_node.insert(key, value).unwrap()The final
.unwrap()on thetransaction(...)result at line 283 is likewise intentional to bubble errors. No otherunwrap()calls remain in sled transaction closures across the codebase.crates/tree/src/tests/mod.rs (3)
12-13: Imports and feature gates are correct and minimize cfg churn.Unconditional sled imports with conditional RocksDB imports align with making sled the default and RocksDB optional behind a feature.
Also applies to: 20-22
25-29: DBType and trait impls extended for sled — LGTM.Adding
DBType::SledandTreeReadWriterimpls forSledConnectionand its boxed form match the existing pattern for other backends.Also applies to: 36-41
458-461: Added sled test variants increase coverage where RocksDB is absent — nice.This ensures the algorithm tests run on the new default backend.
crates/cli/src/apply_args/database.rs (2)
2-6: Imports and feature gating are consistent with making RocksDB optional.Keeping
DatabaseConfigalways available while gatingRocksDBConfigbehind the feature is appropriate.
27-33: RocksDB arg application is straightforward.Only updates the path when provided; LGTM.
crates/storage/src/factory.rs (2)
89-96: Gating RocksDB tests appropriately.Feature gating around RocksDB tests looks correct and avoids build breaks when the feature is off.
Also applies to: 109-116, 138-148
11-12: Great, running a quick search to verify the presence of asledmodule and its types in the storage crate.crates/storage/src/tests/mod.rs (3)
50-69: Good coverage: verifies value selection across multiple versions.Ensures highest version ≤ max_version is returned. This is exactly the cross-backend contract we need.
71-79: Sync height round-trip test looks good.Simple and backend-agnostic. No changes requested.
147-171: Macro-based multi-backend tests approved;pasteis already declared in dev-dependencies
- The
generate_storage_tests!macro cleanly covers Sled, InMemory, and RocksDB backends without duplication.- The
pastecrate is present under[dev-dependencies]inCargo.toml(paste = "1.0.15"), so thepaste::paste!macro will be available for these tests.crates/tree/benches/database_comparison.rs (1)
24-31: Bench setup helpers look correct and feature-gated as expected.RocksDB setup is gated; Sled is unconditional. TempDir usage ensures clean teardown between runs.
crates/cli/src/cfg.rs (2)
120-132: Default config sets Sled and sensible webserver/network defaults.This matches the PR goal of making Sled the default. Looks good.
Also applies to: 135-146
314-336: Initialize DB: mapping and error contexts look correct.RocksDB behind feature gate, Sled default, InMemory included. Good alignment with the storage crate.
| pub async fn initialize_light_da_layer( | ||
| config: &Config, | ||
| ) -> Result<Arc<dyn LightDataAvailabilityLayer + Send + Sync + 'static>> { | ||
| match config.da_layer { | ||
| DALayerOption::Celestia => { | ||
| info!("Initializing light client connection..."); | ||
| info!("Network config: {:?}", config.network); | ||
| let connection = match LightClientConnection::new(&config.network).await { | ||
| Ok(conn) => conn, | ||
| Err(e) => { | ||
| error!("Failed to initialize light client connection: {:?}", e); | ||
| error!("Network config: {:?}", config.network); | ||
| if let Some(celestia_config) = &config.network.celestia_config { | ||
| error!( | ||
| "Celestia connection string: {}", | ||
| celestia_config.connection_string | ||
| ); | ||
| error!("Start height: {}", celestia_config.start_height); | ||
| } | ||
| return Err(anyhow!( | ||
| "Failed to initialize light client connection: {}", | ||
| e | ||
| )); | ||
| } | ||
| }; | ||
| Ok(Arc::new(connection) | ||
| as Arc< | ||
| dyn LightDataAvailabilityLayer + Send + Sync + 'static, | ||
| >) | ||
| } | ||
| DALayerOption::InMemory => { | ||
| let (da_layer, _height_rx, _block_rx) = | ||
| InMemoryDataAvailabilityLayer::new(Duration::from_secs(10)); | ||
| Ok(Arc::new(da_layer)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type-check + logging safety: validate LightClientConnection::new argument and avoid leaking secrets in logs.
- You pass &config.network to LightClientConnection::new. In DA code this often expects a CelestiaLightClientDAConfig. Please confirm.
- On errors, we log the full connection_string. If this can include tokens/credentials in your deployments, consider redacting.
Script to confirm method signature:
If redaction is needed, we can print only scheme/host:
- error!(
- "Celestia connection string: {}",
- celestia_config.connection_string
- );
+ if let Ok(url) = url::Url::parse(&celestia_config.connection_string) {
+ error!("Celestia endpoint: {}://{}", url.scheme(), url.host_str().unwrap_or("<unknown>"));
+ }(Requires adding url = "2" to CLI dependencies.)
🏁 Script executed:
#!/bin/bash
rg -nP 'pub\s+async\s+fn\s+new\s*\(' crates/da/src/celestia/light_client.rs -n -C2Length of output: 288
🏁 Script executed:
#!/bin/bash
# Show the definition of Config and its network field
rg -n 'struct Config' -n -C5 crates/cli/src/cfg.rs
rg -n 'network' -n -C5 crates/cli/src/cfg.rsLength of output: 6831
🏁 Script executed:
#!/bin/bash
# Inspect the definition of NetworkConfig in the CLI
rg -nP 'pub struct NetworkConfig' -n -C5 crates/cli/src/cfg.rs
# Inspect fields related to Celestia within NetworkConfig
rg -n 'celestia' -n -C5 crates/cli/src/cfg.rs
# Inspect the CelestiaConfig type
rg -nP 'pub struct CelestiaConfig' -n -C5 crates/cli/src/cfg.rsLength of output: 6720
🏁 Script executed:
#!/bin/bash
# Locate and inspect CelestiaLightClientDAConfig in the DA crate
rg -nP 'pub\s+(struct|enum|type)\s+CelestiaLightClientDAConfig' -n -C5 crates/da/src/celestia/utils.rs || true
# Fallback: print the first 200 lines of utils.rs to find where CelestiaLightClientDAConfig or NetworkConfig is defined
sed -n '1,200p' crates/da/src/celestia/utils.rsLength of output: 830
🏁 Script executed:
#!/bin/bash
# Dump the full utils.rs to uncover config type definitions
sed -n '1,500p' crates/da/src/celestia/utils.rs
# Search the DA crate for CelestiaLightClientDAConfig references
rg -n 'CelestiaLightClientDAConfig' -n -C5 crates/da/srcLength of output: 9400
🏁 Script executed:
#!/bin/bash
# List all files in the Celestia DA subdirectory
ls -R crates/da/src/celestia
# Search for the definition of NetworkConfig in the DA crate
rg -nP 'pub\s+struct\s+NetworkConfig' -n -C5 crates/da/srcLength of output: 160
🏁 Script executed:
#!/bin/bash
# List all source files in the CLI crate to locate NetworkConfig and CelestiaConfig definitions
ls -R crates/cli/srcLength of output: 327
🏁 Script executed:
#!/bin/bash
# Print the first 200 lines of the CLI config file to locate NetworkConfig and CelestiaConfig definitions
sed -n '1,200p' crates/cli/src/config.rsLength of output: 2034
Fix LightClientConnection instantiation and improve logging safety
LightClientConnection::new expects a &CelestiaLightClientDAConfig, not a &NetworkConfig. Update the code to pass the DA‐layer config and avoid logging raw connection strings:
– In crates/cli/src/cfg.rs around line 387, replace:
let connection = match LightClientConnection::new(&config.network).await {
…
};with:
// Extract the Celestia client DA config
let da_config = config
.network
.celestia_config
.clone()
.expect("Celestia config must be set when DALayerOption::Celestia is selected");
let connection = match LightClientConnection::new(&da_config).await {
…
};– In the error handler around lines 393–396, redact any secrets in the connection string rather than logging it in full. For example:
- error!("Celestia connection string: {}", celestia_config.connection_string);
+ // Only log scheme and host to avoid leaking credentials
+ if let Ok(url) = url::Url::parse(&celestia_config.connection_string) {
+ error!(
+ "Celestia endpoint: {}://{}",
+ url.scheme(),
+ url.host_str().unwrap_or("<unknown>")
+ );
+ }(Requires adding url = "2" to the CLI’s Cargo.toml.)
🤖 Prompt for AI Agents
crates/cli/src/cfg.rs around lines 380 to 416: LightClientConnection::new is
being called with the whole NetworkConfig; extract and pass the Celestia DA
config instead, e.g., clone and unwrap/expect the config from
config.network.celestia_config and pass that reference into
LightClientConnection::new, and remove or replace any direct logging of the raw
connection string with a redacted version (parse the URL and redact credentials
or only log host/port), adding the "url = \"2\"" crate to Cargo.toml if you need
URL parsing to perform safe redaction.
| if let Some(latest) = latest_height { | ||
| if latest as usize + 1 != epoch.height as usize { | ||
| return Err(anyhow!(DatabaseError::WriteError(format!( | ||
| "epoch height mismatch: expected {}, got {}", | ||
| latest + 1, | ||
| epoch.height | ||
| )))); | ||
| } | ||
| } else if epoch.height != 0 { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid usize casts for epoch height comparison; use u64-safe arithmetic.
Casting u64 to usize can truncate on 32-bit targets and risks logic bugs at high heights. Use u64::checked_add to avoid overflow and compare as u64.
Apply:
- if let Some(latest) = latest_height {
- if latest as usize + 1 != epoch.height as usize {
+ if let Some(latest) = latest_height {
+ let expected = latest
+ .checked_add(1)
+ .ok_or_else(|| anyhow!(DatabaseError::WriteError("epoch height overflow".into())))?;
+ if expected != epoch.height {
return Err(anyhow!(DatabaseError::WriteError(format!(
"epoch height mismatch: expected {}, got {}",
- latest + 1,
+ expected,
epoch.height
))));
}🤖 Prompt for AI Agents
In crates/storage/src/sled.rs around lines 147 to 155, avoid casting u64 heights
to usize; instead use u64-safe arithmetic: call latest.checked_add(1) and handle
the None (overflow) case by returning a WriteError, then compare the resulting
u64 to epoch.height directly (no usize conversion). If they differ, return the
existing epoch height mismatch error using the checked_add result in the
message.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
crates/cli/src/cfg.rs (2)
287-295: Fix: wrong field name for RocksDB path breaks build
DatabaseArgsexposesdb_path, notrocksdb_path. This is a compile-time error behind therocksdbfeature.#[cfg(feature = "rocksdb")] DBValues::RocksDB => StorageBackend::RocksDB(RocksDBConfig { - path: args.database.rocksdb_path.unwrap_or_else(|| format!("{}/data", prism_home)), + path: args.database.db_path.unwrap_or_else(|| format!("{}/data", prism_home)), }),
385-405: Fix: pass the correct config type to LightClientConnection and avoid logging secrets
LightClientConnection::newexpects&CelestiaLightClientDAConfig, not&NetworkConfig. Also, avoid printing raw connection strings.DALayerOption::Celestia => { info!("Initializing light client connection..."); - info!("Network config: {:?}", config.network); - let connection = match LightClientConnection::new(&config.network).await { + info!("Network selected: {:?}", config.network.network); + let da_cfg = match config.network.celestia_config.clone() { + Some(c) => c, + None => { + return Err(anyhow!("Celestia configuration not found")); + } + }; + let connection = match LightClientConnection::new(&da_cfg).await { Ok(conn) => conn, Err(e) => { - error!("Failed to initialize light client connection: {:?}", e); - error!("Network config: {:?}", config.network); - if let Some(celestia_config) = &config.network.celestia_config { - error!( - "Celestia connection string: {}", - celestia_config.connection_string - ); - error!("Start height: {}", celestia_config.start_height); - } + error!("Failed to initialize light client connection: {:?}", e); + // Avoid logging full connection strings; redact endpoints if needed. return Err(anyhow!( "Failed to initialize light client connection: {}", e )); } };crates/storage/src/sled.rs (5)
207-213: Improve: closing/removing database directory can fail with live handlesDropping a clone of the Arc may not free all handles. Flush and document exclusive access requirements.
fn flush_database(&self) -> Result<()> { - // sled doesn't have a destroy method, so we need to drop the connection and remove the - // directory - drop(self.connection.clone()); + // Ensure data is persisted and advise callers to ensure exclusive access before removal. + self.connection.flush()?; std::fs::remove_dir_all(&self.path)?; Ok(()) }Consider documenting that no other references (Db, Tree clones) should exist when calling this.
147-155: Fix: height arithmetic uses usize casts (risk on 32-bit, readability)Use u64 math with checked_add; avoid casts.
- if let Some(latest) = latest_height { - if latest as usize + 1 != epoch.height as usize { - return Err(anyhow!(DatabaseError::WriteError(format!( - "epoch height mismatch: expected {}, got {}", - latest + 1, - epoch.height - )))); - } - } else if epoch.height != 0 { + if let Some(latest) = latest_height { + let expected = latest + .checked_add(1) + .ok_or_else(|| anyhow!(DatabaseError::WriteError("epoch height overflow".into())))?; + if expected != epoch.height { + return Err(anyhow!(DatabaseError::WriteError(format!( + "epoch height mismatch: expected {}, got {}", + expected, + epoch.height + )))); + } + } else if epoch.height != 0 {
175-181: Fix: transaction result uses unwrap (panic on error)Propagate
sled::transaction::TransactionErrorinstead of panicking.self.connection .transaction(|tx| { tx.insert(epoch_key.as_bytes(), epoch_data.as_slice())?; tx.insert(KEY_LATEST_EPOCH_HEIGHT, &height_bytes)?; Ok::<(), sled::transaction::ConflictableTransactionError<anyhow::Error>>(()) }) - .unwrap(); + .map_err(|e| match e { + sled::transaction::TransactionError::Abort(e) => e, + sled::transaction::TransactionError::Storage(e) => e.into(), + })?;
259-277: Fix: unwraps inside transactional writer can panic; propagate errorsReplace
unwrap()with proper abortable error propagation to keep atomicity and avoid panics.for (node_key, node) in node_batch.nodes() { - let key = node_key.encode_to_bytes().unwrap(); - let value = node.encode_to_bytes().unwrap(); - tx_node.insert(key, value).unwrap(); + let key = node_key.encode_to_bytes() + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; + let value = node.encode_to_bytes() + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; + tx_node.insert(key, value.as_slice()) + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; } @@ for ((version, key_hash), value) in node_batch.values() { let value_key = key_hash.0; - let encoded_value = value - .as_ref() - .map(|v| v.encode_to_bytes()) - .transpose() - .unwrap() - .unwrap_or_default(); + let encoded_value = value + .as_ref() + .map(|v| v.encode_to_bytes()) + .transpose() + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))? + .unwrap_or_default(); let version_bytes = version.to_be_bytes(); @@ - tx_values.insert(fkey, encoded_value).unwrap(); + tx_values.insert(fkey, encoded_value.as_slice()) + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; } @@ - }) - .unwrap(); + }) + .map_err(|e| match e { + sled::transaction::TransactionError::Abort(e) => e, + sled::transaction::TransactionError::Storage(e) => e.into(), + })?;Also applies to: 280-282
254-286: Replace all panicking.unwrap()calls incrates/storage/src/sled.rs
- Line 181, 282:
.transaction(...).unwrap()- Lines 259–261:
node_key.encode_to_bytes().unwrap(),node.encode_to_bytes().unwrap(),tx_node.insert(...).unwrap()- Lines 270–271:
.transpose().unwrap().unwrap_or_default()- Line 277:
tx_values.insert(...).unwrap()Use
?or convert errors withmap_errinside the transaction closure to propagate failures instead of panicking.
🧹 Nitpick comments (14)
crates/cli/src/tests/mod.rs (2)
182-187: Guarded RocksDB assertion is fine; add a non-RocksDB fallback check.When the feature is disabled, this test silently skips DB assertions. Consider asserting the expected default (Sled) config under cfg(not(feature = "rocksdb")) to keep coverage consistent.
Apply this minimal addition right below the guarded block:
+ #[cfg(not(feature = "rocksdb"))] + match &config.db { + prism_storage::DatabaseConfig::InMemory | prism_storage::DatabaseConfig::Sled(_) => {} + _ => panic!("Expected non-RocksDB config when 'rocksdb' feature is disabled"), + }
286-291: Same note: add non-RocksDB fallback assertion for prover test.Mirror the fallback match used in the full-node test so the test asserts expected state when RocksDB is not compiled.
+ #[cfg(not(feature = "rocksdb"))] + match &config.db { + prism_storage::DatabaseConfig::InMemory | prism_storage::DatabaseConfig::Sled(_) => {} + _ => panic!("Expected non-RocksDB config when 'rocksdb' feature is disabled"), + }crates/tree/Cargo.toml (1)
32-37: Duplication of jmt in dependencies and dev-dependencies.jmt is already a normal dependency (Line 20). The extra dev-dependency entry is redundant.
-[dev-dependencies] -paste = { workspace = true } -prism-storage = { workspace = true } -tempfile.workspace = true -criterion = { version = "0.5", features = ["html_reports"] } -jmt = { workspace = true } +[dev-dependencies] +paste = { workspace = true } +prism-storage = { workspace = true } +tempfile.workspace = true +criterion = { version = "0.5", features = ["html_reports"] }crates/tree/benches/database_comparison.rs (5)
11-11: Remove unused import.SnarkableTree isn’t used in this bench file.
-use prism_tree::snarkable_tree::SnarkableTree;
41-67: Isolate side effects per iteration for write benches.Reusing a single DB across iterations can cause caching/compaction artifacts. Use iter_with_setup to create a fresh DB per iteration for more stable measurements.
- group.bench_function("sled_commitment_write", |b| { - let (_temp_dir, db) = setup_sled(); - b.iter(|| { - for i in 0..100 { - let epoch = black_box(i); - let commitment = prism_common::digest::Digest([i as u8; 32]); - db.set_commitment(&epoch, &commitment).unwrap(); - } - }); - }); + group.bench_function("sled_commitment_write", |b| { + b.iter_with_setup(setup_sled, |(_td, db)| { + for i in 0..100 { + let epoch = black_box(i); + let commitment = prism_common::digest::Digest([i as u8; 32]); + db.set_commitment(&epoch, &commitment).unwrap(); + } + }); + });Replicate the same pattern for rocksdb_commitment_write.
108-119: Same for JMT batch write on Sled.Create a fresh DB per iteration to avoid history growth affecting latency.
- group.bench_function("sled_node_batch_write", |b| { - let (_temp_dir, db) = setup_sled(); - b.iter(|| { + group.bench_function("sled_node_batch_write", |b| { + b.iter_with_setup(setup_sled, |(_td, db)| { let mut batch = jmt::storage::NodeBatch::default(); for i in 0..50 { let key_hash = KeyHash([i as u8; 32]); let value: OwnedValue = vec![i as u8; 100]; batch.insert_value(black_box(i as Version), key_hash, value); } db.write_node_batch(&batch).unwrap(); - }); + }); });Apply similarly to rocksdb_node_batch_write.
414-483: Optional: precompute key hashes outside the timed loop.Avoid hashing strings during measurement to focus on DB read performance.
- b.iter(|| { - for i in 0..100 { - let account_name = format!("acc_{}", i); - let key_hash = KeyHash::with::<TreeHasher>(&account_name); - let _ = tree.get(black_box(key_hash)).unwrap(); - } - }); + let hashes: Vec<KeyHash> = (0..100) + .map(|i| KeyHash::with::<TreeHasher>(&format!("acc_{}", i))) + .collect(); + b.iter(|| { + for h in &hashes { + let _ = tree.get(black_box(*h)).unwrap(); + } + });
486-575: Add throughput to payload size benches for better charts.Report bytes processed to normalize rates across sizes.
- let mut group = c.benchmark_group("payload_sizes"); + let mut group = c.benchmark_group("payload_sizes"); @@ - for size in [100, 1000, 10000, 100000].iter() { + for size in [100usize, 1000, 10_000, 100_000].iter().copied() { + group.throughput(criterion::Throughput::Bytes(size as u64)); @@ - group.bench_with_input( + group.bench_with_input( BenchmarkId::new("sled_large_data", size), - size, - |b, &size| { + &size, + |b, &size| { b.iter_with_setup(crates/cli/src/cfg.rs (5)
23-23: Reduce sensitive/verbose logging; move to debug and avoid dumping full configDumping entire config sources can leak secrets (e.g., endpoints/tokens) and is noisy.
-use tracing::{error, info}; +use tracing::{debug, error, info}; @@ - info!("Config file contents: {:?}", config_source); + debug!("Config loaded from: {}/config.toml", home_path); @@ - info!("Final config: {:?}", final_config); + debug!("Final config constructed.");Also applies to: 196-197, 203-204
221-232: Prefer PathBuf joins over manual string concatenation for pathsImproves portability (Windows, stray slashes) and readability.
-fn ensure_config_file_exists(home_path: &str, network_name: &str) -> Result<()> { - let config_path = &format!("{}/config.toml", home_path); - if !Path::new(config_path).exists() { - if let Some(parent) = Path::new(config_path).parent() { - fs::create_dir_all(parent).context("Failed to create config directory")?; - } - - let default_config = Config::initialize(home_path, network_name); - let config_toml = - toml::to_string(&default_config).context("Failed to serialize default config")?; - - fs::write(config_path, config_toml).context("Failed to write default config to disk")?; - } +fn ensure_config_file_exists(home_path: &str, network_name: &str) -> Result<()> { + let config_path = Path::new(home_path).join("config.toml"); + if !config_path.exists() { + if let Some(parent) = config_path.parent() { + fs::create_dir_all(parent).context("Failed to create config directory")?; + } + let default_config = Config::initialize(home_path, network_name); + let config_toml = + toml::to_string(&default_config).context("Failed to serialize default config")?; + fs::write(&config_path, config_toml) + .context("Failed to write default config to disk")?; + } Ok(()) }
187-189: Nit: variable name and message mismatchThis creates the directory for any chosen backend path, not just RocksDB. Rename for clarity.
- if let Some(rocksdb_path) = &args.database.db_path { - fs::create_dir_all(rocksdb_path).context("Failed to create RocksDB directory")?; + if let Some(db_path) = &args.database.db_path { + fs::create_dir_all(db_path).context("Failed to create DB directory")?; }
236-246: Avoid unwraps on user-provided network; fall back to existing configUsing
unwrapcan panic on invalid/empty--network-name. Gracefully fall back to the current network.fn apply_command_line_args(config: Config, args: CommandArgs) -> Config { - let webserver_config = &config.webserver.unwrap_or_default(); - let network_config = &config.network.network.config(); + let webserver_config = &config.webserver.unwrap_or_default(); + // Resolve selected network or fall back to the existing one + let selected_network = args + .network_name + .as_deref() + .and_then(|n| Network::from_str(n).ok()) + .unwrap_or_else(|| config.network.network.clone()); + let selected_network_config = selected_network.config(); @@ network: NetworkConfig { - network: Network::from_str(&args.network_name.unwrap_or_default()).unwrap(), - celestia_network: network_config.celestia_network.clone(), + network: selected_network, + celestia_network: selected_network_config.celestia_network.clone(), verifying_key: args .verifying_key .and_then(|x| VerifyingKey::from_base64(x).ok()) - .unwrap_or(network_config.verifying_key.clone()), + .unwrap_or(selected_network_config.verifying_key.clone()), celestia_config, },Also applies to: 297-305
239-239: Minor: propagate error instead of unwrap onget_prism_home
Replace- let prism_home = get_prism_home(&args.clone()).unwrap(); + let prism_home = get_prism_home(&args.clone())?;to surface any home‐dir resolution failures rather than panicking.
crates/storage/src/sled.rs (1)
85-105: Optional: use binary keys instead of formatted strings for epochs/commitmentsString-concatenated keys allocate and can harm ordering/scan performance. Consider fixed-prefix binary keys (prefix + big-endian u64), mirroring RocksDB’s
Key::...with(...)approach.If you want, I can draft a small helper:
- make_commitment_key(epoch: u64) -> Vec
- make_epoch_key(height: u64) -> Vec
and replace string formatting in get/set paths.
Also applies to: 128-141
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.toml(3 hunks)crates/cli/Cargo.toml(1 hunks)crates/cli/src/apply_args/database.rs(4 hunks)crates/cli/src/cfg.rs(1 hunks)crates/cli/src/tests/mod.rs(2 hunks)crates/node_types/wasm-lightclient/src/tests/mod.rs(2 hunks)crates/storage/Cargo.toml(2 hunks)crates/storage/src/database.rs(1 hunks)crates/storage/src/factory.rs(6 hunks)crates/storage/src/lib.rs(1 hunks)crates/storage/src/rocksdb.rs(0 hunks)crates/storage/src/sled.rs(1 hunks)crates/storage/src/tests/mod.rs(4 hunks)crates/tests/src/lib.rs(2 hunks)crates/tree/Cargo.toml(1 hunks)crates/tree/benches/database_comparison.rs(1 hunks)crates/tree/src/tests/mod.rs(4 hunks)
💤 Files with no reviewable changes (1)
- crates/storage/src/rocksdb.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/node_types/wasm-lightclient/src/tests/mod.rs
- crates/cli/src/apply_args/database.rs
- crates/tree/src/tests/mod.rs
- crates/storage/src/database.rs
- crates/storage/src/tests/mod.rs
- Cargo.toml
- crates/tests/src/lib.rs
- crates/storage/Cargo.toml
- crates/storage/src/factory.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Follow Rust Coding Standards
Use rustfmt for formatting across the codebase
Files:
crates/cli/src/tests/mod.rscrates/tree/benches/database_comparison.rscrates/cli/src/cfg.rscrates/storage/src/lib.rscrates/storage/src/sled.rs
crates/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
crates/**/*.rs: Naming: snake_case for functions/variables, CamelCase for types
Error handling: Use Result types with descriptive error messages
Handle errors with the ? operator and proper error types
Documentation: Add comments for public APIs and complex logic
File organization: Group related functionality in modules
Avoid allocations in hot paths by using references/borrowing
Use tokio for I/O-bound async/await operations
Files:
crates/cli/src/tests/mod.rscrates/tree/benches/database_comparison.rscrates/cli/src/cfg.rscrates/storage/src/lib.rscrates/storage/src/sled.rs
🧠 Learnings (2)
📚 Learning: 2025-08-25T12:23:29.763Z
Learnt from: CR
PR: deltadevsde/prism#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T12:23:29.763Z
Learning: Applies to tests/**/*.rs : Do not write tests for derived traits (Clone, Debug, PartialEq, etc.) unless there is a custom implementation
Applied to files:
crates/cli/src/tests/mod.rs
📚 Learning: 2025-08-18T05:30:36.919Z
Learnt from: jns-ps
PR: deltadevsde/prism#406
File: crates/storage/src/factory.rs:47-58
Timestamp: 2025-08-18T05:30:36.919Z
Learning: The codebase uses Arc<Box<dyn Database>> pattern throughout for database trait objects. While Arc<dyn Database> would be more efficient, changing this requires updating all call sites and should be done in a separate focused PR rather than as part of larger refactoring efforts.
Applied to files:
crates/storage/src/sled.rs
🧬 Code graph analysis (3)
crates/tree/benches/database_comparison.rs (2)
crates/storage/src/sled.rs (2)
new(32-38)new(60-81)crates/storage/src/rocksdb.rs (3)
new(61-65)new(75-88)with(107-114)
crates/cli/src/cfg.rs (5)
crates/da/src/celestia/light_client.rs (2)
default(214-235)new(354-398)crates/da/src/celestia/full_node.rs (2)
default(119-128)new(160-190)crates/da/src/memory.rs (2)
default(42-44)new(48-69)crates/storage/src/rocksdb.rs (3)
new(61-65)new(75-88)connection(140-140)crates/keys/src/verifying_keys.rs (1)
from_base64(327-343)
crates/storage/src/sled.rs (2)
crates/storage/src/rocksdb.rs (14)
new(61-65)new(75-88)get_commitment(127-137)set_commitment(139-144)get_last_synced_height(146-155)set_last_synced_height(157-159)get_epoch(161-174)add_epoch(176-214)get_latest_epoch_height(216-225)get_latest_epoch(227-230)flush_database(232-234)get_node_option(238-247)get_value_option(249-273)write_node_batch(283-309)crates/storage/src/database.rs (9)
get_commitment(18-18)set_commitment(19-19)get_last_synced_height(27-27)set_last_synced_height(28-28)get_epoch(21-21)add_epoch(22-22)get_latest_epoch_height(24-24)get_latest_epoch(25-25)flush_database(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: clippy
- GitHub Check: unused dependencies
- GitHub Check: wasm-test
- GitHub Check: coverage
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: build-and-push-image
🔇 Additional comments (8)
crates/cli/Cargo.toml (1)
15-15: Feature gate wiring for RocksDB looks correct.The feature name matches the storage crate's feature and keeps the default lean. No issues.
crates/cli/src/tests/mod.rs (1)
156-166: No changes needed forrocksdb_path
TheCliDatabaseArgsstruct still definespub rocksdb_path: Option<String>, so the test’s usage is correct.crates/storage/src/lib.rs (1)
90-93: Add Sled docs and gate RocksDB snippets; verify doctests
Incrates/storage/src/lib.rsdoc comments, add a “Sled (default)” entry under Supported Backends with a Quick Start snippet, and gate the RocksDB example behind#[cfg(feature = "rocksdb")](or mark it ignored) to prevent doctest failures on the default build.//! ## Supported Backends //! -//! ### RocksDB +//! ### Sled (default) +//! - Embedded, lightweight, great for development and CI +//! - Persistent with minimal configuration +//! +//! ### RocksDB //! - Production-ready embedded database with LSM-tree architecture @@ -//! ### Production Setup with RocksDB +//! ### Production Setup with RocksDB //! ```rust,no_run -//! use prism_storage::{DatabaseConfig, create_storage, rocksdb::RocksDBConfig}; +//! # #[cfg(feature = "rocksdb")] +//! use prism_storage::{DatabaseConfig, create_storage, rocksdb::RocksDBConfig}; @@ //! ``` +//! +//! ### Quick Start with Sled +//! ```rust,no_run +//! use prism_storage::{DatabaseConfig, create_storage, sled::SledConfig}; +//! #[tokio::main] +//! async fn main() -> anyhow::Result<()> { +//! let config = DatabaseConfig::Sled(SledConfig::new("/tmp/prism-sled")); +//! let _db = create_storage(&config).await?; +//! Ok(()) +//! } +//! ```crates/tree/Cargo.toml (2)
38-41: Bench target looks good.criterion harness is correctly disabled; aligns with the new benches file.
44-44: Feature passthrough matches storage crate.rocksdb feature is correctly forwarded; consistent with workspace gating.
crates/tree/benches/database_comparison.rs (1)
136-175: Read benches look good.Dataset prepared outside the measured loop; iterations only perform reads. LGTM.
crates/cli/src/cfg.rs (1)
314-336: DB initialization paths look solidFeature-gated RocksDB and default Sled wiring with consistent error mapping and Arc<Box> align with repo conventions.
crates/storage/src/sled.rs (1)
60-81: Sled initialization looks goodConfigurable cache/flush and separate trees for nodes/values are clean and straightforward.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
crates/storage/src/tests/mod.rs (1)
156-181: Macro-based parametrization across backends looks solid.The generated tests cover Sled, InMemory, and RocksDB (when enabled) without duplication. The earlier reference vs. owned cfg issue is resolved here.
crates/storage/src/sled.rs (4)
148-156: Avoid usize casts for epoch height; use u64-safe arithmetic.Casting
u64tousizerisks truncation on 32-bit targets and is unnecessary. Compare asu64and guard overflow.Apply:
- if let Some(latest) = latest_height { - if latest as usize + 1 != epoch.height as usize { - return Err(anyhow!(DatabaseError::WriteError(format!( - "epoch height mismatch: expected {}, got {}", - latest + 1, - epoch.height - )))); - } - } else if epoch.height != 0 { + if let Some(latest) = latest_height { + let expected = latest + .checked_add(1) + .ok_or_else(|| anyhow!(DatabaseError::WriteError("epoch height overflow".into())))?; + if expected != epoch.height { + return Err(anyhow!(DatabaseError::WriteError(format!( + "epoch height mismatch: expected {}, got {}", + expected, + epoch.height + )))); + } + } else if epoch.height != 0 {
176-183: Do not.unwrap()transactions; propagate errors.A panic here would bring down the process. Map sled transaction errors and bubble them up.
self.connection .transaction(|tx| { tx.insert(epoch_key.as_bytes(), epoch_data.as_slice())?; tx.insert(KEY_LATEST_EPOCH_HEIGHT, &height_bytes)?; Ok::<(), sled::transaction::ConflictableTransactionError<anyhow::Error>>(()) }) - .unwrap(); + .map_err(|e| match e { + sled::transaction::TransactionError::Abort(e) => e, + sled::transaction::TransactionError::Storage(e) => e.into(), + })?;
256-284: Replaceunwrap()s inside the batch transaction with proper error propagation.Multiple
unwrap()s can panic on serialization or storage errors. Use?withConflictableTransactionError::Abort.(&self.node_tree, &self.value_tree) .transaction(|(tx_node, tx_values)| { for (node_key, node) in node_batch.nodes() { - let key = node_key.encode_to_bytes().unwrap(); - let value = node.encode_to_bytes().unwrap(); - tx_node.insert(key, value).unwrap(); + let key = node_key + .encode_to_bytes() + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; + let value = node + .encode_to_bytes() + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; + tx_node + .insert(key, value.as_slice()) + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; } for ((version, key_hash), value) in node_batch.values() { let value_key = key_hash.0; - let encoded_value = value + let encoded_value = value .as_ref() .map(|v| v.encode_to_bytes()) .transpose() - .unwrap() + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))? .unwrap_or_default(); let version_bytes = version.to_be_bytes(); let mut fkey = Vec::with_capacity(32 + version_bytes.len()); fkey.extend_from_slice(&value_key); fkey.extend_from_slice(&version_bytes); - tx_values.insert(fkey, encoded_value).unwrap(); + tx_values + .insert(fkey, encoded_value.as_slice()) + .map_err(|e| sled::transaction::ConflictableTransactionError::Abort(e.into()))?; } Ok::<(), sled::transaction::ConflictableTransactionError<anyhow::Error>>(()) }) - .unwrap(); + .map_err(|e| match e { + sled::transaction::TransactionError::Abort(e) => e, + sled::transaction::TransactionError::Storage(e) => e.into(), + })?;
208-214:flush_databaselikely fails to remove directory; drop of a cloned Arc doesn’t close DB.Other references (trees, clones) keep file handles open. Removal will intermittently fail.
- fn flush_database(&self) -> Result<()> { - // sled doesn't have a destroy method, so we need to drop the connection and remove the - // directory - drop(self.connection.clone()); - std::fs::remove_dir_all(&self.path)?; - Ok(()) - } + fn flush_database(&self) -> Result<()> { + // Best-effort flush. Full destruction requires exclusive ownership. + self.connection.flush()?; + // If you need to remove the directory, implement a `destroy(self)` that consumes `self` + // (ensuring all refs are dropped) and then calls `remove_dir_all`. + Ok(()) + }If you want, I can draft a
pub fn destroy(self) -> Result<()>that consumesself, flushes, drops the trees/DB, and then removes the directory.
🧹 Nitpick comments (3)
crates/storage/src/sled.rs (3)
85-107: Prefer binary/typed keys over formatted strings for cross-backend parity.RocksDB uses a typed
Key::...with(encode_to_bytes()). Using string-formatted keys here diverges from that layout and can complicate tooling/migration.I recommend introducing a shared key builder and switching to byte keys, e.g., prefix bytes +
height.encode_to_bytes()?, forcommitmentsandepochs. I can provide a small shared helper if desired.Also applies to: 129-142
1-13: Minor consistency nit: use the importedResultalias.You import
anyhow::ResultasResultbut still writeanyhow::Resultin methods. Pick one for consistency.- fn get_commitment(&self, epoch: &u64) -> anyhow::Result<Digest> { + fn get_commitment(&self, epoch: &u64) -> Result<Digest> { ... - fn set_commitment(&self, epoch: &u64, commitment: &Digest) -> anyhow::Result<()> { + fn set_commitment(&self, epoch: &u64, commitment: &Digest) -> Result<()> {(Same for other methods in this impl.)
Also applies to: 85-107
250-252: No usages ofget_rightmost_leaffound – stubs won’t panic at runtime currently. If these methods should remain unsupported, replaceunimplemented!with a non-panicking result (e.g.Ok(None)or a descriptive error).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
crates/storage/src/sled.rs(1 hunks)crates/storage/src/tests/mod.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Follow Rust Coding Standards
Use rustfmt for formatting across the codebase
Files:
crates/storage/src/tests/mod.rscrates/storage/src/sled.rs
crates/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
crates/**/*.rs: Naming: snake_case for functions/variables, CamelCase for types
Error handling: Use Result types with descriptive error messages
Handle errors with the ? operator and proper error types
Documentation: Add comments for public APIs and complex logic
File organization: Group related functionality in modules
Avoid allocations in hot paths by using references/borrowing
Use tokio for I/O-bound async/await operations
Files:
crates/storage/src/tests/mod.rscrates/storage/src/sled.rs
🧠 Learnings (1)
📚 Learning: 2025-08-18T05:30:36.919Z
Learnt from: jns-ps
PR: deltadevsde/prism#406
File: crates/storage/src/factory.rs:47-58
Timestamp: 2025-08-18T05:30:36.919Z
Learning: The codebase uses Arc<Box<dyn Database>> pattern throughout for database trait objects. While Arc<dyn Database> would be more efficient, changing this requires updating all call sites and should be done in a separate focused PR rather than as part of larger refactoring efforts.
Applied to files:
crates/storage/src/sled.rs
🧬 Code graph analysis (2)
crates/storage/src/tests/mod.rs (4)
crates/tree/src/tests/mod.rs (13)
KeyHash(85-85)KeyHash(196-196)KeyHash(250-250)KeyHash(279-279)KeyHash(337-337)KeyHash(338-338)KeyHash(374-374)KeyHash(375-375)KeyHash(422-422)KeyHash(431-431)KeyHash(432-432)KeyHash(487-487)setup_db(45-63)crates/storage/src/sled.rs (4)
new(33-39)new(61-82)get_value_option(227-248)get_latest_epoch_height(187-201)crates/storage/src/rocksdb.rs (4)
new(61-65)new(75-88)get_value_option(249-273)get_latest_epoch_height(216-225)crates/storage/src/database.rs (1)
get_latest_epoch_height(15-15)
crates/storage/src/sled.rs (2)
crates/storage/src/rocksdb.rs (14)
new(61-65)new(75-88)get_commitment(127-137)set_commitment(139-144)get_last_synced_height(146-155)set_last_synced_height(157-159)get_epoch(161-174)add_epoch(176-214)get_latest_epoch_height(216-225)get_latest_epoch(227-230)flush_database(232-234)get_node_option(238-247)get_value_option(249-273)write_node_batch(283-309)crates/storage/src/database.rs (9)
get_commitment(9-9)set_commitment(10-10)get_last_synced_height(18-18)set_last_synced_height(19-19)get_epoch(12-12)add_epoch(13-13)get_latest_epoch_height(15-15)get_latest_epoch(16-16)flush_database(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: clippy
- GitHub Check: coverage
- GitHub Check: unused dependencies
- GitHub Check: integration-test
- GitHub Check: unit-test
- GitHub Check: build-and-push-image
🔇 Additional comments (1)
crates/storage/src/tests/mod.rs (1)
22-28: Test harness backend factory is clear and correct.The backend dispatch and feature-gated RocksDB path look good. Nice touch keeping InMemory in the matrix.
Summary by CodeRabbit