fix(bots): count only broadcast submits and explain skipped plans - #134
fix(bots): count only broadcast submits and explain skipped plans#134haydenshively wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d4d253585
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
6d4d253 to
9d6a2ac
Compare
9d6a2ac to
87c2a32
Compare
87c2a32 to
31e9257
Compare
c3a912b to
7e4a4da
Compare
7e4a4da to
e0f448d
Compare
A BetterStack review of bot.liquidation.midnight found the bot looking healthy while doing nothing: since 2026-07-31 every tick reported `liquidatable: 12, planned: 0` with every skip counter at zero, for ~215,000 ticks, and no way to learn why. Three defects, all silent, both liquidators affected identically: 1. Two `continue`s (position in flight, sizing refused) incremented no counter and logged nothing. Now `inflightSkipped` / `planSkipped`, plus a sampled `plan.skipped` carrying the sizing inputs AND the derived numbers so the decision replays from one log line. 2. `submitted` counted submit *calls*, not broadcasts: on 2026-07-30 it summed 2,425 while tx.sent was 0 and tx.submit_failed was 2,666. `PendingQueue.submit` now returns a `SubmitOutcome`, and only a real broadcast counts. 3. `backoff.clear` ran unconditionally after submit, wiping the attempt count so the delay never grew — a sim-ok/send-fail position was re-quoted, re-simulated and re-sent every block forever. Only a broadcast clears it now; only the per-position failure (tx.submit_failed) records it, since the other three queue exits are queue-wide refusals that would otherwise suppress healthy positions. Also fixes a latent sizing bug found while planning: when `debt - maxDebt < badDebt < debt` the RCF numerator goes negative, and `capBoundPlan`'s `=== 0n` guard let a plan through with a NEGATIVE `seizedAssets` (verified: -8146) that reverts opaquely once abi-encoded. Sizing gains `planWithReason`, with `plan()` kept as a thin facade so the existing exact-bigint sizing tests are untouched. `tick.end` now carries `complete`, and is emitted even when a submit aborts the tick, so partial counters can never read as a genuinely idle tick. Counter identities are asserted in every tick test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review pass for simplicity and comment density. No behavior change: the same 1,475 tests pass, and every bug-reintroducing mutant is still killed. - The sampler's edge-triggering rationale was explained in four places (the primitive, both tick deps, both constants). Now stated once on `BlockSampler.claim`; the call sites just say what they bound. - `planWithReason` computed `effectiveDebt` and `wholeSlotRepaid` in both mode branches. Hoisted into the dispatcher and passed via one shared `ModeStage` type, which also removes the duplicated inline param types and shrinks `postMaturityOutcome` to two lines. - Trimmed the `SubmitOutcome`, `TickCounters` and `PlanSkipReason` docs to the load-bearing parts, dropping prose that restated an adjacent table or field comment. Same for the README paragraph under the queue-exit table. - `invalid` counting reads as one filter instead of a mutable loop. - Dropped editorialising test comments; kept the ones that explain a non-obvious fixture. Closes a coverage gap the mutation run surfaced: nothing pinned that a post-maturity cap uses the post-writeoff debt (`debt - badDebt`) rather than the gross debt, so mutating that sign survived. Now asserted by equivalence, so the test does not re-derive the arithmetic it checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e0f448d to
2d3ee9e
Compare
… overhead
Apply the /simplify pass: SubmitOutcome carries scope ('position' | 'queue')
so callers branch on it instead of string-matching reasons, and drops the
never-read sent payload; the tick epilogue uses try/finally instead of a
closure + tryCatch; the plan-skip sampler is claimed once eagerly per tick;
invalid lens rows are counted without throwaway arrays; sizing builds trace
objects only on refusal paths. Also convert midnight's tick-test helpers to
arrow constants per the repo convention (codex review thread).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Closing as superseded by #181, which absorbs the load-bearing parts of this PR. Recording what moved and what did not, so nothing here is lost. Absorbed into #181:
One deliberate departure: #181 does not introduce a Deferred, tracked in BOTS-87 (https://linear.app/morpho-labs/issue/BOTS-87):
Two latent bugs surfaced while porting the reason taxonomy, both fixed in #181 and neither caught by the previous
What this PR's fix is now known to have cost. The 157 |
Absorbs the load-bearing parts of #134, which supersedes it. `PendingQueue.submit` already documents that it resolves `false` when nothing was broadcast, and that "a caller counting real broadcasts must not count those" — but both liquidators' index.ts closures typed the dep as `Promise<void>` and discarded it. So a failed send cleared the position's backoff and incremented `submitted`, resetting a failing position to attempt 1 with no suppression. Honor the boolean in both bots and add a `notSent` counter. On a no-broadcast outcome the failure history now stands: backoff is neither cleared nor recorded, since a queue refusal says nothing about the position. Replace midnight's `plan()` with `planWithReason()` so a sizing skip reports why (`plan.skipped`) instead of vanishing, and restructure `TickCounters` around documented sum identities so a stage added without a counter breaks a sum instead of silently dropping a position. `tick.end` is now emitted even when a submit aborts the tick, with `complete: false`. Two of the skip reasons guard latent bugs rather than just naming outcomes: - `cap_not_positive`: `maxRepaidNormalMode` takes `effectiveDebt - maxDebt` as its numerator, which goes negative whenever a write-off pushes effective debt under `maxDebt` while `debt > maxDebt` keeps normal mode open (debt 1000, maxDebt 900, badDebt 200). A negative cap propagated through `maxSeizeForCap` into a plan with negative `seizedAssets`. - `nothing_to_seize`: an empty best slot built a `(0, 0)` plan, which `isBadDebtRealization` reads as a write-off against a solvent position. Deferred from #134, not absorbed: `BlockSampler` / `block-cadence.ts`, and blue's `planWithReason` + counter-identity restructure. Blue takes the submit-outcome fix only, since that bug is live on two chains. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Why
A BetterStack review of
bot.liquidation.midnight(source 2607569, 14 days) found the bot looking healthy while doing nothing: ~1,780 ticks/hour, 0.19 ETH funded, 99.99%info. Since 2026-07-31 15:37 every tick reportedliquidatable: 12, planned: 0with every skip counter at zero — ~215,000 consecutive ticks — and no way to learn why without a debugger.Three defects, all silent. Both liquidators had all three verbatim.
continues (in-flight, sizing refused) incremented no counter and logged nothingsubmittedcounted submit calls, not broadcastssubmitted: 2425,tx.sent: 0,tx.submit_failed: 2666backoff.clearran unconditionally after submit, wiping the attempt count so the delay never grewbackoffSkipped: 0for the whole hour — a hot loop burning a LiFi quote + a simulation + a send per position per blockWhat changed
submitreports whether it sent.PendingQueue.submitreturns aSubmitOutcome(sent, orfailedcarrying ascope—'position'or'queue'— plus the exit it took, so adding a reason forces a scope decision in the type). Onlysentcountssubmittedand clears a backoff. TheTxSendError-with-nonce path still throws — the tick must abort rather than race the signer's cursor rollback.Crucially, only the per-position failure (
tx.submit_failed) backs a position off.tx.send_aborted,nonce.sync_failedandqueue.nonce_holeare queue-wide refusals that reject every send that tick; attributing them to whichever position was in hand would suppress healthy positions for 2, 4, 8… blocks after the latch itself cleared.Sizing explains itself. New
planWithReasonreturns a discriminated reason plus aSizingTraceof the derived values (lif,effectiveDebt,cap,capEff,seizedAssets) — the ones an operator cannot compute by hand.plan()stays a thin facade, so the 236-line exact-bigint sizing test file is untouched and still passing. The tick logsplan.skippedwith the inputs and the trace, atinfofor ordinary dust andwarnfor reasons that should be unreachable (a live assertion that eligibility and sizing have not diverged).Bounded diagnostics. One line per position per block would be ~21k lines/hour/bot on a paid source. A new
createBlockSamplerin@repo/bot-kitis claimed once per tick and reset when nothing was skipped — so a quiet stretch never consumes the window (the first skip after any gap always reports), and a persistent condition settles to one coherent snapshot per ~5 min.createBalanceMonitorwas refactored onto it rather than leaving two copies of the cadence pattern.Counters that can't lie. New shape, ordered as the pipeline runs, with identities asserted in every tick test so a future stage added without a counter breaks a sum instead of silently dropping a position:
tick.endnow carriescompleteand is emitted even when a submit aborts the tick — previously such a tick emitted nothing at all, so its counters vanished. The identities hold only forcomplete: true, which is documented.Latent bug found while planning
When
debt - maxDebt < badDebt < debtthe RCF numerator goes negative, somaxRepaid, the cap and the derived seize all go negative.capBoundPlan's=== 0nguard missed it and returned a plan with a negativeseizedAssets(reproduced:-8146) that reverts opaquely once abi-encoded as uint256. The guard now discriminates on the raw cap:cap <= 0n→cap_not_positiveatwarn; a positive cap that floors to zero stays ordinary dust atinfo. (Splitting oncapEffinstead would have mislabelled every 1-wei cap with a non-zero margin as an impossible state.)Also closed: an empty best slot could emit a
(0, 0)plan thatisBadDebtRealizationmisread as a write-off against a solvent position — nownothing_to_seize.Corrected diagnosis
My first read of the incident listed
bestCollateralPrice == 0as a candidate cause. It cannot be: a zero price makesimpliedRepaidUnits0, so0 <= effectiveDebtholds andplan()returnsseizeWholeSlot. UnderisLiquidatablethe only reachable causes are the cap flooring to zero (dust) and the negative-cap path above.Verification
pnpm testafter rebasing onto currentmain— 1,729 pass. The 3 local failures are credential-gated:midnight-liquidation/test/fork/{liquidation,queue}andmarket-making/test/e2e/setup-check, all requiringRPC_URL_8453; GitHub Actions is authoritative for those suites.pnpm -r run typecheckpassed for all workspace packages.pnpm lintpassed with 0 warnings / 0 errors,pnpm formatmade no changes,pnpm knippassed, andgit diff --checkpassed.clearbeforerecord, so a single tick still ended up backed off and my assertion passed. What the bug actually destroys is the accumulated attempt count, so a test now pins that the delay grows (seed at block 1, fail at 100 → a 4-block wait, not 2).debt - badDebt) rather than the gross debt — a pre-existing hole, since sizing against gross debt would over-repay and revert on-chain (Panic 0x11). Now asserted by equivalence, so the test does not re-derive the arithmetic it checks.seizedAssets <= 0n→=== 0ncannot change behavior because the cap guard runs first. Documented in-code as belt-and-braces rather than tested.Acceptance is ultimately a production query — the stuck 12 are live. After deploy,
plan.skippedon source 2607569 answers the original question directly; expectseize_rounds_to_zerowith a dustcap.Deliberately out of scope
submittedchanges the meaning of any chart or alert built on it (sources 2607564 / 2607569), and the new fields need the dashboard metrics-collection step before they are queryable. Needs a follow-up pass.gas required exceeds allowance (0)— a funding problemcreateBalanceMonitorlogs but never warns on.getBaseFee()in both submit wrappers still aborts a tick, now visible astick.end{complete:false}+tick.error. The fix is hoisting the fee read to once per tick (also N RPC calls → 1), not wideningSubmitOutcome.breaking the loop on the three latched refusals, where no later submit can succeed this tick.plan.builtfires before the cooldown and backoff gates, so a plannable but permanently backed-off position logs it every block — pre-existing. Sampling it would lose the audit record for real liquidations; the fix is reordering the gates.Notes for review
Three commits: the implementation, a self-review pass for simplicity and comment density, and a final review-finding remediation. The self-review collapses sampler rationale repeated across four files, hoists
effectiveDebt/wholeSlotRepaidout of both sizing mode branches into one sharedModeStage, and trims docs that restated an adjacent table.No new
Errorsubclass: the liquidators' precedent is result unions (QuoteOutcome,SimulateResult), and the one genuine failure (TxSendError) already is a named subclass and correctly stays a throw.sizing/plan.tsconverted wholesale to arrow consts per the CLAUDE.md rule, rather than leaving a mixed-style file.Rebased directly onto
mainafter refactor(repo): drop bun for vitest, node, and esbuild #130 merged; the obsolete migration commits are no longer part of this PR.Conflict watch: fix(bot-kit): stamp pending txs with the broadcast-time head #116 also edits
packages/bot-kit/src/queue/pending-queue.ts.TIB addenda (not edits, per
docs/GUIDANCE.md) added to both bot TIBs, reconcilingtick.enddrift in both directions —backoffSkipped/cooledDownshipped undocumented,badRoutedocumented but never implemented.🤖 Generated with Claude Code