[codex] add evm-only giga executor path#3583
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3583 +/- ##
==========================================
- Coverage 59.16% 58.42% -0.74%
==========================================
Files 2262 2180 -82
Lines 187009 177728 -9281
==========================================
- Hits 110643 103843 -6800
+ Misses 66430 64778 -1652
+ Partials 9936 9107 -829
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
codchen
changed the title
PR SummaryMedium Risk Overview The Custom precompiles are fail-closed placeholders; blob txs are rejected until block blob-gas accounting exists; post-London blocks require Reviewed by Cursor Bugbot for commit d555bd4. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
codchen
force-pushed
the
codex/sei-v3-evm-only-scaffold
branch
from
44a10bc to
ef04c0e
Compare
codchen
force-pushed
the
codex/sei-v3-evm-only-scaffold
branch
from
ef04c0e to
cb338ff
Compare
codchen
force-pushed
the
codex/sei-v3-evm-only-scaffold
branch
from
06357b4 to
966d056
Compare
|
|
||
| - sequential execution of the ordered block transaction list | ||
| - RLP decoding and sender recovery through go-ethereum signers | ||
| - go-ethereum `core.ApplyMessage` execution against an SDK-free `vm.StateDB` |
There was a problem hiding this comment.
does "SDK" mean Cosmos?
| for _, acct := range s.accounts { | ||
| if acct.SelfDestructed { | ||
| acct.Code = nil | ||
| acct.Storage = map[common.Hash]common.Hash{} |
There was a problem hiding this comment.
do we need to set acct.Created = false?
There was a problem hiding this comment.
Created is used to decided whether SelfDestruct6780 should early return, and I think the behavior of SelfDestruct6780 is to not early return if selfdestruct is called multiple times, so I think we shouldn't set Created to false upon selfdestruct (consistent with geth)
| if gasLimit == 0 { | ||
| gasLimit = math.MaxUint64 | ||
| } |
There was a problem hiding this comment.
is this dangerous? could someone maliciously set gasLimit = 0 to bypass any limits we may have?
There was a problem hiding this comment.
gasLimit here is the block gas limit, so it'd be decided by consensus and not set by tx senders
| @@ -0,0 +1,653 @@ | |||
| package evmonly | |||
There was a problem hiding this comment.
this is a lot of critical state changing code. i think we need a lot of tests here. Could we use AI to generate a ton of unit cases, interleaving ordering of contract creation/deletion, invalid txs (out of gas, invalid state transition, invalid nonce, verify receipt outputs...etc).
codchen
force-pushed
the
codex/sei-v3-evm-only-scaffold
branch
from
966d056 to
ab82ec3
Compare
codchen
force-pushed
the
codex/sei-v3-evm-only-scaffold
branch
from
ab82ec3 to
23fc6d3
Compare
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-structured, heavily-tested EVM-only giga execution boundary, but the OCC path has a correctness bug: speculative execution runs every tx against base state with nonce checks on, so a valid block containing two consecutive-nonce txs from the same sender aborts with ErrNonceTooHigh instead of falling back to sequential.
Findings: 2 blocking | 3 non-blocking | 1 posted inline
Blockers
- OCC fails valid blocks with same-sender consecutive nonces (Codex P1, confirmed):
executeTxSpeculativeruns each tx against the base state with nonce checks enabled, so a second tx from the same sender (nonce 1 while base nonce is 0) returnscore.ErrNonceTooHigh, which propagates out ofexecuteBlockOCCand aborts the block rather than being treated as a sender-nonce read/write conflict that triggers sequential fallback. This is a very common transaction pattern and is currently untested (both OCC tests use distinct senders all at nonce 0). Fix direction: run speculation with nonce checks skipped (or catch nonce errors) and letvalidateOCCResultsdetect the nonce conflict and fall back. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- OCC will rarely engage under realistic conditions: every non-zero-tip tx credits the block Coinbase balance via ApplyMessage, producing a guaranteed write/write conflict on the coinbase key for any multi-tx block and forcing sequential fallback. The OCC tests only avoid this because they use gasPrice=0 (zero-value AddBalance short-circuits before markWrite). Consider excluding coinbase fee accrual from conflict tracking (e.g. accumulate tips separately) so OCC provides real parallelism.
- Cursor second-opinion review file (cursor-review.md) was empty — that pass produced no output.
- Add an OCC test covering multiple transactions from the same sender (sequential nonces) to lock in correct conflict-detection/fallback behavior; the current gap is what hides the blocker above.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 29047e5. Configure here.
There was a problem hiding this comment.
A well-structured, well-tested new giga/evmonly EVM-only execution boundary, but the opt-in OCC path has a correctness gap: transaction-fee (coinbase) rewards are applied commutatively without being recorded in the OCC write set, so a later transaction that reads the coinbase balance can execute against a stale value and pass validation undetected. Secondary: blob base fee is not validated symmetrically with base fee.
Findings: 1 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Test coverage gap: there is a test asserting fee-paying transfers do NOT conflict on the coinbase (TestExecutorOCCFeePayingTransfersDoNotConflictOnCoinbase), but no test covers a later transaction that reads the coinbase balance (e.g. BALANCE(COINBASE)) after earlier txs credited fees. Adding such a test would surface the stale-read issue and lock in the fix.
- REVIEW_GUIDELINES.md is empty, so no repo-specific standards were applied. cursor-review.md is empty (Cursor produced no output for this pass). Codex's two findings are incorporated (both confirmed).
- BLOCKHASH beyond the parent returns a zero hash (buildBlockContext GetHash). This is documented as a known limitation, but contracts relying on historical BLOCKHASH will observe zero rather than a real/failing value; worth a follow-up before this path is load-bearing.
- nativeStateDB.Finalise ignores its deleteEmptyObjects argument and does not prune EIP-161 empty touched accounts. In practice ChangeSetInto emits no change for a still-empty account so the changeset is unaffected, but the divergence from go-ethereum finalisation semantics is worth a comment or follow-up.
- Executor concurrency relies on the injected StateReader being safe for concurrent reads across speculative goroutines (MemoryState is, via RWMutex). This requirement isn't documented on the StateReader interface — a doc note would prevent an unsafe production backend.
Comments that couldn't be anchored to the diff
giga/evmonly/executor.go:587-- [suggestion]validateBlockContextenforces thatBaseFeeis present for post-London blocks but performs no equivalent check forBlobBaseFeeon Cancun-enabled configs.buildBlockContextpassesBlobBaseFeestraight intovm.BlockContextand blob receipts copy it viacloneOptionalBig(block.BlobBaseFee). The defaultparams.AllDevChainProtocolChangesenables blob txs, so a blob transaction submitted with a nilBlobBaseFeeis not rejected deterministically up front (and risks a nil-deref inside geth's blob fee handling). Suggest validatingBlobBaseFee != nilwhen the chain config is Cancun-active (or when any blob tx is present), symmetrically with the base-fee check.
| if amount == nil || amount.IsZero() { | ||
| return prev | ||
| } | ||
| s.recordAccount(addr) |
There was a problem hiding this comment.
[blocker] OCC correctness gap (confirmed): addCommutativeBalance credits the coinbase fee reward and calls recordAccount, but unlike the normal AddBalance/SubBalance paths it never calls markWrite(stateAccessKey{kind: stateAccessBalance, address: addr}). OCC validation in occ.go (addConflicts) only treats a later transaction's balance read as a conflict when the key is present in a prior transaction's writeSet. Because the commutative fee credit is deliberately kept out of the write set (so fee-paying txs don't serialize on the coinbase), a later transaction that reads the coinbase balance — e.g. BALANCE(COINBASE) and then branches on it — executes speculatively against the pre-block coinbase balance, and validation misses the dependency. mergeOCCResults/mergeChangeSetsInto only reconcile the coinbase's final balance; they cannot fix any storage writes, logs, status, or gas that the later tx decided based on the stale read, so the OCC result can diverge from sequential execution. Since this only affects the opt-in OCCWorkers > 1 path it isn't triggered by default, but it's a consensus-divergence risk on that path. Consider tracking commutative-credit addresses so any later read of that balance forces a conflict/fallback (or otherwise makes the dependency visible to the validator).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds a new, standalone EVM-only giga executor (giga/evmonly) with a native StateDB, deterministic changeset, and an optional OCC path; the code is well-structured and heavily tested. However, the OCC access-tracking has a correctness gap in GetCodeHash that can let OCC diverge from sequential execution, which is a blocker for a path whose whole premise is sequential-equivalence.
Findings: 1 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- cursor-review.md is empty — the Cursor second-opinion pass produced no output, so it was not incorporated.
- REVIEW_GUIDELINES.md is empty/missing on the base branch, so no repo-specific standards were applied beyond AGENTS.md.
- Consider adding an OCC regression test for the read/write-set cross-kind dependency (e.g. tx1 transfers value to a previously-empty address, tx2 does EXTCODEHASH on it, OCC result must equal sequential). No existing test exercises the account-existence dependency of GetCodeHash under OCC.
- Worker-pool cleanup relies partly on runtime.SetFinalizer(e, Close); finalizers are best-effort, so an Executor with OCCWorkers>1 that is never explicitly Closed can leak worker goroutines. Documenting/encouraging explicit Close (or tying lifetime to a context) would be safer.
| } | ||
|
|
||
| func (s *nativeStateDB) GetCodeHash(addr common.Address) common.Hash { | ||
| s.markRead(stateAccessKey{kind: stateAccessCode, address: addr}) |
There was a problem hiding this comment.
[blocker] OCC read-set gap: GetCodeHash returns common.Hash{} for a missing account vs ethtypes.EmptyCodeHash for an existing codeless account, a decision that reads acct.Nonce and acct.Balance (lines 316-319) — but markRead here only records a stateAccessCode access.
In occ.go, stateAccessIndex.conflictsWith matches a code read only against an exact code write, an account-kind write, or a commutative-balance write — never against a plain balance/nonce write. So a block where tx1 transfers value to a previously-empty address X (a balance write) and tx2 later executes EXTCODEHASH on X (this code read) is not detected as a conflict. tx2 is validated against base state where X is missing and commits the stale common.Hash{} instead of EmptyCodeHash, diverging from sequential execution. Since a contract can SSTORE/branch on that hash, the divergence persists into the merged changeset.
Fix: record balance and nonce (or an account-existence) reads here as well, e.g. also markRead the stateAccessBalance and stateAccessNonce keys (or a stateAccessAccount key) whenever the code is empty, so the existence dependency participates in conflict detection. (Matches Codex's High finding.)
There was a problem hiding this comment.
A well-structured, thoroughly-tested new giga/evmonly package that ports the sei-v3-style EVM-only block executor (sequential + OCC paths) behind a Cosmos-free boundary. No blocking correctness bugs found; the notable items are documented validation/trust gaps in a path that is explicitly not yet wired into consensus.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Repo
REVIEW_GUIDELINES.mdis empty/missing, so no repo-specific standards were applied. The Cursor second-opinion review (cursor-review.md) is also empty — that pass produced no output. Only the Codex review contributed external findings (both incorporated below). - OCC correctness looks sound: each speculative tx executes against the shared base state, and
validateOCCResultsconservatively falls back to sequential on any read/write conflict, gas-overflow, storage-clear, or per-block declared-gas overflow. The declared-gas summation is stricter than sequential's GasPool semantics (OCC-commit ⇒ sequential would also succeed with identical gas), so it is safe though occasionally over-conservative. Commutative fee-to-coinbase handling and the sender==coinbase merge path are non-obvious but correct and covered by dedicated tests. - The OCC worker pool relies on
runtime.SetFinalizer(e, (*Executor).Close)to stop its goroutines; callers who never callClose()will leak worker goroutines until GC runs the finalizer. Consider documenting that long-lived callers should callClose()explicitly, or tie pool lifetime to eachexecuteBlockOCCcall. chainConfig()defaults toparams.AllDevChainProtocolChangeswhenConfig.ChainConfigis nil — appropriate for tests but a foot-gun for any production wiring; worth a doc/log warning when the default is used.
Comments that couldn't be anchored to the diff
giga/evmonly/executor.go:491-- [suggestion]ExecutePreparedBlock(and thus this message construction) trusts the exportedPreparedTx.Senderverbatim ascore.Message.FromwithSkipFromEOACheck:false, and never re-recovers it from the signature.PrepareBlockis the sanctioned producer that recovers senders, but becausePreparedBlock/PreparedTxare exported, any caller that builds or mutates aPreparedBlockdirectly can execute a signed tx as an arbitrary funded address. Since this is billed as the "final-form" execution boundary, consider either re-validating the sender against the signature inExecutePreparedBlockor clearly documenting thatPreparedBlockis a trusted, executor-produced type only. (Raised by Codex.)giga/evmonly/executor.go:468-- [suggestion] Blob transactions are executed and emitBlobGasUsed/BlobGasPrice, but the executor never accumulates per-block blob gas nor enforcesparams.MaxBlobGasPerBlock. A block carrying too many blob hashes will pass execution as long as normal EVM gas fits, which violates EIP-4844 block validity. The README notes blob/BLOCKHASH work is incomplete, so this is acceptable for the current scope, but block-level blob-gas enforcement should be added before this path validates real blocks. (Raised by Codex.)
seidroid
Bot
dismissed stale reviews from themself
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Adds a standalone, well-tested EVM-only giga execution package (executor, native StateDB, OCC scheduler, pooling). It is fully isolated with no callers wired in, so nothing here affects live consensus yet; findings are documented limitations and test-coverage gaps rather than blockers.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Blob-tx handling (Codex's finding): the executor decodes and executes
BlobTxTypeand emits blob receipt fields, but does not enforce block-level blob gas accounting,MaxBlobGasPerBlock, or sidecar validation, and never rejects blob txs. This is a known gap explicitly documented in the README's limitations. Because the package is standalone and has no callers (verified — no references outsidegiga/evmonly/), it cannot accept real blocks on-chain today, so this is a future-work correctness gap, not an exploitable bug. Recommend either fail-closed rejecting blob txs until blob accounting is wired, or a clear TODO at the execution site so it isn't accidentally exposed via a caller later. - Test-coverage gap: no test executes a top-level contract-creation (deploy) transaction (all tests use
SetCodedirectly or send to non-nilTo). As a resultCreateContract,GetStorageRoot(returns zero hash — safe only because geth's collision check tolerates the zero hash), and the deploy-code-write path are effectively untested. Adding a CREATE/deploy execution test would harden the most delicate untested StateDB paths. - Second-opinion inputs:
cursor-review.mdwas empty (Cursor produced no output for this PR);codex-review.mdcontributed only the blob-tx finding above.REVIEW_GUIDELINES.mdwas also empty, so no repo-specific standards were applied. - Minor robustness:
nativeStateDB.SubBalancesetss.err = errInsufficientBalanceon underflow, but the executor never checksstateDB.Error()aftercore.ApplyMessage(it only callsFinalise, notCommit). Under correct EVM semantics this path shouldn't trigger, but if it ever did the error would be silently dropped rather than aborting the block. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| if tx.To() == nil { | ||
| receipt.ContractAddress = crypto.CreateAddress(p.Sender, tx.Nonce()) | ||
| } | ||
| if tx.Type() == ethtypes.BlobTxType { |
There was a problem hiding this comment.
[suggestion] Blob txs are decoded and executed here and blob receipt fields (BlobGasUsed, BlobGasPrice) are emitted, but the executor never enforces block-level blob gas accounting, MaxBlobGasPerBlock, or sidecar validation (the README lists these as not-yet-wired). Since this path can produce a valid-looking blob receipt without the accounting that the existing ante/checktx paths require, consider fail-closed rejecting BlobTxType here until blob validation/accounting is implemented, so a future caller can't accept blocks the current chain would reject. Not blocking today because the package has no callers.
| func (p *occWorkerPool) runWorker() { | ||
| for { | ||
| select { | ||
| case <-p.stop: | ||
| return | ||
| case job := <-p.jobs: | ||
| p.runJob(job) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 occWorkerPool.runWorker at occ_pool.go:48-57 can deadlock an in-flight Run() when Close() races with it: if any jobs are still queued in the buffered p.jobs channel when p.stop closes, Go's select picks pseudo-randomly between <-p.stop and <-p.jobs — workers that pick <-p.stop exit without draining, so those jobs' defer done.Done() never fires and Run()'s done.Wait() at occ_pool.go:101 blocks forever. Not triggered by any current test or production path (Executor isn't wired into app.go yet and the runtime.SetFinalizer path cannot fire while ExecuteBlock is on a live goroutine's stack), but worth documenting or fixing before Close() gets a live consumer. Fix by either draining remaining p.jobs after p.stop fires (calling done.Done for each) or documenting that Close must not race with in-flight Run.
Extended reasoning...
What the bug is
occWorkerPool.runWorker at giga/evmonly/occ_pool.go:48-57 uses a two-case select:
for {
select {
case <-p.stop:
return
case job := <-p.jobs:
p.runJob(job)
}
}Go's language spec states that when multiple cases are ready, one is chosen uniformly at random. runJob (line 59-70) is the only code path that calls job.done.Done() (via the defer). If a worker exits through <-p.stop, any jobs it might have received via <-p.jobs are stranded — their done.Done() is never invoked.
The exact race window
- Caller invokes
pool.Run(ctx, ranges, fn)(occ_pool.go:72). Rundispatches N jobs to the bufferedp.jobschannel (buffer sizeworkers*2, so up to 32 for the defaultOCCWorkers=4). The dispatch loop's own<-p.stopguard at line 96 correctly handles Close-before-dispatch-finishes — but that guard stops helping once dispatch completes.Runreachesdone.Wait()at line 101 with all N jobs queued and workers still consuming.- Concurrently,
Close()at line 116 closesp.stop. - Workers now see BOTH cases ready — closed channel receives are always ready AND there are still queued jobs in
p.jobs. Per the Go spec, they pick uniformly at random. - If any worker picks
<-p.stopbefore draining all queued jobs, those undrained jobs'defer done.Done()never fires. Run()at line 101 blocks ondone.Wait()forever.
Close() itself returns cleanly (all workers eventually pick <-p.stop, workerWG completes, p.closed is closed at line 43-44), but the goroutine holding Run is stuck.
Step-by-step proof
Assume workers=4, buffer size 8, ranges produces 8 jobs:
Rundispatches all 8 jobs intop.jobs. Workers may or may not have started consuming; say 4 jobs remain buffered whenRunreachesdone.Wait().Close()is called →close(p.stop)at line 116.- Worker A wakes up. Select sees
<-p.stopready ANDjob := <-p.jobsready. Picks<-p.stop(random). Returns. Job it would have taken remains buffered. - Same for workers B and C. Three of the four remaining jobs are stranded.
- Only worker D picks
<-p.jobs(say), runs that one job, then next iteration also picks<-p.stopand returns. done.Wait()blocks — threedone.Done()calls are missing.Close()returns because all workers exited; the caller ofRunhangs.
A verifier reproduced this experimentally with a targeted test: ~30% deadlock rate (7/30 runs) with workers=4, ranges=8, buffer=8.
Why existing code doesn't catch it
The dispatch loop at line 91-99 already has a <-p.stop guard that correctly bails out and marks unqueued jobs done (done.Done() at line 97). The design was aware of the Close-race — but only for the pre-dispatch case. It doesn't extend to jobs that already made it into p.jobs awaiting a worker.
No current test exercises Close() while Run() is executing:
- The executor's finalizer branch (
runtime.SetFinalizer(e, (*Executor).Close)inexecutor.go) cannot race withExecuteBlock, because the receivereis on that goroutine's stack — the object is reachable, so the finalizer will not fire. - No test calls
Close()mid-block. giga/evmonlyis not wired intoapp/app.go, so no production caller triggers it.
Impact
Zero runtime impact today. But Close() is a public API paired with a long-running ExecuteBlock — the natural graceful-shutdown shape any future integrator will reach for is exactly the racy one (call Close() while a block is still executing). At that point, this becomes a silent hang.
How to fix
Two shapes match:
-
Drain on stop. Replace
runWorkerwith a two-tier select that always prefers pending jobs:func (p *occWorkerPool) runWorker() { for { // Drain queued jobs first, even if stop is closed. select { case job := <-p.jobs: p.runJob(job) continue default: } select { case <-p.stop: // Final drain pass so a job racing into p.jobs after stop // still gets done.Done(). for { select { case job := <-p.jobs: p.runJob(job) default: return } } case job := <-p.jobs: p.runJob(job) } } }
runJobwill observejob.ctx.Err() != nil(Close cancels via jobCtx) and return early afterdefer job.done.Done(), so semantically-cancelled jobs still complete the wait group. -
Document the constraint. Add a comment on
Close()stating that callers must not invoke it whileRun()is in flight. Cheaper, but pushes correctness onto every integrator.
Option 1 is preferable because the constraint is easy to violate accidentally.
| for _, addr := range result.changeSet.StorageClears { | ||
| storageClears[addr] = struct{}{} | ||
| for key := range storage { | ||
| if key.address == addr { | ||
| delete(storage, key) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Nit: the StorageClears loop in mergeChangeSetsInto (occ.go:429-436) is unreachable dead code. validateOCCResults (occ.go:202-207) short-circuits with occFallbackReasonConflict as soon as any speculative result has a non-empty StorageClears slice, and mergeOCCResults is only called when validation.valid == true — so by the time the merge runs, every result.changeSet.StorageClears is guaranteed empty. Either delete the loop (current OCC always falls back to sequential on selfdestruct-heavy blocks) or extend OCC to merge storage clears alongside removing the early bail-out in validateOCCResults.
Extended reasoning...
What the dead code is. mergeChangeSetsInto at giga/evmonly/occ.go:429-436 processes result.changeSet.StorageClears for each speculative tx result — copying addresses into the merged storageClears set and evicting any matching per-slot writes from storage. Grep confirms mergeChangeSetsInto has exactly one caller: mergeOCCResults at occ.go:324.\n\nWhy it can never fire. mergeOCCResults is only invoked when validation passes. executeBlockOCC (giga/evmonly/occ.go:88-92) reads:\n\ngo\nvalidation := validateOCCResults(results, gasLimit)\nif !validation.valid {\n result, err := e.executeBlockSequential(ctx, req)\n ...\n}\nresult, err := e.mergeOCCResults(ctx, results)\n\n\nAnd validateOCCResults at occ.go:202-207 short-circuits before any other check:\n\ngo\nfor _, result := range results {\n if len(result.changeSet.StorageClears) > 0 {\n validation.valid = false\n validation.fallbackReason = occFallbackReasonConflict\n return validation\n }\n ...\n}\n\n\nSo whenever any result has a non-empty StorageClears slice, validation.valid is false and mergeChangeSetsInto is never called on that result set. By construction, every result.changeSet.StorageClears reaching the merge loop is guaranteed empty.\n\nStep-by-step proof.\n1. Tx X selfdestructs contract C. Finalise sets acct.StorageCleared=true (state_db.go:497-514).\n2. ChangeSetInto (state_db.go:180-182) emits ChangeSet.StorageClears = [C] for tx X.\n3. executeBlockOCC runs validateOCCResults on all results.\n4. First iteration finds len(result.changeSet.StorageClears) > 0 → returns valid=false, fallbackReason="conflict".\n5. executeBlockOCC falls back to executeBlockSequential — mergeOCCResults is not called.\n6. The loop at occ.go:429-436 is never entered.\n\nImpact. No runtime bug — the current design intentionally forces sequential execution on any selfdestruct-heavy block, and the loop is defensive code that would only matter if that design changed. But it is genuinely dead: three independent verifier passes all confirmed it. The loop is confusing to readers who assume OCC merges storage clears, and it will bit-rot if the surrounding code changes (e.g. the change.Address filter inside delete(storage, key) is subtle enough that a future maintainer might refactor it without realizing nothing exercises it).\n\nHow to fix. Two directions:\n\n1. Delete the loop (for _, addr := range result.changeSet.StorageClears { ... } at occ.go:429-436) and rely on validateOCCResults as the sole gate. Optionally add a short comment on the validator explaining that selfdestruct blocks force sequential fallback. This matches current behavior exactly.\n2. Extend OCC to actually merge storage clears — remove the early-bail at occ.go:202-207 and let the merge do the work. This is more work and requires reasoning about how a selfdestructed-then-recreated address interacts with per-slot writes from other txs in the block, plus updated conflict tracking so a selfdestruct is a conflict against any other access to the same address.\n\nOption 1 is the simple minimal-scope fix; option 2 is a real feature.
There was a problem hiding this comment.
Adds a self-contained, well-tested giga/evmonly EVM-only execution boundary (native StateDB + OCC speculative path) that is not yet wired into the app and is marked non-app-hash-breaking. No blocking correctness issues found; a few minor documentation/dead-code observations.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion review file (
cursor-review.md) is empty — that pass produced no output. Codex reported no material findings but explicitly could not rungo test ./giga/evmonly/...because its sandbox has Go 1.24.13 and the Go 1.25.6 toolchain download was network-blocked, so its pass was static-only. Worth ensuring the package is exercised under the CI Go 1.25.6 + race detector before this is relied upon. - OCC speculative execution runs multiple txs concurrently, each constructing
newNativeStateDB(e.state)against the same sharedStateReader. This makes concurrent-read safety of any injectedStateReadera hard requirement, but theStateReaderinterface doc instate.godoesn't state it.MemoryStateis safe (RWMutex); production/custom backends must be too — recommend documenting this contract onStateReader/WithState. - Block execution aborts the entire block (returns an error, no
BlockResult) on any per-txApplyMessagefailure (nonce, insufficient funds, intrinsic gas, gas exhaustion) as well as the MinGasPrice check. This is intentional per the code comments (the boundary assumes consensus supplies only valid blocks and treats these as block-validity failures), and is tested, but it's a notable contract to make explicit in the package README/BlockExecutor doc so downstream callers don't expect per-tx failure receipts for these cases. - The executor relies on
runtime.SetFinalizeras a backstop to stop worker-pool goroutines whenClose()isn't called (executor.go:56). This is documented, but finalizer-based cleanup is best-effort; callers usingOCCWorkers > 1should be strongly steered todefer Close(). - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| if tx.To() == nil { | ||
| receipt.ContractAddress = crypto.CreateAddress(p.Sender, tx.Nonce()) | ||
| } | ||
| if tx.Type() == ethtypes.BlobTxType { |
There was a problem hiding this comment.
[nit] This blob-receipt block is currently unreachable: validateSupportedTx rejects BlobTxType both in parseBlockTxs and at the top of executeTx, so no blob tx ever reaches here. Fine as forward-looking scaffolding (the README notes blob support isn't wired), but consider a short comment noting it's intentionally dead until block-level blob gas accounting lands, so it doesn't read as an active path.
There was a problem hiding this comment.
This PR adds an isolated, well-tested EVM-only giga execution package (giga/evmonly) that is not yet wired into app/app.go; the code is thorough and the test coverage (sequential, OCC, self-destruct/6780, receipts, validation failures) is strong. No must-fix blockers for merge given the package is un-wired scaffolding, but there is one latent correctness gap (missing Cancun blob-base-fee validation) and a few notes worth addressing before this path is put into production.
Findings: 0 blocking | 7 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass (
cursor-review.md) produced no output (empty file). - Codex reported it could not run
go test ./giga/evmonly/...because the sandbox could not download the pinned Go 1.25.6 toolchain, so its automated pass did not actually execute the included tests. I likewise could not run them in this environment; a CI run confirming the validation commands in the PR description is recommended. - The PR includes
sei-cosmos/storev2/rootmulti/hash_consistency_test.go, which is unrelated to the statedgiga/evmonlyscope. Confirm this test is intentionally bundled with this change rather than belonging to a separate PR. occChunkSizefloors the chunk size at 16, so any block with fewer than ~16*OCCWorkers transactions runs all speculative execution on a single worker (no real parallelism). Acceptable for scaffolding, but worth revisiting when tuning OCC.buildBlockContext'sGetHashreturns the parent hash only and zero for all other historical lookups (documented in the README). This is a consensus-divergence risk if BLOCKHASH-using contracts run on this path before a real historical hash source is wired.- No prompt-injection or instruction-to-reviewer content was found in the PR diff, description, or commit messages.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| func validateBlockContext(chainConfig *params.ChainConfig, ctx BlockContext) error { | ||
| if chainConfig != nil && chainConfig.IsLondon(new(big.Int).SetUint64(ctx.Number)) && ctx.BaseFee == nil { |
There was a problem hiding this comment.
[suggestion] validateBlockContext requires a base fee for post-London blocks but does not require a blob base fee for Cancun+ blocks. Because a nil ChainConfig defaults to params.AllDevChainProtocolChanges (Cancun active from genesis), buildBlockContext can pass a nil BlobBaseFee to the VM. Blob transactions are rejected by validateSupportedTx, but a normal transaction can still execute the BLOBBASEFEE opcode, which reads evm.Context.BlobBaseFee. With nil this either panics (uint256 conversion of a nil big.Int) or silently returns 0, i.e. a crash / consensus-divergent result. Recommend requiring a non-nil BlobBaseFee here when the chain config reports Cancun active for ctx.Number/ctx.Time (or supplying the same default the existing keeper uses). Matches the Codex finding.
3 participants
Summary
giga/evmonlyas the final-form EVM-only giga execution path boundarygiga/evmonly:vm.StateDBStateChangeSetoutputMemoryStatebackend for tests and early integrationNotes
Custom precompile behavior is intentionally still open. Registered custom precompile addresses return
ErrCustomPrecompilesOpen, including through geth's precompile map, so calls fail closed instead of silently executing as empty accounts.The current port is sequential. The state boundary and changeset shape are intended to be replaceable with the sei-v3 OCC scheduler/store once that layer is brought over.
Historical
BLOCKHASHlookups beyond the parent block are not wired yet;BlockHashis currently used for receipt/log metadata.Validation