feat(relayer): optionally defer fills sourced from late-arriving origin blocks - #3750
Conversation
…in blocks A block delivered late within its own slot was published after that slot's attestation deadline, so it is materially more likely to be replaced at the same height by the next proposer. The existing deposit confirmation gate compares block heights and cannot observe a same-height replacement. Record per-block arrival lateness in the listener-backed SpokePoolClient and withhold deposits sourced from a late-arriving block until a subsequent block has been built on top of it. Controlled per origin chain by RELAYER_MAX_ORIGIN_BLOCK_LATENESS_<chainId>, disabled by default. Co-Authored-By: Claude <noreply@anthropic.com>
pxrl
left a comment
There was a problem hiding this comment.
Test is failing - needs a fix.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 392f5bf4bc
ℹ️ 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".
originBlockUnsettled() indexed this.config.maxOriginBlockLateness directly. Relayer tests build their config as a partial object literal cast to RelayerConfig, so the field is undefined there and every deposit evaluation threw. Use optional chaining, matching the existing minFillTime lookup. Add test coverage for the feature, which had none -- that is why the uninitialised-config path shipped broken. The final case pins the regression by omitting maxOriginBlockLateness entirely. Also correct the LATE_BLOCK_MIN_CONFIRMATIONS comment: confirmations are counted as blocks built on top (as at Relayer.ts:440), so a value of 2 means two blocks on top, not the one the comment claimed. Co-Authored-By: Claude <noreply@anthropic.com>
|
Fixed in b9f1145.
Added One thing to flag: |
|
@droplet review all comments. |
Record block arrival before the misordered-block check. A same-height replacement is the exact scenario this gate exists to catch, but it takes the misordered branch, so the replacement's own arrival was never recorded and a late replacement of a punctual block looked settled. Lateness is now recorded for every observed block and never lowered for a height, so a second block at the same height can only make that height more suspect. Reject malformed and negative RELAYER_MAX_ORIGIN_BLOCK_LATENESS values, which previously parsed to NaN and silently disabled the check while appearing configured. Reject it on SVM chains too: the SVM listener reports arrival time in place of the slot timestamp, so lateness always reads as ~0 there and a threshold would never fire. Don't early-exit in simulation mode, matching the confirmation gate above, so a dry run still evaluates late deposits end to end. Document the config surface in the relayer README. Co-Authored-By: Claude <noreply@anthropic.com>
|
Reviewed all 6 Codex comments — I'd missed them earlier because I only pulled the review-scoped endpoint for your review, which was empty. Replied on each thread; 5 were real and are fixed in the new commit.
Tests grew to 15; the two new same-height cases both fail without the fix. 66 passing across the affected files, lint and typecheck clean. One thing to be aware of: |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33d1e17400
ℹ️ 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 listener posted a block update after querying its events, so the query duration was counted as block-arrival lateness. On EVM that inflated the startup head; on TRON, where the head is polled and the range query runs on every pass, it inflated every block. Stamping the observation time when the block is seen fixes the interval, but not the underlying problem: for a block discovered by polling, the interval between its timestamp and the poll measures poll phase, not how late the block was published. A punctual block found by a poll looks arbitrarily late. So carry an optional observedAt on the block update and record lateness only when it is present. Only the EVM live subscription sets it. The startup scrape and the TRON head poll do not, and their blocks are treated as settled, which is the documented behaviour for anything not observed live. Correct the README, which claimed restarted state was unaffected. Co-Authored-By: Claude <noreply@anthropic.com>
|
Codex came back with one P2 and it was a good one — fixed, plus a worse instance of the same bug it didn't spot. What it found: the EVM listener reads the head block, scrapes historical events, then posts the block update. So backfill duration was counted as block-arrival lateness, and a slow startup scrape would falsely mark the head as late. What it missed: TRON has the same shape but inside the poll loop ( Why I didn't fix it the way Codex suggested. Stamping the observation time before the scrape corrects the interval, but the deeper problem is that a polled block has no meaningful lateness at all: the interval measures poll phase, not publication lateness. A punctual block polled 8s into a 12s slot reads as 8s late. Recording that would false-positive at a high rate — worse than not measuring. So the block update now carries an optional Also corrected the README — my earlier claim that restarted state was unaffected was wrong. 23 tests in the feature file, 63 passing across affected files, lint and typecheck clean. Two new cases: polled updates record nothing, and lateness comes from the observation rather than message receipt. Still open for you: Worth noting one residual: |
The hold on a late-arriving origin block has to outlast the re-org notification, not just the replacement. A deposit is only backed out once the listener reports the removal, and that report trails the replacement by appreciably more than one block time, so two confirmations can release a deposit while it is still pending removal. Co-Authored-By: Claude <noreply@anthropic.com>
- Assert at startup that a configured lateness threshold has the external listener enabled. The listener is the only source of block arrival times, so the threshold was previously inert without it, documented but unenforced. - Stop shadowing the outer blockNumber in the retention sweep. - Correct the README: the first block pushed after a (re)connect can carry an inflated interval and be held, so restarts are not wholly unaffected. Co-Authored-By: Claude <noreply@anthropic.com>
|
Two follow-up commits, in response to the review.
Not taking the
Also folded in the standing items: the I left the sweep unconditional. Gating it on map size looked like a cheap win but changes eviction from height-distance to size-based, which breaks the retention test's contract for no real gain — the map is bounded by the sweep itself. |
Rename the measured quantity: it is the interval between a block's timestamp and the listener observing it, which conflates the proposer publishing late with propagation, so naming it after either alone is misleading. "Arrival delay" stays neutral about the cause and matches the surrounding recordBlockArrival / BLOCK_ARRIVAL_HISTORY vocabulary. Retains "late block" where it names the phenomenon rather than the measurement, that being the established term for a block published after its slot's attestation deadline. The operator-facing threshold becomes RELAYER_MAX_ORIGIN_BLOCK_ARRIVAL_DELAY_<chainId>. No migration needed: the feature is unreleased and off by default. Co-Authored-By: Claude <noreply@anthropic.com>
Drop the assertion that a configured arrival-delay threshold has the external listener enabled. Refusing to start is disproportionate for a setting that is merely inert without the listener, and it makes the threshold awkward to carry in a shared config across deployments that don't all run one. Back to documenting it as inert. The test now pins that a threshold parses and is retained with the listener disabled. Co-Authored-By: Claude <noreply@anthropic.com>
|
Dropped the external-listener assertion in fc7866b, replied on the thread. You're right that it was disproportionate — the threshold is merely inert without the listener, not broken, and hard-failing startup makes it awkward to carry in a shared config across deployments that don't all run one. Back to documenting it as inert; the test now pins that a threshold parses and is retained with the listener disabled. I kept the two adjacent asserts in that block — malformed/negative values, and SVM where there's no slot timestamp to compare against. Those are genuinely broken input rather than a no-op, but say the word if you want either of those gone too. Note I was a few commits behind when this came in (your comment referenced |
The arrival-delay gate withholds a deposit for LATE_BLOCK_MIN_CONFIRMATIONS blocks on the assumption that the removal will be reported within that window. Nothing measured whether it actually was, so the floor could be outrun by drifting notification latency without any signal. On removal of a deposit whose origin block was observed live, report the arrival delay, the confirmations accrued, the confirmations still to spare, and the interval from the block's own timestamp to the report. A margin trending to zero, or a removal landing after the gate released, indicates the floor needs re-sizing. Records the block timestamp alongside the arrival delay to express that interval in seconds. Both fields track the most suspect observation of a height: the largest delay against the earliest timestamp. Co-Authored-By: Claude <noreply@anthropic.com>
…orting a removal The removal log characterised every live-observed block as late-arriving and framed the confirmations remaining as gate margin. This layer has no access to the relayer's configured threshold, so it cannot know whether the gate was ever engaged. The framing inverted in the case the instrumentation exists to catch: a punctual block filled at its MDC tier and then re-orged is a real invalid fill, and the log would have reported it as having confirmations to spare. Report measured facts only -- arrivalDelay, confirmations accrued, and the interval from the block's timestamp to the report -- and fold them into the existing removal warn rather than emitting a second line. Establishing whether the gate applied is left to whoever knows the configured threshold. Drops confirmationsToSpare: the field name carries the same unfounded claim as the message did, and it is where an alert rule would have read the false reassurance. Co-Authored-By: Claude <noreply@anthropic.com>
|
Fixed in The log now reports measured facts only, folded into the existing removal warn: Absent entirely for blocks not observed live, so their absence is itself meaningful. Establishing whether the gate applied is left to whoever knows the configured threshold. One step further than proposed: I also dropped Agreed on logging all removals rather than gating — that distribution is what produced the arrival-delay figures in the first place. Also took the noise point: one warn per removal now, fields adjacent to the deposit they describe. Tests re-pointed from message text to fields, including negative assertions that the log does not say "late-arriving" or "to spare", plus a case covering a removal for a block that was never observed live. 20/20 locally. |
|
Thanks — nothing to action, no inline comments on the approval. Approved commit |
The gate has been opt-in, which leaves the default posture exposed to the failure it addresses. Default mainnet to 5 seconds; chains without an entry stay disabled, since the arrival delay is only a re-org signal where slot times are fixed. An explicitly configured 0 still disables the check. 5s sits well above the median mainnet arrival delay but inside the tail that replaced blocks occupy, so it favours protection over fill latency. That is the intended trade: a deferral costs a fill, an invalid fill costs the amount fronted. Holding is bounded by LATE_BLOCK_MIN_CONFIRMATIONS, so a deposit deferred in error is filled a few blocks later rather than dropped. Note this turns the gate on by default wherever the external listener runs; it remains inert without one. Re-tune from the arrival delays the removal logs report once there is deployment data. Co-Authored-By: Claude <noreply@anthropic.com>
|
This is a behaviour change worth a reviewer's attention: the gate was opt-in and is now active wherever the external listener runs, third-party operators included. It remains inert without a listener. Sizing, on mainnet measurements: 5s sits at roughly the 90th percentile of arrival delays, so it withholds on the order of 10% of mainnet-origin deposits until their origin block has That is deliberately tighter than the point that maximises protection per deposit withheld, which sits nearer 7s. The rationale is that fill latency is the recoverable cost and an invalid fill is not, and that the current competitive baseline is the wrong reference if comparable mitigations become common. Loosening is a config change, not a code change. The figures come from indexer ingestion timestamps, which run slightly later than the listener's own observation, so the effective threshold in production will be marginally tighter than these percentiles imply. The Tests: 22/22, covering the chain default, no-default chains, and explicit-zero override. |
|
Thanks — nothing to action, no inline comments again. Confirming the approval covers the on-by-default change: it's against All six checks pass, PR is mergeable. Leaving the merge to you. |
On chains with fixed slot times, a block delivered late within its own slot was published after that slot's attestation deadline and is materially more likely to be replaced at the same height by the next proposer. The existing deposit confirmation gate compares block heights, so a same-height replacement is invisible to it.
SpokePoolClient(listener mixin) records, per block, the interval between the block's own timestamp and its arrival. Bounded to the most recentBLOCK_ARRIVAL_HISTORYblocks.Relayer.originBlockUnsettled()withholds a deposit whose origin block arrived at or beyond the configured threshold until a subsequent block has been built on top of it.RELAYER_MAX_ORIGIN_BLOCK_LATENESS_<chainId>, disabled by default.Blocks not observed live have no recorded arrival time and are treated as settled, so backfilled and restarted state is unaffected.
Threshold selection is deliberately left to config: a useful value depends on the operator's own event-delivery latency, which has to be measured per deployment rather than assumed. Off by default for that reason.
🤖 Generated with Claude Code