feat: Finalize C API and regular API

[roninjin10]

Summary

This PR finalizes the C API and regular API for Guillotine EVM, providing comprehensive configuration options and a clean, ergonomic interface for both C and Zig consumers.

Key Changes

• Comprehensive Build Configuration: Added extensive build-time configuration options for EIPs, hardforks, memory management, tracer settings, and system contracts
• Enhanced EVM Configuration System: Implemented runtime parsing for EIP overrides and tracer presets with full type safety
• Streamlined Module Structure: Simplified root.zig to expose only essential types, removing redundant re-exports
• C API Improvements: Updated C API implementation with better alignment to simplified module structure
• Code Cleanup: Removed obsolete files and improved code formatting

Configuration Features

Build Options Added

• EIP override support (e.g., 3855:true,1153:false)
• Call/stack limits (max_input_size, max_call_depth, stack_size)
• Memory configuration (initial capacity, limits, arena settings)
• Tracer configuration with presets (disabled, debug, full)
• System contract toggles (beacon roots, historical hashes, validator deposits/withdrawals)
• SIMD vector length configuration
• Loop safety quota settings
• Block info compact types option

API Improvements

• Type-safe configuration parsing
• Build-time EIP customization
• Flexible tracer configuration
• Comprehensive memory management options

Files Changed

• build.zig - Expanded with organized configuration options
• src/evm_config.zig - Enhanced with parsing logic and build integration
• src/root.zig - Streamlined to essential exports
• src/root_c.zig - Updated C API implementation
• src/evm_c_api.zig - Code formatting improvements
• Removed obsolete src/main.zig and src/_test_utils/README.md

Test Plan

• Verify all build configurations compile successfully
• Test EIP override parsing with various combinations
• Validate tracer presets (disabled, debug, full)
• Confirm C API compatibility with existing consumers
• Run full test suite: zig build test
• Test with different optimization levels
• Verify system contract toggles work correctly

🤖 Generated with Claude Code

Note: This action was performed by Claude AI assistant, not @roninjin10 or @fucory

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Updated (UTC)
guillotine	Error		Oct 5, 2025 9:02am

[claude]

PR Review - C API and Configuration Finalization

Note: This action was performed by Claude AI assistant, not @roninjin10 or @fucory

Summary

This PR significantly improves the Guillotine EVM build configuration system and C API. The changes are well-structured and provide comprehensive configuration options. However, there are several issues that need to be addressed before merging.

---

Critical Issues

1. Incomplete TODO Comment (evm_config.zig:134)

Issue: Incomplete comment - sentence is cut off
Fix: Complete the comment or remove it

2. Dead Code Flagged for Removal (evm_config.zig:199-217)

Issue: According to CLAUDE.md zero tolerance policy: Commented code should use Git
Impact: Methods optimizeFast/optimizeSmall are still being called in fromBuildOptions() and are tested
Fix: Either remove entirely or implement properly

The current state violates the project zero tolerance for stubbed implementations.

3. Error Handling - Potential Silent Failures

src/evm_config.zig:245 - Missing validation in parseEipOverrides
Issue: If no colon is found, colon_pos remains 0, leading to incorrect parsing
Impact: Invalid EIP override format would silently produce wrong results
Fix: Add validation that colon was found

---

Major Issues

4. Missing Validation in parseTracerConfig

Issue: Invalid preset strings silently fall through to building from individual flags
Recommendation: Add a compile error for invalid presets

5. Build Options Not Tested

The PR adds extensive build options but does not add tests for:

• EIP override parsing edge cases
• Tracer preset parsing with invalid values
• Build option integration end-to-end

Recommendation: Add unit tests for parseEipOverrides and parseTracerConfig

---

Code Quality Issues

6. Memory Leak Risk in C API

root_c.zig:285-297: If EVM init fails after creating a new database, the database is leaked
Fix: Track whether we created the database and clean it up on EVM init failure

7. Magic Numbers

root_c.zig:74: last_error buffer size hardcoded to 512
Recommendation: Define as constant

8. Arena Growth Factor

build.zig:92: Default is 150% - standard is typically 200% for better amortization

---

Security Concerns

No security issues detected. The C API properly validates null pointers, uses bounds-checked slicing, and provides opaque handles.

---

Overall Assessment

Code Quality: 7/10
Test Coverage: 6/10
Documentation: 7/10
Security: 9/10

This is a solid improvement to the codebase, but the critical issues (especially the dead code and incomplete TODO) must be addressed before merging per the project zero-tolerance policy.

The C API design is clean and ergonomic. The configuration system is comprehensive and well-organized.

---

Suggested Next Steps

1. Address critical issues (dead code, incomplete TODO, validation)
2. Run zig build test to verify all tests pass
3. Add missing test coverage for parsing functions
4. Fix memory leak in C API error path

[claude]

Code Review - PR #835: Finalize C API and regular API

Note: This action was performed by Claude AI assistant, not @roninjin10 or @fucory

Summary

This PR adds comprehensive build-time configuration options for the Guillotine EVM. The changes are well-structured but there appear to be critical issues that need addressing before merging.

---

Critical Issues

1. Missing Build Options in build.zig
The fromBuildOptions() function references build options that don't exist in build.zig (eip_overrides, max_input_size, arena_growth_factor, enable_precompiles, vector_length, loop_quota, block_info_use_compact_types, system contract flags, tracer options).

According to the PR diff, these WERE added, but they're missing from the current codebase. This suggests the PR is incomplete.

2. Missing Parser Functions
The functions parseEipOverrides() and parseTracerConfig() are referenced in the PR description but don't exist in evm_config.zig.

3. Incomplete fromBuildOptions() Implementation
The current implementation doesn't use the new parsing functions or apply the new build options shown in the PR diff.

---

Significant Concerns

4. Breaking Changes to root.zig
Removes 120 lines of re-exports without migration guide or documentation. This breaks existing consumers.

5. Large C API Changes
root_c.zig has 510 additions, 166 deletions. Needs careful review for memory safety and FFI correctness.

---

Code Quality Issues

6. Missing Input Validation
parseEipOverrides() doesn't validate malformed input.

7. Magic Numbers
build.zig uses unexplained constants: 131072, 24576, 49152, 150.

8. Silent Failures
Invalid tracer presets fall through silently instead of raising compile errors.

9. Formatting Changes
Pure whitespace changes in evm_c_api.zig violate CLAUDE.md guidelines.

---

Positive Aspects

• Well-organized configuration grouping
• Type-safe comptime parsing
• Comprehensive coverage
• Good inline documentation

---

Test Coverage

Test plan has NO checkboxes marked. Before merging:

• Run: zig build && zig build test-opcodes
• Test EIP override parsing
• Test tracer presets
• Verify C API compatibility

---

Recommendation: DO NOT MERGE

The PR appears incomplete. Missing code from the diff needs to be added and tests must pass.

Before Merging (CRITICAL):

1. Verify all build.zig changes committed
2. Add parseEipOverrides() and parseTracerConfig()
3. Update fromBuildOptions()
4. Add input validation
5. Run full test suite
6. Verify C API compatibility

Next Steps:

1. Check git status on c-api branch
2. Verify all changes committed
3. Force-push if needed
4. Re-run CI/tests
5. Request re-review

[claude]

Code Review: C API and Build Configuration Finalization

Note: This action was performed by Claude AI assistant, not @roninjin10 or @fucory

I have reviewed PR #835 and identified several issues ranging from critical bugs to important improvements. Overall, the PR adds valuable configuration capabilities, but there are some serious issues that need to be addressed before merging.

---

Critical Issues

1. CRITICAL BUG: Memory leak in CApiState singleton (src/root_c.zig:83-97)

The CApiState.get_or_init() function has a serious memory leak. When the singleton is already initialized, it creates a new GPA allocator on the stack that is never cleaned up.

Fix: The GPA should be stored in the singleton struct itself, not created as a local variable. Every call after initialization leaks the GPA.

2. CRITICAL: Comptime parsing bug - colon not found case (src/evm_config.zig:238)

The parseEipOverrides function has undefined behavior when a colon is not found. If the input string does not contain a colon, colon_pos remains 0, leading to empty eip_str. This violates the Zero Tolerance policy.

Fix: Add comptime validation or use std.mem.indexOf with proper error handling.

---

Security Concerns

3. Security: Silent failure in tracer preset parsing (src/evm_config.zig:273-289)

The parseTracerConfig function silently falls through to default config when an invalid preset is provided. User types --tracer-preset=ful (typo), expects full tracing, but gets minimal tracing from individual flags. This is mission-critical as incorrect tracer config could hide bugs.

Fix: Add comptime error for invalid preset values.

4. Security: Missing validation in database operations (src/root_c.zig:155-230)

Database setter functions lack critical validation:

• No null pointer checks on address, balance, value, slot pointers
• No validation of code_len (could be larger than actual buffer)
• Generic error messages do not indicate which parameter failed

Fix: Add explicit null checks and bounds validation before dereferencing.

---

Code Quality and Best Practices

5. Violation: Removed TODO without addressing it (src/evm_config.zig:220-221)

The PR removes the TODO comment about dead code, but the code is NOT removed - it is enhanced with new parsing logic. This violates the project standard of addressing TODOs rather than hiding them.

Impact: The original concern about dead code is unresolved. Is fromBuildOptions() dead code or not?

6. Code organization: Inconsistent config assignment (src/evm_config.zig:309-368)

The eip_overrides is assigned to both config.eips.overrides AND config.eip_overrides. This appears to be duplicate state.

Fix: Clarify if both fields are needed or consolidate.

7. Naming clarity: CU256 is little-endian (src/root_c.zig:21-24)

The comment correctly states little-endian, but Ethereum typically uses big-endian for u256 values. Verify this is intentional.

8. Missing deinit for databases (src/root_c.zig:141-152)

When evm_create allocates a new database internally (if database param is null), there is no way for the caller to know they need to destroy it separately. This creates an unclear ownership model.

Fix: Document ownership or track internally-created databases for cleanup.

---

Performance Considerations

9. Performance: Comptime parsing allocates arrays (src/evm_config.zig:219-270)

The parseEipOverrides function iterates the string multiple times. Consider using std.mem.split for cleaner code.

10. Performance: String comparison in hot path (src/evm_config.zig:321-340)

getHardforkFromString uses linear string comparisons. Consider a comptime perfect hash or switch for better code generation.

---

Positive Aspects

1. Excellent organization - Build options are now logically grouped with clear comments
2. Type safety - Tracer configuration uses proper types instead of booleans
3. Good C API design - Opaque handles and clear separation of concerns
4. Comprehensive coverage - Addresses most configuration needs
5. Code cleanup - Removed obsolete files (main.zig, test utils README)

---

Test Coverage Verification Needed

Per CLAUDE.md: EVERY code change: zig build && zig build test-opcodes

The test plan in the PR description is incomplete. Has this been run? The PR should demonstrate passing tests.

---

Required Actions Before Merge

1. FIX CRITICAL: Memory leak in CApiState singleton
2. FIX CRITICAL: Comptime parsing undefined behavior
3. FIX SECURITY: Validate tracer preset input
4. FIX SECURITY: Add null pointer validation in C API
5. CLARIFY: Is fromBuildOptions() actually dead code per original TODO?
6. TEST: Add unit tests for parseEipOverrides with edge cases
7. TEST: Add unit tests for parseTracerConfig with invalid input
8. VERIFY: Run zig build && zig build test-opcodes per project standards

---

Recommendation

DO NOT MERGE until critical bugs are fixed. The memory leak and undefined behavior in comptime parsing are severe issues that could cause production problems. Once fixed, this will be a valuable addition to the codebase.

The security issues with validation should also be addressed to maintain the zero tolerance standard for this mission-critical financial infrastructure.

[claude]

Code Review - PR #835

Note: This action was performed by Claude AI assistant, not @roninjin10 or @fucory

Summary

This PR significantly expands build-time configuration options and simplifies the module structure. Overall direction is good, but there are critical issues that must be addressed before merging.

🔴 CRITICAL ISSUES

1. Build Failures
CI shows build-test: FAILURE. This violates zero tolerance policy. Run zig build test to fix.

2. Dead Code Warning Contradiction
src/evm_config.zig:220 has a TODO saying fromBuildOptions() should be removed, yet this PR expands it significantly. Remove the TODO or explain.

3. Breaking Changes to root.zig
The PR removes 120+ lines of public exports from src/root.zig, keeping only Evm and EvmConfig. This breaks external consumers importing Frame, Stack, Memory, Tracer, Database, BlockInfo, etc. Is this intentional? Needs documentation/migration guide.

4. Incomplete root_c.zig
Diff truncated at 720 lines - cannot fully review C API changes.

⚠️ MAJOR CONCERNS

5. Comptime String Parsing is Fragile
parseEipOverrides() has no error handling for invalid input. Silent failures possible.

6. Security - Loop Quota Defaults
Default null loop_quota may disable safety limits in production. CLAUDE.md requires 300M instruction limit.

7. Test Plan Incomplete
All checkboxes in PR description are unchecked.

✅ GOOD PARTS

• Organized build configuration with clear sections
• Tracer presets (disabled/debug/full) for better UX
• Type-safe configuration options

🎯 RECOMMENDATIONS

Must Fix (P1):

1. Fix all build/test failures
2. Remove contradictory TODO comment
3. Document breaking changes or restore exports
4. Complete test plan

Should Fix (P2):
5. Add error handling to parseEipOverrides()
6. Add unit tests for parsing functions
7. Document safety limits

Recommendation: DO NOT MERGE until builds pass and breaking changes are clarified.

Per CLAUDE.md: Zero tolerance for broken builds/tests.

[claude]

Code Review: PR #835 - Finalize C API and Regular API

Note: This action was performed by Claude AI assistant, not @roninjin10 or @fucory

Summary

This PR expands the build-time configuration system and cleans up the module structure. The organization is excellent, but there are critical implementation bugs that must be fixed before merging.

🚨 CRITICAL ISSUES

1. Incomplete fromBuildOptions() (src/evm_config.zig:223-261)
The old version does NOT call the new parseEipOverrides() or parseTracerConfig() functions. All new build flags (--evm-eip-overrides, --tracer-preset, etc.) will be completely ignored at runtime. The new parsing functions exist but are never called.

2. Dead Code TODOs
Lines 200-222 have TODO comments saying "Remove it" but the code is still present. Per CLAUDE.md: no commented/dead code.

3. Missing Build Options
These build options are never applied: max_input_size, arena_growth_factor, vector_length, loop_quota, block_info_config, enable_beacon_roots, enable_historical_block_hashes, enable_validator_deposits, enable_validator_withdrawals, tracer_config, enable_precompiles (hardcoded to true).

⚠️ HIGH PRIORITY

4. Root.zig Breaking Changes
Reduced from 128 lines to 8 lines, removing ALL public API exports (Frame, Stack, Memory, Tracer, Database, etc.). Massive breaking change - is this intentional? Migration guide needed?

5. C API Incomplete
Only 4 functions exported with TODO comment "Additional exports should be added". Does this match the "Finalize C API" claim?

✅ POSITIVE ASPECTS

• Excellent organization of build options
• No error swallowing (verified no catch {})
• Good documentation
• Type safety with appropriate integer sizes
• Comprehensive test coverage

🎯 VERDICT

Status: ❌ Changes Requested

The concept is excellent, but the implementation has critical bugs where new parsing functions aren't being called, making all new build flags non-functional.

Estimated fix time: 15-30 minutes to properly wire up fromBuildOptions().

---

Generated by Claude Code reviewing CLAUDE.md standards

[claude]

Note: This action was performed by Claude AI assistant, not @roninjin10 or @fucory

Code Review: PR #835

Critical Issues

1. Missing Implementation: parseEipOverrides() and parseTracerConfig() are referenced but not implemented
2. Dead Code: Lines 200-261 in src/evm_config.zig have TODO-marked dead code
3. No Build Verification: Test plan unchecked, needs zig build && zig build test-opcodes
4. Missing Tests: No tests for new parsing logic
5. Breaking Changes: root.zig removes many exports without documentation
6. No Validation: Build options accept dangerous values (0 for critical params)
7. Parsing Vulnerability: No overflow checking in EIP number parsing

Recommendations

1. Implement or remove parseEipOverrides/parseTracerConfig
2. Delete dead code
3. Run build verification
4. Add unit tests
5. Document breaking changes
6. Add input validation
7. Review root_c.zig for memory safety

Severity: HIGH - Should not merge until core functions implemented, dead code removed, tests added, and build verified.

[claude]

Note: This action was performed by Claude AI assistant, not @roninjin10 or @fucory

Code Review: PR #835 - Finalize C API and regular API

I've reviewed this PR and identified several issues that need attention before merging.

---

CRITICAL ISSUES

1. BUG: Incorrect Assignment in fromBuildOptions()

OLD CODE (evm_config.zig:238):
config.eips uses config.eip_overrides BEFORE it is parsed from build options, so it will always be empty.

NEW CODE (evm_config.zig:311):
Correctly uses the parsed eip_overrides variable.

Impact: EIP overrides were completely broken in old code. This PR fixes it!

2. VIOLATION: Dead Code TODOs (evm_config.zig:200-221)

Per CLAUDE.md Zero Tolerance: Commented code (use Git)

Found misleading TODOs at lines 200-221 saying 'This is either dead code or code that should be dead - Remove it' for optimizeFast(), optimizeSmall(), and fromBuildOptions().

Action Required: These functions ARE actively used (line 228-233), so REMOVE the misleading TODO comments.

3. POTENTIAL BUG: Missing Validation in parseEipOverrides()

The comptime parser (evm_config.zig:221-269) has no error handling:

• No validation that colon exists in segment
• No bounds checking on array access
• Silent failures if format is incorrect

Add @CompileError for invalid input format.

---

MODERATE ISSUES

4. Missing Build Options

Verify all new build options from build.zig are used in fromBuildOptions(). The new code looks correct but needs testing.

5. root.zig Streamlining Incomplete?

PR claims 'streamlined to essential exports' but file still has 180 lines of re-exports. Was this completed?

---

POSITIVE OBSERVATIONS

1. Excellent organization of build options with clear sectioning
2. Good type safety in configuration parsing with comptime evaluation
3. Comprehensive coverage of configuration options
4. Clean removal of obsolete files (main.zig, _test_utils/README.md)
5. Proper formatting fixes in evm_c_api.zig
6. Smart defaults for all build options

---

RECOMMENDATIONS

Before Merge:

1. MUST FIX: Remove misleading TODO comments from evm_config.zig:200-221
2. MUST TEST: Run zig build test and zig build test-opcodes - share results
3. SHOULD ADD: Error handling/validation in parseEipOverrides()
4. SHOULD DOCUMENT: New build options (especially --evm-eip-overrides format)

Assessment: Good work on API consolidation, but DO NOT MERGE until:

1. Dead code TODOs are resolved
2. parseEipOverrides() has proper error handling
3. Full test suite passes and results are shared

The configuration system improvements are valuable, but code quality must meet the project's zero-tolerance standards before merging.

Would you like me to help implement any of these fixes?