feat(server/v2/cometbft): optimistic execution #22560
Conversation
randygrok
requested review from
facundomedica,
sontrinh16 and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the consensus mechanism in the CometBFT server by adding optimistic execution functionality. A new field is added to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@randygrok has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:server/v2
|
@randygrok your pull request is missing a changelog! |
julienrbrt
added
the
backport/v0.52.x
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (2)
server/v2/cometbft/oe/optimistic_execution_test.go (1)
14-16: Enhance test helper function coverageThe current helper function only tests the error path. Consider expanding it to:
- Add documentation explaining its purpose
- Support both success and failure scenarios
- Make it parameterized for different test cases
+// testFinalizeBlock is a test helper that simulates finalization responses +// for optimistic execution testing func testFinalizeBlock(_ context.Context, _ *abci.FinalizeBlockRequest) (*abci.FinalizeBlockResponse, error) { - return nil, errors.New("test error") + switch { + case len(req.Hash) == 0: + return nil, errors.New("test error") + default: + return &abci.FinalizeBlockResponse{}, nil + } }server/v2/cometbft/abci_test.go (1)
721-792: Consider adding a test case for successful optimistic executionCurrently, the
TestOptimisticExecutionfunction simulates an error scenario by havingoptimisticMockFuncreturn an error. To thoroughly validate the optimistic execution feature, it would be beneficial to include a test case whereoptimisticMockFunccompletes successfully. This would verify the behavior when optimistic execution proceeds without errors and ensures that both success and failure paths are adequately tested.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
server/v2/cometbft/abci.go(8 hunks)server/v2/cometbft/abci_test.go(4 hunks)server/v2/cometbft/oe/optimistic_execution.go(1 hunks)server/v2/cometbft/oe/optimistic_execution_test.go(1 hunks)server/v2/cometbft/server.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/cometbft/oe/optimistic_execution.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/oe/optimistic_execution_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/cometbft/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (11)
server/v2/cometbft/oe/optimistic_execution_test.go (2)
1-12: LGTM: Clean package structure and imports
The package name and imports are well-organized and appropriate for the test file.
1-34: Verify test coverage for optimistic execution implementation
Let's ensure adequate test coverage for the optimistic execution functionality.
✅ Verification successful
Test coverage is comprehensive for optimistic execution implementation
The test coverage analysis shows that all critical methods of the OptimisticExecution struct are being tested:
- Constructor (NewOptimisticExecution) and configuration (WithAbortRate)
- Core functionality methods: Execute, WaitResult, AbortIfNeeded, Reset
- State check methods: Enabled, Initialized
The test file includes:
- Error handling verification
- Hash mismatch scenarios
- State transitions (initialization, execution, reset)
- Proper mutex handling through concurrent operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for the optimistic execution package
# Find the implementation file
implementation_file=$(fd -e go -E '*_test.go' . server/v2/cometbft/oe)
# Run coverage analysis
go test -coverprofile=coverage.out ./server/v2/cometbft/oe/...
go tool cover -func=coverage.out | grep -E "optimistic_execution.go.*statements"
Length of output: 392
Script:
#!/bin/bash
# Let's analyze the test coverage by examining the test file against the implementation
# First, find all functions in the implementation file
echo "=== Implementation functions ==="
ast-grep --pattern 'func $_($$$) $$$' server/v2/cometbft/oe/optimistic_execution.go
# Then, find all test functions to verify coverage
echo -e "\n=== Test functions ==="
ast-grep --pattern 'func Test$_($$$) $$$' server/v2/cometbft/oe/optimistic_execution_test.go
# Also check for any mock/helper test functions
echo -e "\n=== Helper/Mock functions ==="
ast-grep --pattern 'func $_($$$) $$$' server/v2/cometbft/oe/optimistic_execution_test.go
Length of output: 3231
Script:
#!/bin/bash
# Let's examine the implementation methods to ensure all are covered by tests
# Get all methods of OptimisticExecution struct
echo "=== OptimisticExecution methods ==="
ast-grep --pattern 'func (oe *OptimisticExecution) $_($$$) $$$' server/v2/cometbft/oe/optimistic_execution.go
# Get the struct definition to understand all fields that need testing
echo -e "\n=== OptimisticExecution struct ==="
ast-grep --pattern 'type OptimisticExecution struct {$$$}' server/v2/cometbft/oe/optimistic_execution.go
Length of output: 8522
server/v2/cometbft/server.go (2)
5-5: LGTM: Import statement follows Go conventions
The import statement for the optimistic execution package is correctly placed and follows the project's module structure.
137-138: Consider adding configuration and documentation for optimistic execution
While the integration point is correctly placed, there are a few considerations:
- Consider adding configuration options to enable/disable optimistic execution
- Add error handling for the setup process
- Document the feature and its implications in the code
Let's verify the configuration and documentation:
server/v2/cometbft/abci.go (3)
5-5: Import of oe package added correctly
The import of cosmossdk.io/server/v2/cometbft/oe is required for the OptimisticExecution feature.
16-16: Import of protoreflect package added correctly
The import of google.golang.org/protobuf/reflect/protoreflect is necessary for handling gRPC queries.
126-128: SetOptimisticExecution method added appropriately
The SetOptimisticExecution method correctly assigns the optimisticExec field.
server/v2/cometbft/abci_test.go (4)
5-5: Approved: Added import for optimistic execution package
The import of "cosmossdk.io/server/v2/cometbft/oe" is appropriate and necessary for implementing optimistic execution functionality in the tests.
8-8: Approved: Included standard errors package for error handling
Adding the "errors" package is essential for error generation and handling within the test code.
60-63: Approved: Enhanced generic parameters in getQueryRouterBuilder
The addition of generic parameters U and UT improves the flexibility and type safety of the getQueryRouterBuilder function, allowing it to handle a wider range of message types.
90-93: Approved: Enhanced generic parameters in getMsgRouterBuilder
The addition of generic parameters U and UT enhances the getMsgRouterBuilder function's capability to work with various message types, increasing its reusability and type safety.
| func TestOptimisticExecution(t *testing.T) { | ||
| oe := NewOptimisticExecution(log.NewNopLogger(), testFinalizeBlock) | ||
| assert.True(t, oe.Enabled()) | ||
| oe.Execute(&abci.ProcessProposalRequest{ | ||
| Hash: []byte("test"), | ||
| }) | ||
| assert.True(t, oe.Initialized()) | ||
|
|
||
| resp, err := oe.WaitResult() | ||
| assert.Nil(t, resp) | ||
| assert.EqualError(t, err, "test error") | ||
|
|
||
| assert.False(t, oe.AbortIfNeeded([]byte("test"))) | ||
| assert.True(t, oe.AbortIfNeeded([]byte("wrong_hash"))) | ||
|
|
||
| oe.Reset() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test coverage and organization
While the test covers basic functionality, it needs improvements in several areas:
- Missing test cases for concurrent execution
- No validation of actual response content
- Lack of edge cases
- No cleanup mechanism
Consider restructuring the test like this:
func TestOptimisticExecution(t *testing.T) {
tests := []struct {
name string
hash []byte
wantErr bool
errMsg string
setup func(*OptimisticExecution)
validate func(*testing.T, *OptimisticExecution)
}{
{
name: "successful execution",
hash: []byte("test"),
wantErr: false,
},
{
name: "error case",
hash: []byte{},
wantErr: true,
errMsg: "test error",
},
// Add more test cases
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
oe := NewOptimisticExecution(log.NewNopLogger(), testFinalizeBlock)
defer oe.Reset() // Ensure cleanup
if tt.setup != nil {
tt.setup(oe)
}
oe.Execute(&abci.ProcessProposalRequest{Hash: tt.hash})
resp, err := oe.WaitResult()
if tt.wantErr {
assert.Error(t, err)
assert.Equal(t, tt.errMsg, err.Error())
assert.Nil(t, resp)
} else {
assert.NoError(t, err)
assert.NotNil(t, resp)
}
if tt.validate != nil {
tt.validate(t, oe)
}
})
}
}
// Add concurrent execution test
func TestOptimisticExecutionConcurrent(t *testing.T) {
oe := NewOptimisticExecution(log.NewNopLogger(), testFinalizeBlock)
defer oe.Reset()
const numGoroutines = 10
var wg sync.WaitGroup
wg.Add(numGoroutines)
for i := 0; i < numGoroutines; i++ {
go func() {
defer wg.Done()
oe.Execute(&abci.ProcessProposalRequest{Hash: []byte("test")})
_, _ = oe.WaitResult()
}()
}
wg.Wait()
}| defer oe.mtx.Unlock() | ||
|
|
||
| if !bytes.Equal(oe.request.Hash, reqHash) { | ||
| oe.logger.Error("OE aborted due to hash mismatch", "oe_hash", hex.EncodeToString(oe.request.Hash), "req_hash", hex.EncodeToString(reqHash), "oe_height", oe.request.Height, "req_height", oe.request.Height) |
There was a problem hiding this comment.
Correct the logging of req_height in error message
In the error log within AbortIfNeeded, both oe_height and req_height are using oe.request.Height, which may not accurately reflect the current request's height. This could lead to confusion when diagnosing issues.
Adjust the log statement to use the correct height for req_height:
oe.logger.Error("OE aborted due to hash mismatch",
"oe_hash", hex.EncodeToString(oe.request.Hash),
"req_hash", hex.EncodeToString(reqHash),
"oe_height", oe.request.Height,
- "req_height", oe.request.Height)
+ "req_height", currentRequestHeight)If currentRequestHeight is not available in this context, you may need to pass the request's height as an additional parameter to AbortIfNeeded.
Committable suggestion skipped: line range outside the PR's diff.
| if !bytes.Equal(oe.request.Hash, reqHash) { | ||
| oe.logger.Error("OE aborted due to hash mismatch", "oe_hash", hex.EncodeToString(oe.request.Hash), "req_hash", hex.EncodeToString(reqHash), "oe_height", oe.request.Height, "req_height", oe.request.Height) | ||
| oe.cancelFunc() | ||
| return true |
There was a problem hiding this comment.
Prevent potential nil pointer dereference when accessing oe.request
In the AbortIfNeeded method, if oe.request is nil, accessing oe.request.Hash will cause a runtime panic. This could happen if AbortIfNeeded is called before Execute, or if Execute failed to initialize oe.request.
Consider adding a nil check for oe.request before accessing its fields:
+ if oe.request == nil {
+ oe.logger.Error("OE aborted due to missing request")
+ oe.cancelFunc()
+ return true
+ }
if !bytes.Equal(oe.request.Hash, reqHash) {
oe.logger.Error("OE aborted due to hash mismatch", "oe_hash", hex.EncodeToString(oe.request.Hash), "req_hash", hex.EncodeToString(reqHash), "oe_height", oe.request.Height, "req_height", oe.request.Height)
oe.cancelFunc()
return true
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !bytes.Equal(oe.request.Hash, reqHash) { | |
| oe.logger.Error("OE aborted due to hash mismatch", "oe_hash", hex.EncodeToString(oe.request.Hash), "req_hash", hex.EncodeToString(reqHash), "oe_height", oe.request.Height, "req_height", oe.request.Height) | |
| oe.cancelFunc() | |
| return true | |
| if oe.request == nil { | |
| oe.logger.Error("OE aborted due to missing request") | |
| oe.cancelFunc() | |
| return true | |
| } | |
| if !bytes.Equal(oe.request.Hash, reqHash) { | |
| oe.logger.Error("OE aborted due to hash mismatch", "oe_hash", hex.EncodeToString(oe.request.Hash), "req_hash", hex.EncodeToString(reqHash), "oe_height", oe.request.Height, "req_height", oe.request.Height) | |
| oe.cancelFunc() | |
| return true |
server/v2/cometbft/abci.go
Outdated
| // optimisticExec contains the context required for Optimistic Execution, | ||
| // including the goroutine handling.This is experimental and must be enabled | ||
| // by developers. | ||
| optimisticExec *oe.OptimisticExecution |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider initializing optimisticExec to avoid nil references
The optimisticExec field is added to the Consensus struct but may remain nil if SetOptimisticExecution is not called. To prevent potential nil pointer dereferences, consider initializing optimisticExec during construction or ensure it is always set before use.
server/v2/cometbft/abci.go
Outdated
| // Since the application can get access to FinalizeBlock state and write to it, | ||
| // we must be sure to reset it in case ProcessProposal timeouts and is called | ||
| // again in a subsequent round. However, we only want to do this after we've | ||
| // processed the first block, as we want to avoid overwriting the finalizeState | ||
| // after state changes during InitChain. | ||
| if req.Height > int64(c.initialHeight) { | ||
| // abort any running OE | ||
| c.optimisticExec.Abort() | ||
| //c.setState(execModeFinalize, header) | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential nil pointer dereference in ProcessProposal
In ProcessProposal, c.optimisticExec.Abort() is invoked without verifying if optimisticExec is nil. This could lead to a runtime panic if optimisticExec is not initialized.
Apply this diff to add a nil check before calling Abort():
if req.Height > int64(c.initialHeight) {
// abort any running OE
- c.optimisticExec.Abort()
+ if c.optimisticExec != nil {
+ c.optimisticExec.Abort()
+ }
//c.setState(execModeFinalize, header)
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Since the application can get access to FinalizeBlock state and write to it, | |
| // we must be sure to reset it in case ProcessProposal timeouts and is called | |
| // again in a subsequent round. However, we only want to do this after we've | |
| // processed the first block, as we want to avoid overwriting the finalizeState | |
| // after state changes during InitChain. | |
| if req.Height > int64(c.initialHeight) { | |
| // abort any running OE | |
| c.optimisticExec.Abort() | |
| //c.setState(execModeFinalize, header) | |
| } | |
| // Since the application can get access to FinalizeBlock state and write to it, | |
| // we must be sure to reset it in case ProcessProposal timeouts and is called | |
| // again in a subsequent round. However, we only want to do this after we've | |
| // processed the first block, as we want to avoid overwriting the finalizeState | |
| // after state changes during InitChain. | |
| if req.Height > int64(c.initialHeight) { | |
| // abort any running OE | |
| if c.optimisticExec != nil { | |
| c.optimisticExec.Abort() | |
| } | |
| //c.setState(execModeFinalize, header) | |
| } |
server/v2/cometbft/abci.go
Outdated
| if c.optimisticExec.Initialized() { | ||
| // check if the hash we got is the same as the one we are executing | ||
| aborted := c.optimisticExec.AbortIfNeeded(req.Hash) | ||
|
|
||
| // Wait for the OE to finish, regardless of whether it was aborted or not | ||
| res, err := c.optimisticExec.WaitResult() | ||
|
|
||
| // only return if we are not aborting | ||
| if !aborted { | ||
| return res, err | ||
| } | ||
|
|
||
| // if it was aborted, we need to reset the state | ||
| c.optimisticExec.Reset() | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential nil pointer dereference in FinalizeBlock
In FinalizeBlock, c.optimisticExec.Initialized() is called without checking if optimisticExec is nil. This could cause a runtime panic if optimisticExec is not initialized.
Apply this diff to add a nil check before calling Initialized():
if c.optimisticExec != nil && c.optimisticExec.Initialized() {
// Existing code...
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if c.optimisticExec.Initialized() { | |
| // check if the hash we got is the same as the one we are executing | |
| aborted := c.optimisticExec.AbortIfNeeded(req.Hash) | |
| // Wait for the OE to finish, regardless of whether it was aborted or not | |
| res, err := c.optimisticExec.WaitResult() | |
| // only return if we are not aborting | |
| if !aborted { | |
| return res, err | |
| } | |
| // if it was aborted, we need to reset the state | |
| c.optimisticExec.Reset() | |
| } | |
| if c.optimisticExec != nil && c.optimisticExec.Initialized() { | |
| // check if the hash we got is the same as the one we are executing | |
| aborted := c.optimisticExec.AbortIfNeeded(req.Hash) | |
| // Wait for the OE to finish, regardless of whether it was aborted or not | |
| res, err := c.optimisticExec.WaitResult() | |
| // only return if we are not aborting | |
| if !aborted { | |
| return res, err | |
| } | |
| // if it was aborted, we need to reset the state | |
| c.optimisticExec.Reset() | |
| } |
server/v2/cometbft/abci.go
Outdated
| // Only execute optimistic execution if the proposal is accepted, OE is | ||
| // enabled and the block height is greater than the initial height. During | ||
| // the first block we'll be carrying state from InitChain, so it would be | ||
| // impossible for us to easily revert. | ||
| // After the first block has been processed, the next blocks will get executed | ||
| // optimistically, so that when the ABCI client calls `FinalizeBlock` the app | ||
| // can have a response ready. | ||
| if c.optimisticExec.Enabled() && req.Height > int64(c.initialHeight) { | ||
| c.optimisticExec.Execute(req) | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential nil pointer dereference when checking optimisticExec.Enabled()
In ProcessProposal, c.optimisticExec.Enabled() is called without verifying if optimisticExec is nil. This may result in a runtime panic if optimisticExec is not set.
Apply this diff to add a nil check:
if c.optimisticExec != nil && c.optimisticExec.Enabled() && req.Height > int64(c.initialHeight) {
c.optimisticExec.Execute(req)
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Only execute optimistic execution if the proposal is accepted, OE is | |
| // enabled and the block height is greater than the initial height. During | |
| // the first block we'll be carrying state from InitChain, so it would be | |
| // impossible for us to easily revert. | |
| // After the first block has been processed, the next blocks will get executed | |
| // optimistically, so that when the ABCI client calls `FinalizeBlock` the app | |
| // can have a response ready. | |
| if c.optimisticExec.Enabled() && req.Height > int64(c.initialHeight) { | |
| c.optimisticExec.Execute(req) | |
| } | |
| // Only execute optimistic execution if the proposal is accepted, OE is | |
| // enabled and the block height is greater than the initial height. During | |
| // the first block we'll be carrying state from InitChain, so it would be | |
| // impossible for us to easily revert. | |
| // After the first block has been processed, the next blocks will get executed | |
| // optimistically, so that when the ABCI client calls `FinalizeBlock` the app | |
| // can have a response ready. | |
| if c.optimisticExec != nil && c.optimisticExec.Enabled() && req.Height > int64(c.initialHeight) { | |
| c.optimisticExec.Execute(req) | |
| } | |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no suggestions.
server/v2/cometbft/abci.go
Outdated
| c.optimisticExec.Reset() | ||
| } | ||
|
|
||
| return c.internalFinalizeBlock(ctx, req) |
There was a problem hiding this comment.
Can't comment on the right line below, but internalFinalizeBlock should not commit to store. I think internalFinalizeBlock should return stateChanges, then everything else should be done on the actual FinalizeBlock call.
This is because there could be an equivocation and we could be running the wrong finalize block request, and this would result in us writing bad data to disk. Also, if internalFinalizeBlock finishes then ProcessProposal would be accessing data that is in the "future"
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no suggestions.
Comments skipped due to low confidence (1)
server/v2/cometbft/abci_test.go:784
- The comment should be 'Because it is aborted, the result comes from the normal execution'.
// Because is aborted, the result comes from the normal execution
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
server/v2/cometbft/abci_test.go (2)
736-740: Consider adding more realistic mock scenariosThe current mock function always returns an error. Consider adding test cases with successful execution paths and varying response data to better simulate production scenarios.
- optimisticMockFunc := func(_ context.Context, _ *abciproto.FinalizeBlockRequest) (*abciproto.FinalizeBlockResponse, error) { - calledTimes++ - return nil, errors.New("test error") - } + optimisticMockFunc := func(_ context.Context, req *abciproto.FinalizeBlockRequest) (*abciproto.FinalizeBlockResponse, error) { + calledTimes++ + if req.Height == 2 { + return nil, errors.New("test error") + } + return &abciproto.FinalizeBlockResponse{ + Events: []*abciproto.Event{ + {Type: "optimistic_execution", Attributes: []*abciproto.EventAttribute{{Key: "success", Value: "true"}}}, + }, + }, nil + }
773-774: Remove redundant hash calculationThe hash was already calculated on line 759 and stored in
theHash. No need to recalculate it.- theHash = sha256.Sum256([]byte("test"))
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/cometbft/abci.go(6 hunks)server/v2/cometbft/abci_test.go(4 hunks)server/v2/cometbft/server.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/server.go
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (7)
server/v2/cometbft/abci.go (4)
81-84: Field documentation looks good
394-401: Abort logic is well documented
Line range hint 438-474: Logic for handling optimistic execution is sound
486-501: Consider potential race conditions in optimistic execution handling
The optimistic execution handling logic should be protected against potential race conditions where multiple goroutines might try to access the execution state simultaneously.
✅ Verification successful
Let me analyze the optimistic execution mutex usage further to verify the race condition concern.
Let me check the complete implementation of the OptimisticExecution type to verify all mutex-protected operations.
Race conditions are properly handled in optimistic execution implementation
The optimistic execution implementation has proper synchronization mechanisms in place:
- All shared state access is protected by a mutex
- Critical sections are kept small and well-defined
- Goroutine lifecycle is properly managed with channels and context cancellation
- State transitions are atomic and thread-safe
The implementation shows careful consideration of concurrent access patterns:
- State modifications are protected by
mtx.Lock()/Unlock() - Channel operations (
stopCh) are used for synchronization - Context cancellation is used for clean goroutine termination
- Initialization and reset operations are mutex-protected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mutex usage in optimistic execution related code
ast-grep --pattern 'sync.Mutex'
rg -A 5 'sync\.Mutex'
Length of output: 14839
Script:
#!/bin/bash
# Check the implementation of optimistic execution methods
ast-grep --pattern 'func ($x *OptimisticExecution) $method($$$) {
$$$
}'
# Check for concurrent access patterns
rg -A 10 'go func' server/v2/cometbft/oe/
Length of output: 966
Script:
#!/bin/bash
# Get the full implementation of OptimisticExecution
cat server/v2/cometbft/oe/optimistic_execution.go
# Check for any other files that might interact with OptimisticExecution
rg -l "OptimisticExecution" server/v2/cometbft/
Length of output: 5137
server/v2/cometbft/abci_test.go (3)
5-5: LGTM: Import additions are appropriate
The new imports support the optimistic execution feature and error handling requirements.
Also applies to: 8-8
61-64: LGTM: Generic function signature formatting improvements
The generic function signatures have been reformatted for better readability while maintaining the same functionality.
Also applies to: 91-94
730-801: Comprehensive test coverage for optimistic execution
The test thoroughly covers the optimistic execution functionality:
- Success path with correct hash
- Error handling from optimistic execution
- Fallback to normal execution with wrong hash
- Reset verification
However, consider adding the following test cases:
- Concurrent optimistic executions
- Edge cases with nil handlers
- Recovery after failed optimistic execution
Also, verify that the mock function behavior matches the production scenarios:
✅ Verification successful
Mock implementation aligns well with production scenarios
The test's mock implementation correctly simulates the production behavior:
- It uses the same function signature as the production
FinalizeBlockFunc - The error handling matches the production error propagation pattern
- The test verifies core optimistic execution features that are present in production:
- Hash-based validation
- Execution state management
- Proper reset behavior
- Error propagation
The mock's simplicity (always returning an error) is appropriate for testing the error handling path, while other test files cover the success scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for production implementations of optimistic execution
ast-grep --pattern 'func $_(context.Context, *abciproto.FinalizeBlockRequest) (*abciproto.FinalizeBlockResponse, error) {
$$$
}'
Length of output: 132
Script:
#!/bin/bash
# Let's try a broader search for optimistic execution implementations
rg -l "OptimisticExecution" --type go
# Then look for FinalizeBlock implementations
rg "FinalizeBlock.*\(.*context\.Context.*\*abciproto\.FinalizeBlockRequest\)" --type go -A 5
Length of output: 940
Script:
#!/bin/bash
# Let's examine the actual optimistic execution implementation
cat server/v2/cometbft/oe/optimistic_execution.go
# And check how it's used in production
rg "NewOptimisticExecution" --type go -A 5
Length of output: 8361
server/v2/cometbft/abci.go
Outdated
| func (c *consensus[T]) internalFinalizeBlock( | ||
| ctx context.Context, | ||
| req *abciproto.FinalizeBlockRequest, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider separating state changes from commit
As noted in previous comments, committing state changes directly in internalFinalizeBlock could lead to issues with equivocation and future state access. Consider returning state changes and handling the commit in the actual FinalizeBlock call.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
server/v2/cometbft/server.go (1)
196-202: Document optimistic execution integration pointsAs this introduces a significant new feature (optimistic execution consensus v2), please ensure:
- Documentation exists explaining the integration points and configuration options
- Examples of common usage patterns are provided
- Performance implications and trade-offs are documented
This will help maintainers and users understand when and how to use this feature effectively.
server/v2/cometbft/abci_test.go (2)
730-801: Enhance test robustnessWhile the test covers the main paths, consider these improvements:
- Extract magic values like "test" and "wrong_hash" into named constants
- Add assertions for the response content when optimistic execution is aborted
- Consider adding test cases for concurrent optimistic executions
🧰 Tools
🪛 golangci-lint (1.62.2)
780-780: ineffectual assignment to fbResp
(ineffassign)
737-742: Consider enhancing mock function coverageThe mock function currently only tests the error path. Consider parameterizing it to test successful optimistic execution as well.
+ type mockResult struct { + resp *abciproto.FinalizeBlockResponse + err error + } + mockResults := []mockResult{{nil, errors.New("test error")}} + currentResult := 0 optimisticMockFunc := func(_ context.Context, _ *abciproto.FinalizeBlockRequest) (*abciproto.FinalizeBlockResponse, error) { calledTimes++ - return nil, errors.New("test error") + result := mockResults[currentResult] + currentResult = (currentResult + 1) % len(mockResults) + return result.resp, result.err }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
server/v2/cometbft/abci_test.go(4 hunks)server/v2/cometbft/server.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/cometbft/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/cometbft/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/cometbft/abci_test.go
14-14: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
61-61: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
91-91: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
780-780: ineffectual assignment to fbResp
(ineffassign)
server/v2/cometbft/server.go
5-5: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🔇 Additional comments (4)
server/v2/cometbft/server.go (2)
Line range hint 168-195: LGTM: Well-structured consensus initialization
The consensus initialization is comprehensive and follows good practices with proper handler separation and thread-safe primitives.
196-199: Verify optimistic execution lifecycle management
While the initialization looks correct, we should verify that the optimistic execution context is properly managed throughout its lifecycle.
✅ Verification successful
Optimistic execution lifecycle is properly managed
The optimistic execution implementation has comprehensive lifecycle management:
- Proper initialization through
NewOptimisticExecution - Thread-safe execution with mutex protection
- Clean abort mechanism with context cancellation
- Proper cleanup through
Reset()method - Graceful shutdown with
stopChchannel - Error handling and result propagation via
WaitResult()
The implementation includes all necessary lifecycle hooks and cleanup mechanisms, making the initialization in the review snippet safe and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cleanup/shutdown handling of optimistic execution
rg -A 5 "optimisticExec.*(?:Stop|Close|Shutdown|Clean)" --type go
# Search for potential error handling of optimistic execution
rg -A 5 "optimisticExec.*err" --type go
Length of output: 109
Script:
#!/bin/bash
# Let's try a broader search for optimistic execution related code
rg -A 10 "optimisticExec" --type go
# Search for the OptimisticExecution type definition and methods
ast-grep --pattern 'type OptimisticExecution struct {
$$$
}'
# Search for any Stop/Close methods in the optimistic execution package
fd -e go . | grep -i "optimistic" | xargs cat
Length of output: 24187
server/v2/cometbft/abci_test.go (2)
7-7: LGTM: Import additions are appropriate
The new imports support the optimistic execution test implementation.
Also applies to: 14-14
61-64: LGTM: Interface formatting improvements
The interface constraint formatting changes improve readability without affecting functionality.
Also applies to: 91-94
🧰 Tools
🪛 golangci-lint (1.62.2)
61-61: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
| fbResp, err := c.FinalizeBlock(context.Background(), fbReq) | ||
| require.Error(t, err) | ||
| require.ErrorContains(t, err, "test error") // from optimisticMockFunc | ||
| require.Equal(t, 1, calledTimes) | ||
|
|
There was a problem hiding this comment.
Fix ineffectual assignment
The fbResp assignment on line 780 is never used. Consider removing it or adding assertions on the response content.
- fbResp, err := c.FinalizeBlock(context.Background(), fbReq)
+ _, err = c.FinalizeBlock(context.Background(), fbReq)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fbResp, err := c.FinalizeBlock(context.Background(), fbReq) | |
| require.Error(t, err) | |
| require.ErrorContains(t, err, "test error") // from optimisticMockFunc | |
| require.Equal(t, 1, calledTimes) | |
| _, err = c.FinalizeBlock(context.Background(), fbReq) | |
| require.Error(t, err) | |
| require.ErrorContains(t, err, "test error") // from optimisticMockFunc | |
| require.Equal(t, 1, calledTimes) |
🧰 Tools
🪛 golangci-lint (1.62.2)
780-780: ineffectual assignment to fbResp
(ineffassign)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
server/v2/cometbft/abci_test.go (3)
737-742: Consider adding more test scenarios for the mock functionThe mock function currently only tests the error case. Consider adding test cases for:
- Successful execution
- Different types of errors
- Panic recovery
calledTimes := 0 +mockResponse := &abciproto.FinalizeBlockResponse{} +mockError := errors.New("test error") optimisticMockFunc := func(_ context.Context, _ *abciproto.FinalizeBlockRequest) (*abciproto.FinalizeBlockResponse, error) { calledTimes++ - return nil, errors.New("test error") + if calledTimes == 1 { + return mockResponse, nil + } + return nil, mockError }
773-784: Add assertions for the response contentThe test verifies the error but doesn't check the response content. Consider adding assertions for the response fields when testing with the correct hash.
theHash = sha256.Sum256([]byte("test")) fbReq := &abciproto.FinalizeBlockRequest{ Height: 2, Hash: theHash[:], Time: ppReq.Time, Txs: ppReq.Txs, } fbResp, err := c.FinalizeBlock(context.Background(), fbReq) require.Error(t, err) require.ErrorContains(t, err, "test error") +require.Nil(t, fbResp) require.Equal(t, 1, calledTimes)
730-801: Add test coverage for edge casesThe test suite should include additional edge cases:
- Zero height
- Empty transaction list
- Invalid transaction format
- Timeout scenarios
Example test case for timeout:
func TestOptimisticExecutionTimeout(t *testing.T) { c := setUpConsensus(t, 100_000, mempool.NoOpMempool[mock.Tx]{}) // Mock function that simulates a timeout optimisticMockFunc := func(_ context.Context, _ *abciproto.FinalizeBlockRequest) (*abciproto.FinalizeBlockResponse, error) { time.Sleep(2 * time.Second) return nil, nil } c.optimisticExec = oe.NewOptimisticExecution(log.NewNopLogger(), optimisticMockFunc) ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) defer cancel() // Test should handle timeout gracefully _, err := c.ProcessProposal(ctx, &abciproto.ProcessProposalRequest{ Height: 1, Time: time.Now(), }) require.Error(t, err) require.Contains(t, err.Error(), "context deadline exceeded") }server/v2/cometbft/abci.go (2)
81-85: Consider documenting thread-safety guaranteesThe
optimisticExecfield handles goroutines and concurrent operations. Consider adding documentation about thread-safety guarantees and synchronization requirements.// optimisticExec contains the context required for Optimistic Execution, -// including the goroutine handling.This is experimental and must be enabled -// by developers. +// including the goroutine handling. This is experimental and must be enabled +// by developers. Thread-safety: All operations on this field are thread-safe +// and internally synchronized.
557-601: Consider adding metrics for optimistic executionThe
internalFinalizeBlockmethod is crucial for performance. Consider adding metrics to track execution time and success/failure rates.func (c *consensus[T]) internalFinalizeBlock( ctx context.Context, req *abciproto.FinalizeBlockRequest, ) (*server.BlockResponse, store.WriterMap, []T, error) { + start := time.Now() + defer func() { + metrics.ObserveFinalizeBlockDuration(time.Since(start)) + }() // ... existing code ... }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/cometbft/abci.go(7 hunks)server/v2/cometbft/abci_test.go(2 hunks)server/v2/cometbft/oe/optimistic_execution.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/cometbft/oe/optimistic_execution.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/cometbft/oe/optimistic_execution.go
1-1: : # cosmossdk.io/server/v2/cometbft/oe [cosmossdk.io/server/v2/cometbft/oe.test]
oe/optimistic_execution_test.go:19:51: in call to NewOptimisticExecution, type func(_ context.Context, _ "github.com/cometbft/cometbft/api/cometbft/abci/v1".FinalizeBlockRequest) ("github.com/cometbft/cometbft/api/cometbft/abci/v1".FinalizeBlockResponse, error) of testFinalizeBlock does not match FinalizeBlockFunc[T] (cannot infer T)
(typecheck)
🔇 Additional comments (3)
server/v2/cometbft/abci.go (2)
464-474:
Add defensive nil check for optimisticExec
The code directly accesses optimisticExec without checking if it's nil, which could lead to a panic.
-if c.optimisticExec.Enabled() && req.Height > int64(c.initialHeight) {
+if c.optimisticExec != nil && c.optimisticExec.Enabled() && req.Height > int64(c.initialHeight) {
c.optimisticExec.Execute(req)
}Likely invalid or redundant comment.
569-572: Document assumptions about transaction decoding
The TODO comment raises important questions about transaction decoding. This should be properly documented and handled.
server/v2/cometbft/oe/optimistic_execution.go (1)
1-174: Code is well-structured and adheres to best practices
The OptimisticExecution implementation, along with its associated methods and generic types, is well-designed. The code correctly manages concurrency with proper synchronization using mutexes and channels. The use of context cancellation and error handling is appropriate, and the overall structure aligns with the Go language standards and the Uber Go Style Guide.
🧰 Tools
🪛 golangci-lint (1.62.2)
1-1: : # cosmossdk.io/server/v2/cometbft/oe [cosmossdk.io/server/v2/cometbft/oe.test]
oe/optimistic_execution_test.go:19:51: in call to NewOptimisticExecution, type func(_ context.Context, _ "github.com/cometbft/cometbft/api/cometbft/abci/v1".FinalizeBlockRequest) ("github.com/cometbft/cometbft/api/cometbft/abci/v1".FinalizeBlockResponse, error) of testFinalizeBlock does not match FinalizeBlockFunc[T] (cannot infer T)
(typecheck)
server/v2/cometbft/abci_test.go
Outdated
| func TestOptimisticExecution(t *testing.T) { | ||
| c := setUpConsensus(t, 100_000, mempool.NoOpMempool[mock.Tx]{}) | ||
|
|
||
| // Set up handlers | ||
| c.processProposalHandler = DefaultServerOptions[mock.Tx]().ProcessProposalHandler | ||
|
|
||
| // mock optimistic execution | ||
| calledTimes := 0 | ||
| optimisticMockFunc := func(_ context.Context, _ *abciproto.FinalizeBlockRequest) (*abciproto.FinalizeBlockResponse, error) { | ||
| calledTimes++ | ||
| return nil, errors.New("test error") | ||
| } | ||
| c.optimisticExec = oe.NewOptimisticExecution(log.NewNopLogger(), optimisticMockFunc) | ||
|
|
||
| _, err := c.InitChain(context.Background(), &abciproto.InitChainRequest{ | ||
| Time: time.Now(), | ||
| ChainId: "test", | ||
| InitialHeight: 1, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = c.FinalizeBlock(context.Background(), &abciproto.FinalizeBlockRequest{ | ||
| Time: time.Now(), | ||
| Height: 1, | ||
| Txs: [][]byte{mockTx.Bytes()}, | ||
| Hash: emptyHash[:], | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| theHash := sha256.Sum256([]byte("test")) | ||
| ppReq := &abciproto.ProcessProposalRequest{ | ||
| Height: 2, | ||
| Hash: theHash[:], | ||
| Time: time.Now(), | ||
| Txs: [][]byte{mockTx.Bytes()}, | ||
| } | ||
|
|
||
| // Start optimistic execution | ||
| resp, err := c.ProcessProposal(context.Background(), ppReq) | ||
| require.NoError(t, err) | ||
| require.Equal(t, resp.Status, abciproto.PROCESS_PROPOSAL_STATUS_ACCEPT) | ||
|
|
||
| // Initialize FinalizeBlock with correct hash - should use optimistic result | ||
| theHash = sha256.Sum256([]byte("test")) | ||
| fbReq := &abciproto.FinalizeBlockRequest{ | ||
| Height: 2, | ||
| Hash: theHash[:], | ||
| Time: ppReq.Time, | ||
| Txs: ppReq.Txs, | ||
| } | ||
| fbResp, err := c.FinalizeBlock(context.Background(), fbReq) | ||
| require.Error(t, err) | ||
| require.ErrorContains(t, err, "test error") // from optimisticMockFunc | ||
| require.Equal(t, 1, calledTimes) | ||
|
|
||
| resp, err = c.ProcessProposal(context.Background(), ppReq) | ||
| require.NoError(t, err) | ||
| require.Equal(t, resp.Status, abciproto.PROCESS_PROPOSAL_STATUS_ACCEPT) | ||
|
|
||
| theWrongHash := sha256.Sum256([]byte("wrong_hash")) | ||
| fbReq.Hash = theWrongHash[:] | ||
|
|
||
| // Initialize FinalizeBlock with wrong hash - should abort optimistic execution | ||
| // Because is aborted, the result comes from the normal execution | ||
| fbResp, err = c.FinalizeBlock(context.Background(), fbReq) | ||
| require.NotNil(t, fbResp) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 2, calledTimes) | ||
|
|
||
| // Verify optimistic execution was reset | ||
| require.False(t, c.optimisticExec.Initialized()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for concurrent optimistic executions
The test suite should include scenarios testing concurrent optimistic executions to ensure thread safety.
Consider adding a test case that simulates multiple goroutines attempting optimistic execution simultaneously:
func TestConcurrentOptimisticExecution(t *testing.T) {
c := setUpConsensus(t, 100_000, mempool.NoOpMempool[mock.Tx]{})
c.processProposalHandler = DefaultServerOptions[mock.Tx]().ProcessProposalHandler
var wg sync.WaitGroup
numGoroutines := 10
for i := 0; i < numGoroutines; i++ {
wg.Add(1)
go func(height int64) {
defer wg.Done()
theHash := sha256.Sum256([]byte(fmt.Sprintf("test-%d", height)))
ppReq := &abciproto.ProcessProposalRequest{
Height: height,
Hash: theHash[:],
Time: time.Now(),
Txs: [][]byte{mockTx.Bytes()},
}
resp, err := c.ProcessProposal(context.Background(), ppReq)
require.NoError(t, err)
require.Equal(t, resp.Status, abciproto.PROCESS_PROPOSAL_STATUS_ACCEPT)
}(int64(i + 2))
}
wg.Wait()
}| // Initialize FinalizeBlock with wrong hash - should abort optimistic execution | ||
| // Because is aborted, the result comes from the normal execution | ||
| fbResp, err = c.FinalizeBlock(context.Background(), fbReq) | ||
| require.NotNil(t, fbResp) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 2, calledTimes) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions for optimistic execution state
The test should verify the optimistic execution state immediately after the wrong hash is processed, before the final assertion.
// Initialize FinalizeBlock with wrong hash - should abort optimistic execution
// Because is aborted, the result comes from the normal execution
fbResp, err = c.FinalizeBlock(context.Background(), fbReq)
require.NotNil(t, fbResp)
require.NoError(t, err)
require.Equal(t, 2, calledTimes)
+require.False(t, c.optimisticExec.Initialized(), "optimistic execution should be reset immediately after wrong hash")📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Initialize FinalizeBlock with wrong hash - should abort optimistic execution | |
| // Because is aborted, the result comes from the normal execution | |
| fbResp, err = c.FinalizeBlock(context.Background(), fbReq) | |
| require.NotNil(t, fbResp) | |
| require.NoError(t, err) | |
| require.Equal(t, 2, calledTimes) | |
| // Initialize FinalizeBlock with wrong hash - should abort optimistic execution | |
| // Because is aborted, the result comes from the normal execution | |
| fbResp, err = c.FinalizeBlock(context.Background(), fbReq) | |
| require.NotNil(t, fbResp) | |
| require.NoError(t, err) | |
| require.Equal(t, 2, calledTimes) | |
| require.False(t, c.optimisticExec.Initialized(), "optimistic execution should be reset immediately after wrong hash") |
| // Abort any running OE so it cannot overlap with `PrepareProposal`. This could happen if optimistic | ||
| // `internalFinalizeBlock` from previous round takes a long time, but consensus has moved on to next round. | ||
| // Overlap is undesirable, since `internalFinalizeBlock` and `PrepareProoposal` could share access to | ||
| // in-memory structs depending on application implementation. | ||
| // No-op if OE is not enabled. | ||
| // Similar call to Abort() is done in `ProcessProposal`. | ||
| c.optimisticExec.Abort() | ||
|
|
There was a problem hiding this comment.
Add error handling for Abort operation
The Abort() call could potentially return an error that should be handled to ensure proper cleanup.
-c.optimisticExec.Abort()
+if err := c.optimisticExec.Abort(); err != nil {
+ c.logger.Error("failed to abort optimistic execution", "err", err)
+ // Continue execution as the abort error shouldn't block proposal preparation
+}Committable suggestion skipped: line range outside the PR's diff.
server/v2/cometbft/abci.go
Outdated
| if c.optimisticExec.Initialized() { | ||
| // check if the hash we got is the same as the one we are executing | ||
| aborted := c.optimisticExec.AbortIfNeeded(req.Hash) | ||
|
|
||
| // Wait for the OE to finish, regardless of whether it was aborted or not | ||
| res, err := c.optimisticExec.WaitResult() | ||
|
|
||
| if aborted { | ||
| resp, newState, decodedTxs, err = c.internalFinalizeBlock(ctx, req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else { | ||
| resp = res.Resp | ||
| newState = res.StateChanges | ||
| decodedTxs = res.DecodedTxs | ||
| } | ||
|
|
||
| resp, newState, err := c.app.DeliverBlock(ciCtx, blockReq) | ||
| if err != nil { | ||
| return nil, err | ||
| // if it was aborted, we need to reset the state | ||
| c.optimisticExec.Reset() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding timeout for WaitResult
The WaitResult() call could potentially block indefinitely. Consider adding a context with timeout.
-res, err := c.optimisticExec.WaitResult()
+ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
+defer cancel()
+res, err := c.optimisticExec.WaitResult(ctx)
+if err == context.DeadlineExceeded {
+ c.logger.Error("optimistic execution timed out")
+ // Fall back to normal execution
+ resp, newState, decodedTxs, err = c.internalFinalizeBlock(ctx, req)
+ if err != nil {
+ return nil, err
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
| go func() { | ||
| start := time.Now() | ||
| resp, stateChanges, decodedTxs, err := oe.finalizeBlockFunc(ctx, oe.request) | ||
|
|
||
| oe.mtx.Lock() | ||
|
|
||
| executionTime := time.Since(start) | ||
| oe.logger.Debug("OE finished", "duration", executionTime.String(), "height", oe.request.Height, "hash", hex.EncodeToString(oe.request.Hash)) | ||
| oe.response, oe.err = &FinalizeBlockResponse[T]{ | ||
| Resp: resp, | ||
| StateChanges: stateChanges, | ||
| DecodedTxs: decodedTxs, | ||
| }, err | ||
|
|
||
| close(oe.stopCh) | ||
| oe.mtx.Unlock() | ||
| }() |
Check notice
Code scanning / CodeQL
Spawning a Go routine Note
| oe.initialized = true | ||
|
|
||
| go func() { | ||
| start := time.Now() |
Check warning
Code scanning / CodeQL
Calling the system time Warning
server/v2/cometbft/abci.go
Outdated
| if req.Height > int64(c.initialHeight) { | ||
| // abort any running OE | ||
| c.optimisticExec.Abort() | ||
| //c.setState(execModeFinalize, header) |
server/v2/cometbft/abci.go
Outdated
| resp, newState, err := c.app.DeliverBlock(ciCtx, blockReq) | ||
| if err != nil { | ||
| return nil, err | ||
| // if it was aborted, we need to reset the state |
There was a problem hiding this comment.
nit: remove comment, as we are always resetting
server/v2/cometbft/abci.go
Outdated
| @@ -2,6 +2,7 @@ package cometbft | |||
|
|
|||
| import ( | |||
| "context" | |||
| "cosmossdk.io/server/v2/cometbft/oe" | |||
There was a problem hiding this comment.
will need to move lower, otherwise linting will complain
| func (oe *OptimisticExecution[T]) Enabled() bool { | ||
| return oe != nil | ||
| } |
There was a problem hiding this comment.
we should remove this and make it default
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
server/v2/cometbft/abci.go (1)
567-569: Address TODO comment about tx decoding behaviorThe TODO raises important questions about tx decoding failure scenarios. This needs to be addressed to ensure proper error handling in production.
Would you like me to help analyze the tx decoding flow and propose a robust error handling strategy?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/cometbft/abci.go(8 hunks)server/v2/cometbft/oe/optimistic_execution.go(1 hunks)server/v2/cometbft/oe/optimistic_execution_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/v2/cometbft/oe/optimistic_execution.go
- server/v2/cometbft/oe/optimistic_execution_test.go
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
server/v2/cometbft/abci.go (2)
81-84:
Consider initializing optimisticExec to prevent nil pointer dereferences
The optimisticExec field is added but not initialized, which could lead to nil pointer dereferences when methods like Abort(), Execute(), etc., are called.
394-401:
Add error handling for Abort operation
The Abort() call should include error handling to ensure proper cleanup and logging of any failures.
| // Only execute optimistic execution if the proposal is accepted, OE is | ||
| // enabled and the block height is greater than the initial height. During | ||
| // the first block we'll be carrying state from InitChain, so it would be | ||
| // impossible for us to easily revert. | ||
| // After the first block has been processed, the next blocks will get executed | ||
| // optimistically, so that when the ABCI client calls `FinalizeBlock` the app | ||
| // can have a response ready. | ||
| if req.Height > int64(c.initialHeight) { | ||
| c.optimisticExec.Execute(req) | ||
| } |
There was a problem hiding this comment.
Add error handling for Execute operation
The Execute() call should include error handling to properly handle and log any execution failures.
Apply this diff:
-c.optimisticExec.Execute(req)
+if err := c.optimisticExec.Execute(req); err != nil {
+ c.logger.Error("failed to execute optimistically", "height", req.Height, "err", err)
+ // Continue execution as the execute error shouldn't block proposal processing
+}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Only execute optimistic execution if the proposal is accepted, OE is | |
| // enabled and the block height is greater than the initial height. During | |
| // the first block we'll be carrying state from InitChain, so it would be | |
| // impossible for us to easily revert. | |
| // After the first block has been processed, the next blocks will get executed | |
| // optimistically, so that when the ABCI client calls `FinalizeBlock` the app | |
| // can have a response ready. | |
| if req.Height > int64(c.initialHeight) { | |
| c.optimisticExec.Execute(req) | |
| } | |
| // Only execute optimistic execution if the proposal is accepted, OE is | |
| // enabled and the block height is greater than the initial height. During | |
| // the first block we'll be carrying state from InitChain, so it would be | |
| // impossible for us to easily revert. | |
| // After the first block has been processed, the next blocks will get executed | |
| // optimistically, so that when the ABCI client calls `FinalizeBlock` the app | |
| // can have a response ready. | |
| if req.Height > int64(c.initialHeight) { | |
| if err := c.optimisticExec.Execute(req); err != nil { | |
| c.logger.Error("failed to execute optimistically", "height", req.Height, "err", err) | |
| // Continue execution as the execute error shouldn't block proposal processing | |
| } | |
| } |
server/v2/cometbft/abci.go
Outdated
| if c.optimisticExec.Initialized() { | ||
| // check if the hash we got is the same as the one we are executing | ||
| aborted := c.optimisticExec.AbortIfNeeded(req.Hash) | ||
|
|
||
| // Wait for the OE to finish, regardless of whether it was aborted or not | ||
| res, err := c.optimisticExec.WaitResult() | ||
|
|
||
| if aborted { | ||
| resp, newState, decodedTxs, err = c.internalFinalizeBlock(ctx, req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else { | ||
| resp = res.Resp | ||
| newState = res.StateChanges | ||
| decodedTxs = res.DecodedTxs | ||
| } | ||
|
|
||
| resp, newState, err := c.app.DeliverBlock(ciCtx, blockReq) | ||
| if err != nil { | ||
| return nil, err | ||
| c.optimisticExec.Reset() |
There was a problem hiding this comment.
Improve error handling in optimistic execution flow
- The error from
WaitResult()at line 497 is ignored but could indicate execution failures. - The
Reset()call at line 510 lacks error handling.
Apply this diff:
-res, err := c.optimisticExec.WaitResult()
+res, err := c.optimisticExec.WaitResult()
+if err != nil {
+ c.logger.Error("optimistic execution failed", "height", req.Height, "err", err)
+ resp, newState, decodedTxs, err = c.internalFinalizeBlock(ctx, req)
+ if err != nil {
+ return nil, err
+ }
+}
-c.optimisticExec.Reset()
+if err := c.optimisticExec.Reset(); err != nil {
+ c.logger.Error("failed to reset optimistic execution", "height", req.Height, "err", err)
+}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if c.optimisticExec.Initialized() { | |
| // check if the hash we got is the same as the one we are executing | |
| aborted := c.optimisticExec.AbortIfNeeded(req.Hash) | |
| // Wait for the OE to finish, regardless of whether it was aborted or not | |
| res, err := c.optimisticExec.WaitResult() | |
| if aborted { | |
| resp, newState, decodedTxs, err = c.internalFinalizeBlock(ctx, req) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } else { | |
| resp = res.Resp | |
| newState = res.StateChanges | |
| decodedTxs = res.DecodedTxs | |
| } | |
| resp, newState, err := c.app.DeliverBlock(ciCtx, blockReq) | |
| if err != nil { | |
| return nil, err | |
| c.optimisticExec.Reset() | |
| if c.optimisticExec.Initialized() { | |
| // check if the hash we got is the same as the one we are executing | |
| aborted := c.optimisticExec.AbortIfNeeded(req.Hash) | |
| // Wait for the OE to finish, regardless of whether it was aborted or not | |
| res, err := c.optimisticExec.WaitResult() | |
| if err != nil { | |
| c.logger.Error("optimistic execution failed", "height", req.Height, "err", err) | |
| resp, newState, decodedTxs, err = c.internalFinalizeBlock(ctx, req) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| if aborted { | |
| resp, newState, decodedTxs, err = c.internalFinalizeBlock(ctx, req) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } else { | |
| resp = res.Resp | |
| newState = res.StateChanges | |
| decodedTxs = res.DecodedTxs | |
| } | |
| if err := c.optimisticExec.Reset(); err != nil { | |
| c.logger.Error("failed to reset optimistic execution", "height", req.Height, "err", err) | |
| } |
julienrbrt
changed the title
|
Looks like there is one last test failure: https://github.com/cosmos/cosmos-sdk/actions/runs/12066521940/job/33647560503?pr=22560 |
…mos-sdk into feat/optimistic-exec-v2
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
server/v2/cometbft/abci_test.go (2)
521-526: Consider adding positive test casesThe current optimistic execution mock only tests the error path. Consider adding test cases for successful execution to ensure both paths are covered.
Example mock function:
optimisticMockFunc := func(context.Context, *abci.FinalizeBlockRequest) (*server.BlockResponse, store.WriterMap, []mock.Tx, error) { - return nil, nil, nil, errors.New("test error") + if shouldError { + return nil, nil, nil, errors.New("test error") + } + return &server.BlockResponse{}, store.WriterMap{}, []mock.Tx{}, nil }
767-773: Consider using test helper for request creationThe test creates similar requests multiple times. Consider extracting request creation into a helper function to improve maintainability.
func createTestRequest(height int64, hash []byte, time time.Time, txs [][]byte) *abciproto.ProcessProposalRequest { return &abciproto.ProcessProposalRequest{ Height: height, Hash: hash, Time: time, Txs: txs, } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
server/v2/cometbft/abci.go(8 hunks)server/v2/cometbft/abci_test.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 golangci-lint (1.62.2)
server/v2/cometbft/abci_test.go
5-5: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
9-9: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
16-16: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
14-14: File is not gofumpt-ed with -extra
(gofumpt)
63-63: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
93-93: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
788-788: ineffectual assignment to fbResp
(ineffassign)
🔇 Additional comments (8)
server/v2/cometbft/abci.go (6)
81-84: LGTM: Field addition is well documented
The new optimisticExec field is properly documented as experimental and includes clear developer guidance.
563-607: LGTM: Well-structured internal finalize block implementation
The internalFinalizeBlock implementation has:
- Good error handling
- Clear separation of concerns
- Proper validation of inputs
492-511:
Improve error handling in optimistic execution flow
The error handling in the optimistic execution flow needs improvement:
- Missing nil check before calling
Initialized() - The error from
WaitResult()is not properly logged - The
Reset()call lacks error handling
Apply this diff:
-if c.optimisticExec.Initialized() {
+if c.optimisticExec != nil && c.optimisticExec.Initialized() {
aborted := c.optimisticExec.AbortIfNeeded(req.Hash)
// Wait for the OE to finish, regardless of whether it was aborted or not
res, optimistErr := c.optimisticExec.WaitResult()
+ if optimistErr != nil {
+ c.logger.Error("optimistic execution failed", "height", req.Height, "err", optimistErr)
+ }
if !aborted {
if res != nil {
resp = res.Resp
newState = res.StateChanges
decodedTxs = res.DecodedTxs
}
if optimistErr != nil {
return nil, optimistErr
}
}
- c.optimisticExec.Reset()
+ if err := c.optimisticExec.Reset(); err != nil {
+ c.logger.Error("failed to reset optimistic execution", "height", req.Height, "err", err)
+ }
}Likely invalid or redundant comment.
438-446:
Add nil check and error handling for Abort
The code needs to handle both nil pointer and abort errors.
Apply this diff:
if req.Height > int64(c.initialHeight) {
- c.optimisticExec.Abort()
+ if c.optimisticExec != nil {
+ if err := c.optimisticExec.Abort(); err != nil {
+ c.logger.Error("failed to abort optimistic execution", "err", err)
+ }
+ }
}Likely invalid or redundant comment.
394-401:
Add error handling for Abort operation
The Abort() call should include error handling to properly handle and log any failures.
Apply this diff:
-c.optimisticExec.Abort()
+if err := c.optimisticExec.Abort(); err != nil {
+ c.logger.Error("failed to abort optimistic execution", "err", err)
+ // Continue execution as the abort error shouldn't block proposal preparation
+}Likely invalid or redundant comment.
463-472:
Add error handling for Execute operation
The Execute() call should include error handling and nil checks.
Apply this diff:
if req.Height > int64(c.initialHeight) {
- c.optimisticExec.Execute(req)
+ if c.optimisticExec != nil {
+ if err := c.optimisticExec.Execute(req); err != nil {
+ c.logger.Error("failed to execute optimistically", "height", req.Height, "err", err)
+ // Continue execution as the execute error shouldn't block proposal processing
+ }
+ }
}Likely invalid or redundant comment.
server/v2/cometbft/abci_test.go (2)
5-16: LGTM: Import changes and interface updates
The new imports and interface parameter updates are appropriate for supporting the optimistic execution functionality.
Also applies to: 63-66, 93-96
🧰 Tools
🪛 golangci-lint (1.62.2)
5-5: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
9-9: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
16-16: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
14-14: File is not gofumpt-ed with -extra
(gofumpt)
738-809: 🛠️ Refactor suggestion
Fix ineffectual assignment and enhance assertions
The test is well-structured but has a few areas for improvement:
- Fix the ineffectual assignment:
- fbResp, err = c.FinalizeBlock(context.Background(), fbReq)
+ _, err = c.FinalizeBlock(context.Background(), fbReq)- Add assertions to verify the state after optimistic execution:
require.Error(t, err)
require.ErrorContains(t, err, "test error") // from optimisticMockFunc
require.Equal(t, 1, calledTimes)
+require.True(t, c.optimisticExec.Initialized(), "optimistic execution should be initialized")Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
788-788: ineffectual assignment to fbResp
(ineffassign)
Co-authored-by: Randy Grok <@faulttolerance.net> (cherry picked from commit 9caec06)
Description
Closes: #XXXX
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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit