feat(store/v2): backward compatibility store v1 historical queries #23145
Conversation
|
Warning Rate limit exceeded@randygrok has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 38 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the store version 2 commitment management system. The changes focus on improving version and migration height tracking across multiple files in the store package. Key modifications include adding methods to retrieve and set migration heights, refactoring version retrieval logic, and updating the query mechanism to handle migrations between version 1 and version 2 state commitments. The changes aim to provide more robust historical query support and clearer version management. Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
…dy:cosmos/cosmos-sdk into feat/backward-compatibility-store-v2
randygrok
requested review from
cool-develope,
kocubinski and
a team
as code owners
|
@randygrok your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
store/v2/root/store.go (2)
39-41: Consider documenting the new field for clarity.
This field provides a fallback for v1 commitments after a migration. A brief comment explaining when and whyv1StateCommitmentshould be used would improve clarity.
390-398: Evaluate concurrency and memory usage for preserving v1 state store.
After migration completes, the existing store is stored inv1StateCommitmentwithout closing it. This design supports historical queries but retains resources. Ensure that no unintended concurrency or resource leaks occur, and confirm that the old store data is eventually pruned or closed as intended.store/v2/commitment/metadata.go (1)
70-78: Consider adding validation to setV2MigrationHeight.
For safety, consider preventing negative or decreasing heights. Otherwise, no concerns.store/v2/root/migrate_test.go (1)
87-87: Migration triggered by LoadLatestVersion.
This is a valid approach since loading the store version can initiate background migrations. Be sure external documentation is updated to reflect this behavior.store/v2/commitment/store.go (2)
236-244: Consider validating the requested migration height.Currently, there's no safeguard to ensure the new migration height is not lower than an already-set height. Unless intentionally allowed, consider adding a check to prevent inadvertently downgrading the migration height, which might cause inconsistencies in the store’s versioning logic.
func (c *CommitStore) SetV2MigrationHeight(height uint64) error { + current, err := c.metadata.GetV2MigrationHeight() + if err != nil { + return err + } + if height < current { + return fmt.Errorf("cannot set migration height to %d which is lower than the current %d", height, current) + } err := c.metadata.setV2MigrationHeight(height) if err != nil { return err } return nil }
246-249: Return a default or handle an unset height gracefully.Consider clarifying how an unset (zero) height should be interpreted, or if it should trigger a specific error message. This can help users distinguish between a valid height of zero and an unset migration state.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
store/v2/commitment/metadata.go(4 hunks)store/v2/commitment/metadata_test.go(1 hunks)store/v2/commitment/store.go(1 hunks)store/v2/migration/manager.go(1 hunks)store/v2/root/migrate_test.go(7 hunks)store/v2/root/store.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
store/v2/commitment/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/migration/manager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/root/migrate_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
store/v2/commitment/metadata_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
store/v2/root/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/commitment/metadata.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (16)
store/v2/root/store.go (2)
14-14: No concerns about this import.
180-218: Validatev1StateCommitmentbefore usage.
The query logic distinguishes between old (v1) and new (v2) commitments based on the migration height. Whenversion <= v2UpgradeHeight, the code falls back tos.v1StateCommitment, which is set after migration completes. If a query arrives beforev1StateCommitmentis set, this could lead to a nil pointer reference.store/v2/commitment/metadata_test.go (2)
11-26: Good coverage of GetLatestVersion.
The test thoroughly checks the default version of 0 and a subsequent updated version of 10. Consider adding an extra scenario testing extremely large versions or error pathways to ensure robust edge-case handling.
28-42: Sufficient testing of GetV2MigrationHeight.
The test covers initial zero and updated height. No concerns regarding correctness.store/v2/commitment/metadata.go (4)
16-17: Meaningful constant for migration height key.
Defining a dedicated key for v2 migration height is fine. No immediate issues found.
Line range hint
33-50: Internal helper getVersion is well-structured.
Error handling is clear, and returning 0 if no data is found is consistent.
51-54: Refactored GetLatestVersion is concise and maintainable.
Delegating logic togetVersionis a good design choice that reduces code duplication.
65-68: Straightforward retrieval of migration height.
UsinggetVersionensures consistent handling of decoding logic.store/v2/migration/manager.go (1)
110-115: Properly updates the migration height for historical queries.
Setting the migration height right after restoration ensures future queries can differentiate v1 vs. v2 data. Confirm that this value remains consistent if a redo or partial rollback is needed.store/v2/root/migrate_test.go (7)
71-77: Multi-line snapshot manager construction improves readability.
No functional issues.
96-101: Querying fallback logic for older versions.
Excellent coverage of the fallback scenario. Ensuring queries at all historic versions remain valid is vital for backward compatibility.
115-120: Well-structured approach to pushing new changesets during migration.
Code is readable and ensures that old versions keep being tested in parallel with new writes.
150-155: Proper usage of a targetVersion variable.
This helps map queries to the correct version boundary. Good approach for bridging old vs. new store.
167-172: Continued changeset application validated.
Ensures the migrated store can also handle new writes post-migration without issues.
183-189: Verifies the new migration height in the commit store.
This check ensures the store properly stored the original version for fallback.
190-204: Post-migration fallback queries tested without proofs.
This final loop accurately checks whether queries still return the expected data.
|
|
||
| // if the version is less than or equal to the v2 migration height, we need to use the v1 state commitment | ||
| if version <= v2UpgradeHeight { | ||
| cs = s.v1StateCommitment |
There was a problem hiding this comment.
we should check if the old store is still present, it may have been removed.
julienrbrt
changed the title
Co-authored-by: Marko <marko@baricevic.me>
Co-authored-by: Marko <marko@baricevic.me>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
store/v2/root/store.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
store/v2/root/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (4)
store/v2/root/store.go (4)
39-41: LGTM! Well-documented field addition.The new
v1StateCommitmentfield is appropriately typed and documented, clearly indicating its purpose for storing pre-migration state commitment.
213-218: LGTM! Clean return structure.The QueryResult structure is well-organized and includes all necessary fields.
396-398: LGTM! Clear state commitment handover.The handover of the old state commitment to v1StateCommitment is clear and straightforward.
391-395: Clarify the state commitment closure strategy.The commented-out code block suggests uncertainty about the closure strategy. This needs to be resolved to prevent potential resource leaks.
Please clarify:
- Should we keep the old state commitment open for historical queries?
- If yes, when and where should it be closed?
Run this script to check for other state commitment closure patterns in the codebase:
✅ Verification successful
Commented-out code is correct - cleanup is handled in Store.Close()
The old state commitment (stored in
v1StateCommitment) should remain open during migration to maintain access to historical data. The cleanup is properly handled in the Store's Close() method, which closes both state commitments when the store is shut down.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for state commitment closure patterns ast-grep --pattern 'stateCommitment.Close()'Length of output: 46
Script:
#!/bin/bash # Search for state commitment related patterns with broader context rg -i "statecommitment.*close" -A 2 -B 2 # Search for Close method implementations and interfaces ast-grep --pattern 'Close() error' # Search for state commitment initialization patterns rg -i "new.*statecommitment" -A 2 -B 2Length of output: 1639
…dy:cosmos/cosmos-sdk into feat/backward-compatibility-store-v2
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Release Notes
New Features
Improvements
Testing
These updates improve the system's ability to manage and track version migrations, ensuring more robust historical data handling and query mechanisms.