fix(bot-kit): confirm mined replacements and fix premature stuck bumps - #197
Conversation
Two defects in the shared pending queue compounded into liquidators fee-bumping transactions milliseconds after broadcast and then reporting the ones that mined as failures. The stuck-age baseline was whatever block the caller passed, and all five callers passed the head captured at tick entry — before discovery, quoting and simulation. A slow tick is several blocks, so an entry was born past `stuckBlocks` and the next `onBlock` replaced it. `blockNumber` now leaves `SubmitArgs` entirely: the queue stamps `submittedAtBlock` from the first `onBlock` that sights an entry, a head it has actually seen and therefore necessarily after the broadcast. `replaceStuck` resets it the same way, so a replacement is re-sighted rather than aged from a maintenance block that already spent a receipt read and a gas estimation. A fee bump also overwrote the hash it replaced, and both the receipt sweep and the nonce-consumed reconciler only ever looked at the latest one. When the original mined and the replacement did not, the queue saw a consumed nonce with no receipt of its own and logged `tx.dropped/nonce_consumed` for a transaction that had succeeded. Entries now keep every hash broadcast for a nonce, newest first and bounded by `maxBumpAttempts`; `scanReceipts` settles on the first hash with a receipt, and `nonce_consumed` fires only when a clean read of all of them found none. A read failure yields `unknown`, which never retires an entry — otherwise a transient error on the replacement would mask a mined original. Only `tx.confirmed` / `tx.reverted` change shape, to name the hash that actually mined; every other event still names the latest broadcast. Verified against blue-liquidation on Base, 2026-08-19: six `tx.bumped` events fired 176-422ms after their own `tx.sent` on a 2s-block chain, five of which then logged `nonce_consumed` for transactions that mined. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82291fc477
ℹ️ 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".
- vault-v1/v2 reallocation were left half-migrated: their tick `submit` dep type still declared `blockNumber` and still threaded `deps.chainHead` into it. TypeScript accepted the narrower handler in `index.ts`, so this compiled while shipping the exact tick-entry-stale head the change exists to delete. Both seams and their pinned test assertions updated; the `tick.end` log field keeps its own `chainHead`. - Add `test/queue/receipt.utils.test.ts`, mirroring the sibling modules. It covers the branch `pending-queue.test.ts` could not reach: a read failure on an OLDER hash while the newest reads clean must still yield `unknown`, since the unreadable hash is the one that may have mined. Verified by reverting the failure accumulation, not the assertion. - Amend TIB-2026-05-28, which specified both the single-`txHash` record and `currentBlock - submittedAtBlock` stuck detection. - `txHashes` is bounded by `maxBumpAttempts + 1`, not `maxBumpAttempts`: `attempt` also increments on a transient `tx.replace_failed`. - Give each rule one home and drop the incident narration from comments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- `reverts_on_replace` retired an entry on the revert alone, discarding every retained hash unread. The original is a likely reason the replacement reverted: it can mine during that send's gas estimation, after the sweep read it as pending. The path now re-scans first — settling a mined hash, dropping only on a clean negative read, and keeping the entry when the scan itself failed. Both branches covered by tests verified against the pre-fix behaviour. - Scan the retained hashes concurrently. They are independent, the common case reads all of them anyway, and this runs in the maintenance pass the tick waits on, so serial reads multiplied RPC latency by hash count. - Restore JSDoc on `ResolverExecutionService.submit`, whose signature this branch changed. - Four comments in both liquidators still described the polled tick height as the queue's `submittedAtBlock` — the exact behaviour this branch removes. - `settleMined` becomes an arrow constant. Its `function` siblings in the same closure predate this branch and are left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed the automated review in Devin — late mining escapes confirmation ( Codex P1 — scan replacement receipts concurrently. Taken. The reads are independent and the dominant case (nothing mined) reads all of them anyway, so Codex P1 — document the changed public Codex P2 — stale claims that the tick head seeds the queue baseline. Taken, and there were four such sites across both liquidators, all now describing the sighting semantics instead. Codex P1 — @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1694a2b64b
ℹ️ 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".
Codex P1 on the crossed-books service: `run()` documented its return value but not what it emits, what it broadcasts, or what it propagates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Second Codex pass, P1 — document P1 — do not treat an immediate null receipt as definitive. The premise checks out:
So this is the same trade-off as the @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca16d8a88d
ℹ️ 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".
The scan → settle-if-mined → hand-back rule had three call sites: the per-block sweep, the nonce-consumed reconciler, and the replacement- reverted path added while addressing review. Each re-derived it, and each was a place a caller could discard a retained hash unread — which is the defect this branch exists to fix. `settleIfMined` is now the only caller of `scanReceipts`. It settles a mined entry itself and returns what is left to decide, with `unreadable` distinct from `unmined` so no caller can retire an entry on a read that failed. `settleMined` folds into it, so this is a net removal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Also folded the settlement check into one home ( The scan → settle-if-mined → hand-back rule had grown three call sites — the per-block sweep, the nonce-consumed reconciler, and the Behaviour is unchanged, and I re-confirmed the guarantee is still guarded: making The retirement-policy question — including the failover case above — is now BOTS-102. |
cashd
left a comment
cashd
left a comment
There was a problem hiding this comment.
You just need to resolve the comments for the accept gate to open, you should also add into your global Claude or Codex configuration to do this for you once the agent has addressed the specific response
|
Updated the JSDoc to describe |
|
Added substantive JSDoc to |
- Add substantive JSDoc for PendingQueue.onBlock per Codex review. - Clarify CrossedBooksBotService.run() documents attempted queue submission rather than unconditional broadcast. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
b82dfb8 to
6df560e
Compare
2 participants
Problem
Two defects in the shared pending queue compound into liquidators fee-bumping transactions
milliseconds after broadcast, then reporting the ones that mined as failures.
A. The stuck-age baseline was whatever block the caller passed. All five callers passed the head
captured at tick entry — before discovery, quoting and simulation. The stuck check is
blockNumber - entry.submittedAtBlock > stuckBlocks(default4n), so a tick that spends a fewblocks quoting produces an entry that is born stuck and is replaced by the very next
onBlock.B. A fee bump forgot the hash it replaced.
replaceStuckoverwroteentry.txHash, and both thereceipt sweep and the nonce-consumed reconciler only ever inspected the latest. When the original
mined and the replacement did not, the queue saw a consumed nonce with no receipt of its own and
logged
tx.dropped { reason: 'nonce_consumed' }for a transaction that had succeeded.Production evidence
blue-liquidation, Base, 2026-08-19 (Better Stack source 2607564). Every
tx.bumpedfired withinhalf a second of its own
tx.sent, on a 2-second-block chain:tx.senttx.bumpedtx.confirmed(the replacement happened to win)tx.dropped/nonce_consumedtx.dropped/nonce_consumedtx.dropped/nonce_consumedtx.dropped/nonce_consumedtx.dropped/nonce_consumedFive settled liquidations reported as losses. Only nonce 11 reported correctly, and only by luck.
Fix
A — first-sighting baseline.
blockNumberleavesSubmitArgsentirely.submittedAtBlockisnow
bigint | null, stamped by the firstonBlockthat sights the entry — a head the queue hasactually seen, and therefore necessarily after the broadcast.
replaceStuckresets it the same way,so a replacement is re-sighted rather than aged from a maintenance block that already spent a receipt
read per entry and a gas estimation on the send.
Sourcing the baseline from a block the queue observed, rather than one it is handed, removes the
class of bug rather than the instance: there is no longer a caller-supplied value to go stale. The
cost is that a bump waits up to one extra block, which is the safe direction and is now documented in
midnight-liquidation's tuning table (it matters at mainnet's
stuckBlocks: 1n).Safe against a hanging tick:
runner.tsrunsmaintain(the queue'sonBlock) beforetickinits own try/catch, so a throwing tick cannot starve the stamp. A slow tick only delays it, which
makes an entry look younger.
B — retain every hash per nonce.
Pending.txHashes: [Hex, ...Hex[]], newest first, bounded bymaxBumpAttempts. A newscanReceiptshelper walks them newest-first and returnsmined/none/unknown; the sweep and the reconciler both settle on the first hash with a receipt and logtx.confirmed/tx.revertedwith the hash that actually mined.nonce_consumedfires only onnone— a clean read of every hash that found nothing.unknown(any read failed) never retires anentry, so a transient error on the replacement cannot mask a mined original.
The reconciler now settles such an entry instead of dropping it, which also applies the settled
cooldown; that was reviewed and called desired on #98.
Log schema is otherwise unchanged —
tx.sent,tx.bumped,tx.dropped,tx.onblock_error,tx.replace_failedandsnapshot()still name the latest broadcast. The nonce-hole latch, thesendAbortedlatch,settle()'s cooldown rule, the submit mutex andSubmitOutcomeare untouched.Scope
queue.submithas five callers, all with the identical defect, so all five are fixed and verified:blue-liquidation, midnight-liquidation, midnight-crossed-books, vault-v1-reallocation,
vault-v2-reallocation. In crossed-books the block was only ever forwarded to the queue, so it also
drops out of
ResolverService/ResolverTransportandCrossedBooksBotService.run().docs/decisions/TIB-2026-05-28-midnight-liquidation-bot.mdspecified both the single-txHashrecord and
currentBlock - submittedAtBlockstuck detection, so it carries a dated amendment.Testing
packages/bot-kit/test/queue/pending-queue.test.tsgains 10 cases across two new describe blocks,and
receipt.utils.tsgets its own suite alongside its sibling modules. Each new case was verifiedby reverting the logic under test, not by editing the assertion:
stuck-age baselinecases failmulti-hash settlementcases failreceipt.utils.test.tsfails — that one is the mirror of the production misreport: an unreadable OLDER hash is the one
that may have mined, so a clean
nullon the newest is not grounds to retire the nonceThe existing ~50 cases needed mechanical updates: a test that submits and jumps straight to a stuck
block now needs its sighting pass first, and a multi-bump ladder needs one per rung.
bots/midnight-liquidation/test/fork/queue.test.tsruns against a real forked node (anvil 1.5.1) andwas confirmed non-vacuous the same way.
Validation:
@repo/bot-kit+ all five bots typecheck clean;pnpm lint0 warnings;pnpm format;pnpm test2997 passed, 1 skipped (the fork suite, run separately withRPC_URL_8453);knipclean.Prior art
Supersedes #98 (closed) and #116 (draft, 530 commits behind
main). Both diagnosed this correctlyand neither merged; this lands the fix on current
main.Fixes BOTS-50 — https://linear.app/morpho-labs/issue/BOTS-50
Follow-up, not in this PR
Every caller that would retire an entry now goes through
settleIfMined, so none of them can discarda hash unread — but that leaves the retirement rule resting on a single receipt scan, trusted when it
is clean and ignored when it is not. Two consequences are deliberately not decided here: an
unreadablescan has no bound, so a persistently failing read can hold an entry (and its label) inthe queue indefinitely; and a clean
nonecan come from a lagging failover backend rather than fromchain truth. Both are the same question — what evidence retires a tracked nonce — and both should be
answered once, for all three callers, rather than bolted onto one path.
Filed as BOTS-102: https://linear.app/morpho-labs/issue/BOTS-102
Not addressed
The nonce-237 transaction the ticket records (
0x0017b634…d86fe, notx.sentin either source).#98 investigated it and found no unlogged send path; the code offers no new evidence.
🤖 Generated with Claude Code