fix(store/v2): don't delete future version when calling LoadVersion #22681
Conversation
julienrbrt
requested review from
cool-develope,
kocubinski and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request introduce several modifications across multiple files to enhance version loading functionality. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@julienrbrt 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
🧹 Outside diff range and nitpick comments (1)
store/v2/commitment/iavl/tree.go (1)
67-69: Consider adding version invariant checksSince version management is critical for state consistency, consider adding invariant checks to ensure:
- Versions are never accidentally deleted
- Version transitions maintain proper ordering
- Genesis export functionality works correctly across all scenarios
This would help catch any future regressions early.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
store/v2/commitment/iavl/tree.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
store/v2/commitment/iavl/tree.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
store/v2/commitment/iavl/tree.go (1)
68-69: LGTM! This change correctly addresses the version deletion issue.
The switch from LoadVersionForOverwriting to LoadVersion prevents the unintended deletion of future versions, which aligns with the PR objective of fixing state pruning issues during genesis export.
Let's verify this fixes the genesis export issue and doesn't cause regressions:
✅ Verification successful
Let me gather more information about the LoadVersion behavior by checking the test files.
Let me check for any test files that verify the version deletion behavior.
Let me check the implementation of LoadVersion in the IAVL tree to verify the behavior.
LGTM! The change correctly prevents unintended version deletion.
The evidence from the changelog and implementation confirms this is the right fix:
- The changelog shows
LoadVersionForOverwritingwas intentionally removed from the IAVL tree API - The new implementation simply calls
LoadVersionwithout any version deletion logic - Version deletion is now strictly controlled through
DeleteVersionsTofor proper pruning management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the LoadVersion behavior and its impact on version management
# Check for any other direct uses of LoadVersionForOverwriting that might need similar fixes
rg "LoadVersionForOverwriting"
# Look for any tests or documentation mentioning version deletion or genesis export
rg -A 5 "export.*genesis|genesis.*export"
# Check for related test files that verify version management
fd -e go test.*iavl
Length of output: 100147
Script:
#!/bin/bash
# Check test files for LoadVersion behavior and version management
rg -A 5 "LoadVersion.*test"
# Look for any test files that verify version deletion behavior
rg -A 5 "DeleteVersion.*test|delete.*version.*test"
# Check for any test files that verify genesis export with IAVL
rg -A 5 "genesis.*export.*iavl|iavl.*genesis.*export"
Length of output: 135589
Script:
#!/bin/bash
# Check for test files that verify version deletion behavior
rg -A 5 "LoadVersion.*test|DeleteVersion.*test" store/v2/commitment/iavl/
# Look for any test files that verify version management
fd -e go test.*iavl store/v2/commitment/iavl/
# Check for any test files that verify version deletion during LoadVersion
rg -A 5 "LoadVersion.*delete|delete.*LoadVersion" store/v2/commitment/iavl/
Length of output: 203
Script:
#!/bin/bash
# Check the implementation of LoadVersion in the IAVL tree
rg -A 10 "func.*LoadVersion" store/v2/commitment/iavl/
# Look for any references to version deletion in the IAVL implementation
rg -A 5 "DeleteVersion|delete.*version" store/v2/commitment/iavl/
# Check for any changelog entries related to LoadVersion changes
rg -A 5 "LoadVersion" store/CHANGELOG.md
Length of output: 1925
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
store/v2/commitment/mem/tree.go (1)
37-39: Add godoc comment for the new methodSince this is a public method implementing an interface, it should include a godoc comment explaining its purpose and behavior, especially given its role in fixing state pruning during genesis export.
+// LoadVersionForOverwriting loads a specific version of the tree for overwriting. +// This is a mock implementation that always returns nil. func (t *Tree) LoadVersionForOverwriting(version uint64) error { return nil }store/v2/commitment/tree.go (1)
28-28: Consider adding documentation for the new interface method.While the method name is descriptive, it would be beneficial to add a comment explaining:
- The purpose and behavior of overwriting
- How it differs from regular
LoadVersion- Any side effects or important considerations
+// LoadVersionForOverwriting loads a specific version of the tree and allows overwriting future versions. +// Unlike LoadVersion, this method does not delete versions after the specified version. LoadVersionForOverwriting(version uint64) errorstore/v2/database.go (1)
53-55: Enhance method documentation for clarity and safetyWhile the method addition aligns with the PR objectives to fix state pruning issues, the documentation should be expanded to:
- Clarify when this method should be used instead of
LoadVersion- Warn about the irreversible nature of deleting future versions
- Document any potential side effects or concurrent access considerations
Consider expanding the documentation like this:
-// LoadVersionForOverwriting loads the tree at the given version. -// Any versions greater than targetVersion will be deleted. +// LoadVersionForOverwriting loads the tree at the given version and permanently deletes +// any versions greater than targetVersion. This is an irreversible operation. +// Use this method with caution, typically only during genesis export or state migrations. +// For regular version loading, use LoadVersion instead.store/v2/store.go (1)
33-35: Enhance method documentation with usage guidelinesWhile the current documentation explains the basic functionality, consider adding:
- Use cases when this method should be preferred over
LoadVersion- Warning about potential data loss
- Whether this is safe to use during consensus
store/v2/commitment/store.go (2)
93-99: Add documentation for LoadVersionForOverwriting method.Please add documentation explaining:
- When this method should be used vs LoadVersion
- The implications of overwriting future versions
- Any potential risks or side effects
166-174: Consider adding error handling for edge cases.While the implementation is correct, consider adding error handling for:
- Invalid version numbers
- Version conflicts
- Tree state inconsistencies
This will make the code more robust and prevent potential issues during version loading.
if overrideAfter { + if targetVersion == 0 { + return fmt.Errorf("invalid target version: %d", targetVersion) + } if err := c.multiTrees[storeKey].LoadVersionForOverwriting(targetVersion); err != nil { + return fmt.Errorf("failed to load version %d for store %s: %w", targetVersion, storeKey, err) } } else { if err := c.multiTrees[storeKey].LoadVersion(targetVersion); err != nil { + return fmt.Errorf("failed to load version %d for store %s: %w", targetVersion, storeKey, err) } }store/v2/root/store_test.go (1)
286-326: Consider adding edge cases to TestLoadVersionForOverwriting.While the test comprehensively covers the basic functionality of LoadVersionForOverwriting, consider adding these edge cases:
- Loading version 0
- Loading the current version
- Attempting concurrent overwrites
Here's a suggested addition:
+ // attempt to load version 0 + err = s.rootStore.LoadVersionForOverwriting(0) + s.Require().Error(err) + + // attempt to load current version + err = s.rootStore.LoadVersionForOverwriting(5) + s.Require().NoError(err)store/v2/root/store.go (1)
265-271: Add documentation for exported methodLoadVersionForOverwritingThe new method
LoadVersionForOverwritingis exported but lacks a comment. According to the Uber Go Style Guide, all exported functions and methods should have a comment explaining what they do, starting with the function name.Suggested addition:
// LoadVersionForOverwriting loads a specified version and allows overwriting future versions. // It is used when loading a historical version for the purpose of overwriting subsequent data. func (s *Store) LoadVersionForOverwriting(version uint64) error { // Existing code... }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (9)
store/v2/commitment/iavl/tree.go(1 hunks)store/v2/commitment/mem/tree.go(1 hunks)store/v2/commitment/store.go(3 hunks)store/v2/commitment/tree.go(1 hunks)store/v2/database.go(1 hunks)store/v2/mock/db_mock.go(1 hunks)store/v2/root/store.go(4 hunks)store/v2/root/store_test.go(1 hunks)store/v2/store.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- store/v2/commitment/iavl/tree.go
🧰 Additional context used
📓 Path-based instructions (8)
store/v2/commitment/mem/tree.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/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/mock/db_mock.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/root/store_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/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (13)
store/v2/commitment/mem/tree.go (1)
37-39: Verify mock implementation's impact on tests
While returning nil is consistent with other mock methods in this file, we should ensure this implementation adequately supports testing the fix for issue #21633 (state pruning during genesis export).
✅ Verification successful
Let me generate additional scripts to verify the test coverage and usage of this mock implementation.
Mock implementation is appropriate for testing
The LoadVersionForOverwriting implementation in the mem tree mock is consistent with testing needs based on the following findings:
- The mock is used in the store/v2 package's test infrastructure, particularly in
store/v2/root/store_test.gowhich thoroughly tests version loading behavior - The implementation follows the same pattern as other methods in the mock tree (
Commit, etc.) which return nil values - The actual version loading logic is tested against the real IAVL implementation in
store/iavl/store_test.go - The mock's behavior supports testing the fix for issue server/v2: state pruning during export genesis #21633 as it allows the test code to verify version loading without requiring actual state persistence
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are tests using this mock implementation
# Look for test files that might be using this mock implementation
# Search for test files importing this package
rg -l "\"cosmossdk.io/store/v2/commitment/mem\"" --type go --type-add "go:*.go" | grep "_test.go"
# Search for tests related to version loading and genesis export
rg -A 5 "LoadVersionForOverwriting|export genesis" --type go
Length of output: 14901
Script:
#!/bin/bash
# Search for test files that use the mem tree implementation
rg -A 5 "commitment/mem" --type go | grep -A 5 "_test.go"
# Search for tests related to state pruning and genesis export
rg -A 5 "TestExportGenesis|TestPruning" --type go
# Look for tests using LoadVersionForOverwriting
rg -B 5 -A 10 "LoadVersionForOverwriting.*\(.*\)" --type go | grep -A 15 "_test.go"
Length of output: 9003
store/v2/commitment/tree.go (2)
28-28: Method placement and signature look good.
The new method:
- Is logically placed near other version-related methods
- Follows consistent naming patterns
- Has a clear, specific purpose aligned with the PR objectives
28-28: Verify implementations of the new interface method.
Adding a new method to the Tree interface is a breaking change that requires updates to all implementations.
✅ Verification successful
Let me search for implementations of the Tree interface to verify if they implement the new method.
All implementations have been updated with the new interface method.
The verification shows that all relevant implementations have the LoadVersionForOverwriting method:
store/v2/commitment/mem/tree.go: Treestore/v2/commitment/iavl/tree.go: IavlTreestore/v2/commitment/store.go: CommitStorestore/v2/mock/db_mock.go: MockStateCommitter (test mock)
The method is properly implemented across all concrete types that implement the Tree interface, so this change is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of the Tree interface to ensure they implement the new method
# Search for struct types that might implement Tree interface
ast-grep --pattern 'type $_ struct {
$$$
}'
# Search for the new method implementation
rg "func \(\w+ \*?\w+\) LoadVersionForOverwriting"
Length of output: 100242
Script:
#!/bin/bash
# First find the Tree interface definition to understand its full scope
rg "type Tree interface" -A 10
# Then find all implementations of LoadVersionForOverwriting
rg "func \(\w+ \*?\w+\) LoadVersionForOverwriting"
# Find potential struct types that may implement Tree
ast-grep --pattern 'type $_ struct {
$$$
Tree
$$$
}'
Length of output: 1574
store/v2/database.go (1)
53-56: Verify interface implementations for consistency
The addition of LoadVersionForOverwriting looks good, but let's ensure all implementations handle version deletion consistently.
✅ Verification successful
Let's gather more specific information about the implementations.
Implementation consistency verified across the codebase
The implementations of LoadVersionForOverwriting are consistent across the codebase:
store/v2/commitment/store.go: Properly delegates to individual trees and handles version loadingstore/v2/commitment/iavl/tree.go: Correctly implements version overwritingstore/v2/root/store.go: Implements telemetry and proper version loading- Version deletion is handled consistently through
deleteVersionsTo,deleteCommitInfo, anddeleteRemovedStoreKeysmethods
The implementation aligns with the genesis export functionality by ensuring proper version management and cleanup of higher versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of the Committer interface to verify consistent behavior
# Find all Go files that might implement the Committer interface
echo "Searching for potential implementations of LoadVersionForOverwriting..."
rg -l "LoadVersionForOverwriting" --type go
# Look for any existing LoadVersion implementations for comparison
echo "Searching for existing LoadVersion implementations for comparison..."
ast-grep --pattern 'func $_($_ *$_) LoadVersion(version uint64) error {
$$$
}'
Length of output: 752
Script:
#!/bin/bash
# Search for LoadVersionForOverwriting implementations
echo "Searching for LoadVersionForOverwriting implementations..."
rg -A 10 "LoadVersionForOverwriting.*\{" --type go
# Search for version deletion patterns in these implementations
echo -e "\nSearching for version deletion patterns..."
rg "DeleteVersion|delete.*version" --type go store/v2/
Length of output: 8232
store/v2/store.go (1)
33-36: Verify implementations handle version deletion correctly
The method's behavior of deleting future versions is a critical operation. Let's verify that all implementations handle this correctly.
✅ Verification successful
Let me gather more information about the implementation details.
Let me gather one more piece of information to verify the implementation details.
Based on the test implementation and the code analysis, I can now provide a final response:
LoadVersionForOverwriting implementation correctly handles version deletion
The test suite demonstrates that the implementation properly:
- Loads the specified version and deletes higher versions
- Allows new commits at versions higher than the loaded version
- Maintains data consistency after loading an older version
- Properly updates the latest version tracking
The comprehensive test cases in TestLoadVersionForOverwriting verify all critical aspects of this functionality, including:
- Error handling for non-existent versions
- Proper state loading at specified versions
- Correct handling of subsequent commits after loading older versions
- Data integrity verification across store operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of RootStore interface to ensure they properly implement LoadVersionForOverwriting
# Find all potential implementations
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) LoadVersionForOverwriting(version uint64) error {
$$$
}'
# Look for any existing tests
rg -l "TestLoadVersionForOverwriting"
# Check for potential version management related functions
rg "LoadVersion|DeleteVersion" --type go
Length of output: 12695
Script:
#!/bin/bash
# Check the implementation of LoadVersionForOverwriting in root store
ast-grep --pattern 'func ($_ $_) LoadVersionForOverwriting(version uint64) error {
$$$
}'
# Check the test implementation
cat store/v2/root/store_test.go
Length of output: 27491
Script:
#!/bin/bash
# Check the implementation of loadVersion method which is called by LoadVersionForOverwriting
ast-grep --pattern 'func ($_ $_) loadVersion(version uint64, $_, overwrite bool) error {
$$$
}'
Length of output: 99
Script:
#!/bin/bash
# Try a different pattern to find the loadVersion implementation
rg "func.*loadVersion.*overwrite.*bool.*error" -A 20
Length of output: 54
store/v2/mock/db_mock.go (1)
161-173: LGTM! The mock implementation follows the established patterns.
The new LoadVersionForOverwriting mock method and its recorder are correctly implemented following the same patterns as other methods in the file. The implementation:
- Properly uses the gomock helper methods
- Correctly handles error returns
- Maintains consistency with other mock method implementations
Note: Since this is an auto-generated file, any changes to the implementation should be made in the source file (./types.go) rather than directly in this file.
store/v2/commitment/store.go (3)
90-90: LGTM! This change prevents unintended deletion of future versions.
The modification aligns with the PR objective by explicitly setting overrideAfter to false, ensuring future versions are preserved.
145-145: LGTM! Appropriate use of overrideAfter during upgrades.
Setting overrideAfter to true is correct here as upgrades intentionally establish a new version timeline.
90-99: Verify fix for genesis export issue.
Let's verify that these changes resolve the state pruning issue during genesis export mentioned in #21633.
Also applies to: 145-174
✅ Verification successful
Let me gather more specific information about the LoadVersion implementation and its test coverage.
Let me search for more specific information about the genesis export issue and its relation to state pruning.
Based on the gathered information, particularly from issue #21633 and the implementation, I can now provide a final response:
LoadVersionForOverwriting implementation addresses genesis export pruning issue
The changes correctly fix the genesis export state pruning issue mentioned in #21633 by:
- Using
LoadVersionForOverwritingwhich properly handles state pruning during genesis export at specific heights - Ensuring that when exporting genesis at an earlier height, the state after that height is intentionally pruned, which is the expected behavior for genesis export
The implementation aligns with the issue description where:
- Exporting at current height works correctly
- Exporting at earlier heights prunes later state (intended behavior)
- The error mentioned in server/v2: state pruning during export genesis #21633 about loading higher versions after pruning is the correct behavior, preventing access to pruned state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for genesis export related tests and their interaction with LoadVersion
rg -A 5 "export.*genesis.*height|LoadVersion.*genesis"
# Look for any potential test cases that verify this behavior
ast-grep --pattern 'func Test$_(t *testing.T) {
$$$
LoadVersion($$$)
$$$
}'
Length of output: 11627
Script:
#!/bin/bash
# Search for LoadVersion and LoadVersionForOverwriting implementations and tests
rg -A 10 "func.*LoadVersion.*\("
# Look for test files related to commitment store
fd "store.*test\.go" | xargs rg -l "CommitStore"
# Search for any mentions of genesis export or state pruning in issues or docs
rg "genesis.*export.*state.*prun|state.*prun.*genesis"
Length of output: 19312
Script:
#!/bin/bash
# Search for the referenced issue #21633
gh issue view 21633 2>/dev/null || true
# Look for genesis export and state pruning related code
rg -A 5 "LoadVersionForOverwriting.*genesis|genesis.*export.*height"
# Check commit messages related to genesis export and state pruning
git log --grep="genesis.*export.*prun|state.*prun.*genesis" --pretty=format:"%h %s" 2>/dev/null || true
Length of output: 22521
store/v2/root/store_test.go (1)
259-284: LGTM! The test correctly verifies that future versions are protected.
The test additions properly verify that attempting to commit versions after loading an earlier version fails with an appropriate error, and the version remains unchanged. This aligns with the PR objective of preventing future version deletion.
Let's verify the error handling implementation:
✅ Verification successful
The test correctly verifies the protection of future versions
The implementation in store/v2/commitment/store.go shows that LoadVersion properly handles version management by:
- Rolling back metadata to the target version when loading an earlier version
- Deleting commit info for versions after the target version
- Setting the latest version to the target version
This aligns with the test's verification that:
- Attempting to commit versions after loading an earlier version fails
- The version stays at the loaded version (3) after failed commits
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling in LoadVersion implementation
# Expect: Error handling for future versions should be consistent across the codebase
# Search for LoadVersion implementations
ast-grep --pattern 'func $$$LoadVersion($$$) {
$$$
}'
Length of output: 57
Script:
#!/bin/bash
# Let's try a different approach to find LoadVersion implementations
# First, search for files containing LoadVersion
rg -l "LoadVersion"
# Then search for the actual implementation with context
rg "func.*LoadVersion" -A 10
Length of output: 19934
Script:
#!/bin/bash
# Let's check the implementation of loadVersion in store/v2/root/store.go
# to verify the error handling for future versions
rg "func.*loadVersion" store/v2/root/store.go -A 30
Length of output: 1195
Script:
#!/bin/bash
# Let's check the implementation of LoadVersion in the state commitment layer
# to verify how it handles future versions
rg "func.*LoadVersion" store/v2/commitment/store.go -A 30
Length of output: 1724
Script:
#!/bin/bash
# Let's check the private loadVersion implementation in the commitment store
# to see how it handles future versions
rg "func.*loadVersion.*storeKeys.*bool" store/v2/commitment/store.go -A 50
Length of output: 1525
store/v2/root/store.go (3)
253-253: Change maintains existing behavior in LoadLatestVersion
The call to s.loadVersion(lv, nil, false) with overrideAfter set to false ensures that future versions are not overwritten when loading the latest version. This maintains the existing behavior of the method.
262-263: Change maintains existing behavior in LoadVersion
Similarly, s.loadVersion(version, nil, false) is called with overrideAfter set to false, preserving the current functionality of LoadVersion without overwriting future versions.
290-290: Confirm intentional use of overrideAfter as true in LoadVersionAndUpgrade
The LoadVersionAndUpgrade method now calls s.loadVersion(version, upgrades, true), allowing overwriting future versions during an upgrade. Please confirm that this change is intentional and that overwriting future versions is desired in this context.
| func (s *Store) loadVersion(v uint64, upgrades *corestore.StoreUpgrades, overrideAfter bool) error { | ||
| s.logger.Debug("loading version", "version", v) | ||
|
|
||
| if upgrades == nil { | ||
| if err := s.stateCommitment.LoadVersion(v); err != nil { | ||
| return fmt.Errorf("failed to load SC version %d: %w", v, err) | ||
| if !overrideAfter { | ||
| if err := s.stateCommitment.LoadVersion(v); err != nil { | ||
| return fmt.Errorf("failed to load SC version %d: %w", v, err) | ||
| } | ||
| } else { | ||
| if err := s.stateCommitment.LoadVersionForOverwriting(v); err != nil { | ||
| return fmt.Errorf("failed to load SC version %d: %w", v, err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using boolean parameters to control function behavior
The loadVersion function now includes a boolean parameter overrideAfter that alters its behavior. According to the Uber Go Style Guide, it's recommended to avoid boolean parameters that change the function's behavior because they can make the code less readable and harder to maintain.
Suggestion:
Consider refactoring the function to eliminate the boolean parameter. One approach is to split loadVersion into two separate functions with clear and descriptive names:
loadVersion– retains the original behavior without overwriting future versions.loadVersionForOverwriting– handles loading versions with overwriting.
Refactored code:
func (s *Store) loadVersion(v uint64, upgrades *corestore.StoreUpgrades) error {
// Existing behavior without overwriting
// ...
}
func (s *Store) loadVersionForOverwriting(v uint64, upgrades *corestore.StoreUpgrades) error {
// Behavior that allows overwriting future versions
// ...
}This separation enhances clarity by making the function's purpose explicit and aligns with best practices.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for storeKey := range c.multiTrees { | ||
| storeKeys = append(storeKeys, storeKey) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
|
|
||
| func (s *Store) LoadVersionForOverwriting(version uint64) error { | ||
| if s.telemetry != nil { | ||
| now := time.Now() |
Check warning
Code scanning / CodeQL
Calling the system time Warning
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
store/v2/commitment/iavlv2/tree.go (1)
LoadVersionForOverwriting implementation needs attention
The method signature matches the interface requirement, but the current implementation in
iavlv2/tree.goonly contains a panic with "unimplemented". This needs to be properly implemented to fulfill the interface contract.
store/v2/commitment/iavlv2/tree.go:67-69: Replace the panic with proper implementation ofLoadVersionForOverwriting🔗 Analysis chain
Line range hint
13-19: Verify interface compliance with LoadVersionForOverwritingThe file correctly implements multiple interfaces with compile-time checks. Please ensure that the signature of
LoadVersionForOverwritingmatches the interface definition incommitment.Tree.Let's verify the interface definition:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the Tree interface definition ast-grep --pattern 'type Tree interface { $$$ LoadVersionForOverwriting($_) $_ $$$ }'Length of output: 1394
Script:
#!/bin/bash # Check the Tree implementation in iavlv2 ast-grep --pattern 'func (t *Tree) LoadVersionForOverwriting(version uint64) error { $$$ }'Length of output: 484
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
store/v2/commitment/iavlv2/tree.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
store/v2/commitment/iavlv2/tree.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
store/v2/commitment/iavlv2/tree.go (1)
67-69:
Implementation needed for LoadVersionForOverwriting
The current implementation with panic("unimplemented") doesn't address the core issue of preventing future version deletion during genesis export (issue #21633). Based on the PR objectives, this method should load a version without deleting future versions.
Consider implementing this method similar to LoadVersion but without the version deletion:
func (t *Tree) LoadVersionForOverwriting(version uint64) error {
- panic("unimplemented")
+ if err := isHighBitSet(version); err != nil {
+ return err
+ }
+ if version == 0 {
+ return nil
+ }
+ // TODO: Implement version loading without deleting future versions
+ // Consider using t.tree.LoadVersionForOverwriting when available in IAVL
+ return t.tree.LoadVersion(int64(version))
}Let's verify the IAVL dependency for available methods:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
store/v2/root/store_test.go (1)
286-326: LGTM! Consider adding more test cases.The test effectively verifies the core functionality of LoadVersionForOverwriting. However, consider adding the following test cases to improve coverage:
- Concurrent access scenarios to verify thread safety
- Error conditions during commit (e.g., simulated storage failures)
- Edge cases like version 0 or maximum uint64
Example test case structure:
func (s *RootStoreTestSuite) TestLoadVersionForOverwritingConcurrent() { // Setup initial versions similar to TestLoadVersionForOverwriting // Create a wait group for concurrent operations var wg sync.WaitGroup errChan := make(chan error, 2) // Concurrent LoadVersionForOverwriting operations wg.Add(2) go func() { defer wg.Done() if err := s.rootStore.LoadVersionForOverwriting(3); err != nil { errChan <- err } }() go func() { defer wg.Done() if err := s.rootStore.LoadVersionForOverwriting(2); err != nil { errChan <- err } }() wg.Wait() close(errChan) // Assert expected behavior for err := range errChan { s.Require().NoError(err) } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
store/v2/commitment/iavlv2/tree.go(1 hunks)store/v2/root/store_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- store/v2/commitment/iavlv2/tree.go
🧰 Additional context used
📓 Path-based instructions (1)
store/v2/root/store_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"
🔇 Additional comments (1)
store/v2/root/store_test.go (1)
259-284: LGTM! Test cases effectively verify version loading behavior.
The added test cases ensure that attempting to commit future versions after LoadVersion fails appropriately, and the store's state remains consistent with the loaded version. This aligns well with the PR's objective of preventing future version deletion.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
Closes: #21633
This solves #21633, but I would like to understand why it was deleting future version at the first place?
The IAVL v2 PR seems as well not having this behavior: https://github.com/cosmos/cosmos-sdk/pull/22424/files#diff-e81aa7b9bd3ed9670e229c6ee4c5bd438c7e6a58520d2bc9694e848659f9f99fR54-R63
In a follow-up PR, in release/v0.52.x we need to bump the pseudo version of store/v2
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
New Features
LoadVersionForOverwritingmethod across multiple components, allowing for more flexible version management and overwriting capabilities.CommitStoreandRootStoreinterfaces.Bug Fixes
These changes enhance the reliability and flexibility of the application without altering existing functionalities.