fix: export command #3450
fix: export command #3450
Conversation
… of store blob mismatch root store's version; expected 1 got 0
|
Why is the |
…een registered in stores
app/export.go
Outdated
| app.InitializeAppVersion(ctx) | ||
| app.mountKeysAndInit(app.AppVersion()) |
There was a problem hiding this comment.
This is important because without this, we couldn't call export on the app because it wasn't fully initialized. A recent PR made it so that the app needs to have it's app version initialized + key stores mounted after app.New is invoked.
| return servertypes.ExportedApp{ | ||
| AppState: appState, | ||
| Validators: validators, | ||
| Height: height, | ||
| Height: app.getExportHeight(forZeroHeight), |
There was a problem hiding this comment.
[refactor] this extracts logic to a helper to make it easier to read
| moduleNamesForVersion := m.ModuleNames(version) | ||
| moduleNamesToExport := filter(m.OrderExportGenesis, func(moduleName string) bool { | ||
| // filter out modules that are not supported by this version | ||
| return slices.Contains(moduleNamesForVersion, moduleName) | ||
| }) | ||
| for _, moduleName := range moduleNamesToExport { | ||
| genesisData[moduleName] = modules[moduleName].ExportGenesis(ctx, cdc) |
There was a problem hiding this comment.
this fixes a bug so that ExportGenesis is only invoked on the modules that are supported for the given version
| } | ||
| require.Equal(t, want, mm.ExportGenesis(ctx, cdc, 1)) | ||
| }) | ||
| t.Run("export genesis with one modules at version 1, one modules at version 2", func(t *testing.T) { |
There was a problem hiding this comment.
unit tests the fix
| func createAppAndExport( | ||
| logger log.Logger, db dbm.DB, traceStore io.Writer, height int64, forZeroHeight bool, jailWhiteList []string, | ||
| logger log.Logger, | ||
| db dbm.DB, | ||
| traceStore io.Writer, | ||
| height int64, | ||
| forZeroHeight bool, | ||
| jailWhiteList []string, | ||
| appOpts servertypes.AppOptions, | ||
| ) (servertypes.ExportedApp, error) { | ||
| encCfg := encoding.MakeConfig(app.ModuleEncodingRegisters...) // Ideally, we would reuse the one created by NewRootCmd. | ||
| encCfg.Codec = codec.NewProtoCodec(encCfg.InterfaceRegistry) | ||
| var capp *app.App | ||
| config := encoding.MakeConfig(app.ModuleEncodingRegisters...) | ||
| celestiaApp := app.New(logger, db, traceStore, uint(1), config, 0, appOpts) | ||
| if height != -1 { | ||
| capp = app.New(logger, db, traceStore, uint(1), encCfg, 0, appOpts) | ||
|
|
||
| if err := capp.LoadHeight(height); err != nil { | ||
| if err := celestiaApp.LoadHeight(height); err != nil { | ||
| return servertypes.ExportedApp{}, err | ||
| } | ||
| } else { | ||
| capp = app.New(logger, db, traceStore, uint(1), encCfg, 0, appOpts) | ||
| } | ||
|
|
||
| return capp.ExportAppStateAndValidators(forZeroHeight, jailWhiteList) | ||
| return celestiaApp.ExportAppStateAndValidators(forZeroHeight, jailWhiteList) |
There was a problem hiding this comment.
refactored for readability
go.mod
Outdated
| @@ -257,7 +257,7 @@ require ( | |||
| ) | |||
|
|
|||
| replace ( | |||
| github.com/cosmos/cosmos-sdk => github.com/celestiaorg/cosmos-sdk v1.22.0-sdk-v0.46.16 | |||
| github.com/cosmos/cosmos-sdk => /Users/rootulp/git/rootulp/celestiaorg/cosmos-sdk | |||
|
|
Strange because InitGenesis is called on the minfee module when upgrading from app version 1 -> 2. The param does exist in the correct subspace: It looks like the export command isn't loading the correct params |
WalkthroughWalkthroughThe recent changes aim to enhance the Celestia application by refining initialization processes, improving export functionalities, and enhancing version handling. These updates streamline code structure, enhance method clarity, and address issues related to application state exporting for increased stability and reliability. Changes
Assessment against linked issues
The recent modifications enhance overall application functionality and export processes, but they do not explicitly address the nil pointer dereference issue causing panics or provide specific resolutions for identified bugs and inconsistencies. Further clarification is needed to ensure the resolution of these critical issues. Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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
Out of diff range and nitpick comments (3)
app/app.go (3)
172-176: Clarify the deprecation note forupgradeHeightV2.Consider expanding the comment on line 175 to explain what the "dynamically signalling scheme" will involve, or link to documentation if this change is significant.
Line range hint
508-530: Ensure robust error handling inInfo.The function uses a panic for error handling (line 512). Consider using a more graceful error handling mechanism to improve the robustness of the application. For example, you could return an error to the caller instead of panicking.
- panic(err) + return abci.ResponseInfo{}, err
Line range hint
535-553: Review the panic usage inInitChain.The use of panic (lines 539 and 541) could be replaced with more graceful error handling. Consider returning an error to the caller or handling the error within the function to improve the application's stability.
- panic("no consensus params set") + return abci.ResponseInitChain{}, fmt.Errorf("no consensus params set") - panic("app version 0 is not accepted. Please set an app version in the genesis") + return abci.ResponseInitChain{}, fmt.Errorf("app version 0 is not accepted. Please set an app version in the genesis")
| // mountKeysAndInit mounts the keys for the provided app version and then | ||
| // invokes baseapp.Init(). | ||
| func (app *App) mountKeysAndInit(appVersion uint64) { | ||
| app.MountKVStores(app.versionedKeys(appVersion)) | ||
|
|
||
| // Invoke load latest version for it's side-effect of invoking baseapp.Init() | ||
| if err := app.LoadLatestVersion(); err != nil { | ||
| panic(fmt.Sprintf("loading latest version: %s", err.Error())) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure error messages are informative in mountKeysAndInit.
The error message in line 566 could be more informative. Consider including more context about the failure to help with debugging.
- panic(fmt.Sprintf("loading latest version: %s", err.Error()))
+ panic(fmt.Sprintf("Failed to load the latest version during app initialization: %s", err.Error()))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.
| // mountKeysAndInit mounts the keys for the provided app version and then | |
| // invokes baseapp.Init(). | |
| func (app *App) mountKeysAndInit(appVersion uint64) { | |
| app.MountKVStores(app.versionedKeys(appVersion)) | |
| // Invoke load latest version for it's side-effect of invoking baseapp.Init() | |
| if err := app.LoadLatestVersion(); err != nil { | |
| panic(fmt.Sprintf("loading latest version: %s", err.Error())) | |
| } | |
| } | |
| // mountKeysAndInit mounts the keys for the provided app version and then | |
| // invokes baseapp.Init(). | |
| func (app *App) mountKeysAndInit(appVersion uint64) { | |
| app.MountKVStores(app.versionedKeys(appVersion)) | |
| // Invoke load latest version for it's side-effect of invoking baseapp.Init() | |
| if err := app.LoadLatestVersion(); err != nil { | |
| panic(fmt.Sprintf("Failed to load the latest version during app initialization: %s", err.Error())) | |
| } | |
| } |
| exported, err := testApp.ExportAppStateAndValidators(forZeroHeight, jailAllowedAddrs) | ||
| require.NoError(t, err) | ||
| assert.NotNil(t, exported) | ||
| // TODO: the following assertion is commented out because the exported app does not populate consensus params.version |
There was a problem hiding this comment.
TODO: Complete the test for consensus parameters version in the exported app state.
Would you like me to help complete this test case or should we track this as an issue?
celestia-bot
requested review from
a team and
ramin
and removed request for
a team
| moduleNamesForVersion := m.ModuleNames(version) | ||
| moduleNamesToExport := filter(m.OrderExportGenesis, func(moduleName string) bool { | ||
| // filter out modules that are not supported by this version | ||
| return slices.Contains(moduleNamesForVersion, moduleName) | ||
| }) | ||
| for _, moduleName := range moduleNamesToExport { | ||
| genesisData[moduleName] = modules[moduleName].ExportGenesis(ctx, cdc) |
Closes #3392
Opens #3472
Fixes a few bugs:
ExportGenesisinvoked on them even if they weren't supported by the current app version. Now we only callExportGenesisfor the modules that are supported by the current app versionapp.New()based on the current app versionExportGenesisTesting
I could export an app on app version 1 and 2. See output