feat(abci): unsync local client ported from comet v1 [CP-197] #8
Conversation
WalkthroughThis pull request introduces a new unsynchronized client implementation for ABCI, adding an Changes
Sequence Diagram(s)sequenceDiagram
participant C as UnsyncLocalClient
participant A as Application
participant M as Callback Mutex
C->>M: Lock callback
C->>A: Invoke CheckTxAsync (in goroutine)
A-->>C: Return response
C->>M: Unlock callback
C->>Callback: Invoke callback if set
sequenceDiagram
participant App as Application
participant Creator as UnsyncLocalClientCreator
participant Client as UnsyncLocalClient
App->>Creator: NewUnsyncLocalClientCreator(app)
Creator->>Client: NewABCIClient()
Client-->>Creator: Returns new unsync client instance
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
🪧 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
CodeRabbit Configuration File (
|
kakysha
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
proxy/client.go (1)
86-90: Consider validating non-nil application.
A nilappmay lead to runtime panics. Safeguarding here could be beneficial.func NewUnsyncLocalClientCreator(app types.Application) ClientCreator { + if app == nil { + panic("application cannot be nil") + } return &unsyncLocalClientCreator{ app: app, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
abci/client/unsync_local_client.go(1 hunks)proxy/client.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
proxy/client.go (1)
abci/client/unsync_local_client.go (1)
NewUnsyncLocalClient(32-38)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e-test
🔇 Additional comments (27)
proxy/client.go (4)
72-76: Helpful concurrency model explanation.
These docstrings clearly convey that this client creator assumes concurrency is handled externally.
77-79: Struct definition aligns with "unsync" approach.
This minimal struct without any mutex fields is consistent with deferring concurrency to the application.
81-85: Documentation clarifies advanced usage.
The comment strongly warns about concurrency management. No concerns here.
92-94: Correctly instantiates an unsynchronized client.
This method straightforwardly creates and returns the new unsynced client.abci/client/unsync_local_client.go (23)
1-9: Package and imports look clean.
Imports are minimal and relevant, no immediate issues.
11-19: Struct definition for unsyncLocalClient.
It uses a mutex only for callback synchronization, aligning with the stated unsynced model for the rest of the application.
20-38: Constructor function.
Clear initialization of the base service and the embedded application. No issues noted.
40-44: SetResponseCallback with mutex lock.
This ensures thread safety for setting the callback reference. Good approach.
46-55: CheckTxAsync defers concurrency to the application.
After checking the transaction, it callscallback. Make sure the application is concurrency-safe.Could you confirm the underlying application is thread-safe for concurrent CheckTx calls?
57-64: Callback invocation.
Invokes the callback if set, then returns a request-response wrapper. Implementation seems fine.
66-70: Error method returns nil.
No error tracking is done here. If the client needs to propagate errors, consider a future enhancement.Do we need to track any internal error states that might arise at runtime?
72-74: Flush is a stub.
Since it's a local-only client, returning nil may be sufficient.
76-78: Echo method.
Straightforward echo. No concerns about concurrency here.
80-82: Info method.
Just passes through toapp.Application. Looks good.
84-86: CheckTx method (synchronous).
Also directly delegated to the application. Fine for a local approach.
88-90: Query method.
Again, a simple pass-through. No issues.
92-94: Commit method.
Delegates to app’s commit logic. No concurrency guard needed here, by design.
96-98: InitChain method.
No local concurrency management, consistent with unsync design.
100-102: ListSnapshots method.
Pass-through to existing application logic. No further comments.
104-106: OfferSnapshot method.
Delegated snapshot offering. Implementation is straightforward.
108-111: LoadSnapshotChunk method.
Directly callsLoadSnapshotChunkon the embedded app. No concerns.
113-116: ApplySnapshotChunk method.
Another pass-through call. Implementation looks clean.
118-120: PrepareProposal method.
Again delegates concurrency to the application. Matches the unsync design.
122-124: ProcessProposal method.
Consistently delegates proposal handling to the application.
126-128: ExtendVote method.
Continues the unsynchronized pattern with simple pass-through.
130-132: VerifyVoteExtension method.
Delegates vote extension checks. No concurrency or logic issues.
134-136: FinalizeBlock method.
Ends the block by forwarding to the embedded app. Implementation remains consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
abci/client/unsync_local_client.go (1)
11-18: Mutex initialization needs to be clear for maintainersThe
mtxmutex field is declared but not explicitly initialized in the struct or constructor. While Go's zero value for sync.Mutex is valid and usable, it would be helpful to add a comment explaining this for future maintainers.I see that another reviewer previously asked about mutex initialization. Although Go automatically initializes the mutex to its zero value (which is usable), consider adding a clarifying comment in the constructor to address this explicitly.
🧹 Nitpick comments (2)
abci/client/unsync_local_client.go (2)
53-53: Remove or clarify commented codeThe commented line with "don't think this is needed" creates ambiguity about proper error handling. Either remove this comment entirely or add a clear explanation of why error responses aren't needed in this context.
46-62: Document the CheckTxAsync goroutine managementWhile the constructor docs explain the high-level concurrency model, there's no comment explaining the goroutine management in
CheckTxAsync. Consider adding documentation that explains how the goroutine is managed and what happens to it if the client is stopped or closed.Add something like:
// CheckTxAsync asynchronously runs the application's CheckTx in a goroutine. // Note: If the client is stopped, ongoing goroutines will not be explicitly terminated. func (app *unsyncLocalClient) CheckTxAsync(ctx context.Context, req *types.RequestCheckTx) (*ReqRes, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
abci/client/unsync_local_client.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e-test
🔇 Additional comments (4)
abci/client/unsync_local_client.go (4)
57-58: Potential race condition with callback invocationWhile
SetResponseCallback()properly protects the callback with a mutex,reqres.InvokeCallback()is called without mutex protection, which could lead to a race condition if the callback is being modified concurrently.Please verify how
InvokeCallback()is implemented. If it directly accesses the callback field without proper synchronization, you should add mutex protection here:reqres.Response = types.ToResponseCheckTx(res) + app.mtx.Lock() reqres.InvokeCallback() + app.mtx.Unlock()Alternatively, if
reqres.InvokeCallback()uses the callback from theunsyncLocalClient, modify its implementation to acquire the mutex before invoking the callback.
32-38: Robust constructor implementation with proper documentationThe constructor clearly explains the difference between this implementation and
NewLocalClient, which is crucial for users to understand the concurrency implications.
78-134: Clean pass-through implementationsThe implementation correctly forwards all application methods directly to the embedded Application, maintaining the non-synchronized nature of this client as described in the constructor comments.
46-62:Details
❓ Verification inconclusive
Verify if context cancellation is handled correctly
The provided context is passed to the Application's CheckTx call, but it's not clear if the goroutine properly respects context cancellation. If the parent context is canceled, the goroutine should terminate gracefully.
Please verify if the embedded Application's CheckTx implementation properly respects context cancellation. If not, consider adding explicit handling:
🏁 Script executed:
#!/bin/bash # Find out how Application.CheckTx handles context cancellation grep -r "func.*CheckTx.*context\.Context" --include="*.go" .Length of output: 4092
Subject: Verify Context Cancellation Handling in unsyncLocalClient.checkTxAsync
The grep search confirms that all CheckTx implementations (including in the Application used by unsyncLocalClient) accept a context. However, the implementation in unsyncLocalClient simply passes the provided context into Application.CheckTx within a goroutine without additional cancellation checks. This design relies on the underlying Application.CheckTx to honor context cancellation. Please review the exact Application.CheckTx implementation (for example, in
abci/example/kvstore/kvstore.go) to ensure it listens toctx.Done()and terminates work when canceled. If it doesn’t, consider adding explicit cancellation handling in the goroutine to avoid potential goroutine leaks.
abci/client/unsync_local_client.go
Outdated
| if err != nil { | ||
| // reqres.Response = types.ToResponseException(err.Error()) // don't think this is needed | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in CheckTxAsync
When an error occurs from app.Application.CheckTx(), it's currently ignored and the goroutine simply returns without setting any response or notifying the caller. This could lead to requests hanging indefinitely if they're waiting for a response.
Consider setting an error response and still invoking the callback:
if err != nil {
- // reqres.Response = types.ToResponseException(err.Error()) // don't think this is needed
- return
+ reqres.Response = types.ToResponseException(err.Error())
+ reqres.InvokeCallback()
+ return
}📝 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 err != nil { | |
| // reqres.Response = types.ToResponseException(err.Error()) // don't think this is needed | |
| return | |
| } | |
| if err != nil { | |
| reqres.Response = types.ToResponseException(err.Error()) | |
| reqres.InvokeCallback() | |
| return | |
| } |
kakysha
force-pushed
the
CP-197/parallel_checktx
branch
from
13ea52b to
983e4bc
Compare
…tbft#4890) due to sec vuln Vulnerability InjectiveLabs#1: GO-2025-3420 Sensitive headers incorrectly sent after cross-domain redirect in net/http More info: https://pkg.go.dev/vuln/GO-2025-3420 Standard library Found in: net/[email protected] Fixed in: net/[email protected] Example traces found: Error: InjectiveLabs#1: rpc/jsonrpc/client/http_json_client.go:231:34: client.Client.Call calls http.Client.Do Error: InjectiveLabs#2: libs/cli/setup.go:89:26: cli.Executor.Execute calls cobra.Command.Execute, which eventually calls http.Client.Get Error: InjectiveLabs#3: cmd/cometbft/commands/debug/util.go:70:23: debug.dumpProfile calls http.Get Vulnerability InjectiveLabs#2: GO-2025-3373 Usage of IPv6 zone IDs can bypass URI name constraints in crypto/x509 More info: https://pkg.go.dev/vuln/GO-2025-3373 Standard library Found in: crypto/[email protected] Fixed in: crypto/[email protected] Example traces found: Error: InjectiveLabs#1: abci/tutorials/abci-v2-forum-app/model/db.go:143:20: model.DB.Close calls badger.DB.Close, which eventually calls x509.CertPool.AppendCertsFromPEM Error: InjectiveLabs#2: internal/autofile/group.go:468:30: autofile.GroupReader.Read calls bufio.Reader.Read, which eventually calls x509.Certificate.Verify Error: InjectiveLabs#3: rpc/jsonrpc/client/ws_client.go:290:29: client.WSClient.dial calls websocket.Dialer.Dial, which eventually calls x509.Certificate.VerifyHostname Error: InjectiveLabs#4: light/errors.go:483:84: light.errBadWitness.Error calls x509.HostnameError.Error Error: InjectiveLabs#5: rpc/jsonrpc/server/http_server.go:166:19: server.ServeTLSWithShutdown calls http.Server.ServeTLS, which eventually calls x509.ParseCertificate Error: InjectiveLabs#6: rpc/jsonrpc/server/http_server.go:166:19: server.ServeTLSWithShutdown calls http.Server.ServeTLS, which eventually calls x509.ParseECPrivateKey Error: InjectiveLabs#7: rpc/jsonrpc/server/http_server.go:166:19: server.ServeTLSWithShutdown calls http.Server.ServeTLS, which eventually calls x509.ParsePKCS1PrivateKey Error: InjectiveLabs#8: rpc/jsonrpc/server/http_server.go:166:19: server.ServeTLSWithShutdown calls http.Server.ServeTLS, which eventually calls x509.ParsePKCS8PrivateKey <hr>This is an automatic backport of pull request cometbft#4888 done by [Mergify](https://mergify.com). --------- Co-authored-by: Anton Kaliaev <[email protected]>
Summary by CodeRabbit