feat(store/v2): implement the feature to upgrade the store keys #20453
Conversation
|
Warning Rate limit exceeded@cool-develope has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 54 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. WalkthroughWalkthroughThe recent changes enhance Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant KVStore
participant UpgradeStore
participant Database
User->>KVStore: Request Store Upgrade
KVStore->>UpgradeStore: LoadVersionAndUpgrade(upgrades)
UpgradeStore->>Database: MigrateStoreKey(oldKey, newKey)
Database-->>UpgradeStore: Key Migration Status
UpgradeStore-->>KVStore: Migration Complete
KVStore-->>User: Confirm Upgrade
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
@cool-develope 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: 3
Outside diff range and nitpick comments (1)
store/v2/commitment/store.go (1)
Line range hint
124-179: The refactoring ofLoadVersionto support store upgrades and rollbacks is comprehensive. Consider adding detailed comments within the method to explain the handling of store keys and the conditions under which versions are rolled back or initialized.// LoadVersion updates the store to the target version. If the target version is less than the latest, it rolls back to that version. // It also handles upgrades by setting initial versions for new or renamed store keys. func (c *CommitStore) LoadVersion(targetVersion uint64, upgrades *corestore.StoreUpgrades) error { // existing code... }
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- store/v2/commitment/store.go (4 hunks)
- store/v2/database.go (2 hunks)
- store/v2/root/store.go (2 hunks)
Additional Context Used
Path-based Instructions (3)
store/v2/database.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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/root/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (5)
store/v2/database.go (1)
59-59: The updated comment forSetInitialVersionprovides clearer information on its functionality.store/v2/commitment/store.go (3)
9-9: The addition of thesortpackage supports the new sorting functionality inLoadVersion, ensuring deterministic iteration order for store upgrades.
148-155: The sorting logic ensures that store keys are processed in a consistent order when applying upgrades, which is crucial for maintaining state consistency across versions.
497-497: The modification to callLoadVersionat the end of theRestoremethod helps ensure the store is correctly updated post-restore. Verify that this change does not introduce performance issues or affect data integrity.store/v2/root/store.go (1)
218-218: The update toLoadLatestVersionto callloadVersiondirectly simplifies the version loading process and maintains consistency with other version loading methods.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (9)
- core/store/upgrade.go (1 hunks)
- store/v2/commitment/iavl/tree.go (1 hunks)
- store/v2/commitment/store.go (5 hunks)
- store/v2/commitment/store_test_suite.go (2 hunks)
- store/v2/commitment/tree.go (2 hunks)
- store/v2/database.go (3 hunks)
- store/v2/pruning/manager.go (2 hunks)
- store/v2/root/store.go (3 hunks)
- store/v2/root/upgrade_test.go (1 hunks)
Additional context used
Path-based instructions (9)
core/store/upgrade.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/commitment/tree.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/database.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/pruning/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/commitment/iavl/tree.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/upgrade_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/store_test_suite.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
golangci-lint
store/v2/pruning/manager.go
13-13: undefined: store
15-15: undefined: store
17-17: undefined: store
19-19: undefined: store
23-23: undefined: store
store/v2/commitment/iavl/tree.go
19-19: undefined: store
29-29: undefined: Config
30-30: undefined: iavl.AsyncPruningOption
116-116: t.tree.SetCommitting undefined (type *"github.com/cosmos/iavl".MutableTree has no field or method SetCommitting)
118-118: t.tree.UnsetCommitting undefined (type *"github.com/cosmos/iavl".MutableTree has no field or method UnsetCommitting)
133-133: undefined: Exporter
145-145: undefined: Importer
store/v2/root/upgrade_test.go
24-24: undefined: store
28-28: undefined: MigrateStoreTestSuite
37-37: undefined: storeKeys
49-49: undefined: New
57-57: undefined: storeKeys
73-73: undefined: storeKeys
89-89: undefined: New
store/v2/commitment/store_test_suite.go
29-29: undefined: CommitStore
129-129: undefined: store
store/v2/root/store.go
33-33: undefined: store
36-36: undefined: store
22-22: undefined: store
67-67: undefined: store
68-68: undefined: store
72-72: undefined: store
130-130: undefined: store
134-134: undefined: store
166-166: undefined: store
115-115: undefined: NewReaderMap
127-127: undefined: NewReaderMap
177-177: undefined: store
182-182: undefined: store
191-191: undefined: store
197-197: undefined: store
206-206: undefined: store
store/v2/commitment/store.go
46-46: undefined: Tree
32-32: undefined: store
34-34: undefined: store
35-35: undefined: store
50-50: undefined: Tree
491-491: undefined: ErrorExportDone
523-523: undefined: Importer
525-525: storeKey declared and not used
8-8: "math" imported and not used
Additional comments not posted (10)
core/store/upgrade.go (1)
44-55: The implementation ofIsRenamedmethod is correct and aligns with the PR objectives to handle store key upgrades.store/v2/commitment/tree.go (1)
61-66: TheKVStoreGetterinterface and its methodGetKVStoreWithBatchare correctly implemented and essential for the upgrade functionality.store/v2/database.go (1)
40-40: Ensure all implementations of theCommitterinterface are updated to include the newupgradesparameter in theLoadVersionmethod.store/v2/pruning/manager.go (1)
76-118: ThePruneKVStoresmethod is correctly implemented and handles the pruning of KVStores based on the batch flush threshold efficiently.store/v2/commitment/iavl/tree.go (1)
17-40: TheIavlTreestruct correctly implements the required interfaces and provides essential functionality for the upgrade process.Tools
golangci-lint
19-19: undefined: store
29-29: undefined: Config
30-30: undefined: iavl.AsyncPruningOption
store/v2/root/upgrade_test.go (1)
20-125: TheUpgradeStoreTestSuiteis well-implemented and provides comprehensive test coverage for the store upgrade functionality.Tools
golangci-lint
24-24: undefined: store
28-28: undefined: MigrateStoreTestSuite
37-37: undefined: storeKeys
49-49: undefined: New
57-57: undefined: storeKeys
73-73: undefined: storeKeys
89-89: undefined: New
store/v2/commitment/store_test_suite.go (2)
22-22: New constantstoreKey3added for testing.This constant is likely used to simulate a scenario with an additional store key, which is relevant for the new upgrade tests.
171-229: New test functionTestStore_Upgradesadded.This function tests the upgrade process by simulating store operations before and after applying store upgrades. It checks for correct behavior when adding, renaming, and deleting store keys. The test is comprehensive and covers the critical functionality introduced in this PR.
store/v2/root/store.go (1)
224-224: UpdatedLoadVersionAndUpgradeandloadVersionmethods to handle store upgrades.These changes are crucial for enabling the store to handle version upgrades and apply necessary modifications to the store keys as specified by the upgrades. The implementation checks for migration status and correctly integrates the pruning logic for deleted and renamed keys.
Also applies to: 233-233, 237-271, 273-276
store/v2/commitment/store.go (1)
Line range hint
120-223: UpdatedLoadVersionmethod and addedmigrateKVStoremethod to handle store upgrades.The
LoadVersionmethod now supports rolling back or creating new commit info based on the target version, which is essential for handling snapshots and upgrades. ThemigrateKVStoremethod provides the functionality to migrate data between keys, which is necessary for handling renamed keys during upgrades.Also applies to: 225-282
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- store/v2/root/upgrade_test.go (1 hunks)
Additional context used
Path-based instructions (1)
store/v2/root/upgrade_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"
golangci-lint
store/v2/root/upgrade_test.go
25-25: undefined: store
29-29: undefined: MigrateStoreTestSuite
38-38: undefined: storeKeys
50-50: undefined: New
58-58: undefined: storeKeys
74-74: undefined: storeKeys
90-90: undefined: New
Additional comments not posted (3)
store/v2/root/upgrade_test.go (3)
32-66: Review ofSetupTestmethod.The method correctly sets up the test environment, including logging and database connections. It also commits a changeset, which is a good practice for testing state changes.
Tools
golangci-lint
38-38: undefined: storeKeys
50-50: undefined: New
58-58: undefined: storeKeys
68-92: Review ofloadWithUpgradesmethod.The method correctly handles the loading of the store with upgrades, including handling renamed and added store keys.
Tools
golangci-lint
74-74: undefined: storeKeys
90-90: undefined: New
94-126: Review ofTestLoadVersionAndUpgrademethod.The method correctly tests the loading of the store version with upgrades, including handling new, renamed, and deleted store keys.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
store/v2/storage/storage_test_suite.go (1)
643-694: Consider adding more detailed comments to theTestUpgradablefunction to explain the purpose of each major step.This will improve the maintainability of the test code, especially for new developers or when revisiting this code after some time.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (10)
- store/v2/commitment/store.go (5 hunks)
- store/v2/database.go (4 hunks)
- store/v2/root/store.go (3 hunks)
- store/v2/storage/pebbledb/db.go (4 hunks)
- store/v2/storage/rocksdb/db.go (2 hunks)
- store/v2/storage/rocksdb/db_test.go (1 hunks)
- store/v2/storage/rocksdb/iterator.go (1 hunks)
- store/v2/storage/sqlite/db.go (2 hunks)
- store/v2/storage/storage_test_suite.go (1 hunks)
- store/v2/storage/store.go (2 hunks)
Files skipped from review due to trivial changes (1)
- store/v2/storage/rocksdb/db_test.go
Additional context used
Path-based instructions (9)
store/v2/storage/rocksdb/iterator.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/database.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/rocksdb/db.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/sqlite/db.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/pebbledb/db.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/storage/storage_test_suite.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (23)
store/v2/storage/rocksdb/iterator.go (1)
127-129: The implementation of theTimestampmethod looks correct and adheres to the expected behavior for accessing timestamp data from a RocksDB iterator.store/v2/database.go (1)
28-35: The addition of theUpgradableDatabaseinterface with methods for pruning and migrating store keys is well-defined and aligns with the objectives of enabling seamless store modifications.store/v2/storage/store.go (2)
21-21: The implementation of theUpgradableDatabaseinterface in theStorageStoreclass is correctly done.
141-162: The methodsPruneStoreKeyandMigrateStoreKeyare well-implemented, with appropriate checks for the database's capability to handle these operations. This ensures that the operations are only attempted on databases that support them.store/v2/storage/rocksdb/db.go (2)
31-32: The implementation of theUpgradableDatabaseinterface in theDatabaseclass is correctly done.
202-239: The methodsPruneStoreKeyandMigrateStoreKeyare well-implemented. ThePruneStoreKeymethod correctly does nothing, as pruning in RocksDB is handled by compaction. TheMigrateStoreKeymethod includes comprehensive logic for copying keys, which is essential for data migration.store/v2/storage/sqlite/db.go (3)
47-48: The implementation of theUpgradableDatabaseinterface is correctly declared.
246-262: ThePruneStoreKeymethod correctly implements the functionality to prune a specific store key. Ensure proper transaction handling and error management.
264-283: TheMigrateStoreKeymethod correctly implements the functionality to migrate data from one store key to another. Ensure transactional integrity and error handling are maintained throughout the operation.store/v2/storage/pebbledb/db.go (3)
35-36: The implementation of theUpgradableDatabaseinterface is correctly declared.
328-351: ThePruneStoreKeymethod correctly implements the functionality to prune a specific store key. Ensure proper batch handling and error management.
353-382: TheMigrateStoreKeymethod correctly implements the functionality to migrate data from one store key to another. Ensure batch operations are handled correctly and efficiently.store/v2/root/store.go (3)
224-224: TheLoadLatestVersionmethod correctly delegates toloadVersionwith no upgrades specified. This is appropriate for loading the latest version without modifications.
233-233: TheLoadVersionmethod correctly delegates toloadVersionwith no upgrades specified. This is consistent with the expected behavior when loading a specific version.
236-288: TheLoadVersionAndUpgrademethod correctly implements the functionality to load a specific version and apply upgrades. Ensure that the migration check and the handling of store key pruning are correctly implemented.store/v2/commitment/store.go (6)
9-9: Added import"sort"to support deterministic iteration over keys during upgrades.
28-28: Introduced a new constantbatchFlushThresholdset to 64KB, likely used to manage batch operations efficiently.
35-35: Implemented interfacesstore.Committer,snapshots.CommitSnapshotter,store.PausablePruner, andstore.KVStoreGetterto enhance the functionality ofCommitStore.
239-296: AddedmigrateKVStoremethod to migrate data from an old key to a new key. This method ensures data consistency during key migration by using batch operations and handling large data migrations efficiently withbatchFlushThreshold.
630-638: ImplementedGetKVStoreWithBatchto retrieve aKVStoreWithBatchinstance from themultiTreesmap if available. This method supports the new interfacestore.KVStoreGetter.
Line range hint
133-237: UpdatedLoadVersionmethod to handle upgrades and rollback scenarios. The method now supports loading a specific version with potential store upgrades and handles different cases like rollback, snapshot restore, and store key modifications (add, delete, rename).Verification successful
The
rgoutput shows extensive usage ofstoreKeyacross various files, including tests and core implementation files. TheLoadVersionmethod instore/v2/commitment/store.gohandles added, deleted, and renamed store keys. To ensure these modifications are correctly managed in other parts of the codebase, we need to verify the handling of renamed and deleted store keys in the core implementation files.Let's focus on verifying the handling of renamed and deleted store keys in the core implementation files, especially in the context of the
LoadVersionmethod.
The search results confirm that the handling of renamed and deleted store keys is implemented correctly in the core implementation files. Specifically, the
LoadVersionmethod instore/v2/commitment/store.goincludes logic to manage renamed and deleted store keys, and this handling is consistent across other parts of the codebase.
- The
RenamedFromfunction is used to check if a store key has been renamed and to migrate the data accordingly.- The
IsDeletedfunction is used to check if a store key has been deleted and to remove the corresponding tree.These functions are utilized in the
LoadVersionmethod and other relevant parts of the codebase, ensuring that store key modifications are handled correctly.
store/v2/commitment/store.gostore/types/store.gostore/rootmulti/store.gocore/store/upgrade.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all store keys modified by upgrades are handled correctly in other parts of the codebase. # Test: Search for usage of modified store keys. Expect: Proper handling of renamed and deleted keys. rg --type go $'storeKey'Length of output: 47311
Script:
#!/bin/bash # Description: Verify handling of renamed and deleted store keys in core implementation files. # Test: Search for handling of renamed store keys. rg --type go 'RenamedFrom' --context 5 # Test: Search for handling of deleted store keys. rg --type go 'IsDeleted' --context 5Length of output: 6730
store/v2/storage/storage_test_suite.go (2)
669-671: Verify the behavior ofPruneStoreKeymethod in different scenarios.This script will help ensure that
PruneStoreKeyis tested under various conditions, which is crucial for understanding its behavior in edge cases and ensuring robustness.
684-685: Check the error handling forMigrateStoreKey.This script will help confirm that error handling for
MigrateStoreKeyis comprehensive and covers various failure scenarios, which is essential for maintaining data integrity during migrations.Verification successful
The extracted implementations of the
MigrateStoreKeymethod across different files show that error handling is indeed present within the method itself. Each implementation checks for errors at various stages and returns appropriate error messages.However, the
rgcommand to extract the usage ofMigrateStoreKeyfailed due to a regex parse error. To ensure comprehensive verification, we need to correct this and re-run the script to check how errors are handled whenMigrateStoreKeyis called.
The corrected
rgcommand output shows the usage of theMigrateStoreKeymethod in various files, includingstore/v2/storage/storage_test_suite.go. The error handling within theMigrateStoreKeymethod itself appears to be robust, as it checks for errors at multiple stages and returns appropriate error messages.However, the test suite usage (
store/v2/storage/storage_test_suite.go) only checks for no errors usings.Require().NoError(err), which is a standard practice in tests to ensure the method does not return an error under expected conditions.Given the comprehensive error handling within the method implementations and the standard testing practices observed, the error handling for
MigrateStoreKeyappears to be adequately covered.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error handling for MigrateStoreKey is robust across different scenarios. # Test: Search for the method usage and related error handling tests. Expect: Comprehensive error handling tests. rg --type go $'MigrateStoreKey'Length of output: 937
Script:
#!/bin/bash # Extract the implementation of the MigrateStoreKey method in different files ast-grep --lang go --pattern $'func (db *Database) MigrateStoreKey(fromStoreKey, toStoreKey []byte) error { $$$ }' --json ast-grep --lang go --pattern $'func (ss *StorageStore) MigrateStoreKey(oldStoreKey, newStoreKey []byte) error { $$$ }' --json # Extract the usage of the MigrateStoreKey method to check for error handling rg --type go 'MigrateStoreKey(' --context 5Length of output: 7394
Script:
#!/bin/bash # Correct the regex pattern and re-run the search for the usage of MigrateStoreKey to check for error handling rg --type go 'MigrateStoreKey\(' --context 5Length of output: 4401
sequenceDiagram
participant S as Store
participant SS as StateStorage
participant SC as StateCommitment
alt SC is a UpgradeableStore
S->>SC: LoadVersionAndUpgrade
SC->>SC: Mount new store keys
SC->>SC: Prune removed store keys
end
SC->>S: LoadVersion Result
alt SS is a UpgradableDatabase
S->>SS: PruneStoreKeys
end
|
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (3)
store/v2/commitment/metadata.go (1)
113-126: Add logging for debugging purposes.Adding logging can help in debugging issues related to removed store keys.
+ log.Printf("Flushing removed store keys for version %d: %v", version, storeKeys)store/v2/commitment/store.go (1)
253-268: Enhance error messages in theGetProofmethod.Consider providing more detailed error messages to improve debugging.
- return nil, fmt.Errorf("store %s not found: %w", storeKey, err) + return nil, fmt.Errorf("store %s not found while retrieving proof: %w", storeKey, err)store/v2/root/store.go (1)
275-290: LGTM!The
loadVersionmethod correctly handles the presence of upgrades and applies them if necessary. The changes are clean and maintain the DRY principle.Consider adding detailed comments to explain how upgrades are handled within this method for better clarity.
+ // loadVersion loads the specified version and applies any store upgrades if provided. + // If upgrades are provided, it checks if the state commitment supports upgrades and applies them.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
simapp/v2/go.sumis excluded by!**/*.sumstore/v2/go.sumis excluded by!**/*.sum
Files selected for processing (9)
- simapp/v2/go.mod (2 hunks)
- store/v2/README.md (2 hunks)
- store/v2/commitment/iavl/tree_test.go (4 hunks)
- store/v2/commitment/metadata.go (3 hunks)
- store/v2/commitment/store.go (9 hunks)
- store/v2/commitment/store_test_suite.go (3 hunks)
- store/v2/db/wrapper.go (1 hunks)
- store/v2/go.mod (4 hunks)
- store/v2/root/store.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- store/v2/commitment/store_test_suite.go
- store/v2/go.mod
Additional context used
Path-based instructions (6)
store/v2/db/wrapper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"store/v2/commitment/iavl/tree_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.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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/root/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
LanguageTool
store/v2/README.md
[uncategorized] ~56-~56: This expression is ususally spelled with a hyphen
Context: ...nd SC layers. This means pruning can be implementation specific, such as being synchronous or asynchron...(SPECIFIC_HYPHEN)
Markdownlint
store/v2/README.md
63-63: null
Files should end with a single newline character(MD047, single-trailing-newline)
Additional comments not posted (20)
store/v2/db/wrapper.go (4)
22-23: LGTM! Integration withcorestorepackage.The return type change from
idb.Iteratortocorestore.Iteratoris consistent with the integration of thecorestorepackage.
27-28: LGTM! Integration withcorestorepackage.The return type change from
idb.Iteratortocorestore.Iteratoris consistent with the integration of thecorestorepackage.
32-33: LGTM! Integration withcorestorepackage.The return type change from
idb.Batchtocorestore.Batchis consistent with the integration of thecorestorepackage.
37-38: LGTM! Integration withcorestorepackage.The return type change from
idb.Batchtocorestore.Batchis consistent with the integration of thecorestorepackage.store/v2/README.md (4)
23-43: Well-documented upgrade process.The
LoadVersionAndUpgradeAPI section is clear and informative. The sequence diagram effectively illustrates the process.
45-47: Clear explanation of the pruning process.The section accurately describes how pruning marks store keys as pruned without immediate data removal.
55-57: Clear explanation of pruning responsibility.The section accurately describes that pruning is the responsibility of the underlying SS and SC layers.
Tools
LanguageTool
[uncategorized] ~56-~56: This expression is ususally spelled with a hyphen
Context: ...nd SC layers. This means pruning can be implementation specific, such as being synchronous or asynchron...(SPECIFIC_HYPHEN)
61-63: Clear test coverage expectations.The section outlines clear expectations for test coverage of logical components.
Tools
Markdownlint
63-63: null
Files should end with a single newline character(MD047, single-trailing-newline)
store/v2/commitment/iavl/tree_test.go (1)
48-49: Improved error handling in tests.The updates to error handling during version retrieval and other operations improve the robustness of the test suite.
Also applies to: 59-61, 68-70, 112-114
store/v2/commitment/metadata.go (1)
15-15: LGTM!The constant
removedStoreKeyPrefixis correctly defined.store/v2/commitment/store.go (6)
327-337: LGTM!The method
pruneRemovedStoreKeysis correctly implemented.
510-517: LGTM!The method
Closeis correctly implemented.
Line range hint
298-306: LGTM!The method
Getis correctly implemented.
306-325: LGTM!The method
Pruneis correctly implemented.
Line range hint
340-398: LGTM!The method
Snapshotis correctly implemented.
Line range hint
401-478: LGTM!The method
Restoreis correctly implemented.simapp/v2/go.mod (2)
96-96: Verify the version update forgithub.com/cosmos/iavl.Ensure that the update to
v1.3.0does not introduce breaking changes.
8-8: Verify the version update forcosmossdk.io/core.Ensure that the update to
v0.12.1-0.20240725072823-6a2d039e1212does not introduce breaking changes.store/v2/root/store.go (2)
231-231: LGTM!The
LoadLatestVersionmethod correctly delegates toloadVersionwith the new signature. The changes are clean and maintain the DRY principle.
240-240: LGTM!The
LoadVersionmethod correctly delegates toloadVersionwith the new signature. The changes are clean and maintain the DRY principle.
| func (m *MetadataStore) setLatestVersion(version uint64) error { | ||
| var buf bytes.Buffer | ||
| buf.Grow(encoding.EncodeUvarintSize(version)) | ||
| if err := encoding.EncodeUvarint(&buf, version); err != nil { |
There was a problem hiding this comment.
not really relevant to this PR, but why are we using varint encoding? Version is a 64 bit uint so isn't a static 8 byte little endian encoded int fine?
There was a problem hiding this comment.
afaik, it is introduced to optimize the data
store/v2/commitment/store.go
Outdated
| } | ||
|
|
||
| // NewCommitStore creates a new CommitStore instance. | ||
| func NewCommitStore(trees map[string]Tree, db corestore.KVStoreWithBatch, logger corelog.Logger) (*CommitStore, error) { | ||
| func NewCommitStore(trees map[string]Tree, db corestore.KVStoreWithBatch, mountTreeFn MountTreeFn, logger corelog.Logger) (*CommitStore, error) { |
There was a problem hiding this comment.
I'm not sure about this API change.. mountTreeFn MountTreeFn in this constructor feels off from a DevEx perspective. Can it be defined internally in CommitStore?
| if internal.IsMemoryStoreKey(key) { | ||
| trees[key] = mem.New() | ||
| return mem.New(), nil | ||
| } else { | ||
| switch storeOpts.SCType { |
There was a problem hiding this comment.
storeOpts.SCType appears to be the only value of importance bound within this closure when passed to NewCommitStore. If mountTreeFn is strictly required for that constructor I suggest passing the SC type instead since this iteration of store/v2 only supports one type of SC tree anyway.
There was a problem hiding this comment.
the main problem is that the Commitment is an abstraction layer, and has no idea of how to construct the tree.
There was a problem hiding this comment.
Perhaps we can rethink abstraction lines a bit?
Co-authored-by: Matt Kocubinski <mkocubinski@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
simapp/v2/go.sumis excluded by!**/*.sumstore/v2/go.sumis excluded by!**/*.sum
Files selected for processing (3)
- simapp/v2/go.mod (2 hunks)
- store/v2/README.md (2 hunks)
- store/v2/go.mod (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- simapp/v2/go.mod
Additional context used
Path-based instructions (1)
store/v2/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
store/v2/README.md
[uncategorized] ~56-~56: This expression is ususally spelled with a hyphen
Context: ...nd SC layers. This means pruning can be implementation specific, such as being synchronous or asynchron...(SPECIFIC_HYPHEN)
Markdownlint
store/v2/README.md
63-63: null
Files should end with a single newline character(MD047, single-trailing-newline)
Additional comments not posted (5)
store/v2/README.md (2)
23-27: LGTM! Verify the accuracy of the diagram.The description of the
LoadVersionAndUpgradeAPI is clear and concise. The diagram helps illustrate the process.Ensure the accuracy of the sequence diagram to match the actual implementation.
29-43: LGTM! Verify the accuracy of the diagram.The sequence diagram is a helpful visual aid for understanding the upgrade process.
Ensure the accuracy of the sequence diagram to match the actual implementation.
store/v2/go.mod (3)
12-12: LGTM! Verify the compatibility of the new version.The version of
github.com/cosmos/iavlhas been updated. Ensure it does not introduce breaking changes.Verify the compatibility of the new version with the existing codebase.
21-21: LGTM! Verify the compatibility of the new dependency.The
golang.org/x/expdependency has been added. Ensure it does not introduce compatibility issues.Verify the compatibility of the new dependency with the existing codebase.
35-35: LGTM! Verify the compatibility of the new version.The version of
github.com/emicklei/dothas been updated. Ensure it does not introduce breaking changes.Verify the compatibility of the new version with the existing codebase.
github-actions
bot
added
C:server/v2
|
reverted the bump iavl to |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (3)
runtime/v2/go.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sum
Files selected for processing (4)
- runtime/v2/go.mod (2 hunks)
- server/v2/cometbft/go.mod (2 hunks)
- server/v2/go.mod (2 hunks)
- store/v2/commitment/metadata.go (3 hunks)
Files skipped from review due to trivial changes (1)
- server/v2/cometbft/go.mod
Files skipped from review as they are similar to previous changes (1)
- store/v2/commitment/metadata.go
Additional comments not posted (5)
runtime/v2/go.mod (2)
18-18: Dependency Update Approved:cosmossdk.io/coreThe version update for
cosmossdk.io/corefromv0.12.1-0.20231114100755-569e3ff6a0d7tov0.12.1-0.20240725072823-6a2d039e1212is approved.
47-47: Dependency Update Approved:github.com/cosmos/iavlThe version update for
github.com/cosmos/iavlfromv1.2.1-0.20240725141113-7adc688cf179tov1.2.1-0.20240731145221-594b181f427eis approved.server/v2/go.mod (3)
18-18: Dependency Update Approved:cosmossdk.io/coreThe version update for
cosmossdk.io/corefromv0.12.1-0.20231114100755-569e3ff6a0d7tov0.12.1-0.20240725072823-6a2d039e1212is approved.
59-59: Dependency Update Approved:github.com/cosmos/iavlThe version update for
github.com/cosmos/iavlfromv1.2.1-0.20240725141113-7adc688cf179tov1.2.1-0.20240731145221-594b181f427eis approved.
62-62: Dependency Update Approved:github.com/emicklei/dotThe version update for
github.com/emicklei/dotfromv1.6.1tov1.6.2is approved.
|
|
||
| for _, storeKey := range storeKeys { | ||
| key := []byte(fmt.Sprintf("%s%s", encoding.BuildPrefixWithVersion(removedStoreKeyPrefix, version), storeKey)) | ||
| if err := batch.Set(key, []byte{}); err != nil { |
There was a problem hiding this comment.
I think if a StoreInfo (below)
cosmos-sdk/store/v2/proof/commit_info.go
Lines 24 to 28 in f50b207
was stored here instead of packing all metadata into the key we would gain two key advantages:
- ability to preload (mount) deleted trees at construction time (i.e. in factory)
- maintain support for multiple SC types
In particular this PR makes (2) impossible since mountTreeFn is singular.
Description
Closes: #19784
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.
I have...
Summary by CodeRabbit
New Features
LoadVersionAndUpgradeAPI for managing store keys.MetadataStorefor better management of versioned keys, particularly for removed keys.Bug Fixes
Documentation
root.Store.