[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.27% -0.89%
==========================================
Files 2262 2176 -86
Lines 187009 176783 -10226
==========================================
- Hits 110643 103024 -7619
+ Misses 66430 64710 -1720
+ Partials 9936 9049 -887
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 Execution runs through go-ethereum When Reviewed by Cursor Bugbot for commit f1fa418. 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
| Value: newValue, | ||
| Delete: newValue == (common.Hash{}), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Self-destruct omits unstaged storage
High Severity
When a contract is self-destructed or recreated via CreateAccount, Finalise clears only the in-memory storage map. ChangeSet compares storage using keys from base.Storage, which is populated lazily via ensureStorage. Slots that still exist in the persistent StateReader but were never read or written in that block never appear in the changeset, so ApplyChangeSet leaves orphaned storage at that address.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5e36ab5. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A new, self-contained giga/evmonly package adds an SDK-free EVM-only block executor with a go-ethereum-backed vm.StateDB, changeset output, receipts, and extensive tests. It is not wired into the production app path, but contains two genuine EVM-semantics correctness bugs (EXTCODEHASH for existing code-less accounts, and storage clearing on legacy SELFDESTRUCT/SetStorage) plus a notable per-snapshot deep-copy performance cost that should be addressed before this becomes the live giga path.
Findings: 2 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- cursor-review.md was empty — the Cursor second-opinion pass produced no output, so only this review and Codex's findings were merged.
GetStorageRootalways returns the zero hash, so the CREATE/CREATE2 collision check (nonce != 0 || codeHash != EmptyCodeHash || storageRoot != EmptyRootHash) can miss collisions against a pre-existing account that has storage but no code/nonce. Combined with the GetCodeHash issue this narrows collision detection to nonce-only for storage-only accounts. Edge case, but worth documenting alongside the known limitations.- EIP-158/161 empty-account deletion is not implemented in
Finalise(touched-but-empty accounts are not pruned). For the value-based changeset this is mostly benign since no diff is recorded, but it diverges from go-ethereum StateDB semantics and is worth a note in the README limitations. - Consider adding tests that exercise the two correctness gaps above: EXTCODEHASH against a funded code-less EOA (expect EmptyCodeHash, not 0x0), and a legacy SELFDESTRUCT of a pre-existing contract whose persisted storage slots were never read in-block (expect deletes in the changeset).
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| func (s *nativeStateDB) GetCodeHash(addr common.Address) common.Hash { | ||
| code := s.GetCode(addr) | ||
| if len(code) == 0 { | ||
| return common.Hash{} |
There was a problem hiding this comment.
[blocker] EXTCODEHASH semantics bug: this returns common.Hash{} (0x0) whenever code is empty, conflating "account does not exist" with "account exists but has no code". go-ethereum's StateDB returns types.EmptyCodeHash (keccak256 of empty) for an existing account with no code and only common.Hash{} for a non-existent account. As written, EXTCODEHASH on a funded/active EOA returns 0x0 instead of the empty-code hash, so contracts observe incorrect account existence/code state. Distinguish existence (e.g. via Exist(addr)) and return types.EmptyCodeHash for existing code-less accounts.
| Delete: len(acct.Code) == 0, | ||
| }) | ||
| } | ||
| storageKeys := storageKeyUnion(base.Storage, acct.Storage) |
There was a problem hiding this comment.
[blocker] The changeset only diffs storage keys present in base.Storage/acct.Storage, and base.Storage is populated lazily only for slots read via ensureStorage. When Finalise clears a self-destructed account (legacy SELFDESTRUCT) or SetStorage resets an account, persisted slots that were never read in-block are in neither map, so no delete is emitted for them. Applying the changeset to durable state then leaves stale storage behind. Because the backend is key-addressable with no range reads, the executor cannot enumerate those slots — this needs an explicit account-clear/delete signal in the changeset (or a documented constraint) rather than per-slot diffs.
| return nil | ||
| } | ||
|
|
||
| func (s *nativeStateDB) Snapshot() int { |
There was a problem hiding this comment.
[suggestion] Snapshot() deep-copies the entire accounts, base, logs, accessList, transientStates, and preimages on every call, and the EVM snapshots on every CALL/CREATE/STATICCALL. This is O(touched-state) per snapshot and will be a severe performance cliff for any non-trivial contract execution. Acceptable for the sequential placeholder, but flag it as a known cost — a journal/undo-log approach (as in go-ethereum) will be needed before this is the live giga path.
There was a problem hiding this comment.
This new, isolated giga/evmonly EVM-only executor is well-structured and well-tested, but contains correctness bugs that diverge from go-ethereum semantics: stale committed-state reads across transactions, an OCC validation gap on per-tx gas reservation, and incorrect EXTCODEHASH for empty-but-existing accounts. The package is not yet wired into app.go (non-app-hash-breaking), so these are not yet consensus-affecting, but must be fixed before integration.
Findings: 2 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- cursor-review.md is empty — the Cursor second-opinion pass produced no output, so its findings could not be merged.
- Performance/observability: in
executeBlockOCC, every transaction that pays a non-zero fee writes the coinbase balance, so the read/write conflict index will mark a coinbase conflict on virtually every multi-tx block and force a full fallback to sequential execution. With real (non-zero) base fees / tips this defeats the OCC fast path entirely. Consider special-casing coinbase fee accrual (e.g. commutative balance additions) and/or logging when OCC falls back so the regression is observable. - The exported
SetTxContext(hash, index)(state_db.go:519) does not settxIndexUint, while the executor relies on the unexportedsetTxContext(hash, index, indexUint). The exported method is part of thevm.StateDBsurface; if any future caller uses it, logs will get a stale/zeroTxIndex. Worth unifying or documenting why two variants exist. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| func (s *nativeStateDB) GetCommittedState(addr common.Address, key common.Hash) common.Hash { | ||
| s.markRead(stateAccessKey{kind: stateAccessStorage, address: addr, slot: key}) | ||
| s.ensureStorage(addr, key) | ||
| return s.baseAccount(addr).Storage[key] |
There was a problem hiding this comment.
[blocker] GetCommittedState always reads from base, which is the pre-block snapshot and is never advanced between transactions (Finalise does not update base). go-ethereum's GetCommittedState must return the slot value as of the start of the current transaction — i.e. it must reflect writes committed by earlier transactions in the same block (geth checks pendingStorage before originStorage). As written, a later transaction's SSTORE net-gas/refund accounting (EIP-2200/3529) uses the block-start value instead of the current-tx-start value, producing incorrect gas used, receipts, and potentially divergent results when two transactions in a block touch the same slot. Add a per-transaction "committed" layer that is advanced on Finalise, rather than reading the block-base directly.
| stateDB.enableAccessTracking() | ||
| evm := vm.NewEVM(blockCtx, stateDB, chainConfig, vm.Config{}, nil) | ||
| stateDB.SetEVM(evm) | ||
| gasPool := new(core.GasPool).AddGas(gasLimit) |
There was a problem hiding this comment.
[blocker] Each speculative transaction is given a fresh full-block gas pool (AddGas(gasLimit)), and validateOCCResults only checks cumulative actual gasUsed against the block gas limit (occ.go:137). Sequential execution instead reserves each tx's GasLimit from a shared pool up front (gp.SubGas(msg.GasLimit) in ApplyMessage). These disagree: e.g. two non-conflicting transfers each with GasLimit=90k in a 100k block each use ~21k, so OCC validation passes (42k ≤ 100k) and commits a block that sequential execution would reject with ErrGasLimitReached after the first tx reserves 90k. OCC must reproduce the sequential gas-reservation semantics (track cumulative reserved msg.GasLimit, not just used gas) or it can accept invalid blocks.
| func (s *nativeStateDB) GetCodeHash(addr common.Address) common.Hash { | ||
| code := s.GetCode(addr) | ||
| if len(code) == 0 { | ||
| return common.Hash{} |
There was a problem hiding this comment.
[suggestion] GetCodeHash returns the zero hash for every account with empty code. Ethereum semantics require zero only for non-existent accounts; an account that exists (non-zero nonce or balance) but has no code must return types.EmptyCodeHash (keccak256 of empty). As written, EXTCODEHASH on a funded codeless account returns 0 instead of EmptyCodeHash, diverging from geth and the existing x/evm keeper. Return types.EmptyCodeHash for existing-but-codeless accounts and zero only when the account is empty/non-existent.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f1fa418. Configure here.
| }) | ||
| } | ||
| if err := group.Wait(); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
OCC aborts without sequential fallback
High Severity
When OCCWorkers is enabled, speculative transactions each run against the same pre-block state. A later transaction that depends on an earlier one (for example the next nonce from the same sender) can fail during parallel execution, and executeBlockOCC returns that error immediately. Conflict validation falls back to sequential execution, but these execution errors do not, so otherwise valid blocks can fail outright.
Reviewed by Cursor Bugbot for commit f1fa418. Configure here.
| } | ||
| writes.addAll(result.writeSet) | ||
| } | ||
| return true |
There was a problem hiding this comment.
OCC ignores per-transaction gas limits
High Severity
Parallel OCC execution gives every transaction its own GasPool funded with the full block gas limit, while validateOCCResults only checks that the sum of reported gasUsed fits the block limit. go-ethereum’s sequential path also requires each transaction’s gas limit to fit the remaining pool after prior transactions. OCC can therefore accept and merge a block that sequential execution would reject with ErrGasLimitReached.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f1fa418. Configure here.
There was a problem hiding this comment.
A new, isolated giga/evmonly EVM-only execution package with strong test coverage, but it contains a clear EVM-correctness bug in the default (sequential) path — GetCodeHash returns zero for existing code-less accounts — plus two OCC divergences from sequential execution (same-sender nonce blocks error out instead of falling back, and per-tx gas-limit accounting is bypassed). Codex's four findings are all valid; Cursor produced no output.
Findings: 2 blocking | 6 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
./REVIEW_GUIDELINES.mdis empty/missing, so no repo-specific standards were applied../cursor-review.mdis also empty — the Cursor second-opinion pass produced no output; only Codex's review was available to merge.- OCC speculative execution shares
e.state(aStateReader) across worker goroutines.MemoryStateisRWMutex-guarded so it is safe, but theStateReaderinterface contract does not document a thread-safety requirement; a non-concurrent backend would race under OCC. Consider documenting thatStateReadermust be safe for concurrent reads. - No test exercises a block with two transactions from the same sender under OCC, nor a block where two txs declare large gas limits but use little gas — the exact cases that trigger the two OCC divergences below. Adding these would catch regressions.
- Codex notes it could not run
go test ./giga/evmonly/...due to the sandbox being unable to fetch the Go 1.25.6 toolchain; tests were not executed during review. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| func (s *nativeStateDB) GetCodeHash(addr common.Address) common.Hash { | ||
| code := s.GetCode(addr) | ||
| if len(code) == 0 { | ||
| return common.Hash{} |
There was a problem hiding this comment.
[blocker] GetCodeHash returns the zero hash for every account with empty code, but per EIP-1052 EXTCODEHASH (and go-ethereum's StateDB.GetCodeHash) must return zero only for accounts that do not exist / are empty. An account that exists (non-zero balance or nonce) but holds no code should return the empty-code hash (keccak256("")). As written, contracts observing such accounts via EXTCODEHASH see incorrect existence, and this affects the default sequential path too (not just OCC). Gate the zero return on account emptiness and otherwise return the empty-code hash. (Raised by Codex.)
| } | ||
| }) | ||
| } | ||
| if err := group.Wait(); err != nil { |
There was a problem hiding this comment.
[blocker] OCC speculatively executes every tx against the pre-block state, so a second tx from the same sender (nonce N+1) fails core.ErrNonceTooHigh, and a tx depending on a prior create/write hits missing-state errors. Because executeTxSpeculative returns that error and group.Wait() propagates it here as return nil, err, the whole block fails instead of falling back to sequential — even though it is perfectly valid sequentially. Same-sender multi-tx blocks are extremely common, so OCC is effectively unusable for them. Speculative execution errors should trigger the sequential fallback (like validateOCCResults returning false) rather than aborting ExecuteBlock. (Raised by Codex.)
| }, nil | ||
| } | ||
|
|
||
| func validateOCCResults(results []occTxExecution, gasLimit uint64) bool { |
There was a problem hiding this comment.
[suggestion] validateOCCResults only sums actual gasUsed against the block gas limit, but Ethereum's GasPool reserves each tx's full declared gas limit at buyGas time (refunding the unused remainder). Since each speculative tx gets a fresh full-block GasPool, two txs with high gas limits but low gas used can pass OCC validation (sum of gasUsed ≤ limit) while sequential execution would reject the second with core.ErrGasLimitReached. This is a divergence from sequential semantics; the validation needs to model the per-tx gas-limit reservation, not just cumulative gasUsed. (Raised by Codex.)
| if acct.SelfDestructed { | ||
| s.recordAccount(addr) | ||
| acct.Code = nil | ||
| acct.Storage = map[common.Hash]common.Hash{} |
There was a problem hiding this comment.
[suggestion] On legacy (pre-6780) selfdestruct, Finalise clears acct.Storage, but ChangeSet() diffs against base.Storage, which only contains slots lazily read via ensureStorage. Slots that were persisted but never read during the block are absent from base.Storage, so no delete is emitted for them and applying the changeset leaves stale storage behind. A correct legacy-selfdestruct wipe needs range iteration over the account's full storage, which the README states is intentionally out of scope — so at minimum document this as a known limitation alongside the existing ones (it currently only lists BLOCKHASH). (Raised by Codex.)
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