Skip to content

fix(giga): route EVM validation failures to v2 fallback (CON-368)#3753

Open
bdchatham wants to merge 10 commits into
mainfrom
brandon2/con-368-giga-validation-failure-fallback
Open

fix(giga): route EVM validation failures to v2 fallback (CON-368)#3753
bdchatham wants to merge 10 commits into
mainfrom
brandon2/con-368-giga-validation-failure-fallback

Conversation

@bdchatham

@bdchatham bdchatham commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Describe your changes and provide context

Fixes the LastResultsHash divergence between the v2 and giga executors that halted v2 followers on arctic-1 at height 172900033 (CON-368). Full RCA is on the ticket.

Root Cause

On a delivery-time EVM validation failure (fee/nonce/balance), giga stamped a synthetic receipt of GasUsed = intrinsicGas and GasWanted = ethTx.Gas(). The comment justifying the stamp claims v2 reports intrinsic gas on validation failure, and that does not match v2's behavior. v2 rejects these txs in its ante chain, where GasWanted is never assigned (stays 0) and GasUsed is the ante meter's store-gas consumption. LastResultsHash covers exactly (Code, Data, GasWanted, GasUsed), so one such tx forks the hash. Verified against the incident block: v2 reported (5, 0, 25330), giga reported (5, 21000, 21000), and both tuples reconstruct their incident hashes exactly.

The same path carried a second divergence. Giga pre-checked fee+value against balance while v2's ante requires only the fee, so a fee-affordable tx with unaffordable value failed under giga but executed under v2, a Code and AppHash fork rather than a gas-only one.

Fix

Validation failures now return an ErrValidationFailed sentinel and join the existing fallback scheme (balance migration, self-destruct, store iteration). The sync path re-runs the tx through DeliverTxWithResult and OCC re-runs the batch through the v2 OCC scheduler, so the receipt, the nonce bump, and all state effects are v2's by construction rather than emulated. All fallback sites now increment app_giga_fallback_to_v2 with a reason label; the sync abort branch was previously uncounted.

Test Coverage

TestGigaValidation_FailureClassParity_AllModes locks five failure classes (fee-exceeds-balance, value-exceeds-balance, nonce-too-high, nonce-too-low, and the incident's drain-race block shape) across sequential and OCC modes, each asserting receipt-field and LastResultsHash equality against a real v2 run. The pre-existing test that asserted the stamped values is rewritten as TestGigaValidation_FailureReceiptParity, and the insufficient-balance and mixed-block tests are upgraded to full receipt+hash parity. Against the pre-fix code the new tests fail on exactly the incident's field divergence. A harness-only redundant InitGenesis in NewGigaTestWrapper, which skewed ante store-gas by a constant 24 between test contexts, is removed.

Testing performed to validate your change

  • go test ./giga/tests/ green, including the new 10-subtest parity matrix
  • go test ./app/ -run 'TestGiga|TestOCC|TestProcess' green
  • Red-proof: the new tests fail against the pre-fix app.go with GasWanted mismatch (expected 0, got 21000)

Giga's validation-failure path stamped a synthetic receipt
(GasUsed=intrinsicGas, GasWanted=tx.Gas()) justified by a comment
claiming v2 reports intrinsic gas on validation failure. V2 does not:
those failures abort in the ante, where the envelope reports
GasWanted=0 (never assigned pre-ante-success) and GasUsed as the ante
meter's store-gas consumption. The receipts hash into
LastResultsHash, so any block carrying a delivery-time-failed EVM tx
split consensus on a mixed giga/v2 fleet — the arctic-1 halt at
height 172900033.

Validation failures now return an AbortError sentinel, joining the
established fallback doctrine (balance migration, self-destruct,
iterator): the sync path re-runs the single tx via v2 and OCC falls
back batch-wide, so the receipt, nonce bump, and state effects are
v2's by construction. This also closes a second divergence the old
path carried: giga pre-checked fee+value while v2's ante requires
only the fee, so a fee-affordable/value-unaffordable tx failed under
giga but executed under v2 — different Code, not just gas.

The parity test suite now asserts byte-equal receipts and
LastResultsHash on the full failure matrix (fee-exceeds-balance,
value-exceeds-balance, nonce high/low, and the incident's drain-race
block shape) under both GigaSequential and GigaOCC, replacing the
tests that logged gas divergence as expected. NewGigaTestWrapper's
redundant EvmKeeper.InitGenesis is removed: it re-wrote a param with
different bytes than the genesis round-trip (nil slice marshaling to
null instead of []), skewing ante store-gas by a constant 24 between
test contexts — a harness-only artifact, since production param bytes
are AppHash-covered.
@cursor

cursor Bot commented Jul 13, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Changes consensus-critical tx delivery receipts and LastResultsHash on mixed giga/v2 fleets; incorrect fallback would fork validators.

Overview
Fixes LastResultsHash divergence between giga and v2 when EVM txs fail fee/nonce/balance checks at delivery (CON-368). Giga no longer returns a locally built failure receipt (intrinsic GasUsed / tx GasWanted and optional nonce bump); it returns ErrValidationFailed and reuses the existing v2 fallback so DeliverTxWithResult produces the same receipt and nonce behavior as the ante path.

app_giga_fallback_to_v2 now records reason (e.g. validation_failed, store_iterator, balance migration) and scope (tx vs batch); abort sentinels put the reason in response Info instead of a single GigaAbortInfo string. gigaValidationResult drops nonce-bump fields used only for the old synthetic path.

Tests assert full receipt and LastResultsHash parity vs v2 across failure classes and giga sequential/OCC modes; NewGigaTestWrapper drops redundant InitGenesis that skewed test gas.

Reviewed by Cursor Bugbot for commit ee36fc2. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedJul 14, 2026, 3:04 PM

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid, well-tested fix for the CON-368 LastResultsHash consensus divergence: instead of hand-stamping a synthetic receipt on delivery-time EVM validation failures, giga now returns an AbortError sentinel and reuses the existing v2 fallback doctrine, so the failure receipt, nonce bump, and state effects are v2's by construction. The mechanism correctly hooks into both the sync path (ShouldExecutionAbort → DeliverTxWithResult) and OCC (batch-wide fallback via GigaAbortCode), and the added parity tests assert byte-identical receipts + LastResultsHash across executors.

Findings: 0 blocking | 5 non-blocking | 1 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • Verified the wiring: ValidationFallbackAbortError.IsAbortError() → ShouldExecutionAbort() returns true, matching the sync-path check (app.go:1622) and the OCC GigaAbortCode conversion in makeGigaDeliverTx (app.go:2307) — so both executor modes fall back correctly, and validateGigaEVMTx does no state writes before returning, so nothing partial needs discarding. No correctness concerns found.
  • OCC batch-wide fallback DoS surface: a cheap validation-failing tx forces the whole OCC batch to re-run under v2. The PR acknowledges this matches the existing ErrBalanceMigrationRequired profile (cheaper trigger already exists) and is observable via the gigaFallback metric, so it is not a new/worse attack surface — noting for awareness only.
  • Both second-opinion passes produced no findings: codex-review.md reports "No material issues found" (targeted tests couldn't run due to a network-blocked Go 1.25.6 toolchain download), and cursor-review.md is empty.
  • Follow-up already flagged by the author in the PR description (generateDefaultWhitelistedCwCodeHashesForDelegateCall returning a nil slice that marshals to null vs []) is a reasonable latent-fragility note to track separately.
  • 1 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread app/app.go
- Instrument every giga fallback site with a reason attribute on
  app_giga_fallback_to_v2 (the sync abort branch was previously
  uncounted, and the counter had no cause label — operators could not
  tell a validation-failure wave from balance-migration churn).
- Rename the sentinel to ErrValidationFailed: the family names the
  condition, not the remedy.
- Correct the drain-race comment: the OCC fallback re-runs through the
  v2 OCC scheduler, not serial execution.
- Present-tense the validation-branch comment; use ExecutorMode.String()
  and nested subtests per house form; rename the now-misnomered
  ErrorCodeParity test to MixedBlockReceiptParity.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A well-reasoned consensus fix (CON-368): giga now routes delivery-time EVM validation failures to the v2 fallback instead of hand-stamping a divergent receipt, backed by strong receipt/LastResultsHash parity tests. No blockers; a few non-blocking notes about a documented OCC-fallback cost and a doc-comment placement nit.

Findings: 0 blocking | 4 non-blocking | 1 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • OCC amplification (documented by author, flagging for visibility): before this PR a validation failure was handled inline by giga; now any validation-failing EVM tx returns ErrValidationFailed, which under OCC discards the entire EVM batch and re-runs all EVM txs through the v2 OCC scheduler (app/app.go ~1850). Validation failures (nonce-too-high/low, insufficient balance) are common in normal traffic and trivially attacker-constructible, so one such tx per block doubles the EVM execution work for that block. This is the correct tradeoff given giga cannot reproduce v2's ante-abort receipt, and the author accepts it as the same profile as ErrBalanceMigrationRequired — worth monitoring the new app_giga_fallback_to_v2{reason="validation_failed"} / {reason="occ_batch"} metrics after rollout.
  • Rollout hazard is real and well-documented: {old-giga, new-giga} do not agree on the value-exceeds-balance class (old rejects with no fee charged; new charges the fee via v2) — an AppHash fork that SkipLastResultsHashValidation does not mask. There is no activation height; the giga cohort must be upgraded atomically or via a coordinated restart height. PR is correctly labeled app-hash-breaking.
  • Second-opinion passes: OpenAI Codex reported no material issues; the Cursor review file was empty (no output produced).
  • 1 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread app/app.go

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 app/app.go:2025-2036 — In the synchronous giga dispatch loop (ProcessTxsSynchronousGiga, app/app.go), the branch that catches ShouldExecutionAbort(execErr) — which now also catches the new ErrValidationFallback returned by every fee/nonce/balance validation failure — falls back to v2 via DeliverTxWithResult without incrementing utilmetrics.IncrGigaFallbackToV2Counter()/appMetrics.gigaFallback.Add(), unlike the sibling store-iteration fallback branch a few lines above and unlike the OCC path. Since validation failures are now far more common than the pre-existing balance-migration/self-destruct triggers that used this same path, sync-mode fallback rate becomes effectively invisible in the gigaFallback metric.

    Extended reasoning...

    The bug: executeEVMTxWithGigaExecutor (app/app.go:2025-2036) now returns (nil, gigaprecompiles.ErrValidationFallback) for every fee/nonce/balance validation failure, replacing the old code path that stamped a synthetic receipt and returned it inline (no abort, no fallback). In the synchronous dispatch loop (ProcessTxsSynchronousGiga), this comes back as execErr. Two adjacent branches handle abort-like errors very differently:

    • The store-iteration fallbackToV2 branch (~app.go:1611-1618) explicitly calls utilmetrics.IncrGigaFallbackToV2Counter() and appMetrics.gigaFallback.Add(1) before re-running the tx through v2.
    • The if gigautils.ShouldExecutionAbort(execErr) branch (~app.go:1620-1627) — which catches every AbortError, including the new ErrValidationFallback (its IsAbortError() returns true) — calls app.DeliverTxWithResult and continues, incrementing nothing.

    The OCC path (ProcessTXsWithOCCGiga, ~app.go:1847-1849) does increment the metric when a batch falls back for any GigaAbortCode, so OCC and sync giga are now inconsistent with each other for the exact same class of error.

    Why this matters more after this PR: before this change, fee/nonce/balance validation failures never reached the abort branch at all — validation.err was returned directly with a synthetic GasUsed/GasWanted, landing in the normal non-abort result path. This PR intentionally routes all of those failures through ErrValidationFallback to fix the LastResultsHash divergence (CON-368), which is the right fix for correctness — but it also multiplies traffic through the sync abort branch that was already silently uninstrumented for the rarer ErrBalanceMigrationRequired/ErrSelfDestructUnsupported cases. Fee/nonce/balance failures are common in production (nonce races, low-balance senders, fee-market drift), so this is a meaningful increase in blind-spot traffic, not just an edge case.

    Step-by-step proof:

    1. A tx with insufficient balance for fee+value is dispatched in pure sync giga mode (GigaExecutorEnabled && !GigaOCCEnabled).
    2. validateGigaEVMTx detects the insufficient-funds condition and sets validation.err (app.go ~3182-3188).
    3. executeEVMTxWithGigaExecutor sees validation.err != nil and returns (nil, gigaprecompiles.ErrValidationFallback) (app.go:2025-2036).
    4. Back in ProcessTxsSynchronousGiga, execErr is non-nil; gigautils.ShouldExecutionAbort(execErr) returns true because ValidationFallbackAbortError.IsAbortError() returns true (giga/executor/precompiles/failfast.go).
    5. Execution enters the ShouldExecutionAbort branch, calls app.DeliverTxWithResult, and continues — no call to utilmetrics.IncrGigaFallbackToV2Counter() or appMetrics.gigaFallback.Add() occurs anywhere in this branch.
    6. The tx executes correctly under v2 and produces the correct receipt (state/consensus are fine), but the operator-facing gigaFallback metric never reflects that this fallback happened.
    7. Meanwhile, the exact same class of failure occurring during an OCC batch does increment the metric (app.go ~1847-1849), so dashboards built on gigaFallback will undercount sync-mode fallback rate relative to OCC, with no way to distinguish 'no fallbacks happened' from 'fallbacks happened but sync mode doesn't report them.'

    On the PR description: the PR's reviewer notes scope the 'observable via the existing gigaFallback metric' claim specifically to the OCC batch case, which is accurate for OCC. The description isn't wrong, but it also doesn't mention that the sync path lacks equivalent observability — worth calling out since sync giga (ProcessTxsSynchronousGiga) is a real production path (used whenever giga runs without OCC, and also for the single-recipient-EVM-transfer optimization even under OCC mode).

    Suggested fix: add the same two increments (utilmetrics.IncrGigaFallbackToV2Counter() and appMetrics.gigaFallback.Add()) to the ShouldExecutionAbort branch in ProcessTxsSynchronousGiga, matching the sibling fallbackToV2 branch just above it.

    Impact assessment: this is purely an observability/metrics gap. The fallback itself executes correctly — v2 produces the receipt, nonce bump, and all state effects by construction, so there is no correctness, consensus, or crash impact, and LastResultsHash parity (the actual goal of this PR) is unaffected. It's a one-line fix worth making for metric consistency between sync and OCC modes, but it does not block merge.

  • 🟡 app/app.go:3029 — The comment at app/app.go:3029 still says the nonce lookup 'determines if we bump nonce on fee/balance failures', but this PR removed the bumpNonce/currentNonce fields from gigaValidationResult and routed all validation failures to v2 via ErrValidationFallback, which now owns the nonce bump entirely. Please trim the trailing clause (keep 'Check nonce validity') as part of this PR's cleanup.

    Extended reasoning...

    This is a leftover comment that describes behavior this exact PR deleted. Before this change, validateGigaEVMTx computed bumpNonce := txNonce == currentNonce and stashed it on gigaValidationResult so that executeEVMTxWithGigaExecutor could call app.GigaEvmKeeper.SetNonce(...) directly on validation failure. The comment at line 3029 — // Check nonce validity - determines if we bump nonce on fee/balance failures — accurately described that dual purpose of the nonce lookup at the time it was written.

    The diff removes bumpNonce and currentNonce from gigaValidationResult entirely, and replaces the hand-rolled nonce-bump-and-stamp path in executeEVMTxWithGigaExecutor with a single return nil, gigaprecompiles.ErrValidationFallback. Per the new comment a few lines above that call site (app/app.go:2028-2030) and the ValidationFallbackAbortError doc in giga/executor/precompiles/failfast.go, validation failures now always fall back to v2's DeliverTxWithResult, which owns the nonce bump (and the receipt) by construction. validateGigaEVMTx no longer makes any bump-nonce decision — currentNonce is now used purely for the nonce-too-high/too-low/overflow comparisons later in the function.

    Concretely: read line 3029 in isolation post-PR and it tells a future reader that this function decides whether the nonce gets bumped on fee/balance failure. That is false — grep the function body and there is no bumpNonce field or nonce-mutating call anywhere in validateGigaEVMTx; the only nonce write in this whole code path happens inside v2's fallback execution, which this function never touches. A reader chasing "where does the nonce get bumped on failure" would land here, read the comment, and conclude (incorrectly) that this is the decision point — sending them on a wild goose chase instead of straight to the v2 fallback path.

    This is squarely in the PR's own cleanup surface (the PR description explicitly calls out removing "the now-dead nonce-bump plumbing"), and the fix is trivial: change the comment to just // Check nonce validity (or similar), since that clause remains accurate — currentNonce genuinely is used for the nonce-too-high/too-low/overflow checks below.

    This is comment-only with zero effect on behavior, tests, or consensus — nothing breaks if it ships as-is. All three verifiers independently confirmed the same finding with no refutations, so severity is nit.

Comment thread app/test_helpers.go
…pper

Same redundant EvmKeeper.InitGenesis, same divergent param bytes;
caught by Claude Code Review after the sibling's removal.
@bdchatham bdchatham requested review from codchen and philipsu522 July 13, 2026 22:07

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, well-tested fix for the CON-368 LastResultsHash divergence: EVM validation failures now route through the existing v2 fallback doctrine instead of hand-stamping a synthetic receipt, so receipts, nonce bump, and state are v2's by construction. No blockers; two minor non-blocking observations, and the app-hash rollout risk is thoroughly documented in the PR.

Findings: 0 blocking | 4 non-blocking | 1 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • Observability (Codex): the OCC batch-wide fallback always labels app_giga_fallback_to_v2 with reason="occ_batch" (app/app.go:1854), so within OCC mode the metric cannot distinguish a validation-failure wave from balance-migration/self-destruct/precompile churn — the stated per-cause goal only holds on the synchronous path. This is inherent to the batch-wide fallback (individual abort causes are collapsed to GigaAbortCode in evmBatchResult), but if the cause is recoverable you could label with the first aborting result's reason.
  • cursor-review.md was empty — the Cursor second-opinion pass produced no output. codex-review.md contributed the single OCC-label point noted above.
  • Follow-ups the author explicitly deferred (executed-tx gas parity for revert/OOG/EIP-7623, v2 BasicDecorator pre-validation gaps, and the nil-slice vs [] serialization fragility) are reasonable to track separately on CON-368, but note the BasicDecorator gap is the opposite divergence class (giga executes a tx v2 would reject) and remains a latent consensus risk until closed.
  • 1 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread app/app.go
gigaFallbackReason's insertion had split the function from its godoc.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A well-constructed, well-tested fix for the CON-368 LastResultsHash consensus divergence: delivery-time EVM validation failures now return the new ErrValidationFailed abort sentinel and fall back to v2, so receipts/nonce/state are v2's by construction rather than emulated. Code is clean and correctly wired into both the sync and OCC abort paths; the only items of note are non-blocking operational/performance considerations the author already documents.

Findings: 0 blocking | 4 non-blocking | 0 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • OCC batch-wide fallback cost: in ProcessTXsWithOCCGiga a single validation-failure tx now discards the entire giga OCC batch and re-runs the whole block through the v2 scheduler (reason="occ_batch"), whereas the sync path (ProcessTxsSynchronousGiga) falls back per-tx. Delivery-time validation failures are mostly filtered by CheckTx admission, so this should be rare in practice, but a block containing one such tx loses all giga parallelism for that block. Author explicitly accepts this cost profile (same as ErrBalanceMigrationRequired) and it is now observable via the reason label — flagging only for awareness.
  • Rollout risk (documented by author, not a code defect): {old-giga, new-giga} do NOT agree during a rolling upgrade for the value-exceeds-balance class — an AppHash fork that SkipLastResultsHashValidation does not mask. Executor behavior is config-gated with no activation height in this PR, so any live giga fleet must upgrade atomically or via a coordinated restart height. Worth ensuring this is captured in the deployment runbook since it is enforced only by operator discipline.
  • Second-opinion passes produced no findings: codex-review.md reports "No material issues found" and cursor-review.md is empty. No additional issues to merge in.
  • Follow-ups the author defers (tracked on CON-368) are legitimate residual gaps for the rollout gate: executed-tx gas parity (revert/OOG/EIP-7623) has no fallback backstop; v2 BasicDecorator checks (intrinsic-gas floor, MaxInitCodeSize, block MaxGas) that giga does not pre-validate; and generateDefaultWhitelistedCwCodeHashesForDelegateCall returning a nil slice (marshals null vs []). Reasonable to keep out of this PR, but they remain open divergence surfaces.

Comment thread app/app.go Outdated
The bump-on-failure mechanism it described moved to v2 via the
fallback.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A well-scoped consensus-correctness fix (CON-368) that replaces giga's divergent hand-stamped validation-failure receipt with a fallback to v2, making the receipt, nonce bump, and state effects v2's by construction; the implementation is consistent across sync/OCC paths and backed by thorough parity tests. No blocking issues found.

Findings: 0 blocking | 4 non-blocking | 0 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • Second-opinion passes produced no actionable output: Codex reported "No material issues found" (noting it could not run focused tests because the sandbox Go module cache was read-only), and cursor-review.md was empty. Neither surfaced additional findings to merge.
  • AppHash-breaking rollout risk is real and well-documented in the PR body, but worth re-flagging for approvers: executor behavior is config-gated with no activation height, so a rolling {old-giga, new-giga} upgrade forks the AppHash on the value-exceeds-balance class (not masked by SkipLastResultsHashValidation). Ensure the giga cohort is upgraded atomically / at a coordinated restart height, or straggler resync is explicitly accepted.
  • Accepted-but-worth-monitoring cost: in an OCC batch, a single validation-failing tx triggers a batch-wide re-run through the v2 OCC scheduler. A stream of cheap validation-failing txs (e.g. nonce-too-high) could repeatedly force fallback and erode giga's throughput toward v2. The PR notes this mirrors the existing balance-migration trigger (which is strictly cheaper to hit), so it is not a regression — the new per-cause reason label makes it observable, which is good.
  • Follow-ups the author deliberately deferred and tracked on CON-368 are legitimate residual gaps for the rollout gate: executed-tx gas parity (revert/OOG/EIP-7623) has no fallback backstop, and v2 BasicDecorator checks (intrinsic-gas floor, MaxInitCodeSize, block MaxGas) that giga does not pre-validate remain a latent sibling divergence class.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix correctly resolves the CON-368 LastResultsHash divergence by routing giga EVM validation failures to the v2 fallback path (which owns the receipt and nonce bump by construction) instead of hand-stamping a synthetic receipt, and it is backed by strong parity/hash tests. No blockers; one confirmed observability limitation in the OCC fallback labeling and a couple of minor notes.

Findings: 0 blocking | 7 non-blocking | 2 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • Confirmed (Codex P2): the OCC batch-fallback path labels every abort with reason="occ_batch", so validation-failure vs balance-migration vs self-destruct vs precompile-interop causes are indistinguishable there — unlike the sync path, which uses gigaFallbackReason(). This makes the PR description's claim that fallbacks are 'now observable per-cause via the reason label' only true for the sync path. The abort cause is collapsed into GigaAbortCode/Codespace by makeGigaDeliverTx before the caller inspects evmBatchResult; preserving it (e.g. distinct Info/Log per abort type in makeGigaDeliverTx, read back at app/app.go:1844) would restore per-cause visibility. Non-blocking observability gap.
  • Under OCC, a single delivery-time validation failure now triggers a batch-wide re-run through the v2 scheduler. This is a strictly cheaper attacker/griefing trigger than the pre-existing balance-migration fallback (e.g. an insufficient-fee or drain-race tx), and it is a behavior change from the old inline synthetic-receipt path (which had no OCC fallback). The PR acknowledges this cost profile; worth keeping in mind for the rollout gate but not blocking.
  • The Cursor second-opinion review file (cursor-review.md) is empty — that pass produced no output.
  • gigaFallbackReason() returns "other" for any future abort type not in its switch, which will silently mislabel new sentinels; low risk given the small set of abort types.
  • Rollout hazard is real and correctly documented by the author: {old-giga, new-giga} disagree at the AppHash level (not maskable by SkipLastResultsHashValidation) on the value-exceeds-balance class, and there is no activation height — only config gating. Any live giga cohort must be upgraded atomically. Ensure ops follows the PR's stated guidance.
  • 2 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread app/app.go Outdated
Comment thread app/app.go Outdated
Comment thread app/app.go
…tric

The OCC batch fallback labeled every cause occ_batch; the reason label
now carries the aborting tx's cause on both paths, with scope=tx|batch
distinguishing single-tx from batch-wide fallbacks.
Keeps the PLT-327 legacy/OTel cross-check consistent across all three
fallback sites.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctly fixes the v2/giga LastResultsHash divergence (CON-368) by routing delivery-time EVM validation failures to the v2 fallback instead of emulating a receipt; the change is coherent across sync and OCC paths, imports/symbols check out, and the added parity test matrix meaningfully locks the fixed behavior. No blocking issues found.

Findings: 0 blocking | 3 non-blocking | 0 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • Cursor produced no output (cursor-review.md empty) and Codex reported no material issues (codex-review.md); no second-opinion findings to merge. REVIEW_GUIDELINES.md was also empty, so no repo-specific standards were applied.
  • Rollout caveat (already documented in the PR and covered by the app-hash-breaking label): {old-giga, new-giga} do not agree during a rolling upgrade for the value-exceeds-balance class, and SkipLastResultsHashValidation does not mask it. Worth ensuring ops upgrade the giga cohort atomically as the PR description states.
  • gigaFallbackReason and ShouldExecutionAbort both rely on unwrapped concrete error types (type switch / type assertion rather than errors.As); consistent today, but a future wrapped abort error would silently fall through to the "other" label / non-abort handling. Not a current bug.

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.00000% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.01%. Comparing base (9bf4af1) to head (ee36fc2).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
app/app.go 62.22% 17 Missing ⚠️
giga/executor/utils/errors.go 50.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3753      +/-   ##
==========================================
- Coverage   59.90%   59.01%   -0.90%     
==========================================
  Files        2288     2201      -87     
  Lines      189840   180038    -9802     
==========================================
- Hits       113724   106242    -7482     
+ Misses      65968    64452    -1516     
+ Partials    10148     9344     -804     
Flag Coverage Δ
sei-chain-pr 60.04% <62.00%> (?)
sei-db 70.41% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
app/test_helpers.go 67.07% <100.00%> (+0.05%) ⬆️
giga/executor/utils/errors.go 50.00% <50.00%> (ø)
app/app.go 71.56% <62.22%> (+0.80%) ⬆️

... and 88 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread app/app.go Outdated
Comment thread giga/executor/precompiles/failfast.go Outdated
@bdchatham

Copy link
Copy Markdown
Contributor Author

Live mixed-fleet validation: red/green confirmed on harbor.

Fresh chains in eng-fromtherain (3 giga+OCC validators + 1 v2 follower with LastResultsHash validation fully enforced), driving the incident's drain-race shape with an associated sender: two same-sender txs land in one block, the second passes admission against pre-drain state and fails its delivery fee check.

Red (pre-fix rc2 image 8af84bf55), block 2498 tx[1]:

executor code codespace gas_wanted gas_used
giga validator (stamped) 5 (empty) 21000 21000
v2 follower (computed) 5 sdk 0 25018

Follower hit CONSENSUS FAILURE!!! ... wrong Block.Header.LastResultsHash: expected 99D6DD36..., got 533BE7F0... at height 2499 and crash-looped — the arctic-1 incident reproduced in miniature.

Green (this PR's image 53597327b), block 3068 tx[1]: the giga validators produced (5, sdk, 0, 25018) — v2's receipt, including the identical path-dependent store-gas value — /block_results byte-identical between follower and validator, and the enforcing follower advanced past the block with zero restarts.

Notes from the run, matching the PR's design: an unassociated sender never reaches the validation path (balance-migration fallback masks it), so the harness pre-associates the sender — the incident sender was an active account. The pre-fix stamp also leaves codespace empty where v2 writes sdk (not hash-covered, but visible in block_results).

Validation infra: harbor-engineering-workspace engineers/fromtherain/con368-fix-validation/ (remove with CON-368).

…onstants

The sentinel is not precompile-related; it joins the abort plumbing in
executor/utils. Metric reason labels get named constants shared across
all sites.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A well-tested, correctly-scoped fix for the CON-368 LastResultsHash divergence: EVM validation failures now return an ErrValidationFailed sentinel and join the existing v2 fallback scheme instead of emulating v2's receipt, so receipts, nonce bumps, and state effects are v2's by construction. No code blockers found; only procedural notes about the second-opinion tool outputs.

Findings: 0 blocking | 4 non-blocking | 0 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • REVIEW_GUIDELINES.md (pulled from the base branch) is empty, so no repo-specific standards could be applied in this review.
  • The Cursor second-opinion review (cursor-review.md) is empty — that pass produced no output. Codex reported "No material issues found in the reviewed diff."
  • Optional: the OCC batch-scope reason label is taken from the first aborting result's Info and, on a mixed-cause batch, hides secondary abort reasons. Cardinality stays bounded (Info is always a known constant on sentinel responses), so this is only a metric-fidelity nit, not a correctness issue.
  • Consider a short comment on the ErrValidationFailed singleton noting it is a stateless sentinel intentionally compared by concrete type in gigaFallbackReason (type switch), not via errors.Is, to prevent a future refactor from wrapping it and silently breaking the reason label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants