feat: abci state sync #2413
feat: abci state sync #2413
Conversation
WalkthroughThis pull request introduces comprehensive state synchronization (snapshot) functionality to the Dash Platform blockchain application. The changes span multiple packages and modules, adding new configurations, error handling, and implementation details for snapshot management. Key additions include environment variable configurations for checkpoints, snapshot-related structs and methods in various application components, and new handler functions for snapshot operations like offering, applying, and finalizing snapshots. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SnapshotManager
participant Application
participant GroveDB
Client->>Application: Offer Snapshot
Application->>SnapshotManager: Initialize Snapshot Session
SnapshotManager-->>Application: Session Created
loop Chunk Processing
Client->>Application: Apply Snapshot Chunk
Application->>GroveDB: Update State
GroveDB-->>Application: Chunk Applied
end
Client->>Application: Finalize Snapshot
Application->>SnapshotManager: Commit Snapshot
SnapshotManager-->>Application: Snapshot Finalized
Possibly related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (16)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 12
🧹 Nitpick comments (19)
packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs (2)
27-30: Include detailed error information when snapshot retrieval failsCurrently, the error handling when retrieving the snapshot at the specified height discards the original error information. Including the underlying error message can provide more context for debugging.
Apply this diff to include the original error details:
- .map_err(|_| AbciError::StateSyncInternalError("load_snapshot_chunk failed".to_string()))? + .map_err(|e| AbciError::StateSyncInternalError(format!("load_snapshot_chunk failed: {}", e)))?
29-30: Differentiate between not found and other errorsWhen the snapshot is not found, the code returns a generic
StateSyncInternalError. It would be more appropriate to return an error indicating that the snapshot was not found to distinguish it from other internal errors.Apply this diff to specify a not found error:
.ok_or_else(|| { - AbciError::StateSyncInternalError("load_snapshot_chunk failed".to_string()) + AbciError::StateSyncBadRequest("Snapshot not found at the given height".to_string()) })?;packages/rs-drive-abci/src/abci/app/state_source.rs (1)
86-89: Include error details in gRPCStatusfor better debuggingWhen mapping errors to
tonic::Status, consider including more detailed error messages to aid in debugging.Apply this diff to include the error source:
.map_err(|e| tonic::Status::internal(format!("list_snapshots failed: {:?}", e)))Repeat this pattern for other error mappings in the file.
packages/rs-drive-abci/src/abci/handler/offer_snapshot.rs (3)
16-18: Avoid potential panic when convertingapp_hashThe
try_into()method can fail if theapp_hashdoes not have the expected length. Currently, the error handling uses a generic error message. Include specific details about the expected and actual lengths to improve error clarity.Apply this diff to provide detailed error information:
let request_app_hash: [u8; 32] = request.app_hash.try_into().map_err(|e| { - AbciError::StateSyncBadRequest("offer_snapshot invalid app_hash length".to_string()) + AbciError::StateSyncBadRequest(format!( + "offer_snapshot invalid app_hash length: expected 32 bytes, got {} bytes", + request.app_hash.len() + )) })?;
75-79: Consolidate duplicate error handling logicThe error handling code for wiping the GroveDB is duplicated in both branches of the conditional. Consider refactoring this into a separate function or closure to reduce code duplication and improve maintainability.
Extract the duplicated code into a helper function:
fn wipe_grovedb(app: &A) -> Result<(), AbciError> { app.platform().drive.grove.wipe().map_err(|e| { AbciError::StateSyncInternalError(format!( "offer_snapshot unable to wipe grovedb: {}", e )) }) }Then, replace the duplicated calls with:
wipe_grovedb(app)?;
69-72: Clarify error message when a newer snapshot is already being syncedThe error message "offer_snapshot already syncing newest height" might be unclear to users. Consider rephrasing it to more clearly indicate that the offered snapshot is not newer than the current one.
Apply this diff to improve the error message:
return Err(Error::Abci(AbciError::StateSyncBadRequest( - "offer_snapshot already syncing newest height".to_string(), + "Offered snapshot height is not newer than the current syncing snapshot".to_string(), )));packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs (3)
76-76: Replaceprintln!with proper logging macrosUsing
println!for logging is not recommended in production code. Consider using thetracingmacros for consistent and configurable logging.Apply these diffs to replace
println!withtracingmacros:For line 76:
-println!("[state_sync] state sync completed. verifying"); +tracing::info!("[state_sync] state sync completed. verifying");For lines 97-100:
-println!( - "[state_sync] incorrect hash in prefix:{:?}", - hex::encode(prefix) -); +tracing::error!( + "[state_sync] incorrect hash in prefix:{:?}", + hex::encode(prefix) +);Also applies to: 97-100
87-91: Include underlying error details when wrapping errorsCurrently, the error message does not include details from the underlying error, which can make debugging difficult. Including the error message provides more context.
Apply this diff to include the underlying error:
- AbciError::StateSyncInternalError( - "apply_snapshot_chunk unable to verify grovedb".to_string(), - ) + AbciError::StateSyncInternalError(format!( + "apply_snapshot_chunk unable to verify grovedb: {}", + e + ))
71-75: Include underlying error details when wrapping errorsSimilar to the previous comment, include the error message from
commit_sessionto aid in debugging.Apply this diff to include the underlying error:
- AbciError::StateSyncInternalError( - "apply_snapshot_chunk unable to commit session".to_string(), - ) + AbciError::StateSyncInternalError(format!( + "apply_snapshot_chunk unable to commit session: {}", + e + ))packages/rs-drive-abci/src/abci/app/consensus.rs (1)
40-50: Refactor shared initialization code to avoid duplicationThe
snapshot_managerinitialization code is duplicated in bothConsensusAbciApplicationandFullAbciApplication. Refactoring this into a common helper function promotes code reuse and maintainability.Consider creating a method in the
Platformstruct or a separate helper function to initialize theSnapshotManager:impl<C> Platform<C> { fn create_snapshot_manager(&self) -> Result<SnapshotManager, Error> { let checkpoints_path = self .config .state_sync_config .checkpoints_path .to_str() .ok_or_else(|| { Error::InitializationError("Invalid checkpoints path: non-UTF8 characters present".to_string()) })? .to_string(); Ok(SnapshotManager::new( checkpoints_path, self.config.state_sync_config.max_num_snapshots, self.config.state_sync_config.snapshots_frequency, )) } }Then, in
ConsensusAbciApplication::new:let snapshot_manager = platform.create_snapshot_manager()?;Adjust error handling as needed.
packages/rs-drive-abci/src/abci/app/full.rs (1)
40-50: Refactor shared initialization code to avoid duplicationThe initialization code for
snapshot_manageris duplicated across multiple files. Refactoring this code into a shared function improves maintainability.Refer to the suggestion made for
ConsensusAbciApplicationand apply the same refactoring here.packages/rs-drive-abci/tests/strategy_tests/state_sync.rs (1)
255-271: Fix the elapsed time measurement for per-chunk processingThe
elapsedvariable is calculated usingstart_time.elapsed(), which measures the total time since the beginning of the test, not the time taken to process the current chunk. To accurately measure the time per chunk, initialize a newInstantat the start of each loop iteration.Apply this diff to correct the elapsed time measurement:
while let Some(chunk_id) = chunk_queue.pop_front() { + let start_time_chunk = Instant::now(); // ... existing code ... - let elapsed = start_time.elapsed(); + let elapsed = start_time_chunk.elapsed(); // ... existing code ... }packages/rs-drive-abci/src/abci/app/mod.rs (2)
28-32: Add documentation for the SnapshotManagerApplication trait.The trait would benefit from more detailed documentation explaining:
- Its purpose and responsibilities
- When it should be implemented
- How it relates to state synchronization
/// Platform-based ABCI application pub trait SnapshotManagerApplication { - /// Returns Platform + /// Returns a reference to the SnapshotManager instance + /// + /// This trait provides snapshot management capabilities to ABCI applications, + /// enabling state synchronization features like creating, listing, and managing snapshots. fn snapshot_manager(&self) -> &SnapshotManager; }
52-59: Add documentation for the SnapshotFetchingApplication trait.The trait would benefit from more detailed documentation explaining:
- Its purpose and responsibilities
- The relationship between snapshot fetching sessions and platform
- Generic parameter constraints
-/// Application that can maintain state sync +/// Application that manages state synchronization through snapshot fetching +/// +/// This trait provides capabilities for maintaining state synchronization by: +/// - Managing snapshot fetching sessions +/// - Accessing the platform instance for state operations +/// +/// Type Parameters: +/// - 'p: Lifetime of the Platform reference +/// - C: Type implementing core RPC functionality pub trait SnapshotFetchingApplication<'p, C> { /// Returns the current snapshot fetching session fn snapshot_fetching_session(&self) -> &RwLock<Option<SnapshotFetchingSession<'p>>>;packages/rs-drive-abci/src/abci/error.rs (1)
57-64: LGTM! Consider adding documentation examples.The new error variants for state sync are well-structured and appropriately separated between client and server errors. Consider adding documentation examples to illustrate typical error scenarios.
Add documentation examples:
/// Client State sync bad request /// /// # Examples /// ``` /// // Example of a bad request when chunk height is invalid /// StateSyncBadRequest("Invalid chunk height: expected 100, got 50".to_string()) /// ``` #[error("bad request state sync: {0}")] StateSyncBadRequest(String), /// Server State sync bad request /// /// # Examples /// ``` /// // Example of an internal error when snapshot creation fails /// StateSyncInternalError("Failed to create snapshot: IO error".to_string()) /// ``` #[error("internal error state sync: {0}")] StateSyncInternalError(String),packages/rs-drive-abci/src/abci/handler/finalize_block.rs (2)
103-103: Remove unnecessary clone for primitive type.The
clone()call is unnecessary forblock_heightas it's likely a primitive type (u64/i64) which implements Copy.- .store(block_height.clone(), Ordering::Relaxed); + .store(block_height, Ordering::Relaxed);
105-114: Improve snapshot creation error handling and conditional check.The snapshot creation logic can be improved in terms of error handling and Rust idioms.
- if (app.platform().config.state_sync_config.snapshots_enabled) { + if app.platform().config.state_sync_config.snapshots_enabled { app.snapshot_manager() .create_snapshot(&app.platform().drive.grove, block_height as i64) .map_err(|e| { - Error::Execution(ExecutionError::CorruptedDriveResponse(format!( - "Unable to create snapshot:{}", - e - ))) + Error::Execution(ExecutionError::CorruptedDriveResponse( + format!("Unable to create snapshot: {e}") + )) })?; }packages/rs-drive/src/error/drive.rs (1)
191-194: Enhance snapshot error variant with more specific details.The current
Snapshoterror variant could be more informative by including specific error types or codes.Consider replacing with a more detailed variant:
- /// Error todo - #[error("snapshot error")] - Snapshot(String), + /// Snapshot operation errors + #[error("snapshot error: {kind}: {message}")] + Snapshot { + /// Type of snapshot error + kind: SnapshotErrorKind, + /// Detailed error message + message: String, + },Add the error kind enum:
/// Types of snapshot errors #[derive(Debug, Clone, PartialEq, Eq)] pub enum SnapshotErrorKind { /// Error creating snapshot Creation, /// Error loading snapshot Loading, /// Error applying snapshot chunk ChunkApplication, }packages/rs-drive-abci/.env.local (1)
16-16: Add a descriptive comment forCHECKPOINTS_PATH.Consider adding a descriptive comment explaining the purpose of this variable, similar to how
GROVEDB_LATEST_FILEis documented.+# Path to store ABCI state sync checkpoints CHECKPOINTS_PATH=${DB_PATH}/checkpoints
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpackages/dashmate/templates/platform/drive/tenderdash/config.toml.dotis excluded by!**/*.dot
📒 Files selected for processing (22)
packages/rs-drive-abci/.env.local(1 hunks)packages/rs-drive-abci/src/abci/app/consensus.rs(4 hunks)packages/rs-drive-abci/src/abci/app/full.rs(4 hunks)packages/rs-drive-abci/src/abci/app/mod.rs(2 hunks)packages/rs-drive-abci/src/abci/app/state_source.rs(1 hunks)packages/rs-drive-abci/src/abci/config.rs(2 hunks)packages/rs-drive-abci/src/abci/error.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/finalize_block.rs(3 hunks)packages/rs-drive-abci/src/abci/handler/list_snapshots.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/mod.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/offer_snapshot.rs(1 hunks)packages/rs-drive-abci/src/config.rs(7 hunks)packages/rs-drive-abci/src/platform_types/mod.rs(1 hunks)packages/rs-drive-abci/src/platform_types/snapshot/mod.rs(1 hunks)packages/rs-drive-abci/src/server.rs(2 hunks)packages/rs-drive-abci/tests/strategy_tests/main.rs(1 hunks)packages/rs-drive-abci/tests/strategy_tests/state_sync.rs(1 hunks)packages/rs-drive/Cargo.toml(1 hunks)packages/rs-drive/src/error/drive.rs(1 hunks)packages/rs-platform-version/Cargo.toml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/rs-drive-abci/src/platform_types/mod.rs
- packages/rs-drive-abci/tests/strategy_tests/main.rs
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (8)
packages/rs-platform-version/Cargo.toml (1)
14-14: Consider using a fixed version or commit hash instead of develop branch.Using a git dependency pointing to the
developbranch can lead to build reproducibility issues and unexpected breaking changes. Consider one of these alternatives:
- Use a specific commit hash for better reproducibility:
grovedb-version = { git = "https://github.com/dashpay/grovedb", rev = "<commit-hash>" }
- Wait for a stable release and use version constraints:
grovedb-version = { version = "^2.2.0" }Let's verify the current state of the grovedb repository:
packages/rs-drive/Cargo.toml (1)
Line range hint
91-116: Verify feature flags required for ABCI state sync.The PR implements ABCI state sync, but it's unclear which feature flags are required for this functionality. Please:
- Document which feature flags are necessary for state sync
- Consider creating a dedicated feature flag group for state sync functionality
- Ensure all required grovedb features are enabled
Let's analyze the feature usage:
packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs (1)
39-40: Use consistent GroveDB versioningThe code uses
PlatformVersion::latest().drive.grove_versionto specify the GroveDB version. Ensure that the version used here aligns with the versions used elsewhere in the application to maintain compatibility.packages/rs-drive-abci/tests/strategy_tests/state_sync.rs (1)
71-303: Test functionrun_state_syncis well-structured and effectiveThe
run_state_syncfunction sets up the environment, runs the chain with the defined strategy, and tests the state synchronization process comprehensively. The use of temporary directories, configuration settings, and performance measurements demonstrates good practices in test implementation.packages/rs-drive-abci/src/abci/app/mod.rs (1)
35-43: Verify the impact of lifetime parameter change.The lifetime parameter change from 'a to 'p in
TransactionalApplicationtrait could affect existing implementations.✅ Verification successful
Lifetime parameter change is safe
The change from 'a to 'p in
TransactionalApplicationtrait is a non-breaking change as it only renames the generic lifetime parameter. Existing implementations inConsensusAbciApplicationandFullAbciApplicationalready use matching lifetimes, and all trait bounds in handlers are correctly aligned with their function signatures.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for implementations of TransactionalApplication to verify the impact ast-grep --pattern 'impl$_ TransactionalApplication<$_> for $_'Length of output: 65
Script:
#!/bin/bash # Search for any mentions of TransactionalApplication rg "TransactionalApplication" -A 5 # Also try a more flexible ast-grep pattern ast-grep --pattern 'impl TransactionalApplication' # And check the specific file content cat "packages/rs-drive-abci/src/abci/app/mod.rs"Length of output: 10868
packages/rs-drive-abci/src/abci/handler/mod.rs (1)
38-48: LGTM! Well-structured module organization.The new snapshot-related modules follow the existing pattern and are properly organized and exported.
Also applies to: 53-62
packages/rs-drive-abci/src/server.rs (1)
35-47: Consider service registration order impact.While the implementation is correct, consider if the order of service registration matters for the ABCI protocol. Currently, we have:
- CheckTx service
- StateSource service
Verify if this order aligns with the ABCI protocol requirements or if it needs to be documented.
✅ Verification successful
Service registration order appears non-critical
After thorough verification of the codebase, there's no evidence suggesting that the ABCI service registration order is critical. The services appear to operate independently, handling different types of requests. The current implementation is correct as is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any documentation or comments about ABCI service order requirements rg -i "service.*order" --type md rg -i "abci.*service.*order" --type mdLength of output: 284
Script:
#!/bin/bash # Search for ABCI service implementations and registration patterns ast-grep --pattern 'Server::builder().$$$add_service($$$)' # Look for comments about ABCI services and configuration rg -i "abci.*service" -A 3 -B 3 --type rust --type ts # Search for any configuration or setup related comments rg -i "(abci|service).*configuration" --type rustLength of output: 530
packages/rs-drive-abci/src/abci/config.rs (1)
90-106: Review environment-specific configurations.The
snapshots_frequencyandmax_num_snapshotsvalues are identical for local and testnet environments. Consider if these should be different:
- Local environment might benefit from more frequent snapshots for development
- Testnet might need different retention policies
packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs
Outdated
Show resolved
Hide resolved
| /// Configuration for StateSync feature | ||
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| pub struct StateSyncAbciConfig { | ||
| /// Enable snapshot | ||
| pub snapshots_enabled: bool, | ||
|
|
||
| /// Path to checkpoints | ||
| pub checkpoints_path: PathBuf, | ||
|
|
||
| /// Frequency of snapshot creation (in blocks) | ||
| pub snapshots_frequency: i64, | ||
|
|
||
| /// Maximum number of snapshots to keep | ||
| pub max_num_snapshots: usize, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for configuration values.
The struct should validate that:
snapshots_frequencyis positivemax_num_snapshotsis greater than zero
+impl StateSyncAbciConfig {
+ pub fn validate(&self) -> Result<(), &'static str> {
+ if self.snapshots_frequency <= 0 {
+ return Err("snapshots_frequency must be positive");
+ }
+ if self.max_num_snapshots == 0 {
+ return Err("max_num_snapshots must be greater than zero");
+ }
+ Ok(())
+ }
+}📝 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.
| /// Configuration for StateSync feature | |
| #[derive(Clone, Debug, Serialize, Deserialize)] | |
| pub struct StateSyncAbciConfig { | |
| /// Enable snapshot | |
| pub snapshots_enabled: bool, | |
| /// Path to checkpoints | |
| pub checkpoints_path: PathBuf, | |
| /// Frequency of snapshot creation (in blocks) | |
| pub snapshots_frequency: i64, | |
| /// Maximum number of snapshots to keep | |
| pub max_num_snapshots: usize, | |
| } | |
| /// Configuration for StateSync feature | |
| #[derive(Clone, Debug, Serialize, Deserialize)] | |
| pub struct StateSyncAbciConfig { | |
| /// Enable snapshot | |
| pub snapshots_enabled: bool, | |
| /// Path to checkpoints | |
| pub checkpoints_path: PathBuf, | |
| /// Frequency of snapshot creation (in blocks) | |
| pub snapshots_frequency: i64, | |
| /// Maximum number of snapshots to keep | |
| pub max_num_snapshots: usize, | |
| } | |
| impl StateSyncAbciConfig { | |
| pub fn validate(&self) -> Result<(), &'static str> { | |
| if self.snapshots_frequency <= 0 { | |
| return Err("snapshots_frequency must be positive"); | |
| } | |
| if self.max_num_snapshots == 0 { | |
| return Err("max_num_snapshots must be greater than zero"); | |
| } | |
| Ok(()) | |
| } | |
| } |
| /// State sync configuration | ||
| pub state_sync_config: StateSyncAbciConfig, | ||
|
|
There was a problem hiding this comment.
Include state_sync_config in deserialization intermediate struct.
The state_sync_config field is not included in the PlatformConfigIntermediate struct, which means it won't be deserialized from environment variables or config files.
struct PlatformConfigIntermediate {
// ... other fields ...
+ #[serde(default)]
+ pub state_sync_config: StateSyncAbciConfig,
}
impl<'de> Deserialize<'de> for PlatformConfig {
fn deserialize<D>(deserializer: D) -> Result<PlatformConfig, D::Error>
where
D: Deserializer<'de>,
{
let config = PlatformConfigIntermediate::deserialize(deserializer)?;
Ok(PlatformConfig {
// ... other fields ...
- state_sync_config: StateSyncAbciConfig::default(),
+ state_sync_config: config.state_sync_config,
})
}
}Also applies to: 281-281
| @@ -12,6 +12,12 @@ ABCI_LOG_STDOUT_FORMAT=pretty | |||
| ABCI_LOG_STDOUT_COLOR=true | |||
|
|
|||
| DB_PATH=/tmp/db | |||
|
|
|||
| CHECKPOINTS_PATH=${DB_PATH}/checkpoints | |||
There was a problem hiding this comment.
💡 Codebase verification
The codebase has a proper default production path (/var/lib/dash-platform/data), but it's being overridden by environment files to use temporary storage:
.env.mainnet:DB_PATH=/tmp/db.env.testnet:DB_PATH=/tmp/db
This configuration will lead to data loss on system reboot. Update the environment files to use persistent storage paths instead of /tmp/db.
🔗 Analysis chain
Verify database paths for production environments.
Both CHECKPOINTS_PATH and GROVEDB_LATEST_FILE inherit from DB_PATH which is set to /tmp/db. While this is fine for local development, using /tmp in production could lead to data loss as it's typically cleared on system reboot.
Run this script to check if there are any production environment files with similar configuration:
Also applies to: 19-19
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for production environment files and their DB_PATH configuration
# to ensure they don't use temporary directories.
echo "Searching for environment files:"
fd -t f "^\.env\.(production|prod)" packages/rs-drive-abci/
echo -e "\nChecking DB_PATH in found files:"
rg "^DB_PATH=" packages/rs-drive-abci/.env.*
Length of output: 702
Script:
#!/bin/bash
# Check for deployment documentation and configuration
echo "Searching for deployment-related files and documentation:"
fd -t f "^(deploy|docker|k8s|kubernetes|README)" packages/rs-drive-abci/
echo -e "\nChecking content of found files for database path configuration:"
rg -i "(\bdb[_\s-]path|database[_\s-]path|data[_\s-]dir)" packages/rs-drive-abci/
Length of output: 2638
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rs-drive-abci/src/abci/app/consensus.rs (1)
41-51:⚠️ Potential issueHandle potential UTF-8 conversion failure
The
to_str()call on the checkpoints path could fail if the path contains invalid UTF-8 characters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-drive-abci/src/abci/app/consensus.rs(3 hunks)packages/rs-drive-abci/src/server.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (3)
packages/rs-drive-abci/src/abci/app/consensus.rs (3)
24-35: Good use of lifetime documentation and field organizationThe struct is well-organized with clear documentation for the lifetime parameter. The new fields for snapshot management are logically grouped.
68-82: Well-structured trait implementationsThe new trait implementations for
SnapshotManagerApplicationandSnapshotFetchingApplicationare clean and follow good separation of concerns.
192-218: Verify error handling in snapshot operationsThe snapshot-related ABCI methods are properly integrated, but we should verify the error handling in the corresponding handlers.
Let's check the error handling implementation:
✅ Verification successful
Error handling in snapshot operations is robust and comprehensive
All snapshot handlers implement proper error handling with:
- Detailed error messages for debugging
- Proper error propagation and mapping
- Comprehensive coverage of error cases
- Consistent cleanup on errors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check error handling in snapshot handlers # Expected: Each handler should have proper error handling and mapping # Look for error handling patterns in snapshot handlers rg -A 5 "fn (offer_snapshot|apply_snapshot_chunk|list_snapshots|load_snapshot_chunk)" packages/rs-drive-abci/src/abci/handler/Length of output: 2494
Script:
#!/bin/bash # Check error handling implementation in snapshot handlers # Look for error types and handling patterns # First, check for error type definitions rg "pub.*enum.*Error" packages/rs-drive-abci/src/abci/ # Then check actual implementations in handlers for file in offer_snapshot apply_snapshot_chunk list_snapshots load_snapshot_chunk; do echo "=== $file implementation ===" cat "packages/rs-drive-abci/src/abci/handler/${file}.rs" doneLength of output: 12836
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rs-drive-abci/src/abci/app/consensus.rs (1)
41-48: 🛠️ Refactor suggestionAdd path validation for checkpoints directory
Consider validating that the checkpoints directory exists and is writable before initializing the
SnapshotManager.
🧹 Nitpick comments (2)
packages/rs-drive-abci/src/abci/app/full.rs (1)
188-214: Add documentation for snapshot-related ABCI methodsConsider adding documentation comments explaining the purpose and behavior of each snapshot-related method:
offer_snapshot: When it's called and what it decidesapply_snapshot_chunk: The chunk application processlist_snapshots: The listing criteriaload_snapshot_chunk: The chunk loading process+ /// Handles an incoming snapshot offer from a peer. + /// Returns a decision on whether to accept or reject the snapshot. fn offer_snapshot( &self, request: proto::RequestOfferSnapshot, ) -> Result<proto::ResponseOfferSnapshot, proto::ResponseException> { handler::offer_snapshot(self, request).map_err(error_into_exception) } + /// Applies a chunk of snapshot data to the local state. + /// Returns the result of the chunk application process. fn apply_snapshot_chunk( &self, request: proto::RequestApplySnapshotChunk, ) -> Result<proto::ResponseApplySnapshotChunk, proto::ResponseException> { handler::apply_snapshot_chunk(self, request).map_err(error_into_exception) } + /// Lists available snapshots in the local state. + /// Returns metadata about each available snapshot. fn list_snapshots( &self, request: proto::RequestListSnapshots, ) -> Result<proto::ResponseListSnapshots, proto::ResponseException> { handler::list_snapshots(self, request).map_err(error_into_exception) } + /// Loads a specific chunk of a local snapshot. + /// Returns the requested chunk data for state sync. fn load_snapshot_chunk( &self, request: proto::RequestLoadSnapshotChunk, ) -> Result<proto::ResponseLoadSnapshotChunk, proto::ResponseException> { handler::load_snapshot_chunk(self, request).map_err(error_into_exception) }packages/rs-drive-abci/src/abci/app/state_source.rs (1)
75-97: Enhance error handling in gRPC method implementations.Currently, all errors in
list_snapshotsandload_snapshot_chunkare mapped totonic::Status::internal, which may obscure the nature of the errors. Consider mapping specific error types to more appropriate gRPC status codes to provide clearer feedback to clients.Here is an example of how you might map different error kinds:
async fn list_snapshots( &self, request: tonic::Request<proto::RequestListSnapshots>, ) -> Result<tonic::Response<proto::ResponseListSnapshots>, tonic::Status> { handler::list_snapshots(self, request.into_inner()) .map(tonic::Response::new) - .map_err(|e| tonic::Status::internal(format!("list_snapshots failed: {}", e))) + .map_err(|e| match e { + SomeSpecificErrorType::NotFound => tonic::Status::not_found(e.to_string()), + SomeSpecificErrorType::InvalidInput => tonic::Status::invalid_argument(e.to_string()), + _ => tonic::Status::internal(format!("list_snapshots failed: {}", e)), + }) }Apply similar error handling to
load_snapshot_chunk:async fn load_snapshot_chunk( &self, request: tonic::Request<proto::RequestLoadSnapshotChunk>, ) -> Result<tonic::Response<proto::ResponseLoadSnapshotChunk>, tonic::Status> { handler::load_snapshot_chunk(self, request.into_inner()) .map(tonic::Response::new) - .map_err(|e| tonic::Status::internal(format!("load_snapshot_chunk failed: {}", e))) + .map_err(|e| match e { + SomeSpecificErrorType::NotFound => tonic::Status::not_found(e.to_string()), + SomeSpecificErrorType::InvalidInput => tonic::Status::invalid_argument(e.to_string()), + _ => tonic::Status::internal(format!("load_snapshot_chunk failed: {}", e)), + }) }This refactoring will help clients better understand the reasons for failures and allow them to handle different error cases appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/rs-drive-abci/src/abci/app/consensus.rs(3 hunks)packages/rs-drive-abci/src/abci/app/full.rs(4 hunks)packages/rs-drive-abci/src/abci/app/state_source.rs(1 hunks)packages/rs-drive-abci/src/abci/config.rs(2 hunks)packages/rs-drive-abci/src/platform_types/snapshot/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-drive-abci/src/abci/config.rs
- packages/rs-drive-abci/src/platform_types/snapshot/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (7)
packages/rs-drive-abci/src/abci/app/full.rs (2)
Line range hint
1-33: Clean and well-documented struct definition!The new snapshot-related imports and struct fields are properly organized with clear documentation.
63-77: Well-implemented trait implementations!The snapshot-related trait implementations are clean and efficient, using references appropriately.
packages/rs-drive-abci/src/abci/app/consensus.rs (3)
24-35: Great improvements to lifetime parameters!The change from
'ato'pmakes the code more self-documenting, and the added comment clearly explains the lifetime relationship.
65-79: Well-structured trait implementations!The snapshot-related trait implementations are clean and consistent with the updated lifetime parameters.
189-215: Add documentation for snapshot-related ABCI methodsConsider adding documentation comments explaining the purpose and behavior of each snapshot-related method.
packages/rs-drive-abci/src/abci/app/state_source.rs (2)
17-25: StructStateSourceAbciApplicationis correctly implemented.The struct definition with generic bounds and field declarations is appropriate and follows Rust conventions.
50-63: Safe initialization ofSnapshotManagerwithout potential panics.By cloning the
checkpoints_pathand passing it directly toSnapshotManager::new, the code avoids potential panics that could occur from invalid Unicode conversions. This ensures robust handling of file paths.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (10)
packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs (5)
38-38: Extract magic number into a named constant.The hardcoded value
1u16should be extracted into a named constant with clear documentation of its purpose.+const DEFAULT_SNAPSHOT_CHUNK_VERSION: u16 = 1; + pub fn apply_snapshot_chunk<'a, 'db: 'a, A, C: 'db>( // ... ) { - session.state_sync_info.apply_chunk(..., 1u16, ...) + session.state_sync_info.apply_chunk(..., DEFAULT_SNAPSHOT_CHUNK_VERSION, ...)
76-76: Use tracing instead of println for consistency.Replace
println!with the tracing framework for consistent logging throughout the codebase.- println!("[state_sync] state sync completed. verifying"); + tracing::info!("[state_sync] state sync completed. verifying");
92-92: Use is_empty() instead of comparing len() with zero.For better readability and idiomatic Rust, use
is_empty()when checking if a collection has no elements.- if incorrect_hashes.len() > 0 { + if !incorrect_hashes.is_empty() {
21-25: Deduplicate error handling for session locking.The error message for session locking is duplicated. Consider extracting it into a constant or helper function.
const SESSION_LOCK_ERROR: &str = "apply_snapshot_chunk unable to lock session (poisoned)"; fn handle_session_lock_error<T>(result: Result<T, impl std::error::Error>) -> Result<T, Error> { result.map_err(|_| AbciError::StateSyncInternalError(SESSION_LOCK_ERROR.to_string()).into()) }Also applies to: 61-65
77-91: Improve error context for GroveDB verification.The error message for GroveDB verification could include more context about what failed during verification.
.map_err(|e| { AbciError::StateSyncInternalError( - "apply_snapshot_chunk unable to verify grovedb".to_string(), + format!("apply_snapshot_chunk unable to verify grovedb: {}", e), ) })?;packages/rs-drive-abci/src/platform_types/snapshot/mod.rs (5)
11-13: Clean up imports and resolve potential duplication.
- Line 11 contains a commented-out import that should be removed
- Both lines 11 and 13 import
PlatformVersionfrom different sources, suggesting potential confusion-//use platform_version::version::PlatformVersion;
17-17: Remove unused constant.The constant
CHUNK_SIZE_16MBis defined but never used in the code.-const CHUNK_SIZE_16MB: usize = 16 * 1024 * 1024;
36-43: Remove Default derive and improve documentation.
- The
Defaultderive might create invalid instances with empty paths and zero values- The documentation comment is duplicated at line 45
-#[derive(Default, Clone)] +#[derive(Clone)] pub struct SnapshotManager { freq: i64, number_stored_snapshots: usize, checkpoints_path: PathBuf, }
47-55: Fix copy-pasted documentation comments.Several documentation comments are incorrectly copy-pasted:
- Line 50: "Snapshot accepted" for
app_hash- Line 53: "Snapshot accepted" for
state_sync_info/// Snapshot accepted pub snapshot: abci::Snapshot, - /// Snapshot accepted + /// Application hash pub app_hash: Vec<u8>, // sender_metrics: Option<HashMap<String, Metrics>>, - /// Snapshot accepted + /// State synchronization information pub state_sync_info: Pin<Box<MultiStateSyncSession<'db>>>,
205-214: Add concurrency protection.The
save_snapshotsmethod should handle concurrent access to prevent race conditions when multiple instances try to update snapshots simultaneously.Consider using a mutex or other synchronization mechanism to protect access to the snapshots data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/rs-drive-abci/src/abci/config.rs(2 hunks)packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/list_snapshots.rs(1 hunks)packages/rs-drive-abci/src/platform_types/snapshot/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive-abci/src/abci/handler/list_snapshots.rs
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Rust packages (dpp) / Detect immutable structure changes
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (3)
packages/rs-drive-abci/src/abci/config.rs (2)
68-81: Add validation for configuration values.The struct should validate numeric fields to prevent invalid configurations:
snapshots_frequencyshould be positivemax_num_snapshotsshould be greater than zeroimpl StateSyncAbciConfig { pub fn validate(&self) -> Result<(), String> { if self.snapshots_frequency <= 0 { return Err("snapshots_frequency must be positive".into()); } if self.max_num_snapshots == 0 { return Err("max_num_snapshots must be greater than zero".into()); } Ok(()) } }
118-122: 🛠️ Refactor suggestionFollow platform's environment variable naming convention.
The environment variable
CHECKPOINTS_PATHshould follow the platform's naming convention, prefixed withDASH_PLATFORM_.- std::env::var("CHECKPOINTS_PATH") + std::env::var("DASH_PLATFORM_CHECKPOINTS_PATH")Likely invalid or redundant comment.
packages/rs-drive-abci/src/platform_types/snapshot/mod.rs (1)
21-34: Well-structured and documented Snapshot entity.The struct has clear field descriptions and appropriate trait derivations.
| pub fn default_local() -> Self { | ||
| Self { | ||
| snapshots_enabled: true, | ||
| checkpoints_path: PathBuf::from("/var/lib/dash-platform/data/checkpoints"), | ||
| snapshots_frequency: 3, | ||
| max_num_snapshots: 10, | ||
| } | ||
| } | ||
|
|
||
| pub fn default_testnet() -> Self { | ||
| Self { | ||
| snapshots_enabled: true, | ||
| checkpoints_path: PathBuf::from("/var/lib/dash-platform/data/checkpoints"), | ||
| snapshots_frequency: 3, | ||
| max_num_snapshots: 10, | ||
| } | ||
| } | ||
|
|
||
| pub fn default_mainnet() -> Self { | ||
| Self { | ||
| snapshots_enabled: true, | ||
| checkpoints_path: PathBuf::from("/var/lib/dash-platform/data/checkpoints"), | ||
| snapshots_frequency: 3, | ||
| max_num_snapshots: 10, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate configuration.
The default configurations for local, testnet, and mainnet are identical. Consider:
- Using a single private method to create the base configuration
- Allowing environment-specific overrides only where needed
impl StateSyncAbciConfig {
fn base_config() -> Self {
Self {
snapshots_enabled: true,
checkpoints_path: Self::default_checkpoints_path(),
snapshots_frequency: 3,
max_num_snapshots: 10,
}
}
pub fn default_local() -> Self {
Self::base_config()
}
pub fn default_testnet() -> Self {
Self::base_config()
}
pub fn default_mainnet() -> Self {
Self::base_config()
}
}| refetch_chunks: vec![], // TODO: Check when this is needed | ||
| reject_senders: vec![], // TODO: Check when this is needed |
There was a problem hiding this comment.
💡 Codebase verification
Implement chunk refetching and sender rejection for state sync robustness
The empty vectors for refetch_chunks and reject_senders should be replaced with proper implementation to:
- Request refetch of corrupted or invalid chunks
- Reject nodes that consistently send invalid chunks
packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs: Implement chunk validation and track sender reliability
🔗 Analysis chain
Address TODO comments regarding chunk refetching and sender rejection.
The TODO comments indicate missing implementation details for refetch_chunks and reject_senders. These parameters are important for handling corrupted or invalid chunks.
Let's verify if these features are implemented in other state sync implementations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for refetch_chunks and reject_senders usage in other implementations
rg -A 5 'refetch_chunks|reject_senders' --type rust
Length of output: 1246
Script:
#!/bin/bash
# Find the struct definition and related snapshot handling code
ast-grep --pattern 'struct $_ {
$$$
refetch_chunks: $_,
$$$
}'
# Search for documentation and comments about snapshot handling
rg -B 5 'struct.*Snapshot|fn.*snapshot|//.*snapshot' --type rust
# Look for error handling related to chunks
rg -B 3 'Error.*chunk|chunk.*Error|invalid.*chunk|corrupt.*chunk' --type rust
Length of output: 24974
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use std::fs; | ||
|
|
||
| #[test] | ||
| fn test_create_snapshot() { | ||
| let test_cases = vec![ | ||
| (1000, 1000, vec![1000]), | ||
| (1000, 1001, vec![1000, 1001]), | ||
| (1000, 1002, vec![1000, 1001, 1002]), | ||
| (1000, 1004, vec![1002, 1003, 1004]), | ||
| (1000, 1005, vec![1003, 1004, 1005]), | ||
| ]; | ||
| for (start, end, want) in test_cases { | ||
| let grove_dir = tempfile::tempdir().unwrap(); | ||
| let checkpoints_dir = tempfile::tempdir().unwrap(); | ||
| let grove = GroveDb::open(grove_dir.path()).unwrap(); | ||
| let manager = | ||
| SnapshotManager::new(checkpoints_dir.path().to_str().unwrap().to_string().into(), 3, 1); | ||
| for height in start..=end { | ||
| manager.create_snapshot(&grove, height).unwrap(); | ||
| } | ||
| let snapshots = manager.get_snapshots(&grove).unwrap(); | ||
| let res: Vec<i64> = snapshots.iter().map(|s| s.height).collect(); | ||
| assert_eq!(want, res); | ||
|
|
||
| let paths: Vec<String> = snapshots.iter().map(|s| s.path.to_string()).collect(); | ||
| for path in paths { | ||
| assert!(Path::new(&path).exists()); | ||
| } | ||
| fs::remove_dir_all(grove_dir.path()).unwrap(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test coverage.
Add tests for:
- Error handling (invalid paths, concurrent access)
- Edge cases (zero/negative freq, zero stored snapshots)
get_snapshot_at_heightfunctionality
Example test cases to add:
#[test]
fn test_get_snapshot_at_height() {
let grove_dir = tempfile::tempdir().unwrap();
let checkpoints_dir = tempfile::tempdir().unwrap();
let grove = GroveDb::open(grove_dir.path()).unwrap();
let manager = SnapshotManager::new(checkpoints_dir.path().to_str().unwrap().to_string().into(), 3, 1);
// Create some snapshots
for height in 1000..=1002 {
manager.create_snapshot(&grove, height).unwrap();
}
// Test existing height
let snapshot = manager.get_snapshot_at_height(&grove, 1001).unwrap();
assert!(snapshot.is_some());
assert_eq!(snapshot.unwrap().height, 1001);
// Test non-existent height
let snapshot = manager.get_snapshot_at_height(&grove, 999).unwrap();
assert!(snapshot.is_none());
}
#[test]
fn test_invalid_parameters() {
let checkpoints_dir = tempfile::tempdir().unwrap();
let manager = SnapshotManager::new(
checkpoints_dir.path().to_str().unwrap().to_string().into(),
0, // Invalid number_stored_snapshots
-1, // Invalid freq
);
// Assert logs contain error messages
}| let snapshot = Snapshot { | ||
| height, | ||
| version: SNAPSHOT_VERSION, | ||
| path: checkpoint_path.to_str().unwrap().to_string(), | ||
| hash: root_hash as [u8; 32], | ||
| metadata: vec![], | ||
| }; |
There was a problem hiding this comment.
Handle non-UTF8 paths safely.
The unwrap() call on to_str() could panic with non-UTF8 paths.
let snapshot = Snapshot {
height,
version: SNAPSHOT_VERSION,
- path: checkpoint_path.to_str().unwrap().to_string(),
+ path: checkpoint_path.to_string_lossy().into_owned(),
hash: root_hash as [u8; 32],
metadata: vec![],
};📝 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.
| let snapshot = Snapshot { | |
| height, | |
| version: SNAPSHOT_VERSION, | |
| path: checkpoint_path.to_str().unwrap().to_string(), | |
| hash: root_hash as [u8; 32], | |
| metadata: vec![], | |
| }; | |
| let snapshot = Snapshot { | |
| height, | |
| version: SNAPSHOT_VERSION, | |
| path: checkpoint_path.to_string_lossy().into_owned(), | |
| hash: root_hash as [u8; 32], | |
| metadata: vec![], | |
| }; |
| let root_hash = grove | ||
| .root_hash(None, &PlatformVersion::latest().drive.grove_version) | ||
| .unwrap() | ||
| .map_err(|e| Error::Drive(Drive(DriveError::Snapshot(e.to_string()))))?; | ||
|
|
There was a problem hiding this comment.
Remove unwrap() to prevent panics.
The unwrap() call on root_hash could panic. Handle the error properly.
let root_hash = grove
.root_hash(None, &PlatformVersion::latest().drive.grove_version)
- .unwrap()
.map_err(|e| Error::Drive(Drive(DriveError::Snapshot(e.to_string()))))?;📝 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.
| let root_hash = grove | |
| .root_hash(None, &PlatformVersion::latest().drive.grove_version) | |
| .unwrap() | |
| .map_err(|e| Error::Drive(Drive(DriveError::Snapshot(e.to_string()))))?; | |
| let root_hash = grove | |
| .root_hash(None, &PlatformVersion::latest().drive.grove_version) | |
| .map_err(|e| Error::Drive(Drive(DriveError::Snapshot(e.to_string()))))?; | |
| pub fn new(checkpoints_path: PathBuf, number_stored_snapshots: usize, freq: i64) -> Self { | ||
| if let Err(e) = fs::create_dir_all(checkpoints_path.clone()) { | ||
| tracing::error!( | ||
| "Failed to create directory {}: {}", | ||
| checkpoints_path.display(), | ||
| e | ||
| ); | ||
| } | ||
| Self { | ||
| freq, | ||
| number_stored_snapshots, | ||
| checkpoints_path, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation in constructor.
The new method should validate its parameters:
freqshould be positivenumber_stored_snapshotsshould be greater than zero
pub fn new(checkpoints_path: PathBuf, number_stored_snapshots: usize, freq: i64) -> Self {
+ if freq <= 0 {
+ tracing::error!("Frequency must be positive, got {}", freq);
+ }
+ if number_stored_snapshots == 0 {
+ tracing::error!("Number of stored snapshots must be greater than zero");
+ }
if let Err(e) = fs::create_dir_all(checkpoints_path.clone()) {📝 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.
| pub fn new(checkpoints_path: PathBuf, number_stored_snapshots: usize, freq: i64) -> Self { | |
| if let Err(e) = fs::create_dir_all(checkpoints_path.clone()) { | |
| tracing::error!( | |
| "Failed to create directory {}: {}", | |
| checkpoints_path.display(), | |
| e | |
| ); | |
| } | |
| Self { | |
| freq, | |
| number_stored_snapshots, | |
| checkpoints_path, | |
| } | |
| } | |
| pub fn new(checkpoints_path: PathBuf, number_stored_snapshots: usize, freq: i64) -> Self { | |
| if freq <= 0 { | |
| tracing::error!("Frequency must be positive, got {}", freq); | |
| } | |
| if number_stored_snapshots == 0 { | |
| tracing::error!("Number of stored snapshots must be greater than zero"); | |
| } | |
| if let Err(e) = fs::create_dir_all(checkpoints_path.clone()) { | |
| tracing::error!( | |
| "Failed to create directory {}: {}", | |
| checkpoints_path.display(), | |
| e | |
| ); | |
| } | |
| Self { | |
| freq, | |
| number_stored_snapshots, | |
| checkpoints_path, | |
| } | |
| } |
| // TODO: Use Metrics for statistics | ||
| struct Metrics { | ||
| success: usize, | ||
| error: usize, | ||
| } | ||
|
|
||
| enum MetricType { | ||
| Success, | ||
| Error, | ||
| } | ||
|
|
||
| impl Metrics { | ||
| fn new() -> Self { | ||
| Self { | ||
| success: 0, | ||
| error: 0, | ||
| } | ||
| } | ||
|
|
||
| fn incr(&mut self, metric: MetricType) { | ||
| match metric { | ||
| MetricType::Success => self.success += 1, | ||
| MetricType::Error => self.error += 1, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Remove unused Metrics implementation
The Metrics struct and its implementation are currently unused in the codebase. Since there's no active metrics tracking, this code should be removed and reintroduced when metrics implementation is actually needed to avoid maintaining unused code.
🔗 Analysis chain
Verify the need for Metrics implementation.
The Metrics implementation is complete but currently unused. Consider either:
- Implementing the metrics tracking now
- Moving this to a separate PR when metrics are needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing metrics implementations or TODOs
rg -g '!target' -g '*.rs' "TODO.*metrics|metrics.*TODO" .
Length of output: 59
Script:
#!/bin/bash
# Search for Metrics struct usage
ast-grep --pattern 'struct Metrics'
# Search for any metric-related implementations
rg -g '!target' -g '*.rs' -B 2 -A 2 'impl.*Metrics|Metrics::'
# Get more context around the current implementation
rg -g '!target' -g '*.rs' -B 5 -A 5 'struct Metrics'
Length of output: 1572
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/rs-drive-abci/src/abci/handler/offer_snapshot.rs (3)
34-39: Refactor duplicated code to improve maintainabilityThe code for wiping the GroveDB and handling errors is duplicated in both the
ifandelseblocks. Consider refactoring this repeated code into a helper function to reduce duplication and enhance maintainability.Apply this diff to extract the duplicated code into a helper function:
+fn wipe_grovedb(app: &A) -> Result<(), AbciError> { + app.platform().drive.grove.wipe().map_err(|e| { + AbciError::StateSyncInternalError(format!( + "offer_snapshot unable to wipe grovedb: {}", + e + )) + }) +} // Replace the duplicated code in both blocks with: - app.platform().drive.grove.wipe().map_err(|e| { - AbciError::StateSyncInternalError(format!( - "offer_snapshot unable to wipe grovedb:{}", - e - )) - })?; + wipe_grovedb(app)?;Also applies to: 76-81
40-54: Refactor duplicated snapshot syncing codeThe code for starting snapshot syncing is repeated in both the
ifandelseblocks. Refactoring this code into a helper function will reduce duplication and potential errors during future updates.You can create a helper function like this:
+fn start_snapshot_syncing(app: &A, request_app_hash: [u8; 32]) -> Result<_, AbciError> { + app.platform() + .drive + .grove + .start_snapshot_syncing( + request_app_hash, + CURRENT_STATE_SYNC_VERSION, + &PlatformVersion::latest().drive.grove_version, + ) + .map_err(|e| { + AbciError::StateSyncInternalError(format!( + "offer_snapshot unable to start snapshot syncing session: {}", + e + )) + }) +} // Replace the duplicated code in both blocks with: - let state_sync_info = app - .platform() - .drive - .grove - .start_snapshot_syncing( - request_app_hash, - CURRENT_STATE_SYNC_VERSION, - &PlatformVersion::latest().drive.grove_version, - ) - .map_err(|e| { - AbciError::StateSyncInternalError(format!( - "offer_snapshot unable to start snapshot syncing session:{}", - e - )) - })?; + let state_sync_info = start_snapshot_syncing(app, request_app_hash)?;Also applies to: 82-96
17-19: Enhance error message for invalidapp_hashlengthThe error message
"offer_snapshot invalid app_hash length"could be more informative by including the expected and actual lengths, aiding in debugging.Consider updating the error message:
- AbciError::StateSyncBadRequest("offer_snapshot invalid app_hash length".to_string()) + AbciError::StateSyncBadRequest(format!( + "offer_snapshot invalid app_hash length: expected 32 bytes, got {} bytes", + request.app_hash.len() + ))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/abci/handler/offer_snapshot.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
| let mut response = proto::ResponseOfferSnapshot::default(); | ||
| response.result = i32::from(response_offer_snapshot::Result::Accept); | ||
| Ok(response) |
There was a problem hiding this comment.
Ensure consistent setting of response.result field
In the if block, response.result is explicitly set to Accept:
response.result = i32::from(response_offer_snapshot::Result::Accept);However, in the else block, you're returning a default response without setting the result field. This could lead to inconsistent behavior or incorrect responses to the ABCI client. Ensure that response.result is set appropriately in both cases.
Apply this diff to set response.result in the else block:
let response = proto::ResponseOfferSnapshot::default();
+response.result = i32::from(response_offer_snapshot::Result::Accept);
Ok(response)Also applies to: 100-101
| let session = session_write_guard | ||
| .as_mut() | ||
| .ok_or(AbciError::StateSyncInternalError( | ||
| "offer_snapshot unable to lock session".to_string(), | ||
| ))?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify session retrieval and error handling
Since you're in the else block where session_write_guard.is_some() is guaranteed, using ok_or is unnecessary. You can safely unwrap the Option or use expect with a clear message.
Apply this diff to simplify the code:
-let session = session_write_guard
- .as_mut()
- .ok_or(AbciError::StateSyncInternalError(
- "offer_snapshot unable to lock session".to_string(),
- ))?;
+let session = session_write_guard.as_mut().expect(
+ "Session should be present in else block of offer_snapshot"
+);📝 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.
| let session = session_write_guard | |
| .as_mut() | |
| .ok_or(AbciError::StateSyncInternalError( | |
| "offer_snapshot unable to lock session".to_string(), | |
| ))?; | |
| let session = session_write_guard.as_mut().expect( | |
| "Session should be present in else block of offer_snapshot" | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs (4)
4-4: Remove commented-out import.The commented-out import
platform_version::version::PlatformVersionappears to be unused. If it's no longer needed, please remove it to maintain code cleanliness.-//use platform_version::version::PlatformVersion;
19-23: Consider using a higher logging level for state sync operations.State sync is a critical operation. Consider using
tracing::info!ortracing::debug!instead oftrace!to ensure these important operations are visible in typical logging configurations.- tracing::trace!( + tracing::info!(
24-30: Improve error messages and fix typo.The error messages could be more descriptive and consistent. Also, there's a typo in "mattched".
- AbciError::StateSyncInternalError("load_snapshot_chunk failed: empty mattched snapshot".to_string()) + AbciError::StateSyncInternalError("load_snapshot_chunk failed: no snapshot found at height".to_string())
44-46: Add chunk validation before response construction.Consider validating the chunk size and content before constructing the response to prevent potential memory issues or invalid data transmission.
+ // Validate chunk size and content + if chunk.len() > MAX_CHUNK_SIZE { + return Err(AbciError::StateSyncInternalError("Chunk size exceeds maximum allowed size".to_string()).into()); + } let mut response = proto::ResponseLoadSnapshotChunk::default(); response.chunk = chunk;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Formatting
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (2)
packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs (2)
11-18: Well-structured function signature with appropriate generic constraints!The generic design with
SnapshotManagerApplicationandPlatformApplicationtrait bounds provides good flexibility while ensuring type safety.
34-43: Verify version compatibility and conversion safety.Two potential issues to address:
- Converting request.version from i32 to u16 could truncate data.
- Using
PlatformVersion::latest()might cause compatibility issues with older snapshots.Consider:
- Adding version range validation
- Using the snapshot's original version instead of latest
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/rs-drive-abci/src/abci/config.rs (2)
66-81: 🛠️ Refactor suggestionAdd validation for configuration values.
The struct should validate that:
snapshots_frequencyis positivemax_num_snapshotsis greater than zeroimpl StateSyncAbciConfig { + pub fn validate(&self) -> Result<(), &'static str> { + if self.snapshots_frequency <= 0 { + return Err("snapshots_frequency must be positive"); + } + if self.max_num_snapshots == 0 { + return Err("max_num_snapshots must be greater than zero"); + } + Ok(()) + } }
91-116: 🛠️ Refactor suggestionConsolidate duplicate configuration.
The default configurations for local, testnet, and mainnet are identical. Consider using a single private method to create the base configuration.
impl StateSyncAbciConfig { fn base_config() -> Self { Self { snapshots_enabled: true, checkpoints_path: Self::default_checkpoints_path(), snapshots_frequency: 10, max_num_snapshots: 100, } } pub fn default_local() -> Self { Self::base_config() } pub fn default_testnet() -> Self { Self::base_config() } pub fn default_mainnet() -> Self { Self::base_config() } }
🧹 Nitpick comments (2)
packages/rs-drive-abci/src/abci/config.rs (1)
118-122: Use a more specific environment variable name.The environment variable
CHECKPOINTS_PATHshould be prefixed with the application name to avoid conflicts.fn default_checkpoints_path() -> PathBuf { - std::env::var("CHECKPOINTS_PATH") + std::env::var("DASH_PLATFORM_CHECKPOINTS_PATH") .map(PathBuf::from) .unwrap_or_else(|_| PathBuf::from("/var/lib/dash-platform/data/checkpoints")) }packages/rs-drive-abci/tests/strategy_tests/state_sync.rs (1)
156-161: Document magic numbers in test configuration.The test configuration uses magic numbers without explanation. Consider adding constants with descriptive names or comments explaining these values.
+ // Constants for test configuration + const TEST_SNAPSHOT_FREQUENCY: i64 = 10; // Take snapshot every 10 blocks + const TEST_MAX_SNAPSHOTS: usize = 3; // Keep last 3 snapshots + let local_state_sync_config = StateSyncAbciConfig { snapshots_enabled: true, checkpoints_path: checkpoint_test_directory, - snapshots_frequency: 10, - max_num_snapshots: 3, + snapshots_frequency: TEST_SNAPSHOT_FREQUENCY, + max_num_snapshots: TEST_MAX_SNAPSHOTS, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rs-drive-abci/src/abci/config.rs(2 hunks)packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs(1 hunks)packages/rs-drive-abci/tests/strategy_tests/state_sync.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs
| fn get_target_tmp_folder() -> PathBuf { | ||
| PathBuf::from("/Users/odysseasg/Downloads/state_sync/").join("tmp") | ||
| //get_target_folder().join("tmp") | ||
| } |
There was a problem hiding this comment.
Remove hardcoded user path.
The function contains a hardcoded user-specific path. Use the commented-out implementation that uses CARGO_MANIFEST_DIR instead.
fn get_target_tmp_folder() -> PathBuf {
- PathBuf::from("/Users/odysseasg/Downloads/state_sync/").join("tmp")
+ get_target_folder().join("tmp")
}📝 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 get_target_tmp_folder() -> PathBuf { | |
| PathBuf::from("/Users/odysseasg/Downloads/state_sync/").join("tmp") | |
| //get_target_folder().join("tmp") | |
| } | |
| fn get_target_tmp_folder() -> PathBuf { | |
| get_target_folder().join("tmp") | |
| } |
| println!( | ||
| "source app_hash:{}", | ||
| hex::encode( | ||
| source_outcome | ||
| .abci_app | ||
| .platform | ||
| .drive | ||
| .grove | ||
| .root_hash(None, &PlatformVersion::latest().drive.grove_version) | ||
| .unwrap() | ||
| .unwrap() | ||
| ) | ||
| ); | ||
| println!( | ||
| "target app_hash:{}", | ||
| hex::encode( | ||
| target_abci_app | ||
| .platform() | ||
| .drive | ||
| .grove | ||
| .root_hash(None, &PlatformVersion::latest().drive.grove_version) | ||
| .unwrap() | ||
| .unwrap() | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit assertions for state sync verification.
The test only prints the source and target app hashes without explicitly verifying they match. Add assertions to validate the state sync was successful.
+ let source_hash = source_outcome
+ .abci_app
+ .platform
+ .drive
+ .grove
+ .root_hash(None, &PlatformVersion::latest().drive.grove_version)
+ .unwrap()
+ .unwrap();
+
+ let target_hash = target_abci_app
+ .platform()
+ .drive
+ .grove
+ .root_hash(None, &PlatformVersion::latest().drive.grove_version)
+ .unwrap()
+ .unwrap();
+
+ assert_eq!(
+ source_hash,
+ target_hash,
+ "Source and target app hashes must match after state sync"
+ );
+
println!(
"source app_hash:{}",
- hex::encode(
- source_outcome
- .abci_app
- .platform
- .drive
- .grove
- .root_hash(None, &PlatformVersion::latest().drive.grove_version)
- .unwrap()
- .unwrap()
- )
+ hex::encode(&source_hash)
);
println!(
"target app_hash:{}",
- hex::encode(
- target_abci_app
- .platform()
- .drive
- .grove
- .root_hash(None, &PlatformVersion::latest().drive.grove_version)
- .unwrap()
- .unwrap()
- )
+ hex::encode(&target_hash)
);📝 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.
| println!( | |
| "source app_hash:{}", | |
| hex::encode( | |
| source_outcome | |
| .abci_app | |
| .platform | |
| .drive | |
| .grove | |
| .root_hash(None, &PlatformVersion::latest().drive.grove_version) | |
| .unwrap() | |
| .unwrap() | |
| ) | |
| ); | |
| println!( | |
| "target app_hash:{}", | |
| hex::encode( | |
| target_abci_app | |
| .platform() | |
| .drive | |
| .grove | |
| .root_hash(None, &PlatformVersion::latest().drive.grove_version) | |
| .unwrap() | |
| .unwrap() | |
| ) | |
| ); | |
| let source_hash = source_outcome | |
| .abci_app | |
| .platform | |
| .drive | |
| .grove | |
| .root_hash(None, &PlatformVersion::latest().drive.grove_version) | |
| .unwrap() | |
| .unwrap(); | |
| let target_hash = target_abci_app | |
| .platform() | |
| .drive | |
| .grove | |
| .root_hash(None, &PlatformVersion::latest().drive.grove_version) | |
| .unwrap() | |
| .unwrap(); | |
| assert_eq!( | |
| source_hash, | |
| target_hash, | |
| "Source and target app hashes must match after state sync" | |
| ); | |
| println!( | |
| "source app_hash:{}", | |
| hex::encode(&source_hash) | |
| ); | |
| println!( | |
| "target app_hash:{}", | |
| hex::encode(&target_hash) | |
| ); |
| println!( | ||
| "best_snapshot height:{} app_hash:{}", | ||
| best_snapshot.height, | ||
| hex::encode(&best_snapshot.hash) | ||
| ); | ||
|
|
||
| let target_platform = TestPlatformBuilder::new() | ||
| .with_config(config.clone()) | ||
| .build_with_mock_rpc(); | ||
| let target_abci_app = FullAbciApplication::new(&target_platform); | ||
|
|
||
| let offer_snapshot_request = RequestOfferSnapshot { | ||
| snapshot: Some(best_snapshot.clone()), | ||
| app_hash: best_snapshot.hash.to_vec(), | ||
| }; | ||
|
|
||
| let _ = target_abci_app | ||
| .offer_snapshot(offer_snapshot_request) | ||
| .expect("should offer_snapshot succeed"); | ||
|
|
||
| let mut chunk_queue: VecDeque<Vec<u8>> = VecDeque::new(); | ||
| chunk_queue.push_back(best_snapshot.hash.to_vec()); | ||
|
|
||
| let start_time = Instant::now(); | ||
|
|
||
| let mut duration_sum_fetch: Duration = Duration::ZERO; | ||
| let mut duration_sum_apply: Duration = Duration::ZERO; | ||
|
|
||
| let mut chunk_counter = 0; | ||
| let mut ops_counter = 0; | ||
| while let Some(chunk_id) = chunk_queue.pop_front() { | ||
| let request_load_chunk = RequestLoadSnapshotChunk { | ||
| height: best_snapshot.height, | ||
| version: best_snapshot.version, | ||
| chunk_id: chunk_id.clone(), | ||
| }; | ||
| let start_time_fetch = Instant::now(); | ||
| let load_chunk_response = source_outcome | ||
| .abci_app | ||
| .load_snapshot_chunk(request_load_chunk) | ||
| .expect("should fetch chunk"); | ||
| duration_sum_fetch += start_time_fetch.elapsed(); | ||
|
|
||
| let request_apply_chunk = RequestApplySnapshotChunk { | ||
| chunk_id, | ||
| chunk: load_chunk_response.chunk, | ||
| ..Default::default() | ||
| }; | ||
| let request_apply_num_ops = request_apply_chunk.chunk.len(); | ||
| ops_counter += request_apply_num_ops; | ||
|
|
||
| let elapsed = start_time.elapsed(); | ||
| let chunk_id_hex = hex::encode(&request_apply_chunk.chunk_id); | ||
| let start_time_apply = Instant::now(); | ||
| let apply_chunk_response = target_abci_app | ||
| .apply_snapshot_chunk(request_apply_chunk) | ||
| .expect("should apply chunk succeed"); | ||
| duration_sum_apply += start_time_apply.elapsed(); | ||
| println!( | ||
| "#{} apply:{} num_ops:{} returned:{} queue:{} {:.2?}", | ||
| chunk_counter, | ||
| chunk_id_hex, | ||
| request_apply_num_ops, | ||
| apply_chunk_response.next_chunks.len(), | ||
| chunk_queue.len(), | ||
| elapsed | ||
| ); | ||
| chunk_queue.extend(apply_chunk_response.next_chunks); | ||
| chunk_counter += 1; | ||
| } | ||
| println!("total chunks:{} ops:{}", chunk_counter, ops_counter); | ||
| println!("duration_sum_fetch: {}", duration_sum_fetch.as_secs_f64()); | ||
| println!("duration_sum_apply: {}", duration_sum_apply.as_secs_f64()); | ||
|
|
||
| println!( | ||
| "source app_hash:{}", | ||
| hex::encode( | ||
| source_outcome | ||
| .abci_app | ||
| .platform | ||
| .drive | ||
| .grove | ||
| .root_hash(None, &PlatformVersion::latest().drive.grove_version) | ||
| .unwrap() | ||
| .unwrap() | ||
| ) | ||
| ); | ||
| println!( | ||
| "target app_hash:{}", | ||
| hex::encode( | ||
| target_abci_app | ||
| .platform() | ||
| .drive | ||
| .grove | ||
| .root_hash(None, &PlatformVersion::latest().drive.grove_version) | ||
| .unwrap() | ||
| .unwrap() | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup after test.
The test creates temporary directories but doesn't clean them up after the test completes. Add cleanup in a Drop implementation or use a cleanup function.
#[test]
fn run_state_sync() {
+ struct Cleanup {
+ base_dir: PathBuf,
+ }
+
+ impl Drop for Cleanup {
+ fn drop(&mut self) {
+ if let Err(e) = remove_dir(&self.base_dir) {
+ eprintln!("Failed to cleanup test directories: {}", e);
+ }
+ }
+ }
+
let platform_version = PlatformVersion::latest();
// ... rest of the test setup ...
+
+ let _cleanup = Cleanup {
+ base_dir: base_test_directory.clone(),
+ };
// ... rest of the test ...
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs (5)
21-25: Extract duplicate error message into a constant.The error message "apply_snapshot_chunk unable to lock session (poisoned)" is duplicated. Consider extracting it into a constant for better maintainability.
+const LOCK_SESSION_ERROR: &str = "apply_snapshot_chunk unable to lock session (poisoned)"; + let mut session_write_guard = app.snapshot_fetching_session().write().map_err(|_| { - AbciError::StateSyncInternalError( - "apply_snapshot_chunk unable to lock session (poisoned)".to_string(), - ) + AbciError::StateSyncInternalError(LOCK_SESSION_ERROR.to_string()) })?; // ... later in the code ... let session = session_write_guard .take() .ok_or(AbciError::StateSyncInternalError( - "apply_snapshot_chunk unable to lock session (poisoned)".to_string(), + LOCK_SESSION_ERROR.to_string(), ))?;Also applies to: 63-65
38-38: Document the purpose of the hardcoded chunk format version.The hardcoded value
1u16appears to be a chunk format version. Please add a comment explaining its purpose or consider making it a named constant.+// Chunk format version 1 represents the initial version of the snapshot chunk format session .state_sync_info .apply_chunk( &app.platform().drive.grove, &request.chunk_id, request.chunk, - 1u16, + CHUNK_FORMAT_VERSION, &PlatformVersion::latest().drive.grove_version, )
76-76: Replace println! with tracing macro for consistent logging.Use the tracing macro for consistency with the rest of the codebase.
-println!("[state_sync] state sync completed. verifying"); +tracing::info!("[state_sync] state sync completed. verifying");
92-97: Simplify the length comparison and improve error message.The length comparison can be simplified, and the error message could include the incorrect hashes for better debugging.
-if incorrect_hashes.len() > 0 { +if !incorrect_hashes.is_empty() { Err(AbciError::StateSyncInternalError(format!( - "apply_snapshot_chunk grovedb verification failed with {} incorrect hashes", - incorrect_hashes.len() + "apply_snapshot_chunk grovedb verification failed with {} incorrect hashes: {:?}", + incorrect_hashes.len(), + incorrect_hashes )))?; }
70-75: Include original error in StateSyncInternalError.The error handling for
commit_sessiondiscards the original error information. Include it in the error message for better debugging..commit_session(state_sync_info) .map_err(|e| { AbciError::StateSyncInternalError( - "apply_snapshot_chunk unable to commit session".to_string(), + format!("apply_snapshot_chunk unable to commit session: {}", e) ) })?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs(1 hunks)
🔇 Additional comments (2)
packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs (2)
1-14: LGTM! Well-structured function signature with proper trait bounds.The function signature is well-designed with appropriate lifetime bounds and trait constraints for the application parameter.
53-54: Implement chunk refetching and sender rejection for state sync robustness.The empty vectors for
refetch_chunksandreject_sendersshould be replaced with proper implementation to:
- Request refetch of corrupted or invalid chunks
- Reject nodes that consistently send invalid chunks
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
packages/rs-drive-abci/src/abci/app/check_tx.rs (2)
26-27: Ensure forward-compatibility of thesnapshot_managerfieldDeclaring a
snapshot_managerin theCheckTxAbciApplicationis a sound design choice to handle snapshot operations. Make sure the field remains optional if there's a scenario in which snapshots are disabled, especially to avoid overhead or uninitialized usage in minimal builds.
116-124: Implementation oflist_snapshotsThe method delegates to a handler; minimal logic is within the ABCI application itself. This structure is desirable for separation of concerns. Consider adding explicit logging for better traceability of snapshot listing requests.
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (2)
37-304:finalize_snapshotfunction logic
- Validation: The step-by-step checks ensure all snapshot fields are present and have appropriate sizes. This is crucial to avoid malformed snapshot data.
- Construction: Building
BlockInfo,BlockStateInfoV0, and loading masternode lists carefully addresses the required domain logic.- State update: Storing and committing the new
PlatformStateis properly done in a transactional manner.- Deterministic ordering: Sorting validator sets by height, then by quorum hash is an excellent approach to guarantee consistency across nodes.
One minor suggestion is to log or handle any potential missing or invalid quorum data from
core_rpcmore robustly—currently, an error is returned from the chain-limited search. Confirm that partial results or re-requests are not necessary in edge cases.
305-332: Helper functionbuild_masternode_listsThe approach of filtering Masternodes by type (e.g.,
Evo) is clear, resulting in distinct sets. If the primary difference is potential upgrade or new masternode types, consider using a configuration-based approach or an enumeration check to future-proof the code.packages/rs-drive-abci/src/abci/app/mod.rs (2)
26-30: Add documentation for the SnapshotManagerApplication trait.The trait lacks comprehensive documentation explaining its purpose, responsibilities, and usage patterns. Consider adding documentation that describes:
- The purpose of snapshot management
- When and how this trait should be implemented
- Any invariants that implementations must maintain
50-57: Add documentation for the SnapshotFetchingApplication trait.The trait would benefit from detailed documentation explaining:
- The relationship between snapshot fetching and platform operations
- The lifecycle of a snapshot fetching session
- Thread safety considerations for the RwLock
packages/rs-drive-abci/src/abci/app/consensus.rs (2)
189-208: Add error handling documentation for snapshot operations.The new ABCI handlers (offer_snapshot, apply_snapshot_chunk, finalize_snapshot) would benefit from documentation describing:
- Expected error conditions
- Recovery mechanisms
- Impact on the application state
72-74: Consider adding debug assertions for RwLock operations.The snapshot_fetching_session RwLock operations could benefit from debug assertions to catch potential deadlocks or invalid states early in development.
fn snapshot_fetching_session(&self) -> &RwLock<Option<SnapshotFetchingSession<'p>>> { + debug_assert!(!self.snapshot_fetching_session.is_poisoned(), "snapshot_fetching_session RwLock is poisoned"); &self.snapshot_fetching_session }
📜 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 (8)
packages/dapi-grpc/Cargo.toml(1 hunks)packages/rs-drive-abci/Cargo.toml(1 hunks)packages/rs-drive-abci/src/abci/app/check_tx.rs(5 hunks)packages/rs-drive-abci/src/abci/app/consensus.rs(3 hunks)packages/rs-drive-abci/src/abci/app/mod.rs(3 hunks)packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/mod.rs(1 hunks)packages/rs-drive-proof-verifier/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive-abci/src/abci/handler/mod.rs
🧰 Additional context used
📓 Learnings (1)
packages/rs-drive-abci/Cargo.toml (1)
Learnt from: shumkov
PR: dashpay/platform#2375
File: packages/rs-drive-abci/Cargo.toml:61-63
Timestamp: 2024-12-05T09:29:38.918Z
Learning: In the `drive-abci` package, avoid adding unused dependencies like `hashbrown` to `Cargo.toml`. The team relies on CI to detect dependency version issues.
🔇 Additional comments (8)
packages/rs-drive-abci/src/abci/app/check_tx.rs (4)
1-1: Add missing references toSnapshotManagerApplicationandSnapshotManagerThe newly added imports for snapshot management are consistent with the broader PR objectives of supporting state synchronization. Good job importing them in a consolidated manner to maintain clarity.
Also applies to: 13-13
54-62: Snapshot Manager instantiation in constructorInitializing the
SnapshotManagerwith relevant config parameters is appropriate. Double-check that thecheckpoints_pathandsnapshots_frequencyvalues comply with the default or user-defined environment paths.
125-132: Implementation ofload_snapshot_chunkSimilar to
list_snapshots, the method references a handler. Validate large chunk sizes or potential DoS vectors by applying sensible rate limits and verifying chunk size constraints.
[security_consideration, verify]
39-46: ImplementingSnapshotManagerApplicationtraitThis trait implementation provides a straightforward reference to the internal
snapshot_manager. Just confirm that no concurrency issues arise when snapshot management is invoked simultaneously from other ABCI applications.To further verify concurrency safety, consider reviewing potential shared mutable state. If needed, use the script below to locate all references:
✅ Verification successful
Thread-safe snapshot manager implementation confirmed
The implementation is concurrency-safe because:
- It provides only immutable access through
&SnapshotManager- The type bounds
Send + Syncensure thread-safety- Consistent with other ABCI applications that all use the same immutable access pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash ast-grep --pattern $'snapshot_manager($_)'Length of output: 43
Script:
#!/bin/bash # Search for SnapshotManager struct definition and usage echo "=== SnapshotManager struct and trait definitions ===" rg "struct SnapshotManager|trait SnapshotManagerApplication" -A 5 echo -e "\n=== SnapshotManagerApplication implementations ===" rg "impl.*SnapshotManagerApplication" -A 5 echo -e "\n=== Mutable snapshot_manager access ===" rg "mut.*snapshot_manager|snapshot_manager.*mut" -A 2Length of output: 2760
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (1)
1-36: New file imports for snapshot finalizationThe high volume of imports suggests that this file handles complex logic. It’s acceptable given the scope, but keep an eye on the file size to maintain readability.
packages/rs-drive-proof-verifier/Cargo.toml (1)
37-38: Updatedtenderdash-abcidependency referencing a specific commitPinning to a specific commit ensures stable builds but may require frequent updates if ongoing changes occur upstream. Regularly re-check that this commit remains valid, or consider referencing a tagged release for simpler maintainability.
packages/dapi-grpc/Cargo.toml (1)
45-47: Verify commit hash and document dependency pinning.The dependency is pinned to a specific commit hash. While this ensures reproducible builds, it's important to:
- Verify that this commit hash is the intended version
- Document why this specific commit is required
packages/rs-drive-abci/Cargo.toml (1)
52-54: LGTM!The dependency update is consistent with the changes in dapi-grpc and follows team preferences for dependency management.
| let snapshot_manager = SnapshotManager::new( | ||
| platform | ||
| .config | ||
| .state_sync_config | ||
| .checkpoints_path.clone(), | ||
| platform.config.state_sync_config.max_num_snapshots, | ||
| platform.config.state_sync_config.snapshots_frequency, | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
Add parameter validation in SnapshotManager::new
The current implementation lacks validation for number_stored_snapshots and freq parameters, which could lead to runtime issues:
packages/rs-drive-abci/src/platform_types/snapshot/mod.rs: Add checks innew()to ensure:number_stored_snapshotsis greater than 0freqis within reasonable bounds (e.g., positive and not too frequent)
🔗 Analysis chain
Handle potential errors in SnapshotManager initialization.
The initialization of SnapshotManager should include error handling for invalid configurations:
- Validate max_num_snapshots is positive
- Ensure snapshots_frequency is within reasonable bounds
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these validations exist in SnapshotManager::new
ast-grep --pattern 'impl SnapshotManager {
$$$
pub fn new($_, $_, $_) {
$$$
}
$$$
}'
Length of output: 98
Script:
#!/bin/bash
# Find SnapshotManager implementation
rg -p "impl SnapshotManager" -A 20
Length of output: 897
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (4)
50-50: Enhance error messages with more context.The error messages could be more descriptive to help with debugging. Consider including the actual values or additional context.
- .ok_or(Error::Abci(AbciError::BadRequest("Empty Snapshot Block".to_string())))?; + .ok_or(Error::Abci(AbciError::BadRequest("Snapshot Block is missing in the request".to_string())))?; - .ok_or(Error::Abci(AbciError::BadRequest("Empty Snapshot Signed Header".to_string())))?; + .ok_or(Error::Abci(AbciError::BadRequest("Signed Header is missing in the Snapshot Block".to_string())))?; - .ok_or(Error::Abci(AbciError::BadRequest("Empty Snapshot Header".to_string())))?; + .ok_or(Error::Abci(AbciError::BadRequest("Header is missing in the Snapshot Signed Header".to_string())))?;Also applies to: 55-55, 60-60
62-144: Refactor repeated byte array validation pattern.There are multiple instances of similar byte array size validation and copying. Consider extracting this pattern into a helper function to reduce code duplication and improve maintainability.
fn validate_and_copy_bytes<const N: usize>( input: &[u8], field_name: &str, ) -> Result<[u8; N], Error> { if input.len() != N { return Err(Error::Abci(AbciError::BadRequestDataSize( format!("Invalid {} Size: expected {}, got {}", field_name, N, input.len()) ))); } let mut output = [0u8; N]; output.copy_from_slice(&input[..N]); Ok(output) }Usage example:
- if snapshot_header.proposer_pro_tx_hash.len() != 32 { - return Err(Error::Abci(AbciError::BadRequestDataSize("Invalid Proposer Tx Hash Size".to_string()))); - } - let mut snapshot_proposer_pro_tx_hash_32 = [0u8; 32]; - snapshot_proposer_pro_tx_hash_32.copy_from_slice(&snapshot_header.proposer_pro_tx_hash[..32]); + let snapshot_proposer_pro_tx_hash_32 = validate_and_copy_bytes::<32>( + &snapshot_header.proposer_pro_tx_hash, + "Proposer Tx Hash" + )?;
294-301: Simplify validator set sorting logic.The sorting logic is complex and could be simplified for better readability. Consider extracting the comparison logic into a dedicated function.
impl ValidatorSet { fn compare_by_height_and_hash(&self, other: &Self) -> std::cmp::Ordering { let height_cmp = other.core_height().cmp(&self.core_height()); if height_cmp == std::cmp::Ordering::Equal { other.quorum_hash() .cmp(self.quorum_hash()) .then_with(|| other.core_height().cmp(&self.core_height())) } else { height_cmp } } }Usage:
- .sort_by(|_, quorum_a, _, quorum_b| { - let primary_comparison = quorum_b.core_height().cmp(&quorum_a.core_height()); - if primary_comparison == std::cmp::Ordering::Equal { - quorum_b - .quorum_hash() - .cmp(quorum_a.quorum_hash()) - .then_with(|| quorum_b.core_height().cmp(&quorum_a.core_height())) - } else { - primary_comparison - } - }); + .sort_by(|_, quorum_a, _, quorum_b| quorum_a.compare_by_height_and_hash(quorum_b));Also applies to: 319-330
349-351: Add documentation for the build_masternode_lists function.Consider adding documentation to explain the purpose of the function, its parameters, and return values.
+/// Builds two masternode lists from a masternode list diff: +/// - A full list containing all masternodes +/// - A high-priority masternode (HPMN) list containing only Evo masternodes +/// +/// # Arguments +/// * `mn_list_diff` - The masternode list diff containing added and removed masternodes +/// +/// # Returns +/// A tuple containing: +/// - The full masternode list as a BTreeMap +/// - The HPMN list as a BTreeMap fn build_masternode_lists( mn_list_diff: &MasternodeListDiff, ) -> Result<(BTreeMap<ProTxHash, MasternodeListItem>, BTreeMap<ProTxHash, MasternodeListItem>), Error> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/info.rs(1 hunks)
🔇 Additional comments (4)
packages/rs-drive-abci/src/abci/handler/info.rs (1)
25-29: Verify that no sensitive or large data is logged at the info levelLogging the entire
platform_stateatinfolevel may expose sensitive information or generate overly large logs. Consider verifying that no private user data is contained inplatform_stateor switching to a more appropriate logging level (e.g.,debugortrace) to avoid potential data leakage and reduce log verbosity.packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (3)
62-67: Remove duplicate validation of proposer_pro_tx_hash length.The validation of
proposer_pro_tx_hashlength is duplicated.Also applies to: 93-98
171-174: Fix incorrect header access in genesis block validation.The code is accessing the snapshot signed header instead of the genesis signed header.
338-343: Handle transaction commit failure.The transaction commit result is being ignored with
let _, which could hide potential errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/rs-drive-abci/src/abci/handler/finalize_block.rs (2)
103-103: Remove unnecessary.clone().Since
block_heightis likely a numeric type implementing theCopytrait, the.clone()call is redundant and can be removed.- .store(block_height.clone(), Ordering::Relaxed); + .store(block_height, Ordering::Relaxed);
105-105: Remove unnecessary parentheses in if condition.The parentheses around the condition are not needed in Rust.
- if (app.platform().config.state_sync_config.snapshots_enabled) { + if app.platform().config.state_sync_config.snapshots_enabled {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rs-drive-abci/src/abci/handler/finalize_block.rs(3 hunks)packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/info.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs
- packages/rs-drive-abci/src/abci/handler/info.rs
🔇 Additional comments (2)
packages/rs-drive-abci/src/abci/handler/finalize_block.rs (2)
1-4: LGTM! Clean trait bound addition.The addition of
SnapshotManagerApplicationtrait bound is well-structured and aligns with the state sync feature objectives.Also applies to: 20-23
120-124: LGTM! Well-structured logging.The logging implementation is clean and informative, using appropriate structured fields and log level.
| .create_snapshot(&app.platform().drive.grove, block_height as i64) | ||
| .map_err(|e| { | ||
| Error::Execution(ExecutionError::CorruptedDriveResponse(format!( | ||
| "Unable to create snapshot:{}", | ||
| e | ||
| ))) | ||
| })?; |
There was a problem hiding this comment.
💡 Codebase verification
Implement suggested block height conversion safety and error message improvements
The suggested changes are valid and necessary:
- The codebase consistently uses
u64for block heights, making thei64conversion potentially unsafe - The improved error messages follow the codebase's pattern of including specific context
🔗 Analysis chain
Verify block height conversion and enhance error handling.
A few concerns with the snapshot creation:
- The conversion of
block_heighttoi64could potentially overflow for very large block heights - The error message could be more specific about what went wrong during snapshot creation
Consider adding overflow checking and improving the error message:
- .create_snapshot(&app.platform().drive.grove, block_height as i64)
+ .create_snapshot(&app.platform().drive.grove, i64::try_from(block_height).map_err(|e| {
+ Error::Execution(ExecutionError::CorruptedDriveResponse(
+ format!("Block height {} too large for snapshot: {}", block_height, e)
+ ))
+ })?)
.map_err(|e| {
Error::Execution(ExecutionError::CorruptedDriveResponse(format!(
- "Unable to create snapshot:{}",
+ "Failed to create snapshot at height {}: {}",
+ block_height,
e
)))
})?;🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find the type of block_height and surrounding context
rg "block_height" "packages/rs-drive-abci/src" -A 2 -B 2
# Look for create_snapshot usage
ast-grep --pattern 'create_snapshot($$$)'
# Search for similar error handling patterns with CorruptedDriveResponse
rg "CorruptedDriveResponse" -A 2 -B 2
Length of output: 65807
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/rs-drive-abci/src/abci/app/check_tx.rs (1)
54-58:⚠️ Potential issueHandle potential invalid paths in SnapshotManager initialization.
The
checkpoints_pathis used directly without validation.packages/rs-drive-abci/src/abci/app/full.rs (1)
39-43:⚠️ Potential issueAdd path validation for checkpoints directory.
The initialization of
SnapshotManagershould validate that the checkpoints directory exists and is writable to prevent runtime errors.Apply this diff to validate the path:
- let snapshot_manager = SnapshotManager::new( - platform.config.state_sync_config.checkpoints_path.clone(), - platform.config.state_sync_config.max_num_snapshots, - platform.config.state_sync_config.snapshots_frequency, - ); + let snapshot_manager = { + let path = platform.config.state_sync_config.checkpoints_path.clone(); + std::fs::create_dir_all(&path).map_err(|e| { + Error::InitializationError(format!( + "Failed to create checkpoints directory: {}", + e + )) + })?; + SnapshotManager::new( + path, + platform.config.state_sync_config.max_num_snapshots, + platform.config.state_sync_config.snapshots_frequency, + ) + };packages/rs-drive-abci/tests/strategy_tests/state_sync.rs (2)
71-171:⚠️ Potential issueAdd cleanup after test.
The test creates temporary directories but doesn't clean them up after completion.
Add cleanup using a Drop implementation:
+ struct Cleanup { + base_dir: PathBuf, + } + + impl Drop for Cleanup { + fn drop(&mut self) { + if let Err(e) = remove_dir(&self.base_dir) { + eprintln!("Failed to cleanup test directories: {}", e); + } + } + } + #[test] fn run_state_sync() { let platform_version = PlatformVersion::latest(); + let _cleanup = Cleanup { + base_dir: base_test_directory.clone(), + };
279-303:⚠️ Potential issueAdd explicit assertions for state sync verification.
The test only prints the source and target app hashes without explicitly verifying they match.
Add proper assertions:
+ let source_hash = source_outcome + .abci_app + .platform + .drive + .grove + .root_hash(None, &PlatformVersion::latest().drive.grove_version) + .unwrap() + .unwrap(); + + let target_hash = target_abci_app + .platform() + .drive + .grove + .root_hash(None, &PlatformVersion::latest().drive.grove_version) + .unwrap() + .unwrap(); + + assert_eq!( + source_hash, + target_hash, + "Source and target app hashes must match after state sync" + );packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (2)
72-78:⚠️ Potential issueRemove duplicate validation of proposer_pro_tx_hash.
The validation of
proposer_pro_tx_hashlength is duplicated.Remove the duplication:
- if snapshot_header.proposer_pro_tx_hash.len() != 32 { - return Err(Error::Abci(AbciError::BadRequestDataSize( - "Invalid Snapshot Header Proposer ProTx Hash Size".to_string(), - ))); - } - let mut snapshot_header_proposer_pro_tx_hash_32 = [0u8; 32]; - snapshot_header_proposer_pro_tx_hash_32 - .copy_from_slice(&snapshot_header.proposer_pro_tx_hash[..32]);Also applies to: 118-125
366-366:⚠️ Potential issueHandle transaction commit failure.
The transaction commit result is being ignored with
let _.Handle the commit result:
- let _ = app.platform().drive.grove.commit_transaction(tx); + app.platform().drive.grove.commit_transaction(tx)?;
🧹 Nitpick comments (8)
packages/rs-drive-abci/src/abci/handler/list_snapshots.rs (3)
7-7: Remove unused import.The
GroveDbimport is not used in the code. Consider removing it to keep the imports clean.-use drive::grovedb::GroveDb;
19-20: Remove redundant logging.The function uses both
println!andtracing::trace!for the same message. Consider using onlytracing::trace!for consistent logging throughout the application.- println!("[state_sync] api list_snapshots called"); tracing::trace!("[state_sync] api list_snapshots called");
24-29: Enhance error message with context.The error message could be more specific about what failed during snapshot retrieval.
.map_err(|e| { AbciError::StateSyncInternalError(format!( - "list_snapshots unable to get snapshots: {}", + "Failed to retrieve snapshots from snapshot manager: {}. Please check snapshot storage configuration and permissions.", e )) })?;packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs (1)
19-36: Consider enhancing error messages with more context.The error messages could be more descriptive by including the height and chunk ID in the error message.
Apply this diff to improve error messages:
.map_err(|_| { AbciError::StateSyncInternalError( - "load_snapshot_chunk failed: error matched snapshot".to_string(), + format!("load_snapshot_chunk failed: error matching snapshot at height {}", request.height), ) })? .ok_or_else(|| { AbciError::StateSyncInternalError( - "load_snapshot_chunk failed: empty matched snapshot".to_string(), + format!("load_snapshot_chunk failed: no snapshot found at height {}", request.height), ) })?;packages/rs-drive-abci/src/abci/app/check_tx.rs (1)
117-133: Consider adding request validation.The gRPC endpoints directly pass the requests to handlers without validation.
Apply this diff to add basic request validation:
async fn list_snapshots( &self, request: tonic::Request<proto::RequestListSnapshots>, ) -> Result<tonic::Response<proto::ResponseListSnapshots>, tonic::Status> { + let request = request.into_inner(); + if request.pagination.as_ref().map_or(false, |p| p.limit > 1000) { + return Err(tonic::Status::invalid_argument("pagination limit too large")); + } - handler::list_snapshots(self, request.into_inner()) + handler::list_snapshots(self, request) .map(tonic::Response::new) .map_err(|e| tonic::Status::internal(format!("list_snapshots failed: {}", e))) }packages/rs-drive-abci/src/abci/app/consensus.rs (1)
32-35: Consider documenting snapshot-related fields.The new fields would benefit from more detailed documentation explaining their purpose and lifecycle.
Apply this diff to improve documentation:
- /// The State sync session + /// The active state sync session for fetching snapshots from peers. + /// This is None when no sync is in progress. snapshot_fetching_session: RwLock<Option<SnapshotFetchingSession<'p>>>, - /// The snapshot manager + /// Manages creation, storage, and retrieval of local snapshots. + /// Handles snapshot rotation based on configured frequency and max count. snapshot_manager: SnapshotManager,packages/rs-drive-abci/src/abci/app/full.rs (1)
30-33: Add comprehensive documentation for new fields and consider encapsulation.The new fields would benefit from detailed documentation explaining their purpose, lifecycle, and thread-safety considerations. Consider making the fields private with public accessor methods for better encapsulation.
- /// The State sync session - pub snapshot_fetching_session: RwLock<Option<SnapshotFetchingSession<'a>>>, - /// The snapshot manager - pub snapshot_manager: SnapshotManager, + /// The State sync session that manages the fetching of snapshot chunks during state synchronization. + /// Protected by RwLock to ensure thread-safe access across ABCI operations. + snapshot_fetching_session: RwLock<Option<SnapshotFetchingSession<'a>>>, + /// The snapshot manager responsible for creating, storing, and managing platform state snapshots. + /// Handles snapshot operations based on configured frequency and retention policies. + snapshot_manager: SnapshotManager,packages/rs-drive-abci/tests/strategy_tests/state_sync.rs (1)
172-278: Add progress tracking and error recovery.The state sync execution would benefit from better progress tracking and error recovery mechanisms.
Consider:
- Adding a progress callback for monitoring sync status
- Implementing retry logic for failed chunk transfers
- Adding checkpoints to resume interrupted syncs
Example implementation:
struct SyncProgress { total_chunks: usize, processed_chunks: usize, total_ops: usize, processed_ops: usize, } impl SyncProgress { fn update(&mut self, chunk_size: usize) { self.processed_chunks += 1; self.processed_ops += chunk_size; } }
📜 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 (13)
packages/dapi-grpc/Cargo.toml(1 hunks)packages/rs-drive-abci/Cargo.toml(1 hunks)packages/rs-drive-abci/src/abci/app/check_tx.rs(4 hunks)packages/rs-drive-abci/src/abci/app/consensus.rs(4 hunks)packages/rs-drive-abci/src/abci/app/full.rs(4 hunks)packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/list_snapshots.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs(1 hunks)packages/rs-drive-abci/src/abci/handler/mod.rs(1 hunks)packages/rs-drive-abci/src/platform_types/snapshot/mod.rs(1 hunks)packages/rs-drive-abci/tests/strategy_tests/state_sync.rs(1 hunks)packages/rs-drive-proof-verifier/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-drive-proof-verifier/Cargo.toml
- packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs
- packages/rs-drive-abci/Cargo.toml
- packages/dapi-grpc/Cargo.toml
- packages/rs-drive-abci/src/platform_types/snapshot/mod.rs
🔇 Additional comments (10)
packages/rs-drive-abci/src/abci/handler/list_snapshots.rs (1)
31-47: LGTM! Clean and efficient snapshot processing.The snapshot processing logic is well-implemented:
- Efficient conversion using a dedicated closure
- Proper filtering of non-existent checkpoints
- Clean use of iterator methods
packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs (2)
11-18: LGTM! Well-structured function signature with appropriate trait bounds.The function is properly generic over application types with clear trait bounds for
SnapshotManagerApplicationandPlatformApplication.
43-49: Verify version compatibility.The code uses
PlatformVersion::latest()for grove version, but we should ensure compatibility between the requested version and the platform's grove version.packages/rs-drive-abci/src/abci/handler/mod.rs (1)
38-49: LGTM! Well-organized module structure.The new snapshot-related modules and exports are properly organized and follow the existing pattern.
Also applies to: 54-64
packages/rs-drive-abci/src/abci/app/consensus.rs (2)
186-205: LGTM! Well-structured ABCI endpoint implementations.The snapshot-related ABCI endpoints are properly implemented with error handling.
68-76: Add error handling for platform access.The
platformmethod could potentially be called after the platform is dropped due to lifetime issues.packages/rs-drive-abci/src/abci/app/full.rs (2)
60-74: LGTM! Clean trait implementations.The trait implementations are simple, focused, and provide clear access to the snapshot management components.
185-211: LGTM! Consistent ABCI method implementations.The new snapshot-related ABCI methods follow the established pattern of delegating to handlers and mapping errors consistently.
packages/rs-drive-abci/tests/strategy_tests/state_sync.rs (1)
30-69: LGTM! Clean utility functions with proper path handling.The utility functions are well-implemented with proper path resolution and error handling.
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (1)
373-539: LGTM! Well-structured helper functions.The helper functions are well-implemented with clear responsibilities, proper error handling, and type safety.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (1)
625-625: Address the TODO comment about quorum health.The comment suggests implementing quorum health checks for rotation decisions.
Would you like me to help implement the quorum health check functionality or create an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs(1 hunks)
🔇 Additional comments (4)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (4)
1-40: LGTM!The imports are well-organized and provide all necessary dependencies for the snapshot finalization functionality.
381-547: LGTM!The helper functions are well-implemented with proper error handling and type safety.
236-242:⚠️ Potential issueFix incorrect header access in genesis block validation.
The code is accessing the snapshot signed header instead of the genesis signed header.
Apply this diff to fix the incorrect header access:
- snapshot_signed_header + genesis_signed_header .header .as_ref() .ok_or(Error::Abci(AbciError::BadRequest( "Empty Genesis Header".to_string(), )))?;Likely invalid or redundant comment.
374-374:⚠️ Potential issueHandle transaction commit failure.
The transaction commit result is being ignored with
let _, which could hide potential errors.Apply this diff to handle the commit result:
- let _ = app.platform().drive.grove.commit_transaction(tx); + app.platform().drive.grove.commit_transaction(tx)?;Likely invalid or redundant comment.
| if snapshot_header.proposer_pro_tx_hash.len() != 32 { | ||
| return Err(Error::Abci(AbciError::BadRequestDataSize( | ||
| "Invalid Proposer Tx Hash Size".to_string(), | ||
| ))); | ||
| } | ||
| let mut snapshot_proposer_pro_tx_hash_32 = [0u8; 32]; | ||
| snapshot_proposer_pro_tx_hash_32.copy_from_slice(&snapshot_header.proposer_pro_tx_hash[..32]); |
There was a problem hiding this comment.
Remove duplicate validation of proposer_pro_tx_hash.
The validation of proposer_pro_tx_hash length and the slice operation is duplicated.
Apply this diff to remove the duplication:
- if snapshot_header.proposer_pro_tx_hash.len() != 32 {
- return Err(Error::Abci(AbciError::BadRequestDataSize(
- "Invalid Snapshot Header Proposer ProTx Hash Size".to_string(),
- )));
- }
- let mut snapshot_header_proposer_pro_tx_hash_32 = [0u8; 32];
- snapshot_header_proposer_pro_tx_hash_32
- .copy_from_slice(&snapshot_header.proposer_pro_tx_hash[..32]);Also applies to: 121-128
| )?; | ||
|
|
||
| let current_protocol_version_in_consensus = snapshot_header_version.app as u32; | ||
| let next_epoch_protocol_version = snapshot_header.proposed_app_version as u32; | ||
| let current_validator_set_quorum_hash = | ||
| QuorumHash::from_byte_array(snapshot_commit_quorum_hash_32); | ||
|
|
||
| let mut platform_state = PlatformState::V0(PlatformStateV0 { | ||
| genesis_block_info: Some(genesis_block_info), | ||
| last_committed_block_info: Some(ExtendedBlockInfo::from(ExtendedBlockInfoV0 { | ||
| basic_info: BlockInfo { | ||
| time_ms: snapshot_block_time, | ||
| height: snapshot_header.height as u64, | ||
| core_height: snapshot_header.core_chain_locked_height, | ||
| epoch: Epoch::new(current_epoch_info.current_epoch_index)?, | ||
| }, | ||
| app_hash: snapshot_header_app_hash_32, | ||
| quorum_hash: snapshot_commit_quorum_hash_32, | ||
| block_id_hash: snapshot_header_last_block_id_hash_32, | ||
| proposer_pro_tx_hash: snapshot_header_proposer_pro_tx_hash_32, | ||
| signature: snapshot_commit_threshold_block_sig_96, | ||
| round: snapshot_commit.round as u32, | ||
| })), | ||
| current_protocol_version_in_consensus, | ||
| next_epoch_protocol_version, | ||
| current_validator_set_quorum_hash, | ||
| next_validator_set_quorum_hash: None, | ||
| patched_platform_version: None, | ||
| validator_sets: Default::default(), | ||
| chain_lock_validating_quorums: SignatureVerificationQuorumSet::from( | ||
| SignatureVerificationQuorumSet::new( | ||
| &config.chain_lock, | ||
| PlatformVersion::get(current_protocol_version_in_consensus)?, | ||
| )?, | ||
| ), | ||
| instant_lock_validating_quorums: SignatureVerificationQuorumSet::from( | ||
| SignatureVerificationQuorumSet::new( | ||
| &config.instant_lock, | ||
| PlatformVersion::get(current_protocol_version_in_consensus)?, | ||
| )?, | ||
| ), | ||
| full_masternode_list: BTreeMap::new(), | ||
| hpmn_masternode_list: BTreeMap::new(), | ||
| previous_fee_versions: Default::default(), | ||
| }); | ||
|
|
||
| build_masternode_lists( | ||
| app, | ||
| &mut platform_state, | ||
| snapshot_header.core_chain_locked_height, | ||
| )?; | ||
|
|
||
| let mut extended_quorum_list = app | ||
| .platform() | ||
| .core_rpc | ||
| .get_quorum_listextended(Some(snapshot_header.core_chain_locked_height))?; | ||
| build_quorum_verification_set( | ||
| app, | ||
| &extended_quorum_list, | ||
| QuorumSetType::ChainLock(config.chain_lock.quorum_type), | ||
| platform_state.chain_lock_validating_quorums_mut(), | ||
| )?; | ||
| build_quorum_verification_set( | ||
| app, | ||
| &extended_quorum_list, | ||
| QuorumSetType::InstantLock(config.instant_lock.quorum_type), | ||
| platform_state.instant_lock_validating_quorums_mut(), | ||
| )?; | ||
|
|
||
| build_validators_list( | ||
| app, | ||
| &mut platform_state, | ||
| &mut extended_quorum_list, | ||
| config.validator_set.quorum_type, | ||
| )?; | ||
|
|
||
| build_next_validator_set_quorum_hash( | ||
| snapshot_header_proposer_pro_tx_hash_32, | ||
| &mut platform_state, | ||
| )?; | ||
|
|
||
| let block_height = platform_state.last_committed_block_height(); | ||
|
|
||
| tracing::info!( | ||
| block_height, | ||
| platform_state = ?platform_state, | ||
| "state_finalize_snapshot", | ||
| ); | ||
|
|
||
| let tx = app.platform().drive.grove.start_transaction(); | ||
|
|
||
| app.platform() | ||
| .store_platform_state(&platform_state, Some(&tx), &PlatformVersion::latest())?; | ||
|
|
||
| let _ = app.platform().drive.grove.commit_transaction(tx); | ||
|
|
||
| app.platform().state.store(Arc::new(platform_state)); | ||
|
|
||
| Ok(Default::default()) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Critical: Missing test coverage for snapshot finalization
The finalize_snapshot function lacks test coverage despite being a critical component that handles:
- Platform state initialization
- Validator set management
- Quorum verification
- Transaction processing
This poses significant risks as the function:
- Manages blockchain state finalization
- Contains complex validation logic
- Handles security-critical operations
- Processes validator sets and quorums
Recommendation:
- Add comprehensive unit tests covering:
- Valid snapshot processing scenarios
- Error handling for malformed inputs
- Edge cases in validator set management
- Transaction processing verification
- Include integration tests for the complete snapshot finalization flow
🔗 Analysis chain
Verify snapshot functionality with test cases.
The snapshot finalization is a critical component. Please ensure comprehensive test coverage for various scenarios:
- Valid snapshot finalization
- Invalid header cases
- Various quorum rotation conditions
- Error cases in transaction handling
Let's verify the test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for snapshot functionality
# Search for snapshot-related tests
rg -A 5 "test.*snapshot" packages/rs-drive-abci/tests/
Length of output: 56
Script:
#!/bin/bash
# Broader search for snapshot and test-related code
# Search for test modules in the project
echo "=== Searching for test modules ==="
rg "#\[cfg\(test\)\]" packages/rs-drive-abci/src/
# Search for finalize_snapshot mentions in tests
echo -e "\n=== Searching for finalize_snapshot mentions ==="
rg "finalize_snapshot" packages/rs-drive-abci/src/ packages/rs-drive-abci/tests/
# Look for test files
echo -e "\n=== Looking for test files ==="
fd "test" packages/rs-drive-abci/
Length of output: 11352
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (3)
79-85:⚠️ Potential issueRemove duplicate validation of proposer_pro_tx_hash.
The validation of
proposer_pro_tx_hashlength and the slice operation is duplicated.Apply this diff to remove the duplication:
- if snapshot_header.proposer_pro_tx_hash.len() != 32 { - return Err(Error::Abci(AbciError::BadRequestDataSize( - "Invalid Proposer Tx Hash Size".to_string(), - ))); - } - let mut snapshot_proposer_pro_tx_hash_32 = [0u8; 32]; - snapshot_proposer_pro_tx_hash_32.copy_from_slice(&snapshot_header.proposer_pro_tx_hash[..32]);Also applies to: 125-132
240-246:⚠️ Potential issueFix incorrect header access in genesis block validation.
The code is accessing the snapshot signed header instead of the genesis signed header.
Apply this diff to fix the incorrect header access:
- snapshot_signed_header + genesis_signed_header
392-397:⚠️ Potential issueHandle transaction commit failure.
The transaction commit result is being ignored with
let _, which could hide potential errors.Apply this diff to handle the commit result:
- let _ = app.platform().drive.grove.commit_transaction(tx); + app.platform().drive.grove.commit_transaction(tx)?;
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (1)
442-492: Add debug logging for quorum verification set building.Consider adding debug logging to track the number of quorums processed and any key verification steps.
Add logging statements:
+ tracing::debug!( + "Building quorum verification set for type {:?} with {} quorums", + quorum_set_type, + quorums_list.len() + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs(1 hunks)
🔇 Additional comments (3)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (3)
404-440: LGTM! Clean and efficient implementation.The function correctly handles:
- Fetching masternode list differences
- Filtering HPMNs (High Performance Masternodes)
- Extending both full and HPMN lists
648-648: Address the TODO comment about quorum health checks.The comment indicates that quorum health checks might be needed for rotation decisions.
Would you like me to help design and implement the quorum health check functionality? This could include:
- Defining health metrics
- Implementing health checks
- Adding rotation logic based on health status
45-741: Add comprehensive test coverage for snapshot finalization.The snapshot finalization code is critical and complex, but lacks sufficient test coverage.
Key areas requiring test coverage:
- Snapshot block validation
- Genesis block validation
- Validator set rotation scenarios
- Error handling paths
- Transaction processing
Let's verify the current test coverage:
#!/bin/bash # Description: Check test coverage for snapshot functionality # Search for snapshot-related tests echo "=== Searching for snapshot tests ===" rg -A 5 "test.*snapshot" packages/rs-drive-abci/tests/ # Search for finalize_snapshot mentions in tests echo -e "\n=== Searching for finalize_snapshot mentions ===" rg "finalize_snapshot" packages/rs-drive-abci/tests/
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (4)
399-399:⚠️ Potential issueHandle transaction commit failure.
The transaction commit result is being ignored with
let _, which could hide potential errors.Apply this diff to handle the commit result:
- let _ = app.platform().drive.grove.commit_transaction(tx); + app.platform().drive.grove.commit_transaction(tx)?;
79-85:⚠️ Potential issueRemove duplicate validation of proposer_pro_tx_hash.
The validation of
proposer_pro_tx_hashlength and the slice operation is duplicated.Apply this diff to remove the duplication:
- if snapshot_header.proposer_pro_tx_hash.len() != 32 { - return Err(Error::Abci(AbciError::BadRequestDataSize( - "Invalid Proposer Tx Hash Size".to_string(), - ))); - } - let mut snapshot_proposer_pro_tx_hash_32 = [0u8; 32]; - snapshot_proposer_pro_tx_hash_32.copy_from_slice(&snapshot_header.proposer_pro_tx_hash[..32]);Also applies to: 125-132
240-246:⚠️ Potential issueFix incorrect header access in genesis block validation.
The code is accessing the snapshot signed header instead of the genesis signed header.
Apply this diff to fix the incorrect header access:
- let genesis_header = - snapshot_signed_header + let genesis_header = + genesis_signed_header .header .as_ref() .ok_or(Error::Abci(AbciError::BadRequest("Empty Genesis Header".to_string())))?;
45-404:⚠️ Potential issueCritical: Missing test coverage for snapshot finalization.
The
finalize_snapshotfunction lacks test coverage despite being a critical component that handles:
- Platform state initialization
- Validator set management
- Quorum verification
- Transaction processing
This poses significant risks as the function:
- Manages blockchain state finalization
- Contains complex validation logic
- Handles security-critical operations
- Processes validator sets and quorums
Recommendation:
- Add comprehensive unit tests covering:
- Valid snapshot processing scenarios
- Error handling for malformed inputs
- Edge cases in validator set management
- Transaction processing verification
- Include integration tests for the complete snapshot finalization flow
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs(1 hunks)
🔇 Additional comments (3)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (3)
406-442: LGTM! Well-structured masternode list building.The function correctly:
- Fetches masternode list diff from core RPC
- Filters HPMNs (High Performance Masternodes)
- Extends both full and HPMN masternode lists
444-494: LGTM! Robust quorum verification set building.The function demonstrates:
- Clear error handling for missing quorums
- Proper RPC interaction for quorum info
- Safe BLS public key conversion
650-650: Address the TODO comment about quorum health.The comment suggests implementing rotation based on quorum health, which could be important for system reliability.
Would you like me to help implement the quorum health check logic? This would involve:
- Defining health metrics
- Implementing health checks
- Adding rotation logic based on health status
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (3)
650-651: Consider implementing quorum health-based rotation.The TODO comment suggests an enhancement to perform quorum rotation based on health metrics. This could improve system reliability by proactively rotating away from unhealthy quorums.
Would you like me to help implement the quorum health check and rotation logic? I can:
- Define health metrics (e.g., response time, error rate)
- Implement health monitoring
- Add rotation triggers based on health thresholds
671-677: Extract magic number into a named constant.The quorum count threshold of 10 should be defined as a constant with a meaningful name for better maintainability and documentation.
+const MAX_QUORUM_COUNT_FOR_FULL_ROTATION: usize = 10; + let oldest_quorum_index_we_can_go_to = if count > 10 { // if we have a lot of quorums (like on testnet and mainnet) // we shouldn't start using the last ones as they could cycle out count - 2 } else { count };
1-742: Standardize error handling patterns.The file uses inconsistent error handling patterns. Consider:
- Using structured error types instead of string messages
- Adding error codes for better error tracking
- Including context in all error messages
- Standardizing error message format
Example of structured error handling:
#[derive(Debug, Error)] pub enum SnapshotError { #[error("Invalid header: {field} is empty")] EmptyHeader { field: &'static str }, #[error("Invalid size for {field}: expected {expected}, got {actual}")] InvalidSize { field: &'static str, expected: usize, actual: usize, }, // ... more error variants }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs(1 hunks)
🔇 Additional comments (4)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (4)
79-85: Remove duplicate validation of proposer_pro_tx_hash.The validation of
proposer_pro_tx_hashlength and the slice operation is duplicated.Also applies to: 125-132
240-246: Fix incorrect header access in genesis block validation.The code is accessing the snapshot signed header instead of the genesis signed header.
365-384: Uncomment and fix the validator hash verification.The commented-out code for validator hash verification is crucial for ensuring the integrity of the validator set. The code also has a bug where it creates an error but doesn't return it.
394-399: Handle transaction commit failure.The transaction commit result is being ignored with
let _, which could hide potential errors.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/dashmate/templates/platform/drive/tenderdash/config.toml.dotis excluded by!**/*.dot
📒 Files selected for processing (2)
Dockerfile(2 hunks)packages/dashmate/configs/defaults/getBaseConfigFactory.js(1 hunks)
🔇 Additional comments (3)
packages/dashmate/configs/defaults/getBaseConfigFactory.js (1)
312-312: Avoid using feature branch tags in production configsUsing a feature branch tag (
feat-statesync-integration) instead of a specific version or SHA digest is risky because:
- The image can be rebuilt with different code while keeping the same tag
- It may cause reproducibility issues across different environments
- It suggests this is temporary development code
Is this intended to be temporary for testing? If not, consider:
- Using a specific version tag (e.g.,
v1.2.3)- Using the image's SHA digest for guaranteed reproducibility
- Creating a separate development config for testing feature branches
Run this script to check the available tags and digests:
Dockerfile (2)
555-555: LGTM! Environment variable follows conventions.The
CHECKPOINTS_PATHenvironment variable is well-defined and follows the established path structure conventions.
565-572: Technical debt: Remove temporary checkpoint directory.The TODO comment indicates a temporary workaround with duplicate checkpoint directories. This should be addressed to avoid confusion and maintain a clean directory structure.
Let's verify the usage of both checkpoint paths:
✅ Verification successful
Confirmed: Remove redundant checkpoint directory
The codebase is configured to use
/var/lib/dash/rs-drive-abci/db-checkpointsvia theCHECKPOINTS_PATHenvironment variable, making the creation of/var/lib/dash-platform/data/checkpointsredundant. This technical debt should be addressed by:
- Removing the creation of
/var/lib/dash-platform/data/checkpointsdirectory in the Dockerfile- Removing the associated TODO comments
- Removing the
chowncommand for this directory🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to both checkpoint paths echo "Searching for references to checkpoint paths..." rg -i "checkpoints" --type rustLength of output: 2765
Script:
#!/bin/bash # Search for references to checkpoints in all files echo "Searching for references to checkpoints in all files..." rg -i "checkpoints" --type-list rg -i "checkpoints" -g '!*.rs'Length of output: 1048
| # TODO: remove /var/lib/dash-platform/data/checkpoints when drive-abci is fixed | ||
| RUN addgroup -g $USER_GID $USERNAME && \ | ||
| adduser -D -u $USER_UID -G $USERNAME -h /var/lib/dash/rs-drive-abci $USERNAME && \ | ||
| chown -R $USER_UID:$USER_GID /var/lib/dash/rs-drive-abci /var/log/dash | ||
| chown -R $USER_UID:$USER_GID /var/lib/dash/rs-drive-abci /var/log/dash /var/lib/dash-platform/data/checkpoints |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate checkpoint directory paths.
The ownership command includes a temporary checkpoint directory path (/var/lib/dash-platform/data/checkpoints). Once the TODO is addressed, this should be consolidated to use only the ${CHECKPOINTS_PATH} variable.
Apply this diff after the drive-abci fix is implemented:
- chown -R $USER_UID:$USER_GID /var/lib/dash/rs-drive-abci /var/log/dash /var/lib/dash-platform/data/checkpoints
+ chown -R $USER_UID:$USER_GID /var/lib/dash/rs-drive-abci /var/log/dash ${CHECKPOINTS_PATH}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (3)
.github/actions/docker/action.yaml(1 hunks).github/actions/sccache/action.yaml(1 hunks)Dockerfile(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Formatting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (4)
.github/actions/sccache/action.yaml (1)
37-37: Verify sccache version update compatibility and currency.The version update from 0.8.2 to 0.9.1 looks reasonable, but let's verify a few things:
✅ Verification successful
Version 0.9.1 is valid and current
The update to sccache v0.9.1 is appropriate as it's the latest stable release, containing important bug fixes and improvements including Rust coverage support fixes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify sccache version 0.9.1 exists, check for breaking changes, and validate if it's the latest version # Check if the version exists and get the latest version echo "Checking sccache releases..." RELEASES=$(curl -s https://api.github.com/repos/mozilla/sccache/releases) # Check if 0.9.1 exists echo "Verifying version 0.9.1..." VERSION_EXISTS=$(echo "$RELEASES" | jq -r '.[].tag_name' | grep -c "v0.9.1" || true) # Get latest version LATEST_VERSION=$(echo "$RELEASES" | jq -r '.[0].tag_name') # Get changelog between 0.8.2 and 0.9.1 echo "Checking for breaking changes..." CHANGELOG_URL="https://raw.githubusercontent.com/mozilla/sccache/v0.9.1/CHANGELOG.md" curl -s "$CHANGELOG_URL" | awk '/## 0\.9\.1/,/## 0\.8\.2/' echo "Version exists: $VERSION_EXISTS" echo "Latest version: $LATEST_VERSION"Length of output: 65668
.github/actions/docker/action.yaml (1)
195-195: Verify the impact of disabling sccache environment variables.Commenting out the sccache environment variables might impact build performance as Rust compilation results won't be cached. Please confirm if this change is intentional and if an alternative caching strategy is in place.
✅ Verification successful
Sccache environment variables were intentionally disabled with no negative impact
While sccache environment variables are commented out in the Docker build step, the build performance remains optimized through:
- Active Cargo registry caching via
actions/cache- Docker layer caching via BuildX
- Yarn dependency caching for JavaScript components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if sccache is still being used in the build process # Check if sccache is referenced in build logs gh run list --workflow=docker.yml --limit 2 --json databaseId --jq '.[].databaseId' | while read -r run_id; do echo "Checking run $run_id" gh run view "$run_id" --log | grep -i "sccache" doneLength of output: 199
Script:
#!/bin/bash # Find all workflow files and check their content for sccache and caching configurations echo "=== Workflow files ===" fd -e yml -e yaml . .github/workflows/ echo -e "\n=== Checking for sccache configuration ===" rg -l "sccache" .github/ echo -e "\n=== Checking for Rust caching configurations ===" rg -l "rust|cargo.*cache" .github/ echo -e "\n=== Examining the docker action file ===" cat .github/actions/docker/action.yamlLength of output: 8624
Dockerfile (2)
565-571: Consolidate checkpoint directory paths.The directory creation includes a temporary checkpoint directory path (
/var/lib/dash-platform/data/checkpoints). Once the TODO is addressed, this should be consolidated to use only the${CHECKPOINTS_PATH}variable.
555-555: LGTM! State sync directory structure is well-organized.The implementation of the checkpoints directory structure for ABCI state sync is clean and follows good practices:
- Environment variable for the checkpoints path is properly defined
- Directories are created with appropriate permissions
- Ownership is correctly set for the dash user
Also applies to: 565-571, 582-585
| @@ -147,7 +147,7 @@ ENV NODE_ENV=${NODE_ENV} | |||
| FROM deps-base AS deps-sccache | |||
|
|
|||
| # SCCACHE_VERSION must be the same as in github actions, to avoid cache incompatibility | |||
| ARG SCCHACHE_VERSION=0.8.2 | |||
| ARG SCCHACHE_VERSION=0.9.1 | |||
There was a problem hiding this comment.
Fix typo in variable name.
The variable name SCCHACHE_VERSION contains a typo and should be SCCACHE_VERSION.
Apply this diff to fix the typo:
-ARG SCCHACHE_VERSION=0.9.1
+ARG SCCACHE_VERSION=0.9.1📝 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.
| ARG SCCHACHE_VERSION=0.9.1 | |
| ARG SCCACHE_VERSION=0.9.1 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (1)
530-684: Document the complex validator set rotation logic.The validator set rotation logic is complex and would benefit from additional documentation explaining:
- The conditions that trigger rotation
- The selection criteria for the next validator set
- The fallback mechanisms when rotation fails
Consider adding detailed documentation for this complex logic to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (json-schema-compatibility-validator) / Tests
- GitHub Check: Rust packages (json-schema-compatibility-validator) / Linting
- GitHub Check: Rust packages (json-schema-compatibility-validator) / Formatting
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (4)
packages/rs-drive-abci/src/abci/handler/finalize_snapshot.rs (4)
79-85: Remove duplicate validation of proposer_pro_tx_hash.The validation of
proposer_pro_tx_hashlength and the slice operation is duplicated.Apply this diff to remove the duplication:
- if snapshot_header.proposer_pro_tx_hash.len() != 32 { - return Err(Error::Abci(AbciError::BadRequestDataSize( - "Invalid Snapshot Header Proposer ProTx Hash Size".to_string(), - ))); - } - let mut snapshot_header_proposer_pro_tx_hash_32 = [0u8; 32]; - snapshot_header_proposer_pro_tx_hash_32 - .copy_from_slice(&snapshot_header.proposer_pro_tx_hash[..32]);Also applies to: 125-132
355-355: Handle transaction commit failure.The transaction commit result is being ignored with
let _, which could hide potential errors.Apply this diff to handle the commit result:
- let _ = app.platform().drive.grove.commit_transaction(tx); + app.platform().drive.grove.commit_transaction(tx)?;
606-606: Address the TODO comment about quorum health rotation.The comment suggests implementing quorum health checks for rotation decisions. This could be important for system reliability.
Would you like me to help implement the quorum health check logic or create an issue to track this enhancement?
45-360: Add comprehensive test coverage for snapshot finalization.The
finalize_snapshotfunction handles critical operations including:
- Platform state initialization
- Validator set management
- Quorum verification
- Transaction processing
Run the following script to check test coverage:
#!/bin/bash # Description: Check test coverage for snapshot functionality # Search for snapshot-related tests echo "=== Searching for snapshot tests ===" rg -A 5 "test.*snapshot" packages/rs-drive-abci/tests/ # Search for finalize_snapshot mentions in tests echo -e "\n=== Searching for finalize_snapshot mentions ===" rg "finalize_snapshot" packages/rs-drive-abci/src/ packages/rs-drive-abci/tests/
This reverts commit dad9fc2.
…to feat/abci-state-sync
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/rs-drive-abci/src/platform_types/snapshot/mod.rs (4)
15-15: Remove commented-out import.The commented-out import
platform_version::version::PlatformVersionshould be removed as it's no longer needed and the correct import is already present on line 17.-//use platform_version::version::PlatformVersion;
37-37: Enhance documentation for the metadata field.The
metadatafield's purpose and expected content should be documented more clearly.- /// Metadata + /// Metadata associated with the snapshot. + /// This can include additional information such as: + /// - Application-specific data + /// - Configuration parameters + /// - Version information
52-57: Fix duplicate comments.The comments for fields are duplicated and unclear.
- /// Snapshot accepted + /// The accepted snapshot information pub snapshot: abci::Snapshot, - /// Snapshot accepted + /// The application hash for the accepted snapshot pub app_hash: Vec<u8>, - /// Snapshot accepted + /// State synchronization session information pub state_sync_info: Pin<Box<MultiStateSyncSession<'db>>>,
173-176: Enhance error handling for file system operations.The file system operation could fail for various reasons (permissions, concurrent access, etc.). The error message should include more context.
if Path::new(&snapshot.path).is_dir() { - fs::remove_dir_all(&snapshot.path) - .map_err(|e| Error::Drive(Drive(DriveError::Snapshot(e.to_string()))))?; + fs::remove_dir_all(&snapshot.path).map_err(|e| { + Error::Drive(Drive(DriveError::Snapshot(format!( + "Failed to remove snapshot directory {}: {}", + snapshot.path, e + )))) + })?; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive-abci/src/platform_types/snapshot/mod.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Formatting
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (2)
packages/rs-drive-abci/src/platform_types/snapshot/mod.rs (2)
87-100: Add parameter validation in constructor.The constructor should validate its parameters to prevent invalid states.
pub fn new(checkpoints_path: PathBuf, number_stored_snapshots: usize, freq: i64) -> Self { + if freq <= 0 { + tracing::error!("Frequency must be positive, got {}", freq); + } + if number_stored_snapshots == 0 { + tracing::error!("Number of stored snapshots must be greater than zero"); + } if let Err(e) = fs::create_dir_all(checkpoints_path.clone()) {
193-230: Improve test coverage.Add tests for error handling and edge cases.
Add tests for:
- Error handling (invalid paths, concurrent access)
- Edge cases (zero/negative freq, zero stored snapshots)
get_snapshot_at_heightfunctionalityExample test cases to add:
#[test] fn test_get_snapshot_at_height() { let grove_dir = tempfile::tempdir().unwrap(); let checkpoints_dir = tempfile::tempdir().unwrap(); let grove = GroveDb::open(grove_dir.path()).unwrap(); let manager = SnapshotManager::new(checkpoints_dir.path().to_str().unwrap().to_string().into(), 3, 1); // Create some snapshots for height in 1000..=1002 { manager.create_snapshot(&grove, height).unwrap(); } // Test existing height let snapshot = manager.get_snapshot_at_height(&grove, 1001).unwrap(); assert!(snapshot.is_some()); assert_eq!(snapshot.unwrap().height, 1001); // Test non-existent height let snapshot = manager.get_snapshot_at_height(&grove, 999).unwrap(); assert!(snapshot.is_none()); } #[test] fn test_invalid_parameters() { let checkpoints_dir = tempfile::tempdir().unwrap(); let manager = SnapshotManager::new( checkpoints_dir.path().to_str().unwrap().to_string().into(), 0, // Invalid number_stored_snapshots -1, // Invalid freq ); // Assert logs contain error messages }
| #[derive(Default, Clone)] | ||
| pub struct SnapshotManager { | ||
| freq: i64, | ||
| number_stored_snapshots: usize, | ||
| checkpoints_path: PathBuf, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove Default derive as it may lead to invalid states.
The SnapshotManager requires specific parameters to function correctly. Deriving Default could create instances with invalid values (e.g., zero frequency).
-#[derive(Default, Clone)]
+#[derive(Clone)]📝 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.
| #[derive(Default, Clone)] | |
| pub struct SnapshotManager { | |
| freq: i64, | |
| number_stored_snapshots: usize, | |
| checkpoints_path: PathBuf, | |
| } | |
| #[derive(Clone)] | |
| pub struct SnapshotManager { | |
| freq: i64, | |
| number_stored_snapshots: usize, | |
| checkpoints_path: PathBuf, | |
| } |
|
|
||
| const SNAPSHOT_KEY: &[u8] = b"snapshots"; | ||
|
|
||
| const CHUNK_SIZE_16MB: usize = 16 * 1024 * 1024; |
There was a problem hiding this comment.
💡 Codebase verification
CHUNK_SIZE_16MB constant is unused and should be removed
This constant is not referenced anywhere in the codebase and appears to be dead code. Consider removing it to maintain code cleanliness.
🔗 Analysis chain
Verify usage of CHUNK_SIZE_16MB constant.
The CHUNK_SIZE_16MB constant is defined but not used in this file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of CHUNK_SIZE_16MB across the codebase
rg "CHUNK_SIZE_16MB" --type rust
Length of output: 139
Script:
#!/bin/bash
# Show the entire snapshot module content
cat packages/rs-drive-abci/src/platform_types/snapshot/mod.rs
# Also search for the computed value in case it's used directly
rg "16777216" --type rust
Length of output: 7682
Issue being fixed or feature implemented
Implementation of state sync abci applications.
What was done?
SnapshotManagerand its related configuration inPlatformConfigStateSourceAbciApplicationfor all the replication reading operations (for the source nodes)ConsensusAbciApplicationFullAbciApplicationfor testsHow Has This Been Tested?
New strategy test. Comparing root hash, verifying grovedb after state sync is complete
Breaking Changes
to be determined
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates
Infrastructure
Compatibility