[FIX] Checking assigned task before task type is decided #1748
Conversation
WalkthroughIntroduces ProverTaskManager to centralize prover parameter validation and context creation; GetTaskController now uses it to determine task type. BaseProverTask.checkParameter is simplified to retrieve stored context. Task implementations adjust Assign logic and constructors to rely on the new manager-driven flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as GetTaskController
participant Manager as ProverTaskManager
participant Context as gin.Context
participant BaseTask as BaseProverTask
Controller->>Manager: CheckParameter(ctx)
activate Manager
Manager->>Manager: validate publicKey, prover metadata,<br/>provider type, hard-fork names, blocklist, assignment
Manager->>Context: store proverTaskContext (proverTaskCtxKey)
Manager-->>Controller: return assignedProverTask / nil or error
deactivate Manager
alt Manager returns context (or assignment)
Controller->>BaseTask: checkParameter(ctx)
activate BaseTask
BaseTask->>Context: retrieve proverTaskContext
BaseTask-->>Controller: return cached context
deactivate BaseTask
Controller->>Controller: determine proof type & select proverTask
else Manager returns error or nil assignment
Controller-->>Controller: return get-task error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
coordinator/internal/controller/api/get_task.go(3 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(2 hunks)coordinator/internal/logic/provertask/bundle_prover_task.go(2 hunks)coordinator/internal/logic/provertask/chunk_prover_task.go(2 hunks)coordinator/internal/logic/provertask/prover_task.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
coordinator/internal/logic/provertask/chunk_prover_task.go (3)
coordinator/internal/orm/chunk.go (1)
NewChunk(65-67)coordinator/internal/orm/l2_block.go (1)
NewL2Block(43-45)coordinator/internal/orm/prover_task.go (1)
NewProverTask(50-52)
coordinator/internal/logic/provertask/bundle_prover_task.go (5)
coordinator/internal/orm/l2_block.go (1)
NewL2Block(43-45)coordinator/internal/orm/chunk.go (1)
NewChunk(65-67)coordinator/internal/orm/batch.go (1)
NewBatch(75-77)coordinator/internal/orm/bundle.go (1)
NewBundle(48-50)coordinator/internal/orm/prover_task.go (1)
NewProverTask(50-52)
coordinator/internal/logic/provertask/batch_prover_task.go (4)
coordinator/internal/orm/l2_block.go (1)
NewL2Block(43-45)coordinator/internal/orm/chunk.go (1)
NewChunk(65-67)coordinator/internal/orm/batch.go (1)
NewBatch(75-77)coordinator/internal/orm/prover_task.go (1)
NewProverTask(50-52)
coordinator/internal/logic/provertask/prover_task.go (4)
coordinator/internal/orm/prover_task.go (3)
ProverTask(20-47)ProverTask(55-57)NewProverTask(50-52)coordinator/internal/orm/prover_block_list.go (3)
ProverBlockList(13-24)ProverBlockList(32-34)NewProverBlockList(27-29)coordinator/internal/types/auth.go (5)
PublicKey(16-16)ProverName(18-18)ProverVersion(20-20)ProverProviderTypeKey(22-22)HardForkName(24-24)coordinator/internal/types/prover.go (2)
ProverProviderType(59-59)ProverProviderTypeInternal(76-76)
coordinator/internal/controller/api/get_task.go (5)
common/types/message/message.go (1)
ProofType(14-14)coordinator/internal/logic/provertask/prover_task.go (3)
ProverTask(36-38)ProverTaskManager(41-44)NewProverTaskManager(49-54)coordinator/internal/orm/prover_task.go (2)
ProverTask(20-47)ProverTask(55-57)common/types/response.go (1)
RenderFailure(36-38)common/types/errno.go (1)
ErrCoordinatorGetTaskFailure(24-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
🔇 Additional comments (11)
coordinator/internal/logic/provertask/batch_prover_task.go (1)
42-49: LGTM: Constructor simplification aligns with BaseProverTask refactor.The removal of
proverBlockListOrminitialization is consistent with the refactoring described in the PR summary, where parameter validation (including blocklist checks) is now handled by ProverTaskManager before task assignment.coordinator/internal/logic/provertask/chunk_prover_task.go (2)
39-45: LGTM: Constructor refactored consistently.The removal of
proverBlockListOrmfromBaseProverTaskinitialization aligns with the centralization of blocklist checks inProverTaskManager. All necessary ORM fields remain properly initialized.
58-61: Verification confirms error handling is properly layered.The validation error flow is correctly implemented:
ProverTaskManager.CheckParameter(called in controller atget_task.go:106) catches and surfaces validation errors viatypes.RenderFailurebefore proceedingAssigninchunk_prover_task.gois only reached after successful validation- The
checkParametercall inAssign(lines 58-61) simply retrieves pre-validated context; if the context key is missing at this point, the generic error is appropriateThe error handling is correctly structured across layers and all validation errors are properly surfaced to the caller.
coordinator/internal/logic/provertask/bundle_prover_task.go (2)
39-47: LGTM: Consistent with chunk task refactor.Constructor changes mirror those in
chunk_prover_task.go, maintaining consistency across task implementations.
60-63: LGTM: Consistent error handling pattern.The parameter checking follows the same pattern as
ChunkProverTask, ensuring consistency. The same consideration about error specificity applies here as well.coordinator/internal/logic/provertask/prover_task.go (3)
40-54: LGTM: Clean separation of concerns.The new
ProverTaskManagerproperly encapsulates task assignment and blocklist validation logic, with a standard constructor pattern.
122-128: LGTM: BaseProverTask properly refactored.The removal of
proverBlockListOrmis consistent with the centralization pattern, and all necessary ORM fields remain in place.
210-217: LGTM: Simplified parameter checking.The refactored
checkParameterappropriately delegates heavy validation toProverTaskManager.CheckParameterand simply retrieves the pre-validated context. This assumes the controller properly callsCheckParameterbefore invoking task assignment.coordinator/internal/controller/api/get_task.go (3)
26-27: LGTM: Clean dependency injection.The
ProverTaskManageris properly initialized and injected into the controller, following standard patterns.Also applies to: 36-44
106-111: LGTM: Proper error handling for parameter validation.The controller correctly handles
CheckParametererrors and returns them to the client with an appropriate error code. This ensures validation errors are surfaced before task assignment.
106-118: Add validation or documentation for assigned task type enforcement.The concern is valid: when a prover has an assigned task, the code retrieves it via
IsProverAssigned()which queries only by public key and status, without filtering by the prover's declaredTaskTypes. This means a prover can be forced to handle a task type it didn't request.The behavior appears intentional (provers must complete assigned tasks before requesting new ones), but it lacks safeguards:
- No validation: The assigned task type is used directly without checking if it matches the prover's declared capabilities in
TaskTypes- No documentation: Provers may not expect to receive task types outside their declared
TaskTypes- Silent type mismatch: If downstream code expects the prover to handle only declared types, mismatches could cause errors
Recommended fixes:
- Add a validation check: if an assigned task type is not in the prover's declared
TaskTypes, either reject it with clear error or log a warning- Document this behavior clearly in comments or API docs: "Assigned tasks override declared TaskTypes; provers must handle their assigned task regardless of capabilities"
- Consider if prover-side validation is needed to handle unexpected task types gracefully
| taskCtx := bp.checkParameter(ctx) | ||
| if taskCtx == nil { | ||
| return nil, fmt.Errorf("check prover task parameter missed") | ||
| } |
There was a problem hiding this comment.
Add logging and improve error message for parameter validation failures.
The current implementation returns a generic error without logging or context when parameter validation fails. This significantly reduces observability and makes debugging difficult in production.
Consider adding a log statement and a more descriptive error message:
func (bp *BatchProverTask) Assign(ctx *gin.Context, getTaskParameter *coordinatorType.GetTaskParameter) (*coordinatorType.GetTaskSchema, error) {
taskCtx := bp.checkParameter(ctx)
if taskCtx == nil {
- return nil, fmt.Errorf("check prover task parameter missed")
+ log.Warn("parameter validation failed for batch task assignment", "height", getTaskParameter.ProverHeight)
+ return nil, fmt.Errorf("failed to validate prover task parameters")
}Note: If checkParameter stores error details in the context, consider retrieving and logging them here for better diagnostics.
📝 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.
| taskCtx := bp.checkParameter(ctx) | |
| if taskCtx == nil { | |
| return nil, fmt.Errorf("check prover task parameter missed") | |
| } | |
| taskCtx := bp.checkParameter(ctx) | |
| if taskCtx == nil { | |
| log.Warn("parameter validation failed for batch task assignment", "height", getTaskParameter.ProverHeight) | |
| return nil, fmt.Errorf("failed to validate prover task parameters") | |
| } |
🤖 Prompt for AI Agents
In coordinator/internal/logic/provertask/batch_prover_task.go around lines 62 to
65, the code returns a generic error when parameter validation fails; update
this to log a descriptive message and return a more informative error. Add a
process/scoped logger call (e.g., bp.logger or ctx logger) before returning,
include contextual details such as which parameters failed and any error stored
in the context by checkParameter (retrieve from ctx if checkParameter puts an
error there), and change the returned error text to something like "invalid
prover task parameters: <brief detail>" so both logs and the error carry
actionable information.
| func (b *ProverTaskManager) CheckParameter(ctx *gin.Context) (*orm.ProverTask, error) { | ||
| var ptc proverTaskContext | ||
| ptc.HardForkNames = make(map[string]struct{}) | ||
|
|
||
| publicKey, publicKeyExist := ctx.Get(coordinatorType.PublicKey) | ||
| if !publicKeyExist { | ||
| return nil, errors.New("get public key from context failed") | ||
| } | ||
| ptc.PublicKey = publicKey.(string) | ||
|
|
||
| proverName, proverNameExist := ctx.Get(coordinatorType.ProverName) | ||
| if !proverNameExist { | ||
| return nil, errors.New("get prover name from context failed") | ||
| } | ||
| ptc.ProverName = proverName.(string) | ||
|
|
||
| proverVersion, proverVersionExist := ctx.Get(coordinatorType.ProverVersion) | ||
| if !proverVersionExist { | ||
| return nil, errors.New("get prover version from context failed") | ||
| } | ||
| ptc.ProverVersion = proverVersion.(string) | ||
|
|
||
| ProverProviderType, ProverProviderTypeExist := ctx.Get(coordinatorType.ProverProviderTypeKey) | ||
| if !ProverProviderTypeExist { | ||
| // for backward compatibility, set ProverProviderType as internal | ||
| ProverProviderType = float64(coordinatorType.ProverProviderTypeInternal) | ||
| } | ||
| ptc.ProverProviderType = uint8(ProverProviderType.(float64)) | ||
|
|
||
| hardForkNamesStr, hardForkNameExist := ctx.Get(coordinatorType.HardForkName) | ||
| if !hardForkNameExist { | ||
| return nil, errors.New("get hard fork name from context failed") | ||
| } | ||
| hardForkNames := strings.Split(hardForkNamesStr.(string), ",") | ||
| for _, hardForkName := range hardForkNames { | ||
| ptc.HardForkNames[hardForkName] = struct{}{} | ||
| } | ||
|
|
||
| isBlocked, err := b.proverBlockListOrm.IsPublicKeyBlocked(ctx.Copy(), publicKey.(string)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to check whether the public key %s is blocked before assigning a chunk task, err: %w, proverName: %s, proverVersion: %s", publicKey, err, proverName, proverVersion) | ||
| } | ||
| if isBlocked { | ||
| return nil, fmt.Errorf("public key %s is blocked from fetching tasks. ProverName: %s, ProverVersion: %s", publicKey, proverName, proverVersion) | ||
| } | ||
|
|
||
| assigned, err := b.proverTaskOrm.IsProverAssigned(ctx.Copy(), publicKey.(string)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to check if prover %s is assigned a task, err: %w", publicKey.(string), err) | ||
| } | ||
|
|
||
| ptc.hasAssignedTask = assigned | ||
|
|
||
| ctx.Set(proverTaskCtxKey, &ptc) | ||
|
|
||
| return assigned, nil | ||
| } |
There was a problem hiding this comment.
Address type assertion safety concerns.
The method performs multiple type assertions without checking the underlying type (lines 65, 71, 77, 84, 90), which could panic if the context values have unexpected types. While these values are typically set by middleware, consider either:
- Using type assertion with the comma-ok idiom:
value, ok := publicKey.(string) - Adding defensive checks to prevent panics in case of misconfiguration
Additionally, line 84 has a compound operation that's particularly risky:
ptc.ProverProviderType = uint8(ProverProviderType.(float64))If ProverProviderType is not a float64, this will panic.
Apply this pattern for safer type assertions:
- ptc.PublicKey = publicKey.(string)
+ publicKeyStr, ok := publicKey.(string)
+ if !ok {
+ return nil, fmt.Errorf("public key has invalid type: expected string")
+ }
+ ptc.PublicKey = publicKeyStrConsider applying similar checks for other type assertions, especially line 84 which converts float64 to uint8.
🤖 Prompt for AI Agents
coordinator/internal/logic/provertask/prover_task.go lines 57-113: several
context value type assertions (publicKey, proverName, proverVersion,
ProverProviderType, hardForkNamesStr) use direct assertions and can panic;
change each to use the comma-ok form and return a clear error if the stored type
is unexpected (e.g., assert string for
publicKey/proverName/proverVersion/hardForkNamesStr), handle ProverProviderType
with a type switch (accept float64 or numeric types and convert safely to uint8,
or fall back to the internal default), and ensure any conversion errors return
descriptive errors rather than causing panics; update ptc fields only after
successful assertions and keep ctx.Set(proverTaskCtxKey, &ptc) as the final
step.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
coordinator/test/api_test.go (1)
237-237: Test expectation correctly updated to match refactored error format.The error message format change from "return prover task err:" to "check prover task parameter failed, error:" is more descriptive and aligns with the ProverTaskManager refactor mentioned in the PR objectives.
Minor formatting note: Consider adding a space after "error:" (i.e., "error: public key") for slightly better readability, though the current format may be intentional as a delimiter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
coordinator/test/api_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
🔇 Additional comments (1)
coordinator/test/api_test.go (1)
258-258: Consistent error format update for batch prover scenario.This change mirrors the update on line 237, ensuring both chunk and batch prover blocked scenarios use the same error message format. The consistency is good and properly reflects the refactored error handling flow.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1748 +/- ##
===========================================
- Coverage 36.90% 36.85% -0.05%
===========================================
Files 245 245
Lines 20957 20982 +25
===========================================
- Hits 7734 7733 -1
- Misses 12401 12429 +28
+ Partials 822 820 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
When a prover query for task, if the prover declare that it support multiple proof types (chunk/batch/bundle), coordinator would select one from them randomly and handle by the corresponding
ProverTaskinterface.However, if a task has been assigned to the prover before, the
ProverTaskinterface would found that but can not go through if the assigned task has different proof type with currentProverTaskobject, only to return an error.For example, a prover often support all 3 proof types. Suppose it has been assigned a task before and start another task querying to the coordinator, there would be only 1/3 chance that it can obtain the detail of assigned task again (when coordinator randomly assigned the chunk prover task handler for this query) while in 2/3 it get an error. This behavior cause many unnecessary task query.
This PR try to fix such an issue by inducing a "ProverTaskManager" object, check the query parameter and know if there is an assigned task before any
ProverTaskinterface has been assigned. So prover would always get the detail of assigned task.Summary by CodeRabbit