🤖 Kimi Code Review

Review Summary

This PR implements a proof-of-concept for the Native Rollups EXECUTE precompile (EIP-8079). The implementation is well-structured and follows good practices, but there are several areas that need attention.

Critical Issues

1. Security: Mutex Poisoning Handling (Critical)

File: crates/vm/levm/src/db/guest_program_state_db.rs
Lines: 34-82

All database methods use Mutex::lock() and convert PoisonError to a custom database error. However, this could lead to inconsistent state if the mutex is poisoned. Consider using Mutex::lock().unwrap_or_else(|e| e.into_inner()) to recover from poisoned state, or document why poisoning should be treated as fatal.

2. Overflow Handling (High)

File: crates/vm/levm/src/execute_precompile.rs
Lines: 195-198, 201-204, 250-253, 290-294

Multiple unchecked arithmetic operations could overflow:

• cumulative_gas_used and block_gas_used additions
• amount_in_wei multiplication in process_withdrawals
• Balance additions in apply_deposit

Use checked_add consistently and handle overflow errors properly.

3. State Consistency Risk (High)

File: crates/vm/levm/src/execute_precompile.rs
Lines: 140-150

The state transitions are applied back to the GuestProgramState after each block, but there's no atomicity guarantee. If execution fails mid-way, the state could be partially updated. Consider batching all state updates at the end.

Code Quality Issues

4. Error Handling Inconsistency (Medium)

File: crates/vm/levm/src/execute_precompile.rs
Lines: 45-50, 58-63

The custom_err helper creates VMError::Internal, but some errors (like state root mismatches) should probably be VMError::PrecompileError to distinguish between internal failures and verification failures.

5. Gas Cost Underestimation (Medium)

File: crates/vm/levm/src/execute_precompile.rs
Line: 45

EXECUTE_GAS_COST = 100_000 is likely too low for re-executing entire blocks. This should be dynamically calculated based on the actual work performed.

6. Unnecessary Mutex Usage (Low)

File: crates/vm/levm/src/db/guest_program_state_db.rs

Consider if GuestProgramState could be modified to use interior mutability (e.g., RefCell) instead of Mutex, since this is single-threaded execution.

Minor Issues

7. Code Duplication

File: crates/vm/levm/src/execute_precompile.rs
Lines: 58-63

The custom_err function is simple enough to inline, or could be a macro to reduce boilerplate.

8. Feature Flag Inconsistency

File: crates/vm/levm/src/precompiles.rs
Lines: 259, 346-350

The EXECUTE precompile is marked as active since Paris, but this seems inconsistent with it being a new EIP. Consider using a more appropriate fork.

9. Documentation

File: docs/vm/levm/native_rollups.md

The documentation is excellent, but should mention the security considerations around mutex poisoning and overflow handling.

Recommendations

1. Fix overflow handling throughout the codebase
2. Improve error categorization to distinguish between verification failures and internal errors
3. Add proper gas metering based on actual execution cost
4. Consider atomic state updates to prevent partial failures
5. Add fuzz testing for the arithmetic operations
6. Document security assumptions around mutex usage

The implementation is solid for a PoC, but these issues should be addressed before production use.

Automated review by Kimi (Moonshot AI) · custom prompt

🤖 Codex Code Review

Here are the key findings from the diff, ordered by severity, with file/line references.

Findings

• Consensus/validation gap: execute_block does not validate transactions_root, receipts_root, withdrawals_root, or ommers against the block body. Only gas_used is checked. This allows malformed blocks to pass as long as state root matches, which is not sufficient for L1/L2 block validity. Consider validating all header roots before executing or after executing with computed roots. crates/vm/levm/src/execute_precompile.rs:118
• Incorrect missing-account semantics: GuestProgramStateDb::get_account_state errors on missing accounts. EVM semantics treat missing accounts as empty; this can cause valid executions to fail unless the witness includes explicit empty accounts. The test works around this by inserting Bob/Charlie, which hides the issue. Return AccountState::default() instead of error. crates/vm/levm/src/db/guest_program_state_db.rs:25
• Precompile always reverts: execute_precompile charges gas and then always returns an error because calldata parsing is unimplemented. Once native-rollups is enabled, calls to address 0x0101 will always revert and consume gas, which is surprising for a “registered” precompile. Consider gating is_precompile/dispatch behind a runtime flag or returning a structured “not implemented” success code only in tests. crates/vm/levm/src/execute_precompile.rs:40, crates/vm/levm/src/precompiles.rs:333
• Gas accounting ambiguity: Both cumulative_gas_used (using report.gas_spent) and block_gas_used (using report.gas_used) are tracked, but only block_gas_used is validated and cumulative_gas_used is unused. If gas_used vs gas_spent differ (refunds), ensure the correct one matches header.gas_used and remove the unused accumulator to avoid future mistakes. crates/vm/levm/src/execute_precompile.rs:154

Lower‑risk / maintainability

• is_precompile returns true for EXECUTE regardless of fork; if this should be fork‑gated (or EIP‑specific), include an explicit fork check to avoid accidental activation across chains. crates/vm/levm/src/precompiles.rs:302

If you want, I can suggest concrete patch changes for the missing‑account behavior and header root validation.

Automated review by OpenAI Codex · custom prompt

🤖 Claude Code Review

Based on my review of PR #6186, here is my comprehensive code review:

PR Review: EXECUTE Precompile for Native Rollups (EIP-8079 PoC)

Summary

This PR implements a proof-of-concept EXECUTE precompile for Native Rollups (EIP-8079) that verifies L2 state transitions by re-executing them inside L1's EVM. The implementation is well-structured for a PoC and demonstrates the core concept effectively.

Critical Issues

1. Mutex Poisoning Not Properly Handled ⚠️ HIGH PRIORITY

File: crates/vm/levm/src/execute_precompile.rs:126-131

The code locks the mutex multiple times in a loop without recovering from poison errors:

db.state
    .lock()
    .map_err(|e| custom_err(format!("Lock poisoned: {e}")))?
    .apply_account_updates(&account_updates)

Issue: If apply_account_updates() panics, the mutex becomes poisoned and all subsequent blocks will fail. This breaks atomicity across block execution.

Recommendation: Either:

• Use a single lock acquisition for all blocks (refactor to avoid multiple lock/unlock cycles)
• Or use PoisonError::into_inner() to recover the mutex guard and handle poisoning gracefully
• Consider if Mutex is the right choice here - RefCell might be more appropriate since execution should be single-threaded

2. State Root Verification Race Condition ⚠️ MEDIUM PRIORITY

File: crates/vm/levm/src/execute_precompile.rs:133-139

let final_root = db
    .state
    .lock()
    .map_err(|e| custom_err(format!("Lock poisoned: {e}")))?
    .state_trie_root()
    .map_err(|e| custom_err(format!("Failed to compute final state root: {e}")))?;

Issue: The state is accessed through Arc<GuestProgramStateDb> which is shared. Between line 131 (releasing lock) and line 134 (acquiring lock again), there's no guarantee state hasn't been modified.

Recommendation: Hold the lock for the entire verification sequence or ensure exclusive access to the state during execution.

3. Gas Validation Semantics Unclear 🔍 MEDIUM PRIORITY

File: crates/vm/levm/src/execute_precompile.rs:217-219

ethrex_common::validate_gas_used(block_gas_used, &block.header)
    .map_err(|e| custom_err(format!("Gas validation failed: {e}")))?;

Issue: The precompile validates that block_gas_used matches the block header's gas_used field. However:

• For L2 verification on L1, should the precompile re-calculate gas or trust the header?
• What happens if the provided block header has an incorrect gas_used value?

Recommendation: Add a comment clarifying whether this is validating the L2 block's claimed gas usage or just checking internal consistency.

Security Concerns

4. No Gas Limit Enforcement on Precompile Execution ⚠️ HIGH PRIORITY

File: crates/vm/levm/src/execute_precompile.rs:59

increase_precompile_consumed_gas(EXECUTE_GAS_COST, gas_remaining)?;

Issue: The precompile charges a flat 100k gas upfront but then executes entire blocks with potentially millions of gas worth of transactions. A malicious actor could submit a block that takes very long to execute (DoS attack) for only 100k gas.

Recommendation: For production:

• Implement metered gas consumption during block execution
• Consider a gas-per-transaction multiplier
• Set maximum block gas limits for the precompile
• Document this as a known limitation in the PoC

5. Account State Not Found Returns Generic Error 🔍 MEDIUM PRIORITY

File: crates/vm/levm/src/db/guest_program_state_db.rs:36-38

.ok_or(DatabaseError::Custom(format!(
    "Account {address} not found in witness"
)))

Issue: Missing accounts cause immediate failure. In standard EVM execution, accessing a non-existent account should return an empty/default account, not an error.

Recommendation: Return Ok(AccountState::default()) for missing accounts instead of erroring. This matches EVM semantics where all addresses implicitly exist as empty accounts.

6. Deposit Balance Overflow Uses String Error 🔍 LOW PRIORITY

File: crates/vm/levm/src/execute_precompile.rs:295

.ok_or("Deposit would overflow balance")?;

Issue: Inconsistent error handling - other overflows use VMError::Internal(InternalError::Overflow).

Recommendation: Use consistent error types: ok_or(Box::new(VMError::Internal(InternalError::Overflow)) as Box<dyn std::error::Error>)?

Code Quality Issues

7. Inefficient State Trie Hashing on Deposit Application 🔍 LOW PRIORITY

File: crates/vm/levm/src/execute_precompile.rs:103-110

for deposit in &deposits {
    apply_deposit(&mut guest_state, deposit).map_err(|e| {
        custom_err(format!(
            "Failed to apply deposit to {}: {e}",
            deposit.address
        ))
    })?;
}

Issue: Each apply_deposit call modifies the state trie, potentially triggering trie recalculations. The trie is then hashed again at line 94 (initial root) and 134 (final root).

Recommendation: Batch trie operations if possible, or document that trie caching prevents performance issues.

8. Unused Variable cumulative_gas_used ⚠️ MEDIUM PRIORITY

File: crates/vm/levm/src/execute_precompile.rs:165, 204-206

let mut cumulative_gas_used = 0_u64;
// ...
cumulative_gas_used = cumulative_gas_used
    .checked_add(report.gas_spent)
    .ok_or(VMError::Internal(InternalError::Overflow))?;

Issue: The variable is tracked but never used. This could indicate:

• Missing logic that should validate cumulative gas
• Confusion between gas_spent vs gas_used semantics

Recommendation: Either use this value for validation or remove it. If keeping it, document why both cumulative_gas_used and block_gas_used are needed.

9. execute_block Mutates GeneralizedDatabase But Doesn't Return New State 🔍 MEDIUM PRIORITY

File: crates/vm/levm/src/execute_precompile.rs:152-222

fn execute_block(block: &Block, db: &mut GeneralizedDatabase) -> Result<(), VMError>

Issue: The function modifies GeneralizedDatabase state, but the caller also needs to extract state transitions via get_state_transitions() and manually apply them. This is error-prone.

Recommendation: Consider returning the AccountUpdates directly from execute_block to make the data flow more explicit.

Rust Best Practices

10. Unnecessary Clone in Arc Pattern 🔍 LOW PRIORITY

File: crates/vm/levm/src/execute_precompile.rs:116

let db_dyn: Arc<dyn crate::db::Database> = db.clone();

Issue: Creating a trait object Arc from a concrete Arc requires clarification. Is this intentional coercion?

Recommendation: Add a comment explaining why db (which is Arc<GuestProgramStateDb>) is cloned and cast to Arc<dyn Database>, or use Arc::clone(&db) for explicitness.

11. Custom Error Helper Should Use impl Into<String> 🔍 LOW PRIORITY

File: crates/vm/levm/src/execute_precompile.rs:68-70

fn custom_err(msg: String) -> VMError {
    VMError::Internal(InternalError::Custom(msg))
}

Recommendation: Make it more ergonomic:

fn custom_err(msg: impl Into<String>) -> VMError {
    VMError::Internal(InternalError::Custom(msg.into()))
}

This would allow passing both String and &str without requiring .to_string() at call sites.

12. Magic Number for Success Return Value 🔍 LOW PRIORITY

File: crates/vm/levm/src/execute_precompile.rs:148

Ok(Bytes::from(vec![0x01]))

Recommendation: Define a constant:

const EXECUTE_SUCCESS_BYTE: u8 = 0x01;
// ...
Ok(Bytes::from(vec![EXECUTE_SUCCESS_BYTE]))

Testing Concerns

13. Test Only Covers Happy Path 🔍 MEDIUM PRIORITY

File: crates/vm/levm/tests/native_rollups.rs

Issue: The test only validates successful execution. Missing test cases:

• Invalid pre-state root (should fail early)
• Invalid post-state root (should fail after execution)
• Transaction that reverts
• Out-of-gas transaction
• Invalid signature
• Withdrawal processing
• Empty block
• Multiple blocks
• Deposit overflow scenarios

Recommendation: Add negative test cases and edge cases before merging.

Documentation

14. Missing Safety Documentation for Mutex 🔍 LOW PRIORITY

File: crates/vm/levm/src/db/guest_program_state_db.rs:17-19

/// Uses a `Mutex` for interior mutability because `GuestProgramState` methods
/// require `&mut self` (they lazily populate caches like `account_hashes_by_address`).

Recommendation: Document thread-safety guarantees. Since LEVM execution is single-threaded, clarify whether concurrent access is a concern or if RefCell would be more appropriate.

Minor Issues

15. Inconsistent Fork Activation - execute_precompile.rs:266: EXECUTE is listed as active_since_fork: Paris but EIP-8079 is not part of any fork yet. Consider using a more appropriate marker or comment.

16. Missing Blob Gas Validation - The code processes blob_gas_used and excess_blob_gas but doesn't validate blob transaction constraints for type-3 transactions.

17. Code Format - execute_precompile.rs:15: Using std::cmp::min instead of importing min from the prelude is less idiomatic.

Positive Observations ✅

• Good separation of concerns between the precompile logic, DB adapter, and registration
• Comprehensive overflow checking with checked_add, checked_mul throughout
• Good test documentation - the test clearly explains what it's doing
• Feature flag isolation - properly gates experimental code
• Clear error messages with context
• Well-structured documentation in native_rollups.md

Recommendations Summary

Before merging:

1. ✅ Fix mutex poisoning handling (Issue docs: add milestones #1)
2. ✅ Clarify/fix state access race condition (Issue chore: create project structure #2)
3. ✅ Document gas validation semantics (Issue build: add Github actions boilerplate #3)
4. ✅ Document DoS risk from flat gas cost (Issue docs: update milestones. #4)
5. ✅ Fix account not found behavior (Issue feat: add basic RPC api. #5)
6. ✅ Add negative test cases (Issue Add jwt authentication to engine api #13)

Future improvements:

• Implement proper gas metering
• Add calldata deserialization
• Consider using RefCell instead of Mutex if execution is single-threaded
• Add more comprehensive test coverage

Overall, this is a solid PoC implementation that demonstrates the concept well. The main concerns are around thread safety, gas accounting, and test coverage.

Automated review by Claude (Anthropic) · custom prompt

Lines of code report

Total lines added: 2245
Total lines removed: 44
Total lines changed: 2289

+---------------------------------------------------------------+-------+------+
| File                                                          | Lines | Diff |
+---------------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/build_l2.rs                                 | 478   | +31  |
+---------------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/command.rs                               | 669   | +22  |
+---------------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/deployer.rs                              | 1833  | +312 |
+---------------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/initializers.rs                          | 432   | +78  |
+---------------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/mod.rs                                   | 12    | +2   |
+---------------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/options.rs                               | 1232  | +128 |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs                        | 2512  | +28  |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/common/common.rs                                | 23    | +1   |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/common/merkle_tree.rs                           | 61    | +61  |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/common/src/merkle_tree.rs                    | 1     | -42  |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/common/src/messages.rs                       | 206   | +4   |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/l2.rs                                        | 13    | +2   |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/networking/rpc/l2/messages.rs                | 87    | -2   |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/networking/rpc/l2/mod.rs                     | 6     | +1   |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/networking/rpc/l2/native_withdrawal_proof.rs | 129   | +129 |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/networking/rpc/rpc.rs                        | 222   | +2   |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/sdk/src/sdk.rs                               | 1234  | +6   |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/mod.rs                             | 274   | +2   |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/native_rollup/block_producer.rs    | 382   | +382 |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/native_rollup/l1_advancer.rs       | 218   | +218 |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/native_rollup/l1_watcher.rs        | 222   | +222 |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/native_rollup/mod.rs               | 89    | +89  |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/native_rollup/types.rs             | 28    | +28  |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/tracing.rs                       | 178   | +20  |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/db/guest_program_state_db.rs        | 68    | +68  |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/db/mod.rs                           | 150   | +2   |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/execute_precompile.rs               | 383   | +383 |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/lib.rs                              | 22    | +2   |
+---------------------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/precompiles.rs                      | 1202  | +22  |
+---------------------------------------------------------------+-------+------+

Benchmark Results Comparison

No significant difference was registered for any benchmark run.

Superseded by #6418