Harden EVM nonce allocation against double-send from pending-recovery re-prepare

[zeljkoX]

Summary

The EVM pending-recovery path can produce two signed, broadcast transactions under two different nonces from a single transaction record. handle_pending_state re-queues a prepare job for any transaction still in Pending after 20s, and nonce allocation in prepare_transaction is a non-atomic read-then-write, so two concurrent prepares for the same transaction id can each allocate a distinct nonce, sign, and submit.

For idempotent operations this is harmless. For transfers and other non-idempotent writes it is a double-spend.

This issue proposes hardening the allocation path so the outcome is structurally impossible rather than merely unlikely.

Why this needs hardening rather than monitoring

The duplicate is created below the transaction record — between tx.id and the chain. Nothing above that layer can observe or prevent it: not a client-side at-most-once marker, not a request-level idempotency key, not a plugin-side guard. Callers integrating against the relayer have no defense available to them, so the guarantee has to be provided by the host.

Current behavior

src/domain/transaction/evm/status.rs:658-670 re-queues prepare with no liveness check:

// Check if transaction is stuck in Pending (prepare job may have failed)
let age = get_age_since_created(&tx)?;
if age > get_evm_pending_recovery_trigger_timeout() {   // 20s
    warn!(..., "transaction stuck in Pending, queuing prepare job");
    self.send_transaction_request_job(&tx).await?;
}

The comment assumes still Pending at 20s ⇒ prepare failed. The invariant that actually holds is still Pending ⇒ prepare has not completed, which also covers "hasn't started", "still queued", and "mid-flight in the signer". Status flips to Sent only at the end of prepare, so it is a lagging signal of job progress, not a liveness signal.

The only thing between that re-queue and a duplicate broadcast is ensure_status(&tx, TransactionStatus::Pending) at src/domain/transaction/evm/evm_transaction.rs:637, evaluated against whatever the record reads at that moment.

How two different nonces arise

src/domain/transaction/evm/evm_transaction.rs:754-790 persists the nonce before signing, while status stays Pending:

let tx_with_nonce = if let Some(existing_nonce) = evm_data.nonce {
    tx                                   // reuse — benign path
} else {
    let new_nonce = self.transaction_counter_service.get_and_increment(...).await?;
    // Save transaction with nonce BEFORE signing
    self.transaction_repository.partial_update(tx.id.clone(), presign_update).await?
};

That branch was added for crash recovery, and incidentally acts as dedup: normally a second prepare sees nonce: Some(n), reuses it, and the duplicate broadcast shares a nonce so only one can mine. The dedup holds only while the read is fresh.

Two different nonces require the second prepare to read nonce: None. Three ways:

1. Queue backlog (widest). The status check fires at 20s while the first prepare is still queued and unstarted. Both prepares later read None and both allocate. Requires only request-queue dwell above 20s — no tight interleaving. The first status check lands at 8s (EVM_STATUS_CHECK_INITIAL_DELAY_SECONDS), so the second is already past the trigger.
2. Mid-flight window. The first prepare is between get_and_increment and the presign_update write — one Redis round trip.
3. Replica lag. get_by_id reads the reader pool (src/repositories/transaction/transaction_redis.rs:1150). With REDIS_READER_URL configured, a prepare can read a pre-presign_update snapshot arbitrarily late. The same hazard was already recognised and fixed elsewhere in that file — see line 1972: "Uses primary to avoid replica lag fabricating false gaps."

Each prepare holds its own in-memory updated_evm_data, so each issues its own postsign_update and its own produce_submit_transaction_job — two submit jobs, two distinct raw transactions, one shared record whose final state is whichever write landed last.

Gap analysis: why existing machinery does not cover this

• Hash recovery cannot fire. should_try_hash_recovery (status.rs:1205-1233) requires status == Submitted and hashes.len() >= 3 and age_of_sent_at >= 2min. A Pending transaction is excluded on the first check, at any delay. There is no competing mechanism that re-prepare is beating — the path is simply unguarded.
• The re-prepare window is 20s–120s, bounded above by should_noop failing the transaction at EVM_PREPARE_TIMEOUT_MINUTES (status.rs:335). Re-prepare fires on every status check inside that window, not once.
• fix: Rewind drifted EVM nonce counter and skip resubmits blocked by nonce gaps #831 does not touch handle_pending_state. fix: Prevent transactions stranded in non-final Redis status indexes #823's sweep is gated at 1 hour, far outside this window.

Related gap in the same function

status.rs:627-628:

For Pending state transactions, nonces are not yet assigned, so we mark as Failed instead of NOOP.

The same invariant, violated the same way by presign_update: a Pending transaction can hold an assigned nonce. When should_noop fires at 2 minutes, this branch marks it Failed and abandons a consumed nonce, leaving a gap for the #831 gap-detection machinery to reconcile after the fact.

Worth fixing in the same change, since it is the same root assumption.

Proposed hardening

1. Make nonce allocation idempotent per tx.id (the substantive fix).

Nonce allocation should be a single atomic operation keyed on the transaction id, enforcing at most one nonce consumption per transaction: if the transaction already has an allocation, return it; otherwise consume the next counter value and record it against the transaction. Executed on the primary, in one step, so there is no window between the check and the write.

TransactionCounterServiceTrait already carries atomic operations added for this class of problem (sync_floor, set_if_equals — src/services/transaction_counter/mod.rs:45-63), so this fits the established pattern.

prepare_transaction then calls the atomic allocation instead of get_and_increment and drops the if let Some(existing_nonce) branch entirely — the atomic operation subsumes it, and unlike that branch it cannot be defeated by a stale read. This closes all three paths at once, and removes the ad-hoc dedup rather than adding another guard alongside it.

The allocation record must carry the nonce value itself rather than a flag, so that a crash between allocation and presign_update resumes with the same nonce instead of wedging the transaction.

2. Guard handle_pending_state (status.rs:658-670): skip the re-queue when nonce.is_some() or !tx.hashes.is_empty(). If prepare progressed far enough to persist a nonce, it did not "fail".

3. Read the prepare handler's transaction from primary, mirroring the precedent at transaction_redis.rs:1972.

4. Fix the Pending→Failed nonce leak: branch on nonce presence — nonce assigned → NOOP at that nonce; no nonce → Failed, as today.

(2) and (3) are not load-bearing once (1) lands, but they eliminate duplicated work and same-nonce duplicate broadcasts, and they make the intent of the recovery path legible.

Explicitly out of scope

Two prepares can still both broadcast after this change, now sharing a nonce — one mines, the other is a harmless already known rejection. Suppressing that entirely requires job-level dedup, which is not judged worth the complexity.

Test plan

• Deterministic concurrency test: two prepare_transaction calls against one transaction id, asserting the counter advanced by exactly one and exactly one nonce was allocated. Property-based, no timing dependence.
• Assert a Pending transaction holding a nonce is NOOPed rather than Failed at the prepare timeout.

Operator note

EVM_PENDING_RECOVERY_TRIGGER_SECONDS (src/constants/evm_transaction.rs:62) is a compile-time const; there are no env::var reads anywhere in that module. Operators cannot currently dial the trigger back or disable it without a rebuild. Worth making configurable as a separate change — though it is not a fix for this issue, since raising it only narrows the 20s–120s window before should_noop fails the transaction.