refactor!: reimplement PreFinalizeBlockHook as PreBlocker (#17713)

Co-authored-by: Julien Robert <julien@rbrt.fr>
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

CHANGELOG.md

@@ -59,7 +59,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Bug Fixes
 
-* (types) [#16583](https://github.com/cosmos/cosmos-sdk/pull/16583), [#17372](https://github.com/cosmos/cosmos-sdk/pull/17372), [#17421](https://github.com/cosmos/cosmos-sdk/pull/17421) Introduce `PreBlock`, which runs before begin blocker other modules, and allows to modify consensus parameters, and the changes are visible to the following state machine logics.
+* (types) [#16583](https://github.com/cosmos/cosmos-sdk/pull/16583), [#17372](https://github.com/cosmos/cosmos-sdk/pull/17372), [#17421](https://github.com/cosmos/cosmos-sdk/pull/17421), [#17713](https://github.com/cosmos/cosmos-sdk/pull/17713) Introduce `PreBlock`, which executes in `FinalizeBlock` before `BeginBlock`. It allows the application to modify consensus parameters and have access to VE state. Note, `PreFinalizeBlockHook` is replaced by`PreBlocker`.
 * (baseapp) [#17518](https://github.com/cosmos/cosmos-sdk/pull/17518) Utilizing voting power from vote extensions (CometBFT) instead of the current bonded tokens (x/staking) to determine if a set of vote extensions are valid.
 * (config) [#17649](https://github.com/cosmos/cosmos-sdk/pull/17649) Fix `mempool.max-txs` configuration is invalid in `app.config`.
 * (mempool) [#17668](https://github.com/cosmos/cosmos-sdk/pull/17668) Fix: `PriorityNonceIterator.Next()` nil pointer ref for min priority at the end of iteration.

UPGRADING.md

@@ -134,7 +134,7 @@ app.ModuleManager.SetOrderBeginBlockers(
 
 // ... //
 
-+func (app *SimApp) PreBlocker(ctx sdk.Context, req abci.RequestBeginBlock) (sdk.ResponsePreBlock, error) {
++func (app *SimApp) PreBlocker(ctx sdk.Context, req *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) {
 +	return app.ModuleManager.PreBlock(ctx, req)
 +}
 ```

baseapp/abci.go

@@ -742,13 +742,7 @@ func (app *BaseApp) FinalizeBlock(req *abci.RequestFinalizeBlock) (*abci.Respons
 			WithHeaderHash(req.Hash)
 	}
 
-	if app.preFinalizeBlockHook != nil {
-		if err := app.preFinalizeBlockHook(app.finalizeBlockState.ctx, req); err != nil {
-			return nil, err
-		}
-	}
-
-	if err := app.preBlock(); err != nil {
+	if err := app.preBlock(req); err != nil {
 		return nil, err
 	}
 

baseapp/abci_test.go

@@ -2035,7 +2035,7 @@ func TestABCI_HaltChain(t *testing.T) {
 	}
 }
 
-func TestBaseApp_PreFinalizeBlockHook(t *testing.T) {
+func TestBaseApp_PreBlocker(t *testing.T) {
 	db := dbm.NewMemDB()
 	name := t.Name()
 	logger := log.NewTestLogger(t)
@@ -2045,9 +2045,11 @@ func TestBaseApp_PreFinalizeBlockHook(t *testing.T) {
 	require.NoError(t, err)
 
 	wasHookCalled := false
-	app.SetPreFinalizeBlockHook(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error {
+	app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) {
 		wasHookCalled = true
-		return nil
+		return &sdk.ResponsePreBlock{
+			ConsensusParamsChanged: true,
+		}, nil
 	})
 	app.Seal()
 
@@ -2060,8 +2062,8 @@ func TestBaseApp_PreFinalizeBlockHook(t *testing.T) {
 	_, err = app.InitChain(&abci.RequestInitChain{})
 	require.NoError(t, err)
 
-	app.SetPreFinalizeBlockHook(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error {
-		return errors.New("some error")
+	app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) {
+		return nil, errors.New("some error")
 	})
 	app.Seal()
 
@@ -2122,7 +2124,7 @@ func TestBaseApp_VoteExtensions(t *testing.T) {
 			return &abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT}, nil
 		})
 
-		app.SetPreFinalizeBlockHook(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error {
+		app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) {
 			count := uint64(0)
 			pricesSum := uint64(0)
 			for _, v := range req.Txs {
@@ -2141,7 +2143,9 @@ func TestBaseApp_VoteExtensions(t *testing.T) {
 				ctx.KVStore(capKey1).Set([]byte("avgPrice"), buf)
 			}
 
-			return nil
+			return &sdk.ResponsePreBlock{
+				ConsensusParamsChanged: true,
+			}, nil
 		})
 	}
 

baseapp/baseapp.go

@@ -74,17 +74,16 @@ type BaseApp struct {
 	anteHandler sdk.AnteHandler // ante handler for fee and auth
 	postHandler sdk.PostHandler // post handler, optional, e.g. for tips
 
-	initChainer          sdk.InitChainer                // ABCI InitChain handler
-	preBlocker           sdk.PreBlocker                 // logic to run before BeginBlocker
-	beginBlocker         sdk.BeginBlocker               // (legacy ABCI) BeginBlock handler
-	endBlocker           sdk.EndBlocker                 // (legacy ABCI) EndBlock handler
-	processProposal      sdk.ProcessProposalHandler     // ABCI ProcessProposal handler
-	prepareProposal      sdk.PrepareProposalHandler     // ABCI PrepareProposal
-	extendVote           sdk.ExtendVoteHandler          // ABCI ExtendVote handler
-	verifyVoteExt        sdk.VerifyVoteExtensionHandler // ABCI VerifyVoteExtension handler
-	prepareCheckStater   sdk.PrepareCheckStater         // logic to run during commit using the checkState
-	precommiter          sdk.Precommiter                // logic to run during commit using the deliverState
-	preFinalizeBlockHook sdk.PreFinalizeBlockHook       // logic to run before FinalizeBlock
+	initChainer        sdk.InitChainer                // ABCI InitChain handler
+	preBlocker         sdk.PreBlocker                 // logic to run before BeginBlocker
+	beginBlocker       sdk.BeginBlocker               // (legacy ABCI) BeginBlock handler
+	endBlocker         sdk.EndBlocker                 // (legacy ABCI) EndBlock handler
+	processProposal    sdk.ProcessProposalHandler     // ABCI ProcessProposal handler
+	prepareProposal    sdk.PrepareProposalHandler     // ABCI PrepareProposal
+	extendVote         sdk.ExtendVoteHandler          // ABCI ExtendVote handler
+	verifyVoteExt      sdk.VerifyVoteExtensionHandler // ABCI VerifyVoteExtension handler
+	prepareCheckStater sdk.PrepareCheckStater         // logic to run during commit using the checkState
+	precommiter        sdk.Precommiter                // logic to run during commit using the deliverState
 
 	addrPeerFilter sdk.PeerFilter // filter peers by address and port
 	idPeerFilter   sdk.PeerFilter // filter peers by node ID
@@ -669,10 +668,10 @@ func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context
 	return ctx.WithMultiStore(msCache), msCache
 }
 
-func (app *BaseApp) preBlock() error {
+func (app *BaseApp) preBlock(req *abci.RequestFinalizeBlock) error {
 	if app.preBlocker != nil {
 		ctx := app.finalizeBlockState.ctx
-		rsp, err := app.preBlocker(ctx)
+		rsp, err := app.preBlocker(ctx, req)
 		if err != nil {
 			return err
 		}

baseapp/options.go

@@ -216,14 +216,6 @@ func (app *BaseApp) SetPrecommiter(precommiter sdk.Precommiter) {
 	app.precommiter = precommiter
 }
 
-func (app *BaseApp) SetPreFinalizeBlockHook(hook sdk.PreFinalizeBlockHook) {
-	if app.sealed {
-		panic("SetPreFinalizeBlockHook() on sealed BaseApp")
-	}
-
-	app.preFinalizeBlockHook = hook
-}
-
 func (app *BaseApp) SetAnteHandler(ah sdk.AnteHandler) {
 	if app.sealed {
 		panic("SetAnteHandler() on sealed BaseApp")

docs/architecture/adr-064-abci-2.0.md

@@ -293,15 +293,15 @@ decision based on the vote extensions.
 
 In certain contexts, it may be useful or necessary for applications to persist
 data derived from vote extensions. In order to facilitate this use case, we propose
-to allow app developers to define a pre-FinalizeBlock hook which will be called
+to allow app developers to define a pre-Blocker hook which will be called
 at the very beginning of `FinalizeBlock`, i.e. before `BeginBlock` (see below).
 
 Note, we cannot allow applications to directly write to the application state
 during `ProcessProposal` because during replay, CometBFT will NOT call `ProcessProposal`,
 which would result in an incomplete state view.
 
 ```go
-func (a MyApp) PreFinalizeBlockHook(ctx sdk.Context, req.RequestFinalizeBlock) error {
+func (a MyApp) PreBlocker(ctx sdk.Context, req *abci.RequestFinalizeBlock) error {
 	voteExts := GetVoteExtensions(ctx, req.Txs)
 
 	// Process and perform some compute on vote extensions, storing any resulting
@@ -353,13 +353,17 @@ we can come up with new types and names altogether.
 func (app *BaseApp) FinalizeBlock(req abci.RequestFinalizeBlock) (*abci.ResponseFinalizeBlock, error) {
 	ctx := ...
 
-	if app.preFinalizeBlockHook != nil {
-		if err := app.preFinalizeBlockHook(ctx, req); err != nil {
+	if app.preBlocker != nil {
+		ctx := app.finalizeBlockState.ctx
+		rsp, err := app.preBlocker(ctx, req)
+		if err != nil {
 			return nil, err
 		}
+		if rsp.ConsensusParamsChanged {
+			app.finalizeBlockState.ctx = ctx.WithConsensusParams(app.GetConsensusParams(ctx))
+		}
 	}
-
-	beginBlockResp := app.beginBlock(ctx, req)
+	beginBlockResp, err := app.beginBlock(req)
 	appendBlockEventAttr(beginBlockResp.Events, "begin_block")
 
 	txExecResults := make([]abci.ExecTxResult, 0, len(req.Txs))
@@ -368,7 +372,7 @@ func (app *BaseApp) FinalizeBlock(req abci.RequestFinalizeBlock) (*abci.Response
 		txExecResults = append(txExecResults, result)
 	}
 
-	endBlockResp := app.endBlock(ctx, req)
+	endBlockResp, err := app.endBlock(app.finalizeBlockState.ctx)
 	appendBlockEventAttr(beginBlockResp.Events, "end_block")
 
 	return abci.ResponseFinalizeBlock{

docs/architecture/adr-068-preblock.md

@@ -58,3 +58,4 @@ The new ctx must be passed to all the other lifecycle methods.
 * [1] https://github.com/cosmos/cosmos-sdk/issues/16494
 * [2] https://github.com/cosmos/cosmos-sdk/pull/16583
 * [3] https://github.com/cosmos/cosmos-sdk/pull/17421
+* [4] https://github.com/cosmos/cosmos-sdk/pull/17713

docs/docs/build/building-apps/03-app-upgrade.md

@@ -60,7 +60,7 @@ In addition to basic module wiring, setup the upgrade Keeper for the app and the
 keeper's PreBlocker method:
 
 ```go
-func (app *myApp) PreBlocker(ctx sdk.Context, req req.RequestFinalizeBlock) (sdk.ResponsePreBlock, error) {
+func (app *myApp) PreBlocker(ctx sdk.Context, req req.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) {
       // For demonstration sake, the app PreBlocker only returns the upgrade module pre-blocker.
       // In a real app, the module manager should call all pre-blockers
       // return return app.ModuleManager.PreBlock(ctx, req)

docs/docs/build/building-apps/04-vote-extensions.md

@@ -77,7 +77,7 @@ will be available to the application during the subsequent `FinalizeBlock` call.
 An example of how a pre-FinalizeBlock hook could look like is shown below:
 
 ```go
-app.SetPreFinalizeBlockHook(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error {
+app.SetPreBlocker(func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error {
     allVEs := []VE{} // store all parsed vote extensions here
     for _, tx := range req.Txs {
         // define a custom function that tries to parse the tx as a vote extension

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) (sdk.ResponsePreBlock, error) {
+func (a *App) PreBlocker(ctx sdk.Context, _ *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) {
 	return a.ModuleManager.PreBlock(ctx)
 }
 

simapp/app.go

@@ -583,7 +583,7 @@ func (app *SimApp) setPostHandler() {
 func (app *SimApp) Name() string { return app.BaseApp.Name() }
 
 // PreBlocker application updates every pre block
-func (app *SimApp) PreBlocker(ctx sdk.Context) (sdk.ResponsePreBlock, error) {
+func (app *SimApp) PreBlocker(ctx sdk.Context, _ *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) {
 	return app.ModuleManager.PreBlock(ctx)
 }
 

types/abci.go

@@ -30,8 +30,13 @@ type ExtendVoteHandler func(Context, *abci.RequestExtendVote) (*abci.ResponseExt
 // pre-commit vote extension.
 type VerifyVoteExtensionHandler func(Context, *abci.RequestVerifyVoteExtension) (*abci.ResponseVerifyVoteExtension, error)
 
-// PreBlocker runs code before the `BeginBlocker`.
-type PreBlocker func(Context) (ResponsePreBlock, error)
+// PreBlocker runs code before the `BeginBlocker` and defines a function type alias for executing logic right
+// before FinalizeBlock is called (but after its context has been set up). It is
+// intended to allow applications to perform computation on vote extensions and
+// persist their results in state.
+//
+// Note: returning an error will make FinalizeBlock fail.
+type PreBlocker func(Context, *abci.RequestFinalizeBlock) (*ResponsePreBlock, error)
 
 // BeginBlocker defines a function type alias for executing application
 // business logic before transactions are executed.
@@ -51,14 +56,6 @@ type BeginBlocker func(Context) (BeginBlock, error)
 // and allows for existing EndBlock functionality within applications.
 type EndBlocker func(Context) (EndBlock, error)
 
-// PreFinalizeBlockHook defines a function type alias for executing logic right
-// before FinalizeBlock is called (but after its context has been set up). It is
-// intended to allow applications to perform computation on vote extensions and
-// persist their results in state.
-//
-// Note: returning an error will make FinalizeBlock fail.
-type PreFinalizeBlockHook func(Context, *abci.RequestFinalizeBlock) error
-
 // EndBlock defines a type which contains endblock events and validator set updates
 type EndBlock struct {
 	ValidatorUpdates []abci.ValidatorUpdate

types/module/module.go

@@ -744,21 +744,21 @@ 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) (*sdk.ResponsePreBlock, 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 sdk.ResponsePreBlock{}, err
+				return nil, err
 			}
 			if rsp.IsConsensusParamsChanged() {
 				paramsChanged = true
 			}
 		}
 	}
-	return sdk.ResponsePreBlock{
+	return &sdk.ResponsePreBlock{
 		ConsensusParamsChanged: paramsChanged,
 	}, nil
 }

types/module/module_test.go

@@ -482,23 +482,23 @@ 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{
+	mockAppModule1.EXPECT().PreBlock(gomock.Any()).Times(1).Return(&sdk.ResponsePreBlock{
 		ConsensusParamsChanged: true,
 	}, nil)
 	res, 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{
+	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(sdk.ResponsePreBlock{}, errors.New("some error"))
+	mockAppModule1.EXPECT().PreBlock(gomock.Any()).Times(1).Return(nil, errors.New("some error"))
 	_, err = mm.PreBlock(sdk.Context{})
 	require.EqualError(t, err, "some error")
 }

x/upgrade/CHANGELOG.md

@@ -41,3 +41,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * [#16511](https://github.com/cosmos/cosmos-sdk/pull/16511) `plan.DownloadURLWithChecksum` has been renamed to `plan.DownloadURL` and does not validate the URL anymore. Call `plan.ValidateURL` before calling `plan.DownloadURL` to validate the URL.
 * [#16511](https://github.com/cosmos/cosmos-sdk/pull/16511) `plan.DownloadUpgrade` does not validate URL anymore. Call `plan.ValidateURL` before calling `plan.DownloadUpgrade` to validate the URL.
 * [#16227](https://github.com/cosmos/cosmos-sdk/issues/16227) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey`, methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context` and return an `error`. `UpgradeHandler` now receives a `context.Context`. `GetUpgradedClient`, `GetUpgradedConsensusState`, `GetUpgradePlan` now return a specific error for "not found".
+
+### Bug Fixes
+
+* [#17421](https://github.com/cosmos/cosmos-sdk/pull/17421) Replace ` BeginBlock` by `PreBlock`. Read [ADR-68](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-068-preblock.md) for more information.