fix(baseapp): ensure ABCI listeners always run in FinalizeBlock #19202
Conversation
alexanderbez
added
backport/v0.47.x
WalkthroughThe update to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
|
||
| if app.optimisticExec.Initialized() { | ||
| // check if the hash we got is the same as the one we are executing | ||
| aborted := app.optimisticExec.AbortIfNeeded(req.Hash) | ||
| // Wait for the OE to finish, regardless of whether it was aborted or not | ||
| res, err := app.optimisticExec.WaitResult() | ||
| res, err = app.optimisticExec.WaitResult() | ||
|
|
||
| // only return if we are not aborting | ||
| if !aborted { | ||
| if res != nil { | ||
| res.AppHash = app.workingHash() | ||
| } | ||
|
|
There was a problem hiding this comment.
The logic to handle optimistic execution (OE) seems to be correctly checking if the OE was initialized and aborting if necessary. However, the WaitResult method is called without checking if err is not nil after the OE is aborted. This could potentially lead to a situation where an error from the OE is ignored, and the block execution continues with an invalid state.
if app.optimisticExec.Initialized() {
aborted := app.optimisticExec.AbortIfNeeded(req.Hash)
// Wait for the OE to finish, regardless of whether it was aborted or not
res, err = app.optimisticExec.WaitResult()
// only return if we are not aborting
if !aborted {
if res != nil {
res.AppHash = app.workingHash()
}
return res, err
}
// if it was aborted, we need to reset the state
app.finalizeBlockState = nil
app.optimisticExec.Reset()
}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.
| if app.optimisticExec.Initialized() { | |
| // check if the hash we got is the same as the one we are executing | |
| aborted := app.optimisticExec.AbortIfNeeded(req.Hash) | |
| // Wait for the OE to finish, regardless of whether it was aborted or not | |
| res, err := app.optimisticExec.WaitResult() | |
| res, err = app.optimisticExec.WaitResult() | |
| // only return if we are not aborting | |
| if !aborted { | |
| if res != nil { | |
| res.AppHash = app.workingHash() | |
| } | |
| if app.optimisticExec.Initialized() { | |
| aborted := app.optimisticExec.AbortIfNeeded(req.Hash) | |
| // Wait for the OE to finish, regardless of whether it was aborted or not | |
| res, err = app.optimisticExec.WaitResult() | |
| // only return if we are not aborting | |
| if !aborted { | |
| if res != nil { | |
| res.AppHash = app.workingHash() | |
| } | |
| return res, err | |
| } | |
| // if it was aborted, we need to reset the state | |
| app.finalizeBlockState = nil | |
| app.optimisticExec.Reset() | |
| } | |
| // skipped. This is to support compatibility with proposers injecting vote | ||
| // extensions into the proposal, which should not themselves be executed in cases | ||
| // where they adhere to the sdk.Tx interface. | ||
| func (app *BaseApp) FinalizeBlock(req *abci.RequestFinalizeBlock) (*abci.ResponseFinalizeBlock, error) { | ||
| func (app *BaseApp) FinalizeBlock(req *abci.RequestFinalizeBlock) (res *abci.ResponseFinalizeBlock, err error) { | ||
| defer func() { | ||
| // call the streaming service hooks with the FinalizeBlock messages | ||
| for _, streamingListener := range app.streamingManager.ABCIListeners { | ||
| if err := streamingListener.ListenFinalizeBlock(app.finalizeBlockState.Context(), *req, *res); err != nil { | ||
| app.logger.Error("ListenFinalizeBlock listening hook failed", "height", req.Height, "err", err) | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| if app.optimisticExec.Initialized() { | ||
| // check if the hash we got is the same as the one we are executing | ||
| aborted := app.optimisticExec.AbortIfNeeded(req.Hash) | ||
| // Wait for the OE to finish, regardless of whether it was aborted or not | ||
| res, err := app.optimisticExec.WaitResult() | ||
| res, err = app.optimisticExec.WaitResult() | ||
|
|
||
| // only return if we are not aborting | ||
| if !aborted { | ||
| if res != nil { | ||
| res.AppHash = app.workingHash() | ||
| } | ||
|
|
||
| return res, err | ||
| } | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1043]
Overall, the changes made to the FinalizeBlock method and related logic in the BaseApp struct are consistent with the PR objectives. The use of defer to ensure ABCI listeners are executed is a good practice. However, there is an issue with the handling of errors after aborting optimistic execution. This should be addressed to ensure that errors are not ignored and that the application state remains valid.
(cherry picked from commit bda2d11)
(cherry picked from commit bda2d11) # Conflicts: # baseapp/abci.go
julienrbrt
removed
the
backport/v0.47.x
…port #19202) (#19217) Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Description
Changelog
Ensure ABCI listeners always execute in
FinalizeBlock, regardless if OE is enabled or not. I opted to do this via adeferto make it as clean as possible.closes: #19196
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...