[codex] add dynamic custom precompile gas meter#3644
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## codex/evmonly-staking-precompile #3644 +/- ##
====================================================================
- Coverage 58.16% 58.15% -0.01%
====================================================================
Files 2189 2187 -2
Lines 178615 178718 +103
====================================================================
+ Hits 103884 103939 +55
- Misses 65424 65454 +30
- Partials 9307 9325 +18
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).
|
PR SummaryMedium Risk Overview
The staking precompile drops fixed read/write gas in Reviewed by Cursor Bugbot for commit 35864cf. Bugbot is set up for automated code reviews on this repo. Configure here. |
27e3acc to
550f6fa
Compare
56bf186 to
bd57e4c
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.
Reviewed by Cursor Bugbot for commit bd57e4c. Configure here.
There was a problem hiding this comment.
The dynamic precompile gas meter is implemented carefully and faithfully mirrors go-ethereum's EIP-2929/2200/3529 SLOAD/SSTORE/refund accounting, with refund state being snapshot-safe and OOG correctly surfaced and discarded; tests cover the new metering. The only issue is a now-orphaned helper function and a behavioral note about read-method gas.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
isTransactioningiga/evmonly/precompiles/staking/helpers.gowas the only consumer of the removed read/write gas split and is now unused dead code (no remaining references in the package). Remove it — golangci-lint'sunusedcheck may flag this and fail CI even though the PR notes lint passed.- Behavior change worth noting: read-only query methods (
validators,delegatorDelegations, etc.) previously cost a flatreadGas(3000) and now meter per-SLOAD dynamically. Read-heavy paged queries (up to pageLimit=100 validators, each a multi-chunk read) can consume substantially more gas than before; callers/eth_call gas caps must account for this. This is more accurate, just a notable semantics change. - Codex and Cursor second-opinion passes both produced no material findings.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| gas = writeGas | ||
| } | ||
| return gas + inputByteGas*uint64(len(input)) //nolint:gosec // input length is bounded by memory. | ||
| return baseGas + inputByteGas*uint64(len(input)) //nolint:gosec // input length is bounded by memory. |
There was a problem hiding this comment.
[suggestion] With the read/write gas split removed here, isTransaction in helpers.go is no longer referenced anywhere in the package and is now dead code. Consider removing it — the unused linter may otherwise fail CI.
550f6fa to
accacbf
Compare
bd57e4c to
7c6c44c
Compare
There was a problem hiding this comment.
The PR adds a dynamic gas meter for evm-only custom precompiles whose SLOAD/SSTORE/access-list/keccak/log/transfer pricing faithfully mirrors go-ethereum (EIP-2200/2929/3529), with refund and access-list mutations correctly rolled back on OOG via RevertToSnapshot and good test coverage. The metering relies on GetCommittedState as the SSTORE "original" value, but nativeStateDB.base is block-start state that Finalise never advances, so SSTORE cost/refunds are mispriced for slots written across multiple transactions in the same block.
Findings: 1 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
cursor-review.mdwas empty — the Cursor second-opinion pass produced no output, so its perspective is not reflected here. (REVIEW_GUIDELINES.md was also empty; reviewed without repo-specific guidance.)- Minor:
chargeNativeTransfercharges account-access (cold/warm) for bothfromandto. Standard EVM CALL gas does not recharge access for the caller/from(it is already warm), so transfers slightly overcharge relative to geth. Likely acceptable as a custom model, but worth confirming it is intentional.
| if current == value { | ||
| return m.charge(gasAdd(cost, params.WarmStorageReadCostEIP2929), tracing.GasChangeCallPrecompiledContract) | ||
| } | ||
| original := db.GetCommittedState(addr, slot) |
There was a problem hiding this comment.
[blocker] GetCommittedState is used as the EIP-2200 "original" (transaction-start) value, but in this nativeStateDB the base map holds block-start state and is never advanced between transactions — Finalise (state_db.go:492) only clears selfdestruct/created flags and resets refund, and account()/baseAccount() populate base lazily from source (block start). The executor reuses one stateDB for the whole block (executor.go:80), so for a slot written in tx N and re-written/cleared in tx N+1 of the same block, original here is the block-start value rather than the tx N+1-start value.
This diverges from go-ethereum (where per-tx Finalise moves dirty storage into pending storage so GetCommittedState reflects prior txs in the block), causing SSTORE to take the wrong SET/RESET/dirty branch and emit incorrect AddRefund/SubRefund amounts (under/over-charging and bogus clear refunds). Before this PR the flat read/write gas hid the issue; the dynamic meter now makes it consensus-relevant for gas accounting. Consider tracking a separate transaction-start snapshot (advanced in Finalise) distinct from the block-start base used by ChangeSet.
accacbf to
93a6625
Compare
7c6c44c to
6f9547a
Compare
There was a problem hiding this comment.
Adds a dynamic gas meter for evm-only custom precompiles; the SSTORE/SLOAD/refund/transfer/log accounting faithfully mirrors go-ethereum's EIP-2929/3529 rules, is overflow-guarded, and correctly reverts (refund/access-list/logs/storage) on out-of-gas, with new test coverage. No blocking issues found; a few non-blocking notes.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (
cursor-review.md) is empty — that pass produced no output. - Codex (
codex-review.md) reported no material findings but could not rungo test ./giga/evmonly/...because the sandbox had Go 1.24.13 while go.mod requires 1.25.6; its pass was therefore lint/inspection-only. - Design fragility (not a bug today): when a storage/transfer/log charge fails mid-execution,
storageBackedStore.Get/Set/DeleteandmeteredLogSink.AddLogsilently no-op without surfacing the out-of-gas to the precompile. Correctness currently relies entirely onRunAndCalculateGasobservingmeter.errand the caller reverting. A future precompile that branches on aGet()result after OOG would compute on stale/empty data (e.g. treating an existing key as absent). Consider surfacing the OOG through the Store/LogSink interfaces so the contract short-circuits deterministically. - Tracing cosmetics: warm SLOAD reads, all SSTORE variants, native transfers, and log emission report
tracing.GasChangeCallPrecompiledContractrather than storage/log-specific reasons (only cold accesses useGasChangeCallStorageColdAccess). This only affects tracer output, but it makes gas-change traces harder to interpret. - Gas scaling observation: metering now makes query/write methods cost proportional to the store index lists they touch (e.g.
getStringList-backed indices rewritten on delegation), which is why lifecycle tests needed 8M gas. This is inherent to the existing store design rather than introduced here, but worth confirming the per-op costs are acceptable for large validator/delegator sets.
Superseded: latest AI review found no blocking issues.
93a6625 to
8548c28
Compare
6f9547a to
31f16a8
Compare
There was a problem hiding this comment.
Adds a dynamic gas meter for evm-only custom precompiles and rewires staking's state/balance/log boundaries through it, replacing static read/write gas with EIP-2929/2200/3529-style accounting. The gas math faithfully mirrors go-ethereum, refunds flow through the existing GetRefund path (core applies the 3529 cap), and the OOG path is tested end-to-end; no blocking issues, only minor notes.
Findings: 0 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) is empty — that pass produced no output. Codex (codex-review.md) reported "No material findings."
- Because the staking precompile is SDK-free and performs all state access through ctx.Store/ctx.Balances/ctx.Logs, the reduction of RequiredGas to baseGas (700) is coherent — every read/write is now dynamically metered. Worth confirming there is no un-metered path (e.g. large in-memory work in list/pagination methods like validators()/redelegations() that iterate index lists) that could be underpriced relative to its cost; today only the storage touches are charged, not the O(n) decode/filter loops.
- On out-of-gas, storageBackedStore.Get returns (nil,false) and Set/Delete return silently, relying entirely on RunAndCalculateGas checking meter.err afterward. This is correct and tested for storeWritePrecompile/staking, but it is a fragile contract: a precompile that branches on a read result (now silently empty on OOG) before the adapter re-checks meter.err could take an unintended code path. Consider a doc comment on the pattern and/or a test that OOGs mid-read.
- Nit: most charges (warm SLOAD, native-transfer sub-costs, log gas, keccak) all pass reason tracing.GasChangeCallPrecompiledContract rather than warm/cold-specific reasons; purely cosmetic for tracers but slightly imprecise.
- Nit: chargeNativeTransfer charges account-access for
fromas well asto; real CALL semantics treat the caller as already warm. In practicefromis usually warmed via Prepare so it charges the warm cost, but the extra access is a conservative (safe) over-charge worth a brief comment. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| func (s storageBackedStore) Get(key []byte) ([]byte, bool) { | ||
| if !s.chargeStoreBaseSlot(key) { |
There was a problem hiding this comment.
[suggestion] Silent no-op on out-of-gas: when chargeStoreBaseSlot (or a later charge) fails, Get returns (nil,false) — indistinguishable from "key not found" — and Set/Delete return without applying the write. Correctness here depends entirely on RunAndCalculateGas re-checking meter.err after run() returns (which it does). It works and is tested, but it's a subtle invariant: any future precompile that acts on a read's result before returning would see empty data on OOG. A short comment documenting this contract would help.
| } | ||
|
|
||
| func (m *precompileGasMeter) chargeSStore(db *nativeStateDB, addr common.Address, slot common.Hash, value common.Hash) bool { | ||
| if m.remaining <= params.SstoreSentryGasEIP2200 { |
There was a problem hiding this comment.
[nit] chargeSStore does not short-circuit on an already-set m.err before running the sentry check and mutating the access list / refund counter. In the current call ordering this is unreachable (chargeStoreBaseSlot's keccak charge fails first once m.err is set, so Set/Delete return before reaching chargeSStore), but adding an early if m.err != nil { return false } here would make the invariant robust to future refactors, since AddSlotToAccessList/AddRefund side effects run before the final charge.
8548c28 to
04870c2
Compare
31f16a8 to
9cb00d6
Compare
There was a problem hiding this comment.
Adds a dynamic gas meter for evm-only custom precompiles that faithfully reimplements EIP-2929/2200/3529 storage, keccak, transfer, and log costs; the OOG path reverts state correctly via snapshot and the staking base-gas reduction is safely compensated by per-slot metering. No blocking issues found; a couple of non-blocking notes around dedicated unit-test coverage and a subtle Store return-value semantic.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The new precompileGasMeter reimplements consensus-sensitive EIP-2929/2200/3529 SSTORE/SLOAD gas and refund logic, but there is no dedicated unit test for it. Existing coverage only exercises one OOG path and the staking lifecycle. Consider adding targeted tests for the SSTORE transitions (cold vs warm, create/reset/clear, dirty re-write, refund add/sub, and the SstoreSentry boundary) so future edits to this logic are guarded.
- Cursor's second-opinion review (cursor-review.md) is empty — that pass produced no output. Codex's review (codex-review.md) reported “No material findings.” Flagging so the empty Cursor pass isn't mistaken for an implicit approval.
- Gas is charged before the corresponding s.db.SetState in every store mutation and the whole tx reverts on OOG, which is correct; worth a brief code comment documenting that invariant given how central it is to consensus safety.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| func (s storageBackedStore) Get(key []byte) ([]byte, bool) { | ||
| if !s.chargeStoreBaseSlot(key) { |
There was a problem hiding this comment.
[nit] When a gas charge fails mid-read, Get returns (nil, false) — the same signal as "key not found". A precompile could therefore branch as if the key were absent rather than aborting. This is safe here only because meter.err is set and RunAndCalculateGas unconditionally converts a set meter.err into vm.ErrOutOfGas, overriding any success the contract subsequently returns. Worth a short comment noting this conflation is intentionally masked by the meter, so a future refactor doesn't accidentally rely on the false meaning "not found" after an OOG.
04870c2 to
83c1b62
Compare
9cb00d6 to
35864cf
Compare

Summary
Adds dynamic gas accounting for evm-only custom precompiles, with the staking precompile using the shared meter through the existing store, balance-transfer, and log boundaries.
Changes
storageBackedStore,nativeBalanceTransfer, and log emission in the custom precompile adapter so gas is charged based on the actual execution path.RequiredGasto only base/input gas, avoiding double charging now that state access is dynamically metered.Validation
go test ./giga/evmonly/...golangci-lint v2.8.0 run ./giga/evmonly/...