fix(warden): (re)introduce prophet #1163
Conversation
This reverts commit 7a1f7bd.
The vemanager already takes care of unwrapping the vote extension before calling the verify vote extension handler.
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive implementation of the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Prophet
participant Registry
participant Handler
participant Executor
Client->>Prophet: AddFuture
Prophet->>Registry: GetHandler
Registry-->>Prophet: Return Handler
Prophet->>Executor: Execute Future
Executor->>Handler: Execute
Handler-->>Executor: Return Result
Executor-->>Prophet: Completed Future
Prophet->>Client: Future Result Available
This sequence diagram illustrates the high-level flow of future execution within the prophet package, showing how futures are added, processed, and completed through the system's components. Finishing Touches
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (27)
warden/app/vemanager/vemanager.go (5)
30-31: Correct grammatical error in commentThe comment contains a grammatical error. Replace "it's" with "is" for clarity.
Apply this diff to correct the comment:
-// CONTRACT: changing the order of registration it's a consensus breaking +// CONTRACT: changing the order of registration is a consensus breaking
58-61: Add context to marshaling errorWhen
w.Marshal()fails, returning the error directly might make debugging difficult. Consider adding context to the error message.Apply this diff to enhance the error handling:
- return nil, err + return nil, fmt.Errorf("failed to marshal VoteExtensions: %w", err)
17-21: Use plural naming for handler slicesFor clarity, consider renaming the handler slice fields to their plural forms to indicate that they contain multiple handlers.
Apply this diff to rename the fields:
type VoteExtensionManager struct { - extendVoteHandler []sdk.ExtendVoteHandler - verifyVoteExtensionHandler []sdk.VerifyVoteExtensionHandler - prepareProposalHandler []sdk.PrepareProposalHandler + extendVoteHandlers []sdk.ExtendVoteHandler + verifyVoteExtensionHandlers []sdk.VerifyVoteExtensionHandler + prepareProposalHandlers []sdk.PrepareProposalHandler }Ensure that all references to these fields are updated accordingly throughout the code.
14-16: Enhance the documentation forVoteExtensionManagerConsider providing more detailed documentation for the
VoteExtensionManagerstruct to explain its role and how it affects consensus.Expanding the comment will improve maintainability and help other developers understand the importance of this component.
7-12: Group imports according to standard conventionsTo improve readability, group the import statements into standard library packages, third-party packages, and project-specific packages.
Apply this diff to reorganize the imports:
import ( - fmt "fmt" - - cometabci "github.com/cometbft/cometbft/abci/types" - sdk "github.com/cosmos/cosmos-sdk/types" + "fmt" + + sdk "github.com/cosmos/cosmos-sdk/types" + cometabci "github.com/cometbft/cometbft/abci/types" )prophet/handlers.go (4)
23-26: Use descriptive variable names for clarityConsider renaming the variable
stohandlerto improve code readability and adhere to the naming conventions recommended in the Uber Go Style Guide.Apply this diff to rename the variable:
func Execute(ctx context.Context, f Future) (FutureResult, error) { - s := getHandler(f.Handler) + handler := getHandler(f.Handler) if handler == nil { - return FutureResult{}, fmt.Errorf("no future handler registered for %s", f.Handler) + return FutureResult{}, fmt.Errorf("no future handler registered for %s", f.Handler) }
28-28: Avoid shadowing thelogvariableThe declaration
log := slog.With(...)shadows the package-levellogimport. To prevent confusion and enhance clarity, consider renaming the local variable to something likelogger.Apply this diff to rename the variable:
- log := slog.With("task", "Execute", "future", f.ID, "handler", f.Handler) + logger := slog.With("task", "Execute", "future", f.ID, "handler", f.Handler)
47-50: Use descriptive variable names for claritySimilar to the previous suggestion, rename the variable
stohandlerin theVerifyfunction for better readability.Apply this diff to rename the variable:
func Verify(ctx context.Context, f FutureResult) error { - s := getHandler(f.Handler) + handler := getHandler(f.Handler) if handler == nil { - return fmt.Errorf("no future handler registered for %s", f.Handler) + return fmt.Errorf("no future handler registered for %s", f.Handler) }
52-52: Avoid shadowing thelogvariableThe declaration
log := slog.With(...)shadows the package-levellogimport. Renaming the local variable will prevent confusion.Apply this diff:
- log := slog.With("task", "Verify", "future", f.ID, "handler", f.Handler) + logger := slog.With("task", "Verify", "future", f.ID, "handler", f.Handler)prophet/exec.go (4)
17-38: Pass context toExecFuturesfor proper cancellation and timeout handlingCurrently,
context.TODO()is used inside the goroutine, which is intended for code that needs to be updated. It's recommended to pass acontext.ContexttoExecFuturesand use it within the goroutine to handle cancellations and timeouts appropriately.Apply this diff to modify the function signature and usage:
-func ExecFutures(r FutureReader, w FutureResultWriter) error { +func ExecFutures(ctx context.Context, r FutureReader, w FutureResultWriter) error { log := slog.With("process", "exec_futures") go func() { for future := range r.Read() { - log := log.With("future", future.ID) + logger := log.With("future", future.ID) - log.Debug("running future") - output, err := Execute(context.TODO(), future) + logger.Debug("running future") + output, err := Execute(ctx, future) if err != nil { - log.Error("failed to run future", "err", err) + logger.Error("failed to run future", "err", err) continue } err = w.Write(output) if err != nil { - log.Error("failed to write future result", "err", err) + logger.Error("failed to write future result", "err", err) continue } } }()Ensure to update any calls to
ExecFuturesto pass the appropriate context.
50-69: Pass context toExecVotesfor proper cancellation and timeout handlingSimilar to the previous suggestion, modify
ExecVotesto accept acontext.Contextand use it within the goroutine instead ofcontext.TODO().Apply this diff:
-func ExecVotes(r FutureResultReader, w VoteWriter) error { +func ExecVotes(ctx context.Context, r FutureResultReader, w VoteWriter) error { log := slog.With("process", "exec_votes") go func() { for proposal := range r.Read() { - plog := log.With("future", proposal.ID) + logger := log.With("future", proposal.ID) - plog.Debug("verifying future proposal") - err := Verify(context.TODO(), proposal) + logger.Debug("verifying future proposal") + err := Verify(ctx, proposal) if err := w.Write(Vote{ ID: proposal.ID, Err: err, }); err != nil { - plog.Error("failed to write vote", "err", err) + logger.Error("failed to write vote", "err", err) continue } } }()Update any calls to
ExecVotesto pass the appropriate context.
25-26: Rename variablelogto avoid shadowing and improve clarityThe local variable
logshadows the package-levellogimport. Consider renaming it tologgerto enhance clarity.Apply this diff:
log := log.With("future", future.ID) - log.Debug("running future") + logger := log.With("future", future.ID) + logger.Debug("running future")
55-57: Rename variableplogtologgerfor consistencyUsing consistent variable names improves readability. Rename
plogtologger.Apply this diff:
- plog := log.With("future", proposal.ID) - plog.Debug("verifying future proposal") + logger := log.With("future", proposal.ID) + logger.Debug("verifying future proposal")prophet/prophet.go (3)
105-111: Avoid silent dropping of items in queuesDropping items when the queue is full can lead to data loss or inconsistent states. Evaluate if a bounded buffer is appropriate, or if backpressure mechanisms or blocking calls are better suited for this use case.
Consider using an unbuffered channel or implementing a proper backpressure strategy to ensure that no items are lost.
28-30: Check for potential errors fromnewSfunctionWhen initializing
resultsWriterandvotesWriter, errors fromnewSare checked, but sincenewSonly returnsnilor an error fromlru.New, which generally doesn't fail with the provided parameters, consider handling the error differently or adding context to potential failures.Ensure that any error from
newSis meaningful and, if unlikely to fail, consider simplifying error handling.
133-137: Avoid empty error returns inWritemethodThe
Writemethod for the setscurrently always returnsnil. If there's no possibility of an error, consider removing the error return to simplify the interface.Modify the method signature:
-func (s *s[T]) Write(value T) error { +func (s *s[T]) Write(value T) { id := value.getID() s.l.Add(id, value) - return nil }Update any calls to
Writeaccordingly.warden/x/async/keeper/abci.go (2)
25-25: Consider making the pending futures limit configurableIn the
EndBlockermethod, the limit10ink.futures.PendingFutures(ctx, 10)is hardcoded. For better flexibility and maintainability, consider defining this limit as a constant or making it a configurable parameter.Apply this diff to define a constant for the limit:
+ const pendingFuturesLimit = 10 - futures, err := k.futures.PendingFutures(ctx, 10) + futures, err := k.futures.PendingFutures(ctx, pendingFuturesLimit)
191-205: Ensure proper error logging inprocessVEmethodIn the
processVEmethod, errors are returned immediately upon failure in adding future results or setting future votes. Consider logging these errors before returning to improve observability and ease of debugging.Apply this diff to log errors before returning:
if err := k.AddFutureResult(ctx, r.FutureId, fromAddr, r.Output); err != nil { + ctx.Logger().Error("failed to add future result", "error", err) return fmt.Errorf("failed to add future result: %w", err) }if err := k.SetFutureVote(ctx, vote.FutureId, fromAddr, vote.Vote); err != nil { + ctx.Logger().Error("failed to set task vote", "error", err) return fmt.Errorf("failed to set task vote: %w", err) }prophet/handlers/echo/echo.go (1)
21-26: Consider adding error context.While the error message is clear, consider using
errors.Neworfmt.Errorfwith more context about the mismatch.- return fmt.Errorf("input and output do not match") + return fmt.Errorf("echo verification failed: input %x does not match output %x", input, output)prophet/registry.go (1)
5-8: Consider dependency injection over global state.Global state can make testing difficult and create tight coupling. Consider making the registry an injectable dependency.
prophet/future.go (1)
4-11: Consider adding input validation.The
Futurestruct should validate its fields, especially ensuring non-empty Handler and valid Input.+// NewFuture creates a new Future with validation +func NewFuture(id uint64, handler string, input []byte) (*Future, error) { + if handler == "" { + return nil, errors.New("handler cannot be empty") + } + if input == nil { + return nil, errors.New("input cannot be nil") + } + return &Future{ + ID: id, + Handler: handler, + Input: input, + }, nil +}prophet/dedup.go (2)
7-7: Make LRU cache size configurable.Consider making
dedupLRUSizeconfigurable through environment variables or configuration file.
57-63: Add error documentation.Document the possible error cases in the function comment.
+// newDedupFutureReader creates a new deduplicated future reader. +// It returns an error if the LRU cache initialization fails. func newDedupFutureReader(r FutureReader) (*dedupFutureReader, error) {warden/testutil/keeper/async.go (1)
41-41: Consider using a mock prophet.P instead of nilPassing
nilfor the newprophet.Pparameter could mask potential issues in tests. Consider creating a mock implementation or using a test instance ofprophet.Pto ensure proper testing of the interaction between the keeper and prophet package.warden/x/async/keeper/futures.go (1)
90-116: Optimize PendingFutures implementationConsider these optimizations:
- Early return when limit is 0
- Pre-check if there are any pending futures before allocation
func (k *FutureKeeper) PendingFutures(ctx context.Context, limit int) ([]types.Future, error) { + if limit <= 0 { + return nil, nil + } + + hasAny, err := k.pendingFutures.Has(ctx, 0) + if err != nil { + return nil, err + } + if !hasAny { + return nil, nil + } + it, err := k.pendingFutures.IterateRaw(ctx, nil, nil, collections.OrderAscending)warden/app/app.go (2)
248-252: Consider moving handler registration after prophet initialization.The echo handler is registered before the prophet instance is created. While this works due to the static nature of registration, it would be clearer to move the registration after prophet initialization for better code organization and to prevent potential issues if registration becomes dynamic in the future.
-prophet.Register("echo", echo.Handler{}) -prophetP, err := prophet.New() -if err != nil { - panic(fmt.Errorf("failed to create prophet: %w", err)) -} +prophetP, err := prophet.New() +if err != nil { + panic(fmt.Errorf("failed to create prophet: %w", err)) +} +prophet.Register("echo", echo.Handler{})
407-409: Consider graceful shutdown for prophet.The prophet is started but there's no visible shutdown mechanism. Consider implementing a cleanup in the app's shutdown path.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
api/warden/async/v1beta1/ve.pulsar.gois excluded by!api/**api/warden/vemanager/vemanager.pulsar.gois excluded by!api/**warden/app/vemanager/vemanager.pb.gois excluded by!**/*.pb.gowarden/x/async/types/v1beta1/ve.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
CHANGELOG.md(1 hunks)go.mod(1 hunks)prophet/dedup.go(1 hunks)prophet/doc.go(1 hunks)prophet/exec.go(1 hunks)prophet/future.go(1 hunks)prophet/handlers.go(1 hunks)prophet/handlers/echo/echo.go(1 hunks)prophet/prophet.go(1 hunks)prophet/registry.go(1 hunks)proto/warden/async/v1beta1/ve.proto(1 hunks)proto/warden/vemanager/vemanager.proto(1 hunks)warden/app/app.go(6 hunks)warden/app/oracle.go(7 hunks)warden/app/vemanager/vemanager.go(1 hunks)warden/testutil/keeper/async.go(1 hunks)warden/x/async/keeper/abci.go(1 hunks)warden/x/async/keeper/abci_test.go(1 hunks)warden/x/async/keeper/futures.go(5 hunks)warden/x/async/keeper/keeper.go(5 hunks)warden/x/async/module/module.go(3 hunks)warden/x/async/types/v1beta1/future.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- prophet/doc.go
🧰 Additional context used
📓 Path-based instructions (18)
warden/x/async/types/v1beta1/future.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/module/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/registry.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/testutil/keeper/async.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/handlers/echo/echo.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
prophet/handlers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/app/app.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/keeper/futures.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/keeper/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"
prophet/future.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/exec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/prophet.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/dedup.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/app/vemanager/vemanager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/keeper/keeper.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/keeper/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/app/oracle.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 buf (1.47.2)
proto/warden/vemanager/vemanager.proto
3-3: Files with package "warden.vemanager" must be within a directory "warden/vemanager" relative to root but were in directory "proto/warden/vemanager".
(PACKAGE_DIRECTORY_MATCH)
proto/warden/async/v1beta1/ve.proto
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🔇 Additional comments (18)
warden/app/vemanager/vemanager.go (1)
46-61: Verify the definition and implementation ofVoteExtensionsThe
VoteExtensionsstruct is used but not defined in this file. Ensure that it is properly defined elsewhere in the codebase withMarshalandUnmarshalmethods implemented.Please confirm that
VoteExtensionsis correctly defined and that marshaling and unmarshaling operations function as expected.warden/x/async/keeper/abci.go (2)
31-36: Handle potential errors fromk.p.AddFutureIf
k.p.AddFuturecan return an error, consider adding error handling within the loop to catch and manage any potential issues when adding futures.Does
k.p.AddFuturereturn an error? If so, update the code to handle it appropriately.
236-249: Efficient trimming of excess bytes intrimExcessBytesfunctionThe
trimExcessBytesfunction effectively trims transactions to respect the maximum size limit, ensuring block size constraints are maintained without including oversized transactions.warden/app/oracle.go (2)
219-228: Effective combination of proposal handlersThe
combineProcessProposalandcombinePreBlockerfunctions correctly combine the Slinky and Async handlers, ensuring that both handlers are executed in sequence and their responses are appropriately managed.
279-299: Proper implementation ofWardenSlinkyCodecThe
WardenSlinkyCodeccorrectly wraps the existingslinkyCodecto support the custom vote extension format (vemanager.VoteExtensions). TheDecodemethod properly handles unmarshalling and error checking before delegating to the wrapped codec.warden/x/async/types/v1beta1/future.go (1)
3-6: Correct implementation ofIsValidmethod forFutureVoteTypeThe
IsValidmethod accurately determines the validity of aFutureVoteTypeby checking if the value exists in theFutureVoteType_namemap. This ensures only defined enum values are considered valid.prophet/handlers/echo/echo.go (1)
13-15: LGTM! Good use of interface compliance check.The implementation follows Go best practices with explicit interface compliance verification.
warden/x/async/keeper/keeper.go (2)
29-30: LGTM! Clean integration of prophet.PThe prophet.P field is properly added to the Keeper struct.
110-115: LGTM! Proper validation in SetFutureVoteThe method properly validates the vote type before proceeding with the operation.
warden/x/async/keeper/futures.go (1)
19-19: LGTM! Clean integration of pendingFutures collectionThe pendingFutures KeySet is properly integrated following the existing collections pattern.
Also applies to: 31-31, 37-37
warden/x/async/keeper/abci_test.go (2)
9-73: LGTM! Comprehensive test coverage for trimExcessBytesExcellent test coverage with well-structured test cases covering edge cases and boundary conditions.
75-137: LGTM! Comprehensive test coverage for injectTxWell-structured test cases with good coverage of edge cases and different insertion scenarios.
warden/x/async/module/module.go (1)
24-24: LGTM! Clean integration of the prophet package.The changes properly integrate the prophet package by adding it as an optional dependency in
ModuleInputsand passing it to the keeper.Also applies to: 194-195, 217-217
proto/warden/vemanager/vemanager.proto (1)
7-12: LGTM! Well-documented message type.The
VoteExtensionsmessage type is well-documented and correctly implements the requirement to distinguish between multiple extensions from different sources.proto/warden/async/v1beta1/ve.proto (2)
19-34: LGTM! Well-structured vote extension messages.The message types are well-organized and provide a clear structure for handling async vote extensions:
AsyncVoteExtensionfor containing results and votesVEResultItemfor future resultsVEVoteItemfor future votes
14-16: Address FIXME comment about data duplication.The comment indicates potential space wastage due to duplication with slinky. This should be investigated and resolved.
Let's verify the duplication:
Would you like me to help analyze the duplication and propose a solution to optimize the block space usage?
go.mod (1)
62-62: LGTM: Dependency upgrade for LRU cache library.The upgrade from
github.com/hashicorp/golang-lru v1.0.2togithub.com/hashicorp/golang-lru/v2 v2.0.7is appropriate for implementing the deduplication feature in the newprophetpackage.CHANGELOG.md (1)
57-57: LGTM: Well-formatted changelog entry.The entry follows the changelog guidelines and clearly documents the introduction of the "prophet" module with its key features.
|
|
||
| var resps []*cometabci.ResponseVerifyVoteExtension | ||
| for i, ext := range w.Extensions { | ||
| handler := m.verifyVoteExtensionHandler[i] |
There was a problem hiding this comment.
Prevent potential index out-of-range panic
Accessing m.verifyVoteExtensionHandler[i] without ensuring the index i is within bounds may cause a panic if the lengths are mismatched.
This issue will be resolved by checking for equality of lengths as suggested earlier.
| // future will be executed in the background, the results can be retrieved by | ||
| // calling [P.Results]. | ||
| func (p *P) AddFuture(future Future) { | ||
| p.futures.Add(future) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential error when adding futures to the queue
The Add method does not return an error, but it logs a warning if the queue is full and drops the item. This can lead to silent failures. Consider modifying Add to return an error, allowing the caller to handle it appropriately.
Modify the Add method to return an error:
-func (q *q[T]) Add(item T) {
+func (q *q[T]) Add(item T) error {
select {
case q.ch <- item:
+ return nil
default:
slog.Warn("q.Add: queue is full, dropped item", "item", item)
+ return fmt.Errorf("queue is full, item dropped")
}
}Update the AddFuture method to handle the error:
-func (p *P) AddFuture(future Future) {
- p.futures.Add(future)
+func (p *P) AddFuture(future Future) error {
+ if err := p.futures.Add(future); err != nil {
+ return err
+ }
+ return nil
}Similarly, update AddFutureResult:
-func (p *P) AddFutureResult(proposal FutureResult) {
- p.proposals.Add(proposal)
+func (p *P) AddFutureResult(proposal FutureResult) error {
+ if err := p.proposals.Add(proposal); err != nil {
+ return err
+ }
+ return nil
}Committable suggestion skipped: line range outside the PR's diff.
| log.Error("failed to unmarshal async tx", "err", err) | ||
| // probably not an async tx? | ||
| // but slinky in this case rejects their proposal so maybe we | ||
| // should do the same? | ||
| return &cometabci.ResponseProcessProposal{ | ||
| Status: cometabci.ResponseProcessProposal_ACCEPT, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Re-evaluate proposal acceptance on unmarshal failure
In the ProcessProposalHandler, when tx.Unmarshal(asyncTx) fails, the proposal is still accepted. Given the comment mentioning that Slinky rejects the proposal in such cases, consider rejecting the proposal to prevent processing invalid transactions.
Apply this diff to reject the proposal on unmarshal failure:
- return &cometabci.ResponseProcessProposal{
- Status: cometabci.ResponseProcessProposal_ACCEPT,
- }, nil
+ return &cometabci.ResponseProcessProposal{
+ Status: cometabci.ResponseProcessProposal_REJECT,
+ }, nil📝 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.
| log.Error("failed to unmarshal async tx", "err", err) | |
| // probably not an async tx? | |
| // but slinky in this case rejects their proposal so maybe we | |
| // should do the same? | |
| return &cometabci.ResponseProcessProposal{ | |
| Status: cometabci.ResponseProcessProposal_ACCEPT, | |
| }, nil | |
| } | |
| log.Error("failed to unmarshal async tx", "err", err) | |
| // probably not an async tx? | |
| // but slinky in this case rejects their proposal so maybe we | |
| // should do the same? | |
| return &cometabci.ResponseProcessProposal{ | |
| Status: cometabci.ResponseProcessProposal_REJECT, | |
| }, nil | |
| } |
| log.Error("failed to unmarshal async tx", "err", err, "tx", asyncTx) | ||
| // probably not an async tx? | ||
| // but slinky in this case rejects their proposal so maybe we | ||
| // should do the same? | ||
| return resp, nil | ||
| } |
There was a problem hiding this comment.
Consistent error handling in PreBlocker
In the PreBlocker, when tx.Unmarshal(asyncTx) fails, the error is logged, but the function continues without rejecting the block or handling the invalid transaction. To maintain consistency and ensure invalid data does not affect the system, consider implementing proper error handling, such as rejecting the pre-block or skipping the invalid transaction.
Apply this diff to handle the error appropriately:
return resp, nil
+ }
+
+ // Consider rejecting or handling invalid transactions here
+ // Example:
+ // return resp, fmt.Errorf("invalid async transaction")
}Committable suggestion skipped: line range outside the PR's diff.
| func getHandler(name string) FutureHandler { | ||
| registry.rw.RLock() | ||
| defer registry.rw.RUnlock() | ||
| return registry.futures[name] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in getHandler.
The function should return an error when the handler is not found, rather than potentially returning nil.
-func getHandler(name string) FutureHandler {
+func getHandler(name string) (FutureHandler, error) {
registry.rw.RLock()
defer registry.rw.RUnlock()
- return registry.futures[name]
+ handler, exists := registry.futures[name]
+ if !exists {
+ return nil, fmt.Errorf("handler %q not found", name)
+ }
+ return handler, nil
}📝 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.
| func getHandler(name string) FutureHandler { | |
| registry.rw.RLock() | |
| defer registry.rw.RUnlock() | |
| return registry.futures[name] | |
| } | |
| func getHandler(name string) (FutureHandler, error) { | |
| registry.rw.RLock() | |
| defer registry.rw.RUnlock() | |
| handler, exists := registry.futures[name] | |
| if !exists { | |
| return nil, fmt.Errorf("handler %q not found", name) | |
| } | |
| return handler, nil | |
| } |
| go func() { | ||
| defer close(out) | ||
| for req := range ch { | ||
| if c.Contains(req.getID()) { | ||
| continue | ||
| } | ||
| c.Add(req.getID(), struct{}{}) | ||
| out <- req | ||
| } | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add context for graceful shutdown.
The goroutine should respect context cancellation for graceful shutdown.
- go func() {
+ go func(ctx context.Context) {
defer close(out)
+
+ select {
+ case <-ctx.Done():
+ return
+ default:
for req := range ch {
+ select {
+ case <-ctx.Done():
+ return
+ default:
if c.Contains(req.getID()) {
continue
}
c.Add(req.getID(), struct{}{})
out <- req
+ }
}
- }()
+ }(ctx)Committable suggestion skipped: line range outside the PR's diff.
| func (k Keeper) AddFutureResult(ctx context.Context, id uint64, submitter, output []byte) error { | ||
| if err := k.futures.SetResult(ctx, types.FutureResult{ | ||
| Id: id, | ||
| Output: output, | ||
| Submitter: submitter, | ||
| }); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := k.SetFutureVote(ctx, id, submitter, types.FutureVoteType_VOTE_TYPE_VERIFIED); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Consider implementing store-level transaction handling in AddFutureResult
The review comment is valid. The method performs two separate atomic operations on different collections (results and votes), but there's no mechanism to ensure both operations succeed or fail together. If SetFutureVote fails after SetResult succeeds, it would leave the system in an inconsistent state where a result exists without a corresponding vote.
Consider:
- Using store-level transaction handling to wrap both operations
- Implementing a rollback mechanism for the first operation if the second fails
- Or combining both pieces of data into a single atomic operation if possible
🔗 Analysis chain
Consider transaction handling in AddFutureResult
The method performs two operations (setting result and vote) that should be atomic. Consider handling the case where setting the vote fails after the result has been set.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing error handling patterns in the codebase
rg -A 5 "if err := .*; err != nil \{.*rollback|transaction"
Length of output: 71433
Script:
#!/bin/bash
# Check the implementation of SetResult and SetFutureVote
ast-grep --pattern 'func $_ SetResult($_, $_, $_) {
$$$
}'
ast-grep --pattern 'func $_ SetFutureVote($_, $_, $_, $_) {
$$$
}'
# Look for store patterns in the async module
rg -A 5 "store|Store" warden/x/async/
Length of output: 5345
Script:
#!/bin/bash
# Check futures implementation
rg -A 10 "type FutureKeeper" warden/x/async/
# Look for collections usage patterns
rg -A 5 "collections\." warden/x/async/
# Check if there's any transaction support
rg -A 5 "WithTransaction|BeginTransaction|Transaction" warden/x/async/
Length of output: 6407
| @@ -0,0 +1,12 @@ | |||
| syntax = "proto3"; | |||
|
|
|||
| package warden.vemanager; | |||
There was a problem hiding this comment.
Fix package directory structure.
The package warden.vemanager should be within a directory warden/vemanager relative to the proto root directory.
Move the proto file to the correct directory:
mkdir -p warden/vemanager
mv proto/warden/vemanager/vemanager.proto proto/warden/vemanager/🧰 Tools
🪛 buf (1.47.2)
3-3: Files with package "warden.vemanager" must be within a directory "warden/vemanager" relative to root but were in directory "proto/warden/vemanager".
(PACKAGE_DIRECTORY_MATCH)
Reintroduces #1141 that was bugged in scenarios with 2+ validators.
The fix is at 02a2392. The vemanager already unwraps the vemanager.VoteExtensions before invoking Slinky's VerifyVoteExtension, but Slinky's VerifyVoteExtension was using the WardenSlinkyCodec that's trying to unwrap vemanager.VoteExtensions again.
Summary by CodeRabbit
Release Notes
New Features
Dependency Updates
golang-lrulibrary to version 2.0.7Improvements
Testing