feat(dashmate): single node local network [WIP] #2392
Conversation
WalkthroughThis pull request introduces comprehensive changes across multiple packages and configurations, primarily focusing on updating dependencies, configuration settings, and quorum-related functionality. The changes span various components of the Dash ecosystem, including package dependencies, Docker image versions, configuration schemas, and local network setup scripts. The modifications aim to support new quorum types, adjust masternode configurations, and update core library references to a specific branch. Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (10)
.pnp.cjs (1)
2761-2764: Document the reason for using a specific commit.Since this is a WIP PR for a "single node local network" feature, please document:
- Why this specific commit is required
- Whether this is temporary for testing
- The plan to move back to a stable version before final release
Consider adding a comment in the dependency declaration:
["@dashevo/dashcore-lib", [ ["https://github.com/dashpay/dashcore-lib.git#commit=a61df6ae212647a31b3d58fd78bdf544abc5743a", { + // TODO: Using specific commit for single node network feature testing + // Plan to move back to stable version before final release "packageLocation": "./.yarn/cache/@dashevo-dashcore-lib-https-ae0d28c593-be3f570988.zip/node_modules/@dashevo/dashcore-lib/",scripts/setup_local_network.sh (1)
Line range hint
12-15: Document the implications of single-node setupThe setup command should include a comment or warning about:
- Limitations of single-node setup
- Intended use cases (e.g., development only)
- Any features that may not work with single node
packages/dashmate/src/listr/tasks/setup/local/enableSingleCoreQuorumTaskFactory.js (1)
4-7: Improve JSDoc documentation.The current documentation is incomplete. Add more details about the function's purpose, parameters, and return value.
/** + * Factory function that creates a task to enable single core quorum + * * @param {generateBlocks} generateBlocks - Function to generate new blocks + * @param {Function} generateBlocks.generateBlocks - Generates specified number of blocks + * @param {Object} generateBlocks.service - Core service to generate blocks on + * @param {number} generateBlocks.count - Number of blocks to generate + * @param {string} generateBlocks.network - Network identifier + * * @return {enableSingleCoreQuorumTask} */packages/js-dapi-client/package.json (1)
31-31: Fix typo in branch name: "quourm" → "quorum"The branch name contains a typo:
feat/add-quourm-111should befeat/add-quorum-111.packages/dashmate/package.json (1)
59-59: Consider using a stable version instead of a feature branchUsing a Git branch as a dependency source can lead to instability and reproducibility issues:
- The branch content may change unexpectedly
- The branch might be deleted after merging
- There's a typo in the branch name ("quourm" instead of "quorum")
Consider one of these approaches:
- Wait for the feature to be merged and released, then use the stable version
- Use a specific commit hash instead of a branch name
- Fork the repository and use your own stable branch until the feature is released
- "@dashevo/dashcore-lib": "github:dashpay/dashcore-lib#feat/add-quourm-111", + "@dashevo/dashcore-lib": "github:dashpay/dashcore-lib#<commit-hash>",packages/dashmate/src/listr/tasks/setup/local/enableMultiCoreQuorumsTaskFactory.js (1)
30-38: Enhance comments to explain quorum size implicationsWhile the code correctly handles dynamic quorum sizes, the comments could be more descriptive about:
- Why 3 is used as the maximum value
- The implications of having fewer than 3 nodes
- How the quorum behavior might differ with different node counts
Consider expanding the comments:
- // Those are default values for the quorum size 3 with all nodes - // behaving correctly with "llmq_test" quorum + // Maximum quorum size is capped at 3 nodes for "llmq_test" quorum type. + // When fewer nodes are available, the quorum size adjusts automatically: + // - With 3+ nodes: Uses 3 nodes (optimal case) + // - With 2 nodes: Limited functionality quorum + // - With 1 node: Minimal quorum for testingpackages/dashmate/src/listr/tasks/setup/setupLocalPresetTaskFactory.js (1)
152-175: Review quorum configuration parameters.The quorum configuration has potential issues:
activeSignersis hardcoded to 2 for all quorum types, which might be insufficient for larger networks- No validation exists to ensure
activeSignersdoesn't exceed the actual node count- Different
llmqTypevalues (106, 100, 104) are used without documentation explaining their significanceConsider:
- Making
activeSignersconfigurable or proportional to node count- Adding validation to ensure
activeSigners≤ node count- Adding comments explaining the significance of each
llmqTypepackages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js (2)
120-121: Document the magic number in collateral calculation.The addition of 100 to the collateral amount needs explanation:
- const amount = HPMN_COLLATERAL_AMOUNT * (configGroup.length - 1) + 100; + // Calculate collateral: (nodes - seed node) * HPMN_COLLATERAL_AMOUNT + 100 (for transaction fees) + const TRANSACTION_FEE_BUFFER = 100; + const amount = HPMN_COLLATERAL_AMOUNT * (configGroup.length - 1) + TRANSACTION_FEE_BUFFER;
324-327: Consider making activation heights configurable.Hardcoded activation heights (901 and 1001) could make testing different scenarios difficult.
Consider:
- Making these values configurable through environment variables or configuration
- Adding comments explaining the significance of these specific heights
- Adding validation to ensure proper sequencing (v19/v20 before v21)
Also applies to: 131-131
packages/dashmate/src/config/configJsonSchema.js (1)
113-113: Document the new LLMQ type.The addition of LLMQ type
111looks good and maintains backward compatibility. However, please add documentation to explain:
- The purpose and characteristics of this new LLMQ type
- When it should be used (specifically in single node setups)
- Any specific requirements or limitations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
.yarn/cache/@dashevo-dashcore-lib-https-ae0d28c593-be3f570988.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@dashevo-dashcore-lib-npm-0.22.0-9a6dd273b9-ac9e268f6e.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/fsevents-patch-19706e7e35-10.zipis excluded by!**/.yarn/**,!**/*.zipCargo.lockis excluded by!**/*.lockpackages/dashmate/templates/core/dash.conf.dotis excluded by!**/*.dotyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
.pnp.cjs(10 hunks)packages/bench-suite/package.json(1 hunks)packages/dapi/package.json(1 hunks)packages/dash-spv/package.json(1 hunks)packages/dashmate/configs/defaults/getBaseConfigFactory.js(2 hunks)packages/dashmate/configs/defaults/getLocalConfigFactory.js(1 hunks)packages/dashmate/package.json(1 hunks)packages/dashmate/src/commands/setup.js(1 hunks)packages/dashmate/src/config/configJsonSchema.js(1 hunks)packages/dashmate/src/constants.js(1 hunks)packages/dashmate/src/createDIContainer.js(2 hunks)packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js(9 hunks)packages/dashmate/src/listr/tasks/setup/local/enableMultiCoreQuorumsTaskFactory.js(2 hunks)packages/dashmate/src/listr/tasks/setup/local/enableSingleCoreQuorumTaskFactory.js(1 hunks)packages/dashmate/src/listr/tasks/setup/setupLocalPresetTaskFactory.js(2 hunks)packages/js-dapi-client/package.json(1 hunks)packages/js-dash-sdk/package.json(1 hunks)packages/platform-test-suite/package.json(1 hunks)packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json(1 hunks)packages/rs-drive-abci/Cargo.toml(1 hunks)packages/rs-sdk/Cargo.toml(1 hunks)packages/simple-signer/Cargo.toml(1 hunks)packages/wallet-lib/package.json(1 hunks)packages/wasm-dpp/package.json(1 hunks)scripts/setup_local_network.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/rs-sdk/Cargo.toml
🧰 Additional context used
📓 Learnings (1)
packages/rs-drive-abci/Cargo.toml (1)
Learnt from: shumkov
PR: dashpay/platform#2375
File: packages/rs-drive-abci/Cargo.toml:61-63
Timestamp: 2024-12-05T09:29:38.918Z
Learning: In the `drive-abci` package, avoid adding unused dependencies like `hashbrown` to `Cargo.toml`. The team relies on CI to detect dependency version issues.
🔇 Additional comments (22)
.pnp.cjs (1)
2559-2559: Verify security implications of using a specific git commit.
Switching from a published npm version (0.22.0) to a specific git commit requires careful consideration:
- Security: The commit needs to be verified for security vulnerabilities
- Stability: Moving from a stable version to a specific commit might introduce instability
- Maintenance: Git commits are harder to track for updates compared to semantic versions
Also applies to: 2597-2597, 2642-2642, 2734-2734, 2761-2764, 2947-2947, 3042-3042, 3122-3122, 8487-8487, 8566-8566
✅ Verification successful
Using specific git commit is appropriate for this WIP feature
The commit a61df6ae212647a31b3d58fd78bdf544abc5743a is:
- From the official
dashpay/dashcore-librepository - Made by a core contributor (Ivan Shumkov)
- Specifically adds "single node quorum" feature
- Directly related to the PR's "single node local network" objective
- A recent commit (Dec 18, 2024)
The changes are minimal and focused:
- Adds
LLMQ_SINGLE_NODEtype for testing - Implements single-node quorum parameters
- Updates related configurations
Using this specific commit is justified as it provides the exact functionality needed for the WIP feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the git commit and its origin
# Verify the commit exists in the official repository
gh api repos/dashpay/dashcore-lib/commits/a61df6ae212647a31b3d58fd78bdf544abc5743a
# Check if this commit is part of any release tag
gh api repos/dashpay/dashcore-lib/tags --jq '.[] | select(.commit.sha == "a61df6ae212647a31b3d58fd78bdf544abc5743a")'
Length of output: 6402
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json (1)
8-14:
Schema structure appears incomplete and lacks validation constraints
The current schema structure seems overly simplified:
- Both
postandcommentobjects have identical structure - Missing validation constraints that were present in the previous version
- No documentation about the purpose of these properties
This could lead to data integrity issues. Consider:
- Adding proper validation constraints
- Including property descriptions
- Differentiating between post and comment requirements if they serve different purposes
Also applies to: 16-23
packages/simple-signer/Cargo.toml (1)
11-11: Consider using tagged releases instead of commit hashes
While using a specific commit hash (f14bd9048cbc1767541f25b28db5c9ad1236d0da) provides exact version control, it can make dependency management more difficult:
- Tagged releases are easier to track and update
- Commit hashes are harder to verify for stability
- May indicate temporary fix that should be properly released
Consider waiting for a stable tagged release of dashcore-rpc before merging.
scripts/setup_local_network.sh (1)
6-6: Verify minimum requirements for network functionality
Reducing MASTERNODES_COUNT to 1 could impact:
- Quorum-based operations
- Network consensus mechanisms
- Testing scenarios that require multiple nodes
Please ensure:
- All network features work with a single masternode
- Test coverage includes single-node scenarios
- Documentation is updated to reflect this change
✅ Verification successful
Based on the search results, I can see that:
- The
MASTERNODES_COUNTis only used in the local network setup script and passed as--node-countparameter to dashmate setup command - The dashmate setup command has a minimum requirement check that ensures node count is not less than 1
- There are no explicit quorum or consensus requirements found that would be impacted by single node setup
- The codebase shows consensus-related code but it's mostly about data validation and error handling, not network consensus
Let me generate the final response:
Single node setup is valid for local development
The change to MASTERNODES_COUNT=1 is safe as:
- The dashmate setup command enforces minimum of 1 node
- No explicit quorum requirements found in the codebase
- Local network setup is intended for development purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for potential impacts of single node setup
rg -g '!*.{log,lock}' "MASTERNODES_COUNT|node-count"
# Check for quorum-related configurations
rg -g '!*.{log,lock}' "quorum.*minimum|minimum.*quorum"
Length of output: 694
Script:
#!/bin/bash
# Check for quorum or consensus related code
rg -g '!*.{log,lock}' -B2 -A2 'quorum|consensus'
# Look for test files that might be affected
fd "test|spec" --type f --exec rg -l "node.*count|masternode"
# Check documentation for node requirements
fd "README|docs" --type f --exec rg -l "node.*requirement|minimum.*node"
Length of output: 67242
packages/dash-spv/package.json (1)
17-17: Verify stability of the feature branch dependency.
Using a feature branch (feat/add-quourm-111) as a dependency version could lead to instability:
- The branch might be deleted or rebased
- Changes in the branch could break compatibility
- Difficult to track exact version for security audits
Consider:
- Waiting for a stable release
- Using a specific commit hash
- Documenting the temporary nature of this dependency
packages/bench-suite/package.json (1)
11-11: Ensure benchmark compatibility with feature branch.
The benchmark suite needs to maintain accuracy across different versions. Please verify:
- The feature branch doesn't introduce performance characteristics that could skew benchmarks
- All benchmark scenarios are still valid with the new quorum implementation
packages/dashmate/configs/defaults/getLocalConfigFactory.js (1)
Line range hint 69-91: Verify if identical quorum configurations are intentional
All three quorums (validatorSet, chainLock, instantLock) are configured with identical settings:
- llmqType: 111
- dkgInterval: 24
- activeSigners: 2
- rotation: false
While this might be intentional for local testing, different quorum types typically have different requirements.
✅ Verification successful
Let me gather more information about the quorum configurations in the codebase.
Let me search for one more piece of information about local/test configurations.
Identical quorum configurations are intentional for local testing environment
The identical quorum configurations (llmqType: 111) in the local environment are intentional, as evidenced by:
- This is part of
getLocalConfigFactory.jswhich provides a "template for local configs" - Other environments use different quorum types:
- Base config uses different types (4, 2, 5) for validatorSet, chainLock, and instantLock
- Testnet config uses different types (6, 1, 5) respectively
- Local config is simplified with identical values to facilitate local development and testing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other quorum configurations in the codebase for comparison
rg --type js --type json -A 10 "quorum.*llmqType" | grep -v "111"
Length of output: 8465
Script:
#!/bin/bash
# Search for default quorum configurations in the codebase
rg --type js --type json "llmqType.*111" -B 2 -A 2
# Search for any documentation about quorum types
fd -e md -e txt | xargs rg -i "quorum.*type|llmq.*type"
Length of output: 4809
Script:
#!/bin/bash
# Search for documentation or comments about local/test configurations
rg -B 3 -A 3 "local.*config|test.*config" packages/dashmate/configs/defaults/
# Check if there are different quorum configurations for different environments
rg -B 3 -A 3 "llmqType" packages/dashmate/configs/defaults/
Length of output: 6976
packages/platform-test-suite/package.json (1)
27-27: Consider dependency management strategy
Using a feature branch (feat/add-quourm-111) instead of a version for @dashevo/dashcore-lib has implications:
- ✅ Allows testing the new quorum type features
⚠️ May break if the branch is deleted or rebased⚠️ Makes builds non-reproducible
Since this is a test suite in a WIP PR, this is acceptable but should be updated to a proper version before final release.
packages/wasm-dpp/package.json (1)
46-46: Track dependency on feature branch
The dependency on a feature branch feat/add-quourm-111 introduces potential instability. Since this is a WASM package, ensure thorough testing before merging.
Run the following script to check test coverage for dashcore-lib usage:
Consider creating a feature flag or version constraint to manage the transition to the new quorum functionality.
✅ Verification successful
Let me gather more specific information about the test coverage for the WASM package.
The previous search showed several test files in wasm-dpp, but let's verify the specific test coverage and quorum-related usage to make a more informed decision about the dependency change.
Based on the results, I can now generate the final response:
Dependency change appears safe with existing test coverage
The wasm-dpp package has comprehensive test coverage for dashcore-lib usage, particularly in critical areas:
- State transition validation
- Asset lock proofs
- Identity creation and updates
- Key signatures and ECDSA operations
While the dependency points to a feature branch for quorum functionality, the current codebase shows no direct quorum-related usage in wasm-dpp, suggesting this update is likely preparatory and won't impact existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for dashcore-lib imports and their test coverage
echo "Searching for dashcore-lib imports..."
rg -l "require.*dashcore-lib|import.*dashcore-lib"
echo "\nChecking corresponding test files..."
rg -l "require.*dashcore-lib|import.*dashcore-lib" | sed 's/\.js$/.spec.js/' | xargs -I{} test -f {} && echo "Found: {}" || echo "Missing test: {}"
Length of output: 11917
Script:
#!/bin/bash
# Check test files in wasm-dpp package that might be affected by dashcore-lib
echo "Checking test files in wasm-dpp:"
rg -l "dashcore-lib" packages/wasm-dpp/test/
# Check if there are any direct usages of quorum-related functionality
echo -e "\nChecking quorum-related usage:"
rg -i "quorum" packages/wasm-dpp/
Length of output: 1422
packages/dapi/package.json (1)
38-38: Ensure DAPI compatibility with new quorum features
The DAPI package is critical for network interaction. The transition to the quorum feature branch requires careful testing.
Run the following script to verify test coverage for quorum-related functionality:
Consider documenting any API changes that might result from the new quorum functionality in the PR description.
packages/wallet-lib/package.json (1)
49-49: Verify browser compatibility with new quorum features
The wallet library is user-facing and requires browser compatibility. The new quorum functionality needs thorough browser testing.
Run the following script to check browser-specific test coverage:
Consider adding browser-specific test cases for the new quorum functionality in the karma test suite.
✅ Verification successful
Browser compatibility is maintained through proper build configuration
The wallet library has a robust browser compatibility setup:
- Webpack configuration properly polyfills all necessary Node.js modules for browser environments
- Dedicated browser test infrastructure is in place with Karma
- No direct quorum-related code exists in the wallet-lib source, indicating that quorum functionality is properly abstracted through dashcore-lib
- Browser-specific tests are run as part of the CI pipeline through
test:browsersandtest:browsers:functionalscripts
The update to dashcore-lib's quorum feature branch is safe as the wallet-lib maintains proper browser compatibility through its build system and test infrastructure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check browser-specific tests for quorum functionality
echo "Searching for browser-specific quorum tests..."
rg -i "quorum" "tests/functional" "karma"
echo "\nVerifying browser build configuration..."
rg -i "dashcore-lib" "webpack.config.js"
Length of output: 435
Script:
#!/bin/bash
# Let's first find the correct paths for browser-related tests and configs
echo "Finding browser-related test files..."
fd -t f -e js -e json karma
fd -t f webpack.config
echo "\nSearching for browser tests in the entire repo..."
fd -t f -e spec.js -e test.js
echo "\nChecking package.json for browser-related scripts..."
cat packages/wallet-lib/package.json | jq '.scripts'
Length of output: 28189
Script:
#!/bin/bash
# Let's check the browser test configuration and quorum-related code
echo "Checking karma config for browser tests..."
cat packages/wallet-lib/karma/karma.conf.js
echo "\nChecking for quorum-related code in wallet-lib..."
rg -i "quorum" packages/wallet-lib/src/
echo "\nChecking webpack configuration for browser builds..."
cat packages/wallet-lib/webpack.config.js
Length of output: 2083
packages/js-dapi-client/package.json (1)
31-31: Consider using a specific commit hash instead of a branch reference
Using a branch reference (#feat/add-quourm-111) in package.json can lead to inconsistent builds if the branch is updated. Consider using a specific commit hash for better reproducibility.
packages/rs-drive-abci/Cargo.toml (1)
31-31: LGTM! Good practice using a specific commit hash
The change from using a tag to a specific commit hash (rev = "f14bd9048cbc1767541f25b28db5c9ad1236d0da") for the dashcore-rpc dependency is a good practice for ensuring reproducible builds.
packages/js-dash-sdk/package.json (2)
44-44: Same issues as in js-dapi-client package
This file has the same issues as discussed in the js-dapi-client package:
- Typo in branch name ("quourm")
- Using a branch reference instead of a commit hash
Line range hint 2-2: Verify version consistency across packages
The version 4.8.0-dev.1 differs from the 1.8.0-dev.1 version used in other packages. Please verify if this difference is intentional.
✅ Verification successful
Version inconsistency detected across packages
The version check reveals that most packages use 1.8.0-dev.1, but there are a few exceptions:
dash: 4.8.0-dev.1@dashevo/dash-spv: 2.8.0-dev.1
These version differences appear to be intentional since they follow a consistent versioning pattern and are part of different major version lines (4.x.x, 2.x.x vs 1.x.x).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check versions across all package.json files
echo "Checking package versions:"
fd -e json -x jq -r '. | select(.version != null) | .name + ": " + .version' {}
Length of output: 67099
packages/dashmate/src/commands/setup.js (1)
63-64: LGTM: Node count validation updated for single node support
The validation change correctly implements the minimum node count requirement for single node network support.
packages/dashmate/src/listr/tasks/setup/local/enableMultiCoreQuorumsTaskFactory.js (2)
13-18: LGTM: Function renamed to better reflect its purpose
The renaming from enableCoreQuorumsTaskFactory to enableMultiCoreQuorumsTaskFactory better describes the function's role in handling multiple core quorums.
32-36: Verify quorum functionality with reduced node count
The dynamic calculation of quorum parameters needs verification to ensure proper functionality with fewer than 3 nodes.
✅ Verification successful
Let me gather more context about the quorum functionality by checking for related configuration and test files.
Based on the gathered information, I can now provide a final response:
Dynamic quorum size calculation is properly implemented
The code correctly handles scenarios with fewer than 3 nodes:
- Uses
Math.min()to dynamically adjust quorum parameters based on available nodes - Implements
llmq_testtype quorum which is designed for test configurations - No hard requirements or error conditions found for minimum node count
- All quorum parameters (members, commitments, connections, contributions) scale proportionally
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any hardcoded assumptions about quorum size
rg -A 3 "expectedMembers.*3" --type js
rg -A 3 "quorum.*size.*3" --type js
# Look for related test cases
fd -e js -e ts -E node_modules |
xargs rg -l "test.*quorum.*node"
Length of output: 1358
Script:
#!/bin/bash
# Search for quorum-related configuration and implementation
rg -A 5 "llmq_test" --type js
rg -A 5 "quorum.*config" --type js
# Look for any minimum node requirements or validations
rg -A 3 "minimum.*node" --type js
rg -A 3 "min.*node" --type js
# Check for any error handling related to insufficient nodes
rg -B 2 -A 3 "insufficient.*node" --type js
rg -B 2 -A 3 "not enough.*node" --type js
Length of output: 6958
packages/dashmate/src/listr/tasks/setup/setupLocalPresetTaskFactory.js (1)
56-57: Verify the implications of reducing minimum node count.
The change from requiring 3 nodes to allowing single-node setups represents a significant architectural shift. While this enables simpler local development environments, it could impact:
- Network security and consensus mechanisms
- ChainLock functionality
- InstantSend capabilities
packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js (1)
304-312: LGTM: Clean conditional quorum enabling.
The logic for enabling quorums based on node count is well-structured:
- Single quorum for single node setups
- Multi quorums for larger networks
packages/dashmate/src/createDIContainer.js (2)
81-82: LGTM: Clean separation of quorum handling concerns.
The split into separate task factories for multi-core and single-core quorum handling aligns well with the single node local network feature objective.
304-305: Verify task registration dependencies.
The task registrations look good, but we should verify that all dependencies required by these tasks are properly registered in the container.
✅ Verification successful
Let me analyze the dependencies further by checking the imports and constants used in these files.
Based on the analysis of the imports and container registrations, I can now provide the final response:
Dependencies for task registrations are properly configured
All required dependencies for both tasks are correctly registered in the container:
- The tasks themselves are registered as singletons using
asFunction - Both tasks only require the
generateBlocksfunction as a direct dependency, which is injected through their factory functions - All imported utilities (
wait,waitForQuorumPhase, etc.) are standard utility functions that don't require container registration - The constants used (
LLMQ_TYPE_TEST_PLATFORM,LLMQ_TYPE_1,NETWORK_LOCAL) are imported from a constants file - The
Listrdependency is a third-party package imported directly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any missing dependencies in the task factory implementations
# Search for dependencies in the task factory implementations
echo "Checking enableMultiCoreQuorumsTaskFactory dependencies..."
ast-grep --pattern 'function enableMultiCoreQuorumsTaskFactory($_) {
$$$
}'
echo "Checking enableSingleCoreQuorumTaskFactory dependencies..."
ast-grep --pattern 'function enableSingleCoreQuorumTaskFactory($_) {
$$$
}'
Length of output: 36948
Script:
#!/bin/bash
# Check imports and constants for both task factories
echo "Checking imports for enableMultiCoreQuorumsTaskFactory..."
rg -p "^import|^const" packages/dashmate/src/listr/tasks/setup/local/enableMultiCoreQuorumsTaskFactory.js
echo -e "\nChecking imports for enableSingleCoreQuorumTaskFactory..."
rg -p "^import|^const" packages/dashmate/src/listr/tasks/setup/local/enableSingleCoreQuorumTaskFactory.js
echo -e "\nChecking container registrations for dependencies..."
rg "as[A-Za-z]+\((.*)\)\.singleton\(\)" packages/dashmate/src/createDIContainer.js
Length of output: 6518
| "property1": { | ||
| "type": "object" |
There was a problem hiding this comment.
💡 Codebase verification
Schema appears to be a placeholder requiring proper implementation
The document-meta.json schema appears to be in a placeholder state with several concerns:
- Uses generic "property1" naming in multiple places
- Contains minimal structure with just empty object type definitions
- Lacks proper validation constraints for both post and comment objects
- References the same empty property1 definition across different objects
This schema needs a complete implementation with:
- Properly named properties reflecting their purpose
- Appropriate validation rules and constraints
- Complete structure for post and comment metadata
🔗 Analysis chain
Avoid generic property naming in schema definitions
The name property1 is too generic and doesn't convey its purpose. Consider using a more descriptive name that reflects the property's intended use.
Let me check the full content of this schema file to better understand the context and structure.
Let me check if there's a previous version of this schema file in the git history to understand the changes better.
Let me check if there are any related schema files or documentation that might provide context about the intended structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this generic property name is used elsewhere in the codebase
rg -g '*.{json,js,ts}' '"property1"'
Length of output: 300
Script:
#!/bin/bash
# Get the full content of the schema file
cat packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
Length of output: 465
Script:
#!/bin/bash
# Check git history for this file
git log -p --max-count=1 packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
Length of output: 0
Script:
#!/bin/bash
# Look for related schema files and documentation
fd -e json -e md . packages/rs-dpp/schema/
Length of output: 660
| const seedCoreService = ctx.coreServices | ||
| .filter((coreService) => coreService.getConfig().getName() === 'local_seed')[0]; |
There was a problem hiding this comment.
Add error handling for core service lookup.
The core service lookup could fail if no service with name 'local_seed' is found. Add validation and proper error handling.
- const seedCoreService = ctx.coreServices
- .filter((coreService) => coreService.getConfig().getName() === 'local_seed')[0];
+ const seedCoreService = ctx.coreServices
+ .find((coreService) => coreService.getConfig().getName() === 'local_seed');
+
+ if (!seedCoreService) {
+ throw new Error('Local seed core service not found');
+ }📝 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.
| const seedCoreService = ctx.coreServices | |
| .filter((coreService) => coreService.getConfig().getName() === 'local_seed')[0]; | |
| const seedCoreService = ctx.coreServices | |
| .find((coreService) => coreService.getConfig().getName() === 'local_seed'); | |
| if (!seedCoreService) { | |
| throw new Error('Local seed core service not found'); | |
| } |
| let llmq1 = []; | ||
| do { | ||
| ({ result: { [LLMQ_TYPE_1]: llmq1 } } = await seedRpcClient.quorum('list')); | ||
|
|
||
| await generateBlocks( | ||
| seedCoreService, | ||
| 2, | ||
| NETWORK_LOCAL, | ||
| ); | ||
|
|
||
| await wait(300); | ||
| } while (llmq1.length === 0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve quorum polling implementation.
The current implementation has several potential issues:
- No timeout mechanism could lead to infinite polling
- Hard-coded wait time might not be optimal
- No error handling for RPC calls
Consider implementing this improved version:
- let llmq1 = [];
- do {
- ({ result: { [LLMQ_TYPE_1]: llmq1 } } = await seedRpcClient.quorum('list'));
-
- await generateBlocks(
- seedCoreService,
- 2,
- NETWORK_LOCAL,
- );
-
- await wait(300);
- } while (llmq1.length === 0);
+ const MAX_ATTEMPTS = 30;
+ const POLLING_INTERVAL = 300;
+
+ for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt++) {
+ try {
+ const { result } = await seedRpcClient.quorum('list');
+ const llmq1 = result[LLMQ_TYPE_1] || [];
+
+ if (llmq1.length > 0) {
+ return;
+ }
+
+ await generateBlocks(
+ seedCoreService,
+ 2,
+ NETWORK_LOCAL,
+ );
+
+ await wait(POLLING_INTERVAL);
+ } catch (error) {
+ throw new Error(`Failed to check quorum status: ${error.message}`);
+ }
+ }
+
+ throw new Error('Timeout waiting for quorum to be established');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let llmq1 = []; | |
| do { | |
| ({ result: { [LLMQ_TYPE_1]: llmq1 } } = await seedRpcClient.quorum('list')); | |
| await generateBlocks( | |
| seedCoreService, | |
| 2, | |
| NETWORK_LOCAL, | |
| ); | |
| await wait(300); | |
| } while (llmq1.length === 0); | |
| const MAX_ATTEMPTS = 30; | |
| const POLLING_INTERVAL = 300; | |
| for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt++) { | |
| try { | |
| const { result } = await seedRpcClient.quorum('list'); | |
| const llmq1 = result[LLMQ_TYPE_1] || []; | |
| if (llmq1.length > 0) { | |
| return; | |
| } | |
| await generateBlocks( | |
| seedCoreService, | |
| 2, | |
| NETWORK_LOCAL, | |
| ); | |
| await wait(POLLING_INTERVAL); | |
| } catch (error) { | |
| throw new Error(`Failed to check quorum status: ${error.message}`); | |
| } | |
| } | |
| throw new Error('Timeout waiting for quorum to be established'); |
| @@ -29,6 +29,7 @@ export const NODE_TYPE_FULLNODE = 'fullnode'; | |||
|
|
|||
| export const LLMQ_TYPE_TEST = 'llmq_test'; | |||
| export const LLMQ_TYPE_TEST_PLATFORM = 'llmq_test_platform'; | |||
| export const LLMQ_TYPE_1 = 'llmq_1_100'; | |||
There was a problem hiding this comment.
Add mapping for LLMQ_TYPE_1 in QUORUM_TYPES object
The new constant LLMQ_TYPE_1 is added but not mapped in the QUORUM_TYPES object, unlike other LLMQ types. This seems inconsistent with the value 111 used in the local config factory.
Add the mapping to maintain consistency:
export const QUORUM_TYPES = {
LLMQ_TYPE_TEST: 100,
LLMQ_TYPE_TEST_PLATFORM: 106,
+ LLMQ_TYPE_1: 111,
};Committable suggestion skipped: line range outside the PR's diff.
| @@ -68,23 +68,23 @@ export default function getLocalConfigFactory(getBaseConfig) { | |||
| epochTime: 1200, | |||
| validatorSet: { | |||
| quorum: { | |||
| llmqType: 106, | |||
| llmqType: 111, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace magic number with constant from constants.js
The hardcoded value 111 should use the QUORUM_TYPES.LLMQ_TYPE_1 constant for better maintainability and type safety.
- llmqType: 111,
+ llmqType: QUORUM_TYPES.LLMQ_TYPE_1,Also applies to: 79-79, 87-87
| @@ -53,7 +53,7 @@ export default function getBaseConfigFactory() { | |||
| port: 3001, | |||
| }, | |||
| docker: { | |||
| image: 'dashpay/dashd:22', | |||
| image: 'dashpay/dashd:22.1.0-devpr6437.f0e7eb57', | |||
There was a problem hiding this comment.
Review usage of development Docker image.
Using a development PR version (22.1.0-devpr6437.f0e7eb57) of the Core image in base configuration could introduce instability.
Consider:
- Using a stable release version
- If dev features are required, document the specific features needed from this PR
| @@ -310,7 +310,7 @@ | |||
| tenderdash: { | |||
| mode: 'full', | |||
| docker: { | |||
| image: 'dashpay/tenderdash:1', | |||
| image: 'dashpay/tenderdash:feat-support-quorum-111', | |||
There was a problem hiding this comment.
Avoid using feature branch Docker image in production.
Using a feature branch (feat-support-quorum-111) for the Tenderdash image is not recommended for production environments as it:
- May contain unstable code
- Could be rebased or deleted
- Makes deployments non-reproducible
Consider using a stable, tagged release version instead.
# Conflicts: # packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js # packages/dashmate/templates/core/dash.conf.dot
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/dashmate/src/listr/tasks/setup/local/enableMultiCoreQuorumsTaskFactory.js (1)
Line range hint
187-274: Optimize the polling mechanism for quorum enablementThe current implementation uses a fixed 300ms wait time and potentially unnecessary block generations.
Consider implementing an exponential backoff strategy:
- while (!testPlatformQuorumEnabled) { - await wait(300); + const maxAttempts = 10; + let attempt = 0; + while (!testPlatformQuorumEnabled && attempt < maxAttempts) { + await wait(Math.min(300 * Math.pow(2, attempt), 5000)); + attempt++; await generateBlocks( ctx.seedCoreService, 1, NETWORK_LOCAL, );
🧹 Nitpick comments (3)
packages/dashmate/src/listr/tasks/setup/local/enableMultiCoreQuorumsTaskFactory.js (1)
Line range hint
41-186: Consider enhancing error handling in phase transitionsThe DKG session phases are well-structured, but error handling could be improved to handle phase transition failures more gracefully.
Consider wrapping each phase with try-catch and implementing retry logic:
{ title: 'Waiting for phase 1 (init)', task: async (ctx) => { + const maxRetries = 3; + let retryCount = 0; + while (retryCount < maxRetries) { + try { const { result: quorumHash } = await ctx.seedRpcClient.getBestBlockHash(); ctx.quorumHash = quorumHash; // ... existing phase logic ... + break; + } catch (error) { + retryCount++; + if (retryCount === maxRetries) throw error; + await wait(1000 * retryCount); // Exponential backoff + } + } }, },packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js (2)
120-121: Fix typo in commentThere's a typo in the comment: "evnodes" should be "nodes".
- // Generate for evnodes (- a seed node) + some cash for fees + // Generate for nodes (- a seed node) + some cash for fees
295-298: Consider making activation parameters configurableThe comments provide good context, but the activation height and block generation parameters could be made configurable for different environments.
Consider extracting these values to configuration:
+const FORK_ACTIVATION_HEIGHT = 1001; +const BLOCKS_TO_GENERATE_PER_STEP = 10; - const activationHeight = 1001; - const blocksToGenerateInOneStep = 10; + const activationHeight = FORK_ACTIVATION_HEIGHT; + const blocksToGenerateInOneStep = BLOCKS_TO_GENERATE_PER_STEP;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ 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/getBaseConfigFactory.js(2 hunks)packages/dashmate/src/config/configJsonSchema.js(1 hunks)packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js(8 hunks)packages/dashmate/src/listr/tasks/setup/local/enableMultiCoreQuorumsTaskFactory.js(2 hunks)packages/dashmate/src/listr/tasks/setup/setupLocalPresetTaskFactory.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/dashmate/configs/defaults/getBaseConfigFactory.js
- packages/dashmate/src/config/configJsonSchema.js
- packages/dashmate/src/listr/tasks/setup/setupLocalPresetTaskFactory.js
🔇 Additional comments (4)
packages/dashmate/src/listr/tasks/setup/local/enableMultiCoreQuorumsTaskFactory.js (2)
13-13: LGTM: Function renaming improves clarity
The renaming from enableCoreQuorumsTaskFactory to enableMultiCoreQuorumsTaskFactory better reflects its purpose of handling multiple quorums.
Also applies to: 18-18
30-38: Verify the quorum size calculations
The dynamic calculation of quorum parameters using Math.min() is a good approach, but we should verify that these values align with the network's requirements.
✅ Verification successful
Based on the search results, I can see that the quorum size of 3 used in the code is consistent with the test configuration in the codebase. Specifically, in packages/rs-drive-abci/src/config.rs, there are multiple test configurations that use a quorum size of 3, which matches the values being set in enableMultiCoreQuorumsTaskFactory.js.
Let me verify one more aspect by checking if there are any specific test quorum configurations:
Based on the search results, I can see that the quorum size of 3 used in the code is specifically for the "llmq_test" quorum type, which is a test configuration. This is confirmed by:
- The comment in the code explicitly states: "Those are default values for the quorum size 3 with all nodes behaving correctly with 'llmq_test' quorum"
- The constant
LLMQ_TYPE_TESTis defined inpackages/dashmate/src/constants.js - The test quorum configuration is used in various test files with a size of 3
Let me verify one final aspect about test quorum configurations:
Based on the comprehensive analysis of the codebase, I can now provide a final response:
Quorum size calculations are correctly implemented
The values used in the code align with the test quorum requirements:
- The size of 3 is specifically used for "llmq_test" quorum type
- The
Math.min()approach ensures the quorum parameters never exceed the available masternode services - The proportions between members, commitments, and connections are maintained correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded quorum size requirements in the codebase
# Search for quorum size related constants or configurations
rg -A 3 "quorum.*size|QUORUM.*SIZE"
# Search for any validation of quorum members count
ast-grep --pattern 'if ($members $_) {
$$$
}'
Length of output: 35406
Script:
#!/bin/bash
# Search for test quorum configurations
rg -A 3 "llmq_test|test.*quorum"
Length of output: 60744
Script:
#!/bin/bash
# Search for test quorum configuration files
rg -A 3 "llmq_test.*config|test.*quorum.*config"
Length of output: 50
packages/dashmate/src/listr/tasks/setup/local/configureCoreTaskFactory.js (2)
19-20: LGTM: Parameter changes support new quorum functionality
The addition of enableSingleCoreQuorumTask alongside enableMultiCoreQuorumsTask provides better granularity for quorum management.
Also applies to: 34-35
275-283: LGTM: Clean separation of single and multi quorum scenarios
The conditional execution based on node count provides a clear separation between single and multi quorum scenarios. The conditions are mutually exclusive and handle both cases appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/dashmate/src/core/waitForCorePeersConnected.js (1)
Line range hint
1-21: Add error handling for RPC callsThe RPC call to
getPeerInfocould fail, but there's no error handling. This could lead to unhandled exceptions.export default async function waitForCorePeersConnected(rpcClient) { let hasPeers = false; do { - const { result: peers } = await rpcClient.getPeerInfo(); + try { + const { result: peers } = await rpcClient.getPeerInfo(); + hasPeers = peers && peers.length > 0; + } catch (error) { + console.error('Failed to get peer info:', error.message); + // Wait before retry to avoid hammering the node + await wait(1000); + continue; + } - hasPeers = peers && peers.length > 0; if (!hasPeers) { await wait(1000); } } while (!hasPeers); }
🧹 Nitpick comments (2)
packages/dashmate/src/core/waitForCorePeersConnected.js (2)
19-19: Consider environment-specific wait timesWhile reducing the wait time to 1 second makes sense for local network setup, consider making this configurable based on the environment to avoid potential issues if this code is reused in other contexts.
+const PEER_CHECK_INTERVAL_MS = process.env.PEER_CHECK_INTERVAL_MS || 1000; + export default async function waitForCorePeersConnected(rpcClient) { let hasPeers = false; do { const { result: peers } = await rpcClient.getPeerInfo(); hasPeers = peers && peers.length > 0; if (!hasPeers) { - await wait(1000); + await wait(PEER_CHECK_INTERVAL_MS); } } while (!hasPeers); }
Line range hint
3-8: Enhance JSDoc documentationThe current documentation could be more detailed to better explain the function's behavior and parameters.
/** * Wait Core to connect to peers + * + * @description Polls the Core node until at least one peer is connected. + * Implements a retry mechanism with a 1-second delay between attempts. * - * @typedef {waitForCorePeersConnected} * @param {RpcClient} rpcClient - The RPC client instance for communicating with the Core node + * @throws {Error} If the RPC call fails * @return {Promise<void>} Resolves when at least one peer is connected */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/dashmate/src/core/waitForCorePeersConnected.js(1 hunks)
🔇 Additional comments (1)
packages/dashmate/src/core/waitForCorePeersConnected.js (1)
Line range hint 1-21: Verify the impact on local network setup time
Since this is part of the single node local network feature, we should verify that the reduced wait time actually improves the local network setup experience without causing issues.
✅ Verification successful
Reduced wait time in peer connection check is safe to implement
The function is used in node startup tasks (both single and group nodes) where it ensures nodes have peers before proceeding. The 1-second polling interval is appropriate because:
- It's used as a startup prerequisite check, not in a performance-critical path
- It's called only once during node initialization in
startNodeTaskFactoryandstartGroupNodesTaskFactory - The function exits as soon as peers are detected, so the polling duration is self-limiting
- No test files are directly dependent on the timing, suggesting it's not a critical timing-sensitive operation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other files that might be affected by the reduced wait time
# Look for test files or setup scripts that use this function
echo "Searching for references to waitForCorePeersConnected..."
rg -l "waitForCorePeersConnected"
echo "Searching for related test files..."
fd -g "*test*.js" -g "*spec*.js" | rg -l "waitForCorePeersConnected"
Length of output: 624
Script:
#!/bin/bash
# Let's examine how this function is used in the identified files
echo "Checking createDIContainer.js..."
rg -A 5 -B 5 "waitForCorePeersConnected" packages/dashmate/src/createDIContainer.js
echo -e "\nChecking startNodeTaskFactory.js..."
rg -A 5 -B 5 "waitForCorePeersConnected" packages/dashmate/src/listr/tasks/startNodeTaskFactory.js
echo -e "\nChecking startGroupNodesTaskFactory.js..."
rg -A 5 -B 5 "waitForCorePeersConnected" packages/dashmate/src/listr/tasks/startGroupNodesTaskFactory.js
# Let's also check if there are any test files with a different pattern
echo -e "\nChecking for test files with different patterns..."
fd "test" | grep -i "test" | rg -l "waitForCorePeersConnected"
Length of output: 3437
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
Dependency Updates
@dashevo/dashcore-libdependency reference to a specific GitHub branch across multiple packages.Bug Fixes
node-countflag to allow a minimum of 1.Documentation
Chores