Conversation
🤖 Kimi Code ReviewReview SummaryThis PR changes EIP-7708 event naming from "Selfdestruct" to "Burn" across the codebase. The changes appear correct and follow the updated EIP specification. Specific Issues Found1. Test Function Naming Inconsistency (test file)File: The function fn assert_burn_log(log: &Log, contract: Address, balance: U256) {
// ... existing implementation ...
}2. Test Comments Not Updated (test file)File: Several comments still refer to "Selfdestruct" instead of "Burn":
These should be updated to use "Burn" terminology for consistency. 3. Missing Documentation Update (constants file)File: The comment should be updated to reflect the new event name: /// EIP-7708: keccak256('Burn(address,uint256)')
VerificationThe keccak256 hash for "Burn(address,uint256)" has been correctly calculated and matches the new Security AssessmentNo security vulnerabilities introduced. The changes are purely semantic and don't affect the underlying logic or state transitions. Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Codex Code ReviewFindings
Summary Possible next steps
Automated review by OpenAI Codex · custom prompt |
🤖 Claude Code ReviewNow I have a comprehensive understanding of the PR. Let me create the review. Code Review: PR #6216 - bal-devnet-3: Rename EIP-7708 Selfdestruct event to BurnSummaryThis PR renames the EIP-7708 event from Overall Assessment✅ The changes are correct and well-executed. The renaming is systematic and complete across all relevant files. The hash constant has been correctly updated to match Detailed Findings1. CorrectnessHash Constant (crates/vm/levm/src/constants.rs:107-110) ✅
Function Renaming (crates/vm/levm/src/utils.rs:668) ✅
Import Updates (crates/vm/levm/src/opcode_handlers/system.rs:9) ✅
2. Incomplete Renaming in Test FileIssue: The helper function and comments in test file still use old terminology Location: test/tests/levm/eip7708_tests.rs:331-354 The function
All call sites at lines 1035, 1109 would need updating. 3. Comments Still Using Old TerminologyWhile the code is functionally correct, several comments still reference "Selfdestruct" when they should say "Burn": test/tests/levm/eip7708_tests.rs:
Note: References to "SELFDESTRUCT" (the opcode) or "self-destruct" (the concept) are appropriate and should remain. 4. Documentation AlignmentLocation: docs/eip.md:10 Current description: "All ETH transfers emit Transfer/Selfdestruct logs automatically" Should be updated to: "All ETH transfers emit Transfer/Burn logs automatically" Security & Correctness Verification✅ No security concerns - This is purely a renaming refactor RecommendationsPriority 1 (Consistency):
Priority 2 (Clarity): VerdictAPPROVE with minor improvements suggested The core changes are correct and complete. The new hash constant is accurate, all functional code has been updated, and the changes are systematic. The remaining issues are purely cosmetic (test helper names and comments) and don't affect correctness or security. These can be addressed in a follow-up or before merge. Automated review by Claude (Anthropic) · custom prompt |
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
Lines of code reportTotal lines added: Detailed view |
edg-l
added
the
Upgrade
edg-l
force-pushed
the
bal-devnet-3
branch
2 times, most recently
from
b088951 to
f13160d
Compare
Per ethereum/EIPs#11311, the EIP-7708 log event is renamed from Selfdestruct(address,uint256) to Burn(address,uint256) with a new topic hash.
Reject blocks whose block access list exceeds the gas-derived limit: bal_items * ITEM_COST <= available_gas + system_allowance
This reverts commit 4662fde.
Align DUPN, SWAPN, and EXCHANGE decoding with the updated EIP-8024 spec (ethereum/EIPs#11306). decode_single now uses (x + 145) % 256 instead of branching, and decode_pair uses XOR 0x8F instead of conditional subtraction. EXCHANGE valid range extended from 0x00-0x4F to 0x00-0x51, adding two new swappable pairs (14,15) and (14,16).
- Subtract remaining state_gas_reservoir from gas_used in finalize_execution (unused reservoir is returned, not consumed) - EIP-7702 auth refund returns to state gas reservoir for Amsterdam+ instead of regular refund counter - Skip Osaka TX_MAX_GAS_LIMIT cap for Amsterdam+ (EIP-8037 reservoir model removes the per-tx gas limit restriction)
…P-8037)" This reverts commit 5915fe0.
…IP-8037)" This reverts commit 65e57ee.
When a child frame reverts and its sub-child had also reverted, the state gas reservoir gain from the sub-child was not captured by the parent restoration formula. The old formula used reservoir_snapshot diffs which broke when the reservoir increased during child execution. New formula: new_reservoir = current_reservoir + child_state_gas_used, which correctly matches EELS incorporate_child_on_error behavior where parent.state_gas_left += child.state_gas_used + child.state_gas_left. Fixes CallGoesOOGOnSecondLevel2 Amsterdam EF test failures.
…037) Per EELS, CREATE/CREATE2 collisions return gas_used=0 — the user pays for the consumed child gas but it should not count as regular gas in block accounting (block gas_used = max(regular, state)). Track collision-consumed gas separately and subtract it when computing regular_gas in refund_sender. Fixes 8 EF test failures in collision scenarios (eip7610, eip7928, eip8037, eip6780, stCreateTest).
Amsterdam fork spec is still evolving, so don't block normal PR workflows on consume-engine Amsterdam failures.
edg-l
requested review from
a team,
ManuelBilbao,
avilagaston9 and
ilitteri
as code owners
🤖 Kimi Code ReviewReview SummaryThis PR implements Amsterdam fork support with EIP-7928 (Block Access Lists), EIP-8037 (multidimensional gas), and EIP-7954 (increased initcode/code size limits). The changes are substantial and touch core consensus logic. Critical Issues Found
Security Concerns
Performance Issues
Code Quality Issues
Recommendations
The PR appears to correctly implement the Amsterdam fork changes, but the parallel execution validation needs careful review for thread safety. Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Codex Code Review
Overall: the PR is strong and comprehensive on Amsterdam/EIP integration, but the BAL validation edge case and memory behavior in parallel execution are the most important items to address before merge. Automated review by OpenAI Codex · custom prompt |
🤖 Claude Code ReviewNow I have a thorough understanding of the PR. Let me compile the review. PR Review:
|
Greptile SummaryThis PR implements the Amsterdam Key changes:
Confidence Score: 2/5
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Transaction enters VM] --> B{Fork >= Amsterdam?}
B -- No --> C[Charge full intrinsic gas\nfrom gas_remaining]
B -- Yes --> D[Split intrinsic gas:\nregular_gas + state_gas]
D --> E[Set state_gas_reservoir\n= excess above TX_MAX_GAS_LIMIT]
E --> F[Deduct reservoir from gas_remaining\nso GAS opcode ≤ TX_MAX_GAS_LIMIT]
F --> G[EVM Execution]
G --> H{State gas needed?}
H -- Yes --> I[increase_state_gas:\ndraw from reservoir first]
I --> J{Reservoir empty?}
J -- No --> K[Deduct from reservoir]
J -- Yes --> L[Spill to gas_remaining\nOOG if exhausted]
G --> M{Child call reverts?}
M -- Yes --> N[Restore reservoir:\nnew_reservoir += child_state_gas_used\nstate_gas_used = snapshot]
G --> O{CREATE collision?}
O -- Yes --> P[collision_escrowed_gas += gas_limit\nblock accounting excludes this]
G --> Q[finalize_execution]
Q --> R[gas_used -= state_gas_reservoir\nunused reservoir returned to sender]
R --> S[regular_gas = gas_used - state_gas\nblock_gas_used = max sum_regular, sum_state]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| eprintln!( | ||
| "[DBG] tx_gas_limit={} gas_used_before_reservoir={} reservoir={} state_gas_used={} state_gas_refund={} gas_remaining={} result={:?}", | ||
| vm.env.gas_limit, | ||
| ctx_result.gas_used, | ||
| vm.state_gas_reservoir, | ||
| vm.state_gas_used, | ||
| vm.intrinsic_state_gas_refund, | ||
| vm.current_call_frame.gas_remaining, | ||
| ctx_result.result | ||
| ); |
There was a problem hiding this comment.
Debug eprintln! left in production code
This [DBG] eprintln! statement fires for every Amsterdam+ transaction and will spam stderr in any environment running Amsterdam blocks. It was almost certainly left in during development of EIP-8037 and should be removed before merging.
| eprintln!( | |
| "[DBG] tx_gas_limit={} gas_used_before_reservoir={} reservoir={} state_gas_used={} state_gas_refund={} gas_remaining={} result={:?}", | |
| vm.env.gas_limit, | |
| ctx_result.gas_used, | |
| vm.state_gas_reservoir, | |
| vm.state_gas_used, | |
| vm.intrinsic_state_gas_refund, | |
| vm.current_call_frame.gas_remaining, | |
| ctx_result.result | |
| ); | |
| if vm.env.config.fork >= Fork::Amsterdam { | |
| ctx_result.gas_used = ctx_result.gas_used.saturating_sub(vm.state_gas_reservoir); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/vm/levm/src/hooks/default_hook.rs
Line: 184-193
Comment:
Debug `eprintln!` left in production code
This `[DBG]` `eprintln!` statement fires for every Amsterdam+ transaction and will spam stderr in any environment running Amsterdam blocks. It was almost certainly left in during development of EIP-8037 and should be removed before merging.
```suggestion
if vm.env.config.fork >= Fork::Amsterdam {
ctx_result.gas_used = ctx_result.gas_used.saturating_sub(vm.state_gas_reservoir);
}
```
How can I resolve this? If you propose a fix, please make it concise.
crates/vm/levm/src/vm.rs
Outdated
| eprintln!( | ||
| "[STATE_GAS] depth={} spill={} reservoir={} gas_remaining={} state_gas_used={}", | ||
| self.current_call_frame.depth, | ||
| spill, | ||
| self.state_gas_reservoir, | ||
| self.current_call_frame.gas_remaining, | ||
| self.state_gas_used | ||
| ); |
There was a problem hiding this comment.
Debug eprintln! left in production code
This [STATE_GAS] eprintln! fires every time the state gas reservoir is exhausted and spills into regular gas, which will happen during normal execution. This is debug instrumentation that should be removed (or replaced with a tracing::trace! call if the diagnostic information is genuinely needed).
| eprintln!( | |
| "[STATE_GAS] depth={} spill={} reservoir={} gas_remaining={} state_gas_used={}", | |
| self.current_call_frame.depth, | |
| spill, | |
| self.state_gas_reservoir, | |
| self.current_call_frame.gas_remaining, | |
| self.state_gas_used | |
| ); | |
| // Charge spill from gas_remaining first — if OOG, return early | |
| // without mutating reservoir or state_gas_used (matches EELS behavior) | |
| self.current_call_frame.increase_consumed_gas(spill)?; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/vm/levm/src/vm.rs
Line: 548-555
Comment:
Debug `eprintln!` left in production code
This `[STATE_GAS]` `eprintln!` fires every time the state gas reservoir is exhausted and spills into regular gas, which will happen during normal execution. This is debug instrumentation that should be removed (or replaced with a `tracing::trace!` call if the diagnostic information is genuinely needed).
```suggestion
// Charge spill from gas_remaining first — if OOG, return early
// without mutating reservoir or state_gas_used (matches EELS behavior)
self.current_call_frame.increase_consumed_gas(spill)?;
```
How can I resolve this? If you propose a fix, please make it concise.
Remove two eprintln! calls left from a debug session: - increase_state_gas: logged on every state gas spill (per-opcode) - on_execution_end: logged on every Amsterdam tx exit (per-tx)
- Add STATE_GAS_NEW_ACCOUNT, STATE_GAS_STORAGE_SET, STATE_GAS_AUTH_TOTAL constants replacing 10 runtime checked_mul calls in SSTORE, CREATE, CREATE2, CALL, SELFDESTRUCT, and intrinsic gas handlers - Remove redundant fork >= Amsterdam check inside increase_state_gas (all callers already guard), replace with debug_assert - Merge BAL item_count into validate_block_access_list_hash loop, eliminating a second O(BAL) pass for sequential block validation - Pre-size per-tx FxHashSet for accessed_accounts tracker (capacity 16)
Switch BlockAccessListRecorder to IndexSet/IndexMap for touched_addresses, storage_reads, initial_balances, and addresses_with_initial_code. This enables length-based tx-level checkpoint/restore (tx_checkpoint/tx_restore) instead of cloning the entire recorder per tx attempt in block building. - Per-tx cost goes from O(recorder_size) clone to O(addresses) snapshot - build() sorts at finalization time for canonical BAL ordering (one-time) - Checkpoint taken after set_bal_index flush to preserve previous tx's net-zero-to-read conversions
Add March 10 status update reflecting bal-devnet-3-dev progress: EIP-8037 nested revert fixes, BAL parallel execution improvements, bal@v5.3.0 fixtures, Amsterdam consume-engine hive tests in CI. Fix PR review comments: reference link ordering, bullet point count, and EIP-8037 table status columns. Update all references from closed PR #6293 to tracking PR #6216.
In the parallel execution path, per-tx BAL validation ran before system contract calls, causing BAL errors to mask SYSTEM_CONTRACT_CALL_FAILED for blocks with failing system contracts and inconsistent BALs.
…lookup seed_db_from_bal previously iterated ALL BAL accounts for every parallel tx, making it O(N*M) where N=txs and M=accounts. Now uses a precomputed sorted list of (min_block_access_index, account_idx) with binary search to only visit accounts that have changes at indices <= max_idx.
1 participant
Motivation
Tracking branch for Amsterdam bal-devnet-3 alignment.
EIP Changes
EIP-7708: Rename Selfdestruct event to Burn
0xcc16f5...(BURN_EVENT_TOPIC)create_selfdestruct_logtocreate_burn_logEIP-7928: BAL size cap
validate_block_access_list_sizein validation.rs (3 call sites in blockchain.rs)bal_items > gas_limit / GAS_BLOCK_ACCESS_LIST_ITEM(no system allowance or per-tx base cost)EIP-8024: Branchless normalization + EXCHANGE extension
decode_singleuses(x + 145) % 256instead of branchingdecode_pairuses XOR0x8Finstead of conditional subtraction0x00-0x4Fto0x00-0x51(two new swappable pairs)EIP-8037: State Creation Gas Cost Increase
Full implementation of the reservoir model:
gas_limitbeyondTX_MAX_GAS_LIMITblock.gas_used = max(sum(regular), sum(state))per EIP-7778cumulative_gas_used= post-refund total (what user pays)increase_state_gasreordered: charge first, mutate counters only on success (matches EELS)new_reservoir = current_reservoir + child_state_gas_used(matches EELSincorporate_child_on_error)gas_used=0on collision)max(intrinsic_regular, calldata_floor) > TX_MAX_GAS_LIMIT)BAL Parallel Execution Validation
Comprehensive BAL validation for the parallel block execution path:
validate_bal_withdrawal_index: verify withdrawal/request-extraction index entries against actual post-withdrawal statevalidate_tx_execution: smarter PART B check comparing absent-from-BAL accounts with pre-execution state to distinguish warm-access artifacts from genuine missing entriesvalidate_ordering: storage conflict check (slot in both storage_changes and storage_reads)validate_header_bal_indices: validate BAL indices don't exceed max valid index for the blockaccessed_accountstracker inGeneralizedDatabaseto detect extraneous BAL "pure-access" entriesblock_gas_used = max(regular, state)matching sequential pathCI Changes
Roadmap
Selfdestructevent toBurnwith updated topic hashChecklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync.