feat(drive-abci): state sync - faster sync of new nodes #2486
Conversation
|
Warning Rate limit exceeded@lklimek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (31)
WalkthroughThis update introduces comprehensive enhancements centered on state synchronization, snapshot management, and versioned storage for platform and block state within the system. New modules and traits are added to support the serialization, deserialization, fetching, and storing of reduced platform state and last block information. Snapshot management is integrated into the ABCI application, including snapshot creation, listing, loading, and application during state sync. Configuration structures and environment variables are extended to support these new features. Versioning structures are updated throughout the platform to accommodate the new methods and structures, ensuring compatibility and extensibility for state sync and snapshot operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Node
participant ABCI_App
participant SnapshotManager
participant GroveDB
Node->>ABCI_App: Request State Sync (OfferSnapshot)
ABCI_App->>SnapshotManager: Start Snapshot Fetching Session
Note right of SnapshotManager: Wipe DB, prepare session
Node->>ABCI_App: LoadSnapshotChunk (chunk N)
ABCI_App->>SnapshotManager: Apply Chunk to Session
SnapshotManager->>GroveDB: Store Chunk Data
SnapshotManager-->>ABCI_App: Chunk Applied
Node->>ABCI_App: Finalize Block
ABCI_App->>SnapshotManager: Create Snapshot (if enabled)
SnapshotManager->>GroveDB: Save Snapshot Metadata and Data
SnapshotManager-->>ABCI_App: Snapshot Created
Node->>ABCI_App: ListSnapshots
ABCI_App->>SnapshotManager: Get Snapshots
SnapshotManager-->>ABCI_App: Return Snapshots List
ABCI_App-->>Node: Snapshots List
Node->>ABCI_App: LoadSnapshotChunk (by height/id)
ABCI_App->>SnapshotManager: Get Snapshot at Height
SnapshotManager->>GroveDB: Fetch Chunk Data
SnapshotManager-->>ABCI_App: Chunk Data
ABCI_App-->>Node: Return Chunk
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 6
🧹 Nitpick comments (47)
packages/rs-drive/src/error/drive.rs (1)
191-194: Add a more descriptive docstring for theSnapshoterror variant.The current docstring "Error todo" is temporary placeholder text. Consider replacing it with a detailed description explaining when and why this error variant would be used.
Also, consider including the string message in the error display format for better debugging:
-/// Error todo -#[error("snapshot error")] +/// Error related to snapshot operations such as creating, listing, or loading snapshots +#[error("snapshot error: {0}")] Snapshot(String),packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs (1)
165-174: Added handler for version 1 block proposal executionThe code properly extends the version handling by adding case
1to callrun_block_proposal_v1with the same parameters as the v0 implementation. This follows good versioning practices by maintaining backward compatibility while adding new functionality.However, the error message in the
UnknownVersionMismatchhasn't been updated to include version 1 in theknown_versionslist.Update the error message to include both supported versions:
version => Err(Error::Execution(ExecutionError::UnknownVersionMismatch { method: "run_block_proposal".to_string(), - known_versions: vec![0], + known_versions: vec![0, 1], received: version, })),packages/rs-drive-abci/src/abci/handler/info.rs (1)
25-31: Enhanced logging for block height and platform stateAdded detailed tracing that logs both block height and platform state, which improves observability. This will be helpful for debugging state sync issues.
Note that the block height is already being extracted on line 33, so this introduces a duplicate calculation of
last_committed_block_height(). While this doesn't affect functionality, it's a minor efficiency concern.Consider refactoring to avoid the duplicate call:
let platform_state = app.platform().state.load(); - let block_height = platform_state.last_committed_block_height(); + let block_height = platform_state.last_committed_block_height() as i64; tracing::trace!( block_height, platform_state = ?platform_state, "state_info" ); - let last_block_height = platform_state.last_committed_block_height() as i64; + let last_block_height = block_height;packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/mod.rs (1)
10-10: Consider enhancing the method documentation.This docstring could be more specific about what "reduced platform state bytes" represents and its expected structure to help new developers understand the system better.
packages/rs-drive/src/drive/platform_state/store_last_block_info_bytes/mod.rs (1)
10-10: Consider enhancing the method documentation.The current docstring is generic. It would be more helpful to provide specific details about what "last block info bytes" contains and its expected structure.
packages/rs-drive/src/drive/platform_state/fetch_last_block_info_bytes/mod.rs (2)
7-8: Consider consistent module placement across files.In other similar files (
store_reduced_platform_state_bytes/mod.rsandstore_last_block_info_bytes/mod.rs), the module declaration comes before imports. Consider maintaining a consistent style across all files.
10-10: Enhance method documentation for clarity.This docstring could be more specific about what exactly is being fetched from storage and its expected structure to improve developer understanding.
packages/rs-drive/src/drive/platform_state/store_last_block_info_bytes/v0/mod.rs (1)
1-29: Consider adding documentationThis method lacks documentation comments explaining its purpose, parameters, and return value. Adding rustdoc comments would improve code maintainability and make it easier for other developers to understand the function's role in the state synchronization process.
impl Drive { + /// Stores the last block information bytes in the database. + /// + /// # Arguments + /// + /// * `last_block_info_bytes` - The serialized last block information + /// * `transaction` - The transaction argument for the database operation + /// * `platform_version` - The platform version + /// + /// # Returns + /// + /// * `Ok(())` if the operation was successful + /// * `Err(Error)` if the operation failed pub(super) fn store_last_block_info_bytes_v0( &self, last_block_info_bytes: &[u8], transaction: TransactionArg, platform_version: &PlatformVersion, ) -> Result<(), Error> {packages/rs-drive/src/drive/platform_state/fetch_last_block_info_bytes/v0/mod.rs (3)
1-1: Unused import
REDUCED_PLATFORM_STATE_KEYis imported but not used in this file.-use crate::drive::platform_state::{LAST_BLOCK_INFO_KEY, REDUCED_PLATFORM_STATE_KEY}; +use crate::drive::platform_state::LAST_BLOCK_INFO_KEY;
17-17: Unnecessary clone operationThe query is cloned on line 17 but is only used once, making the clone unnecessary.
- let path_query = PathQuery::new_unsized(misc_path_vec(), query.clone()); + let path_query = PathQuery::new_unsized(misc_path_vec(), query);
1-38: Add method documentationConsider adding rustdoc comments to explain the method's purpose, parameters, and return values.
impl Drive { + /// Fetches the last block information bytes from the database. + /// + /// # Arguments + /// + /// * `transaction` - The transaction argument for the database operation + /// * `platform_version` - The platform version + /// + /// # Returns + /// + /// * `Ok(Some(Vec<u8>))` if the last block info exists + /// * `Ok(None)` if the last block info doesn't exist + /// * `Err(Error)` if an error occurred during the operation pub(super) fn fetch_last_block_info_bytes_v0( &self, transaction: TransactionArg, platform_version: &PlatformVersion, ) -> Result<Option<Vec<u8>>, Error> {packages/rs-drive-abci/src/execution/storage/store_last_block_info/v0/mod.rs (3)
5-7: Unused importsThe imports for
PlatformSerializable,ReducedPlatformSerializable, andProtocolErrorare not used in this file.-use dpp::serialization::{PlatformSerializable, ReducedPlatformSerializable}; -use dpp::version::PlatformVersion; -use dpp::ProtocolError; +use dpp::version::PlatformVersion;
19-23: Improve error messageThe error message "corrupted last block_info serialisation" doesn't accurately describe what happened. The serialization didn't become corrupted; it failed.
let block_info_bytes = bincode::encode_to_vec(block_info, config).map_err(|_| { Error::Drive(drive::error::Error::Drive(CorruptedDriveState( - "corrupted last block_info serialisation".to_string(), + "failed to serialize last block info".to_string(), ))) })?;
12-28: Add method documentationConsider adding rustdoc comments to explain the method's purpose, parameters, and error handling.
+ /// Stores the last block information by serializing the BlockInfo and calling store_last_block_info_bytes. + /// + /// # Arguments + /// + /// * `block_info` - The block information to store + /// * `transaction` - The transaction argument for the database operation + /// * `platform_version` - The platform version + /// + /// # Returns + /// + /// * `Ok(())` if successful + /// * `Err(Error)` if serialization fails or storage fails pub(super) fn store_last_block_info_v0( &self, block_info: &BlockInfo, transaction: TransactionArg, platform_version: &PlatformVersion, ) -> Result<(), Error> {packages/rs-dpp/src/reduced_platform_state/mod.rs (3)
2-2: Unused import
PlatformSerializableis imported but not used in this file.-use crate::serialization::{PlatformSerializable, ReducedPlatformDeserializable}; +use crate::serialization::ReducedPlatformDeserializable;
18-38: Add version check for forward compatibilityThe deserialization method doesn't check or enforce compatibility with the provided platform version. Consider adding version validation to ensure forward compatibility.
fn versioned_deserialize( data: &[u8], platform_version: &PlatformVersion, ) -> Result<Self, ProtocolError> where Self: Sized, { + // Check if platform_version supports the reduced platform state format + if !platform_version.dpp.reduced_platform_state_enabled { + return Err(ProtocolError::UnsupportedFeature( + "Reduced platform state is not enabled for this platform version".to_string(), + )); + } + let config = config::standard().with_big_endian().with_no_limit(); let reduced_platform_state_in_save_format: ReducedPlatformStateForSaving = bincode::decode_from_slice(data, config)
14-16: Add serialization implementationThe
ReducedPlatformDeserializabletrait is implemented, but there's no corresponding implementation for serialization. Consider implementing theReducedPlatformSerializabletrait to provide a complete interface.Consider implementing the serialization counterpart:
impl ReducedPlatformSerializable for ReducedPlatformStateForSaving { fn reduced_serialize(&self, platform_version: &PlatformVersion) -> Result<Vec<u8>, ProtocolError> { let config = config::standard().with_big_endian().with_no_limit(); let data = bincode::encode_to_vec(self, config).map_err(|e| { ProtocolError::PlatformSerializationError(format!( "unable to serialize ReducedPlatformStateForSaving: {}", e )) })?; Ok(data) } }packages/rs-drive-abci/src/abci/handler/list_snapshots.rs (2)
19-21: Redundant logging - console log and tracingThere's both a
println!and atracing::trace!statement with the same message. This creates redundant logging that can clutter logs and decrease performance.- println!("[state_sync] api list_snapshots called"); tracing::trace!("[state_sync] api list_snapshots called");Using the tracing framework alone provides better control over log levels and formatting.
31-47: Consider moving helper functions out of the main function scopeThe
convert_snapshotsandcheckpoint_existsfunctions are defined within thelist_snapshotsfunction. While this works, it would be more maintainable to define these as separate functions, especially if they might be reused elsewhere.Consider moving these helper functions outside the main function:
fn convert_to_proto_snapshot(s: Snapshot) -> proto::Snapshot { proto::Snapshot { height: s.height as u64, version: s.version as u32, hash: s.hash.to_vec(), metadata: s.metadata, } } fn checkpoint_exists(s: &Snapshot) -> bool { Path::new(&s.path).exists() } pub fn list_snapshots<A, C>( app: &A, _: proto::RequestListSnapshots, ) -> Result<proto::ResponseListSnapshots, Error> where A: SnapshotManagerApplication + PlatformApplication<C>, C: CoreRPCLike, { // ... existing code ... response.snapshots = snapshots .into_iter() .filter(checkpoint_exists) .map(convert_to_proto_snapshot) .collect(); Ok(response) }packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rs (1)
39-49: New structs for state sync version management.The implementation of
DriveAbciReducedPlatformStateStorageMethodVersionsandDriveAbciLastBlockInfoStorageMethodVersionsis appropriate, but lacks documentation comments.Consider adding Rust doc comments to enhance clarity and documentation, as recommended in previous feedback:
+/// Versioning for methods that handle reduced platform state storage #[derive(Clone, Debug, Default)] pub struct DriveAbciReducedPlatformStateStorageMethodVersions { + /// Version for the method that fetches reduced platform state pub fetch_reduced_platform_state: FeatureVersion, + /// Version for the method that stores reduced platform state pub store_reduced_platform_state: FeatureVersion, } +/// Versioning for methods that handle last block information storage #[derive(Clone, Debug, Default)] pub struct DriveAbciLastBlockInfoStorageMethodVersions { + /// Version for the method that fetches last block information pub fetch_last_block_info: FeatureVersion, + /// Version for the method that stores last block information pub store_last_block_info: FeatureVersion, }packages/rs-drive-abci/src/abci/handler/offer_snapshot.rs (2)
10-16: Assess error handling and return type.The function signature is clear and the return type is appropriate. However, consider returning more contextual errors (e.g., enumerated error variants) for debugging and telemetry purposes, especially if different snapshot-related failures must be distinguished.
17-22: Check request validation boundaries.Ensuring
app_hashhas the correct length and that the snapshot is not empty is good. Consider also verifying additional snapshot fields (e.g., format, chunk_count) if relevant, to proactively guard against malformed requests.packages/rs-drive-abci/src/abci/handler/finalize_block.rs (2)
105-115: Snapshot creation upon block finalization.Creating a snapshot after committing the block transaction is logically placed here. Consider logging or capturing metrics around snapshot creation time to monitor performance, especially if snapshots occur frequently.
120-124: Detailed tracing is beneficial.Including the
block_heightandplatform_statein the trace log is helpful for debugging. You may also want to log the result of snapshot creation for full visibility in debugging production incidents.packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs (3)
11-61: Validate request integrity and improve error messaging
Theload_snapshot_chunkfunction effectively retrieves and wraps snapshot chunks. However, consider verifying that:
request.chunk_idis not empty or malformed (e.g., unexpected length).- Error messages include more contextual details (e.g., chunk ID, request version) to help with troubleshooting.
Overall, the logic is cohesive, but adding these checks/refinements would enhance reliability.
63-68: CRC32 function is succinct and appropriate
This straightforward CRC32 implementation is correct for the intended usage. If performance becomes critical for large data sets, you might consider more optimized (possibly hardware-accelerated) CRC libraries.
172-197: Unit tests validate core chunk functionality
These tests confirm that serialization and deserialization match, and that tampering with data triggers failures. Adding boundary tests (e.g., empty chunk or very large chunk) might further expand coverage.packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (1)
325-349: Load and insert WalletUtils contract during transition to version 6
The logic for loading the system data contract and inserting it is concise. It might be beneficial to ensure that the contract is validated or checked for existence before re-insertion to prevent overwriting.packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (1)
226-254: Conversion logic to ReducedPlatformStateForSaving
This code checks the correct version fromstructs.reduced_platform_state_for_saving_structure_default. Logging or tracing the attempted version before returningUnknownVersionMismatchcan aid debugging. Otherwise, the branching is consistent.packages/rs-drive-abci/src/abci/config.rs (2)
66-81: Expand doc comments to clarify usage and responsibilities of each field.
Although the field descriptions are concise, consider adding details or examples about when to enable snapshots, how to choose a frequency, or how the snapshots directory is cleaned up. This will improve maintainability and encourage correct usage.
89-123: Avoid duplication across default_local, default_testnet, and default_mainnet.
All three methods return identical values, suggesting a single shared helper with environment-specific overrides. This makes it easier to maintain if any of the default values change.pub fn default_local() -> Self { - Self { - snapshots_enabled: true, - checkpoints_path: PathBuf::from("/var/lib/dash-platform/data/checkpoints"), - snapshots_frequency: 5, - max_num_snapshots: 100, - } + Self::new_defaults("/var/lib/dash-platform/data/checkpoints", true, 5, 100) } // Similarly for default_testnet and default_mainnet, // then define a `new_defaults` function that returns the struct.packages/rs-drive-abci/src/execution/engine/run_block_proposal/v1/mod.rs (2)
164-246: Ensure chain lock updates are thoroughly tested.
A significant portion of code handles verifying chain lock updates and returning specialized errors if validation fails. Always include test scenarios for invalid updates, unsynced cores, and good chain locks.Would you like me to propose a test case showing partial and complete chain lock verification successes/failures?
378-429: Log or handle potential GroveDB errors more gracefully.
While the code transforms the GroveDB error intoError::Drive(GroveDB(e)), consider logging more context for production debugging if a root hash retrieval fails (e.g., the exact cause from the underlying database).packages/rs-drive-abci/src/abci/app/check_tx.rs (2)
1-1: Document the reason for snapshot inclusion in CheckTx.
SinceCheckTxAbciApplicationtypically focuses on transaction validation, it is useful to clarify in doc comments how snapshots are relevant (e.g., for partial replay, caching, or fast sync).Also applies to: 5-5
117-133: Handle partial or missing snapshot chunks gracefully.
The newly introducedlist_snapshotsandload_snapshot_chunklikely wrap I/O or local file lookups. Consider clarifying what to do if a chunk cannot be loaded or if an unexpected offset is requested.packages/rs-drive-abci/src/abci/app/mod.rs (2)
26-30: Correct or update doc commentThe docstring states “Returns Platform,” but the method returns a
SnapshotManagerreference. Update the doc comment to remain accurate and help maintain clarity.- /// Returns Platform + /// Returns the SnapshotManager
50-57: Clarify the trait scope or rename for narrower intent
SnapshotFetchingApplicationincludes a generic reference toplatform(), which may extend beyond simple snapshot fetching. If additional non-snapshot methods are added later, consider a broader trait name or keep snapshot-focused functionality self-contained.packages/rs-drive-abci/tests/strategy_tests/state_sync.rs (3)
30-37: Consider cross-platform path handling
generate_random_pathuses string concatenation ("{}/{}{}"). On Windows, backslashes are standard. Where feasible, preferPathBuf::join()for better OS portability, if test coverage matters outside Unix-like environments.
39-51: Edge-case handling in directory I/OBoth
create_dir_if_not_existsandremove_dircan fail for reasons like permission denial or concurrent writes. Consider more robust error handling or detailed logs if these operations may involve concurrency or ephemeral environments.
71-304: Large monolithic test function
run_state_synccovers multiple steps for snapshot creation, chunk processing, and hash checks. Break it into smaller test functions or helper methods to enhance maintainability and readability. Also note thatnum_opsis derived from raw chunk length, which may not accurately reflect the count of logical operations.packages/rs-drive-abci/src/platform_types/snapshot/mod.rs (4)
1-5: Unused or incomplete referencesImports like
pin::Pinappear to be in active use, but verify all imported paths (e.g.,prost::Message, if introduced) are utilized as intended. Removing unused imports or adding clarifying doc references can improve clarity.
25-38: Document constraints for Snapshot fieldsAll snapshot fields (height, version, hash) rely on usage conventions (e.g., height >= 0). Document constraints or validate them on creation to avoid storing invalid block heights or incomplete data.
85-100: Frequency and directory creation error handlingIn
SnapshotManager::new, any I/O creation errors are just logged. If snapshots are critical, consider error propagation or fallback logic to handle unstoppable errors from filesystem (e.g., insufficient permissions or read-only filesystems).
193-229: Beware test data accumulationIntegration tests create persistent directories for snapshots. Ensure that ephemeral test directories are properly cleaned up to avoid unbounded usage of disk space, especially in continuous integration environments.
packages/rs-drive-abci/src/abci/app/full.rs (1)
39-43: **Potential directory existence check **
SnapshotManager::new()is invoked with paths fromplatform.config.state_sync_config. If creation or validation of the checkpoints path is necessary, consider adding error handling to avoid runtime failures due to invalid or inaccessible paths.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, ); + // Optionally add a check if the directory exists or is writable, returning an error if notpackages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs (1)
38-156: **Comprehensive snapshot chunk application **The
apply_snapshot_chunkfunction is large but well-structured, handling:
- Chunk deserialization
- Chunk application & partial acceptance
- Final commit & Grovedb verification
Potential improvements:
- Modularity: Consider splitting out chunk deserialization/application from the final commit logic for clarity.
- Testing: Ensure thorough test coverage for partial and final chunk flows, including error conditions.
- Concurrency: The single write-lock approach is straightforward, but confirm that extended blocking won’t adversely impact other operations.
packages/rs-drive-abci/src/abci/app/consensus.rs (1)
24-36: **Lifetime documentation clarity **The comment explaining
’p: ’txstands out. Consider adding or expanding docstrings about how the'pvs'txlifetimes interplay across the consensus application. It may help future maintainers.
📜 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 (80)
.github/actions/sccache/action.yaml(1 hunks).github/workflows/release-docker-image.yml(1 hunks)Dockerfile(3 hunks)packages/dapi-grpc/Cargo.toml(1 hunks)packages/dashmate/configs/defaults/getBaseConfigFactory.js(1 hunks)packages/rs-dapi-client/Cargo.toml(1 hunks)packages/rs-dpp/src/lib.rs(1 hunks)packages/rs-dpp/src/reduced_platform_state/mod.rs(1 hunks)packages/rs-dpp/src/reduced_platform_state/v0/mod.rs(1 hunks)packages/rs-dpp/src/serialization/serialization_traits.rs(1 hunks)packages/rs-drive-abci/.env.local(1 hunks)packages/rs-drive-abci/Cargo.toml(2 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/app/mod.rs(3 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/info.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/abci/handler/offer_snapshot.rs(1 hunks)packages/rs-drive-abci/src/config.rs(7 hunks)packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs(2 hunks)packages/rs-drive-abci/src/execution/engine/run_block_proposal/v1/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/platform_events/core_based_updates/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/platform_events/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs(2 hunks)packages/rs-drive-abci/src/execution/storage/fetch_last_block_info/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/storage/fetch_last_block_info/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/storage/fetch_reduced_platform_state/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/storage/fetch_reduced_platform_state/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/storage/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/storage/store_last_block_info/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/storage/store_last_block_info/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/storage/store_reduced_platform_state/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/storage/store_reduced_platform_state/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/platform_types/mod.rs(1 hunks)packages/rs-drive-abci/src/platform_types/platform_state/mod.rs(4 hunks)packages/rs-drive-abci/src/platform_types/platform_state/v0/mod.rs(3 hunks)packages/rs-drive-abci/src/platform_types/snapshot/mod.rs(1 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-abci/tests/strategy_tests/voting_tests.rs(1 hunks)packages/rs-drive-proof-verifier/Cargo.toml(1 hunks)packages/rs-drive/Cargo.toml(1 hunks)packages/rs-drive/src/drive/platform_state/fetch_last_block_info_bytes/mod.rs(1 hunks)packages/rs-drive/src/drive/platform_state/fetch_last_block_info_bytes/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/platform_state/fetch_reduced_platform_state_bytes/mod.rs(1 hunks)packages/rs-drive/src/drive/platform_state/fetch_reduced_platform_state_bytes/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/platform_state/mod.rs(1 hunks)packages/rs-drive/src/drive/platform_state/store_last_block_info_bytes/mod.rs(1 hunks)packages/rs-drive/src/drive/platform_state/store_last_block_info_bytes/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/mod.rs(1 hunks)packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/v0/mod.rs(1 hunks)packages/rs-drive/src/error/drive.rs(1 hunks)packages/rs-json-schema-compatibility-validator/Cargo.toml(1 hunks)packages/rs-platform-version/Cargo.toml(1 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rs(2 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v1.rs(2 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v2.rs(2 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v3.rs(2 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v4.rs(2 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v5.rs(2 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v6.rs(3 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_structure_versions/mod.rs(1 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_structure_versions/v1.rs(1 hunks)packages/rs-platform-version/src/version/drive_versions/mod.rs(2 hunks)packages/rs-platform-version/src/version/drive_versions/v1.rs(2 hunks)packages/rs-platform-version/src/version/drive_versions/v2.rs(2 hunks)packages/rs-platform-version/src/version/drive_versions/v3.rs(2 hunks)packages/rs-platform-version/src/version/drive_versions/v4.rs(2 hunks)packages/rs-platform-version/src/version/mocks/v2_test.rs(2 hunks)packages/rs-platform-version/src/version/mocks/v3_test.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (11)
Dockerfile (1)
Learnt from: lklimek
PR: dashpay/platform#2318
File: Dockerfile:160-198
Timestamp: 2024-11-13T10:31:31.084Z
Learning: In the `Dockerfile` for the rs-drive-abci project, when configuring sccache, we do not need to add validation checks for configuration variables, as sccache performs validation at runtime.
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.
packages/rs-dpp/src/reduced_platform_state/mod.rs (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-11-12T14:56:12.334Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
packages/rs-drive-abci/src/execution/storage/store_reduced_platform_state/v0/mod.rs (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-11-12T14:56:12.334Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (1)
Learnt from: shumkov
PR: dashpay/platform#2422
File: packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:152-163
Timestamp: 2025-01-15T08:09:59.365Z
Learning: In the `transition_to_version_8` function, errors from `grove_get_path_query` when retrieving active contested resource votes are intentionally logged and ignored (returning `Ok(())`) to allow the protocol upgrade to proceed despite query failures.
packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-11-12T14:56:12.334Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
packages/rs-drive-abci/src/execution/engine/run_block_proposal/v1/mod.rs (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2215
File: packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs:119-120
Timestamp: 2024-11-12T14:57:47.419Z
Learning: In the `run_block_proposal` function in `packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs`, it's acceptable to pass `platform_state` to `perform_events_on_first_block_of_protocol_change`, even if `block_platform_state` has been updated.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_structure_versions/v1.rs (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-11-12T14:56:12.334Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
packages/rs-drive/Cargo.toml (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2431
File: packages/rs-drive/Cargo.toml:55-60
Timestamp: 2025-01-19T07:36:46.042Z
Learning: The grovedb dependencies in packages/rs-drive/Cargo.toml and related files are intentionally kept at specific revisions rather than using the latest stable version, with plans to update them at a later time.
packages/rs-drive-abci/src/platform_types/platform_state/v0/mod.rs (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-11-12T14:56:12.334Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rs (1)
Learnt from: shumkov
PR: dashpay/platform#2182
File: packages/rs-platform-version/src/version/drive_abci_versions.rs:116-121
Timestamp: 2024-11-12T14:56:12.333Z
Learning: Adding Rust doc comments to structs and fields is recommended to enhance clarity and documentation.
🪛 actionlint (1.7.4)
.github/workflows/release-docker-image.yml
42-42: label "ubuntu-24.04-arm" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (142)
.github/actions/sccache/action.yaml (1)
37-37:✅ Verification successful
Version bump for sccache looks good.
Updating the sccache version from 0.8.2 to 0.9.1 should bring performance improvements and bug fixes to the build process.
Please ensure that CI/CD pipelines continue to function correctly with this new version. You might want to check the sccache release notes for any breaking changes:
🏁 Script executed:
#!/bin/bash # Check release notes for sccache 0.9.1 curl -s https://api.github.com/repos/mozilla/sccache/releases/tags/v0.9.1 | jq -r '.body'Length of output: 1300
sccache version bump verified and CI/CD confirmation required
The update from version 0.8.2 to 0.9.1 is confirmed to include several bug fixes and performance enhancements (such as improved jobserver handling, nvcc compilation deconstruction, and test fixes) as detailed in the release notes. There are no apparent breaking changes. Please ensure that your CI/CD pipelines continue to run smoothly with this new version.
packages/rs-drive-abci/src/platform_types/mod.rs (1)
35-36: Excellent addition for state synchronization support.Adding a snapshot module is an essential component for implementing ABCI state synchronization functionality, which aligns perfectly with the PR's purpose.
packages/rs-dpp/src/lib.rs (1)
63-63: Good addition for optimized state management.The new
reduced_platform_statemodule should help optimize state synchronization by providing a more efficient representation of the platform state for transfer operations.packages/rs-drive-abci/src/execution/mod.rs (1)
6-6: Appropriate visibility change for cross-module access.Expanding the visibility of the
platform_eventsmodule to crate-level access makes sense for the state synchronization implementation, as it likely needs to be accessed from other modules within the crate.packages/rs-json-schema-compatibility-validator/Cargo.toml (1)
10-10: Dependency version updated to be more flexibleThe change from a specific version (
1.0.115) to a more general specification ({ version = "1.0" }) provides greater flexibility for dependency updates within the 1.0.x range, which follows semantic versioning principles.packages/rs-drive-abci/tests/strategy_tests/main.rs (1)
29-29: New state_sync module added for ABCI state synchronizationThe addition of this module aligns with the "final abci state sync" feature described in the PR objectives. This is an appropriate organization of test code for the new functionality.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_structure_versions/mod.rs (1)
9-9:❓ Verification inconclusive
Added new feature version field for reduced platform state
This new field
reduced_platform_state_for_saving_structure_defaulthas been added to support state synchronization functionality. Make sure this field is properly initialized in all version constructors to avoid potentialNonevalues.
🏁 Script executed:
#!/bin/bash # Check if this new field is properly initialized in version constructors fd --type file "drive_abci_structure_versions" | grep -v "mod.rs" | xargs grep -l "DriveAbciStructureVersions" | xargs grep -A20 "reduced_platform_state_for_saving_structure_default"Length of output: 176
Ensure Field Initialization in Version Constructors
The new field
reduced_platform_state_for_saving_structure_defaultappears correctly added in the module; however, our automated search didn’t reveal any explicit initialization in the version constructors ofDriveAbciStructureVersions. Please manually verify that each constructor (or any default/macro-based initializer) properly sets a value for this field to prevent potentialNonevalues.
- File Affected:
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_structure_versions/mod.rs(line 9)- Action Required: Check all constructors or initialization logic for
DriveAbciStructureVersionsto confirm thatreduced_platform_state_for_saving_structure_defaultis assigned a default value or explicitly initialized.packages/rs-dapi-client/Cargo.toml (1)
38-38: Dependency version updated to be more flexibleThe change from a specific version (
1.0.120) to a more general specification ({ version = "1.0" }) follows the same pattern applied to other dependencies in this PR, providing greater flexibility for dependency updates within the 1.0.x range.packages/rs-drive-abci/tests/strategy_tests/voting_tests.rs (1)
1667-1668: LGTM: Helpful comment about test context.Adding this comment improves code readability by providing context about the total number of votes cast in this test scenario.
packages/rs-drive-abci/src/execution/platform_events/core_based_updates/mod.rs (1)
4-4: LGTM: Increased module visibility for internal code reuse.The visibility change from private to crate-visible (
pub(crate)) allows other parts of the crate to access the quorum info functionality while still maintaining proper encapsulation from external modules.packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/mod.rs (1)
1-1: LGTM: Module visibility adjustment aligns with parent module change.This change is consistent with the visibility update in the parent module, completing the visibility chain needed for proper access to quorum-related functionality throughout the crate.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_structure_versions/v1.rs (1)
7-7: New structure version for reduced platform state.The addition of
reduced_platform_state_for_saving_structure_default: 0aligns with the state sync feature implementation and correctly follows the versioning pattern used for other structure versions.packages/rs-drive-abci/.env.local (1)
16-19: Environment variables added to support state synchronization.These new environment variables properly define paths for checkpoints and the latest state file, which are essential components for the state sync feature.
packages/rs-drive-abci/src/execution/platform_events/mod.rs (1)
8-8: Visibility broadened for core_based_updates module.Changing the visibility from
pub(in crate::execution)topub(crate)makes the module accessible throughout the crate, which appears necessary for the state sync implementation.packages/dashmate/configs/defaults/getBaseConfigFactory.js (1)
312-312: Docker image tag uses feature branch name.While the image change is necessary for the state sync feature, using a feature branch name (
feat-statesync-improvements) in a Docker image tag for default configuration is not ideal for production code.Consider using a versioned tag (e.g.,
1.x.y) once the feature is merged to ensure stability and proper versioning in production environments.packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs (1)
18-18: New module addition for version 1 implementationThe addition of
mod v1;indicates a new version of the block proposal execution implementation. This aligns with the versioning pattern already established with the existingmod v0;.packages/rs-platform-version/Cargo.toml (1)
14-14:❓ Verification inconclusive
Dependency reference changed from a specific commit to a branch
The dependency
grovedb-versionhas been changed from a specific commit reference to thedevelopbranch. This might introduce instability or unpredictable behavior as the dependency will now follow the latest commit on the develop branch instead of a fixed version.
🌐 Web query:
What are the best practices for specifying Git dependencies in Cargo.toml?💡 Result:
When specifying Git dependencies in Rust's
Cargo.toml, follow these best practices to ensure stability, security, and compatibility:1. Use Explicit References for Stability
- Pin to specific commits or tags to avoid unexpected changes from upstream:
This ensures builds are reproducible[1][8]. Avoid relying solely on branches like[dependencies] my_crate = { git = "https://github.com/user/repo.git", rev = "a1b2c3d" }main, which can change over time[4][6].2. Avoid Git Dependencies for Published Crates
- Crates.io blocks packages with Git dependencies[2][8]. Use Git dependencies only for private/unpublished projects or development forks. For public packages:
- Publish dependencies to crates.io.
- Use path dependencies for local workspace crates[2][6].
3. Secure Private Repositories
- Use SSH URLs for private GitLab/GitHub repos and configure authentication:
Add a[dependencies] private_lib = { git = "ssh://[email protected]/group/repo.git", branch = "dev" }.cargo/configfile to use your local Git CLI for SSH key handling[5].4. Validate Version Compatibility
- Include a
versionkey to enforce semver checks:Cargo verifies the crate's declared version matches the Git version[8].[dependencies] serde = { git = "https://github.com/serde-rs/serde", version = "1.0", branch = "main" }5. Structure Workspaces for Local Development
- Prefer path dependencies for internal crates within a monorepo:
This simplifies dependency management and avoids Git overhead for local changes[1][6].[dependencies] utils = { path = "../utils" }6. Handle Authentication in CI/CD Pipelines
- For Docker/CI environments, use SSH agent forwarding or token-based authentication:
Configure Git in your CI scripts to access private repositories securely[5].RUN --mount=type=ssh cargo build7. Enable Optional Features Correctly
- Activate features explicitly for Git-sourced crates:
Avoid assuming default features are enabled[4][8].[dependencies] reqwest = { git = "https://github.com/seanmonstar/reqwest", features = ["json"] }Comparison: Git vs. Registry Dependencies
Aspect Git Dependencies Crates.io Dependencies Publish Compatibility Blocked on crates.io[2][8] Required for publication Version Pinning Commit/tag-based SemVer ranges Offline Availability Requires cache setup Cached locally by default Security SSH/HTTPS auth needed for private Publicly accessible By following these guidelines, you balance flexibility with stability while adhering to Cargo's constraints. For published crates, transition Git dependencies to versioned crates.io entries or workspace paths before release[2][6][8].
Citations:
- 1: https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/cargo/reference/specifying-dependencies.html
- 2: https://users.rust-lang.org/t/help-cargo-package-with-github-dependencies/96275
- 3: rust-lang/cargo#7497
- 4: https://hackernoon.com/introduction-to-cargo-and-cargotoml
- 5: https://oliverjumpertz.com/blog/how-to-handle-private-gitlab-dependencies-with-cargo/
- 6: https://dev.to/alexmercedcoder/introduction-to-cargo-and-cargotoml-2l86
- 7: https://github.com/rust-lang/cargo
- 8: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html
- 9: rust-lang/cargo#13121
- 10: https://users.rust-lang.org/t/best-practice-in-specifying-dependency-versions-in-cargo-toml/54007
Attention: Review Dependency Pinning for
grovedb-versionThe dependency in
packages/rs-platform-version/Cargo.toml(line 14) now tracks thedevelopbranch:grovedb-version = { git = "https://github.com/dashpay/grovedb", branch = "develop" }Using a branch reference rather than a fixed commit or tag can lead to unstable or unpredictable builds since any new upstream commit will be automatically included. Best practices for Cargo dependencies recommend pinning to a specific commit (using the
revkey) to ensure reproducibility and compatibility.Please verify whether this change was intentional, or if you would prefer to revert to a pinned commit for production stability.
packages/rs-drive-abci/src/execution/storage/store_reduced_platform_state/v0/mod.rs (1)
1-24: New implementation for storing reduced platform stateThis new module implements
store_reduced_platform_state_v0which serializes and stores a reduced platform state. The implementation looks sound, properly handling errors and following the project's architecture patterns.Based on the retrieved learning, it seems this is related to state serialization for version 1, though the implementation here is specifically for version 0 handling.
One suggestion is to add a brief documentation comment explaining what "reduced platform state" means in this context, as it would help future developers understand the purpose of this functionality.
packages/rs-drive-abci/src/abci/error.rs (1)
57-63: New error variants for state syncThe addition of
StateSyncBadRequestandStateSyncInternalErrorvariants provides proper error handling for the state synchronization feature being implemented in this PR. These error types appropriately distinguish between client-side bad requests and server-side internal errors.packages/dapi-grpc/Cargo.toml (1)
45-48: Dependency update and feature additionUpgrading tenderdash-proto to 1.3.0 and adding the "serde" feature is consistent with the state sync implementation requirements. The serde feature enables serialization/deserialization capabilities that are likely needed for the state synchronization functionality.
packages/rs-drive-abci/Cargo.toml (2)
52-56: Updated tenderdash-abci dependencyAppropriate upgrade of tenderdash-abci to version 1.3.0 with the necessary "crypto" and "serde" features to support the state sync implementation.
79-79: New crc dependencyThe addition of the crc crate (v3.2.1) is likely needed for checksum calculations in the state synchronization process, which is a common requirement for ensuring data integrity during state transfers.
packages/rs-drive-abci/src/execution/storage/store_reduced_platform_state/mod.rs (1)
1-32: Well-structured versioned implementation for platform state storageThe implementation follows good practices by:
- Using version-based dispatching to support backward compatibility
- Providing clear error messages when an unsupported version is encountered
- Following a modular approach with separate implementation files for each version
The code is well-documented and follows the project's error handling patterns. This structure will make it easy to add new versions in the future while maintaining backward compatibility.
packages/rs-drive-abci/src/execution/storage/mod.rs (1)
1-6: Modular structure looks goodThe addition of modules for handling reduced platform state and last block info follows a consistent naming pattern with existing modules. This modular approach aligns well with the PR objective of implementing state synchronization functionality.
packages/rs-dpp/src/reduced_platform_state/v0/mod.rs (1)
8-21: Well-structured data model for reduced platform stateThe
ReducedPlatformStateForSavingV0struct provides a clear representation of essential state data needed for synchronization. The fields are well-named and documented, and the struct properly derives the necessary traits for serialization/deserialization.packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v2.rs (1)
128-135: Version configuration looks goodThe addition of
platform_reduced_state_storageandlast_block_info_storageto the versioning structure maintains consistency with the existing pattern. Both method groups start with version 0, which is appropriate for new functionality.packages/rs-drive-abci/src/execution/storage/fetch_reduced_platform_state/mod.rs (1)
11-34: Clean implementation with proper version handlingThe implementation follows a version-based dispatch pattern with proper error handling for unknown versions. This approach maintains backward compatibility while allowing for future extensions.
packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/mod.rs (1)
9-35: Implementation looks clean and follows good versioning patterns.The function uses a clean match pattern for version routing that follows the project's approach to backward compatibility. The error handling is also comprehensive, providing detailed information about version mismatches.
packages/rs-drive/src/drive/platform_state/store_last_block_info_bytes/mod.rs (1)
9-35: Implementation looks clean and follows good versioning patterns.The function follows the same versioned approach as other Drive methods, maintaining consistency in the codebase. The error handling is comprehensive with detailed mismatch information.
.github/workflows/release-docker-image.yml (1)
42-42:❓ Verification inconclusive
Verify the runner label exists in your GitHub environment.
The static analysis indicates that
ubuntu-24.04-armis not a standard GitHub-hosted runner label. If this is a custom self-hosted runner in your environment, you should either:
- Create a custom configuration file for actionlint to recognize this label, or
- Use one of the standard GitHub runner labels with appropriate setup steps for ARM builds.
🏁 Script executed:
#!/bin/bash # Check GitHub Actions runner availability in public docs echo "Checking for standard GitHub-hosted runners..." echo "Note: If 'ubuntu-24.04-arm' is a custom self-hosted runner in your environment, it won't appear in this list." echo "Standard runner labels according to GitHub documentation:" echo "- ubuntu-latest, ubuntu-24.04, ubuntu-22.04" echo "- windows-latest, windows-2022, windows-2019" echo "- macos-latest, macos-14, macos-13, etc." # Check for any self-hosted runner reference in the repo echo -e "\nChecking for self-hosted runner configuration in the repository..." grep -r "self-hosted" --include="*.yml" --include="*.yaml" .Length of output: 962
Action: Confirm and Adjust Runner Configuration
- The label
ubuntu-24.04-armis not a standard GitHub-hosted runner label (standard options include labels likeubuntu-latest,ubuntu-24.04, orubuntu-22.04).- Our grep search did not reveal any self-hosted runner configuration in the repository, which suggests that no custom runner is set up for this label.
- Please verify whether you intend to use a custom self-hosted runner. If so, consider adding a custom configuration file for actionlint so it recognizes the label. Otherwise, update the workflow to use one of the standard GitHub runner labels with the appropriate setup for ARM builds.
🧰 Tools
🪛 actionlint (1.7.4)
42-42: label "ubuntu-24.04-arm" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
packages/rs-drive/src/drive/platform_state/fetch_last_block_info_bytes/mod.rs (1)
9-30: Implementation looks clean and follows good versioning patterns.The function follows the same versioned approach as other Drive methods. The error handling is also well-implemented with detailed information in case of version mismatches.
packages/rs-drive-abci/src/execution/storage/fetch_last_block_info/v0/mod.rs (1)
18-40: Implementation looks good and follows best practicesThe
fetch_last_block_info_v0method is well-implemented with proper error handling and follows Rust best practices:
- Correctly handles Drive errors by mapping them to the custom Error type
- Uses bincode for deserialization with appropriate configuration settings
- Handles corrupted serialization state with clear error messages
- Correctly uses
transpose()to convertOption<Result<T, E>>toResult<Option<T>, E>packages/rs-drive-abci/src/execution/storage/store_last_block_info/mod.rs (1)
10-31: Well-structured version-aware implementationThe
store_last_block_infomethod follows a good pattern for version-aware implementations:
- Clear documentation comment explaining the purpose
- Version checking against the platform version configuration
- Delegation to version-specific implementations
- Comprehensive error handling for unknown versions with detailed error information
packages/rs-drive/src/drive/platform_state/fetch_reduced_platform_state_bytes/v0/mod.rs (1)
8-38: Database query implementation follows best practicesThe implementation demonstrates good practices for database interaction:
- Properly constructs the query with the specific key
- Correctly uses the platform version's grove version for compatibility
- Includes proper validation of the result count
- Uses appropriate error handling for invalid query results
One minor suggestion would be to consider adding a trace or debug log statement to help with troubleshooting in case of errors or for monitoring purposes.
packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/v0/mod.rs (2)
24-25: Visibility change enhances code modularityThe change from private to public visibility for the
QuorumSetTypeenum improves the modularity of the codebase by allowing it to be used from other modules, which is necessary for state synchronization features.
30-30: Appropriate visibility modification for methodChanging the
quorum_typemethod visibility topub(crate)is a good design choice as it allows the method to be used within the same crate while restricting external access, maintaining proper encapsulation.packages/rs-platform-version/src/version/drive_versions/v2.rs (2)
15-20: Added new methods for reduced platform state and last block info.The imports now include two new method versions:
DriveReducedPlatformStateMethodVersionsandDriveLastBlockInfoMethodVersions, which will support new functionality related to state synchronization.
104-111: Implemented new methods for platform state sync.Two new method version structures have been added to the
DriveVersionstruct:
reduced_platform_state- For fetching and storing reduced platform statelast_block_info- For fetching and storing last block informationBoth are essential components for implementing ABCI state synchronization. The version number of 0 indicates these are newly introduced methods.
packages/rs-drive-abci/src/config.rs (4)
1-1: Added StateSyncAbciConfig import.Importing the state sync configuration from the ABCI config module to support the new state synchronization feature.
191-193: Added state sync configuration field.The
state_sync_configfield of typeStateSyncAbciConfighas been added to thePlatformConfigstruct, allowing configuration of state synchronization parameters.
285-285: Added default initialization for state sync config.When deserializing configuration, the state sync configuration is initialized with default values, ensuring backward compatibility with existing configuration files.
741-741: Added environment-specific state sync configurations.Environment-specific state sync configurations have been added for local, devnet, testnet, and mainnet environments, ensuring appropriate default settings for each network type.
Also applies to: 785-785, 870-870
packages/rs-drive-abci/src/platform_types/platform_state/v0/mod.rs (3)
33-33: Added import for ReducedPlatformStateForSavingV0.Importing the reduced platform state structure that will be used in the conversion implementation.
118-118: Enhanced Debug implementation.Updated the Debug implementation to include the
previous_fee_versionsfield, improving observability and debugging capabilities.
257-278: Implemented TryFrom for creating reduced platform state.Added a conversion implementation from
PlatformStateV0toReducedPlatformStateForSavingV0, extracting only the essential fields needed for state synchronization:
- protocol versions
- validator set quorum hashes
- previous fee versions
This approach optimizes state transfer by reducing the data size while maintaining the critical information needed for synchronization.
packages/rs-drive-abci/src/execution/storage/fetch_reduced_platform_state/v0/mod.rs (2)
1-15: Added imports for reduced platform state functionality.The imports include error handling, Drive access, serialization utilities, and the reduced platform state structures necessary for implementing the fetch functionality.
16-42: Implemented reduced platform state fetching.This method fetches the reduced platform state from the Drive storage and deserializes it. Key features:
- Proper error handling with appropriate mapping to domain errors
- Version-aware deserialization using the platform version
- Detailed error logging with hex-encoded bytes for debugging
- Clean result handling with
transpose()to maintain the Option wrapperThe implementation follows good practices for error handling and logging, which will be valuable during troubleshooting of state synchronization issues.
packages/rs-drive/src/drive/platform_state/store_last_block_info_bytes/v0/mod.rs (1)
16-26:❓ Verification inconclusive
Double check error handling pattern
The
.unwrap().map_err()pattern on lines 25-26 is unusual. Typically,unwrap()would panic on error, not allowingmap_err()to execute. Please verify if this is the intended behavior for GroveDB's error handling or if it should be replaced with a pattern like.map_err(Error::GroveDB)?.
🏁 Script executed:
#!/bin/bash # Check for similar error handling patterns in the codebase rg "unwrap\(\)\.map_err" --type rustLength of output: 38
Action: Verify Error Handling Chain in GroveDB Insert Operation
At
packages/rs-drive/src/drive/platform_state/store_last_block_info_bytes/v0/mod.rs(lines 16-26), we see the following chain:self.grove .insert( &misc_path(), LAST_BLOCK_INFO_KEY, Element::Item(last_block_info_bytes.to_vec(), None), Some(InsertOptions::default()), transaction, &platform_version.drive.grove_version, ) .unwrap() .map_err(Error::GroveDB)?;This pattern—calling
.unwrap()immediately before.map_err(Error::GroveDB)?—is unconventional becauseunwrap()will panic on error, thus preventing the subsequent error mapping from ever executing.
- Action Required: Please verify whether
self.grove.insert(...)returns a nestedResultwhere an outerResultmust be unwrapped to access an inner result that still needs error mapping.- Suggestion: If the intention is only to propagate errors without panicking, consider replacing
.unwrap().map_err(Error::GroveDB)?with a single.map_err(Error::GroveDB)?call.packages/rs-drive/src/drive/platform_state/fetch_reduced_platform_state_bytes/mod.rs (1)
1-30: Well-structured versioned method implementationThis module implements a versioned approach to fetch reduced platform state bytes, which is good for backward compatibility and future extensibility. The implementation includes proper error handling for unsupported versions and follows a clean pattern of delegating to version-specific implementations.
A few observations:
- The code correctly uses the platform version to determine which implementation to use
- Error handling for unsupported versions is comprehensive and informative
- The documentation comments are helpful but could be expanded to describe the return value structure
packages/rs-drive-abci/src/execution/storage/fetch_last_block_info/mod.rs (1)
25-25:✅ Verification successful
Confirm implementation of
fetch_last_block_info_v0existsEnsure that the
fetch_last_block_info_v0static method referenced here exists in the v0 module and has a matching signature.
🏁 Script executed:
#!/bin/bash # Check if the fetch_last_block_info_v0 function exists in the v0 module grep -r "fn fetch_last_block_info_v0" packages/rs-drive-abci/src/execution/storage/fetch_last_block_info/v0/Length of output: 231
Verification Successful: Implementation Confirmed
The
fetch_last_block_info_v0function is present in thepackages/rs-drive-abci/src/execution/storage/fetch_last_block_info/v0/mod.rsfile with the expected visibility and signature. No further changes are needed.packages/rs-drive-proof-verifier/Cargo.toml (2)
37-40: Version and feature update for tenderdash-abciThe dependency on tenderdash-abci has been updated from 1.2.1 to 1.3.0, and the "serde" feature has been added. This aligns with the ABCI state sync feature implementation, enabling proper serialization and deserialization for state sync data structures.
43-43: Changed serde_json version to major version onlyThe version requirement for serde_json has been changed from a specific patch version (1.0.103) to just the major version (1.0). This allows more flexibility with dependency resolution while maintaining compatibility.
packages/rs-dpp/src/serialization/serialization_traits.rs (2)
34-37: New reduced serialization trait addedThe
ReducedPlatformSerializabletrait introduces a mechanism for creating more compact serialized representations of platform state, which is essential for efficient state synchronization between nodes.
39-46: Added complementary deserialization trait for reduced stateThe
ReducedPlatformDeserializabletrait complements the serialization trait, providing version-aware deserialization for reduced platform state. This is important for maintaining compatibility across different versions during state sync operations.packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v5.rs (2)
9-9: Added new method version imports for state sync functionalityNew imports for
DriveAbciLastBlockInfoStorageMethodVersionsandDriveAbciReducedPlatformStateStorageMethodVersionssupport the state sync feature implementation.Also applies to: 12-12
131-138: Added method versions for state sync operationsTwo new method version groups have been added:
platform_reduced_state_storagefor handling the reduced platform statelast_block_info_storagefor tracking last block informationBoth are essential components for implementing ABCI state synchronization functionality, with initial method versions set to 0.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v1.rs (2)
9-9: Added imports for state sync method versionsImported the same method version structs as in v5.rs to support state sync functionality in earlier platform versions.
Also applies to: 12-12
127-134: Backported state sync method versions to v1The same method version groups added to v5 have been added to v1:
platform_reduced_state_storagelast_block_info_storageThis ensures consistent implementation of state sync functionality across all platform versions.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v3.rs (2)
9-9: Consistent import additionsThese imports align with the new methods being added to the
DRIVE_ABCI_METHOD_VERSIONS_V3constant.Also applies to: 12-12
127-134: New platform state storage methods for state syncThe addition of
platform_reduced_state_storageandlast_block_info_storagewith initial version 0 is appropriate for new functionality. These components will enable the ABCI state sync feature by providing methods to store and retrieve reduced platform state and last block information.packages/rs-drive/src/drive/platform_state/mod.rs (2)
1-1: New modules for state sync functionalityThese module declarations introduce the implementation for fetching and storing reduced platform state and last block information, which are essential components for the ABCI state sync feature.
Also applies to: 3-4, 6-6
9-10: Constants for storage keysThe constants
REDUCED_PLATFORM_STATE_KEYandLAST_BLOCK_INFO_KEYprovide clear, descriptive names for the storage keys used in the state sync implementation. The byte array values are appropriate and follow the existing naming pattern.packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v4.rs (2)
9-9: Consistent import additions across versionsThe imports align with the new methods being added, ensuring consistency across different version files.
Also applies to: 12-12
127-134: Maintaining consistency across version filesThe addition of the same state sync components to
DRIVE_ABCI_METHOD_VERSIONS_V4as were added to V3 ensures a consistent implementation across different protocol versions.Dockerfile (4)
169-169: Update sccache versionUpdating
SCCHACHE_VERSIONfrom 0.8.2 to 0.9.1 ensures consistency with the changes in GitHub Actions workflows.
575-575: New environment variable for checkpointsThe addition of
CHECKPOINTS_PATHprovides a dedicated location for state sync checkpoints.
602-605: Update permissions for new directory structureCorrectly updating the ownership of all required directories, including the new checkpoint path, to the dash user ensures proper permission management.
585-591:✅ Verification successful
Ensure required paths exist
Creating the necessary directories including the new
CHECKPOINTS_PATHensures the application has the proper structure on startup.Note the TODO comment about removing the duplicate path when drive-abci is fixed, which should be addressed in future updates.
🏁 Script executed:
#!/bin/bash # Check if this TODO is mentioned in any issues or other PRs echo "Searching for related TODO mentions in the codebase..." rg "remove /var/lib/dash-platform/data/checkpoints" -A 2 -B 2Length of output: 704
Review Verification – Dockerfile Directory Setup
The Dockerfile correctly creates all necessary directories, including the addition ofCHECKPOINTS_PATH. The TODO comment about removing/var/lib/dash-platform/data/checkpointswhen drive-abci is fixed appears in multiple locations (as confirmed by the search result), so it’s clear the duplicate path removal is pending future fixes.No changes are required at this time since the code meets current expectations.
packages/rs-platform-version/src/version/drive_versions/v3.rs (2)
15-20: Updated imports to support new state functionality.The imports have been properly extended to include the new
DriveLastBlockInfoMethodVersionsandDriveReducedPlatformStateMethodVersionstypes which are essential for the ABCI state synchronization feature.
104-111: New methods for state synchronization support added.The
reduced_platform_stateandlast_block_infofields have been properly added to theDriveMethodVersionsstructure with appropriate versioning. This enables two key functionalities:
- Ability to store and fetch reduced platform state (likely for more efficient state synchronization)
- Ability to store and fetch last block information (necessary for proper state synchronization between nodes)
Both are initialized with version 0, following the pattern for newly introduced methods.
packages/rs-platform-version/src/version/drive_versions/mod.rs (3)
63-64: Public API extension for reduced state and block info.New fields have been properly added to the
DriveMethodVersionsstruct, following the existing pattern. These additions maintain backward compatibility while extending the functionality.
73-77: New struct for reduced platform state operations.The
DriveReducedPlatformStateMethodVersionsstruct is correctly defined with appropriate fields for fetching and storing reduced platform state. This structure follows the established pattern in the codebase for method versioning.
79-83: New struct for last block info operations.The
DriveLastBlockInfoMethodVersionsstruct is properly defined with methods for fetching and storing last block information. These operations are essential for state synchronization between nodes.packages/rs-platform-version/src/version/mocks/v2_test.rs (2)
42-47: Updated imports for test consistency.The import section has been properly updated to include the new method version types, ensuring that the test code can reference these new structures.
140-147: Test fixture updated with new method versions.The test platform configuration has been correctly updated to include the new method versions, ensuring that the test environment matches the production environment. Both new structures are initialized with version 0, maintaining consistency with their production counterparts.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v6.rs (3)
9-12: Added imports for new ABCI method version types.The imports have been properly updated to include the new ABCI method version types for last block information and reduced platform state storage.
128-135: Added new ABCI method versions for state synchronization.Two new method version structures have been correctly added to the
DRIVE_ABCI_METHOD_VERSIONS_V6constant:
platform_reduced_state_storage- For handling reduced platform state operationslast_block_info_storage- For managing last block informationBoth are initialized with version 0, indicating they are newly introduced methods. These additions complete the state synchronization feature across the ABCI layer.
22-22:✅ Verification successful
Version increment for run_block_proposal method.
The version of the
run_block_proposalmethod has been incremented from 0 to 1, indicating a change in functionality. This is likely related to the addition of state synchronization features.
🏁 Script executed:
#!/bin/bash # Search for the implementation of run_block_proposal to understand what changed rg -A 5 "fn run_block_proposal" --type rustLength of output: 1976
Attention: Verified Version Bump for
run_block_proposalThe version mapping in
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v6.rsnow correctly setsrun_block_proposal: 1. This update aligns with the existence of a dedicated implementation inpackages/rs-drive-abci/src/execution/engine/run_block_proposal/v1/mod.rs, confirming that the functionality has indeed been revised—likely to support state synchronization enhancements.
- Confirmed the presence of both
run_block_proposal_v0andrun_block_proposal_v1implementations in the codebase.- The update in the platform version file now directs to the new functionality provided in the v1 implementation.
packages/rs-drive-abci/src/abci/handler/mod.rs (2)
46-48: Additions of new snapshot modules aligned with state sync functionality.The additions of modules
list_snapshots,load_snapshot_chunk, andoffer_snapshotimplement core functionality required for ABCI state synchronization.
60-62: Exported public functions for snapshot operations.These public exports make the snapshot handling functions accessible to other modules, which is necessary for proper state synchronization implementation.
packages/rs-platform-version/src/version/drive_versions/v4.rs (2)
15-20: New imports for state sync functionality.The added imports for
DriveLastBlockInfoMethodVersionsandDriveReducedPlatformStateMethodVersionsproperly prepare the file for the new structs being used.
104-111: Added version support for reduced platform state and last block info.These additions extend the versioning system to support the new state synchronization features. Both method groups are initialized with version 0, which is appropriate for new functionality.
packages/rs-platform-version/src/version/mocks/v3_test.rs (3)
25-28: New imports for test mocks.The imported method version structures ensure that the test mocks can properly incorporate the new state sync functionality.
161-168: Added state sync method versions to test mock.The test mock now correctly includes the new reduced platform state and last block info storage methods, ensuring tests will properly validate the new functionality.
55-55:✅ Verification successful
Updated run_block_proposal version.
The version update from 0 to 1 indicates changes to the block proposal functionality, which is likely related to the state sync implementation.
🏁 Script executed:
#!/bin/bash # Search for the implementation of the run_block_proposal function to understand its changes rg -A 5 -B 5 "fn run_block_proposal" --type rustLength of output: 3766
Attention: Updated run_block_proposal version confirmed
The update in the mock (from 0 to 1) correctly reflects the introduction of the v1 implementation of
run_block_proposal(as seen in packages/rs-drive-abci/src/execution/engine/run_block_proposal/v1/mod.rs), and aligns with the revised block proposal and state sync functionality.
- The test mock in
packages/rs-platform-version/src/version/mocks/v3_test.rsnow accurately indicates the new version.- The change is consistent with the refactored implementation across the codebase.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rs (1)
29-30: New fields for state sync in DriveAbciMethodVersions.These additions properly integrate the new state synchronization capabilities into the versioning system.
packages/rs-platform-version/src/version/drive_versions/v1.rs (3)
15-20: Imports and method versions look consistent.No issues are apparent in the newly added method version imports. They follow the existing pattern, and the references are correct.
104-107: Ensure test coverage for newly introducedreduced_platform_statemethod versions.While adding these version references is valid, it’s recommended to verify that corresponding logic and unit tests exist (or are planned) to confirm that fetching and storing reduced platform state works as intended.
108-111: Confirm usage oflast_block_infoversion references.Similarly, confirm that
fetch_last_block_info_bytesandstore_last_block_info_bytesare fully tested and used. Lack of explicit invocation could lead to future code drift or untested features.packages/rs-drive-abci/src/abci/handler/offer_snapshot.rs (2)
32-63: Wipe and re-initialize logic looks correct.The flow for wiping GroveDB and starting a new snapshot syncing session is logically sound. Confirm that external references or caches to the freshly wiped database are properly reset to avoid stale references elsewhere in the code.
64-102: Validate height comparison and session re-initialization.Rejecting snapshots with heights less than or equal to the current session is sensible. Just ensure the user or system is aware of how multiple snapshot offers at different heights are orchestrated and that older snapshots will not cause erroneous rewinds.
packages/rs-drive-abci/src/abci/handler/finalize_block.rs (4)
1-4: Snapshot management trait import approved.Adding
SnapshotManagerApplicationto the imports is appropriate for the new snapshot feature. Good job maintaining modular design.
20-23: Expanded trait constraints are valid.Requiring the application to implement
SnapshotManagerApplicationensures that snapshot operations can be orchestrated here. This follows best practices for strengthening type guarantees.
103-104: Relaxed atomic storage seems acceptable.Storing the block height with
Ordering::Relaxedmight be sufficient for reading and writing in a single-threaded or lock-protected context. Ensure you have no concurrency requirements (e.g., cross-thread visibility) that warrant stronger ordering.
116-119: Potential race condition in state loading.After committing the block and creating a snapshot, the state is reloaded. If other asynchronous writes or reads occur, verify that the reloaded state is accurate and consistent for subsequent logic.
packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs (3)
1-10: Imports are well-structured and relevant
No issues are found with the selected imports. They match the functionalities used in this file (bincode, grove, dpp, etc.) and adhere to common Rust patterns.
70-84: ChunkData struct is well-defined
The fields—version, crc32, size, and chunk—are clear and serve their respective purposes. Documenting any versioning strategy (e.g., how CHUNK_VERSION changes in future updates) would help maintainers.
85-169: Thorough metadata handling in ChunkData methods
serializeproperly encodes data and logs errors.verifyfully checks version, size, and CRC to ensure integrity.deserializeintegrates the verification step to guarantee consistency before returning the object.This robust approach prevents corrupted or mismatched chunks from propagating.
packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (2)
24-27: Imports for specialized balances
These newly added imports enable refactoring specialized balances functionality. They appear well-organized and are referenced properly below. No issues noted.
231-323: Initialize withdrawal structures during transition to protocol version 4
This function properly creates the sum tree and broadcasted transactions subtree if missing, and updates masternode owner keys. Consider verifying:
- That newly inserted keys do not collide with existing keys, especially if concurrency is possible.
- Whether partial key updates can leave the identity in an inconsistent state.
Otherwise, the code appears logically consistent and follows a clear, stepwise approach.
packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (3)
18-20: New serialization imports introduced
The import of additional serialization traits (PlatformDeserializableFromVersionedStructure, PlatformSerializable, ReducedPlatformSerializable) aligns with the newly introduced extended or reduced serialization steps below. This usage is consistent.
32-33: Importing ReducedPlatformStateForSaving
The new references toReducedPlatformStateForSavingtypes facilitate more compact saving of platform state data. Ensure that the underlying logic for partial serialization remains parallel to the full version to avoid diverging states.
71-88: ReducedPlatformSerializable trait implementation
Implementingreduced_serialize_to_bytesis a clean addition, reusing standard bincode configuration. The error handling is consistent with the rest of the code. This improves code reuse for partial platform state outputs.packages/rs-drive-abci/src/abci/config.rs (2)
5-5: Use consistent import sorting or grouping if required by your style guide.
Currently,std::path::PathBufis introduced here. If your project enforces any particular grouping convention (e.g., standard library imports first), ensure that it aligns with the rest of the file.
83-87: Confirm the default environment is the correct fallback.
TheDefaultimplementation defers todefault_mainnet(). Verify that new local/test/staging environments won't unintentionally inherit the mainnet configuration.packages/rs-drive-abci/src/execution/engine/run_block_proposal/v1/mod.rs (2)
88-98: Check for consistent version negotiation logic.
The code checks whether the proposal’sconsensus_versions.appmatchesplatform_version.protocol_versiononly ifknown_from_usis false. Confirm that local or same-host proposals also need the same protocol version enforcement.
321-331: Validate concurrency impacts for large numbers of state transitions.
Ifraw_state_transitionscan become large, ensure that higher-level concurrency or memory usage is properly handled. Also consider adding performance tests or monitoring for potential bottlenecks in production.packages/rs-drive-abci/src/abci/app/check_tx.rs (3)
26-27: Confirm snapshot_manager initialization within CheckTx usage.
Ensure that the snapshot manager required byCheckTxAbciApplicationis fully needed during the check-tx phase. If this field will see minimal usage here, factor it into a smaller or more relevant context.
39-46: SnapshotManagerApplication trait usage is well-structured.
The approach of returning a reference tosnapshot_manager()is consistent and likely meets the trait’s requirements with minimal overhead.
53-64: Revisit max_num_snapshots and snapshots_frequency for test and local usage.
Although values are driven fromplatform.config.state_sync_config, confirm they are practical for performance duringcheck_tx.packages/rs-drive-abci/src/abci/app/mod.rs (2)
13-13: Ensure consistent error handling around snapshotsThe newly introduced imports for
SnapshotFetchingSessionandSnapshotManagersuggest additional snapshot-based workflows. Verify that all methods triggering or interacting with these types handle snapshot-related failures consistently, especially in downstream code.
33-39: Transaction lifetime parameter changesSwitching from
'ato'prefines lifetime scoping for transactions. Confirm that this change is reflected in all trait implementors, ensuring there are no mismatched lifetimes or leftover references to'a.packages/rs-drive-abci/tests/strategy_tests/state_sync.rs (2)
1-29: Validate module boundaryAdding a dedicated test module is good practice for grouping state sync tests. Ensure no concurrency issues arise if other test modules also manipulate the same network or file paths.
53-69: Potential infinite loop
get_target_folder()repeatedly pops directories until aCargo.lockis found. In rare cases, if missing, this could become an infinite loop at root. Consider a safety check or fallback to avoid that scenario.packages/rs-drive/Cargo.toml (1)
55-60: Pinning vs. branch referencesUsing
branch = "develop"for multiplegrovedbcrates may introduce version uncertainty. If a specific commit is needed for stability or reproducible builds, pin to a commit or tag. Otherwise, confirm that referencing a moving branch won’t cause unexpected dependency breaks.packages/rs-drive-abci/src/platform_types/snapshot/mod.rs (2)
21-23: CHUNK_SIZE_16MB is not referencedIf intended as a boundary for snapshot chunks, ensure it is enforced somewhere in the I/O path or remove unnecessary constants. Unused constants can cause confusion.
139-141: Skipping on non-multiple frequencies
if height == 0 || height % self.freq != 0 { return Ok(()); }effectively ignores all heights that aren’t multiples. Confirm this meets your sync strategy requirements (e.g., skipping block 1 with freq=2). Overlooking certain blocks might hamper debugging or replay scenarios.packages/rs-drive-abci/src/abci/app/full.rs (10)
1-3: **Clean integration of Snapshot traits **All newly introduced traits (
SnapshotFetchingApplication,SnapshotManagerApplication, etc.) are neatly imported and appear consistent with your design. No issues detected with these additions.
11-11: **Snapshot modules import looks good **Importing
SnapshotFetchingSessionandSnapshotManagerhere is straightforward and aligns with the newly introduced functionality.
30-33: **Concurrent data structures are acceptable, but verify thread safety **Storing the
snapshot_fetching_sessionunder anRwLock<Option<SnapshotFetchingSession<'a>>>looks sound for concurrency. However, ifsnapshot_manageralso needs concurrent access (reads/writes from different threads), consider placing it under a lock or confirming that its operations are already thread-safe.
48-49: **Initialization looks standard **Using
Default::default()forsnapshot_fetching_sessionand assigningsnapshot_manageris a typical, concise approach. No issues here.
60-64: **SnapshotManagerApplication trait implementation is concise **The method simply returns the
snapshot_manager. This is well-aligned with the trait’s contract and appears correct.
66-74: **SnapshotFetchingApplication implementation is consistent **Returning a reference to the
snapshot_fetching_sessionlock and platform is coherent. The lifetime usage in this context seems correct.
185-190: **Snapshot offering handler invocation **Delegating
offer_snapshotto thehandler::offer_snapshotfunction is succinct and clear. Ensure that the handler function has comprehensive validation and logging for incoming workflow segments.
192-197: **Snapshot chunk application handler **Forwarding
apply_snapshot_chunkto the handler is well-structured. It’s good that error mapping propagates any internal issues up as exceptions.
199-204: **Snapshot listing logic **Similarly, delegating to
handler::list_snapshotsis straightforward. Looks good.
206-211: **Loading snapshot chunks **Invoking
handler::load_snapshot_chunkis consistent with the snapshot functionality introduced above. Implementation is concise and aligned with the existing pattern.packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs (4)
1-37: **Imports reviewed, all appear relevant **All newly added imports (e.g.,
ChunkData,ExtendedQuorumListResult, etc.) look necessary for snapshot chunk processing and platform state reconstruction.
269-305: **Hard-coded parameter forget_protx_diff_with_masternodes**Using
Some(1)as the base block height may be intended, but might also be incidental:.get_protx_diff_with_masternodes(Some(1), core_block_height)?;Double-check if this is the desired behavior or a placeholder. If it’s a placeholder, expose it as a configurable parameter or document its usage.
307-357: **Quorum verification set construction **Fetching quorums and building
VerificationQuorumobjects is performed cleanly. Error handling for missing quorums is appropriate. This logic is robust.
359-436: **Validator list assembly appears solid **Collecting validator sets, sorting them by core height (with a tie-break on hash), and appending them to the platform state is well-organized. No immediate improvements needed aside from ensuring thorough testing of edge cases.
packages/rs-drive-abci/src/abci/app/consensus.rs (11)
1-4: **Imports for application traits **Bringing in
BlockExecutionApplication,PlatformApplication, etc. is consistent with howFullAbciApplicationwas handled. No issues noted.
11-11: **Snapshot modules import validated **Adding
SnapshotFetchingSessionandSnapshotManagerinto scope aligns with the new state sync logic.
13-13: **GRPC integration note **The
use dapi_grpc::tonic;import might hint at possible expansions for gRPC-based calls. No problems here.
38-53: **Snapshot manager initialization **Similar pattern to
FullAbciApplication—the snapshot manager is correctly instantiated from config. Straightforward and consistent.
56-60: **PlatformApplication trait alignment **Returning a reference to the underlying
platformis consistent with the trait definition.
62-66: **SnapshotManagerApplication trait alignment **The method simply exposes
snapshot_manager. No issues.
68-76: **SnapshotFetchingApplication trait usage **Declaration is identical to
FullAbciApplication, returning theRwLocksession plus aplatformreference. Looks good.
78-82: **BlockExecutionApplication trait usage **No special differences from other apps. The concurrency pattern with
block_execution_contextremains consistent.
84-111: **TransactionalApplication logic is standard **Starting and committing transactions is consistent with the approach used in
FullAbciApplication. No anomalies.
113-117: **Debug trait for easy inspection **Implementing
Debugwith a simple placeholder is typical and helpful for debugging. No concerns.
119-199: **New snapshot ABCI methods **Adding
offer_snapshotandapply_snapshot_chunkfosters snapshot management in consensus mode. Implementation defers to handler functions, matching the approach inFullAbciApplication.
packages/rs-drive/src/drive/platform_state/fetch_last_block_info_bytes/v0/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/v0/mod.rs
Show resolved
Hide resolved
packages/rs-drive-abci/src/execution/storage/fetch_last_block_info/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive-abci/src/execution/engine/run_block_proposal/v1/mod.rs
Show resolved
Hide resolved
packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Dockerfile (3)
176-176: Check Argument Naming Consistency for sccache VersionThe updated argument
ARG SCCHACHE_VERSION=0.9.1correctly upgrades the sccache version. However, note the spelling “SCCHACHE_VERSION” whereas the common convention is “SCCACHE_VERSION”. Please verify that this naming is intentional and consistent across related configurations.
597-603: Ensure Directory Creation with a TODO ReminderThe
RUN mkdir -p ...command now creates all the required directories—including/var/log/dash,${DB_PATH},${CHECKPOINTS_PATH},${REJECTIONS_PATH}, and/var/lib/dash-platform/data/checkpoints. The TODO note indicates that/var/lib/dash-platform/data/checkpointsshould be removed once drive-abci is fixed. Please ensure that this workaround is tracked and eventually resolved in your issue management process.
614-617: User Creation and Ownership UpdateThe commands for adding the user and updating directory ownership now include
/var/lib/dash-platform/data/checkpoints(as noted in the TODO). Confirm that including this path in the ownership change aligns with the deployment strategy and that the pending removal of this directory is addressed in a future fix.packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (1)
71-87: Well-structured implementation of ReducedPlatformSerializableThe implementation follows the same pattern as the existing
PlatformSerializabletrait, providing a clean way to serialize the platform state in a reduced format.Consider adding documentation explaining when reduced serialization should be used versus full serialization, which would improve maintainability.
📜 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 (10)
Dockerfile(3 hunks)packages/dapi-grpc/Cargo.toml(1 hunks)packages/rs-dapi-client/Cargo.toml(1 hunks)packages/rs-dpp/src/lib.rs(1 hunks)packages/rs-drive-abci/Cargo.toml(2 hunks)packages/rs-drive-abci/src/platform_types/platform_state/mod.rs(4 hunks)packages/rs-drive-abci/src/platform_types/platform_state/v0/mod.rs(3 hunks)packages/rs-drive-abci/tests/strategy_tests/main.rs(1 hunks)packages/rs-drive-proof-verifier/Cargo.toml(1 hunks)packages/rs-drive/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/rs-dpp/src/lib.rs
- packages/rs-dapi-client/Cargo.toml
- packages/rs-drive-abci/tests/strategy_tests/main.rs
- packages/rs-drive-abci/src/platform_types/platform_state/v0/mod.rs
- packages/rs-drive-proof-verifier/Cargo.toml
- packages/rs-drive-abci/Cargo.toml
- packages/rs-drive/Cargo.toml
🧰 Additional context used
🧠 Learnings (2)
Dockerfile (1)
Learnt from: lklimek
PR: dashpay/platform#2318
File: Dockerfile:160-198
Timestamp: 2024-11-13T10:31:31.084Z
Learning: In the `Dockerfile` for the rs-drive-abci project, when configuring sccache, we do not need to add validation checks for configuration variables, as sccache performs validation at runtime.
packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-11-12T14:56:12.334Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (json-schema-compatibility-validator) / Linting
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Formatting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- 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 Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Rust crates security audit
🔇 Additional comments (4)
Dockerfile (1)
587-587: New Checkpoints Path Environment VariableThe addition of
ENV CHECKPOINTS_PATH=/var/lib/dash/rs-drive-abci/db-checkpointsimproves the clarity for managing database checkpoints in the Drive ABCI setup. Ensure that the application components reference this variable appropriately.packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (2)
17-19: Import changes look goodThe new imports for
ReducedPlatformSerializableproperly support the state sync functionality being added.
32-33: Import for reduced platform state types are properly addedThese imports correctly reference the new reduced platform state types that will be used for state synchronization.
packages/dapi-grpc/Cargo.toml (1)
34-48:✅ Verification successful
Resolve Duplicate Dependency Definitions for
tenderdash-protoIt appears that
tenderdash-protois defined twice in the[dependencies]section—once at line 34 and then again at lines 45–48 with additional features. TOML does not support duplicate keys in the same table, which may lead to unexpected behavior. Please merge these definitions into a single entry that includes the necessary features.For example, you might update the earlier definition (line 34) and remove the duplicated entry at lines 45–48 as follows:
- tenderdash-proto = { git = "https://github.com/dashpay/rs-tenderdash-abci", version = "1.3.0", tag = "v1.3.0+1.3.0", default-features = false } + tenderdash-proto = { git = "https://github.com/dashpay/rs-tenderdash-abci", version = "1.3.0", tag = "v1.3.0+1.3.0", default-features = false, features = ["grpc", "serde"] }This change ensures a single, consistent configuration for the dependency and avoids potential future conflicts.
Action Required: Merge Duplicate
tenderdash-protoEntries in Cargo.toml
- The file
packages/dapi-grpc/Cargo.tomlcontains two definitions fortenderdash-proto(one starting at line 34 and another at lines 45–48).- TOML does not allow duplicate keys within the same table, which can lead to unpredictable behavior.
- To resolve this, merge the definitions into a single entry that includes the necessary
features(i.e.,"grpc"and"serde").For example, update:
- tenderdash-proto = { git = "https://github.com/dashpay/rs-tenderdash-abci", version = "1.3.0", tag = "v1.3.0+1.3.0", default-features = false } + tenderdash-proto = { git = "https://github.com/dashpay/rs-tenderdash-abci", version = "1.3.0", tag = "v1.3.0+1.3.0", default-features = false, features = ["grpc", "serde"] }This change ensures a single, consistent configuration for the dependency and avoids potential future conflicts.
🏁 Script executed:
#!/bin/bash # Search for all occurrences of "tenderdash-proto" in the Cargo.toml file rg --context 2 'tenderdash-proto' packages/dapi-grpc/Cargo.tomlLength of output: 1113
Merge Duplicate
tenderdash-protoDependency Definition in Cargo.tomlThe shell script output confirms that there are two definitions for
tenderdash-protoin the[dependencies]section. TOML does not support duplicate keys, which can lead to unexpected behavior. To resolve this, please merge the duplicate entries into a single definition that includes the necessary features.For example, update the earlier definition as follows:
- tenderdash-proto = { git = "https://github.com/dashpay/rs-tenderdash-abci", version = "1.3.0", tag = "v1.3.0+1.3.0", default-features = false } + tenderdash-proto = { git = "https://github.com/dashpay/rs-tenderdash-abci", version = "1.3.0", tag = "v1.3.0+1.3.0", default-features = false, features = ["grpc", "serde"] }This change ensures a single, consistent configuration for the dependency and avoids any potential future conflicts.
packages/rs-drive-abci/src/platform_types/platform_state/mod.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs (4)
99-100: Address the TODO for refetch and reject senders.
Refetching chunks or rejecting senders may be necessary for partial failures or malicious peers. Completing this logic will ensure robust snapshot handling.Do you want help drafting the logic for refetching chunks or rejecting senders?
158-274: Consider version alignment.
The calls toPlatformVersion::latest()might cause unintended behavior if the platform version changes during the synchronization phase. Storing and reusing a “negotiated” platform version for the entire sync process can eliminate potential mismatches.
276-312: Future-proof masternode type handling.
Currently, onlyMasternodeType::Evois recognized as HPMNs. If more masternode types become relevant later, consider a more flexible approach (e.g., a match statement) that will scale with additional types.
366-443: Streamline fall-through in sorting logic.
When sorting validator sets by core block height and then by hash, double-check that re-comparing the block height in thethen_withclause (lines 435-436) is needed—it might be redundant.
📜 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)
packages/dapi-grpc/Cargo.toml(1 hunks)packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs(1 hunks)packages/rs-drive/src/drive/platform_state/fetch_last_block_info_bytes/v0/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-drive/src/drive/platform_state/fetch_last_block_info_bytes/v0/mod.rs
- packages/dapi-grpc/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (json-schema-compatibility-validator) / Linting
- GitHub Check: Rust packages (json-schema-compatibility-validator) / Tests
- GitHub Check: Rust packages (json-schema-compatibility-validator) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- 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
🔇 Additional comments (4)
packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs (4)
1-37: No issues found in imports.
The import statements appear consistent and necessary for the file’s functionality.
50-60: Solid concurrency handling with RwLock.
Acquiring the write lock on the session here prevents data races and ensures safe, sequential updates to the state sync session.
104-156: Clean completion and verification flow.
Upon detecting state sync is done, you consume the session, commit it, and verify the updated database. This well-structured block ensures that incomplete or corrupted state does not progress.
314-364: Validate quorum set size growth.
This code only extendscurrent_quorums_mut()without accommodating removal of outdated/unused quorums. Verify that unrestricted growth incurrent_quorumsover time is intended or necessary.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs (4)
50-56: Ensure robust lock error handling when the RwLock is poisoned.
While you are correctly returning an error if the lock is poisoned, consider adding context logs or metrics to aid debugging if lock poisoning becomes frequent.
96-101: Clarify the usage ofrefetch_chunksandreject_senders.
Currently, these vectors are always empty. If you plan to implement chunk refetch or sender rejection based on failed validation, add handling logic or remove the TODO comments to avoid confusion.
276-312: Unit test coverage for masternode list rebuilding is advisable.
This code builds and extends masternode lists but relies heavily oncore_rpccalls. Consider adding mock-based unit tests confirming that onlyEvonodes are added to the HPMN list, for instance, to reduce future regression.Would you like me to suggest a sample test that uses a mock Core RPC client to validate
build_masternode_lists?
426-439: Double-check reversed order in validator set sorting.
You appear to sort by descendingcore_heightfirst (quorum_b.core_height()vs.quorum_a.core_height()) which results in the largest height first. If your code or your system logic expects ascending block height order, ensure that the order is correct.
📜 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)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Rust packages (wasm-dpp) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Detect immutable structure changes
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (dpp) / Check each feature
- 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/apply_snapshot_chunk.rs (4)
125-145: Validateincorrect_hasheshandling and enforce final platform consistency checks.
It’s good to detect incorrect Grovedb hashes, but this block might benefit from an explicit verification path or logging that details which hashes are out of sync. This could significantly reduce the effort needed to troubleshoot if data corruption occurs.
187-203: Confirm if zeroed fields in the block info are acceptable.
All of these block fields (quorum_hash,block_id_hash,proposer_pro_tx_hash,signature,round) default to zero arrays. If this is intentional for newly created sync states, consider adding a comment or referencing the reason. If it’s unintentional, ensure these fields are correctly populated.
190-197: Good handling ofapp_hashconversion without panicking.
By using manual error handling and returning anAbciError, you address the previous feedback about avoiding.unwrap(). This approach improves robustness against unexpected hash lengths.
400-409: Check stable sorting requirements for quorums.
When you sort byheightand then byhash, confirm that the sort is stable if that’s required for determining a deterministic ordering. Some Rust sorting algorithms are stable, but verifying it is crucial for consensus-critical data.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 14
🔭 Outside diff range comments (7)
packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_transition.rs (1)
89-95:⚠️ Potential issueDuplicate UnfreezeAction match arm
There are two identical match arms for
TokenTransitionAction::UnfreezeActionat lines 89-91 and 92-95. This is likely a mistake, as the first arm has incorrect variable usage (freeze_actioninstead ofunfreeze_action).- TokenTransitionAction::UnfreezeAction(unfreeze_action) => TokenEvent::Unfreeze( - freeze_action.public_note().cloned(), - ), TokenTransitionAction::UnfreezeAction(unfreeze_action) => TokenEvent::Unfreeze( unfreeze_action.frozen_identity_id(), unfreeze_action.public_note().cloned(), ),packages/rs-drive/src/state_transition_action/action_convert_to_operations/batch/token/token_freeze_transition.rs (2)
60-63:⚠️ Potential issueDuplicated code in GroupAction initialization
Line 60 and 61 both contain
Some(GroupAction::V0(GroupActionV0 {, which appears to be a duplication issue.- Some(GroupAction::V0(GroupActionV0 { Some(GroupAction::V0(GroupActionV0 { event: GroupActionEvent::TokenEvent(event), }))
93-95:⚠️ Potential issueDuplicated closing brackets
There are duplicated closing brackets
}));at lines 93 and 94, which could cause syntax errors.), - })); }));packages/rs-dpp/src/data_contract/associated_token/token_configuration/v0/mod.rs (1)
62-64: 🛠️ Refactor suggestionRemove unused default function
The
default_keeps_history()function is no longer used since the field type has changed frombooltoTokenKeepsHistoryRules, but it's still present in the code.-// Default function for `keeps_history` -fn default_keeps_history() -> bool { - true // Default to `true` for keeps_history -}packages/rs-dpp/src/errors/consensus/state/token/mod.rs (1)
32-33:⚠️ Potential issueRemove duplicate exports
Lines 32-33 duplicate the exports already present on lines 30-31.
pub use token_setting_max_supply_to_less_than_current_supply_error::*; pub use unauthorized_token_action_error::*; -pub use token_setting_max_supply_to_less_than_current_supply_error::*; -pub use unauthorized_token_action_error::*;packages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/add_distribute_fees_from_oldest_unpaid_epoch_pool_to_proposers_operations/mod.rs (1)
70-70:⚠️ Potential issueUpdate known_versions to include version 1
The error handling mentions only version 0 in the
known_versionsvector, but the code now supports version 1 as well.- known_versions: vec![0], + known_versions: vec![0, 1],packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/v1/v0_methods.rs (1)
650-660:⚠️ Potential issueFix syntax error and consolidate object initialization.
The current code has mismatched braces and repeated fields that introduce a syntax error. The intention seems to be to initialize all fields within a single block.Below is a suggested fix that merges lines 654–660 into the initial object literal:
-let mut freeze_transition = TokenFreezeTransition::V0(TokenFreezeTransitionV0 { - token_id, - using_group_info: None, -}), - identity_to_freeze_id: frozen_identity_id, - public_note, -}); +let mut freeze_transition = TokenFreezeTransition::V0(TokenFreezeTransitionV0 { + token_id, + using_group_info: None, + identity_to_freeze_id: frozen_identity_id, + public_note, +});
🧹 Nitpick comments (52)
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/validation/validate_basic_structure/v0/mod.rs (1)
145-174: Consider consolidating the repetitive match arms or adopting a data-driven approach.Each transition variant calls its own
validate_structure, which is appropriate for type-specific logic. However, this approach can lead to repetitive boilerplate if more variants are added in the future. You may consider unifying these calls through a trait-based or data-driven pattern, reducing repetition and improving maintainability.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_emergency_action_transition/validate_structure/mod.rs (2)
9-14: Consider adding documentation comments.The trait definition is well-structured, but lacks documentation comments. Adding rustdoc comments (///`) would improve code readability and maintainability by explaining the purpose of this trait and its method.
+/// Trait for validating the structure of token emergency action transitions. pub trait TokenEmergencyActionTransitionStructureValidation { + /// Validates the structure of a token emergency action transition. + /// + /// # Arguments + /// + /// * `platform_version` - The platform version to use for validation. + /// + /// # Returns + /// + /// A Result containing either a SimpleConsensusValidationResult or a ProtocolError. fn validate_structure( &self, platform_version: &PlatformVersion, ) -> Result<SimpleConsensusValidationResult, ProtocolError>; }
16-36: Implementation looks solid with proper version handling.The implementation appropriately uses the platform version to determine which validation method to apply. The error handling for unknown versions is comprehensive, providing clear context about the supported versions and the received version.
However, consider adding a brief documentation comment here as well to explain the implementation details.
+/// Implementation of structure validation for TokenEmergencyActionTransition. +/// Currently supports version 0 of the platform. impl TokenEmergencyActionTransitionStructureValidation for TokenEmergencyActionTransition { fn validate_structure( &self, platform_version: &PlatformVersion, ) -> Result<SimpleConsensusValidationResult, ProtocolError> {packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_claim_transition_action/mod.rs (1)
15-25: Definition ofTokenClaimTransitionActionValidationtrait.
This properly mirrors the validation trait structure found in other transitions. You might later consider adding a structure-level validation method if it becomes necessary, mirroring the pattern in document transitions.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/document/document_replace_transition_action/mod.rs (1)
12-12: Module transition from structure_v0 to advanced_structure_v0The refactoring from
structure_v0toadvanced_structure_v0suggests an enhancement in the validation logic. This change appears to be part of a broader coordinated refactoring effort across multiple document transition actions.Consider adding a brief comment explaining the key differences or improvements that the advanced structure validation provides compared to the previous implementation.
Also applies to: 15-15
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/document/document_create_transition_action/mod.rs (1)
13-13: Module transition from structure_v0 to advanced_structure_v0The change from
structure_v0toadvanced_structure_v0is consistent with the refactoring seen in other document transition action modules. This suggests a systematic enhancement to the validation framework.Consider documenting the rationale behind this architectural change and any new features or improvements provided by the advanced structure implementation.
Also applies to: 16-16
packages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/get.ts (1)
1-61: Code looks good but missing documentation for metadata removalBased on the PR summary and AI-generated information, it appears that this file previously had logic for handling contract metadata (importing
Metadatafrom@dashevo/wasm-dppand setting metadata on the contract) which has been removed. While the current implementation looks functionally correct, it would be helpful to document this change with a comment explaining why metadata handling was removed and any potential impacts on downstream consumers.Consider adding a comment near the contract creation (around line 44-45) to document the intentional removal of metadata handling:
const contract = await this.dpp.dataContract .createFromBuffer(dataContractResponse.getDataContract() as Uint8Array); + // Note: Metadata handling was removed as part of ABCI state sync implementationpackages/rs-dpp/src/data_contract/associated_token/token_keeps_history_rules/mod.rs (1)
1-26: Well-structured enum for token history rulesThe new
TokenKeepsHistoryRulesenum is well-structured with appropriate derive macros for serialization/deserialization and encoding/decoding. The implementation of theDisplaytrait provides a clean way to format the output.One minor suggestion would be to consider adding some documentation comments for the enum to explain its purpose and usage.
Consider adding documentation for the enum:
+/// Represents rules governing whether a token keeps history +/// +/// This enum wraps version-specific implementations of token history rules #[derive(Serialize, Deserialize, Encode, Decode, Debug, Clone, Copy, PartialEq, Eq, From)] #[serde(tag = "$format_version")] pub enum TokenKeepsHistoryRules {packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/evaluate_interval.rs (1)
7-40: Comprehensive interval reward computation with clear documentation.
Therewards_in_intervalmethod is well-structured, especially with the doc comments and parameter explanations. Consider adding a test or example usage that covers edge cases (e.g., zero-length intervals, partial block intervals) to ensure correctness across boundary conditions.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs (1)
13-877: Centralized validation logic is well designed but somewhat repetitive.
Your approach comprehensively validates parameters for all distribution function variants. However, multiple checks (e.g., constructing an error result, verifying parameter bounds) recur in each match arm. Extracting shared logic into a helper method could reduce duplication and improve maintainability.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/methods/mod.rs (1)
921-977: New token claim transition method implemented.The method follows the same pattern as other token operations in this file:
- Properly handles version checking
- Passes all necessary parameters to the implementation
- Returns appropriate error on version mismatch
However, there's a minor issue in the comment at line 953:
- // Create the emergency action transition for batch version 1 + // Create the token claim transition for batch version 1packages/rs-dpp/src/errors/consensus/state/token/invalid_token_claim_property_mismatch.rs (1)
9-21: New error type structure defined correctly.The
InvalidTokenClaimPropertyMismatchstruct is properly defined with appropriate derives for error handling, serialization, and deserialization.However, there's a discrepancy between the struct name and the error message:
-#[error( - "Invalid token release property mismatch for '{}', token ID: {}", - property, - token_id -)] +#[error( + "Invalid token claim property mismatch for '{}', token ID: {}", + property, + token_id +)]The error message mentions "token release" while the struct name indicates "token claim".
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_claim_transition/validate_structure/v0/mod.rs (1)
14-29: Well-implemented validation logic for token claiming.The implementation correctly validates that the public note (if present) doesn't exceed the maximum length. Error handling is well-structured and returns appropriate validation results.
One additional validation you might consider is checking if the token amount exceeds the maximum allowable value, similar to the mint transition validation, if that's applicable for token claims.
packages/rs-dpp/src/data_contract/methods/equal_ignoring_time_based_fields/mod.rs (1)
1-42: Consider adding unit tests.As this is functionality for comparing contracts while ignoring specific fields, it's important to have comprehensive test coverage. Ensure there are tests for both positive and negative cases, including edge cases where only time-based fields differ.
#!/bin/bash # Check if there are tests for this functionality rg -A 3 "test.*equal_ignoring_time" --glob "*.rs" packages/rs-dpp/packages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/add_distribute_fees_from_oldest_unpaid_epoch_pool_to_proposers_operations/v0/mod.rs (1)
79-87: Clarify naming for the returned collection.Here,
proposers_paid_countseems to be a collection rather than a numeric count (the code references.1and later uses.len()). Consider renaming this variable or storing the length immediately for improved clarity and consistency.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs (2)
20-43: Confirm that the custom PRF usage forRandomis acceptable.
- This pseudo-random generator appears non-cryptographic. If cryptographic security is needed, consider a more robust RNG.
- Because the seed is
x, the same period always produces the same result, which may or may not be desired.
131-198: Polynomial distribution can be refactored to reduce repetition.
- Each distribution variant similarly clamps to min/max, checks for overflows, etc. Consider factoring out repeated min/max clamping logic into a helper method.
- Watch out for floating-point exponent rounding. The code does attempt to handle large values gracefully, but thorough domain testing is recommended.
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_config_update_transition/validate_structure/mod.rs (1)
1-7: Import ordering could be standardizedWhile functionally correct, the imports are organized differently from other similar modules (for example, having
mod v0;at the end rather than after imports). Consider standardizing the import order across all modules for better consistency.use platform_version::version::PlatformVersion; use crate::ProtocolError; use crate::state_transition::batch_transition::token_config_update_transition::validate_structure::v0::TokenConfigUpdateTransitionStructureValidationV0; use crate::state_transition::batch_transition::TokenConfigUpdateTransition; use crate::validation::SimpleConsensusValidationResult; -mod v0; + +mod v0;packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_burn_transition/validate_structure/mod.rs (1)
1-7: Same naming inconsistency as in the mint transition moduleSimilar to the token mint transition module, there's a naming inconsistency with
TokenBurnTransitionActionStructureValidationV0(includes "Action" in the name) compared to other validation types. Consider standardizing the naming pattern across all transition types for better code consistency.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_claim_transition/validate_structure/mod.rs (1)
1-8: Consider standardizing module and import orderIn this file, imports appear before the module declaration, while in other similar modules (like token_freeze_transition), the module declaration appears first. Consider standardizing this pattern across all similar validation modules for better consistency.
+mod v0; + use crate::state_transition::batch_transition::token_claim_transition::validate_structure::v0::TokenClaimTransitionActionStructureValidationV0; use crate::state_transition::batch_transition::TokenClaimTransition; use crate::validation::SimpleConsensusValidationResult; use crate::ProtocolError; use platform_version::version::PlatformVersion; -mod v0;packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/resolvers/v0/mod.rs (1)
26-30: Maintain consistent formatting between methodsThere appear to be inconsistent blank lines between the newly added methods. While the added methods themselves follow the established interface pattern correctly, consider standardizing the spacing between all methods in the trait for better readability.
fn as_transition_token_destroy_frozen_funds( &self, ) -> Option<&TokenDestroyFrozenFundsTransition>; fn as_transition_token_claim(&self) -> Option<&TokenClaimTransition>; fn as_transition_token_emergency_action(&self) -> Option<&TokenEmergencyActionTransition>; fn as_transition_token_config_update(&self) -> Option<&TokenConfigUpdateTransition>;Either use consistent newlines between all methods or remove extra newlines for consistency.
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs (1)
18-19: Fix parameter name typoThere's a typo in the parameter name:
data_contact_configshould bedata_contract_config.- data_contact_config: &DataContractConfig, + data_contract_config: &DataContractConfig,packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition_action_type.rs (1)
66-66: Check case sensitivity for 'claim'.
This lowercase string matches the pattern used by other variants (e.g. "burn"). Confirm if any uppercase or alternative forms need handling as well.packages/rs-dpp/src/data_contract/config/mod.rs (1)
65-65: Remove or refine the TODO comment.
“// TODO: Remove, it’s not using” likely indicates dead code or an unimplemented stub.packages/rs-dpp/src/data_contract/serialized_version/v1/mod.rs (1)
42-54: Added optional fields for timestamps and block-related data.
Providing finer-grained lifecycle metadata is valuable. Consider validating these fields to ensure creation precedes update.packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/mod.rs (2)
18-32: Documentation needs updating to match new parameters.The method documentation still references the removed boolean parameters (
documents_keep_history_contract_default,documents_mutable_contract_default) but doesn't mention the newdata_contact_configparameter.Update the documentation to reflect the current parameter list and explain the purpose of the
data_contact_configparameter. Also fix the typo in the parameter name (should be "contract" not "contact").-/// documents keep history contract default, documents mutable contract default, +/// data contract configuration, /// and a platform version as input parameters and returns a map of document types /// extracted from the provided value array. /// /// The process of retrieving document types is versioned, and the version is determined /// by the platform version parameter. If the version is not supported, an error is returned. /// /// # Parameters /// /// * `data_contract_id`: Identifier for the data contract. /// * `contract_document_types_raw`: Vector representing the raw contract document types. /// * `definition_references`: BTreeMap representing the definition references. -/// * `documents_keep_history_contract_default`: A boolean flag that specifies the document's keep history contract default. -/// * `documents_mutable_contract_default`: A boolean flag that specifies the document's mutable contract default. +/// * `data_contract_config`: Configuration object containing document defaults and settings. /// * `platform_version`: The platform version being used.
42-42: Typo in parameter name.There's a typo in the parameter name
data_contact_config(should be "contract" not "contact").- data_contact_config: &DataContractConfig, + data_contract_config: &DataContractConfig,packages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/add_distribute_fees_from_oldest_unpaid_epoch_pool_to_proposers_operations/mod.rs (1)
22-33: Update function documentation to include the new parameterThe function documentation doesn't mention the newly added
total_distributed_storage_feesparameter./// * `current_epoch_index` - A u16 indicating the current epoch index. /// * `cached_current_epoch_start_block_height` - An Option wrapping a u64 value representing the current epoch start block height. /// * `cached_current_epoch_start_block_core_height` - An Option wrapping a u32 value representing the current epoch start block core height. + /// * `total_distributed_storage_fees` - The total amount of distributed storage fees in Credits. /// * `transaction` - A Transaction reference. /// * `batch` - A mutable reference to a vector of DriveOperation. /// * `platform_version` - A PlatformVersion reference that dictates which version of the method to call.packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/v1/v0_methods.rs (1)
81-81: Remove duplicate import.
The import ofPrivateEncryptedNoteandSharedEncryptedNotehere duplicates the import on line 85, causing unnecessary repetition.-#[cfg(feature = "state-transition-signing")] -use crate::tokens::{PrivateEncryptedNote, SharedEncryptedNote};packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transfer_transition/validate_structure/v0/mod.rs (1)
1-85: Consider extracting repeated note-check logic to reduce duplication.
The structure validation duplicates similar checks forpublic_note,shared_encrypted_note, andprivate_encrypted_note. You could refactor these into a helper function that simplifies checking note lengths.Example of a possible helper:
+fn check_note_size(note: &[u8], note_name: &str) -> Option<ConsensusError> { + if note.len() > MAX_TOKEN_NOTE_LEN { + Some(ConsensusError::BasicError( + BasicError::InvalidTokenNoteTooBigError( + InvalidTokenNoteTooBigError::new( + MAX_TOKEN_NOTE_LEN as u32, + note_name, + note.len() as u32, + ), + ), + )) + } else { + None + } +} fn validate_structure_v0(&self, owner_id: Identifier) -> Result<SimpleConsensusValidationResult, ProtocolError> { // ... if let Some(public_note) = self.public_note() { - if public_note.len() > MAX_TOKEN_NOTE_LEN { - ... + if let Some(err) = check_note_size(public_note.as_bytes(), "public_note") { + return Ok(SimpleConsensusValidationResult::new_with_error(err)); } } // ... }packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate_interval.rs (1)
61-68: Consider checked addition for iteration variable.
The loop incrementsxbyx += step;, which may risk an overflow if both values are large near the upper bound ofu64. Use a checked addition to ensure safety:-while x <= end_included { - total = total.checked_add(self.evaluate(x)?).ok_or_else(|| { - ProtocolError::Overflow("Total evaluation overflow in evaluate_interval".into()) - })?; - x += step; -} +while x <= end_included { + total = total.checked_add(self.evaluate(x)?).ok_or_else(|| { + ProtocolError::Overflow("Total evaluation overflow in evaluate_interval".into()) + })?; + x = x.checked_add(step).ok_or_else(|| { + ProtocolError::Overflow("Iteration overflow in evaluate_interval".into()) + })?; +}packages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/add_distribute_fees_from_oldest_unpaid_epoch_pool_to_proposers_operations/v1/mod.rs (1)
47-55: Ensure no overflow in height increments.
When incrementingstart_block_core_height + 1, consider a checked add if there's a chance of hitting the u32 maximum height. This can help avoid subtle overflow bugs.-let start_block_core_height = if unpaid_epoch.epoch_index == 0 { - self.drive - .fetch_genesis_core_height(Some(transaction), platform_version)? -} else { - unpaid_epoch.start_block_core_height + 1 -}; +let start_block_core_height = if unpaid_epoch.epoch_index == 0 { + self.drive + .fetch_genesis_core_height(Some(transaction), platform_version)? +} else { + unpaid_epoch.start_block_core_height + .checked_add(1) + .ok_or_else(|| Error::Overflow("start_block_core_height + 1 overflowed".to_string()))? +};packages/rs-dpp/src/data_contract/config/v1/mod.rs (1)
131-139: Validate potential out-of-range inputs more thoroughly.
When extracting bounded key requirements from integer fields, confirm whether additional checks (e.g., max allowed value) are necessary to prevent invalid data from entering the system.packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v0/mod.rs (1)
18-18: Rename parameter for consistency with theDataContractConfigtype.It appears the parameter name has a minor typo. Consider renaming:
- data_contact_config: &DataContractConfig, + data_contract_config: &DataContractConfig,packages/rs-dpp/src/data_contract/document_type/property/mod.rs (2)
1-1: Remove unused import to eliminate lint warnings.- use platform_value::btreemap_extensions::{BTreeValueMapHelper, BTreeValueMapPathHelper}; + use platform_value::btreemap_extensions::BTreeValueMapHelper;Also applies to: 9-11, 18-19
2181-2191: Collapse nested if block for cleaner code.Apply this diff to simplify the nested else-if:
- } else { - if min >= i8::MIN as i64 && max <= i8::MAX as i64 { - DocumentPropertyType::I8 - } else if min >= i16::MIN as i64 && max <= i16::MAX as i64 { - DocumentPropertyType::I16 - } else if min >= i32::MIN as i64 && max <= i32::MAX as i64 { - DocumentPropertyType::I32 - } else { - DocumentPropertyType::I64 - } - } + } else if min >= i8::MIN as i64 && max <= i8::MAX as i64 { + DocumentPropertyType::I8 + } else if min >= i16::MIN as i64 && max <= i16::MAX as i64 { + DocumentPropertyType::I16 + } else if min >= i32::MIN as i64 && max <= i32::MAX as i64 { + DocumentPropertyType::I32 + } else { + DocumentPropertyType::I64 + }🧰 Tools
🪛 GitHub Check: Rust packages (dpp) / Linting
[warning] 2181-2191: this
else { if .. }block can be collapsed
warning: thiselse { if .. }block can be collapsed
--> packages/rs-dpp/src/data_contract/document_type/property/mod.rs:2181:12
|
2181 | } else {
| _______^
2182 | | if min >= i8::MIN as i64 && max <= i8::MAX as i64 {
2183 | | DocumentPropertyType::I8
2184 | | } else if min >= i16::MIN as i64 && max <= i16::MAX as i64 {
... |
2191 | | }
| |^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_else_if
= note:#[warn(clippy::collapsible_else_if)]on by default
help: collapse nested if block
|
2181 ~ } else if min >= i8::MIN as i64 && max <= i8::MAX as i64 {
2182 + DocumentPropertyType::I8
2183 + } else if min >= i16::MIN as i64 && max <= i16::MAX as i64 {
2184 + DocumentPropertyType::I16
2185 + } else if min >= i32::MIN as i64 && max <= i32::MAX as i64 {
2186 + DocumentPropertyType::I32
2187 + } else {
2188 + DocumentPropertyType::I64
2189 + }
|packages/rs-dpp/src/errors/consensus/basic/data_contract/invalid_token_distribution_function_invalid_parameter_error.rs (1)
37-62: Consider adding targeted unit tests
While the constructor and accessor methods look straightforward, you might want to add tests that validate edge cases—e.g., whenmin>maxor whennot_validis None vs. Some value—to confirm the error formatting and serializing behaviors are correct.packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v1/mod.rs (1)
2-3: Verify naming consistency for the new parameter
The parameter namedata_contact_configdiffers slightly from theDataContractConfigstruct (contact vs. contract). Consider renaming it todata_contract_configfor clarity:- data_contact_config: &DataContractConfig, + data_contract_config: &DataContractConfig,Also applies to: 18-44
packages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/add_epoch_pool_to_proposers_payout_operations/v0/mod.rs (2)
77-81: Trace logging
Including total payouts, number of proposers, and block counts in this trace message is helpful. You could optionally expand the log to detail each fee category so that debugging is even more transparent.
302-303: Ignoring the first element of the returned tuple
In the test, only.1is retrieved and validated. You might consider asserting the correctness of thestorage_and_processing_feesdata as well to ensure full coverage.packages/rs-dpp/src/data_contract/associated_token/token_configuration_localization/accessors/v0/mod.rs (1)
13-23: Setter methods align with the trait design; optionalInto<String>could improve ergonomics.These setters are well-formulated to mutate internal states. If future usage often passes owned values of varying types, consider using
impl Into<String>to allow more flexibility for callers (e.g., accepting&str,String, etc.). Otherwise, this is already good.- fn set_singular_form(&mut self, singular_form: String); - fn set_plural_form(&mut self, plural_form: String); + fn set_singular_form<T: Into<String>>(&mut self, singular_form: T); + fn set_plural_form<T: Into<String>>(&mut self, plural_form: T);packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_recipient.rs (2)
80-81: TokenDistributionWeight type alias.Defining
TokenDistributionWeight = u64is simple. For extra safety, consider using a newtype wrapper if domain constraints require it.
82-97: Commented-out code found.If no longer needed, removing these commented lines might improve readability.
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/encode.rs (3)
12-15: Refactor repetitive encoding blocksAll variant arms in
encode()appear consistent and correctly assign unique identifiers (0–8). This matches the new distribution variants. However, the per-variant field encoding is quite repetitive. You might consider a small macro or helper function to reduce code duplication, making it more maintainable.Also applies to: 16-20, 21-36, 37-40, 41-56, 57-78, 79-100, 101-122, 123-145
158-158: Decode implementation matches encode variant tagsThe decode arms correctly align with their respective
encode()identifiers. This consistency ensures proper round-trip serialization. As with encoding, a macro-based approach could shrink repetitive code.Also applies to: 161-163, 165-180, 182-183, 185-199, 202-221, 224-243, 246-265, 268-287
303-304: BorrowDecode logic consistently mirrors DecodeThe borrow-based decoding yields the same variant tags and field mappings, ensuring congruence with
encode(). No issues found. Similar code consolidation techniques might reduce repetition.Also applies to: 307-309, 311-325, 328-329, 333-345, 348-367, 370-389, 392-411, 414-433
packages/rs-dpp/src/data_contract/associated_token/token_distribution_key.rs (2)
16-25: Confirm the default variant usage.
You derivedDefaultfor theTokenDistributionTypeenum, which sets thePreProgrammedvariant as default. Ensure this aligns with your intended logic for newly instantiated distributions.
73-82: Reassess necessity of reference-based conversion.
Theimpl From<&TokenDistributionInfo> for TokenDistributionTypeWithResolvedRecipientis convenient, but consider whether this additional conversion offers significant benefits. Avoid adding too many conversions for references if they're rarely used.packages/rs-dpp/src/data_contract/accessors/mod.rs (1)
361-402: Test version-specific setters thoroughly.
Newly introduced setters forcreated_at,updated_at, block height, and epoch values warrant careful integration and unit testing to confirm correct storage of versioned data.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/mod.rs (1)
75-137: Consider refactoring repetitive byte parsing logic
The logic for handling 8-byte slices (Block/Time) and 2-byte slices (Epoch) is repeated for each distribution variant. You could reduce duplication by extracting this into a small helper function or method (e.g.,parse_be_u64(bytes)/parse_be_u16(bytes)).packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs (2)
34-34: Remove unused import
The static analysis hints indicate thatuse crate::data_contract::config::v1::DataContractConfigGettersV1;is not used. Consider removing it:- use crate::data_contract::config::v1::DataContractConfigGettersV1;🧰 Tools
🪛 GitHub Check: Rust packages (wasm-dpp) / Linting
[warning] 34-34: unused import:
crate::data_contract::config::v1::DataContractConfigGettersV1
warning: unused import:crate::data_contract::config::v1::DataContractConfigGettersV1
--> packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs:34:5
|
34 | use crate::data_contract::config::v1::DataContractConfigGettersV1;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^🪛 GitHub Check: Rust packages (drive) / Linting
[warning] 34-34: unused import:
crate::data_contract::config::v1::DataContractConfigGettersV1
warning: unused import:crate::data_contract::config::v1::DataContractConfigGettersV1
--> packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs:34:5
|
34 | use crate::data_contract::config::v1::DataContractConfigGettersV1;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^🪛 GitHub Check: Rust packages (dash-sdk) / Linting
[warning] 34-34: unused import:
crate::data_contract::config::v1::DataContractConfigGettersV1
warning: unused import:crate::data_contract::config::v1::DataContractConfigGettersV1
--> packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs:34:5
|
34 | use crate::data_contract::config::v1::DataContractConfigGettersV1;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^🪛 GitHub Check: Rust packages (dpp) / Linting
[warning] 34-34: unused import:
crate::data_contract::config::v1::DataContractConfigGettersV1
warning: unused import:crate::data_contract::config::v1::DataContractConfigGettersV1
--> packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs:34:5
|
34 | use crate::data_contract::config::v1::DataContractConfigGettersV1;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
660-764: Opportunities for splittinginsert_values_nested
This function has grown significantly, handling multiple concerns. Consider factoring out smaller helpers to improve readability and maintainability.
🛑 Comments failed to post (14)
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_emergency_action_transition/validate_structure/mod.rs (1)
28-34: 💡 Verification agent
🧩 Analysis chain
Ensure tests exist for version handling.
The version handling logic appears complete, but it's important to ensure there are tests to verify both the success path (version 0) and the error path (unknown version).
🏁 Script executed:
#!/bin/bash # Check for tests covering token emergency action validation and version handling rg -i "test.*token_emergency_action.*validate_structure" --type rust # Look for test modules within the same directory structure find packages/rs-dpp/src/state_transition -name "*test*" | grep -i "token_emergency"Length of output: 155
I'll generate another shell script to more broadly search for tests covering the version handling of
TokenEmergencyActionTransitionto be certain if tests exist for both success (version 0) and error paths.After reviewing the output of these commands, please manually verify that tests exist which:
- Cover a successful validation path where version is 0.
- Ensure an error is returned for unknown versions.
If these tests are missing or incomplete, you'll need to add them accordingly.
Ensure that tests cover both expected valid and error cases for TokenEmergencyActionTransition::validate_structure.
🏁 Script executed:
#!/bin/bash # Broader search for test functions referencing the validate_structure method rg -i "fn\s+test_.*validate_structure" --type rust # Additionally, search for references to TokenEmergencyActionTransition in potential test files rg -i "TokenEmergencyActionTransition" --type rustLength of output: 32299
Attention: Ensure Comprehensive Test Coverage for Version Handling
The version dispatch logic in
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_emergency_action_transition/validate_structure/mod.rs(lines 28–34) looks correct. However, our searches didn’t clearly uncover tests verifying both outcomes. Please double-check and ensure that:
- There is a test case confirming that a version of 0 validates successfully via
validate_structure_v0().- There is a test case confirming that any unknown version returns a
ProtocolError::UnknownVersionMismatcherror.If these tests are missing, please add them to fully cover the version handling logic.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/document/document_transfer_transition_action/mod.rs (1)
12-12: 💡 Verification agent
🧩 Analysis chain
Consistent refactoring to advanced structure validation
Similar to the document update price transition action, this module has been refactored to use the new
advanced_structure_v0module instead ofstructure_v0. This maintains consistency across the document transition actions.Also applies to: 15-15
🏁 Script executed:
#!/bin/bash # Verify that the advanced_structure_v0 implementation covers all functionality from structure_v0 # Check the implementation of advanced_structure_v0 for this module echo "Examining the implementation of advanced_structure_v0 for DocumentTransferTransitionAction..." advanced_structure_path=$(fd --type=file advanced_structure_v0.rs "packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/document/document_transfer_transition_action/") if [[ -n "$advanced_structure_path" ]]; then # Look for validate_structure_v0 implementation rg --type=rust "fn validate_structure_v0" "$advanced_structure_path" # Check for any TODOs or FIXMEs that might indicate incomplete migration rg --type=rust "TODO|FIXME" "$advanced_structure_path" else echo "WARNING: advanced_structure_v0.rs not found for document_transfer_transition_action" fiLength of output: 610
It appears that no file named
advanced_structure_v0.rswas found in the expected directory. To confirm that no alternative module declaration (e.g., amod.rsinside anadvanced_structure_v0folder) exists, please verify that the advanced structure validation module is indeed present in the codebase. For example, you could search for the module by locating a folder namedadvanced_structure_v0or amod.rsfile within that folder.If neither is found, then the change described in this review comment would be incomplete or incorrect, suggesting that either the refactor wasn’t fully applied or the module is misnamed/misplaced.
You should either add the correct implementation or adjust the import accordingly.
Action Required: Ensure that the advanced structure validation for document transfer transitions is correctly implemented and located in the codebase.
- Confirm that a module for
advanced_structure_v0exists (i.e. either asadvanced_structure_v0.rsor as a folder with amod.rs) in the directorypackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/document/document_transfer_transition_action/.- If it does exist under a different name or location, update the import in
mod.rsaccordingly.packages/rs-dpp/src/data_contract/methods/equal_ignoring_time_based_fields/mod.rs (1)
34-34: 💡 Verification agent
❓ Verification inconclusive
Verify
equal_ignoring_time_fields_v0implementation.This line references
equal_ignoring_time_fields_v0, but we can't see its implementation. Ensure that it correctly ignores all six time-related fields mentioned in the documentation.
🏁 Script executed:
#!/bin/bash # Check the implementation of equal_ignoring_time_fields_v0 cat packages/rs-dpp/src/data_contract/methods/equal_ignoring_time_based_fields/v0.rsLength of output: 199
Missing Implementation of
equal_ignoring_time_fields_v0The file referenced for the implementation (
packages/rs-dpp/src/data_contract/methods/equal_ignoring_time_based_fields/v0.rs) could not be located. Please verify that the functionequal_ignoring_time_fields_v0exists—either by ensuring the file is added or by correcting the reference if it’s implemented elsewhere. Also, confirm that the implementation properly ignores all six documented time-related fields.
- Action needed:
- Confirm the existence or correct location of the implementation.
- Ensure the logic correctly omits the six time-based fields as intended.
packages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/add_distribute_fees_from_oldest_unpaid_epoch_pool_to_proposers_operations/v0/mod.rs (1)
108-108: 🛠️ Refactor suggestion
Check for potential overflow when casting to
u16.If the collection’s size can exceed the maximum value of
u16, it may silently truncate. If the design guarantees an upper bound below65536, then this is safe. Otherwise, safeguard with a check or a wider type.packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h (2)
6935-6937:
⚠️ Potential issueField numbers for TokenEvent enum have been reordered
The field numbers for
EmergencyActionandTokenConfigUpdatein theGetGroupActionsResponse_GetGroupActionsResponseV0_TokenEvent_FieldNumberenum have shifted from positions 7 and 8 to positions 6 and 7 respectively. This suggests the removal of a field that previously occupied position 6 (likelyTransferEventas indicated in the AI summary).This change could potentially break backward compatibility for clients expecting these fields at their previous positions. Ensure that all clients consuming this API are updated accordingly.
6946-6948:
⚠️ Potential issueOneOfCase enum values have been reordered to match field number changes
The corresponding OneOfCase enum values for
EmergencyActionandTokenConfigUpdatehave been updated to match their new field numbers (6 and 7). This is consistent with the field number changes and necessary for proper message deserialization.Make sure all clients that handle these message types are updated to use the new enum values, as this is a breaking change that affects message parsing.
packages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/add_epoch_pool_to_proposers_payout_operations/mod.rs (1)
5-5:
⚠️ Potential issueUpdate method documentation to match new return type
The method return type has been changed from
Result<u16, Error>toResult<(StorageAndProcessingPoolCredits, Vec<(Identifier, u64)>), Error>, but the method documentation on line 33 still describes the old return type.Update the method documentation to accurately describe the new return type and explain what each part of the tuple represents.
Also applies to: 8-8, 41-41
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs (1)
43-44: 💡 Verification agent
🧩 Analysis chain
Add case for version 1 in match statement
The
known_versionsvector now includes version 1, but there's no case for handling version 1 in the match statement. If version 1 is supported, you should add a case for it.
🏁 Script executed:
#!/bin/bash # Check if there's an implementation for version 1 in the codebase rg "try_from_schema_v1" --type rustLength of output: 35
I'll run another check to explicitly look for match arms handling numeric versions (e.g. using patterns like
0 =>and1 =>) in the same file so we can confirm if the case for version 1 has been implemented or omitted.
🏁 Script executed:
#!/bin/bash # Check for explicit match arms for literal version numbers in the try_from_schema module file. echo "Searching for match arm for version 0:" rg "0\s*=>" packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs echo "Searching for match arm for version 1:" rg "1\s*=>" packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rsLength of output: 424
Action: Add missing match arm for version 1 in the schema conversion
The current implementation declares support for version 1 by including it in the
known_versionsvector, but our verification shows that only version 0 is handled (e.g.,0 => DocumentTypeV0::try_from_schema_v0(...)). There is no corresponding branch for version 1 in the match statement.
- File:
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs- Location: Around lines 43-44, where
known_versions: vec![0, 1],is specified.- Issue: A match arm for version 1 is missing.
- Suggestion: Add a new case for version 1 in the match statement that handles schema conversion for version 1, similar to the implementation for version 0.
packages/rs-dpp/src/data_contract/config/mod.rs (1)
237-243: 🛠️ Refactor suggestion
SettersV1 with fallback for V0.
Ignoring the setter call on V0 transitions might hide user errors. Consider logging or returning an error if a user mistakenly calls it on V0.packages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/find_oldest_epoch_needing_payment/v0/mod.rs (1)
9-10: 💡 Verification agent
🧩 Analysis chain
New import for error handling.
Make sure these imports are necessary. It appears that
errorandDriveErrorare added but it's not immediately clear where they are used in the modified code.
🏁 Script executed:
#!/bin/bash # Check if these imports are used in the file grep -n "use drive::error;" packages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/find_oldest_epoch_needing_payment/v0/mod.rs grep -n "DriveError" packages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/find_oldest_epoch_needing_payment/v0/mod.rsLength of output: 366
Unused Imports Identified – Remove or Document Their Purpose
I verified the usage of the new error handling imports in this module. The only occurrences of both
use drive::error;(line 9) anduse drive::error::drive::DriveError;(line 10) are the import statements themselves. There’s no additional reference to eithererrororDriveErrorin the file, indicating that they might be redundant.
- File:
packages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/find_oldest_epoch_needing_payment/v0/mod.rs- Lines: 9–10
- Observation: No usage of either import outside their declaration.
Recommendation:
- Remove these imports if they are not used or intended for future error handling.
- Alternatively, add a clarifying comment explaining their purpose if they’re meant to be used later.
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/token_transition.rs (1)
415-451: 🛠️ Refactor suggestion
Comprehensive logic for
TokenClaimTransition; consider clarifyingTokenAmount::MAXusage.This block thoroughly handles the new claim type, checking whether the token distribution is
PreProgrammedorPerpetual. Error handling is present for unsupported distributions. However, usingTokenAmount::MAXto represent an unknown claim amount may introduce ambiguities or potential confusion about actual amounts. Consider storing an actual claim amount or clarifying the indefinite claim logic to avoid misinterpretations. Otherwise, the approach is logically consistent.- TokenEvent::Claim( - distribution_recipient, - TokenAmount::MAX, - claim.public_note().cloned(), - ) + // Example alternative: add a method or a field for an actual or partial claim amount + let claim_amount = claim.estimated_amount().unwrap_or(TokenAmount::MAX); + TokenEvent::Claim( + distribution_recipient, + claim_amount, + claim.public_note().cloned(), + )Committable suggestion skipped: line range outside the PR's diff.
packages/rs-dpp/src/errors/consensus/codes.rs (1)
105-108: 🛠️ Refactor suggestion
Distinct error types share the same error code.
Two different variants (
InvalidTokenDistributionFunctionDivideByZeroErrorandInvalidTokenDistributionFunctionInvalidParameterError) both use code10254. This can lead to ambiguity when diagnosing errors. Consider assigning them separate codes.packages/rs-dpp/src/data_contract/accessors/mod.rs (1)
270-318: 🛠️ Refactor suggestion
Validate
created_atandupdated_atordering.
Both fields are optional, but you may want to ensure thatupdated_atis never earlier thancreated_atto maintain logical consistency.packages/rs-drive-abci/src/execution/types/unpaid_epoch/v0/mod.rs (1)
81-90:
⚠️ Potential issueCritical mismatch in trait vs. implementation signatures.
You updated the trait methods to acceptBlockHeight/CoreBlockHeightbut the actual implementations (lines 150, 154, 158, 162) still takeu64/u32. This likely breaks compilation or leads to hidden conversions.Below is a suggested fix, aligning the types in the implementation:
fn set_start_block_height(&mut self, start_block_height: BlockHeight) { - self.start_block_height = start_block_height as u64; + self.start_block_height = start_block_height; } fn set_next_epoch_start_block_height(&mut self, next_epoch_start_block_height: BlockHeight) { - self.next_epoch_start_block_height = next_epoch_start_block_height as u64; + self.next_epoch_start_block_height = next_epoch_start_block_height; } fn set_start_block_core_height(&mut self, start_block_core_height: CoreBlockHeight) { - self.start_block_core_height = start_block_core_height as u32; + self.start_block_core_height = start_block_core_height; } fn set_next_epoch_start_block_core_height(&mut self, next_epoch_start_block_core_height: CoreBlockHeight) { - self.next_epoch_start_block_core_height = next_epoch_start_block_core_height as u32; + self.next_epoch_start_block_core_height = next_epoch_start_block_core_height; }📝 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 set_start_block_height(&mut self, start_block_height: BlockHeight) { - self.start_block_height = start_block_height as u64; + self.start_block_height = start_block_height; } fn set_next_epoch_start_block_height(&mut self, next_epoch_start_block_height: BlockHeight) { - self.next_epoch_start_block_height = next_epoch_start_block_height as u64; + self.next_epoch_start_block_height = next_epoch_start_block_height; } fn set_start_block_core_height(&mut self, start_block_core_height: CoreBlockHeight) { - self.start_block_core_height = start_block_core_height as u32; + self.start_block_core_height = start_block_core_height; } fn set_next_epoch_start_block_core_height(&mut self, next_epoch_start_block_core_height: CoreBlockHeight) { - self.next_epoch_start_block_core_height = next_epoch_start_block_core_height as u32; + self.next_epoch_start_block_core_height = next_epoch_start_block_core_height; }
…in apply snapshot
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/rs-drive-abci/src/abci/app/consensus.rs (1)
83-110: 🛠️ Refactor suggestionConsider error handling instead of unwrap()
The use of
unwrap()on RwLock operations could potentially cause panics in production. Consider using proper error handling instead.For example in the start_transaction method:
fn start_transaction(&self) { let transaction = self.platform.drive.grove.start_transaction(); - self.transaction.write().unwrap().replace(transaction); + if let Ok(mut guard) = self.transaction.write() { + guard.replace(transaction); + } else { + // Log error, possibly return Result instead of () + } }Similar changes should be applied to other methods using
unwrap().
♻️ Duplicate comments (1)
packages/rs-drive-abci/src/abci/app/consensus.rs (1)
19-23: Documentation needs expansionThe documentation should be expanded to clearly describe the purpose and responsibilities of the ConsensusAbciApplication. This is mentioned in a previous review comment.
Consider adding more comprehensive documentation like:
/// ConsensusAbciApplication is responsible for consensus-related functions that affect state, /// such as processing and executing blocks and votes. It also handles state synchronization /// operations including snapshot management and application during state sync. /// /// See also other ABCI applications like [CheckTxAbciApplication] and [QueryAbciApplication].
🧹 Nitpick comments (4)
packages/rs-drive-abci/src/abci/app/consensus.rs (4)
31-34: Add documentation for new fieldsThe new snapshot-related fields should have more detailed documentation explaining their purpose and how they're used in the state sync process.
/// The State sync session snapshot_fetching_session: RwLock<Option<SnapshotFetchingSession<'p>>>, /// The snapshot manager + /// Manages snapshot creation, retention, and retrieval based on configured checkpoints path, + /// maximum number of snapshots, and creation frequency snapshot_manager: SnapshotManager,
67-75: Consider implementing Debug trait for SnapshotFetchingSessionSince the SnapshotFetchingSession is stored in an RwLock and might be accessed in various contexts, implementing the Debug trait for it would help with debugging and logging.
You should also add documentation comments for the StateSyncApplication trait implementation to explain how these methods are used within the state sync process.
185-190: Add documentation for offer_snapshot methodAdd documentation for this method to explain its purpose and how it fits into the state sync process.
+ /// Processes a request to offer a snapshot for state synchronization. + /// This is called when a node is trying to sync and needs to know if this node has a useful snapshot. + /// The handler evaluates the offered snapshot and decides whether to accept it. fn offer_snapshot( &self, request: proto::RequestOfferSnapshot, ) -> Result<proto::ResponseOfferSnapshot, proto::ResponseException> { handler::offer_snapshot(self, request).map_err(error_into_exception) }
192-197: Add documentation for apply_snapshot_chunk methodAdd documentation for this method to explain its purpose and how it fits into the state sync process.
+ /// Processes a request to apply a chunk of a snapshot during state synchronization. + /// This is called sequentially for each chunk of an accepted snapshot until the entire snapshot is applied. + /// The handler processes the chunk data and integrates it into the platform state. fn apply_snapshot_chunk( &self, request: proto::RequestApplySnapshotChunk, ) -> Result<proto::ResponseApplySnapshotChunk, proto::ResponseException> { handler::apply_snapshot_chunk(self, request).map_err(error_into_exception) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-dpp/src/reduced_platform_state/mod.rs(1 hunks)packages/rs-drive-abci/src/abci/app/consensus.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-dpp/src/reduced_platform_state/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Rust packages (rs-dapi-client) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Formatting
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- 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 Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (2)
packages/rs-drive-abci/src/abci/app/consensus.rs (2)
39-44:❓ Verification inconclusive
Ensure configuration values are validated
The snapshot_manager is initialized with values from the platform config, but there's no validation to ensure these values are appropriate.
Run this script to verify if the configuration values are validated somewhere else in the codebase:
🏁 Script executed:
#!/bin/bash # Check if the StateSyncConfig values are validated echo "Checking for StateSyncConfig validation..." rg -A 10 "struct StateSyncConfig" --type rust rg "StateSyncConfig::validate" --type rust rg "StateSyncConfig::new" --type rustLength of output: 223
Validate
state_sync_configvaluesI wasn’t able to find any validation logic for
state_sync_configin this codebase (its struct and anyvalidate()calls appear to live in the externalplatformcrate). Please confirm and/or add checks to ensure:
checkpoints_pathpoints to a valid, writable directorymax_num_snapshotsandsnapshots_frequencyfall within safe, sane rangesYou can either:
- Implement a
validate()method onStateSyncConfigin theplatformcrate and invoke it at config load time- Or add these sanity checks in
SnapshotManager::newLocations to review:
- packages/rs-drive-abci/src/abci/app/consensus.rs around lines 39–44
85-92:✅ Verification successful
Verify transaction management in state sync context
Ensure that transaction management works correctly when state synchronization is active. The code should handle cases where a transaction might already be in progress when a snapshot chunk is being applied.
🏁 Script executed:
#!/bin/bash # Check how transactions are managed during state sync echo "Checking transaction management during state sync..." rg -A 10 "apply_snapshot_chunk" --type rust rg -A 10 -B 5 "startTransaction|commitTransaction" "snapshot" --type rustLength of output: 14527
🏁 Script executed:
#!/bin/bash # Inspect transaction management in consensus app echo "=== start_transaction context ===" rg -C5 "fn start_transaction" -g "packages/rs-drive-abci/src/abci/app/consensus.rs" echo echo "=== commit_transaction context ===" rg -C5 "commit_transaction" -g "packages/rs-drive-abci/src/abci/app/consensus.rs"Length of output: 2956
🏁 Script executed:
#!/bin/bash # Show ABCI methods in consensus app with transaction calls echo "=== begin_block ===" rg -C3 "fn begin_block" -g packages/rs-drive-abci/src/abci/app/consensus.rs echo echo "=== prepare_proposal ===" rg -C3 "fn prepare_proposal" -g packages/rs-drive-abci/src/abci/app/consensus.rs echo echo "=== finalize_block ===" rg -C3 "fn finalize_block" -g packages/rs-drive-abci/src/abci/app/consensus.rs echo echo "=== commit_transaction usage ===" rg -C3 "commit_transaction" -g packages/rs-drive-abci/src/abci/app/consensus.rsLength of output: 2805
No overlap between consensus transactions and state-sync sessions
State-sync’s
apply_snapshot_chunkpath solely uses its own GroveDB snapshot sessions (commit_session) and does not call thestart_transaction/commit_transactionmethods onConsensusAbciApplication. TheTransactionalApplicationmethods inconsensus.rsare only invoked during block processing, so there’s no risk of a pending consensus transaction interfering with snapshot application. No changes needed.
lklimek
changed the title
Co-authored-by: Lukasz Klimek <[email protected]>
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation