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)
| id: entry.label, | ||
| nonce: entry.nonce, | ||
| txHash: entry.txHash, | ||
| txHash: entry.txHashes[0], |
There was a problem hiding this comment.
🟡 Late mining escapes confirmation
When an original transaction mines during replacement, entry.txHashes[0] is reported as dropped and every retained hash is discarded. Successful work escapes confirmation.
Prompt for agents
The reverts_on_replace path in packages/bot-kit/src/queue/pending-queue.ts deletes the entry immediately after an execution-reverted replacement attempt. The original transaction can mine after the preceding receipt scan but before send or gas estimation reports that revert. Because every retained hash is then discarded, no later sweep can confirm the mined original. Re-scan all retained hashes before retiring this path. Settle a discovered receipt normally, drop only after clean negative reads, and preserve the entry on an unknown scan.
Was this helpful? React with 👍 or 👎 to provide feedback.
![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".
| ): Promise<ReceiptScan> => { | ||
| let failure: { error: unknown } | null = null | ||
| for (const txHash of txHashes) { | ||
| const receipt = await tryCatch(getReceipt(txHash)) |
There was a problem hiding this comment.
Once a nonce has been bumped three times, this loop performs four independent receipt RPCs serially for that entry on every onBlock, and the cadence reconciler repeats the scan. Because the inspected createRunner awaits queue maintenance before running the tick, normal RPC latency now multiplies by the number of historical hashes and pending entries, which can make the bot fall behind fast chain heads and delay both confirmation and subsequent work; launch these independent reads together with Promise.all while preserving the newest-first result selection.
AGENTS.md reference: AGENTS.md:L39-L40
Useful? React with 👍 / 👎.
|
|
||
| submit(prepared: PreparedResolution, blockNumber: bigint) { | ||
| return this.transport.submit(prepared, blockNumber) | ||
| submit(prepared: PreparedResolution) { |
There was a problem hiding this comment.
This changed method is public on an exported class but still has no JSDoc. Its documentation needs to cover the prepared-resolution parameter, returned promise, queue-submission side effect, and propagated transport failures so callers can use the changed surface without inspecting the implementation.
AGENTS.md reference: AGENTS.md:L35-L40
Useful? React with 👍 / 👎.
|
|
||
| // Retires an entry the chain settled, naming the hash that ACTUALLY mined — which after a fee bump | ||
| // need not be the latest one broadcast, and is the whole point of tracking every hash per nonce. | ||
| function settleMined( |
There was a problem hiding this comment.
The newly introduced settleMined utility uses a function declaration even though repository rules require utilities to be arrow constants. Convert it to a const arrow helper; the new submitSighted and setupSwappable test helpers introduced by this commit use the same prohibited form and should be converted in the same cleanup.
AGENTS.md reference: AGENTS.md:L51-L52
Useful? React with 👍 / 👎.
| data: encodeExec(market, borrower, plan, swapPlan) | ||
| }), | ||
| submit: async ({ market, borrower, plan, swapPlan, blockNumber, label }) => { | ||
| submit: async ({ market, borrower, plan, swapPlan, label }) => { |
There was a problem hiding this comment.
After this callback stops forwarding blockNumber, the preceding blue-liquidation comment at lines 263–265 and the chainHead documentation in runner/tick.ts lines 95–96 still state that the polled tick height becomes submittedAtBlock; the identical stale claims remain in the midnight liquidator. This now describes the exact premature-aging behavior the commit removes rather than the new first-onBlock sighting semantics, so future queue tuning or debugging will be based on the wrong age baseline unless those comments are updated.
Useful? React with 👍 / 👎.
- 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".
| * @remarks Always simulates first. Readonly mode logs `match.computed` and performs no submission. | ||
| */ | ||
| async run({ blockNumber }: { blockNumber: bigint }) { | ||
| async run() { |
There was a problem hiding this comment.
The changed public run() method documents its return value but not its observable side effects or propagated failures. In write mode it may submit a resolution and emit logs, while failures from market discovery, matching, simulation, or submission can reject the call; callers should not need to inspect the implementation to learn that contract.
AGENTS.md reference: AGENTS.md:L35-L37
Useful? React with 👍 / 👎.
| if (settled?.kind === 'none') { | ||
| settle(entry, blockNumber) |
There was a problem hiding this comment.
When RPC_URL_FALLBACK is configured, the signer uses a per-request failover transport (packages/bot-kit/src/transport.ts), so replacement estimation can observe the original as mined on one backend while this immediate receipt scan receives clean null responses from a lagging backend. This branch then retires the entry as reverts_on_replace, permanently losing the later receipt and recreating the successful-transaction-as-dropped failure this rescan is intended to prevent; retain the entry until a later block or another consistent chain-truth check confirms that none of its hashes mined.
Useful? React with 👍 / 👎.
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".
| * here, so the caller's tick is what isolates them. | ||
| * @remarks Always simulates first, and submits at most one resolution per call. Logs | ||
| * `match.not_profitable` / `match.computed` / `match.submitted`. Readonly mode performs no | ||
| * submission; write mode broadcasts through the pending queue, consuming a nonce. |
There was a problem hiding this comment.
When PendingQueue.submit() resolves { sent: false }—for example after nonce.sync_failed or a pre-broadcast send rejection—ViemResolverTransport.submit() discards that outcome, so this method still logs match.submitted and returns submitted: true without consuming a nonce. The newly added side-effect contract is therefore false for a normal failure path; either propagate the submission outcome or describe this as an attempted queue submission rather than an unconditional broadcast.
AGENTS.md reference: AGENTS.md:L35-L37
Useful? React with 👍 / 👎.
| try { | ||
| const receipt = await getReceipt(entry.txHash) | ||
| if (receipt) { | ||
| const scan = await scanReceipts(getReceipt, entry.txHashes) |
There was a problem hiding this comment.
PendingQueue.onBlock is an externally exposed callable, and this branch materially changes its contract: one invocation now scans every retained hash, records first-sighting state, and can settle, replace, drop, reconcile, mutate cooldown/latch state, and emit isolated RPC errors. The public surface still exposes only a bare onBlock(blockNumber): Promise<void> signature, so add substantive JSDoc covering the parameter, completion, these side effects, and which dependency failures are isolated or propagated.
AGENTS.md reference: AGENTS.md:L35-L37
Useful? React with 👍 / 👎.
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. |
1 participant
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