refactor(store/v2)!: simplify storage #22683
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant enhancements to error handling and state management across various components in the codebase. Key modifications include updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:Store
C:server/v2
julienrbrt
changed the title
julienrbrt
added
the
backport/v0.52.x
tac0turtle
force-pushed
the
marko/SS_removal
branch
from
12c021d to
3ae3676
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (14)
store/v2/commitment/store_test_suite.go (2)
Line range hint
34-139: Consider splittingTestStore_Snapshotterinto smaller helper functions for better readabilityThe
TestStore_Snapshotterfunction is quite lengthy and covers multiple distinct steps. Refactoring it into smaller, well-named helper functions for setup, snapshot creation, restoration, and verification can improve readability and maintainability.
Line range hint
115-121: Avoid using test assertions inside goroutinesUsing
s.Require().NoError(err)inside a goroutine will not properly report failures to the test runner because thetesting.Tmethods are not goroutine-safe. Assertions should be performed in the main test goroutine to ensure that test failures are correctly captured.Consider collecting errors in the goroutine and sending them through a channel back to the main goroutine for proper assertion.
store/v2/mock/types.go (1)
11-11: Clarify the responsibilities ofStateCommitterinterfaceThe
StateCommitterinterface now embedsstore.VersionedReaderandstore.UpgradableDatabase, which are related to reading and upgrading capabilities. Including these in theCommitterinterface may blur its responsibilities. Consider separating read, write, and upgrade functionalities into distinct interfaces to adhere to the Single Responsibility Principle.store/v2/pruning/manager.go (1)
Line range hint
26-27: Update the function comment forPruneto reflect current functionalityThe comment for
Prunementions pruning both SC and SS, but the method now only handles SC after the removal of SS pruning logic. Please update the comment to accurately describe the method's current behavior.server/v2/cometbft/internal/mock/mock_reader.go (1)
Line range hint
42-64: Rename receiverroatomrto improve clarityThe receiver name
roain theMockReadermethods is not immediately clear. Renaming it tomr, which stands forMockReader, would improve readability and conform to Go naming conventions for receiver names.store/v2/snapshots/snapshotter.go (1)
16-16: Ensure consistency in method comments after signature changesThe
Restoremethod signature inCommitSnapshotterhas changed by removing thechStorageparameter. Please review the method's comment to ensure it accurately reflects the updated signature and no longer references the removed parameter.server/v2/cometbft/internal/mock/mock_store.go (1)
Line range hint
53-58: Consider enhancing error handling in Commit methodWhile the error handling is present, consider wrapping the errors with additional context to aid in debugging.
func (s *MockStore) Commit(changeset *corestore.Changeset) (corestore.Hash, error) { - err := s.Committer.WriteChangeset(changeset) + if err := s.Committer.WriteChangeset(changeset); err != nil { + return []byte{}, fmt.Errorf("failed to write changeset: %w", err) + } - _, err = s.Committer.Commit(changeset.Version) - return []byte{}, err + if _, err := s.Committer.Commit(changeset.Version); err != nil { + return []byte{}, fmt.Errorf("failed to commit version %d: %w", changeset.Version, err) + } + return []byte{}, nil }store/v2/root/store_mock_test.go (1)
48-70: Enhance test readability with descriptive test casesWhile the test coverage is good, consider using subtests to improve organization and readability.
func TestQuery(t *testing.T) { ctrl := gomock.NewController(t) sc := mock.NewMockStateCommitter(ctrl) rs := newTestRootStore(sc) - // Query without Proof - sc.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("error")) - _, err := rs.Query(nil, 0, nil, false) - require.Error(t, err) + t.Run("query without proof - error case", func(t *testing.T) { + sc.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("mock error")) + _, err := rs.Query(nil, 0, nil, false) + require.Error(t, err) + })store/v2/root/migrate_test.go (1)
Line range hint
111-117: Consider increasing migration simulation delayThe 100ms sleep duration might be too short for reliable migration testing in CI environments with varying load conditions.
- time.Sleep(100 * time.Millisecond) + time.Sleep(500 * time.Millisecond)store/v2/commitment/iavl/tree.go (1)
106-114: Consider extracting the version check logic.The version checking logic is duplicated across three methods. Consider extracting it into a helper method to improve maintainability.
+func (t *IavlTree) shouldUseMutableTree() (bool, error) { + lv, err := t.tree.GetLatestVersion() + if err != nil { + return false, err + } + return lv == 0, nil +}store/v2/migration/manager_test.go (2)
73-73: Remove debug print statement.The
fmt.Println(1)appears to be a debug statement that should be removed.-fmt.Println(1)
150-168: Consider removing redundant nil check.The nil check on
m.stateCommitmentis unnecessary as thesetupMigrationManagerfunction always initializes it.-if m.stateCommitment != nil { // check the migrated state for version := uint64(1); version < toVersion; version++ { // ... existing code ... } -}store/v2/pruning/manager_test.go (1)
Line range hint
67-82: Consider parameterizing timeout values.The timeout values (10 seconds) and polling interval (1 second) are hardcoded. Consider making these configurable or constants at the package level for easier maintenance.
+const ( + pruneTimeout = 10 * time.Second + pruneCheckPeriod = 1 * time.Second +) + // wait for the pruning to finish in the commitment store checkSCPrune := func() bool { // ... existing code ... } -s.Require().Eventually(checkSCPrune, 10*time.Second, 1*time.Second) +s.Require().Eventually(checkSCPrune, pruneTimeout, pruneCheckPeriod)store/v2/snapshots/helpers_test.go (1)
230-233: Consider implementing hung snapshotter restoreThe panic("not implemented") could be replaced with a proper implementation for completeness in testing.
- panic("not implemented") + return snapshotstypes.SnapshotItem{}, errors.New("restore not supported in hung snapshotter")
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (53)
crypto/keys/bls12_381/key_cgo.go(1 hunks)runtime/v2/builder.go(1 hunks)server/v2/cometbft/abci_test.go(3 hunks)server/v2/cometbft/internal/mock/mock_reader.go(3 hunks)server/v2/cometbft/internal/mock/mock_store.go(2 hunks)server/v2/cometbft/server.go(0 hunks)server/v2/stf/branch/bench_test.go(1 hunks)server/v2/store/snapshot.go(1 hunks)store/iavl/store_test.go(0 hunks)store/v2/commitment/iavl/tree.go(3 hunks)store/v2/commitment/store.go(3 hunks)store/v2/commitment/store_test_suite.go(3 hunks)store/v2/database.go(1 hunks)store/v2/migration/README.md(1 hunks)store/v2/migration/manager.go(7 hunks)store/v2/migration/manager_test.go(3 hunks)store/v2/mock/db_mock.go(4 hunks)store/v2/mock/types.go(1 hunks)store/v2/pruning/manager.go(1 hunks)store/v2/pruning/manager_test.go(1 hunks)store/v2/root/factory.go(2 hunks)store/v2/root/migrate_test.go(2 hunks)store/v2/root/store.go(3 hunks)store/v2/root/store_mock_test.go(3 hunks)store/v2/root/store_test.go(6 hunks)store/v2/root/upgrade_test.go(3 hunks)store/v2/snapshots/helpers_test.go(4 hunks)store/v2/snapshots/manager.go(2 hunks)store/v2/snapshots/manager_test.go(10 hunks)store/v2/snapshots/snapshotter.go(1 hunks)store/v2/storage/README.md(0 hunks)store/v2/storage/database.go(0 hunks)store/v2/storage/pebbledb/batch.go(0 hunks)store/v2/storage/pebbledb/comparator.go(0 hunks)store/v2/storage/pebbledb/comparator_test.go(0 hunks)store/v2/storage/pebbledb/db.go(0 hunks)store/v2/storage/pebbledb/db_test.go(0 hunks)store/v2/storage/pebbledb/iterator.go(0 hunks)store/v2/storage/rocksdb/batch.go(0 hunks)store/v2/storage/rocksdb/comparator.go(0 hunks)store/v2/storage/rocksdb/db.go(0 hunks)store/v2/storage/rocksdb/db_noflag.go(0 hunks)store/v2/storage/rocksdb/db_test.go(0 hunks)store/v2/storage/rocksdb/iterator.go(0 hunks)store/v2/storage/rocksdb/opts.go(0 hunks)store/v2/storage/storage_bench_test.go(0 hunks)store/v2/storage/storage_test_suite.go(0 hunks)store/v2/storage/store.go(0 hunks)store/v2/storage/util/iterator.go(0 hunks)store/v2/store.go(0 hunks)tests/integration/accounts/base_account_test.go(2 hunks)tests/integration/accounts/bundler_test.go(1 hunks)tests/integration/v2/auth/app_test.go(0 hunks)
💤 Files with no reviewable changes (23)
- store/v2/store.go
- store/v2/storage/pebbledb/db_test.go
- tests/integration/v2/auth/app_test.go
- store/v2/storage/rocksdb/db_test.go
- store/v2/storage/pebbledb/comparator_test.go
- store/v2/storage/README.md
- store/v2/storage/storage_bench_test.go
- store/iavl/store_test.go
- store/v2/storage/util/iterator.go
- store/v2/storage/rocksdb/iterator.go
- store/v2/storage/pebbledb/iterator.go
- store/v2/storage/rocksdb/comparator.go
- store/v2/storage/rocksdb/batch.go
- store/v2/storage/database.go
- store/v2/storage/rocksdb/opts.go
- store/v2/storage/pebbledb/batch.go
- store/v2/storage/rocksdb/db_noflag.go
- server/v2/cometbft/server.go
- store/v2/storage/store.go
- store/v2/storage/storage_test_suite.go
- store/v2/storage/pebbledb/db.go
- store/v2/storage/rocksdb/db.go
- store/v2/storage/pebbledb/comparator.go
✅ Files skipped from review due to trivial changes (3)
- store/v2/migration/README.md
- server/v2/stf/branch/bench_test.go
- tests/integration/accounts/base_account_test.go
🧰 Additional context used
📓 Path-based instructions (27)
server/v2/cometbft/internal/mock/mock_reader.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/commitment/iavl/tree.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/mock/types.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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/database.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/accounts/bundler_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
store/v2/migration/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/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/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/root/store_mock_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"
runtime/v2/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
crypto/keys/bls12_381/key_cgo.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/root/factory.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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"
server/v2/cometbft/abci_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/pruning/manager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/snapshots/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"
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/snapshots/snapshotter.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/snapshots/manager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/root/store_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
store/v2/root/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/snapshots/helpers_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/migration/manager.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.
server/v2/store/snapshot.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/mock/db_mock.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (41)
store/v2/migration/manager.go (1)
41-41: Good use of atomic.Uint64 for concurrent version management
Replacing the mutex lock with atomic.Uint64 for the migratedVersion field enhances concurrency control by reducing lock contention and simplifying the code.
Also applies to: 108-108, 152-152, 195-195
store/v2/database.go (1)
31-32: LGTM: Clean interface composition
The addition of UpgradeableStore to the Committer interface is a logical composition that combines version management with upgradeability capabilities.
Let's verify the implementation of this interface across the codebase:
✅ Verification successful
The previous pattern might have failed due to syntax issues or the specific import path. Let's gather more context about the interfaces and their implementations.
The output shows there are implementations in commitment/store.go and mock implementations. Let's verify the actual implementation details to ensure the interface composition is properly handled.
Interface composition is correctly implemented
The verification confirms that the interface composition is properly implemented:
CommitStoreexplicitly implements all required interfaces includingUpgradeableStoreandVersionedReader- The implementation includes all necessary methods:
LoadVersionAndUpgradeforUpgradeableStoreWriteChangesetfor state changes- Version management methods
- The composition follows Go best practices with clear interface segregation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for structs implementing the Committer interface
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) WriteChangeset($_ *corestore.Changeset) error {
$$$
}'
Length of output: 117
Script:
#!/bin/bash
# Let's try a different approach to find implementations
# First, let's find all files that mention Committer
rg -l "Committer"
# Then search for structs implementing UpgradeableStore
rg -l "UpgradeableStore"
# And look for actual implementations of WriteChangeset
rg "func.*WriteChangeset"
Length of output: 1151
Script:
#!/bin/bash
# Let's examine the CommitStore implementation to verify it properly implements all required interfaces
cat store/v2/commitment/store.go
# Also check the interface definition in store.go
cat store/v2/store.go
Length of output: 19487
server/v2/cometbft/internal/mock/mock_store.go (1)
30-31: LGTM: Simplified mock store constructor
Constructor has been simplified to only require the essential Committer dependency, aligning with the storage simplification objective.
store/v2/root/store_mock_test.go (1)
18-25: LGTM: Simplified test store setup
The test store setup has been properly simplified to align with the storage refactoring changes.
store/v2/root/upgrade_test.go (3)
46-47: LGTM: Storage simplification changes align with PR objectives
The removal of storage store and simplified pruning manager initialization aligns with the PR's goal of eliminating state storage dependency.
88-89: LGTM: Consistent storage removal pattern
The changes maintain consistency with the storage simplification pattern seen in SetupTest.
109-109: Verify test coverage for store3
The oldStoreKeys slice includes "store3" which is marked for deletion in upgrades. This might affect test coverage as subsequent queries would fail for this store.
✅ Verification successful
Test coverage for store3 is properly handled
The codebase shows comprehensive test coverage for store3's deletion scenario:
- In
store/v2/root/upgrade_test.go, store3 is explicitly marked for deletion in upgrades and its behavior is tested - Multiple test files (
migrate_test.go,store_test.go) verify the store deletion logic - Tests specifically check that:
- Store3 is properly unmounted after deletion
- Data is properly cleaned up
- Store upgrades handle the deletion correctly
- Invalid scenarios (like duplicate deletions) are caught
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if store3 is properly handled in test scenarios across the codebase
# Check for store3 usage in test files
rg -l "store3" --type go --type test
Length of output: 65
Script:
#!/bin/bash
# Search for store3 in Go test files
rg "store3" --type go -g "*_test.go" -C 3
# Also check for any test setup or teardown related to store3
ast-grep --pattern 'func Test$_($_) {
$$$
"store3"
$$$
}'
Length of output: 12572
crypto/keys/bls12_381/key_cgo.go (2)
12-12: LGTM: Import reorganization
The tmhash import reorganization maintains clean code structure.
Line range hint 78-86: Critical: Verify security implications of removed message length checks
The removal of message length validation and conditional hashing for large messages could have security implications:
- Memory consumption for large messages
- Potential DoS vectors
- Compatibility with existing signatures
Consider maintaining message size validation or documenting the security considerations.
✅ Verification successful
The removal of message length validation is safe in this context
The comment about SHA256 hashing for large messages appears to be outdated documentation. The actual BLS12-381 implementation delegates message handling directly to the underlying bls12381 package from CometBFT, which has its own secure message handling:
- The signature verification enforces strict length checks for signatures (
SignatureLength) - The private and public key operations enforce strict size validations (
PrivKeySize,PubKeySize) - The underlying CometBFT BLS implementation handles message processing securely
The removed code was redundant with the security measures already present in the dependency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for message length validation across the codebase
# Search for message length validations in crypto packages
rg -A 3 "MaxMsgLen|message.*length|msg.*length" --type go ./crypto
# Look for potential security implications in tests
rg -A 3 "TestSign.*Large|TestVerify.*Large" --type go --type test ./crypto
Length of output: 1338
Script:
#!/bin/bash
# Let's check the implementation details and security measures in the BLS signing
# Check the actual implementation in bls12381 package
fd "bls12381" --exec cat {}
# Check for any size-related constants or validations
rg -A 3 "const.*Max|func.*Sign" --type go ./crypto
Length of output: 24617
Script:
#!/bin/bash
# Let's check the BLS12-381 implementation details and any size-related constants
# Search for BLS12-381 specific constants and message handling
rg -A 3 "bls12381|SignatureLength|MaxMsgLen" --type go ./crypto
# Check for any hash-related code in BLS implementation
rg -A 3 "SHA256|Hash|Digest" --type go ./crypto/keys/bls12_381/
Length of output: 6784
store/v2/root/migrate_test.go (2)
71-73: LGTM: Simplified store initialization
The removal of storage store from snapshot and migration managers aligns with the PR objectives.
76-76: LGTM: Consistent storage removal
The root store initialization properly reflects the storage simplification changes.
store/v2/commitment/iavl/tree.go (2)
86-94: LGTM! Well-documented edge case handling.
The implementation correctly handles the case where the mutable tree is empty at genesis or when a key has been removed. The comments clearly explain the rationale.
126-134: Verify iterator behavior at genesis.
The implementation assumes that the iterator behavior is consistent between mutable and immutable trees at genesis. This assumption should be verified.
store/v2/pruning/manager_test.go (1)
45-46: LGTM! Clear pruning option setup.
The pruning option is clearly configured to prune all versions with a keep-recent of 0 and keep-every of 1.
runtime/v2/builder.go (1)
137-137: LGTM: Proper initialization of genesis JSON map
The addition of explicit map initialization improves code clarity and follows Go best practices for JSON handling.
store/v2/snapshots/helpers_test.go (3)
Line range hint 111-139: LGTM: Simplified Restore method signature
The removal of the chStorage channel parameter simplifies the interface while maintaining proper error handling and validation logic.
166-169: LGTM: Consistent error handling in mock implementation
The mock error implementation maintains consistent error handling patterns with the simplified interface.
186-186: LGTM: Updated manager initialization
The setupBusyManager function correctly adapts to the simplified snapshot interface.
tests/integration/accounts/bundler_test.go (1)
212-212: LGTM: Added test helper annotation
The addition of t.Helper() improves test failure reporting by ensuring errors are reported at the correct call site.
server/v2/store/snapshot.go (1)
378-382: LGTM: Simplified snapshot manager initialization
The changes correctly align with the removal of the storage snapshotter functionality, maintaining only the essential parameters for snapshot management.
store/v2/snapshots/manager_test.go (1)
21-21: LGTM: Consistent test adaptation
All test cases have been properly updated to reflect the removal of the storage snapshotter, maintaining test coverage while simplifying the manager initialization.
Also applies to: 42-42, 74-74, 113-113, 133-133, 252-252, 272-272, 358-358, 437-437, 509-509
store/v2/snapshots/manager.go (2)
76-86: LGTM: Clean manager initialization refactor
The NewManager function has been correctly simplified by removing the storage snapshotter dependency while maintaining all essential functionality.
397-397: Verify restoration behavior
The restoration logic has been simplified to use only the commit snapshotter. Please ensure all restoration scenarios are covered in the test suite.
✅ Verification successful
Let me gather more information about the test coverage by examining the test implementations.
Let me check the actual implementation of the restoration logic to compare with the test coverage.
Let me check the commit snapshotter's restore implementation and its test coverage.
Based on the test files examined, I can now provide a final response:
Restoration test coverage appears comprehensive
The test suite includes multiple test scenarios for restoration:
- Basic restoration flow in
TestManager_Restore - End-to-end snapshot and restore cycle in
TestSnapshot_Take_Restore - Multiple test cases for error conditions including:
- Invalid format handling
- Chunk validation
- Restoration state management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for restoration scenarios
# Test: Search for restoration test cases
rg -A 5 "TestManager_Restore|TestSnapshot_Take_Restore" "store/v2/snapshots/manager_test.go"
Length of output: 504
Script:
#!/bin/bash
# Check the implementation of TestManager_Restore and TestSnapshot_Take_Restore
rg -B 5 -A 20 "func TestManager_Restore|func TestSnapshot_Take_Restore" "store/v2/snapshots/manager_test.go"
# Look for other related test cases
rg "Restore.*\(" "store/v2/snapshots/manager_test.go"
Length of output: 2159
Script:
#!/bin/bash
# Check the implementation of Restore method in manager.go
rg -B 5 -A 15 "func.*Restore" "store/v2/snapshots/manager.go"
# Look for any error handling or validation in the restoration path
rg "return.*err.*Restore" "store/v2/snapshots/manager.go"
Length of output: 2863
Script:
#!/bin/bash
# Check the commit snapshotter's restore implementation
ast-grep --pattern 'type CommitSnapshotter struct {
$$$
}
func ($_ *CommitSnapshotter) Restore($$$) {
$$$
}'
# Look for commit snapshotter tests
rg -B 2 -A 10 "TestCommitSnapshotter.*Restore" .
Length of output: 166
store/v2/root/store_test.go (3)
54-55: Simplified store initialization looks good
The removal of the storage store parameter from the pruning manager and store initialization aligns with the PR objective of simplifying storage management.
Also applies to: 74-74, 76-76
82-82: Streamlined store mounting interface
The simplified store mounting interface removes unnecessary storage store dependency, making the code cleaner and more maintainable.
Also applies to: 85-85
597-597: Consistent pruning manager initialization
The changes maintain consistency in pruning manager initialization across different test scenarios, properly removing the storage store dependency.
Also applies to: 599-599, 623-623, 625-625, 676-676, 678-678, 763-763, 765-765
store/v2/mock/db_mock.go (9)
133-140: 'Has' method added to MockStateCommitter
The Has method has been correctly implemented in MockStateCommitter, conforming to the expected interface and following GoMock conventions.
142-146: Recorder for 'Has' method is properly implemented
The mock recorder for the Has method is correctly set up, ensuring that expected calls can be recorded during tests.
148-155: 'Iterator' method added to MockStateCommitter
The Iterator method has been appropriately added, following the standard pattern for GoMock-generated methods.
157-161: Recorder for 'Iterator' method is correctly implemented
The recorder for the Iterator method is properly configured, enabling accurate expectation setting in tests.
232-242: Updated 'PruneStoreKeys' method reflects interface changes
The PruneStoreKeys method has been updated in MockStateCommitter to match interface changes. Parameters are correctly typed, and the method implementation follows the GoMock pattern.
Line range hint 246-257: 'ReverseIterator' method implemented appropriately
The ReverseIterator method is added with correct parameters and return types, adhering to the expected interface and code generation standards.
260-271: 'SetInitialVersion' method reintroduced correctly
The SetInitialVersion method has been reinstated in MockStateCommitter, ensuring proper versioning strategies in the mock implementation.
Line range hint 275-287: 'VersionExists' method added to mock interface
The VersionExists method is correctly implemented in both the mock and its recorder, aligning with the interface requirements.
289-300: 'WriteChangeset' method added to MockStateCommitter
The addition of the WriteChangeset method is appropriate and follows the standard GoMock patterns, ensuring that changeset writing operations can be accurately tested.
store/v2/root/store.go (4)
109-114: Simplified 'getVersionedReader' uses only 'stateCommitment'
The getVersionedReader method now exclusively checks stateCommitment for version existence, removing reliance on stateStorage. This simplification improves readability and aligns with the intended architecture after removal of stateStorage.
178-180: Updated 'Query' method interacts with 'stateCommitment' directly
By querying stateCommitment directly, the Query method streamlines its operation. Error handling is appropriate and messages are clear, following best practices.
269-269: 'LoadVersionAndUpgrade' refactored to use 'stateCommitment'
The method now directly calls LoadVersionAndUpgrade on stateCommitment, simplifying the upgrade process and ensuring consistency.
Line range hint 594-594: Removed dependency on 'stateStorage' in migration logic
The migration logic now solely depends on stateCommitment, reflecting the removal of stateStorage. This change aligns with the overall simplification objective of the PR.
server/v2/cometbft/abci_test.go (2)
Line range hint 788-794: 'TestOptimisticExecution' validates error handling correctly
The test properly checks the behavior when an error occurs during FinalizeBlock, ensuring that the optimistic execution path handles errors as expected.
Line range hint 594-602: Test updated to use 'WriteChangeset'; verify coverage
The test now uses WriteChangeset to write data to the state. Ensure that this change accurately tests the behavior of WriteChangeset, and consider adding tests for edge cases or error handling scenarios to enhance coverage.
As a follow-up, consider verifying the usage of WriteChangeset across tests:
| sc *commitment.CommitStore | ||
| err error |
There was a problem hiding this comment.
Critical Issue: Uninitialized sc variable leading to potential nil pointer dereference
The variable sc is declared but not initialized before being used in pruning.NewManager(sc, storeOpts.SCPruningOption) (line 129) and return New(..., sc, pm, nil, nil) (line 130). This will result in a nil pointer dereference at runtime.
To fix this issue, ensure that sc is properly initialized before use. You can initialize sc after setting up trees and oldTrees:
+ sc, err = commitment.NewCommitStore(trees, oldTrees, opts.SCRawDB, opts.Logger)
+ if err != nil {
+ return nil, err
+ }Insert this code before line 129 to properly initialize sc.
Also applies to: 129-130
store/v2/commitment/store.go
Outdated
| latestVersion, err := c.metadata.GetLatestVersion() | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if latestVersion == 0 { | ||
| return true, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential Logic Error in VersionExists Method
In the VersionExists method, when latestVersion == 0, the method returns true regardless of the input version. This could incorrectly indicate that any version exists when, in fact, no versions may have been committed yet.
Consider adjusting the logic to accurately reflect the existence of the specified version:
if latestVersion == 0 {
- return true, nil
+ return version == 0, nil
}This change ensures that the method only returns true when checking for version 0 when no versions have been committed, aligning with expected behavior.
📝 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.
| latestVersion, err := c.metadata.GetLatestVersion() | |
| if err != nil { | |
| return false, err | |
| } | |
| if latestVersion == 0 { | |
| return true, nil | |
| } | |
| latestVersion, err := c.metadata.GetLatestVersion() | |
| if err != nil { | |
| return false, err | |
| } | |
| if latestVersion == 0 { | |
| return version == 0, nil | |
| } | |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (12)
server/v2/store/snapshot.go (2)
Line range hint
172-174: Remove redundant file closure to prevent potential errorsIn the
DumpArchiveCmdfunction,fpis already deferred for closure after opening. Explicitly callingfp.Close()again at the end is unnecessary and could potentially cause issues if an error occurs during the deferred closure.Apply this diff to remove the redundant
fp.Close()call:... if err := gzipWriter.Close(); err != nil { return fmt.Errorf("failed to close gzip writer: %w", err) } - return fp.Close() + return nil
Line range hint
229-234: Add missing file closure to prevent resource leakIn the
LoadArchiveCmdfunction, the archive filefpis opened but not closed. This can lead to a file descriptor leak.Apply this diff to ensure the file is properly closed:
fp, err := os.Open(path) if err != nil { return fmt.Errorf("failed to open archive file: %w", err) } + defer fp.Close() reader, err := gzip.NewReader(fp) if err != nil { return fmt.Errorf("failed to create gzip reader: %w", err) }store/v2/root/store.go (2)
269-269: Correct the error message to reflectstateCommitmentinstead ofSSThe error message in
LoadVersionAndUpgradereferencesSS(state storage), but the context now usesstateCommitment. This may cause confusion during debugging.Apply this diff to update the error message:
if err := s.stateCommitment.LoadVersionAndUpgrade(v, upgrades); err != nil { - return fmt.Errorf("failed to load SS version with upgrades %d: %w", v, err) + return fmt.Errorf("failed to load SC version with upgrades %d: %w", v, err) }
Line range hint
378-382: Prevent panic by handling unchecked type assertionIn the
createSnapshotsManagerfunction, an unchecked type assertion is used, which can cause a panic ifstateCommitmentdoes not implementsnapshots.CommitSnapshotter.Apply this diff to safely handle the type assertion:
+ sc, ok := store.GetStateCommitment().(snapshots.CommitSnapshotter) + if !ok { + return nil, fmt.Errorf("stateCommitment does not implement snapshots.CommitSnapshotter") + } sm := snapshots.NewManager( snapshotStore, snapshots.NewSnapshotOptions(interval, uint32(keepRecent)), - store.GetStateCommitment().(snapshots.CommitSnapshotter), + sc, nil, logger)store/v2/commitment/store_test_suite.go (1)
34-67: Update function comment to follow Go style guidelinesThe comment for
TestStore_Snapshottershould start with the function name and be a complete sentence, as per Go conventions. This ensures consistency and clarity in documentation.Apply this diff to correct the function comment:
-// TestStore_Snapshotter tests the snapshot functionality of the CommitStore. -// This test verifies that the store can correctly create snapshots and restore from them. -// The test follows these steps: -// -// 1. Setup & Data Population: -// - Creates a new CommitStore with two stores (store1 and store2) -// - Writes 10 versions of data (version 1-10) -// - For each version, writes 10 key-value pairs to each store -// - Total data: 2 stores * 10 versions * 10 pairs = 200 key-value pairs -// - Keys are formatted as "key-{version}-{index}" -// - Values are formatted as "value-{version}-{index}" -// - Each version is committed to get a CommitInfo -// -// 2. Snapshot Creation: -// - Creates a dummy extension item for metadata testing -// - Sets up a new target store for restoration -// - Creates a channel for snapshot chunks -// - Launches a goroutine to: -// - Create a snapshot writer -// - Take a snapshot at version 10 -// - Write extension metadata -// -// 3. Snapshot Restoration: -// - Creates a snapshot reader from the chunks -// - Sets up a channel for state changes during restoration -// - Launches a goroutine to collect restored key-value pairs -// - Restores the snapshot into the target store -// - Verifies the extension metadata was preserved -// -// 4. Verification: -// - Confirms all 200 key-value pairs were restored correctly -// - Verifies the format: "{storeKey}_key-{version}-{index}" -> "value-{version}-{index}" -// - Checks that the restored store's Merkle tree hashes match the original -// - Ensures store integrity by comparing CommitInfo hashes +// TestStore_Snapshotter tests the snapshot functionality of the CommitStore by verifying that the store can correctly create snapshots and restore from them. The test follows these steps: +// +// 1. **Setup & Data Population**: +// - Creates a new CommitStore with two stores (`store1` and `store2`). +// - Writes 10 versions of data (version 1-10). +// - For each version, writes 10 key-value pairs to each store. +// - Total data: 2 stores * 10 versions * 10 pairs = 200 key-value pairs. +// - Keys are formatted as `"key-{version}-{index}"`. +// - Values are formatted as `"value-{version}-{index}"`. +// - Each version is committed to get a `CommitInfo`. +// +// 2. **Snapshot Creation**: +// - Creates a dummy extension item for metadata testing. +// - Sets up a new target store for restoration. +// - Launches a goroutine to: +// - Create a snapshot writer. +// - Take a snapshot at version 10. +// - Write extension metadata. +// +// 3. **Snapshot Restoration**: +// - Creates a snapshot reader from the chunks. +// - Restores the snapshot into the target store. +// - Verifies the extension metadata was preserved. +// +// 4. **Verification**: +// - Confirms all 200 key-value pairs were restored correctly. +// - Verifies the format: `"{storeKey}_key-{version}-{index}"` → `"value-{version}-{index}"`. +// - Checks that the restored store's Merkle tree hashes match the original. +// - Ensures store integrity by comparing `CommitInfo` hashes.server/v2/cometbft/internal/mock/mock_store.go (1)
Line range hint
53-58: Improve error handling in Commit methodThe method currently discards the commit hash and only returns an empty byte slice. Consider returning the actual commit hash or documenting why it's intentionally discarded.
- return []byte{}, err + commitInfo, err := s.Committer.Commit(changeset.Version) + if err != nil { + return nil, err + } + return commitInfo.Hash, nilstore/v2/root/store_mock_test.go (1)
48-70: Enhance Query test coverageConsider improving the test cases:
- Add table-driven tests for different query scenarios
- Test edge cases (empty keys, large values)
- Add concurrent query tests
func TestQuery(t *testing.T) { testCases := []struct { name string key []byte version uint64 prove bool setup func(*mock.MockStateCommitter) wantErr bool }{ // Add test cases here } // Test implementation }store/v2/root/upgrade_test.go (1)
46-47: Ensure consistent nil parameter handling in testsThe pattern of passing nil parameters is consistent with the storage simplification changes. However, we should add test cases to verify behavior with nil parameters.
Consider adding test cases that explicitly verify the behavior of the store with nil parameters to ensure robustness.
Also applies to: 88-89
store/v2/commitment/iavl/tree.go (1)
126-134: LGTM! Consider caching the latest version.The implementation is consistent with GetProof and Get methods. However, since the latest version check is now used in multiple methods, consider caching it to avoid repeated calls.
type IavlTree struct { tree *iavl.MutableTree + latestVersion atomic.Value // cache for latest version }store/v2/migration/manager_test.go (1)
73-73: Remove debug print statement.The fmt.Println(1) appears to be a leftover debug statement and should be removed.
-fmt.Println(1)store/v2/snapshots/manager.go (1)
76-85: Consider documenting migration pathThe removal of
storageSnapshotteris a breaking change. Consider adding documentation to help users migrate from the old implementation to the new simplified version.server/v2/cometbft/abci_test.go (1)
788-788: Consider enhancing error assertion in optimistic execution test.While the current error check is functional, consider using
require.ErrorIsfor more precise error matching, especially when testing specific error conditions.-require.Nil(t, fbResp) -require.Error(t, err) -require.ErrorContains(t, err, "test error") +require.Nil(t, fbResp) +expectedErr := errors.New("test error") +require.ErrorIs(t, err, expectedErr)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (53)
crypto/keys/bls12_381/key_cgo.go(1 hunks)runtime/v2/builder.go(1 hunks)server/v2/cometbft/abci_test.go(3 hunks)server/v2/cometbft/internal/mock/mock_reader.go(3 hunks)server/v2/cometbft/internal/mock/mock_store.go(2 hunks)server/v2/cometbft/server.go(0 hunks)server/v2/stf/branch/bench_test.go(1 hunks)server/v2/store/snapshot.go(1 hunks)store/iavl/store_test.go(0 hunks)store/v2/commitment/iavl/tree.go(3 hunks)store/v2/commitment/store.go(3 hunks)store/v2/commitment/store_test_suite.go(3 hunks)store/v2/database.go(1 hunks)store/v2/migration/README.md(1 hunks)store/v2/migration/manager.go(7 hunks)store/v2/migration/manager_test.go(3 hunks)store/v2/mock/db_mock.go(4 hunks)store/v2/mock/types.go(1 hunks)store/v2/pruning/manager.go(1 hunks)store/v2/pruning/manager_test.go(1 hunks)store/v2/root/factory.go(2 hunks)store/v2/root/migrate_test.go(2 hunks)store/v2/root/store.go(3 hunks)store/v2/root/store_mock_test.go(3 hunks)store/v2/root/store_test.go(6 hunks)store/v2/root/upgrade_test.go(3 hunks)store/v2/snapshots/helpers_test.go(4 hunks)store/v2/snapshots/manager.go(2 hunks)store/v2/snapshots/manager_test.go(10 hunks)store/v2/snapshots/snapshotter.go(1 hunks)store/v2/storage/README.md(0 hunks)store/v2/storage/database.go(0 hunks)store/v2/storage/pebbledb/batch.go(0 hunks)store/v2/storage/pebbledb/comparator.go(0 hunks)store/v2/storage/pebbledb/comparator_test.go(0 hunks)store/v2/storage/pebbledb/db.go(0 hunks)store/v2/storage/pebbledb/db_test.go(0 hunks)store/v2/storage/pebbledb/iterator.go(0 hunks)store/v2/storage/rocksdb/batch.go(0 hunks)store/v2/storage/rocksdb/comparator.go(0 hunks)store/v2/storage/rocksdb/db.go(0 hunks)store/v2/storage/rocksdb/db_noflag.go(0 hunks)store/v2/storage/rocksdb/db_test.go(0 hunks)store/v2/storage/rocksdb/iterator.go(0 hunks)store/v2/storage/rocksdb/opts.go(0 hunks)store/v2/storage/storage_bench_test.go(0 hunks)store/v2/storage/storage_test_suite.go(0 hunks)store/v2/storage/store.go(0 hunks)store/v2/storage/util/iterator.go(0 hunks)store/v2/store.go(0 hunks)tests/integration/accounts/base_account_test.go(2 hunks)tests/integration/accounts/bundler_test.go(1 hunks)tests/integration/v2/auth/app_test.go(0 hunks)
💤 Files with no reviewable changes (23)
- store/v2/store.go
- store/v2/storage/pebbledb/db_test.go
- store/v2/storage/rocksdb/db_test.go
- store/iavl/store_test.go
- store/v2/storage/README.md
- store/v2/storage/pebbledb/comparator_test.go
- tests/integration/v2/auth/app_test.go
- store/v2/storage/util/iterator.go
- store/v2/storage/storage_bench_test.go
- store/v2/storage/rocksdb/iterator.go
- store/v2/storage/rocksdb/batch.go
- store/v2/storage/database.go
- server/v2/cometbft/server.go
- store/v2/storage/pebbledb/iterator.go
- store/v2/storage/rocksdb/opts.go
- store/v2/storage/rocksdb/comparator.go
- store/v2/storage/pebbledb/comparator.go
- store/v2/storage/rocksdb/db_noflag.go
- store/v2/storage/pebbledb/batch.go
- store/v2/storage/store.go
- store/v2/storage/rocksdb/db.go
- store/v2/storage/pebbledb/db.go
- store/v2/storage/storage_test_suite.go
✅ Files skipped from review due to trivial changes (3)
- tests/integration/accounts/base_account_test.go
- server/v2/stf/branch/bench_test.go
- store/v2/migration/README.md
🧰 Additional context used
📓 Path-based instructions (27)
server/v2/cometbft/internal/mock/mock_reader.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/mock/types.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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"
server/v2/cometbft/abci_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/database.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
crypto/keys/bls12_381/key_cgo.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/accounts/bundler_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
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/store_mock_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/pruning/manager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
runtime/v2/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/migration/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/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/commitment/iavl/tree.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/snapshots/manager.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/snapshots/snapshotter.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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/root/factory.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/snapshots/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"
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/migration/manager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/snapshots/helpers_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"
server/v2/store/snapshot.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/root/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/commitment/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/mock/db_mock.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (38)
crypto/keys/bls12_381/key_cgo.go (2)
Line range hint 70-77: Verify the security implications of removing message length checks in Sign method
The Sign method no longer hashes messages that exceed bls12381.MaxMsgLen. By signing messages of arbitrary length directly, there may be potential security risks or performance implications, depending on the underlying cryptographic library's handling of large messages.
Please ensure that removing the message length check and hashing step does not introduce vulnerabilities. Confirm that the cryptographic library securely handles messages of all lengths without the need for pre-hashing.
Line range hint 113-122: Verify the security implications of removing message length checks in VerifySignature method
Similarly, the VerifySignature method no longer hashes long messages before verification. This change could impact the security or correctness of signature verification for large messages.
Ensure that the verification process remains secure without hashing large messages. Verify that the cryptographic library correctly handles verification for messages of all lengths without needing to pre-hash them.
store/v2/mock/db_mock.go (3)
133-161: New methods Has and Iterator in mock are correctly implemented
The Has and Iterator methods added to MockStateCommitter follow the standard mocking patterns and correctly reflect the interface they are intended to mock.
232-242: Method PruneStoreKeys implementation aligns with interface requirements
The PruneStoreKeys method is properly added to the MockStateCommitter, ensuring consistency with the expected interface.
Line range hint 246-257: ReverseIterator method in mock is correctly implemented
The ReverseIterator method correctly mocks the interface, allowing for proper testing of reverse iteration functionality.
store/v2/mock/types.go (1)
11-12: Confirmed interface update to StateCommitter
The removal of unnecessary interfaces from StateCommitter simplifies the interface and focuses on essential functionalities.
store/v2/pruning/manager.go (1)
16-16: Verify removal of SS pruning parameters does not impact the codebase
The NewManager function signature has been updated to remove ssPruner and ssPruningOption. Ensure that no remaining references depend on these parameters elsewhere in the codebase.
Run the following script to check for any remaining references:
✅ Verification successful
No remaining references to SS pruning parameters found in the codebase
The removal of ssPruner and ssPruningOption parameters from the NewManager function signature appears to be safe as no other references to these parameters were found in the codebase. The only pruning-related code in the manager.go file is properly handling the SC (state commitment) pruning functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to `ssPruner` and `ssPruningOption`
rg 'ssPruner|ssPruningOption'
Length of output: 31
Script:
#!/bin/bash
# Let's check for any references to these parameters in the git history of the specific file
git log -p store/v2/pruning/manager.go | grep -E 'ssPruner|ssPruningOption'
# Also search for any pruner-related code to understand the context better
rg -A 3 'Pruner|PruningOption' store/v2/pruning/manager.go
Length of output: 739
Script:
#!/bin/bash
# Let's check for any references to SS pruning in the entire codebase
rg -i "ss.*prun"
# Also check for any test files that might be affected
fd -e test.go -x rg -l "NewManager.*Pruner" {}
Length of output: 78629
server/v2/cometbft/internal/mock/mock_reader.go (1)
Line range hint 42-64: Ensure method calls to GetStateCommitment are correct
The methods now use GetStateCommitment() instead of GetStateStorage(). Verify that this change aligns with the intended functionality and that GetStateCommitment() provides the required data access.
Run the following script to check for any remaining references to GetStateStorage():
✅ Verification successful
The change from GetStateStorage to GetStateCommitment is correct and consistent
The codebase analysis shows that:
GetStateCommitment()is a well-defined part of the store's interface (store/v2/store.go)- There are no remaining references to
GetStateStorage() - The method is consistently used across the codebase for state commitment operations
- The mock implementation in
mock_reader.gocorrectly aligns with the rest of the codebase's usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to `GetStateStorage()` in the codebase
rg 'GetStateStorage\(\)'
Length of output: 26
Script:
#!/bin/bash
# Let's check for both GetStateCommitment and GetStateStorage with context
rg -A 2 'Get(StateCommitment|StateStorage)'
# Also check for any interface or struct definitions related to state storage/commitment
ast-grep --pattern 'interface {
$$$
GetStateCommitment() $_
$$$
}'
ast-grep --pattern 'interface {
$$$
GetStateStorage() $_
$$$
}'
Length of output: 4133
store/v2/snapshots/snapshotter.go (1)
16-16: Verify removal of chStorage parameter from Restore method
The Restore method in the CommitSnapshotter interface has been updated by removing the chStorage parameter. Ensure that all implementations of this interface are updated to reflect this change and that no functionality is adversely affected.
Run the following script to find implementations using the old Restore method signature:
✅ Verification successful
Based on the search results, I can see the implementations of the Restore method across the codebase. The key implementations show that the method signature has been consistently updated to match the interface change, removing the chStorage parameter. Here are the relevant implementations found:
store/v2/commitment/store.go:
func (c *CommitStore) Restore(version uint64, format uint32, protoReader protoio.Reader) (snapshotstypes.SnapshotItem, error)store/rootmulti/store.go:
func (rs *Store) Restore(height uint64, format uint32, protoReader protoio.Reader) (snapshottypes.SnapshotItem, error)- Mock implementations in test files also follow the same signature:
store/v2/snapshots/helpers_test.gostore/snapshots/helpers_test.go
All implementations are consistent with the updated interface, and there are no instances of the old signature with the chStorage parameter. Therefore, I can confidently verify that the change has been properly implemented across the codebase.
All implementations of Restore method have been updated correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find implementations of `Restore` method with the old signature
ast-grep --pattern 'func ($_).Restore($_, $_, $_, $_) {$$$}'
Length of output: 62
Script:
#!/bin/bash
# Let's try a different approach to find Restore method implementations
# First, let's find all files that might contain Restore implementations
fd -e go | xargs rg "func.*Restore.*\(" -l
# Then let's see the actual implementations with context
fd -e go | xargs rg "func.*Restore.*\(" -B 2 -A 10
Length of output: 25061
store/v2/database.go (1)
31-32: Breaking Change: Verify migration guide for interface consolidation
The Committer interface now embeds both UpgradeableStore and VersionedReader, which is a breaking change. While this simplification aligns with the PR objectives, it requires careful migration handling.
✅ Verification successful
Let me check the content of the documentation files to verify if the breaking changes are properly documented.
Let me check the implementations to understand the impact of these interface changes.
Based on the search results and documentation, I can now provide a final response about the interface changes:
Breaking Change: Interface Consolidation is Documented and Expected
The removal of VersionedWriter and consolidation into UpgradeableStore and VersionedReader is a documented breaking change that aligns with ADR-040's storage simplification goals. The change is well-supported because:
- The Committer interface changes are documented in ADR-040, which explicitly mentions interface consolidation
- Existing implementations (rootmulti.Store, iavl.Store, transient.Store) already handle versioning through the Committer interface
- The change is part of a larger storage simplification effort described in the architecture documentation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for migration documentation and potential impact
rg -A 5 "VersionedWriter|UpgradeableStore" docs/
rg -l "implements.*Committer"
Length of output: 247
Script:
#!/bin/bash
# Check documentation content
cat docs/architecture/adr-040-storage-and-smt-state-commitments.md docs/learn/advanced/04-store.md
# Check implementation details
rg -B 2 -A 2 "type.*Committer" store/v2/database.go
Length of output: 37855
Script:
#!/bin/bash
# Check for implementations of Committer interface
rg -B 3 -A 3 "implements.*Committer|struct.*implements.*Committer"
# Check for VersionedWriter usage
rg -B 2 -A 2 "VersionedWriter"
Length of output: 4483
server/v2/cometbft/internal/mock/mock_store.go (1)
30-31: LGTM: Constructor simplified appropriately
The simplified constructor aligns with the interface consolidation changes.
store/v2/root/store_mock_test.go (1)
18-25: LGTM: Test setup properly simplified
The test setup correctly reflects the interface consolidation changes.
store/v2/root/factory.go (2)
64-65: LGTM!
Variable declarations follow Go conventions.
129-130: Document nil parameters and verify function signature usage
While the changes align with the storage simplification, the nil parameters should be documented. Let's verify all callers handle nil parameters correctly.
Consider adding a comment explaining why the last two parameters are nil after the storage simplification.
store/v2/root/upgrade_test.go (1)
109-109: Verify test coverage for modified store keys
The modification to include "store2" in oldStoreKeys affects test coverage. Ensure this change correctly tests the upgrade functionality.
✅ Verification successful
Let me gather more context about the test implementation to verify the coverage.
Test coverage is properly implemented for the store keys
The test implementation shows comprehensive coverage:
- It verifies all old store keys ("store1", "store2", "store3") are queryable for all versions
- The upgrades configuration correctly handles the deletion of "store3" and addition of new stores
- Each store key is tested with multiple versions and key-value pairs
- The test validates both pre-upgrade and post-upgrade scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for upgrade functionality
rg -A 10 "TestLoadVersionAndUpgrade" store/v2/root/
Length of output: 722
Script:
#!/bin/bash
# Check the full test implementation and related setup
rg -B 5 -A 20 "oldStoreKeys :=" store/v2/root/upgrade_test.go
Length of output: 1008
store/v2/root/migrate_test.go (2)
111-111: LGTM!
The version check is correctly simplified to use GetLatestVersion directly.
71-73: Verify migration functionality with removed storage
The removal of storage components affects migration. While the changes are consistent, we should:
- Verify that migration still works correctly without storage
- Ensure error handling is robust with nil parameters
Consider documenting the architectural decision to remove storage from migration process in a comment or documentation.
Also applies to: 76-76
✅ Verification successful
Let me gather more information about the migration functionality and its tests.
Migration functionality is robust with nil parameters and storage removal
Based on the code analysis:
- The migration manager explicitly supports nil state commitment (
sc) parameter as confirmed in both the implementation and documentation comments. - Error handling is comprehensive, covering all critical paths including nil cases and storage-related operations.
- The changes are well-tested, with test cases verifying functionality both with and without storage components.
- The architectural decision is well-documented in the README.md, explicitly stating that state commitment can be nil.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for error handling in migration code
rg "Error|err" store/v2/migration/
# Look for migration tests
ast-grep --pattern 'func Test$$$Migration$$$'
Length of output: 7555
Script:
#!/bin/bash
# Check migration manager implementation
cat store/v2/migration/manager.go
# Check migration tests implementation
cat store/v2/migration/manager_test.go
# Look for any migration-related documentation
cat store/v2/migration/README.md
Length of output: 15585
store/v2/commitment/iavl/tree.go (2)
106-114: LGTM! Consistent implementation pattern.
The implementation follows the same pattern as GetProof, maintaining consistency in handling empty mutable tree cases.
86-94: LGTM! Good handling of edge cases.
The added check for empty mutable tree at genesis or after key removal is a good optimization. It prevents unnecessary immutable tree access.
Let's verify the edge cases:
✅ Verification successful
LGTM! The edge cases are properly tested.
The test file shows comprehensive coverage of the scenarios we were concerned about:
- Genesis state (version 0) is explicitly tested with
initVersioncheck - Key removal scenario is tested in version 2 where
key1is deleted and its proof changes from Exist to Nonexist - Version handling is thoroughly tested across multiple commits
- The GetLatestVersion behavior is verified at different stages (genesis, after commits, after pruning)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases that verify genesis and key removal scenarios
rg -A 5 "TestGet.*genesis|TestGet.*removal" store/v2/commitment/iavl/
Length of output: 71
Script:
#!/bin/bash
# Let's search for test files and test cases related to the tree implementation
fd "tree_test.go" store/v2/commitment/iavl/
# Search for any test cases that might verify the GetLatestVersion behavior
rg -A 5 "TestTree.*Get|Test.*Version" store/v2/commitment/iavl/
# Look for actual implementation of GetLatestVersion to understand the behavior
ast-grep --pattern 'func ($t $_) GetLatestVersion() ($_, $_) {
$$$
}'
Length of output: 221
Script:
#!/bin/bash
# Let's examine the test file content to understand test coverage
cat store/v2/commitment/iavl/tree_test.go
# Also search for any test setup or helper functions that might involve version checks
rg -A 5 "func.*Test|Setup|Before" store/v2/commitment/iavl/tree_test.go
Length of output: 6989
store/v2/migration/manager.go (4)
7-7: LGTM! Good improvement in thread safety.
Converting migratedVersion to atomic.Uint64 eliminates the need for explicit mutex locks while ensuring thread-safe operations.
Also applies to: 41-41
98-99: LGTM! Good simplification of state restoration.
The direct call to stateCommitment.Restore simplifies the code while maintaining proper error handling.
108-108: LGTM! Consistent use of atomic operations.
All migratedVersion operations have been correctly converted to use atomic operations, maintaining thread safety throughout the codebase.
Also applies to: 152-152, 195-195
50-50: Verify all NewManager callers are updated.
The removal of the stateStorage parameter requires updates to all callers of NewManager.
✅ Verification successful
No issues found with NewManager callers
The search results show that all callers of migration.NewManager are using the correct 4-parameter signature (db, snapshotManager, commitStore, logger). The only caller found in store/v2/root/migrate_test.go is already using the updated signature without the stateStorage parameter. The function definition in store/v2/migration/manager.go also confirms the correct parameter list.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NewManager calls that might still use the old signature
ast-grep --pattern 'NewManager($_, $_, $_, $_, $_)'
Length of output: 53
Script:
#!/bin/bash
# Search for NewManager function calls with any number of arguments
rg "NewManager\(" -A 2
# Also search for test files that might use NewManager
rg "NewManager\(" --type go -A 2
Length of output: 40830
Script:
#!/bin/bash
# Search for files that import and use the migration.NewManager
rg "migration\.NewManager\(" -B 2 -A 2
# Also search for direct callers of the NewManager function in the migration package
rg "NewManager\(.*db.*sm.*sc.*logger" --type go -B 2 -A 2
Length of output: 912
store/v2/migration/manager_test.go (2)
20-20: LGTM! Test setup simplified appropriately.
The removal of noCommitStore parameter and related logic simplifies the test setup while maintaining test coverage.
Also applies to: 47-47
97-177: LGTM! Comprehensive test coverage maintained.
The test thoroughly verifies the migration process, including:
- Changeset handling
- State verification
- Error conditions
- DB storage validation
store/v2/pruning/manager_test.go (1)
45-46: LGTM: Simplified manager initialization
The removal of the storage store parameter aligns with the PR's objective of simplifying the storage mechanism.
runtime/v2/builder.go (1)
137-137: LGTM: Improved code readability
The added whitespace enhances readability by clearly separating error handling from the JSON unmarshaling logic.
store/v2/snapshots/helpers_test.go (3)
111-112: LGTM: Simplified Restore method signature
The removal of the chStorage channel parameter streamlines the interface and aligns with the storage simplification goals.
166-167: LGTM: Consistent interface implementation
The mock error snapshotter's signature change maintains consistency with the main implementation.
186-186: LGTM: Consistent test adaptations
The test setup and mock implementations have been correctly adapted to the simplified storage architecture.
Also applies to: 230-230
tests/integration/accounts/bundler_test.go (1)
212-212: LGTM! Good addition of t.Helper()
Adding t.Helper() improves test failure reporting by marking makeTx as a helper function, which helps identify the actual test line where failures occur.
store/v2/snapshots/manager_test.go (1)
21-21: LGTM! Consistent removal of storageSnapshotter parameter
The changes consistently remove the storageSnapshotter parameter from NewManager calls across all test functions, aligning with the PR objective to simplify the storage mechanism. This modification maintains test integrity while reducing unnecessary dependencies.
Also applies to: 42-42, 74-74, 113-113, 133-133, 252-252, 272-272, 358-358, 437-437, 509-509
store/v2/snapshots/manager.go (2)
76-85: LGTM! Simplified Manager initialization
The removal of storageSnapshotter parameter and its associated field simplifies the Manager struct and its initialization, aligning with the PR objective to streamline the storage mechanism.
397-397: LGTM! Simplified restoration process
The restoration process has been streamlined by directly using the commit snapshotter's response, removing the complexity of handling a separate storage snapshotter.
store/v2/root/store_test.go (2)
54-55: LGTM: Simplified store initialization
The removal of storage-related parameters streamlines the store initialization process, aligning with the PR's goal of simplifying storage.
Also applies to: 74-76, 82-85
597-599: LGTM: Consistent pruning manager updates
The pruning manager initialization has been consistently updated across all test scenarios to remove storage dependencies.
Also applies to: 623-625, 676-678, 763-765
server/v2/cometbft/abci_test.go (2)
594-594: LGTM: State commitment write operation looks correct.
The change from ApplyChangeset to WriteChangeset aligns with the storage simplification objective mentioned in the PR description.
695-695: LGTM: Mock store initialization is properly simplified.
The simplified mock store creation with direct usage of state commitment improves test clarity.
store/v2/commitment/store.go
Outdated
| latestVersion, err := c.metadata.GetLatestVersion() | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if latestVersion == 0 { | ||
| return true, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential incorrect behavior when latestVersion is zero in VersionExists method
When latestVersion is zero, the VersionExists method returns true for any version. This may not be accurate, as no versions exist yet.
Consider modifying the logic to return true only when version == 0:
func (c *CommitStore) VersionExists(version uint64) (bool, error) {
latestVersion, err := c.metadata.GetLatestVersion()
if err != nil {
return false, err
}
if latestVersion == 0 {
- return true, nil
+ return version == 0, nil
}
ci, err := c.metadata.GetCommitInfo(version)
return ci != nil, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| latestVersion, err := c.metadata.GetLatestVersion() | |
| if err != nil { | |
| return false, err | |
| } | |
| if latestVersion == 0 { | |
| return true, nil | |
| } | |
| latestVersion, err := c.metadata.GetLatestVersion() | |
| if err != nil { | |
| return false, err | |
| } | |
| if latestVersion == 0 { | |
| return version == 0, nil | |
| } | |
| // 3. Snapshot Restoration: | ||
| // - Creates a snapshot reader from the chunks | ||
| // - Sets up a channel for state changes during restoration | ||
| // - Launches a goroutine to collect restored key-value pairs | ||
| // - Restores the snapshot into the target store | ||
| // - Verifies the extension metadata was preserved | ||
| // |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update comments to reflect code changes
The comments mention setting up a channel for state changes during restoration, but the code no longer includes this step. Please update the comments to accurately reflect the current implementation.
Apply this diff to update the comments:
// 3. Snapshot Restoration:
- Creates a snapshot reader from the chunks
- - Sets up a channel for state changes during restoration
- - Launches a goroutine to collect restored key-value pairs
- Restores the snapshot into the target store
- Verifies the extension metadata was preserved📝 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.
| // 3. Snapshot Restoration: | |
| // - Creates a snapshot reader from the chunks | |
| // - Sets up a channel for state changes during restoration | |
| // - Launches a goroutine to collect restored key-value pairs | |
| // - Restores the snapshot into the target store | |
| // - Verifies the extension metadata was preserved | |
| // | |
| // 3. Snapshot Restoration: | |
| // - Creates a snapshot reader from the chunks | |
| // - Restores the snapshot into the target store | |
| // - Verifies the extension metadata was preserved | |
| // |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if lv == 0 { |
There was a problem hiding this comment.
couldn't genesis be done at a height other than 0?
There was a problem hiding this comment.
when starting a node the version of the tree will always be 0, since its empty and if the root is nil the version is assumed to be 0, but when you write (apply change sets) it would be on the height/version you specify.
If you remove a store key the values are still there, unless pruned. In that case the mutable tree will nil because the version of the tree could be 30 but the key was removed at 20. Clients may still want to query version(s) <20. In this case the latest version of the tree will be 20 not 30.
store/v2/commitment/iavl/tree.go
Outdated
| @@ -93,6 +103,16 @@ func (t *IavlTree) GetProof(version uint64, key []byte) (*ics23.CommitmentProof, | |||
|
|
|||
| // Get implements the Reader interface. | |||
| func (t *IavlTree) Get(version uint64, key []byte) ([]byte, error) { | |||
| // the mutable tree is empty at genesis & when the key is removed, but the immutable tree is not | |||
There was a problem hiding this comment.
do you mean to say that t.tree.Get(key will return nil when key is absent? what's special about genesis?
There was a problem hiding this comment.
sorry meant storekey, the above comment explains the flow
store/v2/commitment/store.go
Outdated
| return false, err | ||
| } | ||
| if latestVersion == 0 { | ||
| return true, nil |
There was a problem hiding this comment.
why does version 0 need special casing?
There was a problem hiding this comment.
its meant to treat the flow as if there is no key set then its not found but we would still want to return true here
tac0turtle
force-pushed
the
marko/SS_removal
branch
from
cbd01ed to
e2a8fd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
store/v2/commitment/store.go (1)
Line range hint
236-262: Improve error handling in GetProof methodThe error message when commit info is nil could be more descriptive to help with debugging.
Consider updating the error message to include more context:
- return nil, fmt.Errorf("commit info not found for version %d", version) + return nil, fmt.Errorf("commit info not found for store %s at version %d", rawStoreKey, version)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
store/v2/commitment/iavl/tree.go(3 hunks)store/v2/commitment/store.go(3 hunks)tests/integration/v2/auth/app_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/v2/auth/app_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- store/v2/commitment/iavl/tree.go
🧰 Additional context used
📓 Path-based instructions (1)
store/v2/commitment/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
store/v2/commitment/store.go (2)
291-298: LGTM! Fixed the version 0 handling
The implementation now correctly handles version 0, returning true only when checking for version 0 itself. This aligns with the intended behavior of treating no key set as "not found".
272-277: Consider the implications of checking oldTrees before multiTrees
The current implementation prioritizes checking oldTrees before multiTrees. This could potentially mask newer versions of stores if a store exists in both maps.
Let's verify if there are any cases where store keys exist in both maps:
(cherry picked from commit 94cfcc1) # Conflicts: # runtime/v2/builder.go # server/v2/stf/branch/bench_test.go # server/v2/store/snapshot.go # store/iavl/store_test.go # store/v2/commitment/iavl/tree.go # store/v2/commitment/store.go # store/v2/commitment/store_test_suite.go # store/v2/database.go # store/v2/migration/README.md # store/v2/migration/manager.go # store/v2/migration/manager_test.go # store/v2/mock/db_mock.go # store/v2/mock/types.go # store/v2/pruning/manager.go # store/v2/pruning/manager_test.go # store/v2/root/factory.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/snapshots/helpers_test.go # store/v2/snapshots/manager.go # store/v2/snapshots/manager_test.go # store/v2/snapshots/snapshotter.go # store/v2/store.go # tests/integration/v2/auth/app_test.go
…port #22683) (#22734) Co-authored-by: Marko <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
This pr simplifies storage. With the offchain inprocess indexer there is not a need for state storage any more. We will hold a retro on the work, but we plan on integrating iavlv1, iavlv2 and memiavl as sane defaults in the storage layer
closes #22652
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
Summary by CodeRabbit