feat!: add migrator to the commit abci call #387
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cmwaters
requested review from
evan-forbes,
rootulp,
staheri14 and
ninabarbakadze
as code owners
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
cmwaters
commented
Apr 11, 2024
Comment on lines
+355
to
+370
| storeMigrations, err := app.migrator.storeMigrator(header.Version.App, app.appVersion) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to get store migrations: %v", err)) | ||
| } | ||
| app.MountKVStores(storeMigrations.Added) | ||
| err = app.cms.LoadLatestVersionAndUpgrade(storeMigrations.ToStoreUpgrades()) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to upgrade stores: %v", err)) | ||
| } | ||
|
|
||
| // create a new cached branch of the store to apply migrations to | ||
| app.setDeliverState(header) | ||
| err = app.migrator.moduleMigrator(app.deliverState.ctx, header.Version.App, app.appVersion) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to migrate modules: %v", err)) | ||
| } |
There was a problem hiding this comment.
We need to be careful about atomicity here. I need to double check how CometBFT would handle this panic but iirc, we should also revert all the transactions previously committed
Comment on lines
+288
to
+291
| for name := range sm.Added { | ||
| added[i] = name | ||
| i++ | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
Comment on lines
+293
to
+296
| for name := range sm.Deleted { | ||
| deleted[i] = name | ||
| i++ | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
rootulp
approved these changes
Apr 15, 2024
baseapp/abci.go
Outdated
| @@ -130,7 +130,7 @@ func (app *BaseApp) SetOption(req abci.RequestSetOption) (res abci.ResponseSetOp | |||
| func (app *BaseApp) Info(req abci.RequestInfo) abci.ResponseInfo { | |||
| lastCommitID := app.cms.LastCommitID() | |||
| // load the app version for a non zero height and zero app hash | |||
There was a problem hiding this comment.
[optional] this comment seems confusing b/c it doesn't immediately apply to the conditional below it
Suggested change
| // load the app version for a non zero height and zero app hash |
There was a problem hiding this comment.
I think it still applies because you are still loading the app version if it hasn't been set
cmwaters
commented
Apr 15, 2024
rootulp
approved these changes
Apr 15, 2024
evan-forbes
approved these changes
Apr 15, 2024
cmwaters
added a commit
to celestiaorg/celestia-app
that referenced
this pull request
Apr 17, 2024
…ores (#3320) As was discovered with the apphash mismatch in #3167, by adding both v1 and v2 stores in the constructor we actually end up with a different app hash to `v1.x` as now there are a bunch of empy but initialized stores. What's needed is a mechanism that can start with only the v1 stores and then during the upgrade process add or remove the store for v2 as necessary. The main problem with this is that because we are using a branch of state we can't actually modify the stores until we write the new state to disk. Hence we can no longer perform the migration in `EndBlock` but in `Commit` whereby we first write state, then create the new stores, then perform the module migrations (like call `InitGenesis`) and then write it again finally returning the app hash. This PR depends on celestiaorg/cosmos-sdk#387 which introduces the upgrade hooks during `Commit`. This PR itself adds the two hooks for updating the stores and performing the migrations. --------- Co-authored-by: Rootul Patel <[email protected]>
0xchainlover
pushed a commit
to celestia-org/celestia-app
that referenced
this pull request
Aug 1, 2024
…ores (#3320) As was discovered with the apphash mismatch in celestiaorg/celestia-app#3167, by adding both v1 and v2 stores in the constructor we actually end up with a different app hash to `v1.x` as now there are a bunch of empy but initialized stores. What's needed is a mechanism that can start with only the v1 stores and then during the upgrade process add or remove the store for v2 as necessary. The main problem with this is that because we are using a branch of state we can't actually modify the stores until we write the new state to disk. Hence we can no longer perform the migration in `EndBlock` but in `Commit` whereby we first write state, then create the new stores, then perform the module migrations (like call `InitGenesis`) and then write it again finally returning the app hash. This PR depends on celestiaorg/cosmos-sdk#387 which introduces the upgrade hooks during `Commit`. This PR itself adds the two hooks for updating the stores and performing the migrations. --------- Co-authored-by: Rootul Patel <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses the app hash bug identified in this issue: celestiaorg/celestia-app#3167
It introduces a
migratorstruct toBaseAppwhich allows the application to handle adding and removing stores as well as performing module migrations to any store.This needs to be done in
Commitas we need to first write the changes on the current deliver tx branch of state, add and remove the stores from theMultiCommitStoreand then create a new branch of state to perform the migrations on. Only once they are completed and written can we calculate the new app hash