foundry-rs / foundry-rs/foundry

forge script --resume: positional receipt cursor can treat a reverted tx as complete and skip an already-mined later one

Open
#16,566 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
10.6k
Forks
2.6k
Avg merge
18h 20m
Merged PRs (30d)
510

Description

### Component

Forge

### What command(s) is the bug in?

forge script --resume

### Describe the bug

`--resume` tracks progress with a **positional cursor** — it assumes the completed transactions in a sequence are always a contiguous prefix — but a reverted transaction breaks that assumption, and the maintainers' own code comment already flags this as an open question.

**The cursor:**

```rust
// crates/script/src/broadcast.rs:454
let already_broadcasted = sequence.receipts.len();
```

and the pinned test makes the assumption explicit:

```rust
// crates/script/src/broadcast.rs:1535, remaining_transactions_skip_receipt_prefix
let mut sequence = ScriptSequence { transactions: [completed, second, third], receipts: vec![receipt()], .. };
assert_eq!(remaining_transactions(&sequence)..., vec![second, third]); // prefix semantics
```

**The hole:** `crates/script/src/progress.rs:266-271` removes a *reverted* transaction from `pending` without adding a receipt:

```rust
Ok(TxStatus::Revert(receipt)) => {
// consider:
// if this is not removed from pending, then the script becomes
// un-resumable. Is this desirable on reverts?
warn!(tx_hash=?tx_hash, "Transaction Failure");
deployment_sequence.remove_pending(receipt.transaction_hash());
...
```

That comment is the maintainers' own doubt about this exact behavior. If a **later** transaction in the same sequence still mines successfully (and does add a receipt), `receipts.len()` no longer corresponds to "everything before this index is done" — the reverted transaction leaves a hole *in the middle* of the sequence, not at the end.

**Consequence on `--resume`:** the reverted transaction is treated as already-completed (the cursor just counts receipts), so it's skipped, while a transaction that already mined can be re-sent — which fails `nonce too low`, retries a few times, then hard-fails. `--resume` is effectively unusable for that deployment. This matters because `--resume` is Foundry's own recommended recovery path (see the "Add information about using --resume if necessary" branch a few lines below the snippet above).

Two existing tests currently pin the prefix assumption as correct behavior — `remaining_transactions_skip_receipt_prefix` and `remaining_unsigned_transactions_skip_completed_transactions` (both in `broadcast.rs`) — so a fix would need to update both, not just the cursor logic.

**A viable fix direction already exists in the codebase**: transactions are hash-stamped when sent (`add_pending` records each transaction's hash), and `format_receipt` elsewhere already matches receipts to transactions **by hash**, not position. Matching completion by hash instead of `receipts.len()` would use data that's already being recorded.

### Reachability — tested live, and it's narrower than it first looks

I built a small repro (4 broadcasts, #2 reverts, low-level `.call()` so the revert doesn't abort the script) and ran `forge script --broadcast --skip-simulation` against a local anvil.

**It did not reproduce as "any reverting transaction."** Foundry's own gas-estimation step catches most reverts *before* the transaction is ever broadcast — the script stops there entirely, so later transactions never get a chance to mine, and the completed set stays a valid prefix. The positional cursor handles that case correctly.

So the real trigger needs a transaction that **passes gas estimation but still reverts when mined** — an estimate-vs-mine state race (state changed between the two, or a block/timestamp-dependent condition). That's real, but narrower than the mechanism alone suggests, and I want to be upfront about that rather than imply I have a one-shot repro.

A second path looks like a more likely real-world trigger, though I haven't driven it live: `PendingReceiptError` at `progress.rs:227` silently drops a pending transaction with **no error surfaced at all**, and execution can still reach `"ONCHAIN EXECUTION COMPLETE & SUCCESSFUL."` This could plausibly happen from a dropped/replaced transaction (e.g. a resubmission at higher gas replacing a stuck one) without needing the harder-to-hit estimate-vs-mine race — but I didn't reproduce it (it needs a dropped/replaced tx against anvil, which is fiddly to set up), so treat it as a lead, not a confirmed second repro.

### Suggested direction

Match completed transactions to receipts by transaction hash (data already recorded via `add_pending`/`format_receipt`) rather than by position/count, so a mid-sequence hole doesn't misalign the cursor. Happy to attempt a PR once there's agreement on approach, given it touches two existing tests' pinned semantics.

Contributor guide

Open the contributing guide

Research direction

Start in crates/script/src/broadcast.rs at the positional receipt cursor and remaining_transactions tests, then read crates/script/src/progress.rs around reverted and pending receipt handling. Run remaining_transactions_skip_receipt_prefix and remaining_unsigned_transactions_skip_completed_transactions first. Done means resume progress remains correct when a sequence has a mid-sequence missing receipt, with both pinned tests updated to the agreed semantics.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
blockchain, cli, tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.