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 #18627

Merged
merged 2 commits into from
Dec 6, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
670c0d2
fix!: postHandler should run regardless of result
facundomedica Dec 4, 2023
45aafba
cl++
facundomedica Dec 4, 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 @@ -184,6 +184,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### State Machine Breaking

* (baseapp) [#18627](https://github.com/cosmos/cosmos-sdk/pull/18627) Post handlers are run on non successful transaction executions too.
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The changelog entry for PR 18627 correctly reflects the changes made to the state machine behavior. However, it may be beneficial to include that this is a consensus-breaking change, as indicated in the PR objective, to ensure users are fully aware of the implications.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I second the bot's suggestion. Perhaps

Run post handlers if set, regardless of if a transaction was successful or not: consensus breaking change!

Copy link
Contributor

Choose a reason for hiding this comment

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

@odeke-em Thank you for the feedback. The changelog entry can be updated to:

* (baseapp) [#18627](https://github.com/cosmos/cosmos-sdk/pull/18627) Run post handlers if set, regardless of if a transaction was successful or not: consensus breaking change!

This clearly communicates the consensus-breaking nature of the change.

* (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.
* (x/staking) [#17655](https://github.com/cosmos/cosmos-sdk/pull/17655) `HistoricalInfo` was replaced with `HistoricalRecord`, it removes the validator set and comet header and only keep what is needed for IBC.

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 @@ -938,24 +938,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.
Comment on lines +942 to +944
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think using the comment, "Run optional postHandlers (should run regardless of the execution results)" could still be as ambiguous and not what would stand out to other maintainers reading through this code. How about?

// If the postHandler is set, it MUST be ran regardless of the execution result.

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 938 to 962
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:811)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).deliverTx (baseapp/baseapp.go:741)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).internalFinalizeBlock (baseapp/baseapp.go:699)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).FinalizeBlock (baseapp/baseapp.go:856)

Copy link
Member Author

Choose a reason for hiding this comment

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

It does affect state, good bot

Comment on lines 938 to 962
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes made in the hunk correctly implement the PR objective by ensuring that the postHandler is executed regardless of the transaction result. The logic to clear the event manager before running the postHandler to avoid duplicate events is also in place. This aligns with the intended behavior and the PR summary.

However, there is a potential issue with the ordering of events. The anteEvents are appended after the postHandler events, which could lead to incorrect event sequencing. The expected behavior would be to have anteEvents first, followed by message processing events, and finally postHandler events.

-       result.Events = append(result.Events, newCtx.EventManager().ABCIEvents()...)
+       result.Events = append(newCtx.EventManager().ABCIEvents(), result.Events...)
Committable suggestion

❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
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()
if err == nil {
result, err = app.runMsgs(runMsgCtx, msgs, msgsV2, mode)
}
// 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(newCtx.EventManager().ABCIEvents(), result.Events...)
}
if err == nil {
if mode == execModeFinalize {
// When block gas exceeds, it'll panic and won't commit the cached store.
consumeBlockGas()

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 @@ -594,6 +594,53 @@ func TestBaseAppAnteHandler(t *testing.T) {
require.NoError(t, err)
}

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)
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)
Comment on lines +631 to +641
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// It should also run on failed message execution
postHandlerRun = false
tx = setFailOnHandler(t, suite.txConfig, tx, true)
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)
// It should also run on failed message execution
postHandlerRun = false
tx = setFailOnHandler(t, suite.txConfig, tx, true)
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