feat(relayer): verify a deposit's origin transaction before filling - #3751
feat(relayer): verify a deposit's origin transaction before filling#3751droplet-rl wants to merge 2 commits into
Conversation
An origin chain re-org can drop the transaction that emitted a deposit, cause it to revert on replay, or re-include it at a different index. Each of these can change or invalidate the deposit, leaving any fill made against the original event unmatched and unrepayable. Before filling, confirm via receipt that the origin transaction is still mined, successful, and at the block and index where it was observed. RPC failures fail open, since an unavailable provider is not evidence that the deposit is invalid. Disable with RELAYER_VERIFY_ORIGIN_TXN=false. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51c7c22af6
ℹ️ 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".
| return true; | ||
| } | ||
|
|
||
| return receipt?.status === 1 && receipt.blockNumber === blockNumber && receipt.transactionIndex === txnIndex; |
There was a problem hiding this comment.
Verify the emitted deposit rather than only its receipt
When a reorg re-includes the same transaction at the same block height and transaction index, all of these checks still pass even though different preceding state can change the emitted deposit—for example, numberOfDeposits can produce a different depositId. The relayer can therefore fill the stale, unmatched relay data this feature is intended to reject; verify that the current receipt still contains the expected FundsDeposited event and relay data, or otherwise bind the check to the originally observed block/event.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and fixed in 31d5db0 — this was the real gap.
The position checks are necessary but not sufficient: a re-org that re-includes the transaction at the same height and index on top of different preceding state passes all three, while numberOfDeposits shifts underneath it and changes the depositId. originTxnUnchanged now additionally requires the receipt to still emit a FundsDeposited event whose relay data matches the deposit under evaluation, via receiptEmitsDeposit. Matching is on getRelayEventKey, since relay data is what a fill is matched against when the dataworker computes repayment — not the position.
I kept the block/index checks rather than replacing them. If the transaction is re-included at a different height it has not necessarily accrued the confirmations computeRequiredDepositConfirmations gated on, and our cached deposit is stale until the next update(). Skipping and re-evaluating next loop is the cheap, safe outcome there.
Covered by test/Relayer.OriginTxnVerification.ts — "Rejects a deposit whose relay data no longer matches the emitted event" mutates depositId/outputAmount/recipient while holding the receipt position fixed. I confirmed that test fails against the previous implementation and passes with the fix.
| this.sendingRelaysEnabled = SEND_RELAYS === "true"; | ||
| this.sendingSlowRelaysEnabled = SEND_SLOW_RELAYS === "true"; | ||
| this.acceptInvalidFills = ACCEPT_INVALID_FILLS === "true"; | ||
| this.verifyOriginTxn = (process.env.RELAYER_VERIFY_ORIGIN_TXN ?? "true") === "true"; |
There was a problem hiding this comment.
Read the verification flag from the supplied environment
When RelayerConfig is constructed with an explicit environment object that differs from the global process environment, RELAYER_VERIFY_ORIGIN_TXN is silently ignored because this line reads process.env directly. This breaks the constructor's existing configuration contract and can unexpectedly enable receipt queries in tests or embedded callers—or disable protection because of unrelated global state—so destructure and read the flag from env like the other relayer settings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct — fixed in 31d5db0. RELAYER_VERIFY_ORIGIN_TXN is now destructured from env with a "true" default alongside the other relayer settings, so the constructor's configuration contract holds.
| this.sendingRelaysEnabled = SEND_RELAYS === "true"; | ||
| this.sendingSlowRelaysEnabled = SEND_SLOW_RELAYS === "true"; | ||
| this.acceptInvalidFills = ACCEPT_INVALID_FILLS === "true"; | ||
| this.verifyOriginTxn = (process.env.RELAYER_VERIFY_ORIGIN_TXN ?? "true") === "true"; |
There was a problem hiding this comment.
Document the default-on verification control
The inspected relayer README does not mention RELAYER_VERIFY_ORIGIN_TXN, although this introduces a default-on operator-facing configuration surface that adds an RPC request to every EVM deposit evaluation and controls a fill-safety check. Document its name, default, supported-chain scope, and fail-open behavior so operators can configure the runtime safely.
AGENTS.md reference: AGENTS.md:L15-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 31d5db0. Added a "Verifying a deposit's origin transaction" section to src/relayer/README.md (the module doc AGENTS.md points at for relayer config surfaces) covering the name and "true" default, what the check requires, the EVM-only scope, the fail-open-on-RPC-error behavior, and the cost — one eth_getTransactionReceipt per unique origin transaction per loop, prefetched and memoised — plus when an operator would want to disable it.
|
|
||
| // The confirmation gate above compares block heights and cannot detect an origin transaction that was dropped, | ||
| // reverted on replay, or re-included at a different position. Verify it directly before committing funds. | ||
| if (!(await this.originTxnUnchanged(deposit))) { |
There was a problem hiding this comment.
Prefetch origin receipts before sequential fill evaluation
In single-shot/sweeper mode with a large unfilled queue, this await adds one serial origin-chain RPC round trip per deposit before destination transactions can be submitted: checkForUnfilledDepositsAndFill deliberately leaves that mode's deposit count unbounded, while evaluateFills awaits each deposit sequentially for balance accounting. This can make sweepers take minutes or exceed their runtime limit, and multiple deposits emitted by one transaction redundantly fetch the same receipt; prefetch or cache receipt verification concurrently before entering the balance-sensitive sequential evaluation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch on both halves — fixed in 31d5db0.
Receipts are now memoised per origin transaction in originTxnReceipts (keyed chainId-txnRef), so multiple deposits emitted by one transaction share a single lookup instead of refetching. The cache holds the in-flight promise, so concurrent and lazy callers dedupe against each other rather than racing.
prefetchOriginTxns resolves them concurrently in checkForUnfilledDepositsAndFill immediately before evaluateFills, which keeps the round trips off the sequential, balance-sensitive path. I put the prefetch after the rate-limit/fill-status filtering rather than over allUnfilledDeposits, so single-shot mode does not fetch receipts for deposits it will not evaluate.
The cache is reset at the top of each loop — a re-org between loops can invalidate a receipt that previously verified, so it deliberately does not persist. Net cost is one eth_getTransactionReceipt per unique origin transaction per loop, issued in parallel. test/Relayer.OriginTxnVerification.ts asserts the call count is 1 across a prefetch plus two evaluations of the same transaction.
Address automated review feedback on the origin transaction check. Receipt position alone was insufficient. A re-org can re-include the same transaction at the same height and index on top of different preceding state, which changes the SpokePool's numberOfDeposits counter and therefore the depositId. The position checks all passed in that case and the relayer could still fill stale, unmatchable relay data -- exactly what this check exists to reject. Additionally require the receipt to still emit a FundsDeposited event whose relay data matches the deposit under evaluation, since relay data is what a fill is matched against when the dataworker computes repayment. Receipts are now memoised per origin transaction for the duration of a loop and prefetched concurrently before evaluateFills() drops into sequential, balance- sensitive evaluation. Previously each deposit added a serial origin-chain round trip, and deposits sharing a transaction each refetched the same receipt; this bounds it to one eth_getTransactionReceipt per unique origin transaction. Also read RELAYER_VERIFY_ORIGIN_TXN from the supplied env rather than process.env, so the constructor's configuration contract holds, and document the flag's default, scope and fail-open behavior in the relayer README. Co-Authored-By: Claude <noreply@anthropic.com>
|
Addressed all four Codex comments in 31d5db0. Replies are on the individual threads; summary here. P1 — verify the emitted deposit, not just the receipt. This was a real gap and the most important of the four. Position checks are necessary but not sufficient: a re-org that re-includes the transaction at the same height and index on top of different preceding state passes all three while I kept the block/index checks rather than replacing them. A transaction re-included at a different height hasn't necessarily accrued the confirmations P2 — read the flag from P2 — prefetch/cache receipts. Receipts are memoised per origin transaction (keyed P2 — document the flag. New section in Testing. New No further iteration needed from my side — the feedback was self-contained and is fully covered. Worth a human eye on one judgment call though: keeping the block/index checks means a benign re-inclusion at a different height is skipped for a loop rather than filled. I think that trade is right, but it's a deliberate choice rather than something the review asked for. |
An origin chain re-org can drop the transaction that emitted a deposit, cause it to revert on replay, or re-include it at a different index. Any of these can change or invalidate the deposit, leaving a fill made against the original event unmatched and therefore unrepayable. The deposit confirmation gate compares block heights and does not detect any of these cases.
Relayer.originTxnUnchanged()fetches the origin transaction receipt immediately before filling and requires that it is still mined, successful, and at the same block number and transaction index as observed.RELAYER_VERIFY_ORIGIN_TXN=false. Enabled by default.Trade-off worth a reviewer's opinion: this adds one
eth_getTransactionReceiptto the fill path, which is latency-sensitive. It is deliberately unconditional rather than scoped to low-confirmation deposits, since larger deposits wait for more confirmations and would otherwise be the least protected.🤖 Generated with Claude Code