From 35d2e48aea1773b2972940fd8a0b33d8f4e23179 Mon Sep 17 00:00:00 2001 From: mmsqe Date: Sat, 12 Aug 2023 01:44:45 +0800 Subject: [PATCH] fix: handle RunMigrationBeginBlock error (#17372) --- CHANGELOG.md | 3 ++- baseapp/baseapp.go | 20 +++++++++++-------- .../building-modules/01-module-manager.md | 2 +- types/module/module.go | 9 +++++---- types/module/module_test.go | 10 +++++++--- 5 files changed, 27 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7000b67d8a..846de58158 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,7 +61,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/group) [#17146](https://github.com/cosmos/cosmos-sdk/pull/17146) Rename x/group legacy ORM package's error codespace from "orm" to "legacy_orm", preventing collisions with ORM's error codespace "orm". * (x/bank) [#17170](https://github.com/cosmos/cosmos-sdk/pull/17170) Avoid empty spendable error message on send coins. * (x/distribution) [#17236](https://github.com/cosmos/cosmos-sdk/pull/17236) Using "validateCommunityTax" in "Params.ValidateBasic", preventing panic when field "CommunityTax" is nil. -* (migrations) [#16583](https://github.com/cosmos/cosmos-sdk/pull/16583) Add `MigrationModuleManager` to handle migration of upgrade module before other modules, ensuring access to the updated context with consensus parameters within the same block that executes the migration. +* (x/upgrade) [#16583](https://github.com/cosmos/cosmos-sdk/pull/16583) Add `MigrationModuleManager` to handle migration of upgrade module before other modules, ensuring access to the updated context with consensus parameters within the same block that executes the migration. +* (x/upgrade) [#17372](https://github.com/cosmos/cosmos-sdk/pull/17372) Stop state machine when `RunMigrationBeginBlock` has error. ### API Breaking Changes diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 44a7fb72fa..84fb00a066 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -46,7 +46,7 @@ type ( // MigrationModuleManager is the interface that a migration module manager should implement to handle // the execution of migration logic during the beginning of a block. type MigrationModuleManager interface { - RunMigrationBeginBlock(ctx sdk.Context) bool + RunMigrationBeginBlock(ctx sdk.Context) (bool, error) } const ( @@ -686,14 +686,18 @@ func (app *BaseApp) beginBlock(req *abci.RequestFinalizeBlock) (sdk.BeginBlock, if app.beginBlocker != nil { ctx := app.finalizeBlockState.ctx - if app.migrationModuleManager != nil && app.migrationModuleManager.RunMigrationBeginBlock(ctx) { - cp := ctx.ConsensusParams() - // Manager skips this step if Block is non-nil since upgrade module is expected to set this params - // and consensus parameters should not be overwritten. - if cp.Block == nil { - if cp = app.GetConsensusParams(ctx); cp.Block != nil { - ctx = ctx.WithConsensusParams(cp) + if app.migrationModuleManager != nil { + if success, err := app.migrationModuleManager.RunMigrationBeginBlock(ctx); success { + cp := ctx.ConsensusParams() + // Manager skips this step if Block is non-nil since upgrade module is expected to set this params + // and consensus parameters should not be overwritten. + if cp.Block == nil { + if cp = app.GetConsensusParams(ctx); cp.Block != nil { + ctx = ctx.WithConsensusParams(cp) + } } + } else if err != nil { + return sdk.BeginBlock{}, err } } resp, err = app.beginBlocker(ctx) diff --git a/docs/docs/building-modules/01-module-manager.md b/docs/docs/building-modules/01-module-manager.md index de04be9c05..b1e88d5963 100644 --- a/docs/docs/building-modules/01-module-manager.md +++ b/docs/docs/building-modules/01-module-manager.md @@ -265,7 +265,7 @@ The module manager is used throughout the application whenever an action on a co * `InitGenesis(ctx context.Context, cdc codec.JSONCodec, genesisData map[string]json.RawMessage)`: Calls the [`InitGenesis`](./08-genesis.md#initgenesis) function of each module when the application is first started, in the order defined in `OrderInitGenesis`. Returns an `abci.ResponseInitChain` to the underlying consensus engine, which can contain validator updates. * `ExportGenesis(ctx context.Context, cdc codec.JSONCodec)`: Calls the [`ExportGenesis`](./08-genesis.md#exportgenesis) function of each module, in the order defined in `OrderExportGenesis`. The export constructs a genesis file from a previously existing state, and is mainly used when a hard-fork upgrade of the chain is required. * `ExportGenesisForModules(ctx context.Context, cdc codec.JSONCodec, modulesToExport []string)`: Behaves the same as `ExportGenesis`, except takes a list of modules to export. -* `RunMigrationBeginBlock(ctx sdk.Context) bool`: At the beginning of each block, this function is called from [`BaseApp`](../core/00-baseapp.md#beginblock) and, in turn, calls the [`BeginBlock`](./05-beginblock-endblock.md) function of the upgrade module implementing the `HasBeginBlocker` interface. The function returns a boolean value indicating whether the migration was successfully executed or not. +* `RunMigrationBeginBlock(ctx sdk.Context) (bool, error)`: At the beginning of each block, this function is called from [`BaseApp`](../core/00-baseapp.md#beginblock) and, in turn, calls the [`BeginBlock`](./05-beginblock-endblock.md) function of the upgrade module implementing the `HasBeginBlocker` interface. The function returns a boolean value indicating whether the migration was executed or not and an error if fails. * `BeginBlock(ctx context.Context) error`: At the beginning of each block, this function is called from [`BaseApp`](../core/00-baseapp.md#beginblock) and, in turn, calls the [`BeginBlock`](./05-beginblock-endblock.md) function of each non-upgrade modules implementing the `HasBeginBlocker` interface, in the order defined in `OrderBeginBlockers`. It creates a child [context](../core/02-context.md) with an event manager to aggregate [events](../core/08-events.md) emitted from non-upgrade modules. * `EndBlock(ctx context.Context) error`: At the end of each block, this function is called from [`BaseApp`](../core/00-baseapp.md#endblock) and, in turn, calls the [`EndBlock`](./05-beginblock-endblock.md) function of each modules implementing the `HasEndBlocker` interface, in the order defined in `OrderEndBlockers`. It creates a child [context](../core/02-context.md) with an event manager to aggregate [events](../core/08-events.md) emitted from all modules. The function returns an `abci` which contains the aforementioned events, as well as validator set updates (if any). * `EndBlock(context.Context) ([]abci.ValidatorUpdate, error)`: At the end of each block, this function is called from [`BaseApp`](../core/00-baseapp.md#endblock) and, in turn, calls the [`EndBlock`](./05-beginblock-endblock.md) function of each modules implementing the `HasEndBlocker` interface, in the order defined in `OrderEndBlockers`. It creates a child [context](../core/02-context.md) with an event manager to aggregate [events](../core/08-events.md) emitted from all modules. The function returns an `abci` which contains the aforementioned events, as well as validator set updates (if any). diff --git a/types/module/module.go b/types/module/module.go index e3abb532d5..f985d0d737 100644 --- a/types/module/module.go +++ b/types/module/module.go @@ -701,16 +701,17 @@ func (m Manager) RunMigrations(ctx context.Context, cfg Configurator, fromVM Ver // RunMigrationBeginBlock performs begin block functionality for upgrade module. // It takes the current context as a parameter and returns a boolean value -// indicating whether the migration was successfully executed or not. -func (m *Manager) RunMigrationBeginBlock(ctx sdk.Context) bool { +// indicating whether the migration was executed or not and an error if fails. +func (m *Manager) RunMigrationBeginBlock(ctx sdk.Context) (bool, error) { for _, moduleName := range m.OrderBeginBlockers { if mod, ok := m.Modules[moduleName].(appmodule.HasBeginBlocker); ok { if _, ok := mod.(UpgradeModule); ok { - return mod.BeginBlock(ctx) == nil + err := mod.BeginBlock(ctx) + return err == nil, err } } } - return false + return false, nil } // BeginBlock performs begin block functionality for non-upgrade modules. It creates a diff --git a/types/module/module_test.go b/types/module/module_test.go index 8dac4f1039..9235b420a3 100644 --- a/types/module/module_test.go +++ b/types/module/module_test.go @@ -482,16 +482,20 @@ func TestCoreAPIManager_RunMigrationBeginBlock(t *testing.T) { mockAppModule1.EXPECT().BeginBlock(gomock.Any()).Times(0) mockAppModule2.EXPECT().BeginBlock(gomock.Any()).Times(1).Return(nil) - success := mm.RunMigrationBeginBlock(sdk.Context{}) + success, err := mm.RunMigrationBeginBlock(sdk.Context{}) require.Equal(t, true, success) + require.NoError(t, err) // test false - require.Equal(t, false, module.NewManager().RunMigrationBeginBlock(sdk.Context{})) + success, err = module.NewManager().RunMigrationBeginBlock(sdk.Context{}) + require.Equal(t, false, success) + require.NoError(t, err) // test panic mockAppModule2.EXPECT().BeginBlock(gomock.Any()).Times(1).Return(errors.New("some error")) - success = mm.RunMigrationBeginBlock(sdk.Context{}) + success, err = mm.RunMigrationBeginBlock(sdk.Context{}) require.Equal(t, false, success) + require.Error(t, err) } func TestCoreAPIManager_BeginBlock(t *testing.T) {