[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.30% -0.87%
==========================================
Files 2262 2179 -83
Lines 187009 177181 -9828
==========================================
- Hits 110643 103298 -7345
+ Misses 66430 64809 -1621
+ Partials 9936 9074 -862
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 When Custom precompiles are stubbed: registered addresses map to contracts that return Reviewed by Cursor Bugbot for commit 145f089. 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 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.
| }) | ||
| } | ||
| 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.
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.)
There was a problem hiding this comment.
New giga/evmonly EVM-only executor is well-structured and heavily tested, but the OCC path has correctness/consensus-divergence bugs: it aborts valid blocks on stale-read validation failures, and its block-gas validation and legacy-selfdestruct changeset diverge from sequential execution. These should be fixed before merge.
Findings: 6 blocking | 4 non-blocking | 3 posted inline
Blockers
- OCC diverges from sequential execution on stale reads and can reject valid blocks. Each speculative tx (
executeTxSpeculative) runs against block-initial state, so a later tx that is only valid after an earlier tx in the same block (e.g. a second tx from the same sender with nonce+1, or a tx spending funds received from an earlier tx) failscore.ApplyMessagewithErrNonceTooHigh/ErrInsufficientFunds.executeBlockOCCpropagates that error fromgroup.Wait()and returns it instead of falling back to sequential — so a block that sequential execution accepts is rejected under OCC. Speculative validation failures caused by stale reads must be treated as conflicts (fall back / retry), not as hard block errors. Same-sender multi-tx blocks are extremely common, so this is a serious divergence. - OCC block-gas validation is not equivalent to sequential gas accounting (also raised by Codex). Each speculative tx gets a fresh full-block
GasPool(occ.go:132) andvalidateOCCResultsonly checks cumulative gasUsed ≤ gasLimit. Sequential execution charges each tx's declared gas limit against the shared pool at buyGas time, so a later tx whose declared gas limit exceeds the remaining pool triggersErrGasLimitReachedand aborts the block. OCC can accept such a block, diverging from sequential (which the 'block gas exhausted' test shows aborts). Validation must account for per-tx declared gas limits against the running remaining pool, not just cumulative gas used. - Legacy (pre-EIP-6780) selfdestruct can leave orphaned storage in the persisted changeset (also raised by Codex).
Finaliseclearsacct.Storage(state_db.go:498), butChangeSet()only diffs keys present inbase.Storage, which is populated lazily viaensureStorage. A pre-existing contract whose storage was never read during the block has an emptybase.Storage, so noDeletestorage entries are emitted; applying the changeset to a durable backend leaves the old slots behind. Reachable with a legacy chain config (the tests uselegacySelfDestructChainConfig) but not covered by a test that pre-populates untouched storage. - 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
REVIEW_GUIDELINES.md(from the base branch) is empty/missing, so no repo-specific review standards were applied.- The Cursor second-opinion review (
cursor-review.md) is empty — that pass produced no output. - Test coverage gaps for the confirmed bugs: add OCC tests for (a) multiple txs from the same sender / a tx depending on an earlier tx's output, (b) a block whose cumulative declared gas limits force
ErrGasLimitReachedunder sequential, and (c) legacy selfdestruct of a pre-existing contract that has untouched persisted storage, asserting the changeset emits the storage deletions. - README documents that historical
BLOCKHASHbeyond the parent is not wired andGetHashreturns a zero hash for older blocks; fine as a documented limitation, but worth a follow-up before production use.
| } | ||
| for idx := txRange.start; idx < txRange.end; idx++ { | ||
| result, err := e.executeTxSpeculative(groupCtx, req, idx, signer, chainConfig, blockCtx, baseFee, gasLimit) | ||
| if err != nil { |
There was a problem hiding this comment.
[blocker] Speculative execution runs against block-initial state, so a tx that is only valid after an earlier tx in the same block (same-sender nonce+1, or spending funds received earlier) fails ApplyMessage with ErrNonceTooHigh/ErrInsufficientFunds. Returning that error here propagates through group.Wait() and aborts the whole block (no fallback), rejecting blocks that sequential execution accepts. Stale-read validation failures should be treated as conflicts and fall back to sequential, not returned as block errors.
| 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 tx gets a fresh full-block gas pool, and validateOCCResults only checks cumulative gasUsed ≤ gasLimit. Sequential execution charges each tx's declared gas limit against the shared pool at buyGas time, so a later tx whose declared limit exceeds the remaining pool triggers ErrGasLimitReached and aborts the block. OCC can accept such a block — a divergence from sequential. Validate per-tx declared gas limits against the running remaining pool.
| if acct.SelfDestructed { | ||
| s.recordAccount(addr) | ||
| acct.Code = nil | ||
| acct.Storage = map[common.Hash]common.Hash{} |
There was a problem hiding this comment.
[blocker] Clearing acct.Storage on selfdestruct isn't reflected in ChangeSet() for untouched slots: ChangeSet diffs against base.Storage, which is populated lazily by ensureStorage. A pre-existing contract whose storage was never read this block has empty base.Storage, so no Delete entries are emitted and the old slots survive when the changeset is applied to a durable backend. Reachable under a legacy (pre-6780) chain config.
There was a problem hiding this comment.
New giga/evmonly EVM-only executor is well-structured and well-tested, but has one consensus-level bug in the default sequential path (GetCodeHash for empty-code accounts) plus two correctness divergences in the opt-in OCC path (speculative errors abort the block, and block-gas validation disagrees with sequential geth semantics). No prompt-injection content was found in the diff.
Findings: 3 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
- OCC effectively never engages for real fee-paying blocks: geth credits priority fees to the coinbase on every tx, so every tx's write set includes the coinbase balance and
validateOCCResultsalways detects a conflict, forcing the sequential fallback. The OCC tests only pass because they use zero base fee / zero gas price so no coinbase credit occurs. Worth documenting or addressing (e.g. treat coinbase fee accrual as commutative) so the parallel path provides real benefit. - Test coverage gaps in the OCC path: there is no test for multiple txs from the same sender with sequential nonces (would surface the speculative-abort bug) nor for the block-gas-pool divergence (per-tx gas limits summing under the block limit while the second tx cannot buy its full limit). Adding these would have caught the two OCC findings.
cursor-review.mdwas empty — the Cursor second-opinion pass produced no output.REVIEW_GUIDELINES.mdwas also empty, so no repo-specific guidelines were applied.- Documented limitation (README):
BLOCKHASHonly exposes the parent hash; historical block-hash lookups are unwired. Fine for now but callers must be aware EVMBLOCKHASHbeyond parent returns zero.
| 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 empty-code account. Per EIP-1052, EXTCODEHASH must return keccak256("") (types.EmptyCodeHash) for an account that exists but has no code (e.g. a funded EOA or an existing empty-code account); only truly non-existent accounts return zero. geth's opExtCodeHash already guards with Empty(addr) before calling GetCodeHash, so this method should return crypto.Keccak256Hash(nil) for empty code to mirror geth's StateDB. As written, EXTCODEHASH of a funded EOA yields 0x0 instead of 0xc5d2..., which makes contracts observe incorrect EVM state — a consensus-level divergence on the default sequential path.
| for idx := txRange.start; idx < txRange.end; idx++ { | ||
| result, err := e.executeTxSpeculative(groupCtx, req, idx, chainConfig, blockCtx, baseFee, gasLimit) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
[blocker] Speculative execution errors abort the entire block instead of falling back to sequential. Each tx is executed against pre-block state in isolation, so a common block with two txs from the same sender using nonces 0 and 1 will execute the nonce-1 tx against pre-block nonce 0, hit core.ErrNonceTooHigh, and return here — group.Wait() propagates the error and executeBlockOCC returns (nil, err) before validateOCCResults can trigger the sequential path. Speculative EVM errors that stem from stale reads (nonce, balance, gas) should be treated as conflicts that force re-execution / sequential fallback, not as fatal block-validation failures.
| if result.gasUsed > math.MaxUint64-totalGas { | ||
| return false | ||
| } | ||
| totalGas += result.gasUsed |
There was a problem hiding this comment.
[blocker] OCC validates the block gas limit using cumulative actual gas used, but sequential geth execution enforces the block gas pool against each tx's declared gas limit at buyGas time (the pool must hold the full tx.Gas() before execution, with the unused portion refunded afterward). This can accept a block sequential execution rejects: e.g. two transfers each declaring a 40k gas limit under a 50k block limit — cumulative used is 42k (accepted here), but sequentially the second tx cannot buy 40k after the first consumed 21k, yielding ErrGasLimitReached. Validation should mirror the sequential gas-pool semantics.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| func validateOCCResults(results []occTxExecution, gasLimit uint64) bool { | ||
| writes := newStateAccessIndex() | ||
| var totalGas uint64 | ||
| for _, result := range results { | ||
| if result.gasUsed > math.MaxUint64-totalGas { | ||
| return false | ||
| } | ||
| totalGas += result.gasUsed | ||
| if totalGas > gasLimit { | ||
| return false | ||
| } | ||
| if writes.conflictsWithAny(result.readSet) || writes.conflictsWithAny(result.writeSet) { | ||
| return false | ||
| } | ||
| writes.addAll(result.writeSet) | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
🔴 OCC will fall back to sequential on every realistic block because state_transition.go calls AddBalance(Coinbase, fee) after every fee-paying tx, and nativeStateDB.AddBalance (state_db.go:204-214) marks a write on (stateAccessBalance, coinbase). Since tx N and tx N+1 both write that exact key, validateOCCResults (occ.go:151-168) sees an exact-match conflict on the second tx onward and returns false, so executeBlockOCC degenerates to speculative-work-then-sequential — strictly slower than plain sequential. The existing OCC tests miss this because they pin gasPrice=0 and baseFee=0 so fee=0 and AddBalance short-circuits via amount.IsZero() without marking a write. Fix by accumulating coinbase tips out-of-band (sum once, apply at merge) or excluding the coinbase balance write from the conflict tracker with matching add-not-overwrite semantics in mergeChangeSets.
Extended reasoning...
What the bug is
The OCC executor added in this PR uses per-tx read/write sets to detect conflicts. Every tx that pays a non-zero fee credits the coinbase via AddBalance in go-ethereum's state_transition.go:
// go-ethereum core/state_transition.go:572
st.state.AddBalance(st.evm.Context.Coinbase, fee, tracing.BalanceIncreaseRewardTransactionFee)This call reaches nativeStateDB.AddBalance in giga/evmonly/state_db.go:204-214, which unconditionally marks a write on stateAccessKey{kind: stateAccessBalance, address: coinbase} whenever amount is non-zero (the amount.IsZero() branch at line 206 short-circuits before the markWrite at line 211).
Every fee-paying speculative tx therefore ends up with {stateAccessBalance, coinbase} in its writeSet.
How validation fails
validateOCCResults at giga/evmonly/occ.go:151-168 iterates results in order, using a stateAccessIndex whose conflictsWith checks i.exact[key] first:
func (i *stateAccessIndex) conflictsWith(key stateAccessKey) bool {
if _, ok := i.exact[key]; ok {
return true
}
...
}After tx 0 is added via writes.addAll(tx0.writeSet), writes.exact contains {stateAccessBalance, coinbase}. When tx 1 is checked via writes.conflictsWithAny(tx1.writeSet), its own {stateAccessBalance, coinbase} entry produces an exact-match hit → validateOCCResults returns false → executeBlockOCC falls back to executeBlockSequential (occ.go:91-92).
Step-by-step proof
Consider two non-conflicting transfers in the same block, each paying fees to the same coinbase (as any real Sei block would):
- Both txs are executed speculatively in
executeTxSpeculativeagainst block-initial state. Each succeeds independently. - During each execution,
ApplyMessagecallsst.state.AddBalance(coinbase, fee, ...). - Inside
nativeStateDB.AddBalance:amountis non-zero (positive baseFee OR positive tip), so line 211 recordsmarkWrite({stateAccessBalance, coinbase}). - Both
occTxExecution.writeSetmaps contain{stateAccessBalance, coinbase}. validateOCCResultsprocesses tx 0: no prior writes →conflictsWithAnyreturnsfalse→writes.addAll(tx0.writeSet)inserts{stateAccessBalance, coinbase}intowrites.exact.validateOCCResultsprocesses tx 1:writes.conflictsWithAny(tx1.writeSet)iteratestx1.writeSet, hits the same key{stateAccessBalance, coinbase}inwrites.exact→ returnstrue→ validation fails.executeBlockOCCreturnse.executeBlockSequential(ctx, req).
Result: the block was executed twice (once speculatively in parallel, once sequentially), so OCC is strictly slower than sequential.
Why the tests miss it
Both TestExecutorOCCNonConflictingTransfersMatchSequential and TestExecutorOCCConflictingTransfersMatchSequential set:
Config.MinGasPrice: big.NewInt(0)signLegacyTxWithGasPrice(..., big.NewInt(0))→ txgasPrice = 0blockContext(chainID)→BaseFee: big.NewInt(0)
So fee = (baseFee + effectiveTip) * gasUsed = 0, and AddBalance(coinbase, 0) early-returns at state_db.go:206 without calling markWrite. The tests only exercise a code path that will never occur on mainnet.
Impact
- Correctness: preserved — sequential fallback runs the block correctly.
- Feature: broken — the OCC parallelism this PR introduces (commits
10c3108,dd519f7) is unreachable for any realistic Sei block. SettingOCCWorkers > 1in production would be a strict regression: it pays the full cost of parallel speculative execution, then runs the block sequentially anyway. - Blast radius: scaffold-level.
giga/evmonlyis not wired intoapp/app.goyet, so no runtime path is affected today. But the merge target for this PR ships OCC as a completed feature.
How to fix
The coinbase fee credit is commutative — the final balance is the base value plus the sum of all per-tx fees regardless of order — so it should not participate in conflict detection at all. Two options match the verifiers' recommendations:
- Aggregate fees out-of-band: skip the
AddBalance(coinbase, fee)inside speculative execution (or filter(stateAccessBalance, coinbase)out of the tracked writeSet/readSet), sumfeeacross all txs at validation time, and apply that single credit inmergeChangeSets. - Exclude coinbase-balance access from the tracker with matching add semantics in
mergeChangeSets: todaymergeChangeSetsdoes last-write-wins on balances (occ.go:250-252 overwritesbalances[change.Address]), which would drop earlier tips. If coinbase writes are excluded from conflict tracking, coinbase balance must instead be merged by summing the deltas across per-tx changesets.
Either way, mergeChangeSets currently doing last-write-wins on the coinbase balance is a second bug that only manifests once the first is fixed.
| receipt := ðtypes.Receipt{ | ||
| Type: tx.Type(), | ||
| Status: status, | ||
| Logs: txLogs, | ||
| TxHash: tx.Hash(), | ||
| GasUsed: execResult.UsedGas, | ||
| EffectiveGasPrice: effectiveGasPrice(tx, baseFee), | ||
| BlockHash: block.BlockHash, | ||
| BlockNumber: new(big.Int).SetUint64(block.Number), | ||
| TransactionIndex: txIndexUint, | ||
| } | ||
| if tx.To() == nil { | ||
| receipt.ContractAddress = crypto.CreateAddress(p.Sender, tx.Nonce()) | ||
| } | ||
| receipt.Bloom = ethtypes.CreateBloom(receipt) |
There was a problem hiding this comment.
🟡 The receipt built in executeTx (giga/evmonly/executor.go:209-223) never populates BlobGasUsed or BlobGasPrice, so any Type-3 blob tx executed by this path produces a receipt where eth_getTransactionReceipt returns blobGasUsed=0 and blobGasPrice=null. go-ethereum's core/state_processor.go sets both when tx.Type() == BlobTxType; the fix is a 3-line branch after the receipt literal using params.BlobTxBlobGasPerBlob and block.BlobBaseFee. Not urgent — the executor is not yet wired into app.go and blob txs are still rejected by x/evm/ante/basic.go — but worth catching before this becomes production RPC surface.
Extended reasoning...
What the bug is. In giga/evmonly/executor.go, executeTx constructs *ethtypes.Receipt with Type, Status, Logs, TxHash, GasUsed, EffectiveGasPrice, BlockHash, BlockNumber, TransactionIndex, and (for contract creations) ContractAddress. It does not set BlobGasUsed or BlobGasPrice. In canonical go-ethereum (core/state_processor.go in sei-protocol/go-ethereum@v1.15.7-sei-16, lines 180-183), the block-processing path explicitly sets these for Type-3 transactions:\n\ngo\nif tx.Type() == types.BlobTxType {\n receipt.BlobGasUsed = uint64(len(tx.BlobHashes()) * params.BlobTxBlobGasPerBlob)\n receipt.BlobGasPrice = evm.Context.BlobBaseFee\n}\n\n\nSo any receipt this executor emits for a blob tx will diverge from geth's semantics — the fields are permanently missing from the persisted/RPC-visible receipt.\n\nWhy blob txs reach this path. The port is otherwise blob-aware:\n- parseTx (giga/evmonly/parser.go:29) uses tx.UnmarshalBinary, which correctly decodes typed transaction envelopes including Type-3.\n- transactionToPreparedMessage (giga/evmonly/executor.go:255-256) forwards tx.BlobHashes() and tx.BlobGasFeeCap() into the core.Message, so blob txs run through ApplyMessage.\n- buildBlockContext plumbs BlobBaseFee into vm.BlockContext, and effectiveGasPrice has a BlobTxType branch.\n- The default chainConfig is params.AllDevChainProtocolChanges, which enables Cancun (blob support).\n\nA blob tx therefore executes normally and the executor gladly produces a receipt for it — just one with BlobGasUsed=0 and BlobGasPrice=nil.\n\nStep-by-step proof.\n1. Caller submits a Type-3 blob tx: BlobHashes=[h1, h2], BlobGasFeeCap=1_000_000, in a block with BlobBaseFee=250_000.\n2. parseTx decodes it via UnmarshalBinary — succeeds. Sender recovered.\n3. executeTx builds the message; ApplyMessage runs the payload and returns ExecutionResult.\n4. The receipt literal at lines 209-223 is populated with all listed fields. tx.Type() == 3 is never checked, so BlobGasUsed stays 0 and BlobGasPrice stays nil.\n5. Receipt is stored via the result sink (giga/evmonly/executor.go:79).\n6. An indexer or RPC client calls eth_getTransactionReceipt. It receives blobGasUsed="0x0" and blobGasPrice=null, whereas geth would return blobGasUsed="0x40000" (2 × BlobTxBlobGasPerBlob) and blobGasPrice="0x3d090".\n\nImpact and why this isn't marked normal. The executor is scaffolding — it is not wired into app/app.go yet, and blob transactions are currently rejected protocol-wide by x/evm/ante/basic.go (ErrUnsupportedTxType). The existing x/evm keeper also doesn't set these fields (grep for BlobGasUsed outside this PR returns no hits). So there is no user-facing regression today. This becomes a real bug the moment blob support is enabled and this executor takes over the receipt-building path — at which point the omission is silent and receipts are immutable once persisted.\n\nFix. After the receipt literal in executeTx, add:\n\ngo\nif tx.Type() == ethtypes.BlobTxType {\n receipt.BlobGasUsed = uint64(len(tx.BlobHashes())) * params.BlobTxBlobGasPerBlob\n receipt.BlobGasPrice = new(big.Int).Set(block.BlobBaseFee)\n}\n\n\nThis matches geth exactly and adds no runtime cost for non-blob txs. Worth a short test in executor_test.go alongside TestExecutorDynamicFeeTx once blob txs are permitted through the ante.
There was a problem hiding this comment.
A new, well-structured giga/evmonly EVM-only execution package with good test coverage, but the optional OCC path has two correctness defects: it hard-errors on speculative pre-check failures instead of falling back to sequential (aborting valid blocks with dependent same-sender nonces), and self-destruct of a contract with pre-existing persisted storage never emits slot deletions. Cursor's second-opinion pass produced no output.
Findings: 2 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Performance/design: OCC conflict tracking marks every priority-fee payment to the coinbase as a write (
AddBalance), so any realistic block with a nonzero tip has all txs conflict on the coinbase balance key and always falls back to sequential — OCC provides no parallelism except in zero-fee blocks. The OCC-vs-sequential tests only exercisegasPrice=0, which sidesteps the coinbase write (AddBalance early-returns on zero amount) and hides this. Consider excluding the coinbase fee credit from conflict tracking (or aggregating it post-merge). - Testing gaps: no test covers (a) OCC execution of a valid block with two dependent same-sender txs (nonce 0 then 1), which would catch the OCC nonce-fallback bug, nor (b) self-destruct (legacy config) of a contract that has pre-existing persisted storage from a prior block, which would catch the stale-storage bug.
StateReaderdoes not document that implementations must be safe for concurrent reads, yet OCC workers call it from multiple goroutines simultaneously; production backends must guarantee this — worth stating in the interface doc.NewExecutorrelies onruntime.SetFinalizer(e, Close)to tear down the OCC worker-pool goroutines; callers that create executors without callingClose()leak worker goroutines until GC runs the finalizer. Consider making explicit lifecycle ownership clearer in the README/interface.- Cursor's second-opinion review file (
cursor-review.md) was empty — that pass produced no output; only the Codex pass contributed external findings, both of which are incorporated here.
| for idx := txRange.start; idx < txRange.end; idx++ { | ||
| result, err := e.executeTxSpeculative(workerCtx, req, idx, chainConfig, blockCtx, baseFee, gasLimit) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
[blocker] OCC aborts valid blocks instead of falling back to sequential. Each tx is speculatively executed against pre-block state, so a valid block containing two txs from the same sender with nonces 0 and 1 makes tx#1 fail with ErrNonceTooHigh in core.ApplyMessage. That error is returned here, propagates out of pool.Run (line 62-64), and executeBlockOCC returns nil, err — the validateOCCResults/executeBlockSequential fallback on lines 65-67 is never reached. Since the design treats sequential as the source of truth, a speculative pre-check failure (nonce/fee/gas) should force the sequential fallback rather than aborting the whole (valid) block. Sequential is the only place block-level validity should be decided.
| if acct.SelfDestructed { | ||
| s.recordAccount(addr) | ||
| acct.Code = nil | ||
| acct.Storage = map[common.Hash]common.Hash{} |
There was a problem hiding this comment.
[blocker] Self-destruct does not delete pre-existing untouched storage from the changeset. Finalise clears only the lazily-loaded in-memory acct.Storage, and ChangeSet() (lines 149-162) emits per-slot deletes only for slots present in base.Storage/acct.Storage — i.e. slots that were actually read during execution. A contract created in an earlier block that is self-destructed under a pre-Cancun (legacy) chain config leaves its un-read storage slots behind when the changeset is applied via ApplyChangeSet, because those slots are never enumerated (the executor has no range reads). Result: stale storage persists in durable state after a legacy SELFDESTRUCT. Needs either a full-account storage-clear signal in the changeset or a range-read to enumerate the slots.
There was a problem hiding this comment.
New giga/evmonly EVM-only executor is well-structured and well-tested, but has two confirmed correctness divergences between its parallel OCC path / changeset construction and canonical sequential execution: OCC validates block gas using only consumed gas (not per-tx declared limits), and legacy selfdestruct drops untouched pre-existing storage slots from the changeset. Both are in opt-in paths and the package is not yet wired into the app.
Findings: 2 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- REVIEW_GUIDELINES.md is empty/missing (taken from base), so no repo-specific review standards could be applied.
- cursor-review.md is empty — the Cursor second-opinion pass produced no output; only Codex's review was available to merge.
- Codex's two findings were both reproduced and confirmed against the diff and geth semantics; they are included as inline comments.
- MemoryState.ApplyChangeSet (state.go) mirrors the selfdestruct storage gap: it deletes code on Delete but never wipes remaining storage slots for a destroyed account, so any durable StateWriter must be prepared to fully clear an account's storage — worth documenting alongside the changeset fix.
- Executor relies on runtime.SetFinalizer to Close() the OCC worker pool; finalizer-based cleanup is non-deterministic, so callers that construct executors with OCCWorkers>1 should be expected to call Close() explicitly.
| return false | ||
| } | ||
| totalGas += result.gasUsed | ||
| if totalGas > gasLimit { |
There was a problem hiding this comment.
[blocker] OCC block-gas validation only checks the sum of consumed gas (totalGas += result.gasUsed), but sequential execution enforces each tx's declared gas limit against the remaining block pool: core.ApplyMessage → buyGas calls gp.SubGas(msg.GasLimit) and returns core.ErrGasLimitReached if tx.Gas() doesn't fit. A block whose per-tx Gas() limits sum beyond gasLimit but whose used gas stays under it will be accepted here (via mergeOCCResults) while executeBlockSequential would reject it — an accept/reject divergence an attacker can construct (e.g. tx1 uses little gas, tx2 declares a large limit but uses little). Validation should account for declared gas limits the way the sequential gas pool does, or otherwise reproduce ErrGasLimitReached before accepting the OCC result.
| if acct.SelfDestructed { | ||
| s.recordAccount(addr) | ||
| acct.Code = nil | ||
| acct.Storage = map[common.Hash]common.Hash{} |
There was a problem hiding this comment.
[blocker] Finalise clears acct.Storage for a selfdestructed account, but ChangeSet() computes storage deltas from storageKeyUnion(base.Storage, acct.Storage), and base.Storage only contains slots that were loaded via ensureStorage during the block. A legacy (pre-EIP-6780) selfdestructed contract with pre-existing storage slots that were never read/written this block will not have those slots in base.Storage, so no delete is emitted and a durable StateWriter retains stale storage. (Under EIP-6780 this is masked because full deletion only occurs for same-tx-created contracts, whose storage is already tracked.) The changeset needs to represent a full storage wipe for destroyed accounts independent of which slots were touched.
There was a problem hiding this comment.
A well-structured, well-tested new EVM-only giga executor boundary (not yet wired into app.go), but it contains three genuine correctness issues in the core executor/state layer: an OCC-vs-sequential block-gas divergence, an EXTCODEHASH spec violation, and a selfdestruct changeset that can leak stale persisted storage. REVIEW_GUIDELINES.md and cursor-review.md were both empty; Codex's three findings are confirmed.
Findings: 3 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
- OCC effectiveness: with non-zero transaction fees, every tx credits the coinbase balance (a tracked write), so all txs conflict on the coinbase address and
validateOCCResultswill always fall back to sequential. Under realistic fees OCC provides no parallelism; consider excluding coinbase-fee crediting from conflict tracking or accumulating fees out-of-band. mergeChangeSets(occ.go) is defined but onlymergeChangeSetsIntois used — dead code; remove it (also a latentunused-linter risk).- Second-opinion passes:
cursor-review.mdis empty (Cursor produced no output) andREVIEW_GUIDELINES.mdis empty/absent, so no repo-specific review standards were applied. Codex's three findings were reviewed and are all confirmed above. - Documentation and test coverage are strong: README clearly states current limitations (sequential-only wiring, historical BLOCKHASH not wired, precompiles fail-closed) and the test suite covers validation failures, EIP-6780, snapshot/revert, pooling, and OCC/sequential equivalence.
| if result.gasUsed > math.MaxUint64-totalGas { | ||
| return false | ||
| } | ||
| totalGas += result.gasUsed |
There was a problem hiding this comment.
[blocker] OCC block validation only checks cumulative gasUsed against gasLimit, but the sequential/geth reference decrements the block GasPool by each tx's gas limit (buyGas → gp.SubGas(msg.GasLimit)) and returns ErrGasLimitReached if a tx's gas limit exceeds the remaining pool. A block with txs whose gas limits sum above the block limit (but whose gas used fits) is accepted here yet rejected by executeBlockSequential. Because OCC only falls back when validateOCCResults returns false, this produces a nondeterministic accept/reject between the OCC and sequential paths. Validation must model the per-tx gas-limit reservation against the remaining block gas, not just cumulative usage.
| 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 empty-code account. EXTCODEHASH (geth's opExtCodeHash) already guards with Empty(), so this method is reached for existing accounts with balance/nonce but no code — those must return the empty-code hash (crypto.Keccak256Hash(nil) = 0xc5d246…), and zero must be reserved for nonexistent accounts. As written, existing empty-code EOAs are misclassified as nonexistent.
| if acct.SelfDestructed { | ||
| s.recordAccount(addr) | ||
| acct.Code = nil | ||
| acct.Storage = map[common.Hash]common.Hash{} |
There was a problem hiding this comment.
[blocker] On selfdestruct Finalise clears the in-memory Storage map, but ChangeSetInto derives storage deletes from storageKeyUnion(base.Storage, acct.Storage), which only contains slots that were read (ensureStorage) during this block. A destroyed contract's persisted-but-unread slots are therefore never emitted as deletes, so ApplyChangeSet leaves stale storage in the backing StateReader. The tests only exercise contracts created in the same block (all slots cached), which hides this. Legacy selfdestruct (and any real durable backend) needs an account-level tombstone or a way to enumerate/delete all persisted slots.
| } | ||
| if err := e.resultSink.StoreReceipts(ctx, height, result.Receipts); err != nil { | ||
| return fmt.Errorf("store receipts for block %d: %w", height, err) | ||
| } |
There was a problem hiding this comment.
Split sink partial persistence
Medium Severity
When the executor uses a plain ResultSink (not BlockResultSink), it calls StoreChangeSet and then StoreReceipts separately. If the changeset store succeeds and the receipt store fails, the method still returns an error, but the sink may already hold persisted state for that height without matching receipts.
Reviewed by Cursor Bugbot for commit a38d56e. Configure here.
There was a problem hiding this comment.
A well-structured, well-tested new giga/evmonly EVM-only execution boundary (not yet wired into production), but it has several genuine EVM-semantics/consensus-equivalence defects: the OCC path can accept a block the sequential path would reject on block-gas reservation, selfdestruct leaves untouched persisted storage undeleted in the changeset, and GetCodeHash returns the wrong value for existing empty-code accounts.
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
- OCC has a coinbase write-hotspot limitation:
core.ApplyMessagecredits fees to the coinbase on every tx, so under any non-zero effective gas price every tx write-conflicts on the coinbase balance andvalidateOCCResultsfalls back to sequential for the whole block. The OCC tests only pass because they use gas price 0 (zero-amountAddBalanceskips the write mark). This is correct but makes OCC largely ineffective in production; consider special-casing fee accumulation. Worth documenting as a known limitation alongside the others in the README. Executor.Closeis registered viaruntime.SetFinalizerto shut down the worker-pool goroutines. Finalizers are not guaranteed to run promptly, so an executor that is dropped without an explicitClose()can leak the pool goroutines; consider requiring callers to callClose()and documenting it.- Cursor produced no second-opinion output (
cursor-review.mdwas empty) andREVIEW_GUIDELINES.mdwas empty, so this review relied on the Codex pass plus direct inspection. - Codex noted its
go test ./giga/evmonly/...run was blocked by the offline toolchain, so the test suite was not independently executed in that pass. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| 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 tx gets a fresh gas pool with the full block gas limit, but validateOCCResults (line ~150) only checks the summed actual gasUsed. The sequential path shares one depleting GasPool, and core.ApplyMessage reserves the tx's full GasLimit (gp.SubGas(tx.Gas())), not just gas used. So a block whose txs' summed GasLimit exceeds the block limit — while summed gasUsed stays under it — is accepted by OCC but returns core.ErrGasLimitReached (a fatal, whole-block error) under sequential. The two modes must be equivalent; OCC validation should also account for per-tx gas-limit reservation before accepting the speculative result.
| if acct.SelfDestructed { | ||
| s.recordAccount(addr) | ||
| acct.Code = nil | ||
| acct.Storage = map[common.Hash]common.Hash{} |
There was a problem hiding this comment.
[blocker] Finalise clears only the in-memory acct.Storage, and ChangeSetInto (line ~155) can emit storage deletes only for keys present in storageKeyUnion(base.Storage, acct.Storage). base.Storage holds only slots that were lazily loaded via ensureStorage, so a (legacy, pre-6780) selfdestructed contract's persisted slots that were never read/written in the block are never emitted as deletes and survive in the underlying state after the changeset is applied — the changeset no longer represents true post-block state. Under EIP-6780 rules this is masked (only same-tx-created contracts are fully destroyed), but the legacySelfDestructChainConfig path exercised by the tests can hit it; the existing tests pass only because the destroyed contracts have no untouched persisted slots.
|
|
||
| func (s *nativeStateDB) GetCodeHash(addr common.Address) common.Hash { | ||
| code := s.GetCode(addr) | ||
| if len(code) == 0 { |
There was a problem hiding this comment.
[suggestion] GetCodeHash returns the zero hash for any account with empty code. EXTCODEHASH semantics require zero only for non-existent accounts; an existing account with empty code must return the empty-code hash (crypto.Keccak256Hash(nil) / types.EmptyCodeHash). Contracts using EXTCODEHASH will observe incorrect results here. Distinguish existence (nonce/balance/code) before returning zero.
| key common.Hash | ||
| } | ||
|
|
||
| func mergeChangeSets(results []occTxExecution) StateChangeSet { |
There was a problem hiding this comment.
[nit] mergeChangeSets is unexported and never called (only mergeChangeSetsInto is used). This is dead code that golangci-lint's unused linter will flag and fail CI on; remove it or wire it up.
| if err := pool.Run(ctx, occRanges(txCount, chunkSize), func(workerCtx context.Context, txRange occTxRange, scratch *occWorkerScratch) error { | ||
| for idx := txRange.start; idx < txRange.end; idx++ { | ||
| result, err := e.executeTxSpeculative(workerCtx, scratch, req, idx, chainConfig, blockCtx, baseFee, gasLimit) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| results[idx] = result | ||
| } | ||
| return nil | ||
| }); err != nil { | ||
| return nil, err | ||
| } | ||
| if !validateOCCResults(results, gasLimit) { | ||
| return e.executeBlockSequential(ctx, req) | ||
| } | ||
| return e.mergeOCCResults(ctx, results) | ||
| } |
There was a problem hiding this comment.
🔴 OCC speculative execution errors bypass the sequential fallback. executeBlockOCC (occ.go:66-77) returns pool.Run's error directly, and executeTxSpeculative (occ.go:143-145) wraps every core.ApplyMessage error — including preCheck failures like ErrNonceTooHigh/ErrNonceTooLow/ErrInsufficientFunds against pre-block state. The fallback at lines 78-80 is never reached, so blocks that sequential execution would accept (e.g. two txs from the same sender with nonces 0 and 1) are rejected outright when OCC is enabled. Fix: treat speculative preCheck failures as conflicts that trigger the sequential fallback rather than fatal errors.
Extended reasoning...
Bug and code path
executeBlockOCC in giga/evmonly/occ.go runs every transaction speculatively against the block-initial state via pool.Run and short-circuits on the first error:
// occ.go:66-77
if err := pool.Run(ctx, occRanges(txCount, chunkSize), func(...) error {
for idx := txRange.start; idx < txRange.end; idx++ {
result, err := e.executeTxSpeculative(...)
if err != nil { return err }
results[idx] = result
}
return nil
}); err != nil {
return nil, err // <-- fatal exit, no fallback
}
if !validateOCCResults(results, gasLimit) {
return e.executeBlockSequential(ctx, req) // <-- unreachable when a speculative tx errored
}executeTxSpeculative (occ.go:143-145) wraps the raw core.ApplyMessage error and returns it. Since each speculative tx resets its scratch StateDB to e.state (block-initial), any tx whose validity depends on a prior tx's writes hits ApplyMessage's preCheck against the wrong nonce/balance/fee state.
Concrete step-by-step proof
Consider a valid block with two txs from the same sender A (pre-block nonce = 0):
- tx0: sender=A, nonce=0
- tx1: sender=A, nonce=1
Sequential path (executeBlockSequential, executor.go): tx0 executes, Finalise(true) advances A's nonce to 1 in the StateDB, tx1's preCheck sees state nonce=1, tx1 accepted. Block succeeds.
OCC path (executeBlockOCC):
- Two speculative jobs are dispatched. Each calls
scratch.resetStateDB(e.state)so both see A's pre-block nonce=0. - tx0 executes fine (nonce=0 matches state nonce=0).
- tx1 executes with
msg.Nonce=1against state nonce=0.core.ApplyMessage's preCheck returnscore.ErrNonceTooHigh. executeTxreturns that error;executeTxSpeculativewraps it withfmt.Errorf; the pool callback returns non-nil.pool.Runpropagates the error;executeBlockOCCreturns(nil, err)at line 76.validateOCCResults/executeBlockSequentialat lines 78-80 is never reached.
The block is rejected under OCC but would have been accepted sequentially.
Why existing code doesn't prevent it
The validateOCCResults / sequential fallback pattern is the correct place to catch stale-read conflicts — but it only runs on successful speculative results. The current error propagation treats a speculative pre-check failure as a fatal block-validation error, even though sequential is the source-of-truth path and would have succeeded. Existing tests (TestExecutorOCCNonConflictingTransfersMatchSequential, TestExecutorOCCConflictingTransfersMatchSequential) generate a fresh key per tx, so every sender has nonce=0 and this case is never exercised.
Impact
Same-sender multi-tx blocks are extremely common (bots, market makers, DEX arbitrage, batch senders). Any node with OCCWorkers > 1 will reject these otherwise-valid blocks. If any subset of validators enables OCC while others use sequential (or if OCC is enabled while a legacy block with multi-nonce sender arrives), this creates a hard accept/reject divergence — i.e. a consensus fork. Even absent app-level wiring today, this ships OCC as a strict regression versus sequential: any block OCC accepts can be run sequentially, but blocks exist that OCC rejects and sequential accepts. Multiple external reviewers (seidroid × 4, cursor bugbot × 2, claude) flagged this as a blocker on 2026-06-30, 2026-07-02 (×3), and 2026-07-02.
Suggested fix
Distinguish speculative-execution errors caused by stale reads from genuine block-invalidating failures. The former should trigger the same sequential fallback that validateOCCResults returning false does. Concretely: on any error from executeTxSpeculative that could result from block-initial-state divergence (nonce mismatch, insufficient funds, fee-cap-below-basefee, intrinsic-gas — essentially any core.ApplyMessage preCheck failure), fall back to executeBlockSequential. Only propagate errors that are guaranteed to be block-invalidating regardless of execution ordering (e.g. context cancellation, panics).
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9ae1cfb. Configure here.
There was a problem hiding this comment.
This PR adds a new, well-tested EVM-only giga execution boundary (giga/evmonly) that is isolated from production wiring, but the optional OCC path can diverge from the sequential reference path on block-gas accounting, the changeset model cannot express full storage deletion on legacy SELFDESTRUCT, and two unused symbols will fail the staticcheck-based CI lint.
Findings: 2 blocking | 5 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- OCC will almost always fall back to sequential in production: every transaction with a non-zero effective tip credits the coinbase via AddBalance, so each tx's write-set contains {balance, coinbase} and tx N+1 conflicts with tx N on the coinbase balance. The provided OCC tests only pass because they use base fee 0 and gas price 0. Consider excluding coinbase fee accrual from conflict tracking (or aggregating it deterministically) so OCC is usable when fees are non-zero.
- REVIEW_GUIDELINES.md is empty/missing on the base branch, so no repo-specific standards were applied. The Cursor second-opinion review (cursor-review.md) was also empty — that pass produced no output. The Codex review flagged the same two correctness issues captured here as inline blockers.
- Neither the OCC gas-reservation divergence nor the legacy-SELFDESTRUCT stale-storage gap is covered by a test; the existing
block gas exhaustedtest only exercises the sequential path (OCCWorkers unset). Add OCC-vs-sequential equivalence tests for (a) txs whose summed declared gas exceeds the block limit but summed used gas does not, and (b) a pre-6780 self-destruct of a pre-existing contract with unread persisted slots. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| validation.fallbackReason = occFallbackReasonGasOverflow | ||
| return validation | ||
| } | ||
| totalGas += result.gasUsed |
There was a problem hiding this comment.
[blocker] OCC validates only the sum of actual gasUsed against the block gas limit, but the sequential reference path reserves each tx's declared tx.Gas() from the shared block GasPool (go-ethereum buyGas calls gp.SubGas(msg.GasLimit) before execution). This makes the two paths disagree: e.g. two independent txs each declaring Gas=90k under a 100k block limit but each only using 21k are accepted by OCC (42k ≤ 100k) while the sequential path rejects the second tx with ErrGasLimitReached. Since OCC output must be replay-consistent with sequential execution, OCC validation should account for declared gas reservation (or fall back when cumulative tx.Gas() exceeds the block gas pool).
| Delete: len(acct.Code) == 0, | ||
| }) | ||
| } | ||
| storageKeys := storageKeyUnion(base.Storage, acct.Storage) |
There was a problem hiding this comment.
[blocker] ChangeSetInto can only emit storage deletes for keys present in base.Storage or acct.Storage (i.e. slots that were read/written during the block). Legacy SelfDestruct clears acct.Storage in Finalise, but for a pre-existing contract with persisted slots that were never touched this block, those keys appear in neither map, so no Delete is emitted and applying the changeset leaves stale storage after account deletion. The key-addressable changeset model has no way to express "delete all storage for this account." This is latent under the default Cancun/EIP-6780 config (existing contracts don't clear storage), but is a real correctness gap for the legacy self-destruct semantics the executor still supports and tests. Needs full-account deletion semantics in the changeset or a backend capability to enumerate/delete all storage for self-destructed accounts.
| key common.Hash | ||
| } | ||
|
|
||
| func mergeChangeSets(results []occTxExecution) StateChangeSet { |
There was a problem hiding this comment.
[suggestion] mergeChangeSets has no callers (only mergeChangeSetsInto is used). staticcheck (U1000), enabled in .golangci.yml, will flag this unused function and fail CI. Remove it or wire it in.
| } | ||
| } | ||
|
|
||
| func (i *stateAccessIndex) conflictsWithAny(set map[stateAccessKey]struct{}) bool { |
There was a problem hiding this comment.
[nit] conflictsWithAny is defined but never called, so staticcheck (U1000) will report it as unused and fail the lint job. Remove it or use it.
| s.AddAddressToAccessList(addr) | ||
| slots, ok := s.accessList.slots[addr] | ||
| if !ok { | ||
| slots = map[common.Hash]struct{}{} | ||
| s.accessList.slots[addr] = slots | ||
| } | ||
| slots[slot] = struct{}{} | ||
| } | ||
|
|
||
| func (s *nativeStateDB) Prepare(_ params.Rules, sender, coinbase common.Address, dest *common.Address, precompiles []common.Address, txAccesses ethtypes.AccessList) { | ||
| s.accessList.reset() | ||
| clearNestedHashMaps(s.transientStates) | ||
| s.AddAddressToAccessList(sender) | ||
| s.AddAddressToAccessList(coinbase) | ||
| if dest != nil { | ||
| s.AddAddressToAccessList(*dest) | ||
| } | ||
| for _, addr := range precompiles { |
There was a problem hiding this comment.
🟡 🟡 Prepare at giga/evmonly/state_db.go:420 declares its params.Rules argument as _ and unconditionally (a) resets the access list plus warms sender/dest/precompiles, and (b) adds the coinbase to the access list. go-ethereum's StateDB.Prepare (core/state/statedb.go:1343-1372 in sei-fork v1.15.7-sei-16) gates the access-list setup on rules.IsEIP2929 and the coinbase warm-up on rules.IsShanghai (EIP-3651). For a Berlin-but-pre-Shanghai config — e.g. this file's own legacySelfDestructChainConfig (HomesteadBlock..LondonBlock=0, ShanghaiTime nil) exercised by TestExecutorCreateSelfDestructThenTransferSameAddress/TestExecutorFinalisesAfterEachTx — this warms coinbase when geth leaves it cold, so any BALANCE/EXTCODESIZE/EXTCODEHASH/CALL against coinbase is charged 100 gas here vs 2600 gas in geth (a receipt-visible divergence on the default sequential path). AllDevChainProtocolChanges masks it because Shanghai is on. Fix by threading rules through and gating the two behaviors like geth does — a ~4-line change.
Extended reasoning...
What the bug is
In giga/evmonly/state_db.go, nativeStateDB.Prepare declares its signature as Prepare(_ params.Rules, sender, coinbase common.Address, ...) — the params.Rules argument is explicitly discarded. The body then unconditionally:
- Resets the access list (
s.accessList.reset()) - Clears transient state (correct — a separate, orthogonal fix)
- Warms
sender,coinbase,dest,precompiles, and everytxAccessesentry
go-ethereum's canonical StateDB.Prepare (verified in sei-fork github.com/sei-protocol/go-ethereum@v1.15.7-sei-16, core/state/statedb.go:1343-1372) gates these behaviors on the incoming params.Rules:
- Access-list reset + sender/dest/precompiles/txAccesses warm-up happens only when
rules.IsEIP2929is true (Berlin+). - Coinbase warm-up happens only when
rules.IsShanghaiis true (EIP-3651).
core/state_transition.go:501 already passes the correct rules derived from the executor's chain config, so the call site is fine — this implementation actively discards them.
Concrete divergence for a Berlin-but-pre-Shanghai config
The test file defines and exercises exactly this shape via legacySelfDestructChainConfig:
HomesteadBlock..LondonBlock = big.NewInt(0)
// ShanghaiTime is nil, CancunTime is nilUnder these rules, IsEIP2929=true but IsShanghai=false. In geth, that means the access list is set up but the coinbase is not added — so the coinbase remains cold for the entire transaction. Any EVM opcode that touches the coinbase address (BALANCE(coinbase), EXTCODESIZE(coinbase), EXTCODEHASH(coinbase), CALL(coinbase, ...)) pays the cold access cost of 2600 gas (EIP-2929).
This implementation instead unconditionally calls AddAddressToAccessList(coinbase), so the same opcode sees the coinbase as warm and pays only 100 gas. That is a per-transaction gasUsed and per-transaction receipt divergence — a consensus-level difference on the default sequential path.
The reverse concern (access-list warming being active pre-Berlin) is largely benign because pre-Berlin opcodes do not consult the access list for gas pricing, but the semantically correct fix gates both paths anyway.
Step-by-step proof
Consider a transaction under legacySelfDestructChainConfig that executes BALANCE(coinbase) once:
- Executor calls
evm.SetTxContext(...), which callsstateDB.Prepare(rules, sender, coinbase, dest, precompiles, txAccesses). Rules hasIsEIP2929=true,IsShanghai=false. Prepareignoresrulesand runsAddAddressToAccessList(coinbase). Coinbase is now ins.accessList.addresses.- EVM executes
BALANCE(coinbase). Gas cost is computed viagasEip2929AccountCheck→SlotInAccessList(coinbase, ...)returns(true, _)→ warm access, 100 gas. - Under geth with the same rules: step 2 leaves coinbase absent from the access list. Step 3's access check returns
(false, _)→ cold access, 2600 gas. Additionally geth increments the read-only warm-up in the same call.
Same block, same tx, same input state — differing receipt.GasUsed and differing cumulativeGasUsed for every downstream tx in the block.
Why existing code does not prevent it
- The default
AllDevChainProtocolChangesused by most tests has Shanghai active, soIsShanghai=trueand geth also warms coinbase — the two impls happen to match, masking the divergence. - The two tests that use
legacySelfDestructChainConfig(TestExecutorCreateSelfDestructThenTransferSameAddress,TestExecutorFinalisesAfterEachTx) exercise the code path but never inspect coinbase-touch gas costs, so the divergence does not fail any assertion. - Sei mainnet configs are post-Shanghai/Cancun, so no live production consumer hits this today.
Impact and severity
Marking as nit because:
giga/evmonlyis not wired intoapp/app.go— no runtime consumer is affected today.- Every Sei production chain config has Shanghai active, so the always-warm behavior matches geth on those configs.
- The affected path (pre-Shanghai
ChainConfig) is only reachable via test-owned configs, and no test asserts on it.
Still worth fixing before this executor is wired into a real callsite, because (a) this ships incorrect EVM semantics on a shipped code path that is exercised by tests, and (b) the fix is a small, mechanical change that mirrors geth exactly.
How to fix
Thread rules through and gate the two behaviors like core/state/statedb.go:1343-1372:
func (s *nativeStateDB) Prepare(rules params.Rules, sender, coinbase common.Address, dest *common.Address, precompiles []common.Address, txAccesses ethtypes.AccessList) {
clearNestedHashMaps(s.transientStates)
if rules.IsEIP2929 {
s.accessList.reset()
s.AddAddressToAccessList(sender)
if dest != nil {
s.AddAddressToAccessList(*dest)
}
for _, addr := range precompiles {
s.AddAddressToAccessList(addr)
}
for _, tuple := range txAccesses {
s.AddAddressToAccessList(tuple.Address)
for _, key := range tuple.StorageKeys {
s.AddSlotToAccessList(tuple.Address, key)
}
}
if rules.IsShanghai {
s.AddAddressToAccessList(coinbase)
}
}
}A regression test that runs a BALANCE(coinbase) (or any EXTCODESIZE/EXTCODEHASH/CALL against the block coinbase) under legacySelfDestructChainConfig and asserts receipt.GasUsed == 21000 + 2600 + ... would catch this and belongs alongside the existing legacy-selfdestruct tests.
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.
| for idx := txRange.start; idx < txRange.end; idx++ { | ||
| result, err := e.executeTxSpeculative(workerCtx, req, idx, chainConfig, blockCtx, baseFee, gasLimit) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
[blocker] Blocker: this returns speculative-execution errors as a hard block failure before conflict validation can run. Because executeTxSpeculative runs each tx against the base state with nonce checks enabled (DisableNonceCheck defaults false), two consecutive-nonce txs from the same sender make the second speculative run see the base nonce and return core.ErrNonceTooHigh. That error propagates here and aborts a perfectly valid block instead of being detected as a sender-nonce read/write conflict and falling back to sequential execution. Same-sender sequential nonces are extremely common. Skip nonce checks during speculation (or classify nonce errors as a conflict) so validateOCCResults can trigger the intended fallback.
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