fix(forge): classify skips by minted cheatcode payload - #16204
Open
mattsse wants to merge 2 commits into
Open
Conversation
mattsse
requested review from
0xrusowsky,
DaniPopes,
figtracer,
grandizzy,
mablr and
stevencartavia
as code owners
August 15, 2026 01:10
Contributor
✅ Changelog foundThe deterministic check will validate the changed entry. |
stevencartavia
previously approved these changes
Aug 15, 2026
Base automatically changed from
revert-16200-mablr/skip-setup-after-revert
to
master
August 15, 2026 14:20
The executor classified a top-level revert as a skip only when the skip cheatcode was the first recorded reverter. Any caught revert earlier in setUp claimed that slot, so a genuine vm.skip bubbled to the top was reported as [FAIL: FOUNDRY::SKIP...]. The skip cheatcode now records the payload it mints in the cheatcode state, and a revert is classified as a skip iff its data byte-equals a recorded payload. Reverter attribution is untouched, and user-crafted FOUNDRY::SKIP revert data still fails because nothing was minted. Fixes #16197
mattsse
force-pushed
the
matt/fix-skip-after-caught-revert
branch
from
August 15, 2026 17:57
1229e54 to
e369717
Compare
mablr
approved these changes
Aug 17, 2026
mablr
left a comment
mablr
left a comment
Member
There was a problem hiding this comment.
SGTM, just an optional suggestion
| cheats.broadcastable_transactions.clear(); | ||
| cheats.ignored_traces.ignored.clear(); | ||
| // Skip payloads are scoped to the call they were minted in. | ||
| cheats.skip_payloads.clear(); |
Member
There was a problem hiding this comment.
Consider pinning this lifetime boundary with an integration test: catch vm.skip(..., "x") in setUp and return successfully, then have the test revert with the exact FOUNDRY::SKIPx bytes. It should fail, proving minted payloads cannot authenticate a later executor call.
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
3 participants
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.
Fixes #16197. The executor only classified a top-level revert as a skip when the skip cheatcode was the first recorded reverter, so any revert caught in
setUpbeforevm.skipclaimed that slot and the genuine skip was reported as[FAIL: FOUNDRY::SKIP...]. The previous fix (#16200) addressed this with opcode-level payload provenance tracking in the inspector stack and was reverted in #16202.This replaces the reverter-identity check with payload identity: the skip cheatcode records each payload it mints in the cheatcode state, result conversion copies the payloads onto
RawCallResult, and a revert is classified as a skip iff its data byte-equals a recorded payload. The newRawCallResult::skip_reasonhelper is shared by the executor error path, fuzz, showmap, and symbolic replay, which previously each paired the reverter check withSkipReason::decodeby hand. Reverter attribution itself is untouched, so first-reverter semantics forshould_ignore_revertand invariant failure attribution do not change, and no per-opcode tracking is involved. User-craftedFOUNDRY::SKIPrevert data still fails the test because no matching payload was minted, including forging different bytes after catching a genuine skip. One semantic is pinned deliberately: a caught genuine skip re-raised byte-identically counts as a skip, since distinguishing manual re-raises from compiler-generated revert bubbling requires exactly the byte-flow tracking #16200 was reverted for, and either way the payload provably originated fromvm.skip(true)in the same call.The issue's reproduction is converted into the
issue_16197repro test (caught revert inside an inherited basesetUpbefore the skip), with further tests covering the minimal caught-revert trigger under--isolate, forged payloads with and without a preceding caught genuine skip, and the identical re-raise.AI assistance was used for the investigation and implementation.