feat(store/v2): add support for iavl/v2 #22424
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes updates to multiple Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (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
|
github-actions
bot
added
C:Store
C:server/v2
|
@kocubinski 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: 14
🧹 Outside diff range and nitpick comments (10)
store/v2/go.mod (1)
Line range hint
3-3: Invalid Go version specified.The go.mod file specifies Go version 1.23 which hasn't been released yet. The latest stable version is Go 1.22.
-go 1.23 +go 1.22runtime/v2/go.mod (1)
Line range hint
3-3: Invalid Go version specifiedThe go.mod file specifies
go 1.23, but this version does not exist yet (latest stable is 1.22). This could cause build issues.-go 1.23 +go 1.22store/v2/storage/sqlite/batch.go (1)
58-63: Unnecessary transaction initiation inReset()methodStarting a new transaction with
b.db.Begin()in theReset()method may lead to redundant or nested transactions, which can cause unexpected behavior. Transaction management should be consistent and centralized.Consider removing the transaction initiation from the
Reset()method if it's not required, and ensure that transactions are properly managed in theWrite()method or other appropriate locations.Apply this diff to remove the unnecessary transaction initiation:
func (b *Batch) Reset() error { b.ops = nil b.ops = make([]batchOp, 0) b.size = 0 - err := b.db.Begin() - if err != nil { - return err - } return nil }store/v2/storage/sqlite/iterator.go (4)
88-91: Handle Iterator Stepping Errors AppropriatelyThe use of
itr.statement.Step()correctly handles advancing the iterator. However, consider whether additional context in the error message would be beneficial for debugging purposes.You might enhance the error message by including information about the current state or the query being executed.
135-137: Simplify the Logic inValid()MethodThe
Valid()method contains a redundant check that can be simplified for clarity.Apply this diff to simplify the method:
func (itr *iterator) Valid() bool { - if !itr.valid { - return itr.valid - } + if !itr.valid { + return false + } // if key is at the end or past it, consider it invalid if end := itr.end; end != nil { if bytes.Compare(end, itr.Key()) <= 0 { itr.valid = false - return itr.valid + return false } } return true }
151-157: Consider Error Handling Enhancements inNext()MethodIn the
Next()method, after callingitr.statement.Step(), you setitr.validtofalseif an error occurs or there are no more rows. Ensure that this behavior aligns with the expected iterator semantics, and consider whether additional error logging or handling is necessary.Would you like assistance in reviewing the error handling logic to ensure robustness?
Line range hint
169-173: Check for Potential Scan Errors inparseRow()In the
parseRow()method, the error handling correctly setsitr.erranditr.valid. Consider whether additional context could be added to the error message to aid in debugging.You might include information about the expected data types or the current row number if applicable.
store/v2/storage/sqlite/db.go (3)
Line range hint
117-152: Avoid using named return parameters unless necessaryThe use of named return parameters in
GetLatestVersionis generally discouraged unless they provide significant clarity or are required for deferred error handling.Consider removing named return parameters for clarity:
-func (db *Database) GetLatestVersion() (version uint64, err error) { +func (db *Database) GetLatestVersion() (uint64, error) {Adjust the function body to return values explicitly.
Line range hint
237-293: Ensure transaction rollback on error inPrunemethodIn the deferred function, if an error occurs during
db.storage.Commit(), the transaction may not be properly rolled back.Adjust the deferred function to handle commit errors and ensure rollback:
defer func() { if err != nil { + if rbErr := db.storage.Rollback(); rbErr != nil { + err = fmt.Errorf("%v; additionally failed to rollback transaction: %w", err, rbErr) + } } }()
389-417: Avoid duplicated code betweenGetLatestVersionandgetPruneHeightBoth functions perform similar actions to retrieve a single value from the database. Consider refactoring to eliminate duplication.
Extract a helper function to handle single-value queries.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (5)
runtime/v2/go.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumstore/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
runtime/v2/go.mod(2 hunks)server/v2/cometbft/go.mod(1 hunks)server/v2/go.mod(2 hunks)simapp/v2/go.mod(1 hunks)store/v2/commitment/iavlv2/tree.go(1 hunks)store/v2/go.mod(4 hunks)store/v2/storage/sqlite/batch.go(4 hunks)store/v2/storage/sqlite/db.go(13 hunks)store/v2/storage/sqlite/db_test.go(1 hunks)store/v2/storage/sqlite/iterator.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
store/v2/commitment/iavlv2/tree.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/storage/sqlite/batch.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/sqlite/db_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/storage/sqlite/iterator.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (17)
store/v2/go.mod (3)
Line range hint 30-49: Review indirect dependency changes.
The changes align with the PR objective of standardizing SQLite library usage:
- Added
bvinc/go-sqlite-literelated dependencies - Removed
mattn/go-sqlite3dependencies
10-17: Verify IAVL version compatibility.
The changes introduce both IAVL v1 and v2 as direct dependencies. While this aligns with the PR objective to add iavl/v2 support, we should ensure that both versions can coexist without conflicts.
✅ Verification successful
IAVL version coexistence verified as safe
The codebase shows a clean separation between IAVL v1 and v2 usage:
- IAVL v1 is used in existing store implementations (
store/iavl/,store/rootmulti/) - IAVL v2 is isolated to new v2 store implementation (
store/v2/commitment/iavlv2/) - No file imports both versions simultaneously except
go.modandgo.sum - The only shared file showing both versions (
store/v2/commitment/iavlv2/tree.go) only imports v2
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential version conflicts or issues in dependent modules
# Test 1: Check if other modules also use both IAVL versions
echo "Checking IAVL usage across modules..."
fd -t f "go.mod" --exec rg "github.com/cosmos/iavl"
# Test 2: Check for any replace directives for IAVL
echo "Checking IAVL replace directives..."
fd -t f "go.mod" --exec rg "replace.*github.com/cosmos/iavl"
Length of output: 1856
Script:
#!/bin/bash
# Let's check for actual usage patterns of IAVL v1 and v2 in the codebase
# Check for imports and usage of IAVL v1
echo "Checking IAVL v1 imports and usage..."
rg "\"github.com/cosmos/iavl\"" -A 2
# Check for imports and usage of IAVL v2
echo "Checking IAVL v2 imports and usage..."
rg "\"github.com/cosmos/iavl/v2\"" -A 2
# Check for any potential import conflicts in the same files
echo "Checking for files using both versions..."
rg -l "github.com/cosmos/iavl/v2" | xargs rg -l "github.com/cosmos/iavl"
Length of output: 2173
72-72: Verify local core module replacement.
The replace directive points to a relative path. This is common in monorepo setups but we should verify the path exists.
✅ Verification successful
Core module replacement verified successfully
The core module exists at the correct relative path ../../core from store/v2 and contains a valid Go module definition. The replace directive in store/v2/go.mod pointing to ../../core is correct and functional.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the core module
# Test: Check if the core module exists at the specified path
if [ -d "../../core" ] && [ -f "../../core/go.mod" ]; then
echo "Core module found and verified"
cat "../../core/go.mod" | head -n 3
else
echo "Error: Core module not found at ../../core"
exit 1
fi
Length of output: 127
Script:
#!/bin/bash
# Let's first check the actual path relative to the repository root
pwd
# Find the core directory and its go.mod file anywhere in the repository
echo "Searching for core/go.mod files:"
fd -t f "go.mod" -p "core/go.mod"
# List all go.mod files to understand the repository structure
echo -e "\nAll go.mod files in the repository:"
fd -t f "go.mod"
Length of output: 1369
Script:
#!/bin/bash
# Let's check the current working directory relative to store/v2
cd store/v2 2>/dev/null && pwd
# Check if core exists relative to store/v2
if [ -d "../../core" ]; then
echo "Core directory exists relative to store/v2"
echo "Contents of core/go.mod:"
cat "../../core/go.mod" | head -n 5
else
echo "Core directory does not exist at ../../core relative to store/v2"
fi
# Show the directory structure to understand the paths
echo -e "\nDirectory structure from store/v2:"
ls -la ../../core 2>/dev/null || echo "Cannot access ../../core"
Length of output: 2024
runtime/v2/go.mod (2)
37-37: LGTM: SQLite library migration
The addition of github.com/bvinc/go-sqlite-lite aligns with the PR objectives to standardize SQLite library usage across the codebase. The chosen library provides better explicit locking mechanisms.
52-52: Verify btree version update compatibility
The update of github.com/google/btree from v1.1.2 to v1.1.3 needs verification to ensure no breaking changes.
✅ Verification successful
Let me analyze the btree version update by checking the changelog and actual usage.
Let me check the actual btree usage in the memdb implementation to understand the impact.
Let me check the changelog between btree v1.1.2 and v1.1.3 to understand the changes.
Based on the analysis of the code and version update:
- The btree package is only used in the in-memory database implementation (MemDB) in store/v2/db/memdb.go
- The update is a minor version bump (v1.1.2 to v1.1.3) which indicates backward compatibility
- The MemDB implementation uses only the core btree functionality (New, Get, Has, ReplaceOrInsert, Delete, Ascend, Descend, etc.) which are stable APIs in the btree package
- The usage pattern is straightforward and doesn't rely on any advanced or potentially changed features
The update from btree v1.1.2 to v1.1.3 is safe and compatible
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes in btree v1.1.3
# Test: Look for any changes in method signatures or behavior
rg -A 5 "func.*btree"
# Test: Check if there are any existing usages that might be affected
rg "github.com/google/btree"
Length of output: 277616
Script:
# Check actual btree usage in code
rg -B 3 -A 3 "github.com/google/btree" . --type go
Length of output: 324
Script:
# Check memdb implementation using btree
cat ./store/v2/db/memdb.go
Length of output: 10805
Script:
# Get the changelog or commit history between v1.1.2 and v1.1.3
curl -s https://raw.githubusercontent.com/google/btree/master/CHANGELOG.md || \
(cd /tmp && git clone https://github.com/google/btree.git && cd btree && git log v1.1.2..v1.1.3 --oneline)
Length of output: 91
server/v2/go.mod (2)
50-50: LGTM: SQLite library migration.
The addition of github.com/bvinc/go-sqlite-lite and removal of github.com/mattn/go-sqlite3 aligns with the PR objectives to standardize the SQLite library implementation. The chosen library's explicit locking mechanisms should provide better control over concurrent database access.
67-67: Verify compatibility with updated btree version.
The indirect dependency update of github.com/google/btree from v1.1.2 to v1.1.3 should be verified for compatibility with the new SQLite implementation.
✅ Verification successful
Let me search for actual btree usage in the codebase to verify if there are any direct dependencies that could be affected by the version update.
No compatibility concerns with btree v1.1.3 update
The codebase uses btree in a way that is compatible with the v1.1.3 update. The changes between v1.1.2 and v1.1.3 were minor and did not introduce breaking changes. The main usage patterns found in the codebase:
- Basic BTree operations (Get, Set, Delete) in store/v2/db/memdb.go
- Generic BTreeG usage in server/v2/stf/branch and store/cachekv/internal
- Simple btree.Map usage in schema/testing and collections packages
All these patterns remain fully supported in v1.1.3 with no API changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any breaking changes in btree v1.1.3 that might affect the codebase
# Test: Search for btree usage patterns that might be affected by the version update
rg -A 5 'github.com/google/btree'
# Test: Look for any direct btree.New() calls or type assertions
ast-grep --pattern 'btree.New($$$)'
Length of output: 44907
Script:
#!/bin/bash
# Search for actual btree usage patterns in the code
rg -A 5 "btree\."
# Specifically look for btree type declarations and initializations
ast-grep --pattern 'btree.$_'
Length of output: 15613
store/v2/storage/sqlite/db_test.go (1)
Line range hint 1-217: Test coverage looks comprehensive.
The test suite effectively covers the key functionality:
- Basic storage operations
- Iterator behavior including reverse iteration
- Concurrent write operations
- Concurrent write and pruning scenarios
- Edge cases and error conditions
server/v2/cometbft/go.mod (1)
63-63: Verify SQLite library standardization across modules.
The addition of github.com/bvinc/go-sqlite-lite aligns with the PR objective of standardizing SQLite library usage. However, we should ensure this transition is complete across all affected modules.
Let's verify the SQLite library standardization:
✅ Verification successful
Based on the go.mod files analysis, I can see that the SQLite library transition is consistent across the codebase. The following modules have adopted github.com/bvinc/go-sqlite-lite:
- server/v2/cometbft/go.mod
- simapp/v2/go.mod
- store/v2/go.mod
- runtime/v2/go.mod
- server/v2/go.mod
Let's verify the actual SQLite usage in the code to ensure complete transition:
SQLite library transition is complete and consistent
The verification confirms that the SQLite library transition is complete and properly implemented:
- All SQLite-related code in
store/v2/storage/sqlite/consistently usesgithub.com/bvinc/go-sqlite-lite/sqlite3 - No references to the old
mattn/go-sqlite3library were found in the codebase - The only other SQL usage is in the postgres indexer which correctly uses the standard
database/sqlpackage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old SQLite library
# and confirm consistent usage of the new one across all Go modules
echo "Checking for any remaining mattn/go-sqlite3 references..."
rg "github.com/mattn/go-sqlite3"
echo "Verifying bvinc/go-sqlite-lite usage in all relevant modules..."
fd -e mod -x grep -l "github.com/bvinc/go-sqlite-lite"
Length of output: 453
Script:
#!/bin/bash
# Let's check the actual usage of SQLite libraries in the codebase
# to ensure the transition is complete
echo "Checking SQLite imports in Go files..."
rg "import.*sqlite" -A 5 -B 5
echo "Checking direct SQLite usage patterns..."
rg "sql\.(Open|OpenDB).*sqlite"
Length of output: 243
Script:
#!/bin/bash
# Let's check for SQLite-related code more broadly
echo "Checking for SQLite-related code patterns..."
rg -i "sqlite" -t go -A 5 -B 5
echo "Checking go.mod files content for SQLite-related dependencies..."
fd -e mod -x cat {} \; -x echo "---"
Length of output: 338463
Script:
#!/bin/bash
# Check for SQLite-related imports and usage patterns
echo "Checking SQLite imports and usage..."
rg -t go "github.com/bvinc/go-sqlite-lite|github.com/mattn/go-sqlite3" -A 2 -B 2
echo "Checking SQLite-related function calls..."
rg -t go "sql\.(Open|OpenDB)|sqlite3?\.(Open|Connect)"
Length of output: 1287
simapp/v2/go.mod (1)
82-82: LGTM: SQLite library standardization
The addition of github.com/bvinc/go-sqlite-lite aligns with the PR objective of standardizing SQLite library usage across the codebase.
Let's verify this standardization across other modules:
✅ Verification successful
SQLite library standardization verified across modules
The verification confirms successful standardization of SQLite library usage:
github.com/bvinc/go-sqlite-lite v0.6.1is consistently used across all relevant modules:- simapp/v2
- store/v2
- server/v2
- server/v2/cometbft
- runtime/v2
- No instances of
github.com/mattn/go-sqlite3were found in any go.mod files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SQLite library standardization across modules
# Expected: bvinc/go-sqlite-lite should be present and mattn/go-sqlite3 should be absent
echo "Checking for new SQLite library usage:"
rg "github.com/bvinc/go-sqlite-lite" --type go-mod
echo -e "\nChecking for old SQLite library usage (should be empty):"
rg "github.com/mattn/go-sqlite3" --type go-mod
Length of output: 375
Script:
#!/bin/bash
# Let's try a different approach to find go.mod files and check SQLite dependencies
echo "Searching for go.mod files with SQLite libraries:"
fd "go.mod" --type f --exec grep -l "github.com/bvinc/go-sqlite-lite\|github.com/mattn/go-sqlite3" {} \;
echo -e "\nChecking contents of found go.mod files:"
fd "go.mod" --type f --exec sh -c 'echo "\n=== {} ==="; grep -A1 -B1 "github.com/bvinc/go-sqlite-lite\|github.com/mattn/go-sqlite3" {} || true' \;
Length of output: 2897
store/v2/storage/sqlite/iterator.go (4)
10-11: Import Statement Correctly Added
The import statement for the new SQLite library github.com/bvinc/go-sqlite-lite/sqlite3 is correctly added and appropriately grouped.
18-18: Update Iterator Struct with New Statement Type
The statement field in the iterator struct is updated to use *sqlite3.Stmt, aligning with the new SQLite library's types.
77-80: Ensure Binding Parameters Match SQL Query Placeholders
When binding parameters using stmt.Bind(queryArgs...), ensure that the number of placeholders in the SQL query matches the number of elements in queryArgs. Mismatches could lead to runtime errors or unexpected behavior.
To confirm that the placeholders and arguments are correctly matched, you can review the SQL statement and the queryArgs slice.
35-35:
Avoid Casting from uint64 to int64 to Prevent Potential Overflow
Casting version from uint64 to int64 may lead to negative values or overflow issues when dealing with very large version numbers exceeding math.MaxInt64. This could cause unexpected behavior in the SQL query.
Consider the following solutions:
- Solution 1 (preferred): Modify the database schema to store version numbers as unsigned integers, if supported, to match the
uint64type. - Solution 2: Add a check to ensure that
versiondoes not exceedmath.MaxInt64before casting, and handle the error appropriately if it does.
To verify if any version numbers exceed math.MaxInt64 in the codebase, you can run the following script:
store/v2/storage/sqlite/db.go (3)
165-167: Check for errors when setting the latest version
While locking is correctly applied, consider handling possible errors from db.storage.Exec.
Ensure that the SetLatestVersion method handles all potential errors:
err := db.storage.Exec(reservedUpsertStmt, reservedStoreKey, keyLatestHeight, int64(version), 0, int64(version))
+if err != nil {
+ return fmt.Errorf("failed to set latest version: %w", err)
+}202-222: Handle possible SQL errors and edge cases in Get method
Ensure all errors from SQL operations are checked and handled appropriately, and consider the implications of the tombstone logic.
Review error handling for stmt.Bind, stmt.Step, and stmt.Scan to ensure robustness.
127-132:
Prevent error overwriting in deferred function
Assigning to the named return variable err within a deferred function can overwrite any existing errors, potentially causing loss of important error information.
Modify the deferred function to wrap the error without overwriting the original:
defer func(stmt *sqlite3.Stmt) {
if cErr := stmt.Close(); cErr != nil {
err = fmt.Errorf("failed to close GetLatestVersion statement: %w", cErr)
}
}(stmt)Alternatively, log the close error if it's less critical.
Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (13)
store/v2/go.mod (1)
Line range hint
3-3: Invalid Go version specifiedThe module specifies
go 1.23which is not yet released. The latest stable version of Go is 1.22.Please update to a released version:
-go 1.23 +go 1.22store/v2/storage/sqlite/db_test.go (1)
Line range hint
1-100: Consider adding specific tests for SQLite driver features.While the test coverage is generally good, consider adding tests for:
- SQLite locking mechanisms specific to the
bvinc/go-sqlite-litedriver- Verification of proper cleanup after test completion
- Connection handling under concurrent load
Would you like me to help generate additional test cases for these scenarios?
store/v2/commitment/iavlv2/tree.go (4)
99-100: Name the parameter in thePausePruningmethodThe parameter in the
PausePruningmethod is unnamed. According to the Uber Go Style Guide, all parameters should be named for clarity, even if they are not used. This improves code readability and consistency.Apply this diff to name the parameter:
-func (t *Tree) PausePruning(bool) {} +func (t *Tree) PausePruning(enable bool) {}
54-60: Address the TODO: FixLoadVersionbehavior in IAVL v2The
LoadVersionmethod contains a TODO comment indicating that there is an issue when loading version 0. This might cause unexpected behavior if version 0 is a valid version in certain contexts. Consider resolving this TODO by implementing the necessary fix or providing a clear explanation of the limitation.Would you like assistance in addressing this TODO or would you like me to open a GitHub issue to track this task?
23-30: Consider renaming the variablesqlto avoid confusionIn the
NewTreefunction, the variablesqlis used to store the SQLite database connection. Sincesqlis commonly associated with the standard library packagedatabase/sql, using it as a variable name may cause confusion. Consider renaming the variable todborsqliteDBfor better clarity.Apply this diff to rename the variable:
-func NewTree(treeOptions iavl.TreeOptions, dbOptions iavl.SqliteDbOptions, pool *iavl.NodePool) (*Tree, error) { - sql, err := iavl.NewSqliteDb(pool, dbOptions) +func NewTree(treeOptions iavl.TreeOptions, dbOptions iavl.SqliteDbOptions, pool *iavl.NodePool) (*Tree, error) { + db, err := iavl.NewSqliteDb(pool, dbOptions) if err != nil { return nil, err } - tree := iavl.NewTree(sql, pool, treeOptions) + tree := iavl.NewTree(db, pool, treeOptions) return &Tree{tree: tree}, nil }
32-104: Add documentation comments to exported methodsThe exported methods of the
Treestruct lack documentation comments. According to Go conventions and the Uber Go Style Guide, all exported functions and methods should have comments explaining their purpose and usage. This enhances code readability and helps other developers understand the functionality.Would you consider adding appropriate documentation comments to each exported method?
store/v2/storage/sqlite/iterator.go (4)
Line range hint
135-142: Simplify and correct theValidmethod logic.The
Validmethod should accurately reflect the iterator's state. Ensure that it doesn't returntruewhen the iterator has become invalid due to reaching the end.Consider refactoring the
Validmethod:if !itr.valid { return false } if end := itr.end; end != nil { if bytes.Compare(end, itr.Key()) <= 0 { itr.valid = false - return itr.valid + return false } } return trueThis makes the return values explicit and improves readability.
77-80: Ensure statement is properly closed on error during binding.The error handling when binding parameters should reliably close the statement to prevent resource leaks.
Consider checking the error from
stmt.Close():err = stmt.Bind(queryArgs...) if err != nil { - _ = stmt.Close() + closeErr := stmt.Close() + if closeErr != nil { + return nil, fmt.Errorf("failed to close statement after bind error: %w; original error: %v", closeErr, err) + } return nil, fmt.Errorf("failed to bind SQL iterator query arguments: %w", err) }This ensures that any errors during closure are also captured and reported.
10-11: Organize imports according to style guidelines.Imports should be grouped and separated into standard library imports, third-party imports, and local package imports.
Reorganize the imports for clarity:
import ( "bytes" "database/sql" "fmt" "slices" "strings" - "github.com/bvinc/go-sqlite-lite/sqlite3" corestore "cosmossdk.io/core/store" + + "github.com/bvinc/go-sqlite-lite/sqlite3" )This improves readability and maintains consistency with the Uber Golang style guide.
25-25: Function documentation is missing.Public functions should have a comment describing their purpose and usage.
Add a comment for the
newIteratorfunction:+ // newIterator creates a new iterator for the given parameters. func newIterator(db *Database, storeKey []byte, version uint64, start, end []byte, reverse bool) (*iterator, error) {This enhances the maintainability of the codebase.
store/v2/storage/sqlite/db.go (3)
52-54: Consider renaming 'storage' to 'conn' for clarity.In the
Databasestruct, the fieldstorage *sqlite3.Connmay be better namedconnordbConnto clearly indicate that it holds a database connection, improving code readability.
109-114: Avoid opening a new connection for each batch.In the
NewBatchmethod, a new database connection is opened for each batch. This can lead to resource exhaustion and decreased performance due to the overhead of establishing connections.Consider reusing the existing connection or implementing a connection pool to manage multiple connections efficiently.
238-239: Review the necessity ofwriteLockfor concurrency control.The use of
writeLock *sync.Mutexsuggests thatsqlite3.Connmay not be safe for concurrent writes. Confirm whether the SQLite driver handles concurrency internally or if external synchronization is required.Consider documenting the concurrency model or exploring alternative approaches if the mutex is unnecessary or could lead to performance bottlenecks.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (5)
runtime/v2/go.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumstore/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
runtime/v2/go.mod(2 hunks)server/v2/cometbft/go.mod(1 hunks)server/v2/go.mod(2 hunks)simapp/v2/go.mod(1 hunks)store/v2/commitment/iavlv2/tree.go(1 hunks)store/v2/go.mod(4 hunks)store/v2/storage/sqlite/batch.go(4 hunks)store/v2/storage/sqlite/db.go(13 hunks)store/v2/storage/sqlite/db_test.go(1 hunks)store/v2/storage/sqlite/iterator.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
store/v2/commitment/iavlv2/tree.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/storage/sqlite/batch.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/sqlite/db_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/storage/sqlite/iterator.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (12)
store/v2/go.mod (4)
30-30: Review indirect dependency additions
New indirect dependencies have been added:
- github.com/aybabtme/uniplot: Plotting library
- github.com/dustin/go-humanize: Human-readable numbers
- github.com/kocubinski/costor-api: Custom API
These seem to be pulled in by the IAVL v2 dependency. While they don't directly affect the codebase, it's good to be aware of the additional dependencies being introduced.
Also applies to: 38-38, 49-49
71-72: Verify local development setup
The replace directive for the core module is pointing to a relative path. This is typically used for local development and testing.
#!/bin/bash
# Verify the core module exists at the specified path
if [ -d "../../core" ]; then
echo "Core module found at ../../core"
cat ../../core/go.mod 2>/dev/null
else
echo "Warning: Core module not found at ../../core"
fi10-10: Verify SQLite library migration completeness
The PR objectives mention migrating from mattn/go-sqlite3 to bvinc/go-sqlite-lite. Let's verify that all SQLite-related code has been updated to use the new library.
✅ Verification successful
Let me try a different search approach to verify the SQLite library migration.
SQLite migration is complete and consistent
The codebase shows a complete migration to bvinc/go-sqlite-lite. All SQLite-related code consistently uses the new library through the github.com/bvinc/go-sqlite-lite/sqlite3 import. There are no references to the old mattn/go-sqlite3 library, and the implementation in store/v2/storage/sqlite/ package properly uses the new library's APIs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to the old SQLite library
rg "github.com/mattn/go-sqlite3"
# Check for the new SQLite library usage
rg "github.com/bvinc/go-sqlite-lite"
Length of output: 122
Script:
#!/bin/bash
# Search for SQLite-related imports and usage patterns
rg -i "sqlite" --type go
# Look for database connection/initialization code
ast-grep --pattern 'sql.Open($$$)'
Length of output: 4174
10-10: Verify IAVL version compatibility
The changes introduce both IAVL v1 and v2 as direct dependencies. While this might be intentional for migration purposes, we should ensure that:
- Both versions can coexist without conflicts
- There's a clear migration path from v1 to v2
Also applies to: 14-15
✅ Verification successful
Let me analyze the IAVL version usage in the codebase to verify compatibility and migration concerns.
Let me check one more aspect to understand the IAVL v2 implementation details.
IAVL v1 and v2 coexistence is properly managed
Based on the analysis of the codebase:
- IAVL v2 is cleanly isolated in
store/v2/commitment/iavlv2package with a proper abstraction layer - IAVL v1 (v1.3.0) is used as an indirect dependency in various x/ modules
- IAVL v2 is only used in store/v2, indicating a clear separation of concerns
- The implementation shows proper version handling and compatibility considerations
The coexistence is intentional and well-structured, with v2 being introduced alongside v1 in a way that prevents conflicts. The migration path is clear - v2 is being introduced in the new store/v2 package while maintaining backward compatibility with v1 in existing modules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential version conflicts or migration code
rg -A 5 "github.com/cosmos/iavl/v2"
rg -A 5 "github.com/cosmos/iavl v1"
Length of output: 43279
Script:
#!/bin/bash
# Check for IAVL v1 to v2 migration code and usage patterns
rg -A 5 "iavl.Migration"
rg -A 5 "MigrateStore"
rg -A 5 "store/v2/commitment/iavlv2"
Length of output: 14372
Script:
#!/bin/bash
# Check IAVL v2 implementation and usage
cat store/v2/commitment/iavlv2/tree.go
Length of output: 2370
runtime/v2/go.mod (2)
37-37: LGTM: SQLite library migration.
The addition of bvinc/go-sqlite-lite and removal of mattn/go-sqlite3 aligns with the PR objective to converge on a single SQLite library with explicit locking mechanisms.
52-52: Verify the impact of btree version update.
The minor version bump of google/btree from v1.1.2 to v1.1.3 might introduce changes that could affect performance or behavior.
✅ Verification successful
Let me check the actual usage of btree in the codebase to better understand the impact.
Let me check the changelog between v1.1.2 and v1.1.3 by looking at the commit history since the GitHub API didn't return the release notes.
The btree v1.1.3 update is safe to proceed with
The update from v1.1.2 to v1.1.3 contains a single bugfix for copy-on-write operations during item removal in cloned trees. After reviewing the codebase, the MemDB implementation uses basic btree operations (Get, Has, ReplaceOrInsert, Delete) and doesn't utilize tree cloning functionality, so this bugfix doesn't impact the current usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes or significant updates in google/btree
# Test: Look for changelog entries between v1.1.2 and v1.1.3
gh api repos/google/btree/releases | jq -r '.[] | select(.tag_name == "v1.1.3") | .body'
# Test: Search for btree usage in the codebase to assess impact
rg --type go 'github.com/google/btree'
Length of output: 177
Script:
#!/bin/bash
# Check how btree is used in memdb.go
cat store/v2/db/memdb.go
Length of output: 10803
Script:
#!/bin/bash
# Get the commit history between v1.1.2 and v1.1.3
gh api repos/google/btree/compare/v1.1.2...v1.1.3 | jq -r '.commits[] | "* " + .commit.message'
Length of output: 242
server/v2/go.mod (1)
67-67: LGTM: Minor version update for btree.
The update of github.com/google/btree from v1.1.2 to v1.1.3 is a minor version bump, which typically includes bug fixes and improvements while maintaining backward compatibility.
server/v2/cometbft/go.mod (1)
63-63: LGTM: SQLite library migration
The addition of github.com/bvinc/go-sqlite-lite v0.6.1 aligns with the PR objective to migrate from mattn/go-sqlite3. The version is consistent with other module updates.
Let's verify the SQLite library migration across all modules:
✅ Verification successful
SQLite library migration verified successfully
The verification confirms:
- Complete removal of
mattn/go-sqlite3with no remaining references - Consistent version
v0.6.1ofbvinc/go-sqlite-liteacross all modules:- simapp/v2
- server/v2/cometbft
- store/v2 (direct dependency)
- server/v2
- runtime/v2
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old SQLite library
# and verify consistent versions of the new library across all modules
echo "Checking for any remaining mattn/go-sqlite3 references..."
rg "github.com/mattn/go-sqlite3"
echo "Verifying bvinc/go-sqlite-lite versions across modules..."
fd -e mod | xargs rg "github.com/bvinc/go-sqlite-lite"
Length of output: 679
simapp/v2/go.mod (1)
82-82: SQLite library migration looks good.
The addition of github.com/bvinc/go-sqlite-lite v0.6.1 aligns with the PR objective to standardize SQLite usage across iavl/v2 and store/v2/storage/sqlite components.
Let's verify the SQLite library usage across the codebase:
✅ Verification successful
SQLite library migration is complete and consistent
The verification shows a clean migration from mattn/go-sqlite3 to bvinc/go-sqlite-lite:
- No references to the old library (
mattn/go-sqlite3) remain in the codebase - The new library is properly imported in all SQLite-related code under
store/v2/storage/sqlite/ - All v2 modules (
simapp,store,runtime,server) consistently use the same version (v0.6.1) - Implementation in
store/v2/storage/sqlite/correctly uses the new library's explicit locking mechanisms
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SQLite library usage and potential conflicts
# Test 1: Check for any remaining references to the old SQLite library
rg "github.com/mattn/go-sqlite3"
# Test 2: Find all files using the new SQLite library to ensure proper migration
rg "github.com/bvinc/go-sqlite-lite"
# Test 3: Look for SQLite-related code that might need updates
rg -g '*.go' -i "sqlite"
Length of output: 5870
store/v2/storage/sqlite/batch.go (1)
79-81: Proper use of mutex for thread safety in Write method
Good job on locking the mutex at the beginning of the Write method and deferring the unlock. This ensures thread safety during write operations and aligns with concurrency best practices.
store/v2/commitment/iavlv2/tree.go (1)
54-60:
Validate version numbers to prevent potential overflows
In methods like LoadVersion, GetProof, Get, and Prune, version numbers are cast from uint64 to int64. This could lead to incorrect behavior or panic if the version number exceeds the maximum value of int64. Consider adding validation to ensure that the provided version does not exceed math.MaxInt64 before casting.
Run the following script to check for version numbers exceeding int64 limits:
Also applies to: 72-75, 76-81, 95-97
store/v2/storage/sqlite/db.go (1)
356-371:
Handle errors properly when iterating over result set in PrintRowsDebug.
Currently, if an error occurs during stmt.Step(), the loop continues without handling it, which may lead to unexpected behavior.
Apply this diff to handle errors correctly:
for {
hasRow, err := stmt.Step()
if err != nil {
panic(fmt.Errorf("failed to step through SQL rows: %w", err))
}
if !hasRow {
break
}
// Existing code to scan and process the row
}Ensure that any error during Step() halts the iteration and reports the issue.
Likely invalid or redundant comment.
server/v2/go.mod
Outdated
| @@ -47,6 +47,7 @@ require ( | |||
| github.com/DataDog/zstd v1.5.5 // indirect | |||
| github.com/Microsoft/go-winio v0.6.1 // indirect | |||
| github.com/beorn7/perks v1.0.1 // indirect | |||
| github.com/bvinc/go-sqlite-lite v0.6.1 // indirect | |||
There was a problem hiding this comment.
💡 Codebase verification
Consider using a more actively maintained SQLite library
The current choice of github.com/bvinc/go-sqlite-lite v0.6.1 raises several concerns:
- The library hasn't been updated since May 2019 (last commit: 2019-05-02)
- There are unresolved module-related issues (Issue Fee as type Coins instead of Coin #25)
- The SQLite version is significantly outdated (Issue CLI Updates, Coin fix #26)
- No response to issues or maintenance activity for over 4 years
Consider evaluating alternative SQLite libraries with explicit locking mechanisms that are actively maintained, such as:
modernc.org/sqlitegithub.com/glebarez/go-sqlite
🔗 Analysis chain
Verify the SQLite library migration.
The addition of github.com/bvinc/go-sqlite-lite v0.6.1 aligns with the PR objective to migrate from mattn/go-sqlite3. However, let's verify if v0.6.1 is the latest stable version and if there are any known issues.
Let me gather more information about the issues and the repository activity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the latest version of go-sqlite-lite and any potential issues
# Expected: Latest version information and any relevant issues
# Get the latest release version
echo "Latest release version:"
gh release list --repo bvinc/go-sqlite-lite --limit 1
# Check for open issues related to v0.6.1
echo -e "\nRelevant open issues:"
gh issue list --repo bvinc/go-sqlite-lite --search "v0.6.1 in:body,title,comments"
Length of output: 355
Script:
#!/bin/bash
# Check the content of the open issues
echo "Issue #25 details:"
gh issue view 25 --repo bvinc/go-sqlite-lite
echo -e "\nIssue #26 details:"
gh issue view 26 --repo bvinc/go-sqlite-lite
# Check commit history to understand maintenance status
echo -e "\nRecent commits:"
gh api repos/bvinc/go-sqlite-lite/commits?per_page=5 | jq -r '.[] | "[\(.commit.committer.date)] \(.commit.message)"'
# Check if v0.6.1 is tagged
echo -e "\nTag information:"
gh api repos/bvinc/go-sqlite-lite/git/refs/tags/v0.6.1 || echo "Tag v0.6.1 not found"
Length of output: 2015
There was a problem hiding this comment.
It's a fair point, but I personally favor how bvinc/go-sqlite-lite is a very minimal wrapper around the original libc API.
There was a problem hiding this comment.
@kocubinski, understood. Thank you for explaining your preference for bvinc/go-sqlite-lite due to its minimal wrapper around the original libc API. I'll keep this in mind for future reviews.
✏️ Learnings added
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: server/v2/go.mod:50-50
Timestamp: 2024-11-04T23:39:12.128Z
Learning: The team prefers using `github.com/bvinc/go-sqlite-lite` because it is a very minimal wrapper around the original libc API.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
modernc.org/sqlite could be a good fit since it's a pure go implementation (won't conflict with iavl/v2) and maybe even more performant than the C one.
There was a problem hiding this comment.
@kocubinski, that's a great point. Modernc.org/sqlite, being a pure Go implementation, could indeed avoid conflicts with iavl/v2 and potentially offer better performance.
From the information gathered:
- Latest version: v1.33.1
- Active maintenance: The library has recent updates and ongoing issue discussions.
- Open issues: There are some open issues to consider, such as performance optimizations.
It seems like a promising alternative worth evaluating. Let me know if you need any assistance with this.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the latest version of modernc.org/sqlite
echo "Latest version:"
curl -s https://proxy.golang.org/modernc.org/sqlite/@latest | jq '.Version'
# Fetch recent commits
echo -e "\nRecent commits:"
curl -s 'https://gitlab.com/api/v4/projects/cznic/sqlite/repository/commits' | jq -r '.[].title' | head -5
# Fetch open issues
echo -e "\nOpen issues:"
curl -s 'https://gitlab.com/api/v4/projects/cznic%2Fsqlite/issues?state=opened' | jq -r '.[] | "\(.title) - \(.web_url)"'
Length of output: 2119
✏️ Learnings added
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: server/v2/go.mod:50-50
Timestamp: 2024-11-04T23:48:30.715Z
Learning: The team is considering using modernc.org/sqlite because it's a pure Go implementation, avoids conflicts with iavl/v2, and may offer better performance than the C-based implementations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
store/v2/storage/sqlite/db.go
Outdated
| return fmt.Errorf("failed to create SQL transaction: %w", err) | ||
| } | ||
| defer func() { | ||
| if err != nil { | ||
| err = tx.Rollback() | ||
| err = db.storage.Rollback() | ||
| } |
There was a problem hiding this comment.
Avoid overwriting errors during transaction rollback in PruneStoreKeys.
Similar to Prune, the deferred function in PruneStoreKeys may overwrite the original err, potentially hiding important error information.
Apply this diff:
defer func() {
if err != nil {
- err = db.storage.Rollback()
+ if rollbackErr := db.storage.Rollback(); rollbackErr != nil {
+ err = fmt.Errorf("%v; additionally failed to rollback transaction: %w", err, rollbackErr)
+ }
}
}()📝 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.
| return fmt.Errorf("failed to create SQL transaction: %w", err) | |
| } | |
| defer func() { | |
| if err != nil { | |
| err = tx.Rollback() | |
| err = db.storage.Rollback() | |
| } | |
| return fmt.Errorf("failed to create SQL transaction: %w", err) | |
| } | |
| defer func() { | |
| if err != nil { | |
| if rollbackErr := db.storage.Rollback(); rollbackErr != nil { | |
| err = fmt.Errorf("%v; additionally failed to rollback transaction: %w", err, rollbackErr) | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (4)
store/v2/storage/sqlite/batch.go (1)
35-38: Improve version overflow check messageThe check for version overflow could be more explicit about why the highest bit can't be set.
Apply this diff:
- if version&(1<<63) != 0 { - return nil, fmt.Errorf("%d too large; uint64 with the highest bit set are not supported", version) + if version > math.MaxInt64 { + return nil, fmt.Errorf("version %d exceeds maximum supported value of %d", version, math.MaxInt64)store/v2/storage/sqlite/db.go (2)
Line range hint
342-366: Replace panic with proper error handling in PrintRowsDebugUsing panic for error handling is not recommended in production code. This method should return an error instead.
Refactor the method to return an error:
-func (db *Database) PrintRowsDebug() { +func (db *Database) PrintRowsDebug() error { stmt, err := db.storage.Prepare("SELECT store_key, key, value, version, tombstone FROM state_storage") if err != nil { - panic(fmt.Errorf("failed to prepare SQL statement: %w", err)) + return fmt.Errorf("failed to prepare SQL statement: %w", err) } defer stmt.Close() err = stmt.Exec() if err != nil { - panic(fmt.Errorf("failed to execute SQL query: %w", err)) + return fmt.Errorf("failed to execute SQL query: %w", err) }
356-379: Use strings.Builder more efficientlyThe current implementation creates the StringBuilder after the query execution. For better memory efficiency, it should be pre-allocated with an estimated size.
Consider pre-allocating the StringBuilder:
var ( - sb strings.Builder + sb = strings.Builder{} ) +// Pre-allocate the builder with an estimated size +sb.Grow(1024) // Adjust size based on expected outputstore/v2/commitment/iavlv2/tree.go (1)
24-29: Rename variablesqlto avoid confusion with standard library packagesUsing
sqlas a variable name may cause confusion with the standard library packagedatabase/sql. It's advisable to avoid naming variables after common package names to improve code clarity.Consider renaming the variable to
dborsqliteDB.Apply this diff to rename the variable:
func NewTree(treeOptions iavl.TreeOptions, dbOptions iavl.SqliteDbOptions, pool *iavl.NodePool) (*Tree, error) { - sql, err := iavl.NewSqliteDb(pool, dbOptions) + sqliteDB, err := iavl.NewSqliteDb(pool, dbOptions) if err != nil { return nil, err } - tree := iavl.NewTree(sql, pool, treeOptions) + tree := iavl.NewTree(sqliteDB, pool, treeOptions) return &Tree{tree: tree}, nil }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
store/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
store/v2/commitment/iavlv2/tree.go(1 hunks)store/v2/go.mod(4 hunks)store/v2/storage/sqlite/batch.go(4 hunks)store/v2/storage/sqlite/db.go(13 hunks)store/v2/storage/sqlite/db_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- store/v2/go.mod
- store/v2/storage/sqlite/db_test.go
🧰 Additional context used
📓 Path-based instructions (3)
store/v2/commitment/iavlv2/tree.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/storage/sqlite/batch.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.
📓 Learnings (1)
store/v2/storage/sqlite/batch.go (1)
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: store/v2/storage/sqlite/batch.go:82-84
Timestamp: 2024-11-04T22:46:19.502Z
Learning: In `store/v2/storage/sqlite/batch.go`, the `lock` in the `Batch` struct is used to synchronize concurrent writers to the SQLite database, and the `Batch` methods (`Set()`, `Delete()`, `Reset()`) are designed for single-threaded access.
🔇 Additional comments (3)
store/v2/storage/sqlite/batch.go (2)
28-32:
Change version field type to uint64
The version field is stored as int64 but the NewBatch function accepts a uint64. This type conversion could lead to overflow issues for large version numbers.
Apply this diff:
type Batch struct {
db *sqlite3.Conn
lock *sync.Mutex
ops []batchOp
size int
- version int64
+ version uint64
}⛔ Skipped due to learnings
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: store/v2/storage/sqlite/batch.go:82-84
Timestamp: 2024-11-04T22:46:19.502Z
Learning: In `store/v2/storage/sqlite/batch.go`, the `lock` in the `Batch` struct is used to synchronize concurrent writers to the SQLite database, and the `Batch` methods (`Set()`, `Delete()`, `Reset()`) are designed for single-threaded access.
109-113:
Remove connection close from Write method
The database connection should be managed by the owner of the Batch instance, not closed in the Write method. This could prevent reuse of the connection.
Apply this diff:
if err := b.db.Commit(); err != nil {
return fmt.Errorf("failed to write SQL transaction: %w", err)
}
- return b.db.Close()
+ return nilLikely invalid or redundant comment.
store/v2/storage/sqlite/db.go (1)
122-127:
Improve error handling in deferred function
The current implementation overwrites any existing error with the close error, potentially hiding the original error.
Preserve both errors:
defer func(stmt *sqlite3.Stmt) {
cErr := stmt.Close()
if cErr != nil {
- err = fmt.Errorf("failed to close GetLatestVersion statement: %w", cErr)
+ if err != nil {
+ err = fmt.Errorf("original error: %v; failed to close statement: %w", err, cErr)
+ } else {
+ err = fmt.Errorf("failed to close statement: %w", cErr)
+ }
}
}(stmt)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
store/v2/storage/sqlite/batch.go (1)
36-45: Improve version overflow check documentationWhile the overflow check is good, its purpose should be documented. Also, consider using constants for clarity.
func NewBatch(db *sqlite3.Conn, writeLock *sync.Mutex, version uint64) (*Batch, error) { + // Check if version exceeds maximum safe value for int64 conversion + const maxSafeVersion = uint64(1 << 63) - if version&(1<<63) != 0 { + if version >= maxSafeVersion { return nil, fmt.Errorf("%d too large; uint64 with the highest bit set are not supported", version) }store/v2/storage/sqlite/db.go (2)
67-70: Consider adding additional pragmas for better performance.While setting WAL mode is good, consider adding other performance-related pragmas:
err = db.Exec("PRAGMA journal_mode=WAL;") +if err != nil { + return nil, fmt.Errorf("failed to set journal mode: %w", err) +} +err = db.Exec("PRAGMA synchronous=NORMAL;") +if err != nil { + return nil, fmt.Errorf("failed to set synchronous mode: %w", err) +} +err = db.Exec("PRAGMA cache_size=-2000;") if err != nil { - return nil, fmt.Errorf("failed to set journal mode: %w", err) + return nil, fmt.Errorf("failed to set cache size: %w", err) }
358-378: Optimize string building in PrintRowsDebug.The current implementation appends to StringBuilder in a loop without pre-allocation. Consider pre-allocating the buffer.
+ // Pre-allocate buffer with reasonable size + sb := strings.Builder{} + sb.Grow(1024) // Adjust size based on expected output for { hasRow, err := stmt.Step() if err != nil { panic(fmt.Errorf("failed to step through SQL rows: %w", err)) } if !hasRow { break }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
store/v2/storage/sqlite/batch.go(4 hunks)store/v2/storage/sqlite/db.go(13 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
store/v2/storage/sqlite/batch.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.
📓 Learnings (1)
store/v2/storage/sqlite/batch.go (1)
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: store/v2/storage/sqlite/batch.go:82-84
Timestamp: 2024-11-04T22:46:19.502Z
Learning: In `store/v2/storage/sqlite/batch.go`, the `lock` in the `Batch` struct is used to synchronize concurrent writers to the SQLite database, and the `Batch` methods (`Set()`, `Delete()`, `Reset()`) are designed for single-threaded access.
🔇 Additional comments (5)
store/v2/storage/sqlite/batch.go (5)
4-8: LGTM: Import changes align with migration to go-sqlite-lite
The import changes correctly implement the migration from mattn/go-sqlite3 to bvinc/go-sqlite-lite as specified in the PR objectives.
58-61: Refer to existing review comment about transaction management in Reset
A previous review has already identified the transaction management issue in the Reset method.
78-89: LGTM: Proper transaction and error handling
The implementation correctly handles:
- Write synchronization using mutex
- Transaction rollback on error
- Error joining for comprehensive error reporting
29-33: 🛠️ Refactor suggestion
Consider using uint64 for version consistency
The version field is defined as int64 but is used with uint64 throughout the codebase. This type mismatch could lead to potential issues.
type Batch struct {
db *sqlite3.Conn
lock *sync.Mutex
ops []batchOp
size int
- version int64
+ version uint64
}Likely invalid or redundant comment.
90-104: 🛠️ Refactor suggestion
Consider using parameterized queries for SQL statements
The SQL statements (reservedUpsertStmt, upsertStmt, delStmt) should be parameterized to prevent SQL injection vulnerabilities.
Consider defining the statements as prepared statements during initialization:
type Batch struct {
// ... existing fields ...
stmtReservedUpsert *sqlite3.Stmt
stmtUpsert *sqlite3.Stmt
stmtDelete *sqlite3.Stmt
}
store/v2/storage/sqlite/db.go
Outdated
| func (db *Database) Prune(version uint64) (err error) { | ||
| v := int64(version) | ||
| db.writeLock.Lock() | ||
| defer db.writeLock.Unlock() | ||
| err = db.storage.Begin() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create SQL transaction: %w", err) | ||
| } | ||
| defer func() { | ||
| if err != nil { | ||
| err = tx.Rollback() | ||
| err = errors.Join(db.storage.Rollback()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve transaction handling in Prune method.
The transaction commit should be handled in the deferred function to ensure proper cleanup.
func (db *Database) Prune(version uint64) (err error) {
v := int64(version)
db.writeLock.Lock()
defer db.writeLock.Unlock()
err = db.storage.Begin()
if err != nil {
return fmt.Errorf("failed to create SQL transaction: %w", err)
}
+ committed := false
defer func() {
- if err != nil {
+ if !committed {
err = errors.Join(db.storage.Rollback())
}
}()Committable suggestion skipped: line range outside the PR's diff.
| @@ -0,0 +1,127 @@ | |||
| package iavlv2 | |||
There was a problem hiding this comment.
Could you please add the tree test like iavl v1?
There was a problem hiding this comment.
I implemented the tests, skipping snaphots for now. It needs to be filled out but that can be done in parallel with the performance tests I'm now doing.
|
Could you fix the conflicts and let's merge this |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
store/v2/commitment/iavl/tree_test.go (1)
19-24: LGTM! Consider documenting the ignored parameter.The function signature change looks good and aligns with the PR objectives. The test coverage remains comprehensive, testing key IAVL tree operations including commits, proofs, and iterations.
Consider adding a comment explaining why the string parameter is ignored, to help future maintainers understand the design decision.
NewStore: func( db corestore.KVStoreWithBatch, - _ string, + _ string, // Unused store key parameter - required for interface compatibility storeKeys, oldStoreKeys []string, logger corelog.Logger, ) (*commitment.CommitStore, error) {store/v2/commitment/store_test_suite.go (1)
36-38: Track the missing snapshot support for iavlv2The FIXME comment indicates that snapshot support for iavlv2 is pending implementation.
Would you like me to create a GitHub issue to track the implementation of snapshot support for iavlv2?
store/v2/commitment/iavlv2/tree.go (2)
3-13: Fix import ordering to comply with Go conventionsThe imports are not properly grouped and ordered. According to the Uber Go Style Guide and the linter suggestions, imports should be grouped into standard library packages, third-party packages, and project-specific packages, each separated by a blank line.
Apply this diff to reorder the imports:
import ( + "errors" + "fmt" + "github.com/cosmos/iavl/v2" + ics23 "github.com/cosmos/ics23/go" + corestore "cosmossdk.io/core/store" + "cosmossdk.io/store/v2" + "cosmossdk.io/store/v2/commitment" - "errors" - "fmt" - corestore "cosmossdk.io/core/store" - "github.com/cosmos/iavl/v2" - ics23 "github.com/cosmos/ics23/go" - "cosmossdk.io/store/v2" - "cosmossdk.io/store/v2/commitment" )🧰 Tools
🪛 golangci-lint (1.62.2)
7-7: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
10-10: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
87-102: Simplify control flow by removing unnecessaryelseclauseThe
elseclause after areturnstatement is unnecessary and can be removed for better readability, as recommended by the Uber Go Style Guide.Apply this diff to simplify the code:
func (t *Tree) Get(version uint64, key []byte) ([]byte, error) { if err := isHighBitSet(version); err != nil { return nil, err } if int64(version) != t.tree.Version() { cloned, err := t.tree.ReadonlyClone() if err != nil { return nil, err } if err = cloned.LoadVersion(int64(version)); err != nil { return nil, err } return cloned.Get(key) - } else { - return t.tree.Get(key) } + return t.tree.Get(key) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (6)
runtime/v2/go.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumstore/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
runtime/v2/go.mod(2 hunks)server/v2/cometbft/go.mod(1 hunks)server/v2/go.mod(2 hunks)simapp/v2/go.mod(1 hunks)store/v2/commitment/iavl/tree_test.go(1 hunks)store/v2/commitment/iavlv2/tree.go(1 hunks)store/v2/commitment/iavlv2/tree_test.go(1 hunks)store/v2/commitment/store_test_suite.go(16 hunks)store/v2/go.mod(5 hunks)store/v2/storage/sqlite/batch.go(4 hunks)store/v2/storage/sqlite/db.go(13 hunks)store/v2/storage/sqlite/iterator.go(6 hunks)tests/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- runtime/v2/go.mod
- server/v2/cometbft/go.mod
- server/v2/go.mod
- simapp/v2/go.mod
- store/v2/go.mod
- store/v2/storage/sqlite/batch.go
🧰 Additional context used
📓 Path-based instructions (7)
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/iavlv2/tree.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/commitment/iavlv2/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/store_test_suite.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/sqlite/iterator.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
📓 Learnings (2)
store/v2/storage/sqlite/db.go (3)
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: server/v2/go.mod:50-50
Timestamp: 2024-11-10T03:53:32.474Z
Learning: The team is considering using modernc.org/sqlite because it's a pure Go implementation, avoids conflicts with iavl/v2, and may offer better performance than the C-based implementations.
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: server/v2/go.mod:50-50
Timestamp: 2024-11-12T04:02:46.663Z
Learning: The team prefers using `github.com/bvinc/go-sqlite-lite` because it is a very minimal wrapper around the original libc API.
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: store/v2/storage/sqlite/db.go:198-198
Timestamp: 2024-11-10T03:53:32.474Z
Learning: SQLite3 does not support `uint64` data types. When working with SQLite3 in Go, especially in `store/v2/storage/sqlite/db.go`, variables representing version numbers that interact with SQLite3 should use `int64` instead of `uint64`.
store/v2/storage/sqlite/iterator.go (3)
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: store/v2/storage/sqlite/iterator.go:154-157
Timestamp: 2024-11-10T03:53:32.474Z
Learning: In the Golang package `github.com/bvinc/go-sqlite-lite/sqlite3`, the `Stmt.Step()` method returns `(bool, error)`. When iteration is complete, it returns `false` with `nil` error, not `sqlite3.ErrDone`. Error handling should check if `err != nil`, and iteration completion is indicated by `hasRow == false`.
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: store/v2/storage/sqlite/iterator.go:169-172
Timestamp: 2024-11-12T04:02:46.663Z
Learning: In `cosmossdk.io/store/v2/storage/sqlite/iterator.go`, when using `github.com/bvinc/go-sqlite-lite/sqlite3`, `Stmt.Scan()` does not return `ErrDone` when there are no more rows. Instead, `Stmt.Step()` is used to advance the iterator and check for the end of results.
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: store/v2/storage/sqlite/iterator.go:88-91
Timestamp: 2024-11-10T03:53:32.474Z
Learning: In the `github.com/bvinc/go-sqlite-lite/sqlite3` library, the `Step()` method returns a `bool` indicating whether a new row is ready, not an `error`.
🪛 golangci-lint (1.62.2)
store/v2/commitment/iavlv2/tree.go
7-7: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
10-10: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🔇 Additional comments (19)
store/v2/commitment/iavlv2/tree_test.go (2)
1-14: LGTM: Package and imports are well-organized
The package declaration and import grouping follow Go conventions, with standard library imports, external dependencies, and internal packages properly separated.
16-20: Verify test coverage for the new IAVL v2 implementation
While the test suite is set up correctly, we should ensure comprehensive test coverage for the new IAVL v2 implementation.
Let's check the test coverage:
✅ Verification successful
Let me gather more information about the test coverage by examining the test suite implementation and the actual tree implementation.
Let me check the actual test suite implementation to understand what test cases are being run.
Test coverage is comprehensive through the shared test suite
The test coverage for the IAVL v2 implementation is sufficient as it uses the comprehensive CommitStoreTestSuite which tests:
- Basic store operations (Get, Set, Remove)
- Version management and loading
- Pruning functionality
- Proof generation and verification
- Store upgrades and migrations
- Iterator functionality
The only explicitly skipped test is for snapshots, which is marked as "not yet supported" and tracked with a FIXME comment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for the iavlv2 package
# Note: This will help identify any gaps in test coverage
# Find all Go files in the iavlv2 package
echo "Go files in package:"
fd -e go . store/v2/commitment/iavlv2/
# Search for test functions to verify coverage
echo -e "\nTest functions:"
rg -t go "^func Test" store/v2/commitment/iavlv2/
Length of output: 345
Script:
#!/bin/bash
# Check the implementation details and test coverage
# Check the test suite implementation
echo "Test suite implementation:"
rg -A 10 "type CommitStoreTestSuite struct" store/v2/commitment/
# Check the actual tree implementation to understand what needs to be tested
echo -e "\nTree implementation:"
cat store/v2/commitment/iavlv2/tree.go
# Check if there are any other test files in the commitment package that might be relevant
echo -e "\nOther test files in commitment package:"
fd -e go test store/v2/commitment/
Length of output: 4684
Script:
#!/bin/bash
# Check the test suite implementation details
# Check the content of the test suite file
echo "Test suite implementation details:"
cat store/v2/commitment/store_test_suite.go
# Check the iavlv2 test implementation
echo -e "\nIAVLv2 test implementation:"
cat store/v2/commitment/iavlv2/tree_test.go
Length of output: 17751
store/v2/storage/sqlite/iterator.go (7)
10-11: LGTM: Import change aligns with library migration
The addition of the bvinc/go-sqlite-lite import is consistent with the PR's objective of migrating SQLite libraries.
26-28: LGTM: Important validation to prevent integer overflow
The validation check for the highest bit is crucial as it prevents potential integer overflow when the version is later converted to int64.
80-83: LGTM: Proper error handling with resource cleanup
The statement binding implementation includes appropriate error handling and ensures proper cleanup of resources by closing the statement on error.
90-94: LGTM: Correct usage of Step() method
The implementation correctly handles the Step() method's (bool, error) return values, as confirmed by previous learnings about the bvinc/go-sqlite-lite library.
138-138: LGTM: Appropriate simplification of Valid() method
The removal of unnecessary error checking aligns with the new SQLite library's error handling patterns.
154-160: LGTM: Correct implementation of Next() using Step()
The implementation properly handles iteration using Step() and correctly manages the iterator's validity state, as confirmed by previous learnings about the library.
172-172: LGTM: Correct implementation of row scanning
The Scan() implementation properly aligns with the bvinc/go-sqlite-lite library's behavior, where row completion is handled by Step() rather than Scan().
store/v2/commitment/iavl/tree_test.go (1)
Line range hint 1-199: Excellent test coverage!
The test suite provides comprehensive coverage of IAVL tree functionality including:
- Core operations (Set, Get, Remove)
- Versioning and commits
- Proof generation
- Pruning
- Forward and reverse iteration
- Error handling
tests/go.mod (2)
91-91: Verify complete removal of mattn/go-sqlite3.
Let's ensure the old SQLite library has been completely removed from the dependencies.
✅ Verification successful
The mattn/go-sqlite3 package has been completely removed
The verification confirms that there are no remaining references to mattn/go-sqlite3 in either go.mod files or Go source files across the codebase. The package has been successfully replaced with github.com/bvinc/go-sqlite-lite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to mattn/go-sqlite3
echo "Checking for mattn/go-sqlite3 in go.mod files:"
fd -e mod -x grep -l "mattn/go-sqlite3" {}
echo "Checking for mattn/go-sqlite3 imports in Go files:"
rg "github.com/mattn/go-sqlite3"
Length of output: 291
91-91: Verify the version and security of bvinc/go-sqlite-lite.
Let's ensure we're using a secure and up-to-date version of the SQLite library.
✅ Verification successful
Let me gather more information about the package versions and dependencies.
Version v0.6.1 of bvinc/go-sqlite-lite appears to be appropriate and secure
The verification shows:
- No security vulnerabilities were found for this package
- The package is consistently used across multiple modules (tests, store/v2, server/v2, runtime/v2, simapp/v2)
- It's properly integrated with direct imports in store/v2/storage/sqlite/
- The old
mattn/go-sqlite3dependency has been completely removed - While there are no GitHub releases, v0.6.1 is a stable version used across the ecosystem
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check latest version and security advisories for bvinc/go-sqlite-lite
# Check latest version from GitHub releases
echo "Latest release version:"
gh api repos/bvinc/go-sqlite-lite/releases/latest --jq .tag_name
# Check for security advisories
echo "Security advisories:"
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/bvinc/go-sqlite-lite") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 693
Script:
#!/bin/bash
# Check go.mod files for both sqlite dependencies
echo "Checking go.mod files for sqlite dependencies:"
rg "github.com/(bvinc/go-sqlite-lite|mattn/go-sqlite3)" -A 1 -B 1
# Check import statements for both sqlite packages
echo -e "\nChecking imports for sqlite packages:"
rg "import.*github.com/(bvinc/go-sqlite-lite|mattn/go-sqlite3)" --type go
# Check actual version from go.mod
echo -e "\nChecking exact version from go.mod:"
rg "github.com/bvinc/go-sqlite-lite.*v\d+\.\d+\.\d+" --type go
Length of output: 4699
store/v2/commitment/store_test_suite.go (3)
31-32: LGTM: Well-structured changes to support different tree implementations
The addition of dbDir parameter and TreeType field provides good flexibility for different backend implementations while maintaining clean separation of concerns.
40-40: LGTM: Excellent test isolation using temporary directories
Consistent use of s.T().TempDir() across all test methods ensures proper test isolation and cleanup.
Also applies to: 74-74, 137-138, 159-159, 172-172, 193-193, 233-233, 276-276, 311-312, 340-340, 413-413
442-442: LGTM: Enhanced test observability with descriptive logs
The addition of clear log messages improves test debugging and observability.
Also applies to: 452-452, 462-462, 471-471
store/v2/commitment/iavlv2/tree.go (1)
136-136:
Provide a parameter name in the PausePruning method to avoid syntax error
The PausePruning method has an unnamed parameter, which is invalid in Go. All function parameters must be named to avoid syntax errors.
Apply this fix to name the parameter:
-func (t *Tree) PausePruning(bool) {}
+func (t *Tree) PausePruning(paused bool) {}store/v2/storage/sqlite/db.go (3)
85-88: Execute SQL statements separately to ensure all commands run
The stmt variable contains multiple SQL statements concatenated into a single string. The sqlite3.Conn.Exec method executes only one statement at a time. Executing multiple statements in a single call may result in only the first statement being executed.
Apply this diff to execute each SQL statement individually:
- err = db.Exec(stmt)
+ stmts := strings.Split(stmt, ";")
+ for _, s := range stmts {
+ s = strings.TrimSpace(s)
+ if s == "" {
+ continue
+ }
+ if err := db.Exec(s + ";"); err != nil {
+ return nil, fmt.Errorf("failed to exec SQL statement '%s': %w", s, err)
+ }
+ }198-198: Use int64 for tomb variable due to SQLite limitations
The tomb variable is correctly declared as int64 because SQLite does not support unsigned integer types. This ensures compatibility when scanning values from the database.
352-354: Remove unnecessary stmt.Exec() call in PrintRowsDebug
The stmt.Exec() method is not appropriate for SELECT queries when using github.com/bvinc/go-sqlite-lite/sqlite3. This call can be removed to prevent runtime errors.
Apply this diff to remove the incorrect call:
- err = stmt.Exec()
- if err != nil {
- panic(fmt.Errorf("failed to execute SQL query: %w", err))
- }Proceed directly to iterating over the results using stmt.Step().
| for _, storeKey := range storeKeys { | ||
| multiTrees[storeKey], _ = mountTreeFn(storeKey) | ||
| } |
There was a problem hiding this comment.
Critical: Don't ignore errors in tree mounting
The error returns from mountTreeFn are being silently ignored in both loops. This could mask important setup failures.
Apply this fix:
for _, storeKey := range storeKeys {
- multiTrees[storeKey], _ = mountTreeFn(storeKey)
+ var err error
+ multiTrees[storeKey], err = mountTreeFn(storeKey)
+ require.NoError(t, err)
}
oldTrees := make(map[string]commitment.Tree)
for _, storeKey := range oldStoreKeys {
- oldTrees[storeKey], _ = mountTreeFn(storeKey)
+ var err error
+ oldTrees[storeKey], err = mountTreeFn(storeKey)
+ require.NoError(t, err)
}Also applies to: 37-39
| path := fmt.Sprintf("%s/%s", dbDir, storeKey) | ||
| tree, err := NewTree(iavl.DefaultTreeOptions(), iavl.SqliteDbOptions{Path: path}, nodePool) |
There was a problem hiding this comment.
Security: Sanitize file paths in SQLite database creation
The path construction using string formatting could be vulnerable to path traversal if storeKey contains directory separators.
Consider using filepath.Join and sanitizing the store key:
-path := fmt.Sprintf("%s/%s", dbDir, storeKey)
+sanitizedKey := strings.ReplaceAll(storeKey, string(filepath.Separator), "_")
+path := filepath.Join(dbDir, sanitizedKey)Don't forget to add the import:
import "path/filepath"
import "strings"| multiTrees := make(map[string]commitment.Tree) | ||
| mountTreeFn := func(storeKey string) (commitment.Tree, error) { | ||
| path := fmt.Sprintf("%s/%s", dbDir, storeKey) | ||
| tree, err := NewTree(iavl.DefaultTreeOptions(), iavl.SqliteDbOptions{Path: path}, nodePool) | ||
| require.NoError(t, err) | ||
| return tree, nil | ||
| } | ||
| for _, storeKey := range storeKeys { | ||
| multiTrees[storeKey], _ = mountTreeFn(storeKey) | ||
| } | ||
| oldTrees := make(map[string]commitment.Tree) | ||
| for _, storeKey := range oldStoreKeys { | ||
| oldTrees[storeKey], _ = mountTreeFn(storeKey) | ||
| } | ||
|
|
||
| return commitment.NewCommitStore(multiTrees, oldTrees, db, logger) | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for SQLite database files
The test creates SQLite database files but doesn't clean them up after the test completes.
Consider adding cleanup logic:
func TestCommitterSuite(t *testing.T) {
+ t.Cleanup(func() {
+ // Clean up SQLite files after tests
+ entries, err := os.ReadDir(dbDir)
+ if err != nil {
+ t.Logf("Failed to read test directory: %v", err)
+ return
+ }
+ for _, entry := range entries {
+ if strings.HasSuffix(entry.Name(), ".db") {
+ if err := os.Remove(filepath.Join(dbDir, entry.Name())); err != nil {
+ t.Logf("Failed to remove test database %s: %v", entry.Name(), err)
+ }
+ }
+ }
+ })Don't forget to add the import:
import "os"
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (16)
store/v2/commitment/iavlv2/tree_test.go (1)
28-29: Use path/filepath for SQLite database pathsThe path joining is done using string formatting, which might not handle path separators correctly across different operating systems.
Consider using
path/filepath.Join:-path := fmt.Sprintf("%s/%s", dbDir, storeKey) +path := filepath.Join(dbDir, storeKey)Don't forget to add the import:
import "path/filepath"store/v2/go.mod (1)
Line range hint
3-3: Invalid Go version specifiedThe go.mod file specifies Go version 1.23 which hasn't been released yet. The latest stable version is Go 1.22.
Please update to a valid Go version:
-go 1.23 +go 1.22store/v2/commitment/iavl/tree_test.go (3)
19-24: LGTM! Consider adding a comment for the unused parameter.The function signature change is well-structured and maintains clean code practices. Consider adding a comment explaining why the string parameter is unused to improve code maintainability.
Add a comment above the function to explain the purpose of the unused parameter:
NewStore: func( db corestore.KVStoreWithBatch, + // dbDir is unused in IAVL implementation as it uses the provided KVStore _ string, storeKeys, oldStoreKeys []string, logger corelog.Logger, ) (*commitment.CommitStore, error) {
Line range hint
123-143: Consider strengthening the pruning tests.The async pruning check could be more robust. Consider:
- Adding negative test cases (e.g., pruning non-existent versions)
- Verifying the state of other versions after pruning
- Adding more assertions around the pruning completion
Here's a suggested addition to strengthen the pruning tests:
// Test pruning non-existent version err = tree.Prune(999) require.Error(t, err) // Verify other versions are intact bz, err := tree.Get(2, []byte("key4")) require.NoError(t, err) require.Equal(t, []byte("value4"), bz) // Verify pruned version is truly gone _, err = tree.Get(1, []byte("key1")) require.Error(t, err)
Line range hint
144-219: Consider adding edge cases to iterator tests.While the iterator tests are comprehensive for the happy path, consider adding:
- Tests with start/end key boundaries
- Tests with empty ranges
- Tests with invalid versions
- Tests with deleted keys in the middle of ranges
Here's a suggested addition:
// Test iterator with boundaries iter, err = tree.Iterator(3, []byte("key3"), []byte("key6"), true) require.NoError(t, err) expectedKeys = []string{"key3", "key4", "key5"} // ... verify iteration // Test empty range iter, err = tree.Iterator(3, []byte("key9"), []byte("key99"), true) require.NoError(t, err) require.False(t, iter.Valid()) require.NoError(t, iter.Close()) // Test invalid version iter, err = tree.Iterator(999, nil, nil, true) require.Error(t, err)store/v2/storage/sqlite/db.go (1)
414-416: Consider adding documentation for isHighBitSet utility functionThe purpose and usage context of this function should be documented.
+// isHighBitSet checks if the highest bit (bit 63) is set in the given version number. +// This is used to ... (please add specific use case). func isHighBitSet(version uint64) bool { return version&(1<<63) != 0 }store/v2/commitment/store_test_suite.go (5)
36-38: TODO: Add tracking issue for iavl/v2 snapshot supportThe test skip indicates missing snapshot functionality in iavl/v2.
Would you like me to create a GitHub issue to track the implementation of snapshot support for iavl/v2?
442-442: Consider using structured loggingThe test uses basic logging statements. Consider using structured logging with fields for better observability.
-s.T().Logf("prune to version %d", latestVersion) +s.T().Logf("pruning old stores", "version", latestVersion) -s.T().Log("GetProof should work for the new stores") +s.T().Log("verifying proof generation", "store_type", "new") -s.T().Logf("Prune to version %d", latestVersion*2) +s.T().Logf("pruning stores", "version", latestVersion*2) -s.T().Log("GetProof should work for the new added store") +s.T().Log("verifying proof generation", "store_type", "newly_added")Also applies to: 452-452, 462-462, 471-471
429-430: Consider combining error checksThe error checks can be combined for better readability.
-err = commitStore.WriteChangeset(corestore.NewChangesetWithPairs(i, kvPairs)) -s.Require().NoError(err) +s.Require().NoError(commitStore.WriteChangeset(corestore.NewChangesetWithPairs(i, kvPairs)))
400-402: Consider consistent variable namingThe variable
prfcould be renamed toprooffor consistency with other test cases.-prf, err := commitStore.GetProof([]byte(storeKey2), i, []byte(fmt.Sprintf("key-%d-%d", i, j))) +proof, err := commitStore.GetProof([]byte(storeKey2), i, []byte(fmt.Sprintf("key-%d-%d", i, j))) -s.Require().NotNil(prf) +s.Require().NotNil(proof)
409-413: Consider grouping related store key declarationsThe store key declarations could be grouped together for better readability.
-Added: []string{"newStore3"}, -} -newRealStoreKeys := []string{storeKey1, "newStore1", "newStore2", "newStore3"} -oldStoreKeys = []string{storeKey2, storeKey3} + Added: []string{"newStore3"}, +} + +// Define store keys for the upgrade +newRealStoreKeys := []string{storeKey1, "newStore1", "newStore2", "newStore3"} +oldStoreKeys = []string{storeKey2, storeKey3}store/v2/commitment/iavlv2/tree.go (5)
136-137: Name the parameter in thePausePruningmethod for clarityEven though the parameter is unused, naming it improves code readability and aligns with the Uber Go Style Guide, which advises naming all function parameters.
Apply this diff to name the parameter:
// PausePruning is unnecessary in IAVL v2 due to the advanced pruning mechanism -func (t *Tree) PausePruning(bool) {} +func (t *Tree) PausePruning(paused bool) {}
87-103: Simplify control flow in theGetmethod by removing the unnecessaryelseSince the
ifblock ends with areturnstatement, theelseis redundant. Removing it improves readability.Apply this diff to simplify the method:
func (t *Tree) Get(version uint64, key []byte) ([]byte, error) { if err := isHighBitSet(version); err != nil { return nil, err } if int64(version) != t.tree.Version() { cloned, err := t.tree.ReadonlyClone() if err != nil { return nil, err } if err = cloned.LoadVersion(int64(version)); err != nil { return nil, err } return cloned.Get(key) - } else { + } return t.tree.Get(key) - } }
142-147: Clarify the error message inisHighBitSetfunctionEnhancing the error message provides clearer information to the user about the limitation.
Apply this diff to improve the error message:
func isHighBitSet(version uint64) error { if version&(1<<63) != 0 { - return fmt.Errorf("%d too large; uint64 with the highest bit set are not supported", version) + return fmt.Errorf("version %d is too large; versions must be less than 2^63", version) } return nil }
34-133: Add comments to exported methods for documentationAdding comments to exported methods enhances code readability and aids in generating documentation, conforming to Go best practices and the Uber Go Style Guide.
For example, you can add comments like:
// Set inserts or updates a key-value pair in the tree. func (t *Tree) Set(key, value []byte) error { // method implementation }Consider adding such comments to all exported methods in the
Treestruct.
88-100: Wrap errors with additional context in theGetmethodProviding context when returning errors enhances debuggability by indicating where and why an error occurred.
Apply this diff to wrap errors:
func (t *Tree) Get(version uint64, key []byte) ([]byte, error) { if err := isHighBitSet(version); err != nil { return nil, err } if int64(version) != t.tree.Version() { cloned, err := t.tree.ReadonlyClone() if err != nil { - return nil, err + return nil, fmt.Errorf("failed to clone tree: %w", err) } if err = cloned.LoadVersion(int64(version)); err != nil { - return nil, err + return nil, fmt.Errorf("failed to load version %d: %w", version, err) } return cloned.Get(key) } return t.tree.Get(key) }Consider applying similar error wrapping in other methods where errors are returned from underlying calls.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (6)
runtime/v2/go.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumstore/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
runtime/v2/go.mod(2 hunks)server/v2/cometbft/go.mod(1 hunks)server/v2/go.mod(2 hunks)simapp/v2/go.mod(1 hunks)store/v2/commitment/iavl/tree_test.go(1 hunks)store/v2/commitment/iavlv2/tree.go(1 hunks)store/v2/commitment/iavlv2/tree_test.go(1 hunks)store/v2/commitment/store_test_suite.go(16 hunks)store/v2/go.mod(5 hunks)store/v2/storage/sqlite/batch.go(4 hunks)store/v2/storage/sqlite/db.go(13 hunks)store/v2/storage/sqlite/iterator.go(6 hunks)tests/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- runtime/v2/go.mod
- server/v2/cometbft/go.mod
- server/v2/go.mod
- simapp/v2/go.mod
- store/v2/storage/sqlite/batch.go
- store/v2/storage/sqlite/iterator.go
🧰 Additional context used
📓 Path-based instructions (6)
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/iavlv2/tree.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/commitment/iavlv2/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/store_test_suite.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.
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
📓 Learnings (1)
store/v2/storage/sqlite/db.go (3)
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: server/v2/go.mod:50-50
Timestamp: 2024-11-10T03:53:32.474Z
Learning: The team is considering using modernc.org/sqlite because it's a pure Go implementation, avoids conflicts with iavl/v2, and may offer better performance than the C-based implementations.
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: server/v2/go.mod:50-50
Timestamp: 2024-11-12T04:02:46.663Z
Learning: The team prefers using `github.com/bvinc/go-sqlite-lite` because it is a very minimal wrapper around the original libc API.
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: store/v2/storage/sqlite/db.go:198-198
Timestamp: 2024-11-10T03:53:32.474Z
Learning: SQLite3 does not support `uint64` data types. When working with SQLite3 in Go, especially in `store/v2/storage/sqlite/db.go`, variables representing version numbers that interact with SQLite3 should use `int64` instead of `uint64`.
🔇 Additional comments (11)
store/v2/commitment/iavlv2/tree_test.go (3)
1-14: LGTM: Package and imports are well-organized
The package name and import organization follow Go conventions, with appropriate use of qualified imports for clarity.
45-46: LGTM: Suite execution is correct
The test suite execution follows the standard testify patterns.
29-30: Verify SQLite concurrent access patterns
Since multiple SQLite databases are being created and potentially accessed concurrently during tests, we should verify the SQLite configuration and locking mechanisms.
Let's check the SQLite configuration and usage patterns:
✅ Verification successful
SQLite concurrent access is properly handled
The SQLite implementation has appropriate concurrency controls in place:
-
A global
writeLockmutex is used to synchronize all write operations:- The lock is held during batch writes
- The lock is used for version updates and pruning operations
- Each database instance has its own mutex
-
Write operations are properly transactional:
- Batch operations use explicit BEGIN/COMMIT/ROLLBACK
- Error handling includes proper transaction rollback
- The write lock ensures no concurrent writes can occur
-
The implementation has been tested for concurrent access:
- TestParallelWrites verifies concurrent batch operations
- TestParallelWriteAndPruning validates concurrent writes with pruning
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check SQLite configuration and concurrent access patterns
# Look for SQLite configuration settings
rg -A 5 "SqliteDbOptions" .
# Check for concurrent access patterns
ast-grep --pattern 'func ($x *Tree) $method($$$) {
$$$
db.$$$
$$$
}'
Length of output: 1067
Script:
#!/bin/bash
# Let's check the SQLite implementation and concurrent access handling
# Check SQLite DB implementation
rg -A 10 "NewSqliteDb" .
# Look for any mutex or locking mechanisms
rg -A 5 "sync\.Mutex|sync\.RWMutex" .
# Check test files for concurrent operations
rg -A 5 "parallel|go func|t\.Parallel" .
Length of output: 100356
Script:
#!/bin/bash
# Let's check the SQLite database implementation and concurrent access handling
# Look for SQLite database initialization and configuration
rg -A 10 "NewSqliteDb|SqliteDb" store/v2/commitment/iavlv2/
# Check for any transaction or write locking mechanisms in SQLite implementation
rg -A 10 "writeLock|Write|Set|Delete" store/v2/storage/sqlite/
Length of output: 14707
store/v2/go.mod (1)
10-10: Verify SQLite library migration impact
The migration from mattn/go-sqlite3 to bvinc/go-sqlite-lite is a significant change. Let's verify there are no remaining references to the old library.
✅ Verification successful
SQLite library migration is complete
The codebase shows consistent usage of github.com/bvinc/go-sqlite-lite with no remaining references to mattn/go-sqlite3. All SQLite operations are properly using the new library, particularly in the store/v2/storage/sqlite package where the core SQLite functionality is implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old SQLite library
rg "github.com/mattn/go-sqlite3" --type go
Length of output: 42
Script:
#!/bin/bash
# Let's verify SQLite usage patterns in the codebase
rg "sqlite" --type go -i
# Also check for any SQLite related imports
ast-grep --pattern 'import ($_) "github.com/bvinc/go-sqlite-lite"'
# Check for SQLite database operations
ast-grep --pattern 'sql.$_($$$)'
Length of output: 4156
store/v2/storage/sqlite/db.go (5)
200-213: LGTM! Proper error handling and type conversion
The implementation correctly handles SQLite's int64 requirement and includes comprehensive error handling for all database operations.
352-354:
Remove unnecessary Exec call in PrintRowsDebug
The Exec call is unnecessary with the bvinc SQLite driver as the prepared statement is ready for stepping.
- err = stmt.Exec()
- if err != nil {
- panic(fmt.Errorf("failed to execute SQL query: %w", err))
- }Likely invalid or redundant comment.
241-243: 🛠️ Refactor suggestion
Improve transaction rollback error handling
The current implementation might mask the original error during rollback.
- err = errors.Join(err, db.storage.Rollback())
+ if rErr := db.storage.Rollback(); rErr != nil {
+ err = fmt.Errorf("%v; additionally failed to rollback: %w", err, rErr)
+ }Likely invalid or redundant comment.
123-128: 🛠️ Refactor suggestion
Improve error handling in deferred statement closure
The current implementation might mask the original error with the close error.
- defer func(stmt *sqlite3.Stmt) {
- cErr := stmt.Close()
- if cErr != nil {
- err = errors.Join(err, fmt.Errorf("failed to close GetLatestVersion statement: %w", cErr))
- }
- }(stmt)
+ defer func(stmt *sqlite3.Stmt) {
+ if cErr := stmt.Close(); cErr != nil {
+ if err == nil {
+ err = fmt.Errorf("failed to close GetLatestVersion statement: %w", cErr)
+ } else {
+ err = fmt.Errorf("%v; additionally failed to close statement: %w", err, cErr)
+ }
+ }
+ }(stmt)Likely invalid or redundant comment.
85-88:
Execute SQL statements separately for better error handling
The initialization SQL contains multiple statements. If one fails, it's harder to identify which statement caused the error.
- err = db.Exec(stmt)
+ for _, s := range strings.Split(stmt, ";") {
+ s = strings.TrimSpace(s)
+ if s == "" {
+ continue
+ }
+ if err := db.Exec(s); err != nil {
+ return nil, fmt.Errorf("failed to execute statement %q: %w", s, err)
+ }
+ }Likely invalid or redundant comment.
tests/go.mod (1)
91-91: 💡 Codebase verification
Caution: Consider updating go-sqlite-lite dependency
The analysis reveals several concerns about the go-sqlite-lite v0.6.1 dependency:
- The repository has been inactive since December 2020 (last push)
- Version v0.6.1 was released in May 2019 (3+ years old)
- There are 8 open issues without recent activity
- While version consistency is maintained across all modules (v0.6.1), the dependency's maintenance status is concerning
Consider:
- Evaluating more actively maintained SQLite alternatives
- If keeping this dependency, thoroughly test its functionality and document the decision
- Creating a tracking issue to monitor the dependency's status
🔗 Analysis chain
Verify the stability and security of go-sqlite-lite v0.6.1
The migration to github.com/bvinc/go-sqlite-lite aligns with the PR objectives for explicit locking mechanisms. However, we should verify:
- The stability of v0.6.1
- Any known security vulnerabilities
- Compatibility with other modules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of sqlite library versions across modules and check for any security advisories
# Check for version consistency across all go.mod files
echo "Checking sqlite library versions across modules..."
fd -e mod -x grep -l "bvinc/go-sqlite-lite"
# Check GitHub API for release information and issues
gh api \
-H "Accept: application/vnd.github+json" \
repos/bvinc/go-sqlite-lite/releases | jq '.[0]'
# Check for open issues related to v0.6.1
gh api \
-H "Accept: application/vnd.github+json" \
repos/bvinc/go-sqlite-lite/issues \
-q '.[] | select(.title | contains("v0.6.1"))'
Length of output: 507
Script:
#!/bin/bash
# Let's check the specific version being used across all go.mod files
echo "Checking specific versions used across modules..."
fd -e mod -x grep "bvinc/go-sqlite-lite"
# Check total number of open issues and last commit date
gh api \
-H "Accept: application/vnd.github+json" \
repos/bvinc/go-sqlite-lite | jq '{open_issues: .open_issues_count, last_push: .pushed_at, stars: .stargazers_count}'
# Check commit activity around v0.6.1
gh api \
-H "Accept: application/vnd.github+json" \
repos/bvinc/go-sqlite-lite/commits | jq '.[0:3] | .[] | {sha: .sha, date: .commit.author.date, message: .commit.message}'
Length of output: 1226
⛔ Skipped due to learnings
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22424
File: server/v2/go.mod:50-50
Timestamp: 2024-11-12T04:02:46.663Z
Learning: The team prefers using `github.com/bvinc/go-sqlite-lite` because it is a very minimal wrapper around the original libc API.
store/v2/commitment/store_test_suite.go (1)
31-32: LGTM: Struct changes align with the PR objectives
The addition of dbDir parameter to NewStore and TreeType field supports the integration of iavl/v2 as a commitment backend.
| for _, storeKey := range storeKeys { | ||
| multiTrees[storeKey], _ = mountTreeFn(storeKey) | ||
| } | ||
| oldTrees := make(map[string]commitment.Tree) | ||
| for _, storeKey := range oldStoreKeys { | ||
| oldTrees[storeKey], _ = mountTreeFn(storeKey) | ||
| } |
There was a problem hiding this comment.
Improve error handling in tree initialization
The errors from mountTreeFn are being silently discarded in both loops, which could hide important initialization failures.
Consider handling errors properly:
for _, storeKey := range storeKeys {
- multiTrees[storeKey], _ = mountTreeFn(storeKey)
+ tree, err := mountTreeFn(storeKey)
+ if err != nil {
+ return nil, fmt.Errorf("failed to initialize tree for store key %s: %w", storeKey, err)
+ }
+ multiTrees[storeKey] = tree
}
oldTrees := make(map[string]commitment.Tree)
for _, storeKey := range oldStoreKeys {
- oldTrees[storeKey], _ = mountTreeFn(storeKey)
+ tree, err := mountTreeFn(storeKey)
+ if err != nil {
+ return nil, fmt.Errorf("failed to initialize old tree for store key %s: %w", storeKey, err)
+ }
+ oldTrees[storeKey] = tree
}Committable suggestion skipped: line range outside the PR's diff.
| func TestCommitterSuite(t *testing.T) { | ||
| nodePool := iavl.NewNodePool() | ||
| s := &commitment.CommitStoreTestSuite{ | ||
| TreeType: "iavlv2", | ||
| NewStore: func( | ||
| db corestore.KVStoreWithBatch, | ||
| dbDir string, | ||
| storeKeys, oldStoreKeys []string, | ||
| logger corelog.Logger, | ||
| ) (*commitment.CommitStore, error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for the node pool resource
The nodePool is created but never cleaned up, which could lead to resource leaks during testing.
Consider adding cleanup using t.Cleanup():
func TestCommitterSuite(t *testing.T) {
nodePool := iavl.NewNodePool()
+ t.Cleanup(func() {
+ nodePool.Close()
+ })
s := &commitment.CommitStoreTestSuite{📝 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.
| func TestCommitterSuite(t *testing.T) { | |
| nodePool := iavl.NewNodePool() | |
| s := &commitment.CommitStoreTestSuite{ | |
| TreeType: "iavlv2", | |
| NewStore: func( | |
| db corestore.KVStoreWithBatch, | |
| dbDir string, | |
| storeKeys, oldStoreKeys []string, | |
| logger corelog.Logger, | |
| ) (*commitment.CommitStore, error) { | |
| func TestCommitterSuite(t *testing.T) { | |
| nodePool := iavl.NewNodePool() | |
| t.Cleanup(func() { | |
| nodePool.Close() | |
| }) | |
| s := &commitment.CommitStoreTestSuite{ | |
| TreeType: "iavlv2", | |
| NewStore: func( | |
| db corestore.KVStoreWithBatch, | |
| dbDir string, | |
| storeKeys, oldStoreKeys []string, | |
| logger corelog.Logger, | |
| ) (*commitment.CommitStore, error) { |
| github.com/cosmos/iavl v1.3.1 | ||
| github.com/cosmos/iavl/v2 v2.0.0-20241128205019-1b18c0edbbd9 |
There was a problem hiding this comment.
Remove redundant IAVL v1 dependency
Both IAVL v1 and v2 are listed as direct dependencies. Since we're migrating to IAVL v2, we should remove v1 to avoid potential conflicts.
Remove the v1 dependency:
- github.com/cosmos/iavl v1.3.1📝 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.
| github.com/cosmos/iavl v1.3.1 | |
| github.com/cosmos/iavl/v2 v2.0.0-20241128205019-1b18c0edbbd9 | |
| github.com/cosmos/iavl/v2 v2.0.0-20241128205019-1b18c0edbbd9 |
| @@ -71,3 +75,5 @@ | |||
| google.golang.org/protobuf v1.35.2 // indirect | |||
| gopkg.in/yaml.v3 v3.0.1 // indirect | |||
| ) | |||
|
|
|||
| replace cosmossdk.io/core => ../../core | |||
There was a problem hiding this comment.
💡 Codebase verification
Replace directive in store/v2/go.mod is inconsistent with other modules
The analysis shows that while many modules depend on cosmossdk.io/core v1.0.0-alpha.6, only store/v2 uses a replace directive to point to a local path. Other modules that need local development configurations (like tests, simapp/v2, runtime/v2) use replace directives for core/testing but not for the core module itself.
- Consider removing the replace directive
replace cosmossdk.io/core => ../../corefrom store/v2/go.mod and use the versioned dependency like other modules - If local development is needed, ensure this replace directive is added consistently across all modules that depend on core
🔗 Analysis chain
Verify consistency of local replace directive
The replace directive for the core module is using a relative path. Let's verify this is consistent with other modules.
Let me generate additional scripts to verify the consistency of replace directives across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check replace directives in other go.mod files
fd -e mod -x sh -c 'echo "=== {} ===" && cat {} | rg "^replace cosmossdk.io/core"'
Length of output: 1630
Script:
#!/bin/bash
# Check all replace directives in store/v2/go.mod
echo "=== Replace directives in store/v2/go.mod ==="
cat store/v2/go.mod | rg "^replace"
# Check if core module is imported in store/v2/go.mod
echo -e "\n=== Core module import in store/v2/go.mod ==="
cat store/v2/go.mod | rg "cosmossdk.io/core"
# Check if other modules that import core have similar replace directives
echo -e "\n=== Modules importing core ==="
rg "cosmossdk.io/core" -g 'go.mod'
Length of output: 5949
| @@ -48,6 +51,7 @@ | |||
| github.com/hashicorp/golang-lru v1.0.2 // indirect | |||
| github.com/klauspost/compress v1.17.9 // indirect | |||
| github.com/klauspost/cpuid/v2 v2.2.9 // indirect | |||
| github.com/kocubinski/costor-api v1.1.1 // indirect | |||
There was a problem hiding this comment.
💡 Codebase verification
Remove unused indirect dependency costor-api
The github.com/kocubinski/costor-api appears to be an unused indirect dependency. It's only present in go.mod and go.sum files without any actual usage in the codebase. Since there are no direct imports or usage of this package found in any Go files, it can be safely removed.
🔗 Analysis chain
Verify necessity of costor-api dependency
The addition of github.com/kocubinski/costor-api as an indirect dependency is notable. Let's verify its usage and necessity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of costor-api in the codebase
rg "github.com/kocubinski/costor-api" --type go
# Check if it's a transitive dependency
go mod graph | rg "costor-api"
Length of output: 171
Script:
#!/bin/bash
# Search for any imports or usage of costor
rg -i "costor" --type go -A 3
# Find all Go files and check their content for costor
fd -e go -x cat {} | rg -i "costor" -A 3
# Look for any references to costor in the entire codebase
rg -i "costor" -A 3
Length of output: 913
|
Linter wants a |
kocubinski
requested review from
facundomedica and
sontrinh16
as code owners
kocubinski
requested review from
akhilkumarpilli and
hieuvubk
as code owners
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
store/v2/storage/pebbledb/batch.go (1)
Line range hint
91-99: Implementation looks good, but consider adding safety measures.The Write method correctly handles error propagation and resource cleanup. However, consider these improvements:
- Add a guard against multiple calls to Write() after the batch is closed
- Consider setting b.batch to nil after closing to prevent usage of a closed batch
Here's a suggested implementation:
+// Write flushes any accumulated data to disk and closes the batch. +// After calling Write, the batch cannot be used. func (b *Batch) Write() (err error) { + if b.batch == nil { + return fmt.Errorf("batch already closed") + } defer func() { err = errors.Join(err, b.batch.Close()) + b.batch = nil // Prevent usage after close }() return b.batch.Commit(&pebble.WriteOptions{Sync: b.sync}) }server/v2/cometbft/internal/mock/mock_store.go (1)
24-26: Consider adding PebbleDB configuration optionsThe current implementation uses default PebbleDB settings. Consider making the configuration customizable for different test scenarios.
Example improvement:
+type MockStorageOptions struct { + PebbleDBOpts *pebbledb.Options +} + -func NewMockStorage(logger log.Logger, dir string) storev2.VersionedWriter { +func NewMockStorage(logger log.Logger, dir string, opts *MockStorageOptions) storev2.VersionedWriter { + pdbOpts := pebbledb.DefaultOptions() + if opts != nil && opts.PebbleDBOpts != nil { + pdbOpts = opts.PebbleDBOpts + } - storageDB, _ := pebbledb.New(dir) + storageDB, err := pebbledb.New(dir, pdbOpts)store/v2/storage/store.go (1)
Line range hint
13-15: Document the batch buffer size constant.The
defaultBatchBufferSizeconstant would benefit from more detailed documentation explaining why 100,000 was chosen and what factors influence this value.const ( - // TODO: it is a random number, need to be tuned + // defaultBatchBufferSize defines the maximum number of operations in a batch + // before it needs to be written to the database. This value affects memory usage + // and write performance. A larger value means fewer writes but more memory usage. + // TODO: Tune this value based on performance benchmarks and memory constraints. defaultBatchBufferSize = 100000 )store/v2/root/upgrade_test.go (1)
47-49: Consider adding PebbleDB cleanup in teardownWhile the PebbleDB initialization looks good, consider adding a cleanup step in a
TearDownTestmethod to ensure proper resource cleanup after tests. PebbleDB may need explicit closing to release file handles.Example implementation:
func (s *UpgradeStoreTestSuite) TearDownTest() { if db, ok := s.rootStore.GetStateStorage().(*pebbledb.DB); ok { _ = db.Close() } }store/v2/root/store_test.go (2)
50-52: Consider adding cleanup for PebbleDB resourcesWhile the initialization looks correct, PebbleDB might need explicit cleanup to release resources. Consider adding a cleanup step in the
TearDownTestmethod.func (s *RootStoreTestSuite) TearDownTest() { + if db, ok := s.rootStore.GetStateStorage().(*pebbledb.DB); ok { + _ = db.Close() + } err := s.rootStore.Close() s.Require().NoError(err) }
Missing PebbleDB-specific error handling and recovery tests
The current test coverage for PebbleDB is insufficient in several areas:
- No specific error handling tests for PebbleDB initialization failures
- Missing tests for PebbleDB-specific failure modes (disk full, permission issues)
- No tests for PebbleDB recovery scenarios after crashes
- Lack of tests for PebbleDB-specific configuration options and their impact
Key files that need additional test coverage:
store/v2/storage/pebbledb/db_test.gostore/v2/storage/pebbledb/comparator_test.go🔗 Analysis chain
Line range hint
1-824: Verify test coverage for PebbleDB-specific behaviorsThe test suite has been migrated from SQLite to PebbleDB, but it might be missing tests for PebbleDB-specific behaviors or failure modes.
Would you like me to help generate additional test cases for PebbleDB-specific scenarios?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any PebbleDB-specific test cases rg -l "TestPebbleDB" . # Check for error handling tests related to PebbleDB rg "pebbledb.*err\s*:?=" .Length of output: 76
Script:
#!/bin/bash # Let's check the PebbleDB test coverage more thoroughly # First, check the PebbleDB implementation file fd "pebbledb\.go" . # Then check for PebbleDB error handling in implementation rg -A 5 "pebbledb.*New" . # Check for any PebbleDB-specific test cases in db_test.go cat store/v2/db/db_test.go # Look for PebbleDB usage in test files rg -l "pebbledb" . | grep "_test\.go"Length of output: 8827
Script:
#!/bin/bash # Let's check PebbleDB-specific test coverage in more detail # Check PebbleDB comparator tests cat store/v2/storage/pebbledb/comparator_test.go # Check PebbleDB-specific DB tests cat store/v2/storage/pebbledb/db_test.go # Look for PebbleDB-specific error handling tests rg -A 5 "TestPebbleDB.*Error" . rg -A 5 "pebbledb.*err.*handling" .Length of output: 2095
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (5)
runtime/v2/go.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumserver/v2/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
runtime/v2/go.mod(1 hunks)server/v2/cometbft/go.mod(0 hunks)server/v2/cometbft/internal/mock/mock_store.go(2 hunks)server/v2/go.mod(1 hunks)server/v2/testdata/app.toml(1 hunks)simapp/v2/go.mod(0 hunks)store/v2/go.mod(5 hunks)store/v2/pruning/manager_test.go(2 hunks)store/v2/root/factory.go(2 hunks)store/v2/root/migrate_test.go(2 hunks)store/v2/root/store_test.go(6 hunks)store/v2/root/upgrade_test.go(2 hunks)store/v2/storage/pebbledb/batch.go(1 hunks)store/v2/storage/sqlite/batch.go(0 hunks)store/v2/storage/sqlite/db.go(0 hunks)store/v2/storage/sqlite/db_test.go(0 hunks)store/v2/storage/sqlite/iterator.go(0 hunks)store/v2/storage/storage_bench_test.go(0 hunks)store/v2/storage/store.go(1 hunks)tests/go.mod(0 hunks)tools/confix/data/v2-app.toml(1 hunks)
💤 Files with no reviewable changes (8)
- server/v2/cometbft/go.mod
- simapp/v2/go.mod
- store/v2/storage/sqlite/batch.go
- store/v2/storage/sqlite/db.go
- store/v2/storage/sqlite/db_test.go
- store/v2/storage/sqlite/iterator.go
- store/v2/storage/storage_bench_test.go
- tests/go.mod
🚧 Files skipped from review as they are similar to previous changes (3)
- runtime/v2/go.mod
- server/v2/go.mod
- store/v2/go.mod
🧰 Additional context used
📓 Path-based instructions (8)
server/v2/cometbft/internal/mock/mock_store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/pruning/manager_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
store/v2/root/factory.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/root/migrate_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
store/v2/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/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/storage/pebbledb/batch.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.
🔇 Additional comments (16)
server/v2/testdata/app.toml (1)
29-29:
Breaking Change: Default state storage type changed to 'pebble'
This is a breaking change that could affect existing deployments using SQLite. Please ensure:
- This change is documented in the migration guide
- Instructions for data migration from SQLite to PebbleDB are provided
- The change is mentioned in the CHANGELOG.md
store/v2/storage/pebbledb/batch.go (1)
Line range hint 91-99: Verify consistency with other storage implementations.
Let's ensure the Write implementation is consistent with other storage backends and follows the same patterns.
✅ Verification successful
Write implementation is consistent with other storage backends
The implementation in PebbleDB's batch.go follows similar patterns to RocksDB's implementation:
- Both properly clean up resources (Close/Destroy)
- Both handle the actual write operation (Commit/Write)
- Both use deferred cleanup to ensure resources are released
- Both maintain consistent error handling patterns
The main differences are implementation-specific due to the underlying database APIs, but the overall structure and approach are consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar Write method implementations in other storage backends
ast-grep --pattern 'func $_($_ *Batch) Write() $_ {
$$$
}'
# Look for any batch closing patterns across the codebase
rg -A 5 'func.*Write.*\(\).*error.*{' store/v2/storage/
Length of output: 238
Script:
#!/bin/bash
# Search for similar Write method implementations in other storage backends
ast-grep --pattern 'func ($_ *Batch) Write() error {
$$$
}'
# Look for batch implementations across storage backends
rg -l "type Batch struct" store/v2/storage/
# Look for Write implementations with fixed regex
rg -A 5 "func.*Write.*\(\).*error" store/v2/storage/
Length of output: 852
server/v2/cometbft/internal/mock/mock_store.go (2)
15-15: LGTM: Import change aligns with PR objectives
The change from SQLite to PebbleDB import aligns with the PR's objective of transitioning storage backends.
24-26: Verify the impact of storage backend change
The switch from SQLite to PebbleDB in the mock implementation might affect existing tests or implementations that depend on SQLite-specific behavior.
Let's verify the usage and potential impacts:
store/v2/storage/store.go (2)
131-133: LGTM! Proper batch handling implementation.
The change from resetting to creating a new batch is correct and aligns with the transition to PebbleDB. The error handling is properly implemented.
131-133: Verify batch handling patterns across the codebase.
Let's ensure this batch handling pattern is consistently implemented across the codebase.
✅ Verification successful
Let me gather more context about the batch implementations to ensure a thorough verification.
Batch handling implementation is consistent across the codebase
The batch handling pattern in the code snippet follows the established patterns across the codebase:
- Consistent error handling for batch operations across different database implementations (PebbleDB, RocksDB)
- Proper interface definition in
store/v2/storage/database.gothat all implementations follow - Consistent batch creation with version parameter
- Proper error propagation in all batch operations
The implementation aligns with the codebase's patterns for atomic updates and error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent batch handling patterns across the codebase
# Look for other NewBatch calls and their surrounding context
ast-grep --pattern 'NewBatch($$$)'
# Look for potential batch reuse patterns that might need similar updates
rg -A 5 -B 5 'b.Reset\(\)'
Length of output: 1229
Script:
#!/bin/bash
# Check the actual batch implementations and their error handling
ast-grep --pattern 'type Batch struct {
$$$
}'
# Look for batch-related interfaces and their implementations
ast-grep --pattern 'interface {
$$$
NewBatch($$$)
$$$
}'
# Check error handling patterns around batch operations
rg -A 3 'if err := .*batch.*; err != nil'
Length of output: 7872
tools/confix/data/v2-app.toml (1)
59-60: Verify documentation updates for removed SQLite support
The configuration now only supports "pebble" and "rocksdb" as state storage database types, removing support for SQLite. This is a breaking change that requires proper documentation.
Let's verify if the documentation has been updated:
Consider adding:
- Migration guide for users currently using SQLite
- Clear error messages when users attempt to use unsupported database types
- Documentation updates reflecting the current supported types
store/v2/root/upgrade_test.go (1)
18-18: LGTM: Import change aligns with storage backend migration
The replacement of SQLite with PebbleDB import is consistent with the PR's objective of transitioning to a new storage backend.
store/v2/root/factory.go (2)
36-36: LGTM: Documentation accurately reflects supported storage types
The updated comment correctly lists the supported storage types as "pebble" and "rocksdb", aligning with the removal of SQLite support.
55-55: Verify breaking change documentation
Changing the default storage type from SQLite to Pebble is a breaking change that could affect existing applications.
store/v2/root/migrate_test.go (2)
21-21: LGTM: Import changes align with storage backend migration
The addition of the PebbleDB import aligns with the PR's objective of transitioning away from SQLite.
65-67: LGTM: Clean implementation of storage backend change
The PebbleDB initialization and integration with the storage store is implemented correctly with proper error handling.
store/v2/pruning/manager_test.go (3)
18-18: LGTM: Import statement correctly updated
The import statement for the new PebbleDB storage backend is properly added.
Line range hint 1-248: Test coverage looks comprehensive
The test suite thoroughly covers:
- Basic pruning functionality
- Various pruning options
- Commit signaling and edge cases
- Timing-sensitive operations
The transition to PebbleDB maintains the same level of test coverage without compromising existing test scenarios.
48-50: LGTM: Storage backend properly initialized
The PebbleDB storage initialization is correctly implemented, maintaining consistent error handling and temporary directory usage.
Let's verify that all test cases pass with the new storage backend:
✅ Verification successful
Let me gather more information about the test implementation to ensure the storage backend change is properly tested.
Let me try a different approach to check the test implementation.
Let me try one more search to specifically check the test implementation and assertions:
Let me check one final aspect of the test implementation:
Storage backend initialization and test implementation verified
The PebbleDB storage initialization is correctly implemented and thoroughly tested. The test suite:
- Properly initializes PebbleDB with temporary directory
- Verifies storage operations through extensive write and read tests
- Includes proper error handling and assertions
- Tests pruning functionality with both commitment and storage stores
- Uses appropriate timeouts for async operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any skipped or failing tests in the pruning package
# Search for any skipped tests
rg -B 2 -A 2 "\.Skip\(" "store/v2/pruning"
# Search for potential test reliability issues
rg -B 2 -A 2 "\.Eventually\(" "store/v2/pruning"
Length of output: 1344
Script:
#!/bin/bash
# Check the test implementation details
ast-grep --pattern 'func \(s \*ManagerTestSuite\) Test$_($$$) {
$$$
}'
# Check for any storage-related test setup/teardown
ast-grep --pattern 'func \(s \*ManagerTestSuite\) $_(t \*testing.T) {
$$$
}'
Length of output: 155
Script:
#!/bin/bash
# Check the test file content to understand the test setup and implementation
cat store/v2/pruning/manager_test.go
# Also check if there are any other test files in the package
fd -e go -x grep -l "testing" {} store/v2/pruning/
Length of output: 139523
Script:
#!/bin/bash
# Check the test implementation focusing on storage initialization and assertions
rg -A 10 "func.*TestPrune" store/v2/pruning/manager_test.go
Length of output: 500
Script:
#!/bin/bash
# Get the full test implementation to verify assertions and error handling
cat store/v2/pruning/manager_test.go | grep -A 50 "func (s \*PruningManagerTestSuite) TestPrune()"
Length of output: 1719
store/v2/root/store_test.go (1)
20-20: LGTM: Import statement correctly updated
The import statement has been properly updated to use the PebbleDB package instead of SQLite.
| @@ -25,8 +25,8 @@ minimum-gas-prices = '0stake' | |||
| app-db-backend = 'goleveldb' | |||
|
|
|||
| [store.options] | |||
| # State storage database type. Currently we support: "sqlite", "pebble" and "rocksdb" | |||
| ss-type = 'sqlite' | |||
| # State storage database type. Currently we support: "pebble" and "rocksdb" | |||
There was a problem hiding this comment.
💡 Codebase verification
Documentation update needed for SQLite removal
The removal of SQLite support is not properly documented in the changelog or migration guides. While SQLite is mentioned in the store/v2 documentation as a supported backend, the removal is not documented. Based on ADR-065, SQLite is listed as one of the three default SS backends, which conflicts with the configuration comment change.
- Add documentation in CHANGELOG.md about the removal of SQLite support
- Update store/v2/storage/README.md to reflect the current supported backends
- Add migration instructions for users currently using SQLite backend
🔗 Analysis chain
Verify documentation updates for removed SQLite support
The comment has been updated to remove SQLite from supported database types. Please ensure this removal is properly documented in the project's documentation and changelog.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the removal of SQLite support is documented
echo "Checking documentation for SQLite removal..."
rg -i "sqlite.*remov|deprecat.*sqlite" docs/
rg -i "sqlite.*remov|deprecat.*sqlite" CHANGELOG.md
Length of output: 199
Script:
#!/bin/bash
# Let's check for any SQLite-related documentation or configuration files
echo "Checking for SQLite-related files and documentation..."
fd -t f -e md -e rst -e txt | xargs rg -i "sqlite"
# Check for SQLite in configuration files
fd -t f -e toml -e yaml -e conf | xargs rg -i "sqlite"
# Check if there are any migration guides
fd -t f -e md migration
Length of output: 884
Script:
#!/bin/bash
# Let's check the content of the migration ADR and store v2 ADR
echo "Checking migration ADR content..."
cat docs/architecture/adr-041-in-place-store-migrations.md
echo -e "\nChecking store v2 ADR content..."
cat docs/architecture/adr-065-store-v2.md
Length of output: 24208
| @@ -21,7 +21,7 @@ type MockStore struct { | |||
| } | |||
|
|
|||
| func NewMockStorage(logger log.Logger, dir string) storev2.VersionedWriter { | |||
| storageDB, _ := sqlite.New(dir) | |||
| storageDB, _ := pebbledb.New(dir) | |||
There was a problem hiding this comment.
Improve error handling in PebbleDB initialization
The current implementation discards potential initialization errors using a blank identifier (_). Since this is a critical storage component, proper error handling is essential.
Consider this improvement:
- storageDB, _ := pebbledb.New(dir)
+ storageDB, err := pebbledb.New(dir)
+ if err != nil {
+ logger.Error("failed to initialize pebbledb", "error", err)
+ return nil
+ }Committable suggestion skipped: line range outside the PR's diff.
| pebbleDB, err := pebbledb.New(s.T().TempDir()) | ||
| s.Require().NoError(err) | ||
| ss := storage.NewStorageStore(sqliteDB, testLog) | ||
| ss := storage.NewStorageStore(pebbleDB, testLog) |
There was a problem hiding this comment.
💡 Codebase verification
Add TearDownTest to properly close PebbleDB resources
The verification reveals that while PebbleDB and StorageStore have proper Close methods implemented, the MigrateStoreTestSuite lacks a TearDownTest method to clean up these resources. Please add:
func (s *MigrateStoreTestSuite) TearDownTest() {
if err := s.ss.Close(); err != nil {
s.T().Errorf("failed to close storage store: %v", err)
}
}store/v2/root/migrate_test.go: Add TearDownTest method to ensure proper cleanup
🔗 Analysis chain
Verify PebbleDB cleanup after tests
While using T.TempDir() handles directory cleanup, we should verify that PebbleDB resources are properly released.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Close/Cleanup methods in PebbleDB usage
ast-grep --pattern 'pebbleDB.Close()'
Length of output: 39
Script:
#!/bin/bash
# Search for Close method calls in test files
rg -A 3 "\.Close\(\)" store/v2/root/migrate_test.go
# Search for TearDownTest or cleanup methods
ast-grep --pattern 'func (s $_) TearDownTest() {
$$$
}'
# Check PebbleDB interface or struct definition for cleanup methods
rg -A 5 "type.*PebbleDB"
Length of output: 668
Script:
#!/bin/bash
# Check if PebbleDB implements Close method
rg -A 5 "func.*PebbleDB.*Close" store/v2/db/pebbledb.go
# Check if StorageStore has Close method and how it handles PebbleDB cleanup
rg -A 5 "func.*StorageStore.*Close"
# Check the test suite struct to see if it embeds testing.Suite
rg "type.*MigrateTestSuite" store/v2/root/migrate_test.go -A 3
Length of output: 398
Script:
#!/bin/bash
# Check the test suite struct and its setup/teardown
rg "type MigrateTestSuite struct" -A 5 store/v2/root/migrate_test.go
# Look for suite.Run usage in the test file
rg "suite.Run" store/v2/root/migrate_test.go -A 3
# Check if StorageStore is properly closed in the test
rg "ss.*Close\(\)" store/v2/root/migrate_test.go -A 3
Length of output: 265
| pebbleDB, err := pebbledb.New(s.T().TempDir()) | ||
| s.Require().NoError(err) | ||
| ss := storage.NewStorageStore(sqliteDB, noopLog) | ||
| ss := storage.NewStorageStore(pebbleDB, noopLog) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify consistent PebbleDB initialization across test methods
The PebbleDB initialization pattern is repeated across multiple test methods. Consider extracting this into a helper method to ensure consistency and reduce code duplication.
+func (s *RootStoreTestSuite) newPebbleDB() *pebbledb.DB {
+ db, err := pebbledb.New(s.T().TempDir())
+ s.Require().NoError(err)
+ return db
+}Then use it in all test methods:
- pebbleDB, err := pebbledb.New(s.T().TempDir())
- s.Require().NoError(err)
+ pebbleDB := s.newPebbleDB()Also applies to: 538-540, 569-571, 619-622
julienrbrt
added
the
backport/v0.52.x
|
Added backport label to due cometbft diff |
(cherry picked from commit 24d456d) # Conflicts: # runtime/v2/go.mod # runtime/v2/go.sum # server/v2/go.mod # server/v2/go.sum # server/v2/testdata/app.toml # simapp/v2/go.mod # simapp/v2/go.sum # store/v2/commitment/iavl/tree_test.go # store/v2/commitment/store_test_suite.go # store/v2/go.mod # store/v2/go.sum # store/v2/pruning/manager_test.go # store/v2/root/factory.go # store/v2/root/migrate_test.go # store/v2/root/store_test.go # store/v2/root/upgrade_test.go # store/v2/storage/pebbledb/batch.go # store/v2/storage/storage_bench_test.go # store/v2/storage/store.go
…22700) Co-authored-by: Matt Kocubinski <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Adds support for iavl/v2 as a commitment backend.
Required for this work is to converge on a single sqlite library to use in both iavl/v2 and store/v2/storage/sqlite. store/v2/storage/sqlite has been migrated in this PR to github.com/bvinc/go-sqlite-lite from github.com/mattn/go-sqlite3.
bvincis a bit more explicit, particularly around not hiding locking, so some changes were required to bring the implementation into alignment.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
Treestruct for enhanced interaction with the commitment tree.Treestruct.CommitStorefunctionality.Writemethod to theBatchstruct in the PebbleDB package for improved batch operations.Bug Fixes
Chores