Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(baseapp)!: postHandler should run regardless of result (backport #18627) #18638

Merged
merged 3 commits into from
Dec 6, 2023
Merged

fix(baseapp)!: postHandler should run regardless of result (backport #18627) #18638

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
46a9c0a
fix(baseapp)!: postHandler should run regardless of result (#18627)
facundomedica Dec 6, 2023
f5aa6b1
updates
julienrbrt Dec 6, 2023
7cde26c
fixes
julienrbrt Dec 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#18531](https://github.com/cosmos/cosmos-sdk/pull/18531) Baseapp's `GetConsensusParams` returns an empty struct instead of panicking if no params are found.
* (client/tx) [#18472](https://github.com/cosmos/cosmos-sdk/pull/18472) Utilizes the correct Pubkey when simulating a transaction.
* (baseapp) [#18486](https://github.com/cosmos/cosmos-sdk/pull/18486) Fixed FinalizeBlock calls not being passed to ABCIListeners.
* (baseapp) [#18627](https://github.com/cosmos/cosmos-sdk/pull/18627) Post handlers are run on non successful transaction executions too.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am for merging this into v0.50.2 with the assumption that there are no user of v0.50.1 on prod yet.
With the amount of bug fixes we will add to v0.50.2, I think no one with launch on prod with v0.50.1.
I have checked sourcegraph and github which seems to confirm that. Maybe only berachain (so cc-ing @itsdevbear)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me. ACK

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a testnet on v0.50.1 yuh.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use post handler so this is non consensus breaking for us. All good


## [v0.50.1](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.1) - 2023-11-07

Expand Down
31 changes: 16 additions & 15 deletions baseapp/baseapp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,24 +925,25 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, res
if err == nil {
result, err = app.runMsgs(runMsgCtx, msgs, msgsV2, mode)
}
if err == nil {
// Run optional postHandlers.
//
// Note: If the postHandler fails, we also revert the runMsgs state.
if app.postHandler != nil {
// The runMsgCtx context currently contains events emitted by the ante handler.
// We clear this to correctly order events without duplicates.
// Note that the state is still preserved.
postCtx := runMsgCtx.WithEventManager(sdk.NewEventManager())

newCtx, err := app.postHandler(postCtx, tx, mode == execModeSimulate, err == nil)
if err != nil {
return gInfo, nil, anteEvents, err
}

result.Events = append(result.Events, newCtx.EventManager().ABCIEvents()...)
// Run optional postHandlers (should run regardless of the execution result).
//
// Note: If the postHandler fails, we also revert the runMsgs state.
if app.postHandler != nil {
// The runMsgCtx context currently contains events emitted by the ante handler.
// We clear this to correctly order events without duplicates.
// Note that the state is still preserved.
postCtx := runMsgCtx.WithEventManager(sdk.NewEventManager())

newCtx, err := app.postHandler(postCtx, tx, mode == execModeSimulate, err == nil)
if err != nil {
return gInfo, nil, anteEvents, err
}

result.Events = append(result.Events, newCtx.EventManager().ABCIEvents()...)
}

if err == nil {
if mode == execModeFinalize {
// When block gas exceeds, it'll panic and won't commit the cached store.
consumeBlockGas()
Comment on lines 925 to 949
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change potentially affects state.

Call sequence:

(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).runTx (baseapp/baseapp.go:798)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).deliverTx (baseapp/baseapp.go:728)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).internalFinalizeBlock (baseapp/baseapp.go:680)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).FinalizeBlock (baseapp/baseapp.go:837)

Expand Down
47 changes: 47 additions & 0 deletions baseapp/baseapp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,53 @@
suite.baseApp.Commit()
}

func TestBaseAppPostHandler(t *testing.T) {
postHandlerRun := false
anteOpt := func(bapp *baseapp.BaseApp) {
bapp.SetPostHandler(func(ctx sdk.Context, tx sdk.Tx, simulate, success bool) (newCtx sdk.Context, err error) {
postHandlerRun = true
return ctx, nil
})
}

suite := NewBaseAppSuite(t, anteOpt)

baseapptestutil.RegisterCounterServer(suite.baseApp.MsgServiceRouter(), CounterServerImpl{t, capKey1, []byte("foo")})

_, err := suite.baseApp.InitChain(&abci.RequestInitChain{
ConsensusParams: &cmtproto.ConsensusParams{},
})
require.NoError(t, err)

// execute a tx that will fail ante handler execution
//
// NOTE: State should not be mutated here. This will be implicitly checked by
// the next txs ante handler execution (anteHandlerTxTest).
tx := newTxCounter(t, suite.txConfig, 0, 0)
txBytes, err := suite.txConfig.TxEncoder()(tx)
require.NoError(t, err)

res, err := suite.baseApp.FinalizeBlock(&abci.RequestFinalizeBlock{Height: 1, Txs: [][]byte{txBytes}})
require.NoError(t, err)
require.Empty(t, res.Events)
require.True(t, res.TxResults[0].IsOK(), fmt.Sprintf("%v", res))

// PostHandler runs on successful message execution
require.True(t, postHandlerRun)

// It should also run on failed message execution
postHandlerRun = false
tx = setFailOnHandler(t, suite.txConfig, tx, true)

Check failure on line 626 in baseapp/baseapp_test.go

View workflow job for this annotation

GitHub Actions / Analyze 

too many arguments in call to setFailOnHandler

Check failure on line 626 in baseapp/baseapp_test.go

View workflow job for this annotation

GitHub Actions / tests (00) 

too many arguments in call to setFailOnHandler
txBytes, err = suite.txConfig.TxEncoder()(tx)
require.NoError(t, err)
res, err = suite.baseApp.FinalizeBlock(&abci.RequestFinalizeBlock{Height: 1, Txs: [][]byte{txBytes}})
require.NoError(t, err)
require.Empty(t, res.Events)
require.False(t, res.TxResults[0].IsOK(), fmt.Sprintf("%v", res))

require.True(t, postHandlerRun)
}

// Test and ensure that invalid block heights always cause errors.
// See issues:
// - https://github.com/cosmos/cosmos-sdk/issues/11220
Expand Down
Loading