feat!: store/fetch reduced platform state in drive #2457
Conversation
ogabrielides
requested review from
QuantumExplorer and
shumkov
as code owners
WalkthroughThis change set introduces a new reduced platform state feature. A dedicated module for reduced platform state handling is added with new enums, structs, and traits for reduced serialization and deserialization. In addition, the block proposal processing flow now supports a new version (v1) with dedicated logic. New methods for fetching and storing reduced platform state bytes have been implemented in both the execution and drive layers. Versioning structures across the platform version modules were updated to include the reduced platform state storage methods without altering any previous functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Block Proposal Request
participant Engine as Platform Engine
participant V1 as run_block_proposal_v1
Client->>Engine: Submit block proposal with version info
Engine->>Engine: Match version (0 or 1)
alt Version 1 Detected
Engine->>V1: Call run_block_proposal_v1(...)
V1->>Engine: Return ValidationResult
else Version 0
Engine->>Engine: Process using existing logic
end
sequenceDiagram
participant Platform as Platform
participant Storage as Drive Storage
participant DB as Grovedb
Platform->>Storage: fetch_reduced_platform_state(drive, tx, version)
Storage->>Storage: Match platform version (v0)
Storage->>DB: fetch_reduced_platform_state_bytes_v0(tx, version)
DB-->>Storage: Return state bytes (or None)
Storage->>Platform: Deserialize bytes into ReducedPlatformStateForSaving
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ogabrielides
changed the title
| /// This function may return an `Error` variant if there is a problem with processing the block | ||
| /// proposal, updating the core info, processing raw state transitions, or processing block fees. | ||
| /// | ||
| pub(super) fn run_block_proposal_v1( |
There was a problem hiding this comment.
Call v0 instead of copy-paste?
There was a problem hiding this comment.
This is not possible. As run_block_proposal_v1 performs an extra operation in the middle of run_block_proposal_v0 logic.
| fn try_from(value: PlatformStateV0) -> Result<Self, Self::Error> { | ||
| Ok(ReducedPlatformStateForSavingV0 { | ||
| genesis_block_info: value.genesis_block_info, | ||
| last_committed_block_info: value.last_committed_block_info, |
There was a problem hiding this comment.
This gonna be for previous block. PlatformState will have it for current block. Not sure it's a good idea.
There was a problem hiding this comment.
No, regarding state sync we are fine.
|
|
||
| /// Reduced Platform State for Saving V0 | ||
| #[derive(Clone, Debug, Encode, Decode)] | ||
| pub struct ReducedPlatformStateForSavingV0 { |
There was a problem hiding this comment.
So you want to duplicate these data in both PlatfromState and ReducedPlatform state? and serialize and store it twice a block?
There was a problem hiding this comment.
As this complicates the logic, I propose to make a separate PR for this, since it is only an optimisation/refactoring.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs (1)
175-179:⚠️ Potential issueUpdate version list in error message.
The error message's
known_versionslist needs to be updated to include version 1.Apply this diff to update the version list:
- known_versions: vec![0], + known_versions: vec![0, 1],
🧹 Nitpick comments (9)
packages/rs-drive-abci/src/execution/engine/run_block_proposal/v1/mod.rs (1)
417-425: Increase granularity of the returned outcome.
While returningBlockExecutionOutcomeis convenient, it may be helpful to log or dissect individual components of the outcome in separate debug statements if troubleshooting is needed.packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (1)
32-34: Maintain clarity for versioned reduced state references.
ReducedPlatformStateForSavingV0andReducedPlatformStateForSavingare newly introduced. Consider including remarks on how new state versions will integrate in the future or be handled in transitions.packages/rs-drive-abci/src/execution/storage/mod.rs (1)
2-4: Consider re-exporting newly introduced modules if needed.
fetch_reduced_platform_stateandstore_reduced_platform_stateare added but remain module-scoped. If other parts of the codebase require direct access, consider re-exporting them or providing a higher-level public interface.packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/v0/mod.rs (1)
13-23: Improve error handling.The current error handling using
unwrap().map_err()can be simplified to use the?operator for a more idiomatic approach.- self.grove - .insert_if_not_exists( - &misc_path(), - REDUCED_PLATFORM_STATE_KEY, - Element::Item(reduced_state_bytes.to_vec(), None), - transaction, - &Default::default(), - ) - .unwrap() - .map_err(Error::GroveDB)?; + self.grove + .insert_if_not_exists( + &misc_path(), + REDUCED_PLATFORM_STATE_KEY, + Element::Item(reduced_state_bytes.to_vec(), None), + transaction, + &Default::default(), + )? + .map_err(Error::GroveDB)?;packages/rs-drive-abci/src/execution/storage/store_reduced_platform_state/mod.rs (1)
12-31: LGTM! Consider adding more detailed documentation.The implementation is well-structured with proper error handling and version management. However, the documentation could be more descriptive.
Add more detailed documentation explaining:
- The purpose of reduced platform state
- The versioning strategy
- The expected input/output behavior
- Any side effects
- /// Store the execution state in grovedb storage + /// Stores a reduced version of the platform state in grovedb storage. + /// + /// This method stores only the essential fields needed for state synchronization, + /// omitting fields that can be reconstructed using Core functionalities. + /// + /// # Arguments + /// * `state` - The complete platform state from which reduced state will be derived + /// * `transaction` - The transaction context for the storage operation + /// * `platform_version` - The platform version that determines the storage method + /// + /// # Returns + /// * `Ok(())` if storage is successful + /// * `Err(Error)` if storage fails or version is unsupportedpackages/rs-drive-abci/src/execution/storage/fetch_reduced_platform_state/mod.rs (1)
13-33: Ensure consistency in method calls and add documentation.The implementation is solid but has some inconsistencies with
store_reduced_platform_state.
- Make the method call style consistent with store implementation:
- Platform::<C>::fetch_reduced_platform_state_v0(drive, transaction, platform_version) + self.fetch_reduced_platform_state_v0(drive, transaction, platform_version)
- Add detailed documentation similar to store implementation:
- /// Fetches execution state from grovedb storage + /// Fetches the reduced platform state from grovedb storage. + /// + /// This method retrieves the minimal version of platform state that contains + /// only the essential fields needed for state synchronization. + /// + /// # Arguments + /// * `drive` - The drive instance for storage operations + /// * `transaction` - The transaction context for the fetch operation + /// * `platform_version` - The platform version that determines the fetch method + /// + /// # Returns + /// * `Ok(Some(state))` if state exists and fetch is successful + /// * `Ok(None)` if state doesn't exist + /// * `Err(Error)` if fetch fails or version is unsupportedpackages/rs-drive/src/drive/platform_state/fetch_reduced_platform_state_bytes/v0/mod.rs (1)
9-37: Add comprehensive documentation for the v0 implementation.The implementation looks correct but lacks detailed documentation.
Add documentation explaining:
+ /// Fetches the reduced platform state bytes using groveDB (v0 implementation). + /// + /// # Implementation Details + /// - Uses REDUCED_PLATFORM_STATE_KEY for storage + /// - Performs a single item query + /// - Validates that exactly one result is returned + /// + /// # Arguments + /// * `transaction` - The transaction context + /// * `platform_version` - Platform version for grove version selection + /// + /// # Returns + /// * `Ok(Some(bytes))` - The stored state bytes if found + /// * `Ok(None)` - If no state exists + /// * `Err(Error)` - If query fails or returns invalid resultspackages/rs-drive-abci/src/execution/storage/fetch_reduced_platform_state/v0/mod.rs (1)
17-41: Implementation looks solid!The error handling, logging, and Result transformation are well implemented. The function properly handles the fetching and deserialization of the reduced platform state.
Consider adding debug-level logging for successful deserialization to aid in debugging:
if result.is_err() { tracing::error!( bytes = hex::encode(&bytes), "Unable deserialize reduced platform state for version {}", platform_version.protocol_version ); +} else { + tracing::debug!( + "Successfully deserialized reduced platform state for version {}", + platform_version.protocol_version + ); }packages/rs-platform-version/src/version/drive_versions/v3.rs (1)
18-19: Add documentation for the new reduced platform state methods.Based on previous feedback, consider adding Rust doc comments to enhance clarity and maintainability.
- DriveProveMethodVersions, DriveReducedPlatformStateMethodVersions, - DriveSystemEstimationCostsMethodVersions, DriveVersion, + DriveProveMethodVersions, + /// Methods for handling reduced platform state operations + DriveReducedPlatformStateMethodVersions, + DriveSystemEstimationCostsMethodVersions, + DriveVersion,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
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/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/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_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/platform_state/mod.rs(4 hunks)packages/rs-drive-abci/src/platform_types/platform_state/v0/mod.rs(2 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_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-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 (6)
packages/rs-platform-version/src/version/drive_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-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-abci/src/platform_types/platform_state/v0/mod.rs (2)
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.
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs:94-94
Timestamp: 2024-11-20T10:01:50.837Z
Learning: In `packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs`, when converting with `PublicKey::try_from`, it's acceptable to use `.expect()` to handle potential conversion errors.
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-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v1.rs (2)
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.
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.
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.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (44)
packages/rs-drive-abci/src/execution/engine/run_block_proposal/v1/mod.rs (2)
61-72: Consider clarifying the version selection logic.
The guard at lines 88–98 ensures that incoming block proposals from external sources match the local protocol version. However, if the platform upgrades or a new protocol version is introduced, valid proposals might be rejected unless this logic is updated. Consider clarifying the approach or adding a comment about handling future protocol mismatches.
164-246: Revisit chain lock verification for locally known proposals.
Whenknown_from_usis true, the chain lock validation is effectively bypassed assuming local verification is sufficient. Ensure this doesn't introduce a security or synchronization gap if full verification is still required in some scenarios.packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (2)
18-20: Confirm necessity of new imports.
TheReducedPlatformSerializableimport is introduced here. If used exclusively outside the current file, consider clarifying its purpose or location. Remove it if truly unused.
226-254: Ensure forward compatibility in version checks.
Thistry_from_platform_versionedimplementation only matches version 0 forReducedPlatformStateForSaving. If the platform upgrades, this logic will fail withUnknownVersionMismatch. Confirm that this is the desired approach or if a graceful fallback is needed.packages/rs-drive/src/drive/platform_state/mod.rs (1)
2-2: LGTM! The changes align with the PR objectives.The new modules and constant for reduced platform state management are well-structured and follow naming conventions. The changes are additive and don't impact existing functionality.
Also applies to: 4-4, 7-7
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_structure_versions/v1.rs (1)
7-7: LGTM! The versioning is consistent with existing patterns.The new field for reduced platform state follows the established versioning pattern, initializing to version 0 for retroactive changes.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_structure_versions/mod.rs (1)
9-9: LGTM! The struct field is well-defined.The new field for reduced platform state follows the established pattern, using the correct type and naming convention.
packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/v0/mod.rs (1)
8-12:❓ Verification inconclusive
Verify the method signature.
The AI summary mentions a
platform_versionparameter, but it's not present in the actual code. Please verify if this parameter is needed for version-specific behavior.
🏁 Script executed:
#!/bin/bash # Search for usages of platform_version in similar methods rg -A 5 "platform_version.*store.*state.*bytes"Length of output: 49
Action Required: Confirm Method Signature Details and Error Handling Improvements
Method Signature (Lines 8–12): The AI summary mentioned a
platform_versionparameter, yet none is present in the actual method. Our search for similar methods did not reveal any usage of this parameter, suggesting that its removal may be intentional. Please manually verify that omittingplatform_versionaligns with the desired version-specific behavior.Error Handling (Lines 13–23): There is an opportunity to improve the error handling. Specifically, replacing the use of
unwrapfollowed bymap_errwith the more idiomatic?operator could enhance clarity and robustness.packages/rs-drive-abci/src/execution/storage/store_reduced_platform_state/v0/mod.rs (1)
9-23: LGTM! Clean implementation with proper error handling.The implementation follows good practices:
- Clear error propagation using
map_err- Proper visibility modifiers
- Concise and focused functionality
packages/rs-dpp/src/reduced_platform_state/v0/mod.rs (1)
10-23:❓ Verification inconclusive
Review data duplication and field necessity.
Based on previous review comments:
- Consider potential data duplication between
PlatformStateandReducedPlatformState.- The
last_committed_block_infofield might be unnecessary since it's set to None every block.Let's verify the usage of these fields:
🏁 Script executed:
#!/bin/bash # Search for usage of last_committed_block_info rg -A 5 "last_committed_block_info" # Search for PlatformState fields to compare with ReducedPlatformState ast-grep --pattern $'struct PlatformState { $$$ }'Length of output: 96998
Verify design of duplicated state fields and intended usage of
last_committed_block_info.After reviewing the usage across the codebase, the following points emerged:
- The field
last_committed_block_infoappears extensively in both tests and runtime code. Although one instance shows it initialized asNone, there are several cases where it is updated with an actual block info structure.- Duplication of fields between
PlatformStateandReducedPlatformStateseems deliberate for serialization and state persistence, but its necessity should be re-examined. If the field is expected to carry meaningful data beyond initial block setups, its presence in both structs may be justified.- Please verify the design rationale in the related documentation and confirm that:
- Duplicating this field between the two states is intentional.
- The cases where
last_committed_block_infoisNoneare well understood and documented.- All tests expecting a value (via unwrap and basic_info calls) correctly reflect the intended state transitions.
packages/rs-drive/src/drive/platform_state/fetch_reduced_platform_state_bytes/mod.rs (1)
11-29: LGTM! Well-structured version handling.The implementation follows good practices:
- Clear version-based dispatch
- Informative error messages for unknown versions
- Proper error type usage
packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/mod.rs (1)
11-30: LGTM! Consistent with fetch implementation.The implementation follows the same good practices as the fetch method:
- Clear version-based dispatch
- Informative error messages for unknown versions
- Proper error type usage
packages/rs-drive/src/drive/platform_state/fetch_reduced_platform_state_bytes/v0/mod.rs (1)
19-29:❓ Verification inconclusive
Verify query parameters for groveDB call.
The groveDB query parameters need verification to ensure correct behavior.
🏁 Script executed:
#!/bin/bash # Search for other groveDB query_item_value calls to verify parameter usage rg -A 2 'query_item_value.*true.*false.*true' --type rustLength of output: 59
Verify GroveDB Query Parameters in Query Call
The call to
query_item_valuewith parameters(true, false, true)couldn’t be cross-referenced with similar instances in the repository. Please ensure that these argument values align with GroveDB's expected API behavior. In particular, verify that the order and semantics of the booleans correctly reflect the intended read, caching, and proof-generation flags, and consider adding inline documentation for clarity.packages/rs-dpp/src/reduced_platform_state/mod.rs (1)
27-36:✅ Verification successful
Verify bincode configuration for compatibility.
The bincode configuration uses big-endian with no size limits. Ensure this matches other serialization in the codebase.
🏁 Script executed:
#!/bin/bash # Search for other bincode configurations rg 'config::standard\(\)\.with_big_endian\(\)\.with_no_limit\(\)' --type rustLength of output: 1923
Bincode Configuration Consistency Verified
The usage of
config::standard().with_big_endian().with_no_limit()inpackages/rs-dpp/src/reduced_platform_state/mod.rshas been confirmed to match the configuration used in similar serialization code across the codebase (e.g., inpackages/rs-drive-abciandpackages/rs-platform-serialization-derive). No discrepancies were found.packages/rs-dpp/src/lib.rs (1)
63-63: LGTM!The new module is properly exposed and follows the existing pattern of module declarations.
packages/rs-platform-version/src/version/drive_versions/v1.rs (1)
103-106: LGTM!The addition of reduced platform state methods is consistent with the versioning pattern and properly initialized to version 0 for retroactive compatibility.
packages/rs-platform-version/src/version/drive_versions/v2.rs (1)
103-106: LGTM!The addition of reduced platform state methods maintains consistency with v1 and follows the established versioning pattern.
packages/rs-platform-version/src/version/drive_versions/v3.rs (1)
103-106: LGTM! The reduced platform state methods are correctly initialized.The new methods are properly integrated into the version structure with initial versions set to 0.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v1.rs (1)
126-129: LGTM! The reduced platform state storage methods are correctly initialized.The new methods are properly integrated into the version structure with initial versions set to 0, maintaining consistency with other method versions.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v3.rs (1)
126-129: LGTM! The reduced platform state storage methods are correctly initialized.The new methods are properly integrated into the version structure with initial versions set to 0, maintaining consistency with other method versions.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v4.rs (2)
126-129: LGTM! The reduced platform state storage methods are correctly initialized.The new methods are properly integrated into the version structure with initial versions set to 0, maintaining consistency with other method versions.
1-131:✅ Verification successful
Verify consistent implementation across all version files.
Let's ensure that the reduced platform state storage methods are consistently implemented across all version files.
🏁 Script executed:
#!/bin/bash # Description: Check for consistent implementation of reduced platform state storage methods across all version files. # Search for version files echo "Checking version files for reduced platform state storage methods..." rg -l "platform_reduced_state_storage.*DriveAbciReducedPlatformStateStorageMethodVersions" packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/Length of output: 987
Platform Reduced State Storage Implementation Verified
- The search confirmed that every version file (including mod.rs, v1.rs, v2.rs, v3.rs, v4.rs, v5.rs, and v6.rs) contains the implementation for
DriveAbciReducedPlatformStateStorageMethodVersions.- The consistency across all version files is verified as the same pattern appears in each, as seen in the shell command output.
Overall, the reduced platform state storage methods are implemented consistently across all version files. No changes are required.
packages/rs-platform-version/src/version/drive_versions/v4.rs (2)
18-19: LGTM! Import added for reduced platform state versioning.The import is correctly added to support the new reduced platform state functionality.
103-106: LGTM! Reduced platform state storage methods initialized.The reduced platform state storage methods are correctly initialized with version 0, following the versioning pattern used throughout the codebase.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v2.rs (2)
11-11: LGTM! Import added for reduced platform state storage versioning.The import is correctly added to support the new reduced platform state storage functionality.
127-130: LGTM! Reduced platform state storage methods initialized.The reduced platform state storage methods are correctly initialized with version 0, following the versioning pattern used throughout the codebase.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v6.rs (3)
11-11: LGTM! Import added for reduced platform state storage versioning.The import is correctly added to support the new reduced platform state storage functionality.
21-21: LGTM! Version bump for run_block_proposal.The version bump from 0 to 1 aligns with the PR objectives mentioning updates to this function for storing reduced platform state.
127-130: LGTM! Reduced platform state storage methods initialized.The reduced platform state storage methods are correctly initialized with version 0, following the versioning pattern used throughout the codebase.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v5.rs (2)
11-11: LGTM! Import added for reduced platform state storage versioning.The import is correctly added to support the new reduced platform state storage functionality.
130-133: LGTM! Reduced platform state storage methods initialized.The reduced platform state storage methods are correctly initialized with version 0, following the versioning pattern used throughout the codebase.
packages/rs-platform-version/src/version/drive_versions/mod.rs (2)
72-76: LGTM!The new
DriveReducedPlatformStateMethodVersionsstruct follows the established pattern and naming conventions, with appropriate feature version fields for fetching and storing reduced platform state bytes.
63-63: LGTM!The new
reduced_platform_statefield inDriveMethodVersionsis appropriately placed and follows the established pattern.packages/rs-dpp/src/serialization/serialization_traits.rs (2)
34-37: Consider removing unused associated type.The
Errorassociated type is defined but not used in any trait bounds or constraints. This is similar to a past review comment that questioned the necessity of unused associated types.Do you need this associated type for future implementations, or can it be removed?
39-46: LGTM!The
ReducedPlatformDeserializabletrait follows the established pattern of other deserialization traits, with appropriate versioning support.packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rs (1)
38-42: LGTM!The new
DriveAbciReducedPlatformStateStorageMethodVersionsstruct and its integration intoDriveAbciMethodVersionsfollow the established patterns and naming conventions.Also applies to: 29-29
packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs (1)
165-174: LGTM!The implementation of version 1 follows the established pattern for version upgrades, maintaining consistency with the existing codebase.
packages/rs-platform-version/src/version/mocks/v3_test.rs (3)
27-27: LGTM!The import of
DriveAbciReducedPlatformStateStorageMethodVersionsis consistent with the PR objectives to store/fetch reduced platform state.
54-54: Breaking change:run_block_proposalversion bump.The version bump from 0 to 1 is consistent with the PR objectives, as the function now stores the ReducedPlatformState at the end of its execution process.
160-163: LGTM!The addition of
platform_reduced_state_storagewith methods for fetching and storing reduced platform state is consistent with the PR objectives.packages/rs-platform-version/src/version/mocks/v2_test.rs (2)
45-46: LGTM!The import of
DriveReducedPlatformStateMethodVersionsis consistent with the PR objectives to store/fetch reduced platform state.
139-142: LGTM!The addition of
reduced_platform_statewith methods for fetching and storing reduced platform state bytes is consistent with the PR objectives.packages/rs-drive-abci/src/platform_types/platform_state/v0/mod.rs (2)
33-33: LGTM!The import of
ReducedPlatformStateForSavingV0is consistent with the PR objectives to store/fetch reduced platform state.
256-278:❓ Verification inconclusive
Verify the impact of storing reduced platform state.
The implementation correctly copies essential fields from
PlatformStateV0toReducedPlatformStateForSavingV0. However, based on past review comments, there are concerns about storing state for the previous block.Please verify:
- The impact of storing reduced state for the previous block.
- Whether the selected fields in the reduced state are sufficient for reconstruction.
- Whether any critical fields are missing from the reduced state.
🏁 Script executed:
#!/bin/bash # Description: Search for usages of PlatformState and ReducedPlatformState to understand the impact. # Test: Search for usages of PlatformState and ReducedPlatformState. rg -A 5 $'PlatformState|ReducedPlatformState'Length of output: 68859
Action Required: Verify the impact of reduced platform state storage
Based on a repository-wide search, the conversion from
PlatformStateV0toReducedPlatformStateForSavingV0copies the essential fields—namely, the last committed block info, current and upcoming protocol versions, validator set quorum hashes, and fee version mappings. There’s no immediate evidence from usage in tests and related modules that any critical fields necessary for reconstructing or processing the previous block’s state have been omitted.However, please verify the following manually:
- Impact on Previous Block State: Confirm that storing only this reduced set of data does not affect any downstream logic (e.g., state transitions, epoch info generation, fee calculations) that might rely on additional details of the complete platform state.
- Sufficiency for Reconstruction: Ensure that the chosen fields are indeed sufficient for accurately reconstructing necessary aspects of the platform state in all current workflows.
- Future-proofing: Double-check that no additional fields will become critical as the platform’s requirements evolve.
| self.store_reduced_platform_state( | ||
| &block_execution_context.block_platform_state(), | ||
| Some(transaction), | ||
| platform_version, | ||
| )?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add robust error handling for storing the reduced platform state.
If store_reduced_platform_state fails, you could end up with an inconsistent state mid-block. Consider adding recovery steps or logging details to indicate a partially applied state or to prompt corrective measures.
| pub enum ReducedPlatformStateForSaving { | ||
| V0(ReducedPlatformStateForSavingV0), | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider implementing serialization for symmetry.
The enum only implements deserialization but might need serialization for completeness.
Consider adding serialization implementation:
impl PlatformSerializable for ReducedPlatformStateForSaving {
fn versioned_serialize(&self, platform_version: &PlatformVersion) -> Result<Vec<u8>, ProtocolError> {
let config = config::standard().with_big_endian().with_no_limit();
bincode::encode_to_vec(self, config).map_err(|e| {
ProtocolError::PlatformSerializationError(format!(
"unable to serialize ReducedPlatformStateForSaving: {}", e
))
})
}
}🏁 Script executed:
#!/bin/bash
# Search for PlatformSerializable implementations
rg -A 5 'impl.*PlatformSerializable' --type rustLength of output: 8993
Implement serialization for ReducedPlatformStateForSaving
Currently, the enum in the file packages/rs-dpp/src/reduced_platform_state/mod.rs only supports deserialization. For consistency with similar types in the repository (e.g., implementations for PlatformState and CreatedDataContract), add a serialization implementation. For example:
impl PlatformSerializable for ReducedPlatformStateForSaving {
fn versioned_serialize(&self, platform_version: &PlatformVersion) -> Result<Vec<u8>, ProtocolError> {
let config = config::standard().with_big_endian().with_no_limit();
bincode::encode_to_vec(self, config).map_err(|e| {
ProtocolError::PlatformSerializationError(format!(
"unable to serialize ReducedPlatformStateForSaving: {}", e
))
})
}
}This change will provide symmetry between serialization and deserialization for this type.
|
Will be included in #2486. |
2 participants
Issue being fixed or feature implemented
Because of state sync, the need for storage of Platform state under the root tree in groveDB was brought.
Although, not all fields of PlatformState were required to be saved as many of them can be reconstructed using Core.
A minimal version of PlatformState was created storing only the essential data: ReducedPlatformState
What was done?
run_block_proposal was bumped to v1, and stores the ReducedPlatformState at the very end.
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit