From 4955776845b142594c1fd25a03debdfc04ce4e70 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Fri, 15 Sep 2023 08:14:23 +0000 Subject: [PATCH] refactor!: reimplement PreFinalizeBlockHook as PreBlocker (backport #17713) (#17754) Co-authored-by: mmsqe Co-authored-by: Julien Robert --- UPGRADING.md | 2 +- baseapp/abci.go | 8 +-- baseapp/abci_test.go | 18 ++++--- baseapp/baseapp.go | 25 +++++---- baseapp/options.go | 8 --- docs/architecture/adr-064-abci-2.0.md | 53 +++++++++---------- docs/architecture/adr-068-preblock.md | 1 + .../build/building-apps/03-app-upgrade.md | 2 +- .../build/building-apps/04-vote-extensions.md | 2 +- runtime/app.go | 2 +- simapp/app.go | 2 +- simapp/go.mod | 2 +- simapp/go.sum | 4 +- tests/go.mod | 2 +- tests/go.sum | 4 +- types/abci.go | 17 +++--- types/module/module.go | 6 +-- types/module/module_test.go | 6 +-- x/upgrade/CHANGELOG.md | 4 ++ x/upgrade/abci.go | 41 ++++++++------ 20 files changed, 104 insertions(+), 105 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index 0843062241..d1418e63fd 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -99,7 +99,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) +} ``` diff --git a/baseapp/abci.go b/baseapp/abci.go index 77da964f2f..28b0326cf5 100644 --- a/baseapp/abci.go +++ b/baseapp/abci.go @@ -721,13 +721,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 } diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index d82345e4cf..c7462474b3 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -2015,7 +2015,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) @@ -2025,9 +2025,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() @@ -2040,8 +2042,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() @@ -2102,7 +2104,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 { @@ -2121,7 +2123,9 @@ func TestBaseApp_VoteExtensions(t *testing.T) { ctx.KVStore(capKey1).Set([]byte("avgPrice"), buf) } - return nil + return &sdk.ResponsePreBlock{ + ConsensusParamsChanged: true, + }, nil }) } diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 57d09d41a3..2353282385 100644 --- a/baseapp/baseapp.go +++ b/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 @@ -665,10 +664,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 } diff --git a/baseapp/options.go b/baseapp/options.go index e461ff4898..a56ce19c88 100644 --- a/baseapp/options.go +++ b/baseapp/options.go @@ -198,14 +198,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") diff --git a/docs/architecture/adr-064-abci-2.0.md b/docs/architecture/adr-064-abci-2.0.md index 5949651671..c0dc7f746e 100644 --- a/docs/architecture/adr-064-abci-2.0.md +++ b/docs/architecture/adr-064-abci-2.0.md @@ -292,32 +292,22 @@ decision based on the vote extensions. #### Vote Extension Persistence 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 application developers to manually retrieve the `finalizeState` -context (see [`FinalizeBlock`](#finalizeblock-1) below). Using this context, -state can be directly written to `finalizeState`, which will be used during -`FinalizeBlock` and eventually committed to the application state. Note, since -`ProcessProposal` can timeout and thus require another round of consensus, we -will reset `finalizeState` in the beginning of `ProcessProposal`. +data derived from vote extensions. In order to facilitate this use case, we propose +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). -A `ProcessProposal` handler could look like the following: +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 (h MyHandler) ProcessProposalHandler() sdk.ProcessProposalHandler { - return func(ctx sdk.Context, req abci.RequestProcessProposal) abci.ResponseProcessProposal { - for _, txBytes := range req.Txs { - _, err := h.app.ProcessProposalVerifyTx(txBytes) - if err != nil { - return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_REJECT} - } - } - - fCtx := h.app.GetFinalizeState() - - // Any state changes that occur on the provided fCtx WILL be written to state! - h.myKeeper.SetVoteExtResult(fCtx, ...) +func (a MyApp) PreBlocker(ctx sdk.Context, req *abci.RequestFinalizeBlock) error { + voteExts := GetVoteExtensions(ctx, req.Txs) - return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT} + // Process and perform some compute on vote extensions, storing any resulting + // state. + if err a.processVoteExtensions(ctx, voteExts); if err != nil { + return err } } ``` @@ -360,11 +350,20 @@ legacy ABCI types, e.g. `LegacyBeginBlockRequest` and `LegacyEndBlockRequest`. O we can come up with new types and names altogether. ```go -func (app *BaseApp) FinalizeBlock(req abci.RequestFinalizeBlock) abci.ResponseFinalizeBlock { - // merge any state changes from ProcessProposal into the FinalizeBlock state - app.MergeProcessProposalState() +func (app *BaseApp) FinalizeBlock(req abci.RequestFinalizeBlock) (*abci.ResponseFinalizeBlock, error) { + ctx := ... - beginBlockResp := app.beginBlock(ctx, req) + 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, err := app.beginBlock(req) appendBlockEventAttr(beginBlockResp.Events, "begin_block") txExecResults := make([]abci.ExecTxResult, 0, len(req.Txs)) @@ -373,7 +372,7 @@ func (app *BaseApp) FinalizeBlock(req abci.RequestFinalizeBlock) abci.ResponseFi txExecResults = append(txExecResults, result) } - endBlockResp := app.endBlock(ctx, req) + endBlockResp, err := app.endBlock(app.finalizeBlockState.ctx) appendBlockEventAttr(beginBlockResp.Events, "end_block") return abci.ResponseFinalizeBlock{ diff --git a/docs/architecture/adr-068-preblock.md b/docs/architecture/adr-068-preblock.md index dc0cd2a245..86692c412c 100644 --- a/docs/architecture/adr-068-preblock.md +++ b/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 diff --git a/docs/docs/build/building-apps/03-app-upgrade.md b/docs/docs/build/building-apps/03-app-upgrade.md index f59dff94ca..d5a88c5d7c 100644 --- a/docs/docs/build/building-apps/03-app-upgrade.md +++ b/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) diff --git a/docs/docs/build/building-apps/04-vote-extensions.md b/docs/docs/build/building-apps/04-vote-extensions.md index 8b9fb5943f..f78f4c677f 100644 --- a/docs/docs/build/building-apps/04-vote-extensions.md +++ b/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 diff --git a/runtime/app.go b/runtime/app.go index e2eda9e934..c20d8c9540 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) (sdk.ResponsePreBlock, error) { +func (a *App) PreBlocker(ctx sdk.Context, _ *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) { return a.ModuleManager.PreBlock(ctx) } diff --git a/simapp/app.go b/simapp/app.go index 3aba17081c..fce1f00f88 100644 --- a/simapp/app.go +++ b/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) } diff --git a/simapp/go.mod b/simapp/go.mod index 5708660af1..a643bd1397 100644 --- a/simapp/go.mod +++ b/simapp/go.mod @@ -17,7 +17,7 @@ require ( cosmossdk.io/x/feegrant v0.0.0-20230913185058-9b5a203d35bc cosmossdk.io/x/nft v0.0.0-20230913185058-9b5a203d35bc cosmossdk.io/x/tx v0.10.0 - cosmossdk.io/x/upgrade v0.0.0-20230913185058-9b5a203d35bc + cosmossdk.io/x/upgrade v0.0.0-20230915075604-076dc1ee9619 github.com/cometbft/cometbft v0.38.0 github.com/cosmos/cosmos-db v1.0.0 // this version is not used as it is always replaced by the latest Cosmos SDK version diff --git a/simapp/go.sum b/simapp/go.sum index c92a7d191c..459eb0112f 100644 --- a/simapp/go.sum +++ b/simapp/go.sum @@ -217,8 +217,8 @@ cosmossdk.io/x/nft v0.0.0-20230913185058-9b5a203d35bc h1:Jm0ZJ1L6d8YZ6yQydUknNL6 cosmossdk.io/x/nft v0.0.0-20230913185058-9b5a203d35bc/go.mod h1:B+yw1SVhkUEANcuxisIgiT6cw6re1gGzSsMmY8+lvAI= cosmossdk.io/x/tx v0.10.0 h1:LxWF/hksVDbeQmFj4voLM5ZCHyVZ1cCNIqKenfH9plc= cosmossdk.io/x/tx v0.10.0/go.mod h1:MKo9/b5wsoL8dd9y9pvD2yOP1CMvzHIWYxi1l2oLPFo= -cosmossdk.io/x/upgrade v0.0.0-20230913185058-9b5a203d35bc h1:vLTAmHtVdNjil1QgYVUYKvn5UKk5Ntpz2dpMyXW3fMc= -cosmossdk.io/x/upgrade v0.0.0-20230913185058-9b5a203d35bc/go.mod h1:nLBiFTPw6e4LBT1ZWdGBIOjleiVNaqqmsTxU4GgEYaQ= +cosmossdk.io/x/upgrade v0.0.0-20230915075604-076dc1ee9619 h1:3wgUsnj/ElYA+B33h6Yn1KxWvB4hPFa+K90fw9gRO9M= +cosmossdk.io/x/upgrade v0.0.0-20230915075604-076dc1ee9619/go.mod h1:bqexnYfkwMCqbXXN4SprKS9N7cTwT1lFholB7UQhoDU= dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU= filippo.io/edwards25519 v1.0.0 h1:0wAIcmJUqRdI8IJ/3eGi5/HwXZWPujYXXlkrQogz0Ek= filippo.io/edwards25519 v1.0.0/go.mod h1:N1IkdkCkiLB6tki+MYJoSx2JTY9NUlxZE7eHn5EwJns= diff --git a/tests/go.mod b/tests/go.mod index 55505d1264..9b7a44a4ab 100644 --- a/tests/go.mod +++ b/tests/go.mod @@ -15,7 +15,7 @@ require ( cosmossdk.io/x/feegrant v0.0.0-20230913185058-9b5a203d35bc cosmossdk.io/x/nft v0.0.0-20230913185058-9b5a203d35bc // indirect cosmossdk.io/x/tx v0.10.0 - cosmossdk.io/x/upgrade v0.0.0-20230913185058-9b5a203d35bc + cosmossdk.io/x/upgrade v0.0.0-20230915075604-076dc1ee9619 github.com/cometbft/cometbft v0.38.0 github.com/cosmos/cosmos-db v1.0.0 github.com/cosmos/cosmos-proto v1.0.0-beta.3 diff --git a/tests/go.sum b/tests/go.sum index 0c5bef449a..fadcf31062 100644 --- a/tests/go.sum +++ b/tests/go.sum @@ -215,8 +215,8 @@ cosmossdk.io/x/nft v0.0.0-20230913185058-9b5a203d35bc h1:Jm0ZJ1L6d8YZ6yQydUknNL6 cosmossdk.io/x/nft v0.0.0-20230913185058-9b5a203d35bc/go.mod h1:B+yw1SVhkUEANcuxisIgiT6cw6re1gGzSsMmY8+lvAI= cosmossdk.io/x/tx v0.10.0 h1:LxWF/hksVDbeQmFj4voLM5ZCHyVZ1cCNIqKenfH9plc= cosmossdk.io/x/tx v0.10.0/go.mod h1:MKo9/b5wsoL8dd9y9pvD2yOP1CMvzHIWYxi1l2oLPFo= -cosmossdk.io/x/upgrade v0.0.0-20230913185058-9b5a203d35bc h1:vLTAmHtVdNjil1QgYVUYKvn5UKk5Ntpz2dpMyXW3fMc= -cosmossdk.io/x/upgrade v0.0.0-20230913185058-9b5a203d35bc/go.mod h1:nLBiFTPw6e4LBT1ZWdGBIOjleiVNaqqmsTxU4GgEYaQ= +cosmossdk.io/x/upgrade v0.0.0-20230915075604-076dc1ee9619 h1:3wgUsnj/ElYA+B33h6Yn1KxWvB4hPFa+K90fw9gRO9M= +cosmossdk.io/x/upgrade v0.0.0-20230915075604-076dc1ee9619/go.mod h1:bqexnYfkwMCqbXXN4SprKS9N7cTwT1lFholB7UQhoDU= dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU= filippo.io/edwards25519 v1.0.0 h1:0wAIcmJUqRdI8IJ/3eGi5/HwXZWPujYXXlkrQogz0Ek= filippo.io/edwards25519 v1.0.0/go.mod h1:N1IkdkCkiLB6tki+MYJoSx2JTY9NUlxZE7eHn5EwJns= diff --git a/types/abci.go b/types/abci.go index ff40d1109a..8325f5dadf 100644 --- a/types/abci.go +++ b/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 diff --git a/types/module/module.go b/types/module/module.go index 28027d0746..83b9b63760 100644 --- a/types/module/module.go +++ b/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 } diff --git a/types/module/module_test.go b/types/module/module_test.go index 3be6dd3fa7..660af10449 100644 --- a/types/module/module_test.go +++ b/types/module/module_test.go @@ -478,7 +478,7 @@ 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{}) @@ -486,7 +486,7 @@ func TestCoreAPIManager_PreBlock(t *testing.T) { 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{}) @@ -494,7 +494,7 @@ func TestCoreAPIManager_PreBlock(t *testing.T) { 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") } diff --git a/x/upgrade/CHANGELOG.md b/x/upgrade/CHANGELOG.md index 46980cc856..bfd682d319 100644 --- a/x/upgrade/CHANGELOG.md +++ b/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. diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index 58064868c8..2ce5ea3066 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -6,6 +6,7 @@ import ( "fmt" "time" + "cosmossdk.io/core/appmodule" storetypes "cosmossdk.io/store/types" "cosmossdk.io/x/upgrade/keeper" "cosmossdk.io/x/upgrade/types" @@ -22,17 +23,14 @@ 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 PreBlocker(ctx context.Context, k *keeper.Keeper) (sdk.ResponsePreBlock, error) { - rsp := sdk.ResponsePreBlock{ - ConsensusParamsChanged: false, - } +func PreBlocker(ctx context.Context, k *keeper.Keeper) (appmodule.ResponsePreBlock, error) { defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) sdkCtx := sdk.UnwrapSDKContext(ctx) blockHeight := sdkCtx.HeaderInfo().Height plan, err := k.GetUpgradePlan(ctx) if err != nil && !errors.Is(err, types.ErrNoUpgradePlanFound) { - return rsp, err + return nil, err } found := err == nil @@ -46,7 +44,7 @@ func PreBlocker(ctx context.Context, k *keeper.Keeper) (sdk.ResponsePreBlock, er if !found || !plan.ShouldExecute(blockHeight) || (plan.ShouldExecute(blockHeight) && k.IsSkipHeight(blockHeight)) { lastAppliedPlan, _, err := k.GetLastCompletedUpgrade(ctx) if err != nil { - return rsp, err + return nil, err } if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) { @@ -57,13 +55,15 @@ func PreBlocker(ctx context.Context, k *keeper.Keeper) (sdk.ResponsePreBlock, er appVersion = cp.Version.App } - return rsp, fmt.Errorf("wrong app version %d, upgrade handler is missing for %s upgrade plan", appVersion, lastAppliedPlan) + return nil, fmt.Errorf("wrong app version %d, upgrade handler is missing for %s upgrade plan", appVersion, lastAppliedPlan) } } } if !found { - return rsp, nil + return &sdk.ResponsePreBlock{ + ConsensusParamsChanged: false, + }, nil } logger := k.Logger(ctx) @@ -76,7 +76,12 @@ func PreBlocker(ctx context.Context, k *keeper.Keeper) (sdk.ResponsePreBlock, er logger.Info(skipUpgradeMsg) // Clear the upgrade plan at current height - return rsp, k.ClearUpgradePlan(ctx) + if err := k.ClearUpgradePlan(ctx); err != nil { + return nil, err + } + return &sdk.ResponsePreBlock{ + ConsensusParamsChanged: false, + }, nil } // Prepare shutdown if we don't have an upgrade handler for this upgrade name (meaning this software is out of date) @@ -85,25 +90,27 @@ func PreBlocker(ctx context.Context, k *keeper.Keeper) (sdk.ResponsePreBlock, er // store migrations. err := k.DumpUpgradeInfoToDisk(blockHeight, plan) if err != nil { - return rsp, fmt.Errorf("unable to write upgrade info to filesystem: %w", err) + return nil, 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 rsp, errors.New(upgradeMsg) + return nil, 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()) - err = k.ApplyUpgrade(sdkCtx, plan) - return sdk.ResponsePreBlock{ + if err := k.ApplyUpgrade(sdkCtx, plan); err != nil { + return nil, err + } + return &sdk.ResponsePreBlock{ // the consensus parameters might be modified in the migration, // refresh the consensus parameters in context. ConsensusParamsChanged: true, - }, err + }, nil } // if we have a pending upgrade, but it is not yet time, make sure we did not @@ -113,9 +120,11 @@ func PreBlocker(ctx context.Context, k *keeper.Keeper) (sdk.ResponsePreBlock, er logger.Error(downgradeMsg) // Returning an error will end up in a panic - return rsp, errors.New(downgradeMsg) + return nil, errors.New(downgradeMsg) } - return rsp, nil + return &sdk.ResponsePreBlock{ + ConsensusParamsChanged: false, + }, nil } // BuildUpgradeNeededMsg prints the message that notifies that an upgrade is needed.