feat(dashmate): add state sync configuration - #2558
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce and standardize state synchronization (state sync) configuration across the system, particularly for the Drive ABCI and Tenderdash components. New configuration schema properties, environment variables, and Docker volume mounts are added to support state sync features such as snapshots and checkpoints. The Rust configuration structs are refactored to centralize state sync settings within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dashmate Config
participant Docker Compose
participant Drive ABCI
participant Tenderdash
User->>Dashmate Config: Runs config migration or setup
Dashmate Config->>Dashmate Config: Adds stateSync settings for ABCI and Tenderdash
Dashmate Config->>Docker Compose: Passes stateSync env vars and volume mounts
Docker Compose->>Drive ABCI: Mounts checkpoints directory, sets env vars
Docker Compose->>Tenderdash: Sets stateSync env vars
Drive ABCI->>Drive ABCI: Initializes stateSync from config/env
Tenderdash->>Tenderdash: Initializes stateSync from config/env
Poem
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 review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (1)
packages/dashmate/docker-compose.yml (1)
96-99: Optional: ParameterizeCHECKPOINTS_PATHinstead of hardcoding
Consider using a host-side variable (e.g.${PLATFORM_DRIVE_ABCI_STATE_SYNC_CHECKPOINTS_PATH:-/var/lib/dash/rs-drive-abci/db-checkpoints}) forCHECKPOINTS_PATHto maintain consistency with the other snapshot-related variables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/dashmate/templates/platform/drive/tenderdash/config.toml.dotis excluded by!**/*.dot
📒 Files selected for processing (11)
Dockerfile(2 hunks)packages/dashmate/configs/defaults/getBaseConfigFactory.js(2 hunks)packages/dashmate/configs/defaults/getLocalConfigFactory.js(1 hunks)packages/dashmate/configs/getConfigFileMigrationsFactory.js(1 hunks)packages/dashmate/docker-compose.yml(3 hunks)packages/dashmate/src/config/configJsonSchema.js(2 hunks)packages/rs-drive-abci/.env.local(1 hunks)packages/rs-drive-abci/.env.mainnet(1 hunks)packages/rs-drive-abci/.env.testnet(1 hunks)packages/rs-drive-abci/src/abci/config.rs(4 hunks)packages/rs-drive-abci/src/config.rs(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/rs-drive-abci/src/abci/config.rs (1)
packages/rs-drive-abci/src/config.rs (10)
default_mainnet(832-871)default(87-92)default(366-369)default(450-459)default(540-549)default(676-683)default(687-689)default(906-914)default_local(706-745)default_testnet(790-829)
packages/rs-drive-abci/src/config.rs (1)
packages/rs-drive-abci/src/abci/config.rs (6)
default_local(68-78)default_local(124-131)default_testnet(80-90)default_testnet(133-140)default_mainnet(56-66)default_mainnet(142-149)
🔇 Additional comments (21)
packages/rs-drive-abci/.env.testnet (1)
17-21: Env vars for State Sync added correctly
The new environment variables under the “State Sync” section—CHECKPOINTS_PATH,SNAPSHOTS_ENABLED,SNAPSHOTS_FREQUENCY, andMAX_NUM_SNAPSHOTS—align with the updated configuration schema and existing defaults. No conflicts detected.packages/dashmate/configs/defaults/getLocalConfigFactory.js (1)
66-68: Disable stateSync in local configs
AddingstateSync.enabled: falsehere correctly overrides the base configuration for local development, preventing unnecessary state sync operations on a single-node setup.packages/rs-drive-abci/.env.local (1)
16-20: Local .env: State Sync variables inserted
The “State Sync” header and its associated variables (CHECKPOINTS_PATH,SNAPSHOTS_ENABLED,SNAPSHOTS_FREQUENCY,MAX_NUM_SNAPSHOTS) are in the right place and consistent with other env files.packages/rs-drive-abci/.env.mainnet (1)
17-21: Mainnet .env: State Sync section added
The checkpoint path and snapshot controls appear correct and mirror the testnet/local defaults. No regressions spotted.packages/dashmate/docker-compose.yml (1)
85-85: Verify renaming of DB path var toGROVEDB_LATEST_FILE
You’ve replacedDB_PATHwithGROVEDB_LATEST_FILEin the container’s environment. Ensure the RustAbciConfig(and any related code) now readsGROVEDB_LATEST_FILEinstead ofDB_PATH.packages/dashmate/configs/getConfigFileMigrationsFactory.js (1)
1079-1104: Well-structured migration for state sync configuration.This new migration adds state synchronization configuration to all platforms with sensible defaults, and correctly disables it for local configurations. The implementation iterates over existing configs and applies the new properties appropriately.
packages/dashmate/configs/defaults/getBaseConfigFactory.js (2)
308-314: Appropriate default settings for ABCI state sync snapshots.The new stateSync configuration for the ABCI component sets reasonable defaults with snapshots enabled, a moderate frequency (5), and a reasonable maximum limit (100).
422-429: Good defaults for Tenderdash state sync configuration.The Tenderdash state sync configuration includes appropriate timeout settings, retry logic, and concurrency limits that should work well in production environments.
Dockerfile (2)
608-608: Added necessary volume for state sync checkpoints.This change adds a dedicated volume for state sync checkpoints, which is required for the new state synchronization functionality.
628-628: Correct permissions for checkpoint directory.The updated ownership settings ensure that the dash user has proper access to the new checkpoints directory.
packages/dashmate/src/config/configJsonSchema.js (3)
952-976: New state sync schema for drive.abci looks good.The schema properly defines the structure for ABCI state sync configuration, with appropriate validation for enabling snapshot functionality, frequency of snapshots, and maximum snapshot limit. All properties are correctly set as required with appropriate data types and minimum values.
1236-1264: Well-structured schema for Tenderdash state sync.The Tenderdash state sync configuration schema is comprehensive, including the necessary parameters for enabling state sync, retries, timeout handling, and concurrent operations management. All properties have appropriate minimum values and correct data types.
1266-1266: Good addition of stateSync to required properties.Adding stateSync to the required properties list for drive.tenderdash ensures that this configuration will be present in all configurations, which is important for the feature to work consistently.
packages/rs-drive-abci/src/abci/config.rs (8)
42-44: Good integration of state sync into AbciConfig.The flattening of the state_sync field is a clean approach that will simplify the configuration structure when serialized/deserialized.
56-66: Well-structured default_mainnet implementation.The default_mainnet implementation correctly initializes all configuration properties and uses the specific state sync configuration for mainnet environments.
68-78: Good implementation of default_local method.This method correctly configures ABCI for local development environments, particularly by using the appropriate state sync configuration where snapshots are disabled by default.
88-88: Proper state sync configuration for testnet.Using the TestNet-specific state sync configuration is appropriate here.
93-97: Good Default trait implementation.Setting the default implementation to use default_mainnet() ensures consistency with production environments when explicit environment isn't specified.
127-127: Good refactoring of default path initialization.Using the centralized default_checkpoints_path() method across all environment configurations ensures consistency and follows the DRY principle.
Also applies to: 136-136, 145-145
144-144: Enabling snapshots for mainnet is appropriate.Setting snapshots_enabled to true for mainnet aligns with the testnet configuration and is suitable for production environments where state synchronization is important.
42-97: Verify PR testing status before merging.The PR description mentioned "no testing has been performed yet." Before merging, ensure that comprehensive testing of the state sync configuration is performed in all supported environments (mainnet, testnet, local) to verify correct functionality.
| block_spacing_ms: 5000, | ||
| drive: Default::default(), | ||
| abci: Default::default(), | ||
| abci: AbciConfig::default_local(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improved configuration structure by removing redundant state sync config.
This change refactors the configuration to use environment-specific AbciConfig constructors instead of default initialization. The code now centralizes state sync configuration within the AbciConfig rather than having a separate state_sync_config field in PlatformConfig.
The refactoring makes the configuration structure cleaner and more maintainable by properly delegating state sync configuration responsibility to the AbciConfig struct.
Also applies to: 774-774, 816-816, 858-858
5b769c0 to
8cb7ad7
Compare
2 participants
Issue being fixed or feature implemented
We need to configure state sync with dashmate
What was done?
How Has This Been Tested?
None
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Configuration
Migration