[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 176766 -10243
==========================================
- Hits 110643 103012 -7631
+ Misses 66430 64706 -1724
+ Partials 9936 9048 -888
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 A Reviewed by Cursor Bugbot for commit 10c3108. 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
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.
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 5e36ab5. Configure here.
| 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.
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