fix(payout): bring the EVM expired-tx retry under the designate-before-broadcast protection#4237
Merged
Merged
Conversation
Collaborator
Author
|
Review protocol: 4 full review passes (conformity + logic lens each) until zero findings. Pass 1: log-level/DRY cleanups plus making the retry release a conditional transition pinned to the exact tx (4bcc12f). Pass 2: the released hash must stay reconstructable from the DB — added the append-only releasedPayoutTxIds column with migration (4ce2015). Pass 3: aligned the in-memory history with the persisted slice and documented the stale-full-save limit (f4f51df). Pass 4: clean on both lenses. |
TaprootFreak
marked this pull request as ready for review
July 16, 2026 20:06
…e-broadcast protection The expired-retry path re-entered doPayout with payoutTxId still set, assuming nonce reuse would replace the pending tx. For an expired tx the hash has vanished, getTxNonce resolves undefined and dispatch draws a fresh nonce - the retry was an independent second transaction outside the designate-before- broadcast protection. It now goes through the shared rollbackBroadcastForRetry release like the out-of-gas path, so a crash window stays closed and an ambiguous failure escalates instead of silently looping. The release also records every replaced tx hash in a new append-only releasedPayoutTxIds column (migration included), so a PayoutUncertain investigation can still reconstruct the vanished hash from the DB after the retry nulls payoutTxId. The column is exposed via the debug allowlist. Closes #4229
TaprootFreak
force-pushed
the
fix/payout-evm-expired-retry
branch
from
July 17, 2026 15:58
f4f51df to
39fa68e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Brings the EVM expired-tx retry under the same designate-before-broadcast protection as the first broadcast.
Closes #4229 (follow-up to #4181).
Why
#4181 deliberately exempted re-entries with
payoutTxIdset, assuming they replace the pending tx by reusing its nonce. That holds for the manual speedup, but not for the expired-tx retry:isTxExpiredis only true whengetTransaction(hash)returnsnull, sogetTxNonceon the vanished hash resolvesundefinedand dispatch falls back to a fresh nonce. The retry was therefore an independent second transaction with the pre-#4181 semantics:PayoutPendingwith the old expired hash → the next cron run retried again → two live transactions;handleBroadcastErroris a deliberate no-op whenpayoutTxIdis set), so the order silently re-broadcast every 30 s — and once one attempt confirmed between two runs, the next attempt double-paid.How
The expired branch in
EvmStrategy.checkPayoutCompletionDatanow mirrors the out-of-gas branch directly above it:rollbackPayout()(clearpayoutTxId, status back toPreparationConfirmed) is persisted before re-enteringdoPayout. The retry then goes through the regular protected flow: designation is persisted before dispatch, a crash re-selects the order safely, and an ambiguous failure staysPayoutDesignatedand escalates toPayoutUncertainviaprocessFailedOrdersinstead of looping.Comment-only cleanup in
payout.strategy.ts: after this change the manual speedup is the only re-entry that keepspayoutTxId, and the comments ondesignateBeforeBroadcast/handleBroadcastErrorsay so.The rollback persist is conditional (
update({ id, status: PayoutPending, payoutTxId: <the tx this run saw> }, { status: PreparationConfirmed, payoutTxId: null })): the payout cron lock times out after 1800 s, so runs can overlap — a stale run must not roll back an order a concurrent run has already re-claimed and re-broadcast (that would null the freshpayoutTxIdand re-open the order for a second broadcast). Losing the claim skips the retry. Since both retry branches now end in the identical release, the rollback is hoisted behind the OOG/expired distinction (only the log lines differ; bothwarn, per the logging convention for retries).Released hashes stay reconstructable from the DB: a new append-only column
releasedPayoutTxIds(migration included) records every hash the release replaces, written in the same conditional UPDATE. When a retry's dispatch later fails ambiguously and the order parks inPayoutUncertainwithpayoutTxId = null, the investigation still has the full list of previously broadcast hashes — the primary evidence for "did a vanished tx confirm after all?" (a mempool-vanished tx can be re-gossiped and mine later). The column is exposed via the debug allowlist.The 1 h cooldown in
canRetryFailedPayoutand the speedup path are unchanged.Tests
PayoutDesignated(fail-closed, no rollback save) instead of silently loopingaffected: 0) → no re-entry, no dispatch, no save (concurrent-run skip)payoutTxId);-separated), asserted on the exact update payload