feat(relayer): defer fill submission until later in the origin chain's block - #3752
feat(relayer): defer fill submission until later in the origin chain's block#3752droplet-rl wants to merge 2 commits into
Conversation
…s block An origin chain re-org invalidates a deposit asynchronously: the SpokePoolClient only backs the deposit out once the removal is reported by the listener. Submitting a fill early in the origin chain's current block leaves no room for that report to arrive. Hold fills back until a configured proportion of the origin chain's current block has elapsed. A deposit that is backed out during the delay is simply not re-queued on the following iteration. Controlled per origin chain by RELAYER_MIN_ORIGIN_BLOCK_ELAPSED_PCT_<chainId>, disabled by default. Deferral is skipped when the origin chain view is stale, since the position within the current block is then indeterminate. Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9353100bf0
ℹ️ 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".
| // Hold fills back until far enough into the origin chain's current block; see deferWithinOriginBlock(). | ||
| if (await this.deferWithinOriginBlock(originChainId)) { |
There was a problem hiding this comment.
Require a removal-aware listener for deferral
When RELAYER_MIN_ORIGIN_BLOCK_ELAPSED_PCT_<chainId> is enabled without RELAYER_EXTERNAL_LISTENER=true—which defaults off in RelayerConfig.ts—this only postpones evaluation: the polling SpokePoolClient advances its search past an ingested deposit block and does not receive the listener's removed notifications, so a reorged deposit remains cached and is filled once elapsed >= avgBlockTime. Require listener-backed clients when this setting is nonzero, or implement equivalent canonical-reorg handling for polling clients.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and fixed in 763aad1. RelayerConfig.validate() now asserts that a non-zero RELAYER_MIN_ORIGIN_BLOCK_ELAPSED_PCT_<chainId> requires RELAYER_EXTERNAL_LISTENER=true (which itself requires POLLING_DELAY > 0, per RelayerConfig.ts:216), so startup fails rather than silently offering no protection.
Confirmed the premise: the removal path is SpokeListener.#removeEvent() in src/clients/SpokePoolClient.ts, which deletes from depositHashes on an EventRemovedMessage. The SDK's polling SpokePoolClient (4.4.18) has no removal handling at all — grepping removed across src/clients/SpokePoolClient/ in the package returns nothing — so an ingested deposit is never backed out there. Deferring on a polling client is pure latency.
I went with rejecting the config rather than implementing canonical-reorg handling for polling clients: that would be a much larger change in SDK territory, and the listener path already exists for exactly this.
| // Per-origin-chain proportion (0-100) of the chain's current block that must have elapsed before fills sourced | ||
| // from that chain are submitted. Allows time for re-org notifications to arrive. 0 disables the delay. | ||
| readonly minOriginBlockElapsedPct: { [chainId: number]: number } = {}; |
There was a problem hiding this comment.
Document the new relayer configuration surface
This adds an operator-facing safety setting and changes fill timing, but src/relayer/README.md is unchanged, leaving operators without the environment-variable name, valid range, or listener-mode prerequisite. The repository instructions explicitly require significant config surfaces and runtime flows to be documented in the affected module documentation in the same change.
AGENTS.md reference: AGENTS.md:L15-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair — this is a config surface with a non-obvious prerequisite, so it clears the AGENTS.md bar. Added a ### Deferring fills within the origin block section to src/relayer/README.md in 763aad1 covering the env var name, the 0-100 range, the RELAYER_EXTERNAL_LISTENER prerequisite, the short-block carve-out, and the scope limits.
| : Constants.RELAYER_SPOKEPOOL_LISTENER_EVM; | ||
| const { RELAYER_SPOKEPOOL_LISTENER_PATH = defaultPath } = process.env; | ||
| minFillTime[chainId] = Number(process.env[`RELAYER_MIN_FILL_TIME_${chainId}`] ?? 0); | ||
| minOriginBlockElapsedPct[chainId] = Number(process.env[`RELAYER_MIN_ORIGIN_BLOCK_ELAPSED_PCT_${chainId}`] ?? 0); |
There was a problem hiding this comment.
Reject invalid block-elapsed percentages
If this environment variable is nonnumeric or negative, Number(...) produces NaN or a negative value and the comparison in deferWithinOriginBlock is always false, silently disabling the requested reorg protection; values above 100 also violate the documented contract without failing startup. Validate that the parsed value is finite and within the inclusive 0–100 range.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 763aad1. validate() now asserts the parsed value is finite and within the inclusive 0-100 range, and throws with the offending env var name and raw value otherwise. A NaN or negative value silently disabling the protection was the worse failure mode of the two, since the operator has explicitly asked for it.
| const elapsed = getCurrentTime() - originSpoke.getCurrentTime(); | ||
| if (elapsed < 0 || elapsed >= avgBlockTime) { | ||
| return false; | ||
| } | ||
|
|
||
| return elapsed < (avgBlockTime * minElapsedPct) / 100; |
There was a problem hiding this comment.
Handle sub-second SVM slots before deferring
When this setting is enabled for Solana, arch.svm.averageBlockTime() returns 0.4 seconds while both timestamps in elapsed have whole-second precision. Consequently elapsed can only be 0, which always defers for every positive percentage, or at least 1, which the stale-view guard immediately bypasses; a looping listener that refreshes the client timestamp on each slot can therefore keep deferring every iteration instead of ever reaching the configured point within a slot. Reject this setting for SVM origins or track elapsed time with sub-second precision.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and confirmed against the SDK source — arch/svm/BlockUtils.ts hardcodes { average: 0.4 } for Solana, and both clocks in elapsed are second-granular (getCurrentTime() is Math.round(Date.now()/1000); the client's side is a block timestamp, itself only specified to the second).
Fixed in 763aad1, but generalised rather than SVM-specific: the gate now returns false, with a warn-once per chain, whenever the measured block time is below MIN_RESOLVABLE_BLOCK_TIME (2s). The reasoning is that this isn't really about SVM — it's about resolution. With second-granular clocks a block shorter than ~1s admits exactly one elapsed value, so every non-zero percentage collapses to "defer iff elapsed === 0" and the setting stops tracking what was configured. That catches Solana at 0.4s, but equally Ink/Unichain/MegaETH at 1s and Arbitrum-orbit at ~0.25s, which an SVM-only rejection would have left silently broken. 2s is the floor at which more than one percentage band is distinguishable; Optimism/Polygon at exactly 2s stay enabled, as do Mainnet, BSC, Linea and Tron.
Sub-second precision isn't a viable alternative here: the reference point is the block timestamp, which has 1s resolution regardless of how precisely the wall clock is read, so there's no additional signal to recover.
One note on the mechanics — I don't think it deferred forever even before the fix. elapsed >= 1 trips the stale guard and returns false, so it alternated rather than wedging. Still meaningless, just not stuck.
Covered by new tests in test/Relayer.BasicFill.ts.
| if (await this.deferWithinOriginBlock(originChainId)) { | ||
| this.logger.debug({ | ||
| at, | ||
| message: `Deferring ${originChain} deposit ${depositId.toString()} until later in the current origin block.`, | ||
| txnRef, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
Preserve full evaluation in simulation mode
When a nonzero delay is configured and a simulation happens early in the origin block, this unconditional return prevents the deposit from reaching profitability, balance, and transaction simulation at all. That conflicts with the existing simulation behavior immediately above, which deliberately continues past confirmation failures so operators can evaluate the full run; bypass this timing gate when transactions are being simulated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, fixed in 763aad1. The gate now mirrors the deposit-confirmation check above it: it logs, then returns this.config.sendingTransactionsEnabled, so a simulation run carries on into profitability, balance and transaction simulation.
Worth noting this matters more than the general "see the full run" argument suggests: POLLING_DELAY=0 is single-shot (src/relayer/index.ts:180), so without the carve-out a deferred deposit is dropped from the entire simulation rather than picked up on a later iteration.
(For the record, the nearer precedent — the minFillTime gate right below — returns unconditionally and has no such carve-out. I followed the confirmation gate instead, since it's the one that reasons explicitly about simulation. Happy to make minFillTime consistent in a follow-up if the team wants that.)
| // Hold fills back until far enough into the origin chain's current block; see deferWithinOriginBlock(). | ||
| if (await this.deferWithinOriginBlock(originChainId)) { |
There was a problem hiding this comment.
Recheck origin-block timing at transaction submission
When evaluation passes late in an origin block, the remaining repayment/profitability work and evaluation of other deposits can carry execution into the next origin block before executeFills submits the destination queue. The fill is then sent early in that new block without another timing check, recreating the exact removal-notification window this setting is intended to avoid, especially on short-block chains or slow RPC iterations. Enforce the delay against a fresh origin-chain view immediately before submitting the queued fills, not only when each deposit is evaluated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Legitimate, and partially addressed in 763aad1 — with a residual gap I want to be explicit about rather than paper over.
Fixed: the dominant source of drift was within a single deposit's own evaluation. The gate sat near the top of evaluateFill, before resolveRepaymentChain() and the gas/profitability work, which is the RPC-heavy part. There's now a second check immediately before the fill is committed, placed ahead of reduceOriginChainLimit()/decrementLocalBalance() so returning there mutates no state. It's ~free: arch.evm.averageBlockTime memoises per chain for 15 minutes, and both clock reads are local. I kept the early check as a cheap short-circuit so deposits that will obviously be deferred don't pay for repayment resolution first.
Not fixed: the window between a fill being queued and the destination queue being flushed. evaluateFills() walks up to RELAYER_DEPOSIT_RATE_LIMIT (25) deposits for a destination before executeFills() runs, so a fill queued early in that walk can still be submitted after an origin block boundary.
I deliberately stopped short of closing that one. executeFills(destinationChainId) flushes a queue that mixes origins, so enforcing the gate there means either dropping the whole destination queue when any one origin is deferring — which lets one origin chain block unrelated fills, and discards work already done — or reaching into the txn queue to drop selectively. The latter is exactly the MultiCallerClient coupling that #3749 was rejected for and that this PR exists to avoid. That tradeoff seems like a call for a maintainer rather than something to land unilaterally in a review pass.
Documented the limitation in src/relayer/README.md so it isn't load-bearing tribal knowledge. Worth weighing against the fact that the whole mechanism is bounded by one block time anyway — it only closes the gap where the removal report was already close behind.
Addresses Codex review on #3752. - Fix a TypeError that broke every Relayer test: config objects that omit minOriginBlockElapsedPct made the property lookup throw. Match the optional-chaining used for minFillTime. - Require RELAYER_EXTERNAL_LISTENER when the delay is enabled. Only listener-backed SpokePoolClients report re-org removals; on a polling client the delay buys latency and no protection. - Validate the percentage is finite and within 0-100, rather than letting a NaN or out-of-range value silently disable the requested delay. - Skip the delay, with a warn-once, on chains whose blocks are shorter than two seconds. Both clocks are second-granular, so a shorter block admits a single elapsed value and every non-zero percentage behaves identically. This covers SVM slots (~400ms) and the shortest EVM chains. - Re-check the block position immediately before the fill is queued, so repayment/profitability resolution spanning a block boundary can't land the fill early in the following block. - Don't defer in simulation mode, matching the deposit-confirmation gate. - Document the setting, its listener prerequisite and its scope in src/relayer/README.md. Co-Authored-By: Claude <[email protected]>
|
Addressed the Codex review in 763aad1. Replies are on each inline thread; summary here. Found a bug the review missed, which was the most severe issue in the PR. Accepted and fixed:
Partially addressed, deliberately:
Added five tests covering the deferral gate. One note on scope: the 2s floor means this setting is a no-op on most L2s. It's still useful where it matters (Mainnet 12.5s, BSC 3s, Polygon/Optimism 2s, Linea/Tron 3s), but it's worth a maintainer sanity-check that the feature is still worth its weight given that footprint. This was the one automated review round for this PR — if you want the queue-flush gap closed or the |
Replaces #3749, which coupled the relayer to
MultiCallerClient. This keeps the change entirely insideRelayer.An origin chain re-org invalidates a deposit asynchronously: the
SpokePoolClientonly backs the deposit out once the listener reports the removal. Submitting a fill early in the origin chain's current block leaves no room for that report to arrive.Relayer.deferWithinOriginBlock()holds fills back until a configured proportion of the origin chain's current block has elapsed. Nothing is cancelled — the deposit is simply reconsidered on the next iteration, and if it was backed out in the meantime it is not re-queued.RELAYER_MIN_ORIGIN_BLOCK_ELAPSED_PCT_<chainId>, disabled by default.Scope note for reviewers: the delay this adds is bounded by one block time, so it closes the gap only where the removal report was already close behind. Removal reports that arrive a block or more after the replacement are unaffected by this setting at any percentage.
🤖 Generated with Claude Code