refactor(store/v2): simplify genesis flow #22435
Conversation
|
Warning Rate limit exceeded@kocubinski has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications across multiple files, primarily focusing on the addition of a Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:Store
C:server/v2
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
runtime/v2/go.mod (1)
Line range hint
3-3: Update Go version to a released version.The module currently specifies Go 1.23, which hasn't been released yet. This could cause compatibility issues.
Consider updating to the latest stable version:
-go 1.23 +go 1.21
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
runtime/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
runtime/v2/go.mod(1 hunks)
🔇 Additional comments (1)
runtime/v2/go.mod (1)
8-8: LGTM: Replace directive addition is appropriate.
The addition of cosmossdk.io/core => ../../core to the replace directives is consistent with the module's structure and aligns with similar changes in other modules.
julienrbrt
added
the
backport/v0.52.x
|
adding backport label for partial backport of cometbft server changes. |
I'm trying to but getting errors from CI different from my local like There is a replace directive in store/v2 so I'm confused. Edit: Looks like I need a build tag |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
store/v2/storage/storage_bench_test.go (1)
108-109: Consider using deterministic versioning for benchmarksWhile using
b.N + 1for versioning ensures unique versions, it might lead to non-deterministic behavior across benchmark runs sinceb.Ncan vary. Consider using a more predictable versioning scheme:-ver := uint64(b.N + 1) +ver := uint64(i + 1) // Use loop counter for deterministic versioningAlso applies to: 123-123
store/v2/root/store.go (2)
364-372: Consider enhancing error handling in errgroup usageThe current implementation might mask errors if multiple goroutines fail simultaneously. Consider adding context to errors and ensuring all errors are properly captured.
var cInfo *proof.CommitInfo eg.Go(func() error { - if err := s.stateCommitment.WriteChangeset(cs); err != nil { - return fmt.Errorf("failed to write batch to SC store: %w", err) - } - var scErr error - cInfo, scErr = s.stateCommitment.Commit(cs.Version) - if scErr != nil { - return fmt.Errorf("failed to commit SC store: %w", scErr) + if err := s.stateCommitment.WriteChangeset(cs); err != nil { + return fmt.Errorf("failed to write batch to SC store (version %d): %w", cs.Version, err) + } + var commitErr error + cInfo, commitErr = s.stateCommitment.Commit(cs.Version) + if commitErr != nil { + return fmt.Errorf("failed to commit SC store (version %d): %w", cs.Version, commitErr) } return nil })
Line range hint
423-447: Enhance error handling during migration cleanupThe cleanup process when migration is complete could be more robust by handling multiple potential failures.
if s.migrationManager.GetMigratedVersion() == s.lastCommitInfo.Version { close(s.chDone) close(s.chChangeset) s.isMigrating = false + var errs []error if err := s.stateCommitment.Close(); err != nil { - return fmt.Errorf("failed to close the old SC store: %w", err) + errs = append(errs, fmt.Errorf("failed to close the old SC store: %w", err)) } newStateCommitment := s.migrationManager.GetStateCommitment() if newStateCommitment != nil { s.stateCommitment = newStateCommitment } if err := s.migrationManager.Close(); err != nil { - return fmt.Errorf("failed to close migration manager: %w", err) + errs = append(errs, fmt.Errorf("failed to close migration manager: %w", err)) } + if len(errs) > 0 { + return errors.Join(errs...) + } s.logger.Info("migration completed", "version", s.lastCommitInfo.Version) }store/v2/commitment/store.go (1)
512-544: LGTM: Well-structured commit info retrieval with cachingThe implementation efficiently checks for cached commit info before building it from trees. The version validation and memory store handling are properly implemented.
Consider pre-allocating the storeInfos slice with the exact capacity after filtering out memory store keys to optimize memory allocation:
- storeInfos := make([]proof.StoreInfo, 0, len(c.multiTrees)) + capacity := 0 + for storeKey := range c.multiTrees { + if !internal.IsMemoryStoreKey(storeKey) { + capacity++ + } + } + storeInfos := make([]proof.StoreInfo, 0, capacity)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
store/v2/commitment/store.go(3 hunks)store/v2/commitment/store_bench_test.go(1 hunks)store/v2/root/store.go(5 hunks)store/v2/root/store_mock_test.go(0 hunks)store/v2/root/store_test.go(21 hunks)store/v2/storage/storage_bench_test.go(4 hunks)
💤 Files with no reviewable changes (1)
- store/v2/root/store_mock_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- store/v2/root/store_test.go
🧰 Additional context used
📓 Path-based instructions (4)
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/store_bench_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.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/storage/storage_bench_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (7)
store/v2/commitment/store_bench_test.go (2)
Line range hint 38-56: Test coverage looks comprehensive.
The benchmark setup provides good coverage of the versioned changeset functionality:
- Tests multiple DB backends
- Large dataset (1000 changesets × 100 KV pairs × 3 stores)
- Includes both commit and proof verification scenarios
38-39: LGTM! Proper type usage for version numbers.
The change from int to uint64 for the loop variable aligns well with the version parameter addition to NewChangeset. Using uint64 is appropriate for version numbers as they cannot be negative.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
Based on the search results, I can verify that the change from int to uint64 for the loop variable is consistent with the codebase's usage of version numbers. The search shows:
- The
NewChangesetfunction incore/store/changeset.goacceptsversion uint64 - Version parameters across the codebase consistently use
uint64type - All version-related operations in store, commitment, and database layers use
uint64
The type change is correct and follows the codebase conventions for version numbers.
The change ensures type consistency with the Version field in Changeset and aligns with the broader codebase's handling of version numbers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that NewChangeset is consistently called with version parameter
# and that version types are consistently uint64
# Check NewChangeset calls
ast-grep --pattern 'NewChangeset($_)'
# Check version type declarations
ast-grep --pattern 'version $type'
Length of output: 76
Script:
#!/bin/bash
# Let's try a different approach to find NewChangeset usage patterns
# Search for NewChangeset calls using ripgrep for better context
rg "NewChangeset" -A 2 -B 2
# Search for version-related type declarations and usages
rg "type.*Version|version.*uint64" -A 1 -B 1
Length of output: 215095
store/v2/storage/storage_bench_test.go (3)
72-72: LGTM: Correctly updated Changeset API usage
The changes properly integrate the new Changeset API by including the version in the Changeset creation and simplifying the ApplyChangeset call.
Also applies to: 77-77
156-156: LGTM: Consistent with the new Changeset API
The changes maintain consistency with the new Changeset API while preserving the original benchmark's iteration behavior.
Also applies to: 161-161
Line range hint 72-161: Test coverage assessment: Comprehensive
The benchmarks provide thorough coverage of the modified Changeset API across multiple storage backends, testing:
- Single key retrieval performance
- Batch changeset application
- Iterator functionality
- Error handling
The tests align well with the PR's objectives of simplifying the genesis flow and adding the Version field to Changeset.
store/v2/commitment/store.go (2)
165-168: LGTM: Proper error handling in version loading
The change appropriately uses GetCommitInfo with proper error handling, aligning with the PR's goal of simplifying the genesis flow.
182-191: LGTM: Enhanced version validation in commit process
The added version check ensures data consistency by validating that the tree's commit version matches the target version. The error message is clear and informative.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
server/v2/cometbft/internal/mock/mock_store.go (1)
Line range hint
62-72: Consider returning a meaningful hash instead of an empty byte array.The current implementation always returns an empty byte array as the hash. This could mask potential issues in tests since the hash should typically represent the state after commit.
Consider using the commit info hash:
func (s *MockStore) Commit(changeset *corestore.Changeset) (corestore.Hash, error) { err := s.Storage.ApplyChangeset(changeset) if err != nil { return []byte{}, err } err = s.Committer.WriteChangeset(changeset) if err != nil { return []byte{}, err } - _, err = s.Committer.Commit(changeset.Version) - return []byte{}, err + commitInfo, err := s.Committer.Commit(changeset.Version) + if err != nil { + return []byte{}, err + } + return commitInfo.Hash, nilcore/CHANGELOG.md (1)
42-42: Fix typo in PR reference.There's a typo in the PR reference:
[*22267]should be[#22267].-* [*22267](https://github.com/cosmos/cosmos-sdk/pull/22267) Add `server.ConfigMap` and `server.ModuleConfigMap` to replace `server.DynamicConfig` in module configuration. +* [#22267](https://github.com/cosmos/cosmos-sdk/pull/22267) Add `server.ConfigMap` and `server.ModuleConfigMap` to replace `server.DynamicConfig` in module configuration.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
core/CHANGELOG.md(1 hunks)server/v2/cometbft/abci_test.go(12 hunks)server/v2/cometbft/internal/mock/mock_store.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/abci_test.go
🧰 Additional context used
📓 Path-based instructions (2)
core/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
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.
🔇 Additional comments (2)
server/v2/cometbft/internal/mock/mock_store.go (1)
Line range hint 62-72: LGTM! Verify the commit behavior in integration tests.
The changes align with the PR objectives to simplify the genesis flow. The implementation correctly uses the version from the changeset and maintains proper error handling.
Let's verify that this mock implementation is being properly tested:
core/CHANGELOG.md (1)
39-41: LGTM! The changelog entry is well-formatted and accurate.
The entry correctly documents the API breaking change, following the changelog guidelines with proper PR reference and clear description of the changes.
Co-authored-by: Marko <[email protected]> (cherry picked from commit 43e28b4) # Conflicts: # core/CHANGELOG.md # core/store/changeset.go # runtime/v2/go.mod # runtime/v2/go.sum # server/v2/cometbft/go.mod # server/v2/cometbft/go.sum # server/v2/go.mod # server/v2/go.sum # server/v2/store/server.go # simapp/v2/go.mod # simapp/v2/go.sum # store/v2/commitment/iavl/tree.go # store/v2/commitment/mem/tree.go # store/v2/commitment/store.go # store/v2/commitment/store_bench_test.go # store/v2/commitment/store_test_suite.go # store/v2/commitment/tree.go # store/v2/database.go # store/v2/go.mod # store/v2/go.sum # store/v2/internal/encoding/changeset_test.go # store/v2/migration/manager.go # store/v2/migration/manager_test.go # store/v2/mock/db_mock.go # store/v2/pruning/manager.go # store/v2/pruning/manager_test.go # store/v2/root/builder.go # store/v2/root/migrate_test.go # store/v2/root/store.go # store/v2/root/store_mock_test.go # store/v2/root/store_test.go # store/v2/root/upgrade_test.go # store/v2/storage/storage_bench_test.go # store/v2/storage/storage_test_suite.go # store/v2/storage/store.go # store/v2/store.go # tests/go.mod # tests/go.sum # tests/integration/v2/app.go
…22494) Co-authored-by: Matt Kocubinski <[email protected]> Co-authored-by: Julien Robert <[email protected]>
* main: (31 commits) docs: update links for https security protocol (#22514) build(deps): Bump github.com/bytedance/sonic from 1.12.3 to 1.12.4 in /log (#22513) feat(x/protocolpool)!: allow any coins in continuous funds (#21916) docs: Update protobuf tx signing message format outer link (#22510) test(accounts): fix integration tests (#22418) chore(x): fix some typos in comment (#22508) build(deps): Bump cosmossdk.io/log from 1.4.1 to 1.5.0 (#22487) build(deps): Bump cosmossdk.io/core from 1.0.0-alpha.5 to 1.0.0-alpha.6 (#22502) build(deps): Bump golang.org/x/crypto from 0.28.0 to 0.29.0 (#22480) docs(adr75): server v2 (#21069) fix(server/v2): improve server stop (#22455) chore: prepare core changelog (#22495) refactor(store/v2): simplify genesis flow (#22435) build(deps): Bump google.golang.org/grpc from 1.67.1 to 1.68.0 (#22486) build(deps): Bump golang.org/x/sync from 0.8.0 to 0.9.0 (#22482) feat(x/circuit): Allow msg Reset with empty msgURL (#22459) build(deps): Bump actions/xxx-artifact from v3 to v4 (#22468) feat(stf/branch): simplify merged iterator (#22131) refactor(log): disable coloring in testing logger (#22466) chore(x/tx): update changelog to alpha.2 (#22465) ...
Description
Closes: #22302, #22469
Contains: #22436 ([email protected] upgrade)
Simplifies the genesis flow from server/v2 => store/v2 as a result of upgrading to [email protected]. After this change
WorkingHash()was unused so it was deleted.core
Versionfield tocore/store.Changeset.store/v2
WorkingHash(store.Changeset)from all store/v2 interfaces.store/v2.VersionedWriter.ApplyChangesetis now 1-arity,versionparameter removed (it's now in the Changeset)store/v2/root.Commit(store.Changeset)now passesversionto the commit store from the Changeset instead of fetching from SC on write.SetCommitHeaderfrom root store interface (unused)server/v2
InitChainnow callsCommit(store.Changeset)instead ofWorkingHash(store.Changeset)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
Release Notes
New Features
Versionfield in theChangesetstructure for better version tracking.Commitmethod to streamline state management during commits.Improvements
Changesetinitialization and application.Deprecations
WorkingHashmethod from several interfaces, simplifying state management.Documentation