feat(store/v2): Add historical queries support for IAVL v1 to v2 migration #23685
Conversation
📝 WalkthroughWalkthroughThis pull request introduces historical queries functionality to the IAVL tree migration process. The documentation is updated with a new "Historical Queries After Migration" section that explains configuration, operational details, and node operator instructions. The IAVL configuration is enhanced by adding an Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant T as IavlTree
participant HQ as HistoricalQuerier
participant R as OldReader
participant N as NewTree
C->>T: Query (version, key)
T->>HQ: Forward query request
alt Version < migration height and historical queries enabled
HQ->>R: Retrieve value from legacy tree
R-->>HQ: Return old data
else
HQ->>N: Retrieve value from current tree
N-->>HQ: Return current data
end
HQ-->>T: Return fetched data
T-->>C: Deliver query result
sequenceDiagram
participant M as Migration Manager
participant DB as Database
M->>DB: StoreMigrationHeight(height)
DB-->>M: Acknowledge or error response
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
store/v2/commitment/iavl/tree.go (1)
55-60: Reinitializing querier after setting oldReader is sensible.
This design recalculates the querier only when needed. Keep in mind this may add overhead on the first subsequent call toGetorIterator. If performance is an issue, consider re-creating the querier here directly.store/v2/migration/migration_height.go (1)
10-13: Consider using a more descriptive key format.The current key format
m/heightcould be made more descriptive to avoid potential collisions with other migration-related keys.- migrationHeightKey = "m/height" + migrationHeightKey = "migration/store_v2/height"store/v2/README.md (4)
72-75: Add package import to the configuration example.The configuration example would be clearer with the package import statement.
+import "github.com/cosmos/cosmos-sdk/store/v2/commitment/iavl" + cfg := iavl.DefaultConfig() cfg.EnableHistoricalQueries = true
81-83: Use consistent list style with asterisks.For better maintainability and consistency with Markdown standards, use asterisks instead of dashes for unordered lists.
- - If version < migration height: uses old tree (if historical queries enabled) - - If version >= migration height: uses new tree - - If historical queries disabled: always uses new tree + * If version < migration height: uses old tree (if historical queries enabled) + * If version >= migration height: uses new tree + * If historical queries disabled: always uses new tree🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
81-81: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
82-82: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
83-83: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
88-89: Use consistent list style and add specific preservation instructions.Use asterisks for list items and provide more specific instructions about data preservation.
- - Ensure old tree data is preserved - - Configure `EnableHistoricalQueries` if historical access is needed + * Ensure old tree data is preserved by backing up the database directory + * Configure `EnableHistoricalQueries` in app.toml if historical access is needed🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
88-88: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
89-89: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
92-93: Use consistent list style and clarify configuration location.Use asterisks for list items and specify where to configure the setting.
- - Historical queries will automatically use appropriate tree based on height - - Can disable historical queries later by setting `EnableHistoricalQueries = false` + * Historical queries will automatically use appropriate tree based on height + * Can disable historical queries later by setting `EnableHistoricalQueries = false` in app.toml🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
92-92: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
93-93: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
store/v2/README.md(1 hunks)store/v2/commitment/iavl/config.go(1 hunks)store/v2/commitment/iavl/historical_queries.go(1 hunks)store/v2/commitment/iavl/historical_queries_test.go(1 hunks)store/v2/commitment/iavl/tree.go(2 hunks)store/v2/migration/manager.go(1 hunks)store/v2/migration/migration_height.go(1 hunks)store/v2/migration/migration_height_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.go`: Review the Golang code for conformity with the Ub...
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/commitment/iavl/historical_queries_test.gostore/v2/migration/migration_height.gostore/v2/commitment/iavl/tree.gostore/v2/commitment/iavl/config.gostore/v2/commitment/iavl/historical_queries.gostore/v2/migration/migration_height_test.gostore/v2/migration/manager.go
`**/*_test.go`: "Assess the unit test code assessing suffici...
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
store/v2/commitment/iavl/historical_queries_test.gostore/v2/migration/migration_height_test.go
`**/*.md`: "Assess the documentation for misspellings, gramm...
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
store/v2/README.md
🪛 golangci-lint (1.62.2)
store/v2/commitment/iavl/historical_queries_test.go
21-21: m.Called undefined (type *mockReader has no field or method Called)
(typecheck)
27-27: m.Called undefined (type *mockReader has no field or method Called)
(typecheck)
50-50: m.On undefined (type *mockReader has no field or method On)
(typecheck)
84-84: cannot use mockOldReader (variable of type *mockReader) as "cosmossdk.io/core/store".Reader value in argument to NewHistoricalQuerier: *mockReader does not implement "cosmossdk.io/core/store".Reader (wrong type for method Get)
have Get(uint64, []byte) ([]byte, error)
want Get([]byte) ([]byte, error)
(typecheck)
105-105: mockOldReader.AssertExpectations undefined (type *mockReader has no field or method AssertExpectations)
(typecheck)
107-107: mockOldReader.AssertNotCalled undefined (type *mockReader has no field or method AssertNotCalled)
(typecheck)
148-148: mockOldReader.On undefined (type *mockReader has no field or method On)
(typecheck)
store/v2/commitment/iavl/historical_queries.go
57-57: too many arguments in call to h.oldReader.Get
have (uint64, []byte)
want ([]byte)
(typecheck)
77-77: too many arguments in call to h.oldReader.Iterator
have (uint64, []byte, []byte, bool)
want ([]byte, []byte)
(typecheck)
🪛 markdownlint-cli2 (0.17.2)
store/v2/README.md
81-81: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
82-82: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
83-83: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
88-88: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
89-89: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
92-92: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
93-93: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (16)
store/v2/commitment/iavl/historical_queries.go (3)
1-14: Documentation is clearly written.
The high-level explanation of historical queries is straightforward and helps maintainers understand the new functionality quickly. No issues here.
15-19: Struct naming and field usage look consistent.
The fields in theHistoricalQuerierare well-named and capture all the necessary store references. No further concerns.
25-41: Basic parameter validation is a good practice.
The constructor checks fornilparameters and returns default configurations. This is solid defensive programming.store/v2/commitment/iavl/tree.go (4)
27-30: Adding references for db, oldReader, querier, and config is clear.
These fields neatly group all relevant components for historical queries and IAVL configuration.
34-53: Consider validating db before passing it to the iavl.NewMutableTree call.
dbis assumed not to be nil but presently not validated, and a nildbmight trigger a panic.func NewIavlTree(db corestore.KVStoreWithBatch, logger log.Logger, cfg *Config) (*IavlTree, error) { + if db == nil { + return nil, fmt.Errorf("db cannot be nil") + } if cfg == nil { cfg = DefaultConfig()
143-153: Lazy initialization of querier is fine but be mindful of concurrency.
Repeatedly creating the querier could be costly if called often. However, since the tree is not marked as concurrency-safe, the potential data race is likely not a concern here.
155-165: Iterator logic is consistent with the Get method.
The approach to lazily create the querier if none is present matches the pattern inGet. No further issues.store/v2/migration/migration_height_test.go (1)
1-29: Straightforward coverage of migration height logic.
The test thoroughly checks initial height and after storing a new height, ensuring correct readback. Consider future expansions for negative or boundary scenarios (e.g., storing zero or extremely large heights).store/v2/migration/migration_height.go (2)
15-20: LGTM! Clean and efficient implementation.The function correctly handles the conversion of uint64 to bytes using big-endian encoding and stores it in the database.
22-33: LGTM! Robust error handling and nil checks.The function properly handles:
- Database errors with descriptive error messages
- Nil case for when no migration has occurred
- Byte decoding using big-endian
store/v2/commitment/iavl/config.go (2)
4-18: LGTM! Well-documented configuration options.The Config struct is thoroughly documented with clear explanations for each field, including the new
EnableHistoricalQueriesoption.
20-28: Verify the significant increase in default cache size.The default cache size has been increased from 1000 to 1000000, which is a 1000x increase. While this might improve performance, it could significantly impact memory usage.
Please confirm:
- The memory impact of this change
- Whether this value has been performance tested
- If this is the optimal default for most use cases
store/v2/migration/manager.go (1)
108-112: LGTM! Proper error handling for migration height storage.The implementation correctly:
- Stores the migration height after successful migration
- Uses descriptive error message with proper error wrapping
store/v2/commitment/iavl/historical_queries_test.go (3)
31-168: LGTM! Comprehensive test coverage for historical queries.The test cases thoroughly cover:
- Historical queries before and after migration
- Enabled and disabled configurations
- Mock expectations validation
🧰 Tools
🪛 golangci-lint (1.62.2)
50-50: m.On undefined (type *mockReader has no field or method On)
(typecheck)
84-84: cannot use mockOldReader (variable of type *mockReader) as "cosmossdk.io/core/store".Reader value in argument to NewHistoricalQuerier: *mockReader does not implement "cosmossdk.io/core/store".Reader (wrong type for method Get)
have Get(uint64, []byte) ([]byte, error)
want Get([]byte) ([]byte, error)(typecheck)
105-105: mockOldReader.AssertExpectations undefined (type *mockReader has no field or method AssertExpectations)
(typecheck)
107-107: mockOldReader.AssertNotCalled undefined (type *mockReader has no field or method AssertNotCalled)
(typecheck)
148-148: mockOldReader.On undefined (type *mockReader has no field or method On)
(typecheck)
175-189: LGTM! Good error handling validation.The test properly validates error cases when migration height is not set.
191-250: LGTM! Thorough validation of querier initialization.The test cases cover all edge cases for querier initialization:
- Nil tree validation
- Nil database validation
- Nil configuration handling
| func (h *HistoricalQuerier) Get(version uint64, key []byte) ([]byte, error) { | ||
| if !h.config.EnableHistoricalQueries { | ||
| return h.tree.Get(version, key) | ||
| } | ||
|
|
||
| migrationHeight, err := migration.GetMigrationHeight(h.db) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get migration height: %w", err) | ||
| } | ||
|
|
||
| if version < migrationHeight && h.oldReader != nil { | ||
| return h.oldReader.Get(version, key) | ||
| } | ||
|
|
||
| return h.tree.Get(version, key) | ||
| } |
There was a problem hiding this comment.
Fix interface mismatch in oldReader calls.
Static analysis indicates that h.oldReader.Get(...) may not match the signature of store.Reader, which typically expects only Get(key []byte) ([]byte, error) without a version parameter. This code will fail to compile or will lead to a runtime panic if oldReader does not support version-based APIs.
Below is a possible fix if the old reader does not support version-based queries. You could remove the version parameter in the call (and logic), or employ a specialized versioned interface:
-if version < migrationHeight && h.oldReader != nil {
- return h.oldReader.Get(version, key)
+if version < migrationHeight && h.oldReader != nil {
+ // Consider using a specialized interface or remove version argument:
+ return h.oldReader.Get(key)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (h *HistoricalQuerier) Get(version uint64, key []byte) ([]byte, error) { | |
| if !h.config.EnableHistoricalQueries { | |
| return h.tree.Get(version, key) | |
| } | |
| migrationHeight, err := migration.GetMigrationHeight(h.db) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get migration height: %w", err) | |
| } | |
| if version < migrationHeight && h.oldReader != nil { | |
| return h.oldReader.Get(version, key) | |
| } | |
| return h.tree.Get(version, key) | |
| } | |
| func (h *HistoricalQuerier) Get(version uint64, key []byte) ([]byte, error) { | |
| if !h.config.EnableHistoricalQueries { | |
| return h.tree.Get(version, key) | |
| } | |
| migrationHeight, err := migration.GetMigrationHeight(h.db) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get migration height: %w", err) | |
| } | |
| if version < migrationHeight && h.oldReader != nil { | |
| // Consider using a specialized interface or remove version argument: | |
| return h.oldReader.Get(key) | |
| } | |
| return h.tree.Get(version, key) | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
57-57: too many arguments in call to h.oldReader.Get
have (uint64, []byte)
want ([]byte)
(typecheck)
| // Iterator returns an iterator over a domain of keys at the specified version. | ||
| // If historical queries are enabled and the version is before migration height, | ||
| // it will use the old reader. Otherwise, it uses the current tree. | ||
| func (h *HistoricalQuerier) Iterator(version uint64, start, end []byte, ascending bool) (store.Iterator, error) { | ||
| if !h.config.EnableHistoricalQueries { | ||
| return h.tree.Iterator(version, start, end, ascending) | ||
| } | ||
|
|
||
| migrationHeight, err := migration.GetMigrationHeight(h.db) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get migration height: %w", err) | ||
| } | ||
|
|
||
| if version < migrationHeight && h.oldReader != nil { | ||
| return h.oldReader.Iterator(version, start, end, ascending) | ||
| } | ||
|
|
||
| return h.tree.Iterator(version, start, end, ascending) | ||
| } |
There was a problem hiding this comment.
Same interface mismatch for oldReader in the iterator call.
As with Get, static analysis shows a similar discrepancy in oldReader.Iterator(...). The standard store.Reader interface typically has a method signature like Iterator(start, end []byte), lacking a version argument.
Below is a possible fix if your old reader is not version aware:
-if version < migrationHeight && h.oldReader != nil {
- return h.oldReader.Iterator(version, start, end, ascending)
+if version < migrationHeight && h.oldReader != nil {
+ // Use an interface that supports version-based iteration or remove version argument:
+ return h.oldReader.Iterator(start, end)
}📝 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.
| // Iterator returns an iterator over a domain of keys at the specified version. | |
| // If historical queries are enabled and the version is before migration height, | |
| // it will use the old reader. Otherwise, it uses the current tree. | |
| func (h *HistoricalQuerier) Iterator(version uint64, start, end []byte, ascending bool) (store.Iterator, error) { | |
| if !h.config.EnableHistoricalQueries { | |
| return h.tree.Iterator(version, start, end, ascending) | |
| } | |
| migrationHeight, err := migration.GetMigrationHeight(h.db) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get migration height: %w", err) | |
| } | |
| if version < migrationHeight && h.oldReader != nil { | |
| return h.oldReader.Iterator(version, start, end, ascending) | |
| } | |
| return h.tree.Iterator(version, start, end, ascending) | |
| } | |
| // Iterator returns an iterator over a domain of keys at the specified version. | |
| // If historical queries are enabled and the version is before migration height, | |
| // it will use the old reader. Otherwise, it uses the current tree. | |
| func (h *HistoricalQuerier) Iterator(version uint64, start, end []byte, ascending bool) (store.Iterator, error) { | |
| if !h.config.EnableHistoricalQueries { | |
| return h.tree.Iterator(version, start, end, ascending) | |
| } | |
| migrationHeight, err := migration.GetMigrationHeight(h.db) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get migration height: %w", err) | |
| } | |
| if version < migrationHeight && h.oldReader != nil { | |
| // Use an interface that supports version-based iteration or remove version argument: | |
| return h.oldReader.Iterator(start, end) | |
| } | |
| return h.tree.Iterator(version, start, end, ascending) | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
77-77: too many arguments in call to h.oldReader.Iterator
have (uint64, []byte, []byte, bool)
want ([]byte, []byte)
(typecheck)
| // mockReader implements store.Reader for testing purposes | ||
| type mockReader struct { | ||
| mock.Mock | ||
| } | ||
|
|
||
| // Get implements store.Reader | ||
| func (m *mockReader) Get(version uint64, key []byte) ([]byte, error) { | ||
| args := m.Called(version, key) | ||
| return args.Get(0).([]byte), args.Error(1) | ||
| } | ||
|
|
||
| // Iterator implements store.Reader | ||
| func (m *mockReader) Iterator(version uint64, start, end []byte, ascending bool) (store.Iterator, error) { | ||
| args := m.Called(version, start, end, ascending) | ||
| return args.Get(0).(store.Iterator), args.Error(1) | ||
| } |
There was a problem hiding this comment.
Fix the mock implementation to match the store.Reader interface.
The mockReader implementation doesn't match the store.Reader interface signature.
-func (m *mockReader) Get(version uint64, key []byte) ([]byte, error) {
+func (m *mockReader) Get(key []byte) ([]byte, error) {
args := m.Called(version, key)
return args.Get(0).([]byte), args.Error(1)
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
21-21: m.Called undefined (type *mockReader has no field or method Called)
(typecheck)
27-27: m.Called undefined (type *mockReader has no field or method Called)
(typecheck)
Problem:
Solution:
Features:
Summary by CodeRabbit
New Features
Documentation
Tests