feat(store/v2): full iavl/v2 support #23131
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request introduces significant modifications across multiple packages, primarily focusing on store management, commitment strategies, and application configuration. The changes include removing the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
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:Simulations
C:Store
Type: Build
C:server/v2
kocubinski
requested review from
auricom,
julienrbrt,
tac0turtle,
a team,
cool-develope and
hieuvubk
as code owners
|
@kocubinski your pull request is missing a changelog! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -303,15 +305,17 @@ | |||
| // background pruning process (iavl v1 for example) which must be paused during the commit | |||
| s.pruningManager.PausePruning() | |||
|
|
|||
| var cInfo *proof.CommitInfo | |||
| st := time.Now() | |||
Check warning
Code scanning / CodeQL
Calling the system time Warning
| if err := s.stateCommitment.WriteChangeset(cs); err != nil { | ||
| return nil, fmt.Errorf("failed to write batch to SC store: %w", err) | ||
| } | ||
|
|
||
| writeDur := time.Since(st) | ||
| st = time.Now() |
Check warning
Code scanning / CodeQL
Calling the system time Warning
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (14)
store/v2/root/store.go (1)
117-121: Avoid returning a partially constructed object when an error occurs
While returning bothNewReaderMap(v, vReader)anderris idiomatic in Go, it can cause confusion if the caller assumesReaderMapis always valid. Consider returning(nil, err)whenerris non-nil to avoid ambiguity for future maintainers.store/v2/root/factory.go (1)
36-36: Add clarifying documentation for the new IavlV2Config field.
The newly introducedIavlV2Config iavl_v2.TreeOptionsfield is valuable for configuring and tuning IAVL v2. Consider adding a short code comment explaining its primary use and potential impact on performance and memory usage.store/v2/commitment/iavlv2/tree.go (2)
23-24: Ensure consistency in naming new struct fields.
Addinglog log.Loggerandpath stringclarifies usage, but make sure these fields follow Go naming conventions (e.g., short, descriptive). Optionally consider renaming them tologgeranddbPathfor clarity.
99-116: Review performance overhead of cloning for historical queries.
When the requested version isn't recent, the logic callst.tree.ReadonlyClone()to load the version. This approach is correct but might be expensive for frequent historical reads. Consider caching or reusing clones if justified by performance metrics.store/v2/proof/commit_info.go (1)
207-208: Double-check ephemeral pointer creation.
func (ci *CommitInfo) CommitID() *CommitIDreturns a newly allocated pointer toCommitID. Ensure no references hold onto this for mutation.simapp/v2/app.go (1)
225-238: Global pruning limit might affect multi-tenant usage.
iavlv2.SetGlobalPruneLimit(1)applies process-wide. Verify that no other modules or concurrently running instances rely on a different prune limit.store/v2/commitment/store.go (2)
88-97: Enhance error context inwriteChangeset.
You might improve debuggability by adding store key context or a log statement for each failingRemoveorSetoperation, facilitating easier troubleshooting in complex setups.
267-280: Extra logging for version mismatches.
If the commit version differs from the expected version, log the mismatch. This helps pinpoint storeKey or system context in logs for debugging multi-store concurrency.store/v2/commitment/iavlv2/snapshot.go (2)
49-52: Confirm that repeated commits are not expected.
After callingCommit(), further additions to the sameImporterobject typically shouldn’t be allowed. Consider adding state checks to guard against accidental misuse.
54-59: Check error handling on importer close.
We ignore the potential error fromi.importer.Close(). It might be helpful to return that error instead of always returningnilfor proper resource cleanup and error tracing.store/v2/commitment/iavl/tree.go (1)
203-205: Add a brief method comment explaining concurrency safety guarantees.This method returns
false, correctly indicating the tree is not concurrency-safe. Adding a short comment on why concurrency is unsupported here can help future maintainers.store/v2/commitment/iavlv2/tree_test.go (1)
28-28: Enhance concurrency tests for new constructor usage.You’re correctly verifying instantiation here. However, consider adding additional tests to confirm concurrency behavior with the new
IsConcurrentSafe()method–for instance, parallel writes or other concurrency scenarios.scripts/init-simapp-v2.sh (1)
21-21: Use a placeholder variable or underscore for unused loop index.The loop variable
iis declared but never used, triggering a Shellcheck warning. Rename it or use_to avoid confusion.-for i in $(seq 10); do +for _ in $(seq 10); do alias=$(dd if=/dev/urandom bs=16 count=24 2>/dev/null | base32 | head -c 32) $SIMD_BIN keys add "$alias" --indiscreet aliases="$aliases $alias" done🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 21-21: i appears unused. Verify use (or export if used externally).
(SC2034)
store/v2/database.go (1)
21-26: Consider adding documentation for the new interface.The new
LatestReaderinterface provides methods for reading the latest state, which is crucial for the IAVL/v2 integration. However, it lacks documentation explaining its purpose and relationship withVersionedReader.Add documentation above the interface:
+// LatestReader defines an interface for reading the latest state without version specification. +// This interface is used for concurrent-safe operations on the latest state, complementing +// the versioned access provided by VersionedReader. type LatestReader interface { Has(storeKey, key []byte) (bool, error) Get(storeKey, key []byte) ([]byte, error) Iterator(storeKey, start, end []byte) (corestore.Iterator, error) ReverseIterator(storeKey, start, end []byte) (corestore.Iterator, error) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
server/v2/cometbft/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 (26)
runtime/v2/module.go(0 hunks)scripts/build/build.mk(1 hunks)scripts/init-simapp-v2.sh(1 hunks)server/v2/cometbft/go.mod(3 hunks)server/v2/commands.go(1 hunks)simapp/v2/app.go(3 hunks)simapp/v2/benchmark.go(2 hunks)simapp/v2/go.mod(7 hunks)store/v2/commitment/iavl/tree.go(1 hunks)store/v2/commitment/iavlv2/snapshot.go(1 hunks)store/v2/commitment/iavlv2/tree.go(4 hunks)store/v2/commitment/iavlv2/tree_test.go(1 hunks)store/v2/commitment/mem/tree.go(1 hunks)store/v2/commitment/store.go(6 hunks)store/v2/commitment/store_test_suite.go(0 hunks)store/v2/commitment/tree.go(1 hunks)store/v2/database.go(1 hunks)store/v2/go.mod(1 hunks)store/v2/proof/commit_info.go(6 hunks)store/v2/proof/commit_info_test.go(2 hunks)store/v2/root/factory.go(4 hunks)store/v2/root/factory_test.go(1 hunks)store/v2/root/store.go(4 hunks)store/v2/store.go(1 hunks)tests/go.mod(3 hunks)tools/benchmark/client/cli/tx.go(3 hunks)
💤 Files with no reviewable changes (2)
- store/v2/commitment/store_test_suite.go
- runtime/v2/module.go
✅ Files skipped from review due to trivial changes (1)
- store/v2/store.go
🧰 Additional context used
📓 Path-based instructions (18)
store/v2/commitment/tree.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/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/mem/tree.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/root/factory_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
simapp/v2/benchmark.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/database.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/commitment/iavl/tree.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"
tools/benchmark/client/cli/tx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/app.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/proof/commit_info_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/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/commitment/iavlv2/snapshot.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/proof/commit_info.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
tools/benchmark/client/cli/tx.go
113-113: var i is unused
(unused)
🪛 Shellcheck (0.10.0)
scripts/init-simapp-v2.sh
[warning] 21-21: i appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (34)
tests/go.mod (2)
81-81: Review dependency on outdated package.
The github.com/aybabtme/uniplot package hasn't been updated since 2015. Consider using a more actively maintained plotting library to ensure better compatibility and security.
Run this script to check for alternative plotting libraries:
159-159: Verify the stability and maintenance of costor-api.
The github.com/kocubinski/costor-api package appears to be maintained by the PR author. Please ensure this package is:
- Well-maintained and documented
- Has proper testing
- Is the appropriate choice versus other available solutions
Run this script to check the package's health:
store/v2/root/store.go (3)
145-145: Returning a copy of the CommitID is safer for immutability
This approach correctly prevents external code from altering internal state.
292-297: Repeated system time calls can introduce non-deterministic behavior
Similar to a prior comment, calling time.Now() in blockchain-related logic may lead to non-determinism. Consider using deterministic timing or purified metric gathering solutions if determinism is required for consensus-critical paths.
308-318: Same non-deterministic time concern in subsequent calls
This usage of time.Now() and time.Since() for measurements at lines 308, 312, 313, and 318 could similarly impact determinism in a blockchain environment.
store/v2/root/factory.go (1)
57-57: Assess memory overhead for increased cache size.
Raising the cache size to 1,000,000 can yield performance benefits, but it also significantly increases memory consumption. Verify that the environment has sufficient memory to prevent out-of-memory issues.
store/v2/commitment/iavlv2/tree.go (3)
27-38: Check node pool and SQLite DB creation errors carefully.
In NewTree, you combine a node pool with a SQLite DB. Ensure detailed logs in case of partial failures, especially if file permissions cause database writes to fail.
179-180: Confirm pruning alignment with the broader system.
The Prune method in iavl/v2 is intentionally no-op here. Validate that higher-level store logic handles pruning properly to avoid over-accumulation of old versions.
186-188: Concurrent safety is acknowledged.
Returning true for IsConcurrentSafe is crucial for enabling parallel writes. Confirm that all internal IAVL v2 operations truly support concurrency at scale in production.
store/v2/proof/commit_info.go (3)
17-17: Check for potential nil pointer pitfalls.
Changing StoreInfos to []*StoreInfo introduces the possibility of nil pointers in the slice. Ensure that all references to StoreInfos handle nil safely, especially in iteration.
Line range hint 174-198: Ensure pointer lifecycles in Unmarshal.
Populating ci.StoreInfos[i] = &StoreInfo{} is correct, but be cautious about reusing or mutating these pointers. The code sets CommitID for every store entry, so confirm that each pointer is unique.
225-225: Guard against nil receivers.
For func (cid *CommitID) IsZero() bool, confirm that cid is never nil when called. Alternatively, handle a nil receiver gracefully to prevent potential panics.
simapp/v2/app.go (1)
7-7: Validate the iavl/v2 import usage.
While iavl/v2 is essential for performance gains, confirm that your environment meets any new library dependencies (e.g., SQLite).
store/v2/commitment/store.go (3)
65-86: Concurrent changeset application logic.
Using errgroup.Group in WriteChangeset is good for parallelism. Verify whether partial failures should be rolled back or partially committed. Currently, a single failure cancels the entire group, but data changes might have partially applied to other trees.
181-209: Properly handle multi-tree rollback on load failure.
When loading versions concurrently (via errgroup), any single tree load failure cancels the group. Ensure the system is left in a consistent state if one load fails while others succeed.
Line range hint 590-602: Reconfirm usage of pointer-based StoreInfos in final commit info.
StoreInfos is now a slice of pointers. Make sure any subsequent code reading or aggregating commit data can handle potential nil references.
store/v2/root/factory_test.go (1)
27-28: Confirm the new test expectation.
Previously, this test expected an error when creating an IAVL v2 root store. It now asserts that everything works without error. Please verify that the underlying CreateRootStore logic fully supports IAVL v2 (including its configuration) and that this change in expectations aligns with the intended behavior.
simapp/v2/benchmark.go (1)
24-25: Be cautious about increasing the benchmark scale.
Raising BucketCount to 5 and GenesisCount to 20 million will significantly increase resource usage and may cause prolonged run times. Ensure your environments can handle the memory and CPU overhead.
store/v2/commitment/mem/tree.go (1)
73-75: This method consistently signals non-concurrency-safety.
Returning false is acceptable for an in-memory store if concurrency is out of scope. However, reevaluate whether concurrency might be beneficial in certain use cases—particularly if non-persistent trees are used in parallel processing.
store/v2/commitment/iavlv2/snapshot.go (2)
17-34: Gracefully handle export termination logic.
Next() properly distinguishes the ErrorExportDone condition, returning a commitment error to indicate the end of the export sequence. This is good. Validate that upstream code interprets commitment.ErrorExportDone correctly, ensuring no unexpected behavior in calling contexts.
45-47: Consider additional input validation in Add().
Currently, items are passed directly to ImportNode. If there's a risk of invalid or malformed data in the snapshot, add upstream checks or validations to avoid data corruption in IAVL.
store/v2/commitment/tree.go (1)
37-38: Add concurrency safety tests to ensure correctness.
The new IsConcurrentSafe() method is a great addition for concurrency management. However, please ensure that there is comprehensive unit or integration testing that validates concurrency behavior and locks, especially in multi-threaded or parallel commits.
store/v2/proof/commit_info_test.go (3)
13-35: Pointer usage standardizes store information handling.
Switching to pointers ([]*StoreInfo) improves consistency across the codebase. This ensures that the commit_info_test.go aligns with the new pointer-based semantics from commit_info.go.
43-43: Maintain consistent pointer usage with commit info.
Using &CommitInfo{} rather than creating a value type is consistent with recent changes, allowing for better reference semantics. This helps avoid unintentional data copying in test logic.
62-62: Efficient marshalling and unmarshalling validation.
Assigning ci2 as a pointer ensures serialization logic is thoroughly tested. This matches the broader code changes to pointer usage. Keep ensuring the final fields are deeply compared to confirm accurate round-trip serialization.
scripts/build/build.mk (1)
15-15: Appending build options enhances extensibility.
Using += instead of := allows you to chain multiple build options easily. Confirm that single quotes (' v2') do not introduce unexpected whitespace or quoting issues in downstream builds.
tools/benchmark/client/cli/tx.go (2)
174-176: Adjust microsecond sleep usage carefully.
The switch to time.Microsecond significantly reduces the pause duration. Ensure this has been validated for performance goals, as it may overload the system with near-constant transaction broadcasts.
185-185: Flag documentation updated correctly.
Reflecting “microseconds” in the flag description clarifies usage. Keep it consistent with the actual sleep unit to avoid confusion, and ensure the user is aware of the significantly shorter delay.
scripts/init-simapp-v2.sh (1)
14-16: Confirmed store type update and commit timeout changes.
Switching the store type to iavl-v2, reducing the commit block time, and enabling Prometheus look correct. Ensure that these changes align with your performance and monitoring goals.
Also applies to: 16-17
store/v2/go.mod (1)
14-14: Verify the stability of IAVL v2.0.0-alpha.4.
The dependency update to an alpha version of IAVL v2 could introduce stability concerns. Please ensure that this version has been thoroughly tested and is stable enough for production use.
server/v2/cometbft/go.mod (1)
57-57: Verify consistency of indirect dependencies across modules.
The addition of new indirect dependencies should be consistent across all related modules to prevent version conflicts:
github.com/aybabtme/uniplotgit.luolix.top/bvinc/go-sqlite-litegit.luolix.top/cosmos/iavl/v2git.luolix.top/kocubinski/costor-api
Also applies to: 60-60, 77-77, 122-122
✅ Verification successful
All indirect dependencies are consistent across modules
Based on the verification results, all the specified indirect dependencies maintain consistent versions across all related modules:
github.com/aybabtme/uniplot: v0.0.0-20151203143629-039c559e5e7egit.luolix.top/bvinc/go-sqlite-lite: v0.6.1git.luolix.top/cosmos/iavl/v2: v2.0.0-alpha.4git.luolix.top/kocubinski/costor-api: v1.1.1
Each dependency appears with the exact same version in all four module files: server/v2/cometbft/go.mod, simapp/v2/go.mod, store/v2/go.mod, and tests/go.mod.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check dependency versions across modules
for dep in "uniplot" "go-sqlite-lite" "iavl/v2" "costor-api"; do
echo "Checking $dep versions across modules:"
fd -e mod | xargs grep -l "$dep" | while read -r file; do
echo "$file:"
grep "$dep" "$file"
done
done
Length of output: 3308
simapp/v2/go.mod (3)
69-69: Good practice: Pinned store dependency to specific commit
The store dependency is correctly pinned to a specific commit (v1.10.0-rc.1.0.20241218084712-ca559989da43) which provides stability and reproducibility.
51-51: Verify compatibility with alpha version of iavl/v2
The dependency on github.com/cosmos/iavl/v2 v2.0.0-alpha.4 is an alpha version. While this aligns with the PR objectives for iavl/v2 integration, we should verify its stability.
✅ Verification successful
Let me gather more information about the IAVL v2 usage in the codebase.
IAVL v2 alpha.4 integration appears stable and well-tested
Based on the verification results:
- No open bug issues were found for IAVL v2.0.0-alpha.4 in the repository
- The codebase shows extensive test coverage for IAVL v2 integration, particularly in:
store/v2/commitment/iavlv2/directory with dedicated tests- Integration tests in
store/v2/root/and other store components
- The implementation is properly isolated in v2-specific paths, allowing for safe migration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any reported issues with iavl v2.0.0-alpha.4
gh api graphql -f query='
{
repository(owner: "cosmos", name: "iavl") {
issues(first: 5, states: OPEN, labels: ["bug"], orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
title
body
createdAt
labels(first: 5) {
nodes {
name
}
}
}
}
}
}
'
Length of output: 389
Script:
#!/bin/bash
# Check for IAVL v2 imports and usage
rg -l "github.com/cosmos/iavl/v2" --type go
# Check go.mod files for any other IAVL dependencies
rg "github.com/cosmos/iavl" --type mod
# Check for any IAVL-related test failures or issues in test files
rg "iavl" --type go -g "*_test.go"
Length of output: 17547
218-218: Verify compatibility with alpha version of bbolt
The update to go.etcd.io/bbolt v1.4.0-alpha.1 introduces another alpha dependency. We should verify its stability for production use.
| @@ -100,6 +102,7 @@ require ( | |||
| github.com/cosmos/btcutil v1.0.5 // indirect | |||
| github.com/cosmos/go-bip39 v1.0.0 // indirect | |||
| github.com/cosmos/iavl v1.3.4 // indirect | |||
| github.com/cosmos/iavl/v2 v2.0.0-alpha.4 // indirect | |||
There was a problem hiding this comment.
💡 Codebase verification
Alpha dependency is used in production code and requires attention
The IAVL v2 alpha dependency is not just used in tests but is actually used in critical store and commitment-related code paths:
store/v2/commitment/iavlv2/tree.gostore/v2/commitment/iavlv2/snapshot.gostore/v2/root/factory.gosimapp/v2/app.go
This makes the concern more significant as alpha versions can introduce breaking changes that could affect production stability. Consider:
- Pinning to the stable v1.3.4 version that is also referenced in the codebase
- Or if v2 features are required, document the rationale and ensure thorough testing of the alpha version
🔗 Analysis chain
Verify stability of alpha dependency.
Using github.com/cosmos/iavl/v2 v2.0.0-alpha.4 in tests could be risky as alpha versions may have breaking changes. Consider pinning to a stable version or documenting the rationale for using an alpha version.
Run this script to check for stability indicators:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for stability indicators of iavl/v2
# Check for open issues and PRs related to stability
gh api graphql -f query='
{
repository(owner: "cosmos", name: "iavl") {
issues(states: OPEN, labels: ["bug", "stability"]) {
totalCount
}
}
}'
Length of output: 214
Script:
#!/bin/bash
# Let's check the changelog and README for stability information
rg -i "stability|breaking change|alpha" -g "CHANGELOG*" -g "README*"
# Check if this alpha version is being used in other places
rg "github.com/cosmos/iavl/v2 v2.0.0-alpha.4" --type go
# Check if there are any newer versions available in go.sum
rg "github.com/cosmos/iavl/v2" go.sum
# Check if this is a test-only dependency
fd "go.mod" --exec grep -l "github.com/cosmos/iavl/v2"
Length of output: 9898
Script:
#!/bin/bash
# Let's check the specific version usage in other go.mod files
fd "go.mod" --exec cat {} \; | grep -A 2 -B 2 "github.com/cosmos/iavl/v2"
# Check if this is used in main application code or only in tests
rg -l "github.com/cosmos/iavl" --type go | grep -v "test"
Length of output: 1162
| dir := fmt.Sprintf("%s/data/iavl-v2/%s", opts.RootDir, key) | ||
| return iavlv2.NewTree(opts.Options.IavlV2Config, iavl_v2.SqliteDbOptions{Path: dir}, opts.Logger) |
There was a problem hiding this comment.
Handle directory creation and path validity checks.
When constructing the path fmt.Sprintf("%s/data/iavl-v2/%s", opts.RootDir, key), ensure that the directory exists or is created. Otherwise, concurrent usage might fail if the directory is missing.
dir := fmt.Sprintf("%s/data/iavl-v2/%s", opts.RootDir, key)
+// Ensure the directory is created or handle errors if it doesn't exist.
return iavlv2.NewTree(opts.Options.IavlV2Config, iavl_v2.SqliteDbOptions{Path: dir}, opts.Logger)Committable suggestion skipped: line range outside the PR's diff.
| @@ -23,7 +23,7 @@ type ( | |||
| // between a store name/key and the commit ID. | |||
| StoreInfo struct { | |||
| Name []byte | |||
| CommitID CommitID | |||
| CommitID *CommitID | |||
There was a problem hiding this comment.
Use pointer-based CommitID carefully.
Switching to CommitID *CommitID can make mutation side effects trickier to track. Confirm that no external references cause unexpected changes to the underlying commit state.
| func (ci *CommitInfo) GetStoreCommitID(storeKey []byte) *CommitID { | ||
| for _, si := range ci.StoreInfos { | ||
| if bytes.Equal(si.Name, storeKey) { | ||
| return si.CommitID | ||
| } | ||
| } | ||
| return CommitID{} | ||
| return &CommitID{} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return a meaningful default CommitID instead of an empty pointer.
In GetStoreCommitID, returning &CommitID{} might confuse callers expecting to see whether the store was actually found or not. Logically, consider returning nil and handling it upstream.
| storeInfos := make([]*proof.StoreInfo, 0, len(c.multiTrees)) | ||
| eg := new(errgroup.Group) | ||
| eg.SetLimit(store.MaxWriteParallelism) | ||
|
|
||
| for storeKey, tree := range c.multiTrees { | ||
| if internal.IsMemoryStoreKey(storeKey) { | ||
| continue | ||
| } | ||
| hash, cversion, err := tree.Commit() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if cversion != version { | ||
| return nil, fmt.Errorf("commit version %d does not match the target version %d", cversion, version) | ||
| } | ||
| commitID := proof.CommitID{ | ||
| Version: version, | ||
| Hash: hash, | ||
| si := &proof.StoreInfo{Name: []byte(storeKey)} | ||
| storeInfos = append(storeInfos, si) | ||
|
|
||
| if tree.IsConcurrentSafe() { | ||
| eg.Go(func() error { | ||
| err := c.commit(tree, si, version) | ||
| if err != nil { | ||
| return fmt.Errorf("commit fail: %s: %w", si.Name, err) | ||
| } | ||
| return nil | ||
| }) | ||
| } else { | ||
| err := c.commit(tree, si, version) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| storeInfos = append(storeInfos, proof.StoreInfo{ | ||
| Name: []byte(storeKey), | ||
| CommitID: commitID, | ||
| }) | ||
| } | ||
|
|
||
| cInfo := &proof.CommitInfo{ | ||
| Version: version, | ||
| StoreInfos: storeInfos, | ||
| } | ||
|
|
||
| if err := eg.Wait(); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Parallel commit approach.
Committing trees concurrently is ideal for performance, but confirm that you don’t need an all-or-nothing atomic commit. If partial errors occur, you may end up with inconsistent states between trees.
| serverCfg := cfg[serverName].(map[string]any) | ||
| cpuProfileFile, ok := serverCfg["cpu-profile"] | ||
| if !ok || cpuProfileFile == "" { |
There was a problem hiding this comment.
💡 Codebase verification
Add type assertion validation for server configuration
The review comment is correct. The code performs an unsafe type assertion cfg[serverName].(map[string]any) without validating the existence and type of the value. Looking at the codebase, other components like UnmarshalSubConfig properly validate configurations, but this particular code path lacks proper validation.
Add validation before the type assertion:
serverCfg, ok := cfg[serverName]
if !ok {
return fmt.Errorf("server configuration not found")
}
serverConfigMap, ok := serverCfg.(map[string]any)
if !ok {
return fmt.Errorf("invalid server configuration type: expected map[string]any")
}🔗 Analysis chain
Validate type assertions and existence checks before casting.
Accessing cfg[serverName] with a type cast to map[string]any can panic if serverName is missing or incorrectly typed. Consider checking for type safety explicitly to prevent potential panics.
Let me gather more information about the configuration structure and type safety checks.
Let me check how the configuration is validated before usage.
Let me check for any type assertion validations in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to serverName usage for potential type-safety checks.
rg -A 3 -B 3 'serverName'
Length of output: 1401
Script:
#!/bin/bash
# Check how cfg is used and validated in the codebase
ast-grep --pattern 'cfg[serverName]'
# Also check for any type assertions and validations on ConfigMap
rg -A 3 -B 3 'ConfigMap'
Length of output: 18593
Script:
#!/bin/bash
# Check for any validation of the server configuration
rg -A 5 'UnmarshalConfig|ValidateConfig' server/v2/
Length of output: 1949
Script:
#!/bin/bash
# Check for any type assertion validations in the server package
rg -A 3 'UnmarshalSubConfig|type assertion|, ok :=' server/v2/
Length of output: 13172
store/v2/root/factory.go
Outdated
| @@ -50,7 +54,7 @@ func DefaultStoreOptions() Options { | |||
| Interval: 100, | |||
| }, | |||
| IavlConfig: &iavl.Config{ | |||
| CacheSize: 100_000, | |||
| CacheSize: 1_000_000, | |||
There was a problem hiding this comment.
we may want to revert this, 1m could be a lot for memory. node operators can override to set higher
There was a problem hiding this comment.
cosmos-sdk/store/iavl/store.go
Lines 22 to 24 in eb3bf8b
maybe set to 500k, the default in store/v1?
store/v2/root/store.go
Outdated
| now := time.Now() | ||
| defer s.telemetry.MeasureSince(now, "root_store", "commit") | ||
| } | ||
| now := time.Now() |
There was a problem hiding this comment.
this will now get called even if metrics is off. should avoid this
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/build/build.mk (2)
112-112: Clear Documentation of theallTarget.The comment line is helpful for quickly understanding which tasks will be run by default. Consider adding a brief explanation or summary of the sub-targets included under
allto improve maintainability for new contributors.
130-133: Consider OverridingCCOnly When Needed.This logic forces
CCto useaarch64-linux-gnu-gccif building forarm64, otherwisegcc. While correct for most cross-compilation scenarios, it may override user-definedCCenvironment variables. Evaluate providing an escape hatch or explicit user control over the compiler choice for better flexibility.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(2 hunks)scripts/build/build.mk(2 hunks)
🔇 Additional comments (3)
.github/workflows/build.yml (3)
28-30: Installaarch64-gccOnly When Needed.The conditional installation step is good for cross-compilation. However, ensure that its presence doesn't introduce unnecessary overhead if the environment already has the relevant toolchain installed. Also consider caching to speed up repeated runs.
32-35: Conditional RocksDB Setup.Restricting these steps to
amd64avoids potential architecture-specific issues. It is a good approach. Ifarm64support for RocksDB becomes necessary in the future, ensure that these steps are revisited or extended to handle that scenario.
55-55: Build Matrix with Distinct Options.Including different build options (
v2,rocksdb,bls12381,secp) helps validate a wide range of configurations. This practice can detect potential incompatibilities early. Great approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
store/v2/commitment/iavlv2/tree.go (3)
27-38: Consider exposing node pool configurationWhile internalizing the node pool creation improves encapsulation, it might limit testability and configuration flexibility. Consider accepting node pool configuration options as parameters.
98-116: Enhance error handling with error wrappingConsider wrapping errors with additional context using
fmt.Errorf("get: %w", err)for better error tracing.
143-147: Document IAVL v1 compatibility requirementThe comment about IAVL v1's default behavior is crucial. Consider moving this to package documentation for better visibility.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
simapp/v2/app.go(4 hunks)store/v2/commitment/iavlv2/tree.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- simapp/v2/app.go
🧰 Additional context used
📓 Path-based instructions (1)
store/v2/commitment/iavlv2/tree.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
store/v2/commitment/iavlv2/tree.go (5)
22-24: LGTM: Enhanced Tree struct with logging capabilitiesThe addition of logging and path tracking fields improves operational visibility and debugging capabilities.
179-180: Document pruning configurationA past review raised questions about pruning configuration. While IAVL v2 handles pruning internally, consider documenting how node operators can configure pruning settings through
DefaultOptions.
186-188: LGTM: Concurrent safety implementationThe implementation aligns with the PR objective of supporting concurrent operations.
201-209: Validate DefaultOptions parametersConsider adding validation for
keepVersionsto ensure it's positive. Also, document the rationale behind the chosen default values, especially for:
- CheckpointInterval: 60
- EvictionDepth: 22
186-188: Verify concurrent safety implementationLet's verify that all necessary synchronization mechanisms are in place for concurrent operations.
| @@ -169,8 +169,7 @@ | |||
|
|
|||
| func (s *Store) Query(storeKey []byte, version uint64, key []byte, prove bool) (store.QueryResult, error) { | |||
| if s.telemetry != nil { | |||
| now := time.Now() | |||
| defer s.telemetry.MeasureSince(now, "root_store", "query") | |||
| defer s.telemetry.MeasureSince(time.Now(), "root_store", "query") | |||
Check warning
Code scanning / CodeQL
Calling the system time Warning
| @@ -196,8 +195,7 @@ | |||
|
|
|||
| func (s *Store) LoadLatestVersion() error { | |||
| if s.telemetry != nil { | |||
| now := time.Now() | |||
| defer s.telemetry.MeasureSince(now, "root_store", "load_latest_version") | |||
| defer s.telemetry.MeasureSince(time.Now(), "root_store", "load_latest_version") | |||
Check warning
Code scanning / CodeQL
Calling the system time Warning
| @@ -210,17 +208,15 @@ | |||
|
|
|||
| func (s *Store) LoadVersion(version uint64) error { | |||
| if s.telemetry != nil { | |||
| now := time.Now() | |||
| defer s.telemetry.MeasureSince(now, "root_store", "load_version") | |||
| defer s.telemetry.MeasureSince(time.Now(), "root_store", "load_version") | |||
Check warning
Code scanning / CodeQL
Calling the system time Warning
| } | ||
|
|
||
| return s.loadVersion(version, nil, false) | ||
| } | ||
|
|
||
| func (s *Store) LoadVersionForOverwriting(version uint64) error { | ||
| if s.telemetry != nil { | ||
| now := time.Now() | ||
| defer s.telemetry.MeasureSince(now, "root_store", "load_version_for_overwriting") | ||
| defer s.telemetry.MeasureSince(time.Now(), "root_store", "load_version_for_overwriting") |
Check warning
Code scanning / CodeQL
Calling the system time Warning
Description
Full integration of iavl/v2 into the SDK. iavl/v2 alpha release candidate pinned here.
The store/v2 changes add support for an additional method on commitment trees:
IsConcurrentSafe() bool. If this function returns true then store/v2 commit code paths execute concurrently to each other atMaxWriteParallelism = 8.Since iavl/v2 store keys can execute concurrently with respect to each other this is a large performance enhancement.
Note: The linting problems are unrelated, they also show up in #23132 (which is PR includes)
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
Based on the comprehensive summary, here are the updated release notes:
New Features
Performance Improvements
Dependency Updates
Bug Fixes
Testing
The release focuses on improving the underlying store and commitment infrastructure with enhanced performance, concurrency, and flexibility.