fix(baseapp)!: postHandler should run regardless of result #18627
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughWalkthroughThe recent update involves a significant change in the behavior of the base application's state machine. The postHandlers, which are optional routines run after message processing, are now executed regardless of whether the message execution was successful or not. This change ensures that postHandlers are consistently run, and the event handling has been adjusted to maintain the correct order and prevent duplicates. Additionally, a new test has been added to verify this updated behavior. Changes
TipsChat with CodeRabbit Bot (
|
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Comment on lines
938
to
962
| 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() |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
It does affect state, good bot
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- baseapp/baseapp.go (1 hunks)
- baseapp/baseapp_test.go (1 hunks)
Additional comments: 1
baseapp/baseapp_test.go (1)
- 597-642: The test
TestBaseAppPostHandlercorrectly implements the intended behavior of post handlers running irrespective of the transaction outcome, which aligns with the PR objective. It verifies the post-handler execution after both successful and failed message execution.
Comment on lines
938
to
962
| 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() |
There was a problem hiding this comment.
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() |
| @@ -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. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I second the bot's suggestion. Perhaps
Run post handlers if set, regardless of if a transaction was successful or not: consensus breaking change!
There was a problem hiding this comment.
@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.
|
We don't backport that, right? If we do, we really need to check sourcegraph to see for which chain it would be breaking. |
|
i dont think we can backport this as it could cause consensus breaks. if we do 0.50 then we may need to retract other versions |
Given that no one has integrated with v0.50.x on prod yet, is it maybe too cautious? v0.50.2 will contain bug fixes that no one would want to skip on prod. |
alexanderbez
approved these changes
Dec 5, 2023
There was a problem hiding this comment.
LGTM. I've seen this talked about before. I thought we addressed it, but seems not. Thanks @facundomedica 👍
Comment on lines
+631
to
+641
| // 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) |
There was a problem hiding this comment.
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) |
odeke-em
approved these changes
Dec 6, 2023
There was a problem hiding this comment.
Thank you for this change @facundomedica, thank you for the test as well; just some suggestions but otherwise LGTM!
Comment on lines
+942
to
+944
| // Run optional postHandlers (should run regardless of the execution result). | ||
| // | ||
| // Note: If the postHandler fails, we also revert the runMsgs state. |
There was a problem hiding this comment.
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.| @@ -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. | |||
There was a problem hiding this comment.
I second the bot's suggestion. Perhaps
Run post handlers if set, regardless of if a transaction was successful or not: consensus breaking change!
julienrbrt
changed the title
julienrbrt
added
the
backport/v0.50.x
|
Merging now to get this in v0.50.2 tomorrow. |
mergify bot
pushed a commit
that referenced
this pull request
Dec 6, 2023
(cherry picked from commit b2084dc) # Conflicts: # CHANGELOG.md
julienrbrt
added a commit
that referenced
this pull request
Dec 6, 2023
…18627) (#18638) Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Julien Robert <[email protected]>
marcello33
added a commit
to 0xPolygon/cosmos-sdk
that referenced
this pull request
Dec 13, 2023
* feat: secp256k1 public key constant time (cosmos#18026) Signed-off-by: bizk <[email protected]> * chore: Fixed changelog duplicated items (cosmos#18628) * adr: Un-Ordered Transaction Inclusion (cosmos#18553) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> * docs: lint ADR-070 (cosmos#18634) * fix(baseapp)!: postHandler should run regardless of result (cosmos#18627) * docs: fix typos in adr-007-specialization-groups.md (cosmos#18635) * chore: alphabetize labels (cosmos#18640) * docs(x/circuit): add note on ante handler (cosmos#18637) Co-authored-by: Aleksandr Bezobchuk <[email protected]> * fix: telemetry metric label variable (cosmos#18643) * chore: typos fix (cosmos#18642) * refactor(store/v2): updates from integration (cosmos#18633) * build(deps): Bump actions/setup-go from 4 to 5 (cosmos#18647) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(store/v2): snapshot manager (cosmos#18458) * chore(client/v2): fix typos in the README.md (cosmos#18657) * fix(baseapp): protocompat.go gogoproto.Merge does not work with custom types (cosmos#18654) Co-authored-by: unknown unknown <unknown@unknown> * chore: fix several minor typos (cosmos#18660) * chore(tools/confix/cmd): fix typo in view.go (cosmos#18659) * refactor(x/staking): check duplicate addresses in StakeAuthorization's params (cosmos#18655) * feat(accounts): use gogoproto API instead of protov2. (cosmos#18653) Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(store/commitment/iavl): honor tree.Remove error firstly (cosmos#18651) * build(deps): Bump actions/stale from 8 to 9 (cosmos#18656) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(docs): fix typos & wording in docs (cosmos#18667) * chore: fix several typos. (cosmos#18666) * feat(telemetry): enable `statsd` and `dogstatsd` telemetry sinks (cosmos#18646) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: marbar3778 <[email protected]> Co-authored-by: Marko <[email protected]> * feat(store/v2): add SetInitialVersion in SC (cosmos#18665) * feat(client/keys): support display discreetly for `keys add` (cosmos#18663) Co-authored-by: Julien Robert <[email protected]> * ci: add misspell action (cosmos#18671) * chore: typos fix by misspell-fixer (cosmos#18683) Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Julien Robert <[email protected]> * chore: add v0.50.2 changelog to main (cosmos#18682) * build(deps): Bump github.com/jhump/protoreflect from 1.15.3 to 1.15.4 in /tests (cosmos#18678) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * refactor(bank): remove .String() calls (cosmos#18175) Co-authored-by: Facundo <[email protected]> * ci: use codespell instead of misspell-fixer (cosmos#18686) Co-authored-by: Marko <[email protected]> * feat(gov): add proposal types and spam votes (cosmos#18532) * feat(accounts): use account number as state prefix for account state (cosmos#18664) Co-authored-by: unknown unknown <unknown@unknown> * chore: typos fixes by cosmos-sdk bot (cosmos#18689) Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: marbar3778 <[email protected]> * feat(client/keys): support display discreetly for keys mnemonic (cosmos#18688) * refactor: remove panic usage in keeper methods (cosmos#18636) * ci: rename pr name in misspell job (cosmos#18693) Co-authored-by: Marko <[email protected]> * build(deps): Bump github.com/pelletier/go-toml/v2 from 2.1.0 to 2.1.1 in /tools/confix (cosmos#18702) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(client/keys): support display discreetly for keys export (cosmos#18684) * feat(x/gov): better gov genesis validation (cosmos#18707) --------- Signed-off-by: bizk <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Carlos Santiago Yanzon <[email protected]> Co-authored-by: yihuang <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Akaonetwo <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: dreamweaverxyz <[email protected]> Co-authored-by: Pioua <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: cool-developer <[email protected]> Co-authored-by: leonarddt05 <[email protected]> Co-authored-by: testinginprod <[email protected]> Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: Sukey <[email protected]> Co-authored-by: axie <[email protected]> Co-authored-by: Luke Ma <[email protected]> Co-authored-by: Emmanuel T Odeke <[email protected]> Co-authored-by: 0xn4de <[email protected]> Co-authored-by: hattizai <[email protected]> Co-authored-by: Devon Bear <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Halimao <[email protected]> Co-authored-by: Cosmos SDK <[email protected]> Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Facundo <[email protected]> Co-authored-by: Likhita Polavarapu <[email protected]>
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes a bug discovered by @testinginprod in which post handlers were only run if the transaction execution succeeded, but they were designed to run regardless of the execution's result.
This fix is consensus breaking (especially for chains that have implemented a post handler, for the rest this shouldn't be an issue).
The original PR adding this feature: #13940
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Tests