fix(payin): designate EVM sends before broadcasting (double-send crash window)#4249
Merged
TaprootFreak merged 10 commits intoJul 17, 2026
Merged
Conversation
…h window) The EVM forwarding/return path broadcast first and persisted afterwards; the minute cron re-selects on status + outTxId IS NULL, so a crash between send and save re-broadcast the whole group. Mirrors the payout designation fix: a persisted Sending marker is written per group member before the broadcast, provably pre-broadcast errors restore the captured statuses for auto-retry, ambiguous broadcast failures keep Sending, and a cron escalation moves stranded entries to SendUncertain with one monitoring mail. The EIP-7702 delegation path gets the same barrier plus a proper broadcast boundary around the relayer send. No migration needed (status column is varchar). Part 1 of #4192
…lation Review follow-ups: the restore path now only fires while no tx id was obtained - a plain persistence error after a successful broadcast keeps the group in Sending (and logs the tx id) instead of re-arming it for a second broadcast. The stranded-send escalation is age-gated (ten minutes; the two send crons run concurrently and must not escalate each other's live work) and escalates via atomic conditional transitions instead of stale full-entity saves that could overwrite a concurrently persisted outTxId; the mail lists only actually escalated entries, logs an error and reaches the liquidity recipients. Refund paths reject Sending/SendUncertain pay-ins, the finance log keeps both statuses in its pending set, and the impossible mixed-group narrative in comments/fixtures is gone.
…cleanups Review follow-ups: the in-flight/uncertain guard lives as an exported constant next to CryptoInputSettledStatus and both refund guards use it with a message in the house style; the orphaned sendUncertain() helper (escalation writes conditionally) is removed; spec imports are sorted and the token strategy spec header describes the file again.
Collaborator
Author
|
Review protocol: 3 full review passes (conformity + logic lens each) until zero findings. Pass 1 surfaced a BLOCKER — a plain persistence error after a successful broadcast restored the group's status and re-opened the double-send window — plus two MAJORs (a concurrent second send-cron could escalate the other's live in-flight work via stale full-entity saves; the refund guards did not reject in-flight/uncertain sends). Fixed by guarding the restore on the tx id, moving escalation to age-gated conditional transitions, and adding the refund guards. Pass 2: conformity cleanups (shared guard constant, dead helper removal). Pass 3: clean on both lenses. |
TaprootFreak
marked this pull request as ready for review
July 16, 2026 22:07
1 task
TaprootFreak
added a commit
that referenced
this pull request
Jul 18, 2026
* fix(payin): fail closed on ambiguous non-EVM sends (Monero, Zano, Cardano, Lightning) Ports the designate-before-broadcast boundary from #4249 (EVM) to the non-EVM pay-in send strategies. A failure at or after the broadcast now keeps the pay-in in the non-reselectable SENDING status (escalated to SEND_UNCERTAIN by the cron) instead of re-broadcasting a fresh transfer on the next run, which could pay a customer twice. Adds a shared sendWithBroadcastBoundary helper in SendStrategy used by the bitcoin-based, Monero, Zano, Cardano and Lightning send loops, and makes the Lightning completion lookup fail-safe so a post-broadcast lookup error cannot mask a completed send. Completes the non-EVM part of #4192. * fix(payin): extend the fail-closed boundary to ICP, Solana and Tron sends The ICP, Solana and Tron pay-in send strategies share Cardano's staged PREPARED send loop and carried the same pre-broadcast double-send window; route them through sendWithBroadcastBoundary like the other non-EVM chains. Also sort the PayInRepository import.
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
Closes the broadcast-before-persist double-send on the EVM pay-in forwarding/return path (part 1 of #4192; mirrors the payout fix from #4181).
Why
EvmStrategy.dispatch()(pay-in send) broadcast first and persisted afterwards. The minute-cron re-selects onstatus IN (...) AND outTxId IS NULL, so a process death between the send and the save left the group re-selectable — the next run broadcast a second time. One group can carry several pay-ins (batched by source/destination/asset), so a single crash re-sent the whole batch. The EIP-7702 delegation path (evm.token.strategy.ts) had the same window and additionally never persisted any pre-send marker.How
Mirrors #4181:
PayInStatusvalues:Sending(persisted in-flight marker, written per group member BEFORE the broadcast) andSendUncertain(terminal for the crons; manual investigation). No DB migration needed — the status column is a plain varchar.dispatch()and the delegation path persistSendingfirst, then broadcast inside a boundary-aware try/catch:TxBroadcastError(the send call was reached; the tx may be in flight — the EVM client boundary from fix(payout): prevent double payouts via designate-before-broadcast + structural broadcast boundary (all chains) #4181, and a new equivalent boundary around the delegation relayer send) or any error after a tx id was already obtained (post-broadcast persistence failures): fail closed, keepSending; when the tx id is known but could not be persisted, it is logged so the evidence survives;processFailedOrders): at the end of both send crons,Sendingentries older than ten minutes are stranded (an in-flight dispatch holdsSendingfor seconds; the age gate never matches the other pay-in cron's live work). Each escalates via an atomic conditional transition (update({ id, status: Sending }, { status: SendUncertain })— no stale full-entity saves that could overwrite a concurrentForwarded+outTxId), and ONE monitoring mail (isLiqMail, plus an error log) lists exactly the entries that were escalated. Neither status is selectable by the forward/return/confirmation queries, which is the entire protection.Resolution of a
SendUncertainpay-in is a deliberate operator decision (verify on-chain state first). The refund guards enforce this:returnPayInand the buy-fiat chargeback trigger reject pay-ins inSending/SendUncertain, so a routine chargeback retry cannot re-arm an unresolved send.Sending/SendUncertainalso stay part of the pending-balance set of the finance log.Tests
Sendingon every group member before the broadcast (call-order assertion)TxBroadcastError→ members staySending(no restore), error rethrownSending, tx id loggedSending/SendUncertainpay-ins (payin service + buy-fiat chargeback trigger)