From 3975577940f1c39020f650277edfe637294da4e7 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Wed, 6 Mar 2024 09:27:00 +0100 Subject: [PATCH 1/5] chore: clean-up core and simplify preblock --- baseapp/abci_test.go | 16 ++--- baseapp/baseapp.go | 17 +++--- core/CHANGELOG.md | 1 + core/appmodule/migrations.go | 27 +++++---- core/appmodule/module.go | 59 ++++--------------- core/appmodule/v2/migrations.go | 4 +- core/appmodule/v2/{appmodule.go => module.go} | 10 ++++ runtime/app.go | 2 +- runtime/services/autocli.go | 4 +- testutil/mock/types_mock_appmodule.go | 7 +-- types/abci.go | 10 +--- types/module/configurator.go | 6 +- types/module/module.go | 15 ++--- types/module/module_test.go | 17 +----- x/upgrade/keeper/abci.go | 37 ++++-------- x/upgrade/keeper/abci_test.go | 34 +++++------ x/upgrade/module.go | 2 +- 17 files changed, 100 insertions(+), 168 deletions(-) rename core/appmodule/v2/{appmodule.go => module.go} (88%) 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..47e5c588865 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -58,6 +58,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking Changes +* []() `PreBlock` now returns only an error. 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 7f352a28e7b..c15a713fe75 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" appmodule "cosmossdk.io/core/appmodule/v2" ) @@ -15,16 +14,19 @@ 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. +// It can modify consensus parameters in storage and signal the caller through the return value. +// The new context (ctx) must be passed to all the other lifecycle methods. +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 -// HasConsensusVersion is the interface for declaring a module consensus version. -type HasConsensusVersion = appmodule.HasConsensusVersion +// HasEndBlocker is the extension interface that modules should implement to run +// custom logic after transaction processing in a block. +type HasEndBlocker = appmodule.HasEndBlocker // HasServices is the extension interface that modules should implement to register // implementations of services defined in .proto files. @@ -46,45 +48,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 - // 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 - -// 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 88% rename from core/appmodule/v2/appmodule.go rename to core/appmodule/v2/module.go index eb44c9f513b..a477dc72495 100644 --- a/core/appmodule/v2/appmodule.go +++ b/core/appmodule/v2/module.go @@ -18,6 +18,16 @@ type AppModule interface { IsOnePerModuleType() } +// HasPreBlocker is the extension interface that modules should implement to run +// custom logic before BeginBlock. +// It can modify consensus parameters in storage and signal the caller through the return value. +// The new context (ctx) must be passed to all the other lifecycle methods. +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 d8474b0041a..89764463332 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/testutil/mock/types_mock_appmodule.go b/testutil/mock/types_mock_appmodule.go index 144576e2bf6..c8c68ec9782 100644 --- a/testutil/mock/types_mock_appmodule.go +++ b/testutil/mock/types_mock_appmodule.go @@ -658,12 +658,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/module.go b/types/module/module.go index 7afba9f1211..f355ead2c5d 100644 --- a/types/module/module.go +++ b/types/module/module.go @@ -701,23 +701,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 4853ca86e7d..23a95e58342 100644 --- a/types/module/module_test.go +++ b/types/module/module_test.go @@ -422,24 +422,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{}) + err = mm.PreBlock(sdk.Context{}) require.EqualError(t, err, "some error") } 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 e74005dd894..ad9c94c32a1 100644 --- a/x/upgrade/keeper/abci_test.go +++ b/x/upgrade/keeper/abci_test.go @@ -44,7 +44,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") @@ -52,7 +52,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) @@ -62,7 +62,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") @@ -70,7 +70,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) @@ -174,21 +174,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) @@ -222,7 +222,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) } @@ -259,7 +259,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") @@ -267,7 +267,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 @@ -294,7 +294,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") @@ -327,7 +327,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 @@ -335,7 +335,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") @@ -356,7 +356,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) @@ -446,7 +446,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 { @@ -485,7 +485,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}) @@ -535,7 +535,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 7a609ae4b25..cc45b33ae14 100644 --- a/x/upgrade/module.go +++ b/x/upgrade/module.go @@ -149,6 +149,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) } From e652da91cbdac6ac82bfa1cfbfad2e9622cbfa9c Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Wed, 6 Mar 2024 09:30:27 +0100 Subject: [PATCH 2/5] changelog --- core/CHANGELOG.md | 4 +--- x/upgrade/CHANGELOG.md | 6 +++++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index 47e5c588865..d44c84f4c1a 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -54,11 +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 -* []() `PreBlock` now returns only an error. The SDK has upgraded x/upgrade accordingly. +* [#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/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 From 87236066a4e9b7453e2a034ca232f04d520f4508 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Wed, 6 Mar 2024 22:27:12 +0100 Subject: [PATCH 3/5] fix legacy build --- simapp/app.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/simapp/app.go b/simapp/app.go index be410d5d70d..2b4affd0984 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) } From f346eadf155b18bf0999c439e671a087d393b8e6 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 11 Mar 2024 11:45:42 +0100 Subject: [PATCH 4/5] feedback + mocks --- CHANGELOG.md | 1 + core/appmodule/module.go | 2 -- core/appmodule/v2/module.go | 2 -- types/module/mock_appmodule_test.go | 4 +++- 4 files changed, 4 insertions(+), 5 deletions(-) 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/core/appmodule/module.go b/core/appmodule/module.go index f8513b83817..60c4f80c355 100644 --- a/core/appmodule/module.go +++ b/core/appmodule/module.go @@ -16,8 +16,6 @@ type AppModule = appmodule.AppModule // HasPreBlocker is the extension interface that modules should implement to run // custom logic before BeginBlock. -// It can modify consensus parameters in storage and signal the caller through the return value. -// The new context (ctx) must be passed to all the other lifecycle methods. type HasPreBlocker = appmodule.HasPreBlocker // HasBeginBlocker is the extension interface that modules should implement to run diff --git a/core/appmodule/v2/module.go b/core/appmodule/v2/module.go index 0e1007ebf13..2fe0cc99f07 100644 --- a/core/appmodule/v2/module.go +++ b/core/appmodule/v2/module.go @@ -21,8 +21,6 @@ type AppModule interface { // HasPreBlocker is the extension interface that modules should implement to run // custom logic before BeginBlock. -// It can modify consensus parameters in storage and signal the caller through the return value. -// The new context (ctx) must be passed to all the other lifecycle methods. type HasPreBlocker interface { AppModule // PreBlock is method that will be run before BeginBlock. 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 } From 24679dea3d6a5bab17952174a733b734b068ed93 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 11 Mar 2024 14:51:23 +0100 Subject: [PATCH 5/5] fix test --- types/module/module_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/types/module/module_test.go b/types/module/module_test.go index 535a856448e..670b088d536 100644 --- a/types/module/module_test.go +++ b/types/module/module_test.go @@ -423,7 +423,7 @@ func TestCoreAPIManager_PreBlock(t *testing.T) { require.NoError(t, err) // test error - mockAppModule1.EXPECT().PreBlock(gomock.Any()).Times(1).Return(nil, errors.New("some error")) + mockAppModule1.EXPECT().PreBlock(gomock.Any()).Times(1).Return(errors.New("some error")) err = mm.PreBlock(sdk.Context{}) require.EqualError(t, err, "some error") }