Skip to content

fix: pass FinalizeBlock request and response to ABCIListener #18486

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

Merged
facundomedica merged 5 commits into from
Nov 16, 2023
Merged

fix: pass FinalizeBlock request and response to ABCIListener #18486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2eb307f
fix: pass FinalizeBlock request and response to ABCIListener
MSalopek Nov 16, 2023
fccfed1
chore: add changelog entry
MSalopek Nov 16, 2023
9f820c4
fix: log errors in FinalizeBlock ABCIListeners instead of panicking
MSalopek Nov 16, 2023
9ba0740
Update baseapp/abci.go
MSalopek Nov 16, 2023
caa228b
chore: appease lint rules
MSalopek Nov 16, 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 @@ -66,6 +66,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Bug Fixes

* (baseapp) [#18486](https://github.com/cosmos/cosmos-sdk/pull/18486) Fixed FinalizeBlock calls not being passed to ABCIListeners
* (baseapp) [#18383](https://github.com/cosmos/cosmos-sdk/pull/18383) Fixed a data race inside BaseApp.getContext, found by end-to-end (e2e) tests.
* (client/server) [#18345](https://github.com/cosmos/cosmos-sdk/pull/18345) Consistently set viper prefix in client and server. It defaults for the binary name for both client and server.
* (simulation) [#17911](https://github.com/cosmos/cosmos-sdk/pull/17911) Fix all problems with executing command `make test-sim-custom-genesis-fast` for simulation test.
Expand Down
8 changes: 8 additions & 0 deletions baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,14 @@ func (app *BaseApp) FinalizeBlock(req *abci.RequestFinalizeBlock) (*abci.Respons
if res != nil {
res.AppHash = app.workingHash()
}

// call the streaming service hooks with the FinalizeBlock messages
for _, streamingListener := range app.streamingManager.ABCIListeners {
if err := streamingListener.ListenFinalizeBlock(app.finalizeBlockState.ctx, *req, *res); err != nil {
Comment thread
tac0turtle marked this conversation as resolved.
Outdated
app.logger.Error("ListenFinalizeBlock listening hook failed", "height", req.Height, "err", err)
}
}

return res, err
}
Comment on lines 891 to 903
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Nov 16, 2023

Choose a reason for hiding this comment

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

The added code block correctly implements the logic to call ListenFinalizeBlock on each ABCIListener registered with the streamingManager. However, there are a few considerations to ensure robustness:

  1. Error Handling: The error from ListenFinalizeBlock is logged but not handled beyond that. If the error indicates a critical failure that should prevent the block from finalizing, this should be considered. If the listeners are not critical to block finalization, the current approach is acceptable.

  2. Context Cancellation: The context passed to ListenFinalizeBlock is the finalizeBlockState.ctx. Ensure that this context is appropriate for the listeners and consider if a separate context with a timeout should be used for each listener to prevent one listener from blocking others in case it hangs or takes too long.

  3. Logging: The error logging is good, but it might be beneficial to also log successful calls to ListenFinalizeBlock for debugging and monitoring purposes.

  4. Performance: If there are many listeners and ListenFinalizeBlock is not a lightweight operation, this could introduce a performance bottleneck. Consider if these calls should be made concurrently.

  5. Resilience: If one listener fails, the loop continues to the next. This is typically desired behavior, but ensure that this aligns with the application's fault tolerance requirements.

  6. State Mutation: If ListenFinalizeBlock mutates shared state, ensure that it's safe from race conditions.

Here's a suggestion to log successful listener invocations and to consider the use of goroutines for concurrent execution if appropriate:

	// call the streaming service hooks with the FinalizeBlock messages
	for _, streamingListener := range app.streamingManager.ABCIListeners {
		if err := streamingListener.ListenFinalizeBlock(app.finalizeBlockState.ctx, *req, *res); err != nil {
			app.logger.Error("ListenFinalizeBlock listening hook failed", "height", req.Height, "err", err)
		} else {
+			app.logger.Info("ListenFinalizeBlock listening hook succeeded", "height", req.Height)
+		}
	}

And if concurrency is deemed necessary:

var wg sync.WaitGroup
for _, streamingListener := range app.streamingManager.ABCIListeners {
	wg.Add(1)
	go func(listener ABCIListener) {
		defer wg.Done()
		if err := listener.ListenFinalizeBlock(app.finalizeBlockState.ctx, *req, *res); err != nil {
			app.logger.Error("ListenFinalizeBlock listening hook failed", "height", req.Height, "err", err)
		} else {
			app.logger.Info("ListenFinalizeBlock listening hook succeeded", "height", req.Height)
		}
	}(streamingListener)
}
wg.Wait()

Note that if you choose to execute the listeners concurrently, you must ensure that any shared state accessed by ListenFinalizeBlock is thread-safe.


Expand Down