feat(store/v2): handle store upgrades in RootStore #18277
Conversation
WalkthroughThe changes primarily revolve around the modification of the Changes
TipsChat with CodeRabbit Bot (
|
store/store.go
Outdated
| // | ||
| // Note, handling StoreUpgrades is optional depending on the underlying RootStore | ||
| // implementation. | ||
| LoadVersionAndUpgrade(version uint64, upgrades *StoreUpgrades) error |
There was a problem hiding this comment.
should this be a separate interface that we cast a store to? if its optional then its more boiler plate for implementors
There was a problem hiding this comment.
It's more so that the use of the upgrades argument is optional. In such a case, LoadVersionAndUpgrade would behave the same way LoadVersion does.
We can move LoadVersionAndUpgrade to a separate interface if you'd like?
There was a problem hiding this comment.
i dont think it would make such a big ux win, this is an area where lots of changes wont happen
There was a problem hiding this comment.
So move that method to an interface?
There was a problem hiding this comment.
I am with @tac0turtle that we should move that method to an optional interface perhaps called "UpgradeableStore" or "StoreWithUpgrades" say
type UpgradeableStore inteface {
Store
LoadVersionAndUpgrade(version uint64, upgrades *StoreUpgrades) error
}
github-prbot
requested review from
a team,
facundomedica and
julienrbrt
and removed request for
a team
|
@alexanderbez your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (11)
- store/kv/branch/store.go (2 hunks)
- store/kv/branch/store_test.go (1 hunks)
- store/kv/gas/store.go (1 hunks)
- store/kv/gas/store_test.go (1 hunks)
- store/kv/mem/store.go (1 hunks)
- store/kv/mem/store_test.go (1 hunks)
- store/kv/trace/store.go (1 hunks)
- store/root/store.go (5 hunks)
- store/root/store_test.go (1 hunks)
- store/store.go (2 hunks)
- store/upgrade.go (1 hunks)
Additional comments: 11
store/kv/mem/store_test.go (1)
- 46-50: The
Resetfunction now takes an additional argument. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, it would be good to add a comment explaining what the1argument represents in theResetfunction call for better readability and maintainability.store/kv/branch/store_test.go (1)
- 66-69: The
Resetfunction now takes an additional argument. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, consider adding a comment to explain what the1argument represents for better code readability.store/kv/gas/store_test.go (1)
- 38-39: The
Resetmethod now takes an additional parameter. Ensure that the value1is the correct value to be passed in all scenarios where this test is run. If the value is context-dependent, consider passing it as a parameter to the test function or retrieving it from a configuration file or environment variable.err := s.gasKVStore.Reset(1) s.Require().NoError(err)store/root/store_test.go (1)
- 131-196: The
TestLoadVersionfunction seems to be well written and covers a variety of scenarios including writing and committing changesets, loading non-existent and previously committed versions, querying state based on loaded version, and overwriting values. It also checks for the correct latest version after each operation. This is a good practice as it ensures the integrity of the versioning system.store/store.go (2)
52-67: The new methods
LoadVersionAndUpgradeandLoadLatestVersionhave been added to theRootStoreinterface. Ensure that all implementations of this interface have been updated to include these methods. Also, verify that theLoadVersionmethod no longer returns an error and all its usages have been updated accordingly.127-131: The
Resetmethod in theKVStoreinterface now takes an additional parameterv uint64. Ensure that all implementations of this interface have been updated to include this change. Also, verify that all calls to this method throughout the codebase have been updated to match the new signature.store/kv/trace/store.go (1)
- 83-85: The
Resetfunction now takes an additional parametervof typeuint64. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, make sure that theResetfunction in theparentstore properly handles thevparameter.store/upgrade.go (1)
- 1-53: The new types and methods introduced in this hunk seem to be well-structured and logically sound. The
StoreUpgradesstruct and its methods provide a clear way to manage store transformations. TheStoreRenamestruct is a simple and effective way to represent a change in a store key. The use of theslices.Containsfunction from thegolang.org/x/exp/slicespackage is a good choice for checking if a key should be added or deleted. TheRenamedFrommethod correctly iterates over theRenamedslice to find if a key was renamed.However, the
slices.Containsfunction has a time complexity of O(n) which could be a performance issue if theAddedorDeletedslices are large. Consider using a data structure with faster lookup times, such as a map, if performance becomes an issue.store/kv/branch/store.go (2)
1-7: The new import statement for the "fmt" package is fine as it is used for error formatting in the updated
Resetfunction.88-100: The
Resetfunction now takes an additional parametervof typeuint64. This function sets the latest version of the storage to the value ofvand updates theversionfield of theStorestruct accordingly. The error handling in the function has also been modified. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.store/kv/gas/store.go (1)
- 65-67: The function signature of
Resethas been updated to include a new parameterv. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, make sure that theResetfunction in theparentstore properly handles this new parameter.
There was a problem hiding this comment.
Thank you @alexanderbez for the interesting PR, I've added some feedback, please take a look.
store/store.go
Outdated
| // | ||
| // Note, handling StoreUpgrades is optional depending on the underlying RootStore | ||
| // implementation. | ||
| LoadVersionAndUpgrade(version uint64, upgrades *StoreUpgrades) error |
There was a problem hiding this comment.
I am with @tac0turtle that we should move that method to an optional interface perhaps called "UpgradeableStore" or "StoreWithUpgrades" say
type UpgradeableStore inteface {
Store
LoadVersionAndUpgrade(version uint64, upgrades *StoreUpgrades) error
}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- store/kv/branch/store.go (2 hunks)
- store/kv/gas/store.go (1 hunks)
- store/kv/trace/store.go (1 hunks)
- store/store.go (2 hunks)
- store/upgrade.go (1 hunks)
Additional comments: 6
store/kv/trace/store.go (1)
- 83-85: The
Resetfunction has been updated to accept a new parametertoVersion. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, make sure that theparentobject'sResetmethod can handle this new parameter correctly.store/kv/gas/store.go (1)
- 65-67: The
Resetfunction now takes an additional parametertoVersionof typeuint64. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, make sure that theResetfunction in theparentstore handles this new parameter correctly.store/kv/branch/store.go (2)
1-7: The new import statement for the "fmt" package is fine.
88-100: The
Resetfunction now takes an additional parametertoVersion uint64and returns an error. This is a breaking change and all calls to this function throughout the codebase should be updated to match the new signature. Also, ensure that error handling is properly done wherever this function is called. The logic inside the function seems fine.- func (s *Store) Reset() { + func (s *Store) Reset(toVersion uint64) error {store/store.go (2)
52-67: The new functions
LoadVersion,LoadVersionAndUpgrade, andLoadLatestVersionhave been added to theRootStoreinterface. Ensure that all implementations of this interface have been updated to include these new functions. Also, verify that theversionparameter inLoadVersionandLoadVersionAndUpgradeis being used correctly in the function implementations.130-130: The
Resetfunction in theKVStoreinterface now takes atoVersionparameter. Ensure that all implementations of this interface have been updated to include this new parameter. Also, verify that thetoVersionparameter is being used correctly in the function implementations.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (2)
- store/root/store.go (5 hunks)
- store/store.go (3 hunks)
Additional comments: 3
store/store.go (3)
52-60: The new methods
LoadVersionandLoadLatestVersionhave been added to theRootStoreinterface. Ensure that all implementations of this interface have been updated to include these methods.86-98: The new
UpgradeableRootStoreinterface extends theRootStoreinterface and adds theLoadVersionAndUpgrademethod. This is a good example of interface segregation, a principle of SOLID, where a new interface is created instead of adding the method to the existing interface. This ensures that only the stores that need to handle upgrades implement this interface.137-137: The
Resetmethod in theKVStoreinterface now takes an additionaluint64parameter. Ensure that all implementations of this interface have been updated to match the new method signature.
store/root/store.go
Outdated
| return s.loadVersion(version, nil) | ||
| } | ||
|
|
||
| func (s *Store) loadVersion(v uint64, _ *store.StoreUpgrades) error { |
There was a problem hiding this comment.
It is a WIP? couldn't see the call of loadVersion with upgrades.
There was a problem hiding this comment.
This implementation doesn't support store upgrades since it's all a single tree/store key. In other words, store keys are ignored in the single tree implementation.
Co-authored-by: cool-developer <[email protected]>
| return s.stateCommitment | ||
| } | ||
|
|
||
| func (s *Store) LoadLatestVersion() error { | ||
| lv, err := s.GetLatestVersion() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return s.loadVersion(lv, nil) | ||
| } | ||
|
|
||
| // LastCommitID returns a CommitID based off of the latest internal CommitInfo. | ||
| // If an internal CommitInfo is not set, a new one will be returned with only the | ||
| // latest version set, which is based off of the SS view. |
There was a problem hiding this comment.
The function LastCommitID is returning a new CommitID with only the latest version set when lastCommitInfo is nil. This might lead to unexpected behavior if the caller expects a fully populated CommitID. Consider returning an error in this case.
store/root/store.go
Outdated
| func (s *Store) loadVersion(v uint64, _ *store.StoreUpgrades) error { | ||
| s.logger.Debug("loading version", "version", v) | ||
|
|
||
| // Reset the root KVStore s.t. the latest version is v. Any writes will | ||
| // overwrite existing versions. | ||
| if err := s.rootKVStore.Reset(v); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := s.stateCommitment.LoadVersion(v); err != nil { | ||
| return fmt.Errorf("failed to load SS version %d: %w", v, err) | ||
| } | ||
|
|
||
| // TODO: Complete this method to handle upgrades. See legacy RMS loadVersion() | ||
| // for reference. | ||
| // | ||
| // Ref: https://github.com/cosmos/cosmos-sdk/issues/17314 | ||
| s.workingHash = nil | ||
| s.commitHeader = nil | ||
|
|
||
| // set lastCommitInfo explicitly s.t. Commit commits the correct version, i.e. v+1 | ||
| s.lastCommitInfo = &store.CommitInfo{Version: v} | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The function loadVersion is ignoring the StoreUpgrades parameter. If this is intentional, consider documenting this behavior in the function comment to avoid confusion.
| s.lastCommitInfo.Timestamp = s.commitHeader.GetTime() | ||
| } | ||
|
|
||
| if err := s.rootKVStore.Reset(); err != nil { | ||
| if err := s.rootKVStore.Reset(version); err != nil { | ||
| return nil, fmt.Errorf("failed to reset root KVStore: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
The function Commit is resetting the rootKVStore to the current version after committing the changes. This might lead to data loss if there are uncommitted changes in the rootKVStore. Consider adding a check to ensure that there are no uncommitted changes before resetting the rootKVStore.
| version = previousHeight + 1 | ||
| } | ||
|
|
||
| workingHash := s.stateCommitment.WorkingHash() | ||
|
|
||
| s.lastCommitInfo = &store.CommitInfo{ | ||
| Version: version, | ||
| StoreInfos: []store.StoreInfo{ | ||
| { | ||
| Name: defaultStoreKey, | ||
| CommitID: store.CommitID{ | ||
| Version: version, | ||
| Hash: workingHash, | ||
| Hash: s.stateCommitment.WorkingHash(), | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The function writeSC is creating a new CommitInfo with only one StoreInfo for the default store key. This might lead to unexpected behavior if the caller expects a CommitInfo with all the StoreInfos. Consider adding a check to ensure that all the StoreInfos are included in the CommitInfo.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- store/root/store.go (5 hunks)
Additional comments: 3
store/root/store.go (3)
101-106: The function
LastCommitIDis returning a newCommitIDwith only the latest version set whenlastCommitInfoisnil. This might lead to unexpected behavior if the caller expects a fully populatedCommitID. Consider returning an error in this case.311-317: The function
Commitis resetting therootKVStoreto the current version after committing the changes. This might lead to data loss if there are uncommitted changes in therootKVStore. Consider adding a check to ensure that there are no uncommitted changes before resetting therootKVStore.347-360: The function
writeSCis creating a newCommitInfowith only oneStoreInfofor the default store key. This might lead to unexpected behavior if the caller expects aCommitInfowith all theStoreInfos. Consider adding a check to ensure that all theStoreInfos are included in theCommitInfo.
Description
closes: #18276
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
Refactor:
Resetfunction across various store modules now requires an additional parameter, enhancing control over version management.LoadVersionfunction in theRootStoreinterface has been updated for improved error handling.Commitfunction in theroot/store.gonow uses aversionparameter for resetting the root KVStore, improving data consistency.New Features:
LoadVersionAndUpgradeandLoadLatestVersionfunctions to theRootStoreinterface, providing more flexibility in version handling.Tests:
store_test.gofiles to accommodate the changes in theResetfunction.TestLoadVersioninstore/root/store_test.goto ensure correct version loading and state querying.Chores:
store/upgrade.gowithStoreUpgradesandStoreRenametypes, and associated methods, paving the way for future store transformations.