feat(store/v2): build the migration manager in the root store factory #22336
Conversation
|
Warning Rate limit exceeded@tac0turtle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 56 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 (1)
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily enhance the Changes
Assessment against linked 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
|
![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
🧹 Outside diff range and nitpick comments (2)
store/v2/root/factory.go (2)
Line range hint
144-153: Refactor to remove unnecessary else blockAccording to the Uber Go Style Guide, when an
ifblock ends with a return, the subsequentelsecan be removed to reduce nesting and improve readability.Apply this diff to simplify the code:
newTreeFn := func(key string, scType SCType) (commitment.Tree, error) { if internal.IsMemoryStoreKey(key) { return mem.New(), nil - } else { - switch scType { + } + switch scType { case SCTypeIavl: return iavl.NewIavlTree(db.NewPrefixDB(opts.SCRawDB, []byte(key)), opts.Logger, storeOpts.IavlConfig), nil case SCTypeIavlV2: return nil, errors.New("iavl v2 not supported") default: return nil, errors.New("unsupported commitment store type") } - } }
16-18: Organize import statements for better readabilityPer Go conventions and the Uber Go Style Guide, import statements should be grouped into standard library imports, external imports, and local imports, each separated by a blank line. This improves code clarity.
Consider restructuring the imports as follows:
import ( "errors" "fmt" "os" "cosmossdk.io/core/log" corestore "cosmossdk.io/core/store" "cosmossdk.io/store/v2" "cosmossdk.io/store/v2/commitment" "cosmossdk.io/store/v2/commitment/iavl" "cosmossdk.io/store/v2/commitment/mem" "cosmossdk.io/store/v2/db" "cosmossdk.io/store/v2/internal" "cosmossdk.io/store/v2/migration" "cosmossdk.io/store/v2/pruning" "cosmossdk.io/store/v2/snapshots" "cosmossdk.io/store/v2/storage" "cosmossdk.io/store/v2/storage/pebbledb" "cosmossdk.io/store/v2/storage/rocksdb" "cosmossdk.io/store/v2/storage/sqlite" )
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- store/v2/root/factory.go (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
store/v2/root/factory.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
store/v2/root/factory.go (2)
203-216: EnsurenewSCis properly initialized before useThe variable
newSCmay remainnilifscTypeequalsstoreOpts.SCType, but it is later used inmigration.NewManager. Verify thatmigration.NewManagercan handle anilnewSCwithout causing unexpected behavior.Run the following script to check how
newSCis used withinmigration.NewManager:✅ Verification successful
Let me generate new shell scripts to verify how
newSCis used in the migration manager.
Based on the results, I can now provide a final response as we have sufficient evidence.
newSCbeingnilis an expected and handled caseThe migration manager explicitly supports a
nilstate commitment store (sc) parameter as documented in its README:
sc(Commitment Store) can benil. In that case, the Manager will migrate only the state storage.This means the code is working as intended when
newSCremainsnilin cases wherescType == storeOpts.SCType. The migration manager is designed to handle this scenario gracefully.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that migration.NewManager can handle a nil newSC. # Navigate to the migration package directory cd path/to/cosmossdk.io/store/v2/migration # Search for the usage of newSC in NewManager rg -A 5 'func NewManager\(' # Check for dereferences of newSC rg 'newSC\.\w+' *.goLength of output: 11974
Script:
#!/bin/bash # Find the migration package and check NewManager implementation rg -A 10 "func NewManager" store/v2/migration/ # Find where newSC is used in the migration package rg "newSC" store/v2/migration/ # Check the factory.go file for more context around newSC usage rg -A 10 "migration\.NewManager" store/v2/root/factory.goLength of output: 1662
221-221: Confirm that passingmmandniltoNewis acceptableIn the return statement,
Newis called withmm(which may benil) andnilas the last parameter. Ensure that theNewfunction is designed to handlenilvalues for these parameters appropriately.Run the following script to examine the
Newfunction's handling ofnilarguments:✅ Verification successful
Let me generate new shell scripts to better verify the handling of
nilparameters in theNewfunction.
Based on the code inspection, I can now provide a definitive response about the handling of
nilparameters.Passing
nilvalues toNewis safe and properly handledThe
Newfunction instore/v2/root/store.goexplicitly handlesnilvalues:
- The
mmparameter (migration.Manager) is used to set bothmigrationManagerandisMigratingfields, whereisMigratingis explicitly set based on whethermmis nil:isMigrating: mm != nil- The last parameter
m(metrics.StoreMetrics) is simply assigned to thetelemetryfield, and there are no operations that would cause issues if it's nil🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the New function can handle nil values for mm and the last parameter. # Navigate to the root package directory cd path/to/cosmossdk.io/store/v2/root # Search for the New function definition rg -A 15 'func New\(' # Look for how mm and the last parameter are used within the function rg 'mm\.' *.go rg '.*nil.*' *.goLength of output: 11974
Script:
#!/bin/bash # First, let's find the exact file containing the New function in store/v2/root fd "factory.go|store.go" store/v2/root/ # Then, let's examine how mm and metrics parameters are used in the Store struct and New function rg -A 10 "type Store struct" store/v2/root/ rg -A 20 "func New\(" store/v2/root/ # Let's also check for any usage of mm or metrics in the package rg "mm\." store/v2/root/ rg "metrics\." store/v2/root/Length of output: 1963
store/v2/root/factory.go
Outdated
| // check if we need to migrate the store | ||
| isMigrating := false | ||
| scType := storeOpts.SCType | ||
| ssLatestVersion, err := ss.GetLatestVersion() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if ssLatestVersion != latestVersion { | ||
| isMigrating = true // need to migrate | ||
| if scType != SCTypeIavl { | ||
| scType = SCTypeIavl // only support iavl v1 for migration | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify isMigrating assignment
You can assign the result of the condition directly to isMigrating to make the code more concise.
Apply this diff to streamline the variable initialization:
-isMigrating := false
...
-if ssLatestVersion != latestVersion {
- isMigrating = true // need to migrate
+isMigrating := ssLatestVersion != latestVersion
if isMigrating {
if scType != SCTypeIavl {
scType = SCTypeIavl // only support iavl v1 for migration
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // check if we need to migrate the store | |
| isMigrating := false | |
| scType := storeOpts.SCType | |
| ssLatestVersion, err := ss.GetLatestVersion() | |
| if err != nil { | |
| return nil, err | |
| } | |
| if ssLatestVersion != latestVersion { | |
| isMigrating = true // need to migrate | |
| if scType != SCTypeIavl { | |
| scType = SCTypeIavl // only support iavl v1 for migration | |
| } | |
| } | |
| // check if we need to migrate the store | |
| scType := storeOpts.SCType | |
| ssLatestVersion, err := ss.GetLatestVersion() | |
| if err != nil { | |
| return nil, err | |
| } | |
| isMigrating := ssLatestVersion != latestVersion | |
| if isMigrating { | |
| if scType != SCTypeIavl { | |
| scType = SCTypeIavl // only support iavl v1 for migration | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
store/v2/root/factory_test.go (2)
32-39: Consider using subtests for better test organizationWhile the test coverage is good, consider using
t.Run()to organize different test scenarios into subtests for better clarity and maintainability.Example refactor:
- require.NoError(t, setLatestVersion(fop.SCRawDB, 1)) - fop.Options.SCType = SCTypeIavl - f, err = CreateRootStore(&fop) - require.NoError(t, err) - require.NotNil(t, f) - require.True(t, f.(*Store).isMigrating) + t.Run("migration enabled", func(t *testing.T) { + require.NoError(t, setLatestVersion(fop.SCRawDB, 1)) + fop.Options.SCType = SCTypeIavl + f, err = CreateRootStore(&fop) + require.NoError(t, err) + require.NotNil(t, f) + require.True(t, f.(*Store).isMigrating) + })
41-47: Consider defining "c/latest" as a constantThe function implementation is solid, but the magic string "c/latest" should be defined as a package-level constant for better maintainability and reusability.
Example:
+ const latestVersionKey = "c/latest" + func setLatestVersion(db corestore.KVStoreWithBatch, version uint64) error { var buf bytes.Buffer buf.Grow(encoding.EncodeUvarintSize(version)) if err := encoding.EncodeUvarint(&buf, version); err != nil { return err } - return db.Set([]byte("c/latest"), buf.Bytes()) + return db.Set([]byte(latestVersionKey), buf.Bytes()) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- store/v2/root/factory_test.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
store/v2/root/factory_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 (2)
store/v2/root/factory_test.go (2)
4-4: LGTM: Import additions are appropriateThe new imports for
bytesandencodingpackages are necessary for the version encoding functionality.Also applies to: 12-12
Line range hint
15-39: Verify edge cases in migration testingWhile the basic migration scenarios are covered, consider adding test cases for the following edge cases:
- Migration with version 0
- Migration with very large version numbers
- Failed migration scenarios
Let's check if there are any existing edge case tests in other test files:
e2e migration still is not working fully, created the issue #22360 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
store/v2/root/factory.go (2)
Line range hint
146-159: Consider enhancing error messages with more context.The function structure and error handling look good. Consider making the error messages more specific by including the actual SCType value in the "unsupported commitment store type" error.
- return nil, errors.New("unsupported commitment store type") + return nil, fmt.Errorf("unsupported commitment store type: %s", scType)
161-173: Document the IAVL v1 migration constraint.The code enforces
SCTypeIavlduring migration, but this significant constraint isn't documented. Consider adding a comment explaining why only IAVL v1 is supported during migration.if ssLatestVersion != latestVersion { isMigrating = true // need to migrate if scType != SCTypeIavl { + // IAVL v1 is currently the only supported format for migration due to + // compatibility requirements with the existing state commitment structure scType = SCTypeIavl // only support iavl v1 for migration } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- store/v2/commitment/metadata.go (1 hunks)
- store/v2/root/factory.go (5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/root/factory.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (4)
store/v2/commitment/metadata.go (1)
14-16: Verify migration path for key prefix changesThe change from "c/" to "s/" prefix is a breaking change that affects how metadata is stored and retrieved. This could impact existing databases.
Let's verify if this change is handled by the migration manager:
The comments still reference the old prefix format (e.g., "c/"). Please update them to reflect the new "s/" prefix for consistency.
- commitInfoKeyFmt = "s/%d" // c/<version> - removedStoreKeyPrefix = "s/removed/" // c/removed/<version>/<store-name> + commitInfoKeyFmt = "s/%d" // s/<version> + removedStoreKeyPrefix = "s/removed/" // s/removed/<version>/<store-name>store/v2/root/factory.go (3)
16-18: LGTM: Clean import additions and well-documented constant.The new imports and store prefix constant are well-structured and properly documented.
Also applies to: 38-39
Line range hint
177-189: LGTM: Consistent tree creation implementation.The tree creation logic is properly updated to handle the new scType parameter with consistent error handling.
223-223: LGTM: Clean final setup.The pruning manager setup and final store creation look good.
store/v2/root/factory.go
Outdated
| var mm *migration.Manager | ||
| if isMigrating { | ||
| snapshotDB, err := snapshots.NewStore(fmt.Sprintf("%s/data/snapshots/store.db", opts.RootDir)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| snapshotMgr := snapshots.NewManager(snapshotDB, snapshots.SnapshotOptions{}, sc, nil, nil, opts.Logger) | ||
| var newSC *commitment.CommitStore | ||
| if scType != storeOpts.SCType { | ||
| newTrees := make(map[string]commitment.Tree, len(opts.StoreKeys)) | ||
| for _, key := range opts.StoreKeys { | ||
| tree, err := newTreeFn(key, storeOpts.SCType) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| newTrees[key] = tree | ||
| } | ||
| newSC, err = commitment.NewCommitStore(newTrees, nil, opts.SCRawDB, opts.Logger) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| mm = migration.NewManager(opts.SCRawDB, snapshotMgr, ss, newSC, opts.Logger) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring migration setup for better maintainability.
The migration setup logic is complex and handles multiple concerns. Consider extracting it into a separate function for better maintainability and testing.
func setupMigrationManager(opts *FactoryOptions, ss *storage.StorageStore, sc *commitment.CommitStore, scType SCType) (*migration.Manager, error) {
snapshotDB, err := snapshots.NewStore(fmt.Sprintf("%s/data/snapshots/store.db", opts.RootDir))
if err != nil {
return nil, err
}
snapshotMgr := snapshots.NewManager(snapshotDB, snapshots.SnapshotOptions{}, sc, nil, nil, opts.Logger)
var newSC *commitment.CommitStore
if scType != opts.Options.SCType {
newSC, err = createNewCommitStore(opts, scType)
if err != nil {
snapshotDB.Close() // Cleanup on error
return nil, err
}
}
return migration.NewManager(opts.SCRawDB, snapshotMgr, ss, newSC, opts.Logger), nil
}| snapshotDB, err := snapshots.NewStore(fmt.Sprintf("%s/data/snapshots/store.db", opts.RootDir)) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Add cleanup for snapshot DB on error.
The snapshot DB creation should be cleaned up if subsequent operations fail to prevent orphaned files.
snapshotDB, err := snapshots.NewStore(fmt.Sprintf("%s/data/snapshots/store.db", opts.RootDir))
if err != nil {
+ // Clean up any partially created files
+ _ = os.RemoveAll(fmt.Sprintf("%s/data/snapshots", opts.RootDir))
return nil, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| snapshotDB, err := snapshots.NewStore(fmt.Sprintf("%s/data/snapshots/store.db", opts.RootDir)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| snapshotDB, err := snapshots.NewStore(fmt.Sprintf("%s/data/snapshots/store.db", opts.RootDir)) | |
| if err != nil { | |
| // Clean up any partially created files | |
| _ = os.RemoveAll(fmt.Sprintf("%s/data/snapshots", opts.RootDir)) | |
| return nil, err | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
store/v2/commitment/iavl/exporter.go (2)
42-43: Enhance type documentation for clarityThe documentation should provide more context about when and why an EmptyExporter is used, especially in relation to version migration scenarios.
Consider expanding the documentation:
-// EmptyExporter is a Exporter for an empty tree. +// EmptyExporter implements the commitment.Exporter interface for cases where no data +// should be exported, such as when the requested version is before the initial version +// during store migration. It always returns ErrorExportDone without exporting any items. type EmptyExporter struct{}
45-53: Implementation looks good with minor documentation suggestionsThe EmptyExporter methods are correctly implemented. Consider enhancing the method documentation:
-// Next returns ExportDone. +// Next implements the commitment.Exporter interface by always returning ErrorExportDone, +// indicating that no items are available for export. func (e *EmptyExporter) Next() (*snapshotstypes.SnapshotIAVLItem, error) { -// Close does nothing. +// Close implements the commitment.Exporter interface as a no-op since there are no +// resources to clean up in an empty exporter. func (e *EmptyExporter) Close() error {store/v2/root/factory_test.go (1)
32-39: Consider expanding test coverage for migration scenariosWhile the current test cases cover basic migration functionality, consider adding:
- Tests with different version numbers (0, max uint64)
- Negative test cases for invalid versions
- Verification of cleanup after migration
- Tests for concurrent access during migration
Example test case structure:
func TestFactory_MigrationScenarios(t *testing.T) { testCases := []struct { name string version uint64 expectedError bool checkState func(*testing.T, *Store) }{ {"version zero", 0, false, nil}, {"max version", math.MaxUint64, false, nil}, // Add more cases } // ... test implementation }store/v2/commitment/iavl/tree.go (2)
24-25: Enhance field documentation for clarity.The comment could be more descriptive about when and how this field is used during migration.
Consider updating the comment to:
-// it is only used for new store key during the migration process. +// initialVersion represents the starting version for new store keys during v1 to v2 migration. +// It is used to handle version-specific operations during the migration process.
63-73: Improve error handling and documentation.While the logic is correct, consider these improvements:
- Add more context to the comment explaining why we skip version loading for empty trees during migration
- Consider wrapping the error with additional context
if t.initialVersion > 0 { - // If the initial version is set and the tree is empty, - // we don't need to load the version. + // During migration with initialVersion set, we skip loading versions for empty trees + // as they represent new stores that start from the migration version latestVersion, err := t.tree.GetLatestVersion() if err != nil { - return err + return fmt.Errorf("failed to get latest version: %w", err) } if latestVersion == 0 { return nil } }store/v2/proof/proof.go (1)
233-258: Add documentation and improve error handlingThe function needs documentation explaining:
- Purpose of the conversion
- Expected input format
- Potential error cases
- Why empty byte slice is used in GetStoreProof
Add godoc comment:
+// ConvertV1CommitInfo converts a marshaled v1 CommitInfo to a v2 CommitInfo. +// It preserves version, timestamp, and store information while computing a new +// commit hash using GetStoreProof.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
store/v2/proof/v1/types/commit_info.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
- store/v2/commitment/iavl/exporter.go (1 hunks)
- store/v2/commitment/iavl/tree.go (4 hunks)
- store/v2/commitment/metadata.go (2 hunks)
- store/v2/go.mod (1 hunks)
- store/v2/proof/proof.go (2 hunks)
- store/v2/root/factory.go (5 hunks)
- store/v2/root/factory_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- store/v2/commitment/metadata.go
- store/v2/root/factory.go
🧰 Additional context used
📓 Path-based instructions (4)
store/v2/commitment/iavl/exporter.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/proof/proof.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/factory_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 (8)
store/v2/commitment/iavl/exporter.go (1)
42-53: Verify interface implementationLet's ensure EmptyExporter correctly implements the expected interface.
✅ Verification successful
Let me gather more information about the interface definition.
Let me search for the interface definition and any compile-time checks.
Based on the results, I can now provide a final response:
Interface implementation verified
EmptyExporter correctly implements the Exporter interface by providing:
Next() (*snapshotstypes.SnapshotIAVLItem, error)methodClose() errormethod (satisfying io.Closer)The implementation matches the interface requirements defined in
store/v2/commitment/tree.go.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that EmptyExporter implements the same interface as Exporter # Find the interface definition that both types should implement ast-grep --pattern 'type $interface interface { Next() ($_, error) Close() error }' # Verify both types implement the same methods rg -A 2 'type (Exporter|EmptyExporter) struct'Length of output: 470
Script:
#!/bin/bash # Find interface definitions and usages rg -A 5 'type.*interface' store/v2/commitment # Check for any compile-time interface checks rg 'var.*interface' # Look for the Next method signature in the codebase ast-grep --pattern 'func ($receiver $_) Next() ($return_type $_, error) { $$$ }'Length of output: 110554
Script:
#!/bin/bash # Find the Exporter interface definition rg -A 3 'type Exporter interface' store/v2/commitment # Look for any compile-time interface checks rg 'var.*Exporter'Length of output: 276
store/v2/root/factory_test.go (2)
4-4: LGTM: Import changes are appropriateThe new imports support the buffer operations and encoding functionality required for version management.
Also applies to: 9-9, 12-12
Line range hint
1-47: Verify migration manager test coverage aligns with PR objectivesWhile the tests cover basic migration functionality, consider adding integration tests to verify the end-to-end migration process mentioned in PR #22180, especially given the reported hanging issues in the PR comments.
Consider adding an integration test package specifically for store migration scenarios to catch issues like the hanging node problem early in the development cycle.
store/v2/go.mod (1)
24-24: LGTM! Consider checking for a newer protobuf version.The addition of protobuf as a direct dependency aligns with the migration manager implementation. However, let's verify if v1.35.1 is the latest stable version.
✅ Verification successful
Protobuf version v1.35.1 is up to date
The current version (v1.35.1) of google.golang.org/protobuf in go.mod matches the latest stable release.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there's a newer stable version of google.golang.org/protobuf # Note: Using gh api to query the latest release gh api \ -H "Accept: application/vnd.github+json" \ repos/protocolbuffers/protobuf-go/releases/latest \ --jq '.tag_name'Length of output: 122
store/v2/commitment/iavl/tree.go (2)
122-122: LGTM!The implementation correctly sets the initialVersion field after setting the tree's initial version.
142-144: Verify EmptyExporter implementation.The logic to return EmptyExporter for versions before initialVersion is correct. However, let's verify the EmptyExporter implementation.
✅ Verification successful
EmptyExporter implementation is correct and complete
The EmptyExporter correctly implements the Exporter interface by providing:
- A
Next()method that returnsErrorExportDone, indicating no items to export- The
io.Closerinterface is implicitly satisfied as no cleanup is needed for an empty exporterThe implementation aligns with its purpose of handling versions before initialVersion where no data needs to be exported.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify EmptyExporter implementation # Test: Check EmptyExporter methods and usage rg -A 5 "type EmptyExporter struct" store/v2/commitment/iavl/Length of output: 459
Script:
#!/bin/bash # Check for the interface that EmptyExporter implements rg -A 5 "type Exporter interface" store/v2/commitment/iavl/ # Check for any other methods of EmptyExporter rg "func \(\*EmptyExporter\)" store/v2/commitment/iavl/ # Check for ErrorExportDone definition rg "ErrorExportDone" store/v2/commitment/Length of output: 664
Script:
#!/bin/bash # Check for the Exporter interface definition in the commitment package rg -A 5 "type Exporter interface" store/v2/commitment/ # Check for any usage of EmptyExporter in tests rg "EmptyExporter" store/v2/commitment/iavl/.*_test.goLength of output: 448
store/v2/proof/proof.go (2)
10-10: LGTM: Import statement is well-structuredThe import statement follows Go conventions and uses appropriate aliasing.
254-256: 🛠️ Refactor suggestionImprove error handling and clarify GetStoreProof usage
The error handling could be improved, and the purpose of using an empty byte slice with GetStoreProof should be clarified.
Let's verify the usage of GetStoreProof with empty byte slice in other parts of the codebase:
Consider improving error handling:
- cInfo.CommitHash, _, err = cInfo.GetStoreProof([]byte{}) + var err error + if cInfo.CommitHash, _, err = cInfo.GetStoreProof([]byte{}); err != nil { + return nil, errors.Wrapf(err, "failed to compute commit hash") + }
| func setLatestVersion(db corestore.KVStoreWithBatch, version uint64) error { | ||
| var buf bytes.Buffer | ||
| buf.Grow(encoding.EncodeUvarintSize(version)) | ||
| if err := encoding.EncodeUvarint(&buf, version); err != nil { | ||
| return err | ||
| } | ||
| return db.Set([]byte("s/latest"), buf.Bytes()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing version management robustness
The implementation is correct but could be improved:
- Extract "s/latest" as a package-level constant
- Add version validation
- Consider adding a corresponding getLatestVersion function for symmetry
+const latestVersionKey = "s/latest"
func setLatestVersion(db corestore.KVStoreWithBatch, version uint64) error {
+ // Validate version if needed
var buf bytes.Buffer
buf.Grow(encoding.EncodeUvarintSize(version))
if err := encoding.EncodeUvarint(&buf, version); err != nil {
return err
}
- return db.Set([]byte("s/latest"), buf.Bytes())
+ return db.Set([]byte(latestVersionKey), buf.Bytes())
}
+func getLatestVersion(db corestore.KVStoreWithBatch) (uint64, error) {
+ data, err := db.Get([]byte(latestVersionKey))
+ if err != nil {
+ return 0, err
+ }
+ if len(data) == 0 {
+ return 0, nil
+ }
+ return encoding.DecodeUvarint(bytes.NewReader(data))
+}Committable suggestion was skipped due to low confidence.
| cInfo.StoreInfos[i] = StoreInfo{ | ||
| Name: []byte(v1StoreInfo.Name), | ||
| CommitID: CommitID{ | ||
| Version: uint64(v1StoreInfo.CommitId.Version), | ||
| Hash: v1StoreInfo.CommitId.Hash, | ||
| }, | ||
| Structure: "iavl", | ||
| } |
There was a problem hiding this comment.
Avoid hardcoding store structure type
Hardcoding "iavl" as the Structure value is risky as not all stores might use IAVL.
Consider:
- Getting the structure type from v1StoreInfo
- Adding validation for supported store types
- Adding a mapping of v1 to v2 store types
- Structure: "iavl",
+ Structure: determineStoreStructure(v1StoreInfo),Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
store/v2/root/store.go (1)
Line range hint
486-502: Enhance resource cleanup during migration completion.The current implementation could leak resources if errors occur during cleanup. Consider implementing a more robust cleanup sequence.
Consider implementing a dedicated cleanup method:
func (s *Store) completeMigration() error { // Ensure channels are closed only once once := sync.Once{} closeChannels := func() { once.Do(func() { close(s.chDone) close(s.chChangeset) }) } // Get new state commitment first newSC := s.migrationManager.GetStateCommitment() if newSC == nil { return fmt.Errorf("migration completed but no new state commitment available") } // Replace state commitment oldSC := s.stateCommitment s.stateCommitment = newSC // Cleanup old resources var errs []error if err := oldSC.Close(); err != nil { s.stateCommitment = oldSC // rollback errs = append(errs, fmt.Errorf("failed to close old SC: %w", err)) } if err := s.migrationManager.Close(); err != nil { errs = append(errs, fmt.Errorf("failed to close migration manager: %w", err)) } closeChannels() s.isMigrating = false if len(errs) > 0 { return errors.Join(errs...) } return nil }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- store/v2/migration/manager.go (1 hunks)
- store/v2/root/store.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (3)
store/v2/migration/manager.go (2)
268-268: LGTM! Clear and accurate comment update.The updated comment accurately reflects the current behavior of the Close method.
Line range hint
269-275: Verify database lifecycle management.The removal of database closing logic suggests that the database lifecycle is now managed elsewhere. Please ensure that the database is properly closed by the responsible component (likely the root store factory) to prevent resource leaks.
Let's verify the database lifecycle management:
✅ Verification successful
Based on the search results, I can verify that the database lifecycle is properly managed. The root store (
store/v2/root/store.go) is responsible for closing all database resources, including the state storage and state commitment databases:func (s *Store) Close() (err error) { err = errors.Join(err, s.stateStorage.Close()) err = errors.Join(err, s.stateCommitment.Close()) err = errors.Join(err, s.dbCloser.Close()) s.stateStorage = nil return err }Database lifecycle is properly managed by the root store.
The removal of database closing logic from the migration manager is correct since:
- The root store handles closing all database resources
- The migration manager only needs to notify the snapshots manager about migration completion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the database is properly closed elsewhere # Expected: Find where the database is closed after migration # Search for database closing patterns in the root store factory rg -A 5 "db\.Close\(\)" store/v2/root/ # Search for any cleanup or shutdown patterns that might handle database closure ast-grep --pattern 'func $_(db $_, $_) { $$$ db.Close() $$$ }'Length of output: 111
Script:
#!/bin/bash # Let's broaden our search to find database lifecycle management patterns # Search for Close method implementations and calls rg -g '*.go' -A 5 'func.*Close\(\).*error' # Look for database lifecycle management patterns rg -g '*.go' -A 5 'db\s*:?=.*Open' # Search for any shutdown or cleanup patterns rg -g '*.go' -A 5 'shutdown|cleanup|release|dispose' # Look specifically in store package for DB management rg -g '*.go' "db\." store/Length of output: 114461
store/v2/root/store.go (1)
Line range hint
486-502: Verify migration completion handling to prevent potential deadlocks.The migration completion logic in
writeSCcould lead to deadlocks or race conditions:
- Channel closure and state commitment replacement happen in the same critical section
- If
GetMigratedVersion()returns true butGetStateCommitment()returns nil, we still close channelsLet's verify the migration manager's behavior:
Consider separating migration completion logic from
writeSC:
- Move channel closure and state commitment replacement to a dedicated method
- Add explicit synchronization to prevent concurrent access during migration completion
- Add comprehensive logging around migration state transitions
| // close the old state commitment and replace it with the new one | ||
| if err := s.stateCommitment.Close(); err != nil { | ||
| return fmt.Errorf("failed to close the old SC store: %w", err) | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling during state commitment replacement.
The code correctly closes the old state commitment before replacing it, which prevents resource leaks. However, there's a potential issue where if closing fails, we still proceed with replacing the state commitment.
Consider wrapping the state commitment replacement in a transaction-like pattern to ensure atomicity:
- // close the old state commitment and replace it with the new one
- if err := s.stateCommitment.Close(); err != nil {
- return fmt.Errorf("failed to close the old SC store: %w", err)
- }
- s.stateCommitment = newStateCommitment
+ // close the old state commitment and replace it with the new one
+ oldSC := s.stateCommitment
+ s.stateCommitment = newStateCommitment
+ if err := oldSC.Close(); err != nil {
+ // restore the old state commitment if closing fails
+ s.stateCommitment = oldSC
+ return fmt.Errorf("failed to close the old SC store: %w", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // close the old state commitment and replace it with the new one | |
| if err := s.stateCommitment.Close(); err != nil { | |
| return fmt.Errorf("failed to close the old SC store: %w", err) | |
| } | |
| // close the old state commitment and replace it with the new one | |
| oldSC := s.stateCommitment | |
| s.stateCommitment = newStateCommitment | |
| if err := oldSC.Close(); err != nil { | |
| // restore the old state commitment if closing fails | |
| s.stateCommitment = oldSC | |
| return fmt.Errorf("failed to close the old SC store: %w", err) | |
| } |
| @@ -169,6 +183,9 @@ func (t *IavlTree) PausePruning(pause bool) { | |||
|
|
|||
| // Export exports the tree exporter at the given version. | |||
| func (t *IavlTree) Export(version uint64) (commitment.Exporter, error) { | |||
| if version < t.initialVersion { | |||
There was a problem hiding this comment.
Why not return an error here? Is there ever a case where calling this with this invalid version is valid?
Description
Closes: #22180
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
EmptyExporterfor handling empty trees in the commitment package.CommitInfostructures.Bug Fixes
Tests
Style