fix(bots): restructure keeper drawing phase gate to allow mid-phase entry - #2575
Open
salgozino wants to merge 4 commits into
Open
fix(bots): restructure keeper drawing phase gate to allow mid-phase entry#2575salgozino wants to merge 4 commits into
salgozino wants to merge 4 commits into
Conversation
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
Contributor
WalkthroughThe keeper bot now uses inclusive timing checks, enters drawing during eligible phases, skips empty draw calls, counts actual newly drawn jurors, and warns when unresolved disputes receive no jurors. ChangesKeeper drawing workflow
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/scripts/keeperBot.ts`:
- Around line 717-719: Update the enterDrawingBlock condition in
contracts/scripts/keeperBot.ts (lines 717-719) so drawing dispatch is allowed in
generating or drawing, while hasMinStakingTimePassed() is evaluated only for
staking; update contracts/test/arbitration/keeperBot-phase-gate.ts (lines 82-93)
to mirror this phase rule and add coverage for a generating-phase restart with
elapsed time below minStakingTime.
- Around line 760-773: Update the drawing loop around drawJurors() and
drawIterationsTotal so the total counts only jurors actually drawn, by comparing
getMissingJurors(dispute) before and after each confirmed transaction rather
than adding requested iterations. Before the stall warning, re-query unresolved
disputes and use that refreshed result when deciding whether to warn and
building pending IDs.
In `@contracts/test/arbitration/keeperBot-phase-gate.ts`:
- Around line 4-7: Extract the keeper phase-entry policy from keeperBot.ts into
an importable production module, preserving the intended phase-aware dispatch
and >= boundary behavior. Update the regression tests in keeperBot-phase-gate.ts
to invoke that production predicate or dispatcher instead of local copies, while
retaining coverage for the affected phase transitions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ef9139e-087a-4afe-808b-2187bbddd00d
📒 Files selected for processing (2)
contracts/scripts/keeperBot.tscontracts/test/arbitration/keeperBot-phase-gate.ts
salgozino
added a commit
that referenced
this pull request
Aug 11, 2026
minStakingTime only gates the staking -> generating transition per SortitionModule.sol. The generating -> drawing transition only requires RNG readiness. A keeper that restarts (or resumes after an external actor advanced the phase) while phase is already generating was wrongly held behind the minStakingTime check, skipping the RNG-readiness check entirely. Addresses code review feedback on PR #2575.
salgozino
added a commit
that referenced
this pull request
Aug 11, 2026
…ions drawJurors() returns true once its transaction confirms, not based on how many jurors it actually drew — its pre-flight probe checks a much larger simulated horizon (iterations * MAX_DRAW_CALLS_WITHOUT_JURORS) than the real batch it submits, so a confirmed tx can still draw zero new jurors. Track actual draws via getMissingJurors() deltas instead of accumulating requested drawIterations, so the zero-draw stall warning can no longer be silently suppressed by transactions that succeed without drawing anyone. Also re-query which disputes are still unresolved before building the warning message. Verified against KlerosCore.sol: nbVotes is immutable during the draw loop (draw() requires Period.evidence, the only nbVotes-changing path via appeal requires Period.appeal — mutually exclusive) and drawnJurors is push-only within draw(), so the getMissingJurors() delta is always >= 0 and always equals actual new jurors drawn in that window. Addresses code review feedback on PR #2575.
…entry without minStakingTime
minStakingTime only gates the staking -> generating transition per SortitionModule.sol. The generating -> drawing transition only requires RNG readiness. A keeper that restarts (or resumes after an external actor advanced the phase) while phase is already generating was wrongly held behind the minStakingTime check, skipping the RNG-readiness check entirely. Addresses code review feedback on PR #2575.
…ions drawJurors() returns true once its transaction confirms, not based on how many jurors it actually drew — its pre-flight probe checks a much larger simulated horizon (iterations * MAX_DRAW_CALLS_WITHOUT_JURORS) than the real batch it submits, so a confirmed tx can still draw zero new jurors. Track actual draws via getMissingJurors() deltas instead of accumulating requested drawIterations, so the zero-draw stall warning can no longer be silently suppressed by transactions that succeed without drawing anyone. Also re-query which disputes are still unresolved before building the warning message. Verified against KlerosCore.sol: nbVotes is immutable during the draw loop (draw() requires Period.evidence, the only nbVotes-changing path via appeal requires Period.appeal — mutually exclusive) and drawnJurors is push-only within draw(), so the getMissingJurors() delta is always >= 0 and always equals actual new jurors drawn in that window. Addresses code review feedback on PR #2575.
salgozino
force-pushed
the
fix(contracts)/keeper-drawing-phase-gate
branch
from
August 11, 2026 19:29
764ee7f to
586beda
Compare
The drawing do-while loop could enter with numberOfMissingJurors already at 0 (dispute fully drawn externally, e.g. by another keeper instance, between the pre-loop snapshot and this iteration), calling drawJurors(dispute, 0) and logging a misleading 'Failed to draw jurors' error for a non-error condition. Mirrors the existing zero-iterations guard already used in the executeRepartitions loop. Found by Judgment Day dual review of the keeper phase-gate fix.
|
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
1 participant
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.
Summary
The keeper bot's drawing-phase entry gate was phase-agnostic:
hasMinStakingTimePassed()andhasMaxDrawingTimePassed()both readsortition.lastPhaseChange()regardless of the court's current phase, and the outer gate applied theminStakingTimepredicate to the entire drawing workflow instead of only thestaking -> generatingtransition. If the court was already ingeneratingordrawing(advanced externally, or after a keeper crash mid-cycle) when a fresh keeper run started, the run could skip the entire drawing/RNG-wait block — and withminStakingTime == maxDrawingTimeon non-devnet deployments, the unconditional back-to-staking cleanup could even push the phase back tostakingwith zerodraw()calls, because the keeper's own predicates used strict>whileSortitionModule.soluses>=.Closes #2574
Changes
contracts/scripts/keeperBot.tsonly — this PR is scoped to the production fix. Three rounds, incorporating code review feedback:hasMinStakingTimePassed()/hasMaxDrawingTimePassed():>→>=SortitionModule.sol's own boundary semanticsgeneratingordrawingand disputes still need jurors, without waiting onminStakingTimeminStakingTimeonly gates thestaking -> generatingtransition per the contract;generating -> drawingonly requires RNG readinessgetMissingJurors()deltas) instead of requested iterationsdrawJurors()returnstrueonce its transaction confirms, not based on how many jurors it actually drew, so the warning could be silently suppressed by a fully-stalled runTest Plan
yarn check-types— cleanyarn check-style— cleanNote on scope
This PR previously included a new regression test file (
contracts/test/arbitration/keeperBot-phase-gate.ts). Per review feedback, those tests reimplemented local copies of the keeper's phase predicates rather than exercisingkeeperBot.tsitself (it has no exports and self-executesmain()on import), so they could pass even if the production fix regressed. Rather than ship tests that don't actually protect this code, the tests move to a follow-up PR that first makes the keeper's phase-entry logic importable/testable, then wires proper regression coverage against the real implementation. That follow-up targets this PR's branch as its base.PR-Codex overview
This PR enhances the
keeperBot.tsfunctionality by refining phase transitions for juror drawing and improving logging for disputes. It adjusts conditions for entering drawing phases and adds checks for actual jurors drawn, aiming to prevent stalls and improve clarity in dispute processing.Detailed summary
>to>=forminStakingTimeandmaxDrawingTime.enterDrawingBlocklogic to handle phase transitions more effectively.Summary by CodeRabbit