refactor(core)!: clean-up core and simplify preblock (cosmos#19672)

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.

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
 		})
 	}
 

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
 }

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`)

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

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 {

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

core/appmodule/v2/appmodule.go → 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 {

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)
 }
 

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 {

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)
 }
 

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
-}

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)
 	})

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
 }

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