chore(dashmate): update Core to version 22 #2107
Conversation
|
Do not merge this! |
|
@QuantumExplorer what about now? |
|
Let's make this for 1.4 only please. |
|
We can't really merge this into production. |
shumkov
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
WalkthroughThe changes in this pull request involve modifications across several files, primarily focusing on updating migration functions, altering task sequences, and refining error handling. A new migration function for version Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
shumkov
changed the title
…nightly' into chore/dashmate/update-core-21.2-nightly
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/dashmate/configs/defaults/getTestnetConfigFactory.js (1)
27-29: Consider environment-specific image versioningGiven the concerns about production readiness in the PR comments, consider implementing environment-specific image versioning. This would allow for different versions in development/testing vs. production environments.
Example approach:
docker: { image: process.env.DASH_CORE_IMAGE || 'dashpay/dashd:22.0.0-devpr6375.dde1edf3', },packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js (2)
Line range hint
126-138: Rename variable 'dip3ActivationHeight' for clarityThe variable
dip3ActivationHeightis used to activate both v19 and v20 forks at height901. To improve code readability and reflect its broader purpose, consider renaming it toactivationHeightorv19v20ActivationHeight.Apply this diff to rename the variable:
- const dip3ActivationHeight = 901; + const activationHeight = 901; const blocksToGenerateInOneStep = 10; let blocksGenerated = 0; let { result: currentBlockHeight, } = await ctx.coreService.getRpcClient().getBlockCount(); do { ({ result: currentBlockHeight, } = await ctx.coreService.getRpcClient().getBlockCount()); await generateBlocks( ctx.coreService, blocksToGenerateInOneStep, NETWORK_LOCAL, // eslint-disable-next-line no-loop-func (blocks) => { blocksGenerated += blocks; observer.next(`${blocksGenerated} blocks generated`); }, ); - } while (dip3ActivationHeight > currentBlockHeight); + } while (activationHeight > currentBlockHeight);
Line range hint
312-338: Ensure consistent variable naming for activation heightsIn the 'Activating v21 fork' task, the variable
dip3ActivationHeightis set to1001. For consistency and clarity, rename it toactivationHeightor specify it asv21ActivationHeight.Apply this diff to update the variable name:
- const dip3ActivationHeight = 1001; + const activationHeight = 1001; const blocksToGenerateInOneStep = 10; let { result: currentBlockHeight, } = await ctx.coreService.getRpcClient().getBlockCount(); do { ({ result: currentBlockHeight, } = await ctx.coreService.getRpcClient().getBlockCount()); await generateBlocks( ctx.coreService, blocksToGenerateInOneStep, NETWORK_LOCAL, // eslint-disable-next-line no-loop-func (blocks) => { blocksGenerated += blocks; observer.next(`${blocksGenerated} blocks generated`); }, ); - } while (dip3ActivationHeight > currentBlockHeight); + } while (activationHeight > currentBlockHeight);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/dashmate/templates/core/dash.conf.dotis excluded by!**/*.dot
📒 Files selected for processing (5)
packages/dashmate/configs/defaults/getLocalConfigFactory.js(1 hunks)packages/dashmate/configs/defaults/getTestnetConfigFactory.js(1 hunks)packages/dashmate/configs/getConfigFileMigrationsFactory.js(2 hunks)packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js(3 hunks)packages/dashmate/src/status/scopes/platform.js(1 hunks)
🔇 Additional comments (10)
packages/dashmate/configs/defaults/getLocalConfigFactory.js (2)
Line range hint 1-143: Verify configuration compatibility with Core v22
The configuration structure looks correct, but we should ensure all options are compatible with Core v22, especially around platform and drive settings.
Let's check for any v22-specific configuration changes:
#!/bin/bash
# Description: Look for Core v22 specific configuration requirements
# Expected: Find documentation or migration files related to v22 config changes
# Search for v22 related configuration changes in docs
fd -t f "README|CHANGELOG|UPGRADE" -X grep -l "v22"
# Look for migration scripts handling v22 changes
fd -t f "migration" -X grep -l "22\.0\.0"Consider adding version-specific configuration validation to prevent misconfigurations when switching between Core versions.
27-29:
Review Docker image version choice
The specified Docker image dashpay/dashd:22.0.0-devpr6375.dde1edf3 appears to be from a development PR build rather than an official release. This raises several concerns:
- Development PR builds may be unstable or contain unfinished features
- The image might be temporary and could be removed after PR completion
- This aligns with the WIP status but requires caution for production use
Let's verify the image availability and details:
Consider the following recommendations:
- For production environments, use stable release tags instead of PR-specific builds
- Document the temporary nature of this image in comments
- Add a TODO to update to an official v22 release once available
Would you like me to propose a safer version configuration approach?
✅ Verification successful
Development PR Docker image used consistently across non-production networks
The Docker image dashpay/dashd:22.0.0-devpr6375.dde1edf3 is consistently used across local, testnet, and devnet configurations, which is appropriate for a work-in-progress v22 upgrade:
- The image is referenced in migration logic specifically for non-production networks
- It's properly excluded from mainnet configurations
- The README acknowledges v22 as a major version upgrade requiring data reset
This implementation aligns with the PR's WIP status for testing v22 nightlies on development environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Docker image existence and get additional details
# Expected: Image should exist in Docker Hub and match PR context
# Check if the image exists in Docker Hub
curl -s "https://hub.docker.com/v2/repositories/dashpay/dashd/tags/?name=22.0.0-devpr6375.dde1edf3"
# Search for any references to this specific version in the codebase
rg "22\.0\.0-devpr6375\.dde1edf3"
Length of output: 608
Script:
#!/bin/bash
# Let's check the README and migration files for context about v22 upgrade
cat packages/dashmate/README.md | grep -A 5 -B 5 "v22\|22\.0"
# Check migration logic for this version
cat packages/dashmate/configs/getConfigFileMigrationsFactory.js | grep -A 10 -B 10 "22\.0\.0-devpr6375"
# Look for any stable v22 references
rg "dashpay/dashd:22\.0\.[0-9]+" --no-filename
Length of output: 1461
packages/dashmate/configs/defaults/getTestnetConfigFactory.js (2)
Line range hint 1-150: Verify configuration compatibility with Core v22
The configuration structure needs verification against Core v22 requirements, especially around platform and tenderdash settings.
Let's check for any v22-specific configuration changes:
#!/bin/bash
# Description: Look for other v22-related configuration changes
# Expected: Find any migration scripts or documentation about v22 requirements
# Search for version-related changes in config and migration files
rg -t js "v22|version 22|22\.0\.0" ./packages/dashmate/27-29:
Version mismatch with PR objectives needs clarification
The docker image version dashpay/dashd:22.0.0-devpr6375.dde1edf3 doesn't match the version mentioned in the PR objectives (v22.0.0-nightly.2024.10.29). This discrepancy needs to be addressed.
Let's verify if this version exists and is the intended one:
packages/dashmate/src/status/scopes/platform.js (3)
Line range hint 242-307: LGTM: Well-structured error handling in getDriveInfo.
The error handling is well-designed with:
- Specific handling for container absence
- Proper error propagation
- Granular try-catch blocks for different operations
Line range hint 19-57: Add version compatibility checks for Core v22.
Given this is a WIP update to Core v22 and there are concerns about production deployment, consider adding explicit version compatibility checks in getCoreInfo.
Let's verify Core v22 RPC interface compatibility:
#!/bin/bash
# Description: Check for potential breaking changes in Core v22 RPC interface
# Expected: No incompatible changes in the RPC methods used
# Search for RPC method definitions and changes
rg -A 5 "mnsync.*status|getBlockchainInfo"
# Search for version-specific handling
rg -A 5 "v22|version.*check"Consider adding:
- Version detection logic
- Fallback handling for different Core versions
- Clear error messages for version mismatches
315-315: LGTM: Improved platform activation status reporting.
The change from time-based to height-based reporting is a good improvement as it:
- Provides more precise activation information
- Removes complexity around time calculations
- Uses consistent blockchain terminology
Let's verify the activation height reporting matches Core v22's configuration:
✅ Verification successful
Based on the search results, I can see that the platform activation height is consistently used in test files and the implementation. The test files show that the getBlockchainInfo RPC call returns the MN_RR softfork height in the format:
{
"result": {
"softforks": {
"mn_rr": { "active": true, "height": 1337 }
}
}
}Let me verify the actual implementation to ensure the height is correctly retrieved:
Platform activation height reporting is correctly implemented
The code correctly retrieves the MN_RR softfork height from Core's getBlockchainInfo RPC call and uses it in the platform activation status message. The implementation:
- Makes the RPC call to get blockchain info
- Destructures the response to get
mn_rrsoftfork data - Uses the height value in the status message
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify platform activation height consistency with Core v22
# Expected: The activation height should match Core v22's configuration
# Search for MN_RR activation height in Core v22 configuration
rg -A 2 "mn_rr.*height"
Length of output: 56612
Script:
#!/bin/bash
# Search for the code that retrieves the MN_RR height from getBlockchainInfo
ast-grep --pattern 'const { result: { softforks: { mn_rr: mnRR } } } = $$$'
Length of output: 429
packages/dashmate/configs/getConfigFileMigrationsFactory.js (2)
7-7: LGTM!
The addition of NETWORK_DEVNET to the imports is consistent with its usage in the migration logic.
1033-1036: Verify the Docker image tag and its availability.
The migration sets a specific development PR build of the Docker image. Please ensure:
- The image tag
22.0.0-devpr6375.dde1edf3is correct and available in the Docker registry - This development build is stable enough for the target networks (local, testnet, devnet)
✅ Verification successful
Docker image dashpay/dashd:22.0.0-devpr6375.dde1edf3 is available and actively maintained
The Docker image exists in the registry and shows healthy indicators:
- Recently pushed (Oct 31, 2024) and actively pulled (last pull: Nov 6, 2024)
- Available for multiple architectures (amd64, arm64)
- All image variants have "active" status
- Published by the official Dash deployment account (evodeployng)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Docker image exists and check its details
# Test: The image should exist in the Docker registry
curl -s "https://hub.docker.com/v2/repositories/dashpay/dashd/tags/22.0.0-devpr6375.dde1edf3"
Length of output: 1846
packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js (1)
95-113: 'Create wallet' task is correctly implemented
The wallet creation parameters align with the expected API, and the implementation is accurate.
packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js
Show resolved
Hide resolved
shumkov
changed the title
shumkov
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/dashmate/configs/defaults/getBaseConfigFactory.js(1 hunks)packages/dashmate/configs/getConfigFileMigrationsFactory.js(2 hunks)packages/dashmate/src/status/scopes/platform.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/dashmate/configs/getConfigFileMigrationsFactory.js
- packages/dashmate/src/status/scopes/platform.js
🔇 Additional comments (1)
packages/dashmate/configs/defaults/getBaseConfigFactory.js (1)
56-56: Verify compatibility and perform thorough testing
Given that:
- This is a major version upgrade
- No testing has been performed according to PR description
- The change affects the base configuration
Please ensure:
- Comprehensive testing of Core v22.0.0-rc.1 compatibility with the Platform
- Verification of all dependent services and configurations
Let's verify the image availability and check for any related configuration files that might need updates:
Issue being fixed or feature implemented
Update Platform to Core 22
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
1.6.0to update Docker image configurations.Improvements
Bug Fixes
dashpay/dashd:22.0.0-rc.1.