Fix keeper boot crash, wire profitability gate, add reputation signals, fix ZK worker pool crash (#780, #781, #784, #791) - #1132
Merged
ayomideadeniran merged 2 commits intoAug 31, 2026
Conversation
…s, fix ZK worker pool crash (SoroLabs#780, SoroLabs#781, SoroLabs#784, SoroLabs#791) IMPORTANT bug found and fixed first: keeper/index.js had two separate "initialize execution queue" blocks — `const queue = ...` declared twice in the same function scope. This is a hard SyntaxError ("Identifier 'queue' has already been declared"); the keeper cannot start at all in its current state on main, not even --dry-run. Verified with `node --check keeper/index.js` before and after. Removed the second, incomplete duplicate (missing the retryScheduler wiring the first one has); everything after it already referred to the first declaration, so nothing else needed to change. This looks like a leftover from a merge conflict (the repo has several conflicts_*.txt/ merge_output_*.txt files checked in at the root from past merges). Also found, NOT fixed (out of scope, flagged for follow-up): keeper/src/xdrErrorDecoder.js fails `node --check` with a garbled/ mojibake identifier — genuine file corruption, likely from the same merge history. It's not required anywhere else in the keeper, so it doesn't block boot, but it can't be required either. SoroLabs#780 (webhook trigger): already fully implemented and wired end-to-end — WebhookAuthProtocol (HMAC-SHA256, timestamp tolerance, in-memory and Redis-backed replay protection), WebhookTriggerHandler, and live routing through the metrics server's HTTP listener in index.js. Nothing to fix; added docs/webhook-trigger.md since none existed. SoroLabs#781 (profitability): ProfitabilityEstimator (insights.js) and GasForecaster already existed but neither was imported anywhere outside their own files — completely unwired. Wired both into keeper/index.js: GasForecaster.recordExecution() runs after every successful execution to build per-task fee history; a new pre-execution gate (config-flagged, default OFF via PROFITABILITY_GATE_ENABLED — this changes existing execution behavior, so it must be explicit opt-in) only skips a task when the forecaster has 'high' confidence (real historical samples) that its bounty won't cover the forecasted cost, deferring to normal execution whenever there's insufficient history to be confident. SoroLabs#784 (reputation): KeeperReputationScorer (insights.js) already existed and was already fully wired to a live /admin/reputation-style endpoint via metrics.js — but scored only successRate/uptime/taskCoverage/stake/ missedHeartbeats, none of "execution speed" or "gas efficiency" the issue asks for. Added durationMs and bounty tracking to historyManager.record() calls in index.js, averageDurationMs/ averageGasEfficiency computation in history.js's getExecutionSummary(), and two new weighted signals in scoreKeeper() (both default to neutral 1.0 when a keeper has no history yet, rather than penalizing a fresh node as if it were slow/wasteful). Rebalanced existing weights to make room; verified against the existing insights.test.js assertion (score still > 70 with the same inputs). SoroLabs#791 (ZK worker pool): a real worker_threads pool already existed in zk-proof-service/index.js (spawn, crash-replace, memory limits, 60s timeout, proof caching, in-flight dedup) — but `generateProof()` called `this._acquireWorker()`, which was never defined anywhere. Every /generate-proof request threw `TypeError: this._acquireWorker is not a function` at runtime — confirmed by index.test.js's existing 'should generate ZK proof successfully' test, which exercises exactly this path and would already be failing on main. Added _acquireWorker() (picks an idle worker, marks it active — the load-balancing the issue asks for) and removed a block of dead/unreachable code after an existing `return proofPromise` statement. NOT fixed (separate, larger, out of scope): lib/proof-worker.js still returns a hardcoded mock proof rather than invoking the real Halo2/circom proving logic in lib/halo2-adapter.js — the load-balancing infrastructure this issue asks for is now real, but the per-worker computation itself is still a placeholder. Closes SoroLabs#780 Closes SoroLabs#781 Closes SoroLabs#784 Closes SoroLabs#791 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014jDDop7frnew1xcCJDSKEw
|
@thelux134 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
@thelux134 is attempting to deploy a commit to the Ayomide Adeniran's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Pr under review |
…utation-zk-worker-pool
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important bug found and fixed first
keeper/index.jshad two separate "initialize execution queue" blocks —const queue = ...declared twice in the same function scope. This is a hardSyntaxError("Identifier 'queue' has already been declared"); the keeper cannot start at all in its current state onmain, not even--dry-run. Verified withnode --check keeper/index.jsbefore and after. Removed the second, incomplete duplicate (missing theretrySchedulerwiring the first one has). This looks like a leftover from a merge conflict — the repo has severalconflicts_*.txt/merge_output_*.txtfiles checked in at the root from past merges.Also found, not fixed (out of scope, flagged for follow-up):
keeper/src/xdrErrorDecoder.jsfailsnode --checkwith a garbled/mojibake identifier — genuine file corruption. It's not required anywhere else in the keeper, so it doesn't block boot, but it can't be required either.Summary
WebhookAuthProtocol(HMAC-SHA256, timestamp tolerance, in-memory and Redis-backed replay protection),WebhookTriggerHandler, and live routing through the metrics server's HTTP listener. Nothing to fix; addeddocs/webhook-trigger.mdsince none existed.ProfitabilityEstimatorandGasForecasteralready existed but neither was imported anywhere outside their own files. Wired both intokeeper/index.js:GasForecaster.recordExecution()runs after every successful execution to build per-task fee history; a new pre-execution gate (config-flagged, default OFF viaPROFITABILITY_GATE_ENABLEDsince this changes existing execution behavior) only skips a task when the forecaster has'high'confidence its bounty won't cover the forecasted cost, deferring to normal execution whenever there's insufficient history.KeeperReputationScoreralready existed and was already wired to a live endpoint, but scored nothing for "execution speed" or "gas efficiency". AddeddurationMs/bountytracking through tohistory.js'sgetExecutionSummary(), and two new weighted signals inscoreKeeper()(both default to neutral 1.0 when a keeper has no history yet). Verified against the existinginsights.test.jsassertion (score still > 70 with the same inputs, after rebalancing weights).worker_threadspool already existed (spawn, crash-replace, memory limits, timeout, proof caching) — butgenerateProof()calledthis._acquireWorker(), which was never defined. Every/generate-proofrequest threw aTypeErrorat runtime — confirmed byindex.test.js's existing'should generate ZK proof successfully'test, which already exercises this path and would be failing onmain. Added_acquireWorker()and removed a block of dead/unreachable code after an existingreturn proofPromisestatement. Not fixed (separate, out of scope):lib/proof-worker.jsstill returns a hardcoded mock proof rather than invoking the real Halo2/circom logic inlib/halo2-adapter.js.Test plan
node --checkon every changed file, before and after, to catch both the pre-existing crash and confirm no new syntax errorsnode --checksweep ofkeeper/src/*.jsandzk-proof-service/lib/*.jsto look for further corruption from the same merge history (found onlyxdrErrorDecoder.js, flagged above)index.test.js's existing ZK proof generation test against the_acquireWorkerfix to confirm it now exercises the real, working pathinsights.test.js's existing reputation-score assertion against the rebalanced weights to confirm it still passesCloses #780
Closes #781
Closes #784
Closes #791