fix(giga): fall back to v2 on execution errors#3768
Conversation
Co-authored-by: Philip Su <philip.su.522@gmail.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3768 +/- ##
==========================================
- Coverage 59.98% 59.02% -0.97%
==========================================
Files 2291 2201 -90
Lines 190730 180267 -10463
==========================================
- Hits 114411 106404 -8007
+ Misses 66114 64506 -1608
+ Partials 10205 9357 -848
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
bdchatham
left a comment
There was a problem hiding this comment.
This is a live bugfix, not just defense: the new test fails on main at receipt parity (giga stamps TxType=2, gasUsed=gasLimit; v2 writes the EndBlock synthetic receipt with zeros) while consensus fields match. State the red-proof in the description so the severity (RPC-level receipt divergence on mixed fleets) is on record.
Co-authored-by: FromTheRain <bdchatham@users.noreply.github.com>
Live mixed-fleet validation on harborValidated this PR end-to-end on an ephemeral harbor chain: 4 validators + one v2 RPC node ( Probe: EIP-7623 floor-data-gas tx (1000 zero calldata bytes, gasLimit 27500 — clears the ante's intrinsic check, fails inside geth's Pre-fix giga node — divergence reproduced. Same tx hash,
Consensus held throughout (nonce, sender balance, and fee charge matched v2 on both executors) — receipt-only divergence, invisible to the AppHash/LastResultsHash gate, per the RCA. This PR's image — fixed. Giga node on
The qa-testing spec now guards this class continuously: it locks the canonical v2 receipt shape at the RPC surface, so a regression on any executor fails it regardless of which node serves the query. |
|
Live A/B validation on harbor (ephemeral chains, 4 v2 validators + 1 RPC follower with
Same spec, image, topology, and platform on both legs; only the seid commit differs. Confirms the divergence reproduces on a live giga-serving node and that this PR resolves it to v2 canon. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
This PR correctly fixes a real receipt-parity divergence by routing Giga state-transition errors (e.g. EIP-7623 floor-data-gas) through the existing v2 fallback instead of duplicating v2's fee/nonce/receipt/deferred-info handling. The change is safe, consistent with existing abort-fallback patterns, and adds a strong cross-mode (v2 / Giga-sequential / Giga-OCC) parity test; the only notable point is the operational tradeoff of whole-batch OCC fallback, which the author already documents.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Both second-opinion review files were empty:
codex-review.md(OpenAI Codex) andcursor-review.md(Cursor) produced no output, so this synthesis reflects only the direct review. - Correctness is sound:
ExecutionFailedAbortErrorimplementsIsAbortError(), soShouldExecutionAbortrecognizes it via thevm.AbortErrorcast, and both the sequential (line ~1627) and OCC (line ~2330) callers already handle abort errors by re-running the tx through v2. SincestateDB.Finalize()is not called before the early return,defer stateDB.Cleanup()discards the pre-charged-fee cache — matching the existing self-destruct fallback precedent, so no stale giga state leaks into the v2 re-run. - Net simplification: removes ~60 lines of hand-rolled v2-mirroring logic (nonce bump, Finalize, WriteReceipt, AppendToEvmTxDeferredInfo) that had to be kept in lockstep with v2, replacing it with a single authoritative fallback. This is a maintainability win and eliminates a class of future drift bugs.
- The rewritten
TestGiga_FailedExecutionFallsBackToV2is a meaningful upgrade — it asserts parity on deterministic ABCI fields, LastResultsHash, nonce, receipt, and sender/fee-collector/coinbase balances across v2, Giga-sequential, and Giga-OCC — but it only covers the EIP-7623 floor-data-gas trigger. Consider a follow-up covering at least one other Execute()-path failure to guard the comment's claim that 'other Execute() failure classes are pre-checked upstream.' - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // fee, nonce, receipt, and ABCI result behavior lives in v2. The | ||
| // stateDB has not been finalized, so Cleanup discards its | ||
| // transaction-local cache before the caller re-runs the tx through v2. | ||
| return nil, gigautils.ErrExecutionFailed |
There was a problem hiding this comment.
[suggestion] Correctness here is fine — falling back to v2 for any execErr is safe since v2 is authoritative. Worth flagging the performance/DoS tradeoff the PR body already notes: in OCC mode a single execution_failed abort reverts the entire EVM batch to v2 (scope="batch" at app.go:1861). This is the first deterministic single-tx trigger (passes CheckTx at ~27.5k gas but fails floor-data-gas) that can force a full batch fallback, so an attacker could cheaply negate Giga's parallelism by seeding batches with such txs. The author acknowledges this and defers per-tx OCC fallback to follow-up; please ensure the app_giga_fallback_to_v2{reason="execution_failed"} alert is actually wired up before/at rollout.
| GasWanted: int64(ethTx.Gas()), //nolint:gosec | ||
| Log: fmt.Sprintf("giga executor apply message error: %v", execErr), | ||
| }, nil | ||
| // EIP-7623's floor-data-gas check is the known natural failure here: |
There was a problem hiding this comment.
[nit] nit: the comment asserts "other Execute() failure classes are pre-checked upstream" so EIP-7623 floor-data-gas is "the known natural failure here." The fallback is safe regardless of which class triggers it, but if this assumption ever drifts (e.g. a new geth Execute() error path that upstream checks miss), the only signal will be an uptick in the fallback metric. Consider logging the underlying execErr at debug level before returning so unexpected triggers are diagnosable.
Describe your changes and provide context
This fixes a live RPC-level divergence, not only a defensive edge case. On
main, the parity test reaches matching consensus fields but fails receipt parity: Giga stampsTxType=2andgasUsed=gasLimit, while V2 writes an EndBlock synthetic receipt with zero values. Mixed Giga/V2 fleets can therefore return different receipts for the same transaction.State-transition errors now route through the existing V2 fallback instead of duplicating V2's fee, nonce, receipt, deferred-info, and ABCI result handling. This adds an
execution_failedabort sentinel and fallback metric reason. Because the Giga StateDB has not been finalized whenExecuteTransactionFeeChargedreturns an error, its transaction-local cache is discarded before V2 re-runs the transaction. Both sequential and OCC Giga paths use the existing fallback plumbing.The EIP-7623 floor-data-gas parity test covers deterministic ABCI fields, LastResultsHash, nonce, receipt, sender balance, fee-collector balance, and per-transaction coinbase balance in sequential and OCC modes.
Operational tradeoff: this creates the first deterministic single-transaction trigger that passes CheckTx at roughly 27.5k gas but reverts an entire OCC batch to V2. Operators should alert on
app_giga_fallback_to_v2{reason="execution_failed"}; per-transaction OCC fallback should be tracked as follow-up work.Testing performed to validate your change
go test ./giga/tests -run '^TestGiga_FailedExecutionFallsBackToV2$' -count=1go test ./giga/tests -count=1go test ./app -run 'TestGiga|TestOCC|TestProcess' -count=1gofmt -sandgoimportson all changed Go filesSlack Thread