fix(drive): using new rust dash core methods for reversed quorum hash to maintain backwards compatibility #2489
Conversation
WalkthroughThis pull request updates multiple dependency declarations in various Cargo.toml files, replacing specific commit revisions ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockCargo.lockis excluded by!**/*.lockCargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
packages/rs-dpp/Cargo.toml(1 hunks)packages/rs-drive-abci/Cargo.toml(1 hunks)packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/rpc/core.rs(5 hunks)packages/rs-sdk/Cargo.toml(2 hunks)packages/simple-signer/Cargo.toml(1 hunks)packages/rs-drive-abci/src/platform_types/validator_set/v0/mod.rs(3 hunks)packages/rs-dpp/Cargo.toml(1 hunks)packages/rs-drive-abci/Cargo.toml(1 hunks)packages/rs-drive-abci/src/platform_types/validator_set/v0/mod.rs(0 hunks)packages/rs-drive-abci/src/rpc/core.rs(2 hunks)packages/rs-sdk/Cargo.toml(1 hunks)packages/simple-signer/Cargo.toml(1 hunks)packages/rs-dpp/Cargo.toml(1 hunks)packages/rs-drive-abci/Cargo.toml(1 hunks)packages/rs-sdk/Cargo.toml(1 hunks)packages/simple-signer/Cargo.toml(1 hunks)
🔇 Additional comments (11)
packages/rs-drive-abci/Cargo.toml (1)
31-31:❓ Verification inconclusive
Ensure compatibility with specific commit reference
This change pins the dashcore-rpc dependency to a specific commit hash instead of using a version tag. This appears to be related to the "reversed quorum hash" issue mentioned in the PR objectives.
While other packages are being updated to use tag v0.38.0, this package specifically needs this commit reference. This discrepancy might be intentional to implement the temporary workaround mentioned in the PR description.
🏁 Script executed:
#!/bin/bash # Verify if this specific commit is part of or related to v0.38.0 git clone https://github.com/dashpay/rust-dashcore .temp_dashcore cd .temp_dashcore echo "Checking commit message for the specified revision:" git show 217197c796ff296f428593b391a00f9200beceac --quiet echo -e "\nChecking relationship to v0.38.0 tag:" git merge-base --is-ancestor 217197c796ff296f428593b391a00f9200beceac v0.38.0 && echo "Commit is an ancestor of v0.38.0" || echo "Commit is NOT an ancestor of v0.38.0"Length of output: 766
Action: Verify and Clarify Dependency Pinning Strategy
We've confirmed via the verification script that the commit
217197c796ff296f428593b391a00f9200beceac
(which adds theget_quorum_info_reversedfeature) is indeed an ancestor of thev0.38.0tag in the dashcore-rpc repository. This means that thev0.38.0tag includes the fix for the reversed quorum hash issue.
- If the intent is to strictly lock in the commit that introduces the reversed quorum fix—as a temporary workaround—you may want to update the dependency to use the explicit commit reference (for example, using
rev = "217197c796ff296f428593b391a00f9200beceac"in the Cargo.toml) rather than relying on the tag.- If you’re comfortable that the immutability of the
v0.38.0tag guarantees inclusion of the necessary fix, then continuing to use the tag is acceptable. In that case, please document this decision to clarify the intentional deviation from other packages.Please verify that the chosen strategy aligns with the overall dependency management plan.
packages/rs-sdk/Cargo.toml (1)
41-41: Dependency updated to stable version tagThe dashcore-rpc dependency is now using a version tag (v0.38.0) instead of a specific commit reference. This standardizes dependency management and makes it easier to track which version is being used.
Note that this differs from the approach in rs-drive-abci which uses a specific commit. Ensure that both approaches are compatible, as this difference appears to be intentional based on the PR description about implementing a temporary workaround.
packages/simple-signer/Cargo.toml (1)
11-11: Dependency updated to stable version tagSimilar to the update in rs-sdk, this change moves from a specific commit reference to a stable version tag (v0.38.0) for the dashcore-rpc dependency. This standardizes dependency management across most packages.
This approach is consistent with the changes in rs-sdk, but differs from rs-drive-abci which uses a specific commit - a discrepancy that appears intentional based on the PR description.
packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/v0/mod.rs (1)
293-293: Performance optimization by avoiding cloneThe change from
validator_set.threshold_public_key().clone()to*validator_set.threshold_public_key()is a performance optimization. Instead of creating a new copy via clone(), the code now creates a copy by dereferencing the pointer, which is more efficient for this use case.This change aligns with the purpose of fixing the quorum hash issue while ensuring optimal performance. The code should still function correctly since this change only affects how the data is copied, not the logic itself.
packages/rs-drive-abci/src/platform_types/validator_set/v0/mod.rs (3)
152-156: Improved tracing for quorum hash in update_difference methodAdding structured tracing for the quorum hash is a good improvement for troubleshooting and observability, especially given the context of the PR addressing quorum hash-related issues.
177-181: Consistent tracing added to to_update methodGood addition of structured logging that maintains consistency with the tracing pattern established in the update_difference method.
231-235: Tracing extension to to_update_owned method is appropriateThis completes the tracing coverage across all three relevant methods that deal with quorum hash operations.
packages/rs-drive-abci/src/rpc/core.rs (4)
8-8: Added Hash trait importAdded necessary import for the Hash trait to support the quorum hash operations.
172-204: Improved retry mechanism implementationThe retry macro has been updated to use a more functional approach with
find_mapthat provides better control flow and error handling. The changes look good and maintain the original retry behavior while improving the code structure.
274-291: Implemented workaround for quorum list with reversed hashThis implementation aligns with the PR objective to address the QuorumHash issue in dashcore. Using the reversed version of the RPC call with added tracing provides both the fix and the observability needed for this temporary solution.
300-314: Consistent implementation of quorum info with reversed hashSimilar to the list implementation, this method now correctly uses the reversed version of the RPC call with appropriate tracing, completing the workaround for the QuorumHash issue.
| "signer", | ||
| "serde", | ||
| ], default-features = false, tag = "v0.37.0" } | ||
| ], default-features = false, tag = "v0.38.0" } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Using a specific commit hash instead of semantic version
The change from a semantic version tag to a specific commit reference is likely part of the workaround mentioned in the PR objective for fixing the reversed QuorumHash issue. While pinning to an exact commit increases reproducibility, it makes dependency management harder in the future.
Consider adding a comment in the code or creating a follow-up ticket to eventually revert back to semantic versioning once a permanent solution for the QuorumHash issue is implemented.
🏁 Script executed:
#!/bin/bash
# Check where the specific commit falls in the version history
echo "Checking if this commit is between v0.36.0 and v0.37.0 or relates to quorum hash fixes"
git ls-remote --tags https://github.com/dashpay/rust-dashcore | grep -E "v0.3[67]"
curl -s https://api.github.com/repos/dashpay/rust-dashcore/commits/217197c796ff296f428593b391a00f9200beceac | jq '.commit.message'Length of output: 671
Action: Document Temporary Dependency Pinning
The dependency declaration in packages/rs-dpp/Cargo.toml is currently using an explicit tag ("v0.38.0") for the rust-dashcore dependency. Our verification confirms that the pinned commit (with the message "feat: add get_quorum_info_reversed") is indeed the temporary workaround addressing the reversed quorum issue. However, pinning to this specific reference instead of using a semantic version adds long-term maintenance overhead.
- Location:
packages/rs-dpp/Cargo.toml, line 32. - Impact: This change locks the dependency to a commit that may not receive compatible future updates.
- Recommendation:
- Add an inline comment explaining that the explicit tag is a temporary measure for the reversed quorum issue.
- Consider creating a follow-up ticket to revert to semantic versioning once a permanent fix is available.
There was a problem hiding this comment.
I'm aware, but we aren't publishing crates at the moment so we have to use repo + tag
There was a problem hiding this comment.
@shumkov Thanks for the clarification. You're right - using a Git repository with tags is the appropriate approach when working with dependencies that aren't published to crates.io. The tag-based versioning you're using (v0.38.0) is actually preferable to commit hashes as it maintains semantic version information while addressing your current workflow constraints.
🐰 nods in understanding
✏️ Learnings added
Learnt from: shumkov
PR: dashpay/platform#2489
File: packages/rs-dpp/Cargo.toml:32-32
Timestamp: 2025-03-11T09:39:22.632Z
Learning: In the Dash Platform project, dependencies are currently managed using Git repository references with tags (repo+tag format in Cargo.toml) rather than published crates, as the team is not currently publishing crates to crates.io.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
QuantumExplorer
changed the title
github-project-automation
bot
moved this from In review / testing
to Done
in Platform team
chore: update to latest dash core 37 (#2483) feat(platform)!: token advanced distribution and updates (#2471) fix: token history contract (#2474) Co-authored-by: Ivan Shumkov <[email protected]> Co-authored-by: QuantumExplorer <[email protected]> fix(drive): using new rust dash core methods for reversed quorum hash to maintain backwards compatibility (#2489) feat: more granular integer document property types (#2455) Co-authored-by: Quantum Explorer <[email protected]> docs: update comment for data contract code range (#2476) feat: validate token name localizations (#2468) feat(sdk): get identity by non-unique keys build(deps): update grovedb to current develop test: test identity by non-unique pubkey hashes fix(sdk): dash core client fails to get quorum chore: minor fixes test(drive-abci): identity by non-unique pubkey start after chore: minor changes to verify feat(sdk): token and group queries (#2449) chore: revert limit 1 => limit none chore: add non-unique key to test identities test(sdk): test vectors for test_fetch_identity_by_non_unique_public_keys fix(platform)!: token distribution fixes and tests (#2494) chore(platform): bump to version 2.0.0-dev.1 (#2495) test: update assertion fix(sdk): make some things public (#2496) feat(platform): require token for document actions (#2498) fix: data contract proof doesn't work with new auto fields (#2501)
chore: update to latest dash core 37 (#2483) feat(platform)!: token advanced distribution and updates (#2471) fix: token history contract (#2474) Co-authored-by: Ivan Shumkov <[email protected]> Co-authored-by: QuantumExplorer <[email protected]> fix(drive): using new rust dash core methods for reversed quorum hash to maintain backwards compatibility (#2489) feat: more granular integer document property types (#2455) Co-authored-by: Quantum Explorer <[email protected]> docs: update comment for data contract code range (#2476) feat: validate token name localizations (#2468) feat(sdk): get identity by non-unique keys build(deps): update grovedb to current develop test: test identity by non-unique pubkey hashes fix(sdk): dash core client fails to get quorum chore: minor fixes test(drive-abci): identity by non-unique pubkey start after chore: minor changes to verify feat(sdk): token and group queries (#2449) chore: revert limit 1 => limit none chore: add non-unique key to test identities test(sdk): test vectors for test_fetch_identity_by_non_unique_public_keys fix(platform)!: token distribution fixes and tests (#2494) chore(platform): bump to version 2.0.0-dev.1 (#2495) test: update assertion fix(sdk): make some things public (#2496) feat(platform): require token for document actions (#2498) fix: data contract proof doesn't work with new auto fields (#2501)
3 participants
Issue being fixed or feature implemented
dashcore <=0.36 had a bug with non reverted QuorumHash, in version 0.37 it was fixed. This fix breaks a lot of logic and introduces breaking changes to the state.
What was done?
retry!macros for core to enable code after the macro usage. It was required for debug output during testing.How Has This Been Tested?
Running local network
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Chores
Refactor
These improvements help maintain system stability and performance while preserving current capabilities.