fix(bot-kit): stamp pending txs with the broadcast-time head - #116
fix(bot-kit): stamp pending txs with the broadcast-time head#116garethfuller wants to merge 5 commits into
Conversation
Original prompt from gareth
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
@claude review |
|
@codex review |
![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: 985771130c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| let submittedAtBlock = args.blockNumber | ||
| if (getBlockNumber) { | ||
| const observed = await tryCatch(getBlockNumber()) |
There was a problem hiding this comment.
When getBlockNumber is read here it still happens before send(...); with the real signer, send then runs prepareTransactionRequest before sendTransaction, so a slow gas-estimation/RPC path can still advance more than stuckBlocks before the tx is actually broadcast. In that case the next maintenance pass can again see the entry as already stale and fee-bump it immediately, so the head should be sampled after a successful send (or at least maxed with a post-send sample).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in d8b61d7. The sample now happens after send resolves, so the gas-estimation window inside prepareTransactionRequest is covered. Sampling late is the safe direction — it can only make the entry look younger and delay a legitimate bump by the send duration, whereas sampling early reintroduces the bug in miniature.
| * A slow tick can make its caller-supplied head many blocks stale and trigger stuck-detection | ||
| * immediately; read failures fall back to `submit.blockNumber` without preventing the send. | ||
| */ | ||
| getBlockNumber?: GetBlockNumber |
There was a problem hiding this comment.
Because this new hook is optional, queue consumers that are not updated keep the old tick-start stamping behavior silently; rg createPendingQueue\( shows bots/midnight-crossed-books/src/bootstrap.ts:70 still constructs the shared pending queue without getBlockNumber, while that bot also does market/book API work and simulation before queue.submit, so it remains exposed to the same immediate-bump/false-drop path this patch fixes. Make the hook required for production callers or wire the crossed-books queue as well.
AGENTS.md reference: AGENTS.md:L161-L169
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — midnight-crossed-books was a genuine miss. Fixed in d8b61d7 by taking the stronger option: getBlockNumber is now required rather than optional, and all three constructors are wired. Optionality was the real defect here — unlike syncNonce / getConsumedNonce, which are opt-in features, this is a correctness requirement, so a silent fallback to the old behaviour is a footgun.
| } | ||
| const oldHash = entry.txHash | ||
| entry.txHash = replaced.data.txHash | ||
| entry.txHashes.unshift(replaced.data.txHash) |
There was a problem hiding this comment.
This successful replacement path still resets submittedAtBlock from the onBlock height rather than from a fresh head around the replacement send. If maintenance is catching up after a slow tick, that blockNumber can already be several blocks behind the actual broadcast, so the replacement is born stale and can be bumped again on the next pass before it has had stuckBlocks blocks to mine; apply the same broadcast-time stamping to replacements as to first sends.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in d8b61d7 — the replacement path now uses the same post-send sampling helper as the initial send, falling back to the onBlock height. Both call sites share one helper rather than duplicating the fallback/no-backwards logic.
| const { logger, events } = captureLogger() | ||
| const { queue, sends } = setup({ | ||
| getBlockNumber: async () => { | ||
| throw new Error('rpc down') |
There was a problem hiding this comment.
This new case models an expected getBlockNumber/provider outage with a raw Error, but the repo rule requires expected infrastructure/provider failures to use named exported Error subclasses in their own kebab-case error files. Please reuse or introduce a typed error for the mock so the test does not add another plain expected failure path.
AGENTS.md reference: AGENTS.md:L40-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining this one. The AGENTS.md rule governs expected failures raised by our own domain/application/infrastructure code, so that callers can discriminate them by type. This is a test stub imitating a third-party RPC outage: what the fallback actually catches at runtime is a viem/transport error class, not one of ours. Introducing a BotKit-owned error subclass purely to throw from a mock would make the test less faithful to the failure it models, and the assertion doesn't depend on the error's type — only that any rejection is swallowed, logged as head.read_failed, and doesn't lose the sent entry.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /** | ||
| * Reads the chain head immediately before broadcast so `submittedAtBlock` reflects broadcast time. | ||
| * A slow tick can make its caller-supplied head many blocks stale and trigger stuck-detection | ||
| * immediately; read failures fall back to `submit.blockNumber` without preventing the send. | ||
| */ |
There was a problem hiding this comment.
🔴 This PR wires the new broadcast-time getBlockNumber fix into blue-liquidation and midnight-liquidation, but bots/midnight-crossed-books/src/bootstrap.ts (line ~70) also builds createPendingQueue and is left unwired, so it still stamps submittedAtBlock with the stale tick-start head. This re-exposes the CRTR-2851 root cause for that bot: a slow scan tick can trigger an instant, spurious fee-bump on the very next block. Since chainClient and getBlockNumber are already imported/used in this file, wiring getBlockNumber: () => getBlockNumber(chainClient) into the queue constructor is a one-line fix.
Extended reasoning...
The bug: createPendingQueue's new getBlockNumber accessor (packages/bot-kit/src/queue/pending-queue.ts:118-122) is optional, and this PR only threads it through two of the three bots that construct a shared queue. bots/midnight-crossed-books/src/bootstrap.ts:70-77 builds its queue with send, getReceipt, getBaseFee, syncNonce, getConsumedNonce, maxFeeWei, and logger — but no getBlockNumber. Because the accessor is optional, submit() silently falls through to the args.blockNumber fallback, which is exactly the pre-fix (buggy) behavior this PR is otherwise eliminating.
Code path that triggers it: bootstrap.ts's runner tick calls bot.run({ blockNumber }) with the tick-start head (captured once per tick, before any I/O). CrossedBooksBotService.run iterates markets — each doing an order-book API round trip via RouterApiService, then matching, then an on-chain resolver.simulate call — before finally calling resolver.submit(prepared, blockNumber) (resolver.service.ts:32), which flows into ViemResolverTransport.submit (resolver.transport.ts:51-59) and then queue.submit({ ..., blockNumber }). That chain of HTTP + RPC round trips can easily span multiple ~2s Base blocks on a cold or slow tick, exactly the scenario that caused the original nonce_consumed false-failures.
Why the existing code doesn't prevent it: the queue's own defense — reading the head fresh right before broadcast — depends entirely on the caller wiring getBlockNumber in. Since it's an optional parameter with a silent fallback, a caller can omit it with no compile-time or runtime signal that anything is missing. This bot already imports getBlockNumber from viem/actions and uses it for the runner's own block-watch loop (bootstrap.ts:16,115), so the omission at the queue construction site looks like it was simply missed when porting the fix from the two liquidators.
Impact: with the stale tick-start stamp, submittedAtBlock can already be several blocks old by the time the tx is actually pending. The very next onBlock can satisfy blockNumber - entry.submittedAtBlock > stuckBlocks (4) and fire an immediate, spurious fee-bump — before the original transaction had any real chance to mine. Note that defect B from this PR (the txHashes receipt-history fix) is shared queue code, so it does apply to crossed-books regardless of this wiring gap — meaning the catastrophic false nonce_consumed drop that hit midnight-liquidation in production is not fully reproduced here. The residual harm is premature/spurious fee-bumps, wasted gas on replacement transactions, and potentially exhausting maxBumpAttempts and dropping a resolve that would otherwise have mined.
Step-by-step proof:
- Tick starts at block N;
bot.run({ blockNumber: N })is invoked. - The bot fetches listed markets, order books (external API), matches crossed positions, and calls
resolver.simulate(RPC) — say this takes ~9s, spanning ~4-5 Base blocks (chain is now at block N+5). - Simulation succeeds;
resolver.submit(prepared, N)is called with the now-stale block N. queue.submithas nogetBlockNumber, sosubmittedAtBlock = N(the fallback).- The broadcast lands in the mempool at real time corresponding to block N+5.
- The very next
onBlock(N+6)maintenance pass checks(N+6) - N = 6 > stuckBlocks (4)→ true, and immediately replaces (bumps) a transaction that hasn't even had one full block to confirm.
Fix: mirror the two liquidators — pass getBlockNumber: () => getBlockNumber(chainClient) into the createPendingQueue call in bootstrap.ts. Both the import and the client are already present in this file, so it's a one-line addition.
There was a problem hiding this comment.
Correct, and the analysis of the crossed-books path matches what I found — fixed in d8b61d7. Rather than the one-line wiring, getBlockNumber is now a required option so an unwired caller is a compile error instead of a silent fallback to the old behaviour; all three constructors are wired. Your read on the blast radius is right too: defect B is shared queue code, so crossed-books was already covered there, and the residual exposure was spurious bumps and wasted gas rather than false nonce_consumed drops.
1 participant
Summary
Closes CRTR-2851.
Production
midnight-liquidationlogged both of its last two liquidations astx.dropped / reason: nonce_consumedeven though both mined successfully on Base (0xdbaeba2c…d609dd2nonce 238 block 49202078 status 1,0x30207144…cf166e72nonce 239 block 49228559 status 1). Two defects in the shared pending queue compound into that false failure.A.
submittedAtBlockwas stamped with the tick-start head rather than the broadcast-time head.tick.tscaptureschainHeadbefore discovery and passes it all the way through toqueue.submit. On prod the probe/quote path is cold (candidates are rare), soplan.built→tx.senttook ~11s ≈ 6 Base blocks. The entry was therefore born ~6 blocks stale and the very nextonBlocksatisfiedblockNumber - entry.submittedAtBlock > stuckBlocks (4), fee-bumping 386ms after broadcast — before the original could possibly mine. Staging never reproduced it because constant candidates keep the probes warm and ticks finish in ~1.2s.The queue now samples the head itself, after the send resolves:
Sampling after the send rather than before it matters:
sendrunsprepareTransactionRequest(gas estimation) beforesendTransaction, so a pre-send sample can itself go stale by more thanstuckBlockson a slow estimation path. A slightly late sample is the safe direction — it makes the entry look younger, delaying a legitimate bump by at most the send duration. The read can never lose an already-broadcast tx: on error it falls back to the caller's block, and the>guard means a fallback never stamps backwards.replaceStuckgets the same treatment. It previously reused theonBlockheight, but a maintenance pass does a receipt read per entry and the replacement send does its own gas estimation, so a replacement could also be born stale and be re-bumped before it hadstuckBlocksblocks to mine.getBlockNumberis required, not optional. UnlikesyncNonce/getConsumedNonce, which are genuinely opt-in features, this is a correctness requirement — an unwired caller would silently keep the buggy behaviour with no compile-time signal. All three queue constructors (midnight-liquidation,blue-liquidation,midnight-crossed-books) are wired using each bot's existingviem/actionsaccessor.B. A fee bump forgot the hash it replaced.
replaceStuckoverwroteentry.txHash, so the receipt sweep andreconcile()only ever looked at the replacement. When the original mined and the replacement didn't, the queue saw the nonce consumed with no receipt for the hash it was tracking and emittednonce_consumed— discarding the real confirm/revert outcome. Entries now keep every hash broadcast for the nonce (newest first, naturally bounded bymaxBumpAttempts):entry.txHash = replaced.data.txHash +entry.txHashes.unshift(replaced.data.txHash)onBlocksettles on the first hash with a receipt and logstx.confirmed/tx.revertedwith the hash that actually mined;reconcile()dropsnonce_consumedonly when none of the tracked hashes has one. A read error during that scan still suppresses the drop, preserving the old "a transient RPC failure must not evict a live entry" semantics.entry.txHashkeeps meaning "latest broadcast" for replacement logic and for thetx.sent/tx.bumped/tx.droppedfields, so the log schema is unchanged apart fromtx.confirmed/tx.revertednow carrying the mined hash, plus the newhead.read_failedwarn. Drop reasons, thestuckBlocksdefault, the nonce-hole latch,snapshot(), andinflightLabels()are untouched.Testing
packages/bot-kit/test/queue/pending-queue.test.tsgains five cases, each verified to fail against the pre-fix behaviour (by reverting the logic under test, not by editing the assertion):0n) with a broadcast-time head of12nis not bumped at block13n— pre-fix13 - 0 > 4bumpshead.read_failed, and still sendstx.confirmedwith that hash and never emitsnonce_consumedbun run --filter @repo/bot-kit typecheck,bun lint(0 warnings),bun format, andbun testall pass; CI is green. The fork queue test stubs the accessor at0nso its syntheticonBlockheights keep driving stuck-detection rather than the fork's real head.Follow-up (not in this PR)
bots/midnight-liquidation/src/index.tsclaims maintenance is kept "off the tick" so an API outage can't starve broadcast transactions of receipt checks and fee replacement. That is not true today:watcher.tsawaits eachonBlockcallback in sequence andrunner.tsawaits the fulltick(height)inside it, so a slow tick blocks all queue maintenance. It doesn't cause spurious bumps once the stamp is correct (stuck-detection is measured in block deltas), but it does delay confirmations, and the comment should either become true or be corrected. Worth its own ticket — decoupling them introduces real concurrency over thependingmap.Link to Devin session: https://app.devin.ai/sessions/ea20463be53f4b43b3818d5ebf5f8d4d
Requested by: @garethfuller