From fea88d13c52f66e93cf5e7f72d79d47cc90bc015 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 11 Mar 2024 15:39:37 +0100 Subject: [PATCH] refactor(core)!: clean-up core and simplify preblock (#19672) --- CHANGELOG.md | 1 + baseapp/abci_test.go | 16 ++--- baseapp/baseapp.go | 17 +++-- core/CHANGELOG.md | 3 +- core/appmodule/migrations.go | 27 ++++---- core/appmodule/module.go | 63 ++++--------------- core/appmodule/v2/migrations.go | 4 +- core/appmodule/v2/{appmodule.go => module.go} | 8 +++ runtime/app.go | 2 +- runtime/services/autocli.go | 4 +- simapp/app.go | 2 +- testutil/mock/types_mock_appmodule.go | 7 +-- types/abci.go | 10 +-- types/module/configurator.go | 6 +- types/module/mock_appmodule_test.go | 4 +- types/module/module.go | 15 ++--- types/module/module_test.go | 19 ++---- x/upgrade/CHANGELOG.md | 6 +- x/upgrade/keeper/abci.go | 37 ++++------- x/upgrade/keeper/abci_test.go | 34 +++++----- x/upgrade/module.go | 2 +- 21 files changed, 110 insertions(+), 177 deletions(-) rename core/appmodule/v2/{appmodule.go => module.go} (93%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e96570e82c..7c08fd6566c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i ### Improvements +* (types) [#19672](https://github.com/cosmos/cosmos-sdk/pull/19672) `PreBlock` now returns only an error for consistency with server/v2. The SDK has upgraded x/upgrade accordingly. `ResponsePreBlock` hence has been removed. * (server) [#19455](https://github.com/cosmos/cosmos-sdk/pull/19455) Allow calling back into the application struct in PostSetup. * (types) [#19512](https://github.com/cosmos/cosmos-sdk/pull/19512) The notion of basic manager does not exist anymore. * The module manager now can do everything that the basic manager was doing. diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index 3fd7d86bb64..33c4d75284b 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -2041,11 +2041,9 @@ func TestBaseApp_PreBlocker(t *testing.T) { require.NoError(t, err) wasHookCalled := false - app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) { + app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error { wasHookCalled = true - return &sdk.ResponsePreBlock{ - ConsensusParamsChanged: true, - }, nil + return nil }) app.Seal() @@ -2058,8 +2056,8 @@ func TestBaseApp_PreBlocker(t *testing.T) { _, err = app.InitChain(&abci.RequestInitChain{}) require.NoError(t, err) - app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) { - return nil, errors.New("some error") + app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error { + return errors.New("some error") }) app.Seal() @@ -2148,7 +2146,7 @@ func TestBaseApp_VoteExtensions(t *testing.T) { return &abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT}, nil }) - app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) { + app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error { count := uint64(0) pricesSum := uint64(0) for _, v := range req.Txs { @@ -2167,9 +2165,7 @@ func TestBaseApp_VoteExtensions(t *testing.T) { ctx.KVStore(capKey1).Set([]byte("avgPrice"), buf) } - return &sdk.ResponsePreBlock{ - ConsensusParamsChanged: true, - }, nil + return nil }) } diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 672e5a6dea3..2cef05e7ccd 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -706,19 +706,16 @@ func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context func (app *BaseApp) preBlock(req *abci.RequestFinalizeBlock) error { if app.preBlocker != nil { ctx := app.finalizeBlockState.Context() - rsp, err := app.preBlocker(ctx, req) - if err != nil { + if err := app.preBlocker(ctx, req); err != nil { return err } - // rsp.ConsensusParamsChanged is true from preBlocker means ConsensusParams in store get changed + // ConsensusParams can change in preblocker, so we need to // write the consensus parameters in store to context - if rsp.ConsensusParamsChanged { - ctx = ctx.WithConsensusParams(app.GetConsensusParams(ctx)) - // GasMeter must be set after we get a context with updated consensus params. - gasMeter := app.getBlockGasMeter(ctx) - ctx = ctx.WithBlockGasMeter(gasMeter) - app.finalizeBlockState.SetContext(ctx) - } + ctx = ctx.WithConsensusParams(app.GetConsensusParams(ctx)) + // GasMeter must be set after we get a context with updated consensus params. + gasMeter := app.getBlockGasMeter(ctx) + ctx = ctx.WithBlockGasMeter(gasMeter) + app.finalizeBlockState.SetContext(ctx) } return nil } diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index 2f556daf6a2..d44c84f4c1a 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -54,10 +54,9 @@ Ref: https://keepachangelog.com/en/1.0.0/ * Add `MsgHandler` as an alternative to grpc handlers * Provide separate `MigrationRegistrar` instead of grouping with `RegisterServices` -### Improvements - ### API Breaking Changes +* [#19672](https://github.com/cosmos/cosmos-sdk/pull/19672) `PreBlock` now returns only an error for consistency with server/v2. The SDK has upgraded x/upgrade accordingly. * [#18857](https://github.com/cosmos/cosmos-sdk/pull/18857) Moved `FormatCoins` to `x/tx`. * [#18861](httpes://github.com/cosmos/cosmos-sdk/pull/18861) Moved `coin.ParseCoin` to `client/v2/internal`. * [#18866](https://github.com/cosmos/cosmos-sdk/pull/18866) All items related to depinject have been moved to `cosmossdk.io/depinject` (`Provide`, `Invoke`, `Register`) diff --git a/core/appmodule/migrations.go b/core/appmodule/migrations.go index 63c724108b6..e5fe6fbcfaf 100644 --- a/core/appmodule/migrations.go +++ b/core/appmodule/migrations.go @@ -1,16 +1,17 @@ package appmodule -import "context" +import ( + "cosmossdk.io/core/appmodule/v2" +) -type MigrationRegistrar interface { - // Register registers an in-place store migration for a module. The - // handler is a migration script to perform in-place migrations from version - // `fromVersion` to version `fromVersion+1`. - // - // EACH TIME a module's ConsensusVersion increments, a new migration MUST - // be registered using this function. If a migration handler is missing for - // a particular function, the upgrade logic (see RunMigrations function) - // will panic. If the ConsensusVersion bump does not introduce any store - // changes, then a no-op function must be registered here. - Register(moduleName string, fromVersion uint64, handler func(context.Context) error) error -} +// HasConsensusVersion is the interface for declaring a module consensus version. +type HasConsensusVersion = appmodule.HasConsensusVersion + +// HasMigrations is implemented by a module which upgrades or has upgraded to a new consensus version. +type HasMigrations = appmodule.HasMigrations + +// MigrationRegistrar is the interface for registering in-place store migrations. +type MigrationRegistrar = appmodule.MigrationRegistrar + +// MigrationHandler is the migration function that each module registers. +type MigrationHandler = appmodule.MigrationHandler diff --git a/core/appmodule/module.go b/core/appmodule/module.go index 4646d11d17b..60c4f80c355 100644 --- a/core/appmodule/module.go +++ b/core/appmodule/module.go @@ -4,7 +4,6 @@ import ( "context" "google.golang.org/grpc" - "google.golang.org/protobuf/runtime/protoiface" "cosmossdk.io/core/appmodule/v2" ) @@ -15,16 +14,20 @@ import ( // by other modules (usually via depinject) as app modules. type AppModule = appmodule.AppModule -// HasMigrations is the extension interface that modules should implement to register migrations. -type HasMigrations interface { - AppModule +// HasPreBlocker is the extension interface that modules should implement to run +// custom logic before BeginBlock. +type HasPreBlocker = appmodule.HasPreBlocker - // RegisterMigrations registers the module's migrations with the app's migrator. - RegisterMigrations(MigrationRegistrar) error -} +// HasBeginBlocker is the extension interface that modules should implement to run +// custom logic before transaction processing in a block. +type HasBeginBlocker = appmodule.HasBeginBlocker + +// HasEndBlocker is the extension interface that modules should implement to run +// custom logic after transaction processing in a block. +type HasEndBlocker = appmodule.HasEndBlocker -// HasConsensusVersion is the interface for declaring a module consensus version. -type HasConsensusVersion = appmodule.HasConsensusVersion +// HasRegisterInterfaces is the interface for modules to register their msg types. +type HasRegisterInterfaces = appmodule.HasRegisterInterfaces // HasServices is the extension interface that modules should implement to register // implementations of services defined in .proto files. @@ -46,48 +49,6 @@ type HasServices interface { RegisterServices(grpc.ServiceRegistrar) error } -// ResponsePreBlock represents the response from the PreBlock method. -// It can modify consensus parameters in storage and signal the caller through the return value. -// When it returns ConsensusParamsChanged=true, the caller must refresh the consensus parameter in the finalize context. -// The new context (ctx) must be passed to all the other lifecycle methods. -type ResponsePreBlock interface { - IsConsensusParamsChanged() bool -} - -// HasPreBlocker is the extension interface that modules should implement to run -// custom logic before BeginBlock. -type HasPreBlocker interface { - appmodule.AppModule - // PreBlock is method that will be run before BeginBlock. - PreBlock(context.Context) (ResponsePreBlock, error) -} - -// HasBeginBlocker is the extension interface that modules should implement to run -// custom logic before transaction processing in a block. -type HasBeginBlocker = appmodule.HasBeginBlocker - -// HasEndBlocker is the extension interface that modules should implement to run -// custom logic after transaction processing in a block. -type HasEndBlocker = appmodule.HasEndBlocker - -// HasRegisterInterfaces is the interface for modules to register their msg types. -type HasRegisterInterfaces = appmodule.HasRegisterInterfaces - -// MsgHandlerRouter is implemented by the runtime provider. -type MsgHandlerRouter interface { - // RegisterHandler is called by modules to register msg handler functions. - RegisterHandler(name string, handler func(ctx context.Context, msg protoiface.MessageV1) (msgResp protoiface.MessageV1, err error)) -} - -// HasMsgHandler is implemented by modules that instead of exposing msg server expose -// a set of handlers. -type HasMsgHandler interface { - // RegisterMsgHandlers is implemented by the module that will register msg handlers. - RegisterMsgHandlers(router MsgHandlerRouter) -} - -// ---------------------------------------------------------------------------- // - // HasPrepareCheckState is an extension interface that contains information about the AppModule // and PrepareCheckState. type HasPrepareCheckState interface { diff --git a/core/appmodule/v2/migrations.go b/core/appmodule/v2/migrations.go index 794fb4a28cc..3e62ad7dfee 100644 --- a/core/appmodule/v2/migrations.go +++ b/core/appmodule/v2/migrations.go @@ -11,8 +11,7 @@ type HasConsensusVersion interface { ConsensusVersion() uint64 } -// HasMigrations is implemented by a module which upgrades or has upgraded -// to a new consensus version. +// HasMigrations is implemented by a module which upgrades or has upgraded to a new consensus version. type HasMigrations interface { AppModule HasConsensusVersion @@ -21,6 +20,7 @@ type HasMigrations interface { RegisterMigrations(MigrationRegistrar) error } +// MigrationRegistrar is the interface for registering in-place store migrations. type MigrationRegistrar interface { // Register registers an in-place store migration for a module. The // handler is a migration script to perform in-place migrations from version diff --git a/core/appmodule/v2/appmodule.go b/core/appmodule/v2/module.go similarity index 93% rename from core/appmodule/v2/appmodule.go rename to core/appmodule/v2/module.go index b0bfb19d2fc..2fe0cc99f07 100644 --- a/core/appmodule/v2/appmodule.go +++ b/core/appmodule/v2/module.go @@ -19,6 +19,14 @@ type AppModule interface { IsOnePerModuleType() } +// HasPreBlocker is the extension interface that modules should implement to run +// custom logic before BeginBlock. +type HasPreBlocker interface { + AppModule + // PreBlock is method that will be run before BeginBlock. + PreBlock(context.Context) error +} + // HasBeginBlocker is the extension interface that modules should implement to run // custom logic before transaction processing in a block. type HasBeginBlocker interface { diff --git a/runtime/app.go b/runtime/app.go index c5ac9329d9e..6fdfe08ca1f 100644 --- a/runtime/app.go +++ b/runtime/app.go @@ -153,7 +153,7 @@ func (a *App) Load(loadLatest bool) error { } // PreBlocker application updates every pre block -func (a *App) PreBlocker(ctx sdk.Context, _ *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) { +func (a *App) PreBlocker(ctx sdk.Context, _ *abci.RequestFinalizeBlock) error { return a.ModuleManager.PreBlock(ctx) } diff --git a/runtime/services/autocli.go b/runtime/services/autocli.go index e8a4682e373..b130af0f1c1 100644 --- a/runtime/services/autocli.go +++ b/runtime/services/autocli.go @@ -113,7 +113,9 @@ func (a *autocliConfigurator) RegisterMigration(string, uint64, module.Migration return nil } -func (a *autocliConfigurator) Register(string, uint64, func(context.Context) error) error { return nil } +func (a *autocliConfigurator) Register(string, uint64, appmodule.MigrationHandler) error { + return nil +} func (a *autocliConfigurator) RegisterService(sd *grpc.ServiceDesc, ss interface{}) { if a.registryCache == nil { diff --git a/simapp/app.go b/simapp/app.go index 59e7b03410e..3304c335b4e 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -615,7 +615,7 @@ func (app *SimApp) Close() error { func (app *SimApp) Name() string { return app.BaseApp.Name() } // PreBlocker application updates every pre block -func (app *SimApp) PreBlocker(ctx sdk.Context, _ *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) { +func (app *SimApp) PreBlocker(ctx sdk.Context, _ *abci.RequestFinalizeBlock) error { return app.ModuleManager.PreBlock(ctx) } diff --git a/testutil/mock/types_mock_appmodule.go b/testutil/mock/types_mock_appmodule.go index 13f1c97b712..6118f774fd9 100644 --- a/testutil/mock/types_mock_appmodule.go +++ b/testutil/mock/types_mock_appmodule.go @@ -659,12 +659,11 @@ func (mr *MockCoreAppModuleWithPreBlockMockRecorder) IsOnePerModuleType() *gomoc } // PreBlock mocks base method. -func (m *MockCoreAppModuleWithPreBlock) PreBlock(arg0 context.Context) (appmodule.ResponsePreBlock, error) { +func (m *MockCoreAppModuleWithPreBlock) PreBlock(arg0 context.Context) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "PreBlock", arg0) - ret0, _ := ret[0].(appmodule.ResponsePreBlock) - ret1, _ := ret[1].(error) - return ret0, ret1 + ret0, _ := ret[0].(error) + return ret0 } // PreBlock indicates an expected call of PreBlock. diff --git a/types/abci.go b/types/abci.go index 8325f5dadf0..701772806d9 100644 --- a/types/abci.go +++ b/types/abci.go @@ -36,7 +36,7 @@ type VerifyVoteExtensionHandler func(Context, *abci.RequestVerifyVoteExtension) // persist their results in state. // // Note: returning an error will make FinalizeBlock fail. -type PreBlocker func(Context, *abci.RequestFinalizeBlock) (*ResponsePreBlock, error) +type PreBlocker func(Context, *abci.RequestFinalizeBlock) error // BeginBlocker defines a function type alias for executing application // business logic before transactions are executed. @@ -66,11 +66,3 @@ type EndBlock struct { type BeginBlock struct { Events []abci.Event } - -type ResponsePreBlock struct { - ConsensusParamsChanged bool -} - -func (r ResponsePreBlock) IsConsensusParamsChanged() bool { - return r.ConsensusParamsChanged -} diff --git a/types/module/configurator.go b/types/module/configurator.go index 0ef80fca96a..06d648ff1ff 100644 --- a/types/module/configurator.go +++ b/types/module/configurator.go @@ -1,7 +1,6 @@ package module import ( - "context" "fmt" "github.com/cosmos/gogoproto/grpc" @@ -10,6 +9,7 @@ import ( "google.golang.org/protobuf/reflect/protoreflect" cosmosmsg "cosmossdk.io/api/cosmos/msg/v1" + "cosmossdk.io/core/appmodule" errorsmod "cosmossdk.io/errors" "github.com/cosmos/cosmos-sdk/codec" @@ -47,7 +47,7 @@ type Configurator interface { // Register registers an in-place store migration for a module. // It permits to register modules migrations that have migrated to serverv2 but still be compatible with baseapp. - Register(moduleName string, fromVersion uint64, handler func(context.Context) error) error + Register(moduleName string, fromVersion uint64, handler appmodule.MigrationHandler) error } type configurator struct { @@ -124,7 +124,7 @@ func (c *configurator) RegisterMigration(moduleName string, fromVersion uint64, // Register implements the Configurator.Register method // It allows to register modules migrations that have migrated to server/v2 but still be compatible with baseapp. -func (c *configurator) Register(moduleName string, fromVersion uint64, handler func(context.Context) error) error { +func (c *configurator) Register(moduleName string, fromVersion uint64, handler appmodule.MigrationHandler) error { return c.RegisterMigration(moduleName, fromVersion, func(sdkCtx sdk.Context) error { return handler(sdkCtx) }) diff --git a/types/module/mock_appmodule_test.go b/types/module/mock_appmodule_test.go index 848280147fb..6d470b7d64a 100644 --- a/types/module/mock_appmodule_test.go +++ b/types/module/mock_appmodule_test.go @@ -1,3 +1,5 @@ +// module_test inconsistenctly import appmodulev2 & appmodulev1 due to limitation in mockgen +// eventually, when we change mocking library, we should be consistent in our appmodule imports package module_test import ( @@ -43,5 +45,5 @@ type CoreAppModule interface { type CoreAppModuleWithPreBlock interface { CoreAppModule - appmodule.HasPreBlocker + appmodulev2.HasPreBlocker } diff --git a/types/module/module.go b/types/module/module.go index 9bad87df98a..6dd626f7cd2 100644 --- a/types/module/module.go +++ b/types/module/module.go @@ -706,23 +706,16 @@ func (m Manager) RunMigrations(ctx context.Context, cfg Configurator, fromVM Ver // PreBlock 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) PreBlock(ctx sdk.Context) (*sdk.ResponsePreBlock, error) { +func (m *Manager) PreBlock(ctx sdk.Context) error { ctx = ctx.WithEventManager(sdk.NewEventManager()) - paramsChanged := false for _, moduleName := range m.OrderPreBlockers { if module, ok := m.Modules[moduleName].(appmodule.HasPreBlocker); ok { - rsp, err := module.PreBlock(ctx) - if err != nil { - return nil, err - } - if rsp.IsConsensusParamsChanged() { - paramsChanged = true + if err := module.PreBlock(ctx); err != nil { + return err } } } - return &sdk.ResponsePreBlock{ - ConsensusParamsChanged: paramsChanged, - }, nil + return nil } // BeginBlock performs begin block functionality for all modules. It creates a diff --git a/types/module/module_test.go b/types/module/module_test.go index 373d9cca68c..670b088d536 100644 --- a/types/module/module_test.go +++ b/types/module/module_test.go @@ -418,24 +418,13 @@ func TestCoreAPIManager_PreBlock(t *testing.T) { require.Equal(t, 2, len(mm.Modules)) require.Equal(t, 1, len(mm.OrderPreBlockers)) - mockAppModule1.EXPECT().PreBlock(gomock.Any()).Times(1).Return(&sdk.ResponsePreBlock{ - ConsensusParamsChanged: true, - }, nil) - res, err := mm.PreBlock(sdk.Context{}) + mockAppModule1.EXPECT().PreBlock(gomock.Any()).Times(1).Return(nil) + err := mm.PreBlock(sdk.Context{}) require.NoError(t, err) - require.True(t, res.ConsensusParamsChanged) - - // test false - mockAppModule1.EXPECT().PreBlock(gomock.Any()).Times(1).Return(&sdk.ResponsePreBlock{ - ConsensusParamsChanged: false, - }, nil) - res, err = mm.PreBlock(sdk.Context{}) - require.NoError(t, err) - require.False(t, res.ConsensusParamsChanged) // test error - mockAppModule1.EXPECT().PreBlock(gomock.Any()).Times(1).Return(nil, errors.New("some error")) - _, err = mm.PreBlock(sdk.Context{}) + mockAppModule1.EXPECT().PreBlock(gomock.Any()).Times(1).Return(errors.New("some error")) + err = mm.PreBlock(sdk.Context{}) require.EqualError(t, err, "some error") } diff --git a/x/upgrade/CHANGELOG.md b/x/upgrade/CHANGELOG.md index 7ff56275c00..94422712d9f 100644 --- a/x/upgrade/CHANGELOG.md +++ b/x/upgrade/CHANGELOG.md @@ -25,13 +25,17 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] +### Improvements + +* [#19672](https://github.com/cosmos/cosmos-sdk/pull/19672) Follow latest `cosmossdk.io/core` `PreBlock` simplification. + ### State Machine Breaking * (x/upgrade) [#16244](https://github.com/cosmos/cosmos-sdk/pull/16244) Upgrade module no longer stores the app version but gets and sets the app version stored in the `ParamStore` of baseapp. ### API Breaking Changes -* [#19443](https://github.com/cosmos/cosmos-sdk/pull/19443) Creation of upgrade module receives `appmodule.Environment` instead of individual services +* [#19443](https://github.com/cosmos/cosmos-sdk/pull/19443) `NewKeeper` takes an `appmodule.Environment` instead of individual services. ## [v0.1.1](https://github.com/cosmos/cosmos-sdk/releases/tag/x/upgrade/v0.1.1) - 2023-12-11 diff --git a/x/upgrade/keeper/abci.go b/x/upgrade/keeper/abci.go index cd867f9f654..6ef39c8062b 100644 --- a/x/upgrade/keeper/abci.go +++ b/x/upgrade/keeper/abci.go @@ -6,7 +6,6 @@ import ( "fmt" "time" - "cosmossdk.io/core/appmodule" storetypes "cosmossdk.io/store/types" "cosmossdk.io/x/upgrade/types" @@ -22,13 +21,13 @@ import ( // The purpose is to ensure the binary is switched EXACTLY at the desired block, and to allow // a migration to be executed if needed upon this switch (migration defined in the new binary) // skipUpgradeHeightArray is a set of block heights for which the upgrade must be skipped -func (k Keeper) PreBlocker(ctx context.Context) (appmodule.ResponsePreBlock, error) { +func (k Keeper) PreBlocker(ctx context.Context) error { defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) blockHeight := k.environment.HeaderService.GetHeaderInfo(ctx).Height plan, err := k.GetUpgradePlan(ctx) if err != nil && !errors.Is(err, types.ErrNoUpgradePlanFound) { - return nil, err + return err } found := err == nil @@ -43,7 +42,7 @@ func (k Keeper) PreBlocker(ctx context.Context) (appmodule.ResponsePreBlock, err if !found || !plan.ShouldExecute(blockHeight) || (plan.ShouldExecute(blockHeight) && k.IsSkipHeight(blockHeight)) { lastAppliedPlan, _, err := k.GetLastCompletedUpgrade(ctx) if err != nil { - return nil, err + return err } if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) { @@ -54,15 +53,13 @@ func (k Keeper) PreBlocker(ctx context.Context) (appmodule.ResponsePreBlock, err appVersion = cp.Version.App } - return nil, fmt.Errorf("wrong app version %d, upgrade handler is missing for %s upgrade plan", appVersion, lastAppliedPlan) + return fmt.Errorf("wrong app version %d, upgrade handler is missing for %s upgrade plan", appVersion, lastAppliedPlan) } } } if !found { - return &sdk.ResponsePreBlock{ - ConsensusParamsChanged: false, - }, nil + return nil } logger := k.Logger(ctx) @@ -76,11 +73,9 @@ func (k Keeper) PreBlocker(ctx context.Context) (appmodule.ResponsePreBlock, err // Clear the upgrade plan at current height if err := k.ClearUpgradePlan(ctx); err != nil { - return nil, err + return err } - return &sdk.ResponsePreBlock{ - ConsensusParamsChanged: false, - }, nil + return nil } // Prepare shutdown if we don't have an upgrade handler for this upgrade name (meaning this software is out of date) @@ -89,27 +84,23 @@ func (k Keeper) PreBlocker(ctx context.Context) (appmodule.ResponsePreBlock, err // store migrations. err := k.DumpUpgradeInfoToDisk(blockHeight, plan) if err != nil { - return nil, fmt.Errorf("unable to write upgrade info to filesystem: %w", err) + return fmt.Errorf("unable to write upgrade info to filesystem: %w", err) } upgradeMsg := BuildUpgradeNeededMsg(plan) logger.Error(upgradeMsg) // Returning an error will end up in a panic - return nil, errors.New(upgradeMsg) + return errors.New(upgradeMsg) } // We have an upgrade handler for this upgrade name, so apply the upgrade logger.Info(fmt.Sprintf("applying upgrade \"%s\" at %s", plan.Name, plan.DueAt())) sdkCtx = sdkCtx.WithBlockGasMeter(storetypes.NewInfiniteGasMeter()) if err := k.ApplyUpgrade(sdkCtx, plan); err != nil { - return nil, err + return err } - return &sdk.ResponsePreBlock{ - // the consensus parameters might be modified in the migration, - // refresh the consensus parameters in context. - ConsensusParamsChanged: true, - }, nil + return nil } // if we have a pending upgrade, but it is not yet time, make sure we did not @@ -119,11 +110,9 @@ func (k Keeper) PreBlocker(ctx context.Context) (appmodule.ResponsePreBlock, err logger.Error(downgradeMsg) // Returning an error will end up in a panic - return nil, errors.New(downgradeMsg) + return errors.New(downgradeMsg) } - return &sdk.ResponsePreBlock{ - ConsensusParamsChanged: false, - }, nil + return nil } // BuildUpgradeNeededMsg prints the message that notifies that an upgrade is needed. diff --git a/x/upgrade/keeper/abci_test.go b/x/upgrade/keeper/abci_test.go index 831d39cf195..66c10f3cbd4 100644 --- a/x/upgrade/keeper/abci_test.go +++ b/x/upgrade/keeper/abci_test.go @@ -45,7 +45,7 @@ func (s *TestSuite) VerifyDoUpgrade(t *testing.T) { t.Log("Verify that a panic happens at the upgrade height") newCtx := s.ctx.WithHeaderInfo(header.Info{Height: s.ctx.HeaderInfo().Height + 1, Time: time.Now()}) - _, err := s.preModule.PreBlock(newCtx) + err := s.preModule.PreBlock(newCtx) require.ErrorContains(t, err, "UPGRADE \"test\" NEEDED at height: 11: ") t.Log("Verify that the upgrade can be successfully applied with a handler") @@ -53,7 +53,7 @@ func (s *TestSuite) VerifyDoUpgrade(t *testing.T) { return vm, nil }) - _, err = s.preModule.PreBlock(newCtx) + err = s.preModule.PreBlock(newCtx) require.NoError(t, err) s.VerifyCleared(t, newCtx) @@ -63,7 +63,7 @@ func (s *TestSuite) VerifyDoUpgradeWithCtx(t *testing.T, newCtx sdk.Context, pro t.Helper() t.Log("Verify that a panic happens at the upgrade height") - _, err := s.preModule.PreBlock(newCtx) + err := s.preModule.PreBlock(newCtx) require.ErrorContains(t, err, "UPGRADE \""+proposalName+"\" NEEDED at height: ") t.Log("Verify that the upgrade can be successfully applied with a handler") @@ -71,7 +71,7 @@ func (s *TestSuite) VerifyDoUpgradeWithCtx(t *testing.T, newCtx sdk.Context, pro return vm, nil }) - _, err = s.preModule.PreBlock(newCtx) + err = s.preModule.PreBlock(newCtx) require.NoError(t, err) s.VerifyCleared(t, newCtx) @@ -175,21 +175,21 @@ func TestHaltIfTooNew(t *testing.T) { }) newCtx := s.ctx.WithHeaderInfo(header.Info{Height: s.ctx.HeaderInfo().Height + 1, Time: time.Now()}) - _, err := s.preModule.PreBlock(newCtx) + err := s.preModule.PreBlock(newCtx) require.NoError(t, err) require.Equal(t, 0, called) t.Log("Verify we error if we have a registered handler ahead of time") err = s.keeper.ScheduleUpgrade(s.ctx, types.Plan{Name: "future", Height: s.ctx.HeaderInfo().Height + 3}) require.NoError(t, err) - _, err = s.preModule.PreBlock(newCtx) + err = s.preModule.PreBlock(newCtx) require.EqualError(t, err, "BINARY UPDATED BEFORE TRIGGER! UPGRADE \"future\" - in binary but not executed on chain. Downgrade your binary") require.Equal(t, 0, called) t.Log("Verify we no longer panic if the plan is on time") futCtx := s.ctx.WithHeaderInfo(header.Info{Height: s.ctx.HeaderInfo().Height + 3, Time: time.Now()}) - _, err = s.preModule.PreBlock(futCtx) + err = s.preModule.PreBlock(futCtx) require.NoError(t, err) require.Equal(t, 1, called) @@ -223,7 +223,7 @@ func TestCantApplySameUpgradeTwice(t *testing.T) { func TestNoSpuriousUpgrades(t *testing.T) { s := setupTest(t, 10, map[int64]bool{}) t.Log("Verify that no upgrade panic is triggered in the BeginBlocker when we haven't scheduled an upgrade") - _, err := s.preModule.PreBlock(s.ctx) + err := s.preModule.PreBlock(s.ctx) require.NoError(t, err) } @@ -260,7 +260,7 @@ func TestSkipUpgradeSkippingAll(t *testing.T) { s.VerifySet(t, map[int64]bool{skipOne: true, skipTwo: true}) newCtx = newCtx.WithHeaderInfo(header.Info{Height: skipOne}) - _, err = s.preModule.PreBlock(newCtx) + err = s.preModule.PreBlock(newCtx) require.NoError(t, err) t.Log("Verify a second proposal also is being cleared") @@ -268,7 +268,7 @@ func TestSkipUpgradeSkippingAll(t *testing.T) { require.NoError(t, err) newCtx = newCtx.WithHeaderInfo(header.Info{Height: skipTwo}) - _, err = s.preModule.PreBlock(newCtx) + err = s.preModule.PreBlock(newCtx) require.NoError(t, err) // To ensure verification is being done only after both upgrades are cleared @@ -295,7 +295,7 @@ func TestUpgradeSkippingOne(t *testing.T) { // Setting block height of proposal test newCtx = newCtx.WithHeaderInfo(header.Info{Height: skipOne}) - _, err = s.preModule.PreBlock(newCtx) + err = s.preModule.PreBlock(newCtx) require.NoError(t, err) t.Log("Verify the second proposal is not skipped") @@ -328,7 +328,7 @@ func TestUpgradeSkippingOnlyTwo(t *testing.T) { // Setting block height of proposal test newCtx = newCtx.WithHeaderInfo(header.Info{Height: skipOne}) - _, err = s.preModule.PreBlock(newCtx) + err = s.preModule.PreBlock(newCtx) require.NoError(t, err) // A new proposal with height in skipUpgradeHeights @@ -336,7 +336,7 @@ func TestUpgradeSkippingOnlyTwo(t *testing.T) { require.NoError(t, err) // Setting block height of proposal test2 newCtx = newCtx.WithHeaderInfo(header.Info{Height: skipTwo}) - _, err = s.preModule.PreBlock(newCtx) + err = s.preModule.PreBlock(newCtx) require.NoError(t, err) t.Log("Verify a new proposal is not skipped") @@ -357,7 +357,7 @@ func TestUpgradeWithoutSkip(t *testing.T) { err := s.keeper.ScheduleUpgrade(s.ctx, types.Plan{Name: "test", Height: s.ctx.HeaderInfo().Height + 1}) require.NoError(t, err) t.Log("Verify if upgrade happens without skip upgrade") - _, err = s.preModule.PreBlock(newCtx) + err = s.preModule.PreBlock(newCtx) require.ErrorContains(t, err, "UPGRADE \"test\" NEEDED at height:") s.VerifyDoUpgrade(t) @@ -447,7 +447,7 @@ func TestBinaryVersion(t *testing.T) { for _, tc := range testCases { ctx := tc.preRun() - _, err := s.preModule.PreBlock(ctx) + err := s.preModule.PreBlock(ctx) if tc.expectError { require.Error(t, err) } else { @@ -486,7 +486,7 @@ func TestDowngradeVerification(t *testing.T) { }) // successful upgrade. - _, err = m.PreBlock(ctx) + err = m.PreBlock(ctx) require.NoError(t, err) ctx = ctx.WithHeaderInfo(header.Info{Height: ctx.HeaderInfo().Height + 1}) @@ -536,7 +536,7 @@ func TestDowngradeVerification(t *testing.T) { tc.preRun(k, ctx, name) } - _, err = m.PreBlock(ctx) + err = m.PreBlock(ctx) if tc.expectError { require.Error(t, err, name) } else { diff --git a/x/upgrade/module.go b/x/upgrade/module.go index 7b0f1a53f32..7b7dbc50ec4 100644 --- a/x/upgrade/module.go +++ b/x/upgrade/module.go @@ -151,6 +151,6 @@ func (AppModule) ConsensusVersion() uint64 { return ConsensusVersion } // PreBlock calls the upgrade module hooks // // CONTRACT: this is called *before* all other modules' BeginBlock functions -func (am AppModule) PreBlock(ctx context.Context) (appmodule.ResponsePreBlock, error) { +func (am AppModule) PreBlock(ctx context.Context) error { return am.keeper.PreBlocker(ctx) }