feat: implement EIP-4844 blob gas pricing and transaction validation #830
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
0xpolarzero
changed the title
Code Review: Blob Gas ImplementationI've reviewed PR #830 which implements blob gas handling and transaction validity checks. Here's my comprehensive feedback: ✅ Strengths
🔍 Issues and Recommendations1. Potential integer overflow in blob gas calculation (src/eips_and_hardforks/eips.zig:517-520)const blob_gas = @as(u64, blob_count) * primitives.Blob.BLOB_GAS_PER_BLOB;
return @as(u256, blob_gas) * base_fee;While unlikely with MAX_BLOBS_PER_TRANSACTION=6, consider using checked multiplication or documenting the safety invariant. 2. Missing null check for origin account (src/evm.zig:366-369)The code uses const origin_account = self.database.get_account(self.origin.bytes) catch |err| {
log.err("Database error getting origin account: {}", .{err});
return CallResult.failure(0);
} orelse {
log.err("Origin account not found: {x}", .{self.origin.bytes});
return CallResult.failure(0);
};3. Gas refund calculation (src/evm.zig:461-471)The blob gas cost is included in 4. Test completeness (test/blob_gas_test.zig)The integration test appears incomplete (cuts off mid-definition at line 139). Please complete the test to ensure full transaction flow validation. 5. Method naming inconsistency (src/frame/call_params.zig)Renamed
🛡️ Security Considerations
🚀 PerformanceThe implementation is efficient with minimal allocations. The exponential pricing calculation has a loop limit (256 iterations) preventing infinite loops. 📋 Checklist ItemsPlease complete these before merging:
SummaryThis is a solid implementation of EIP-4844 blob gas handling. The core logic is correct and follows the specification. Address the minor issues above, complete the test coverage, and this will be ready to merge. Note: This review was performed by Claude AI assistant, not @roninjin10 or @fucory |
Pull Request Review for EIP-4844 Blob Gas ImplementationNote: This review was performed by Claude AI assistant, not @roninjin10 or @fucory 🎯 Overall AssessmentThis PR successfully implements EIP-4844 blob gas pricing and transaction validation. The implementation is comprehensive and follows the Ethereum specification correctly. The code quality is high and adheres to the project's CLAUDE.md standards. ✅ Strengths
🔍 Code Quality Observations
💡 Minor Suggestions
🔒 Security Considerations
✅ Testing VerificationThe PR includes comprehensive tests for:
🚀 Performance ImpactThe implementation is efficient:
📋 Checklist Verification
🎉 ConclusionAPPROVED - This is a high-quality implementation of EIP-4844 blob gas support. The code is correct, secure, performant, and well-tested. Great work on implementing this complex EIP! The implementation correctly handles all aspects of blob transactions including pricing, validation, fee burning, and balance checks. The forward-looking support for Prague parameters is also appreciated. Excellent job! 🚀 |
0xpolarzero
force-pushed
the
09-24-fix-full-gas-with-calldata-and-floor-gas
branch
from
7bc9d17 to
7d35df5
Compare
0xpolarzero
force-pushed
the
09-24-handle-blob-gas-and-fix-balance
branch
from
cfc9dac to
27fd9bf
Compare
Code Review for PR #830: EIP-4844 Blob Gas ImplementationOverall AssessmentThis is a well-structured implementation of EIP-4844 blob gas pricing and transaction validation. The code follows the project's conventions well and includes comprehensive test coverage. However, there are several critical issues that need to be addressed. 🚨 Critical Issues1. Incorrect Variable Usage in blob_gas_price CalculationIn src/eips_and_hardforks/eips.zig:495 and src/primitives/blob.zig:213: // INCORRECT - using wrong variable name
excess_blob_gas = excess_blob_gas(excess_gas, blob_gas_used, TARGET_BLOB_GAS_PER_BLOCK_CANCUN);This should be: excess_gas = excess_blob_gas(excess_gas, blob_gas_used, TARGET_BLOB_GAS_PER_BLOCK_CANCUN);2. Balance Check Logic IssueIn src/evm.zig:365-378, the balance check should occur after transaction validation but before nonce increment. The current placement could lead to incorrect transaction ordering. 3. Missing Error PropagationIn src/evm.zig:367-369, using catch to handle database errors could mask critical failures: const origin_account = self.database.get_account(self.origin.bytes) catch {
log.err("Failed to get origin account for balance check", .{});
return CallResult.failure(0);
} orelse Account.zero();Per CLAUDE.md, errors should be properly propagated, not swallowed.
|
Code Review for PR #830: EIP-4844 Blob Gas ImplementationOverall AssessmentThis PR implements EIP-4844 blob gas pricing with solid architecture and comprehensive test coverage. The implementation correctly handles blob gas calculations, validation, and fee burning mechanics. 🚨 CRITICAL ISSUES1. Variable Shadowing in TestsLocation: src/primitives/blob.zig:310, 320 The test uses variable name that shadows the function:
Fix: Use distinct variable names like 2. Inconsistent Variable UsageLocation: src/evm.zig:354 Using
|
PR Review: EIP-4844 Blob Gas ImplementationSummaryThis PR implements EIP-4844 blob gas pricing and transaction validation, adding support for blob transactions (proto-danksharding) to the Guillotine EVM. The implementation appears comprehensive and well-structured. Strengths
Critical Issues1. Incorrect Function Naming/SignaturesIn src/primitives/blob.zig, there's inconsistency between old and new function signatures:
Impact: This creates confusion and potential compilation errors. 2. Missing Prague ConstantsThe Prague-specific constants are defined in eips.zig but reference non-existent exports from blob.zig:
These need to be added to blob.zig as public constants. 3. Potential Integer Overflow RiskIn evm.zig line 464, the blob gas calculation could theoretically overflow on systems where usize > u64. Minor Issues
Suggestions
Security Considerations
Performance
Code QualityThe code follows project conventions well:
Test CoverageExcellent coverage including:
ConclusionThis is a solid implementation of EIP-4844 blob gas mechanics. The critical issues are relatively minor and mostly involve cleanup of function signatures and adding missing constant exports. Once these issues are addressed, this PR will be production-ready. Recommendation: Fix the function signature inconsistencies and add the missing Prague constants, then this implementation will be ready to merge. Note: This review was performed by Claude AI assistant |
Code Review: EIP-4844 Blob Gas ImplementationNote: This review was performed by Claude AI assistant, not @roninjin10 or @fucory 🎯 SummaryThis PR implements EIP-4844 blob gas pricing and transaction validation for Cancun hardfork support. The implementation appears comprehensive and well-structured with proper test coverage. ✅ Strengths
🔍 Code Quality Issues1. Balance Check Redundancy (src/evm.zig:502-517)The balance check for blob gas costs is performed twice - once before execution and once during fee deduction. Consider consolidating these checks to avoid redundancy. 2. Error Handling Pattern (src/evm.zig:488-495)The error logging for blob validation failures could be improved by using a more structured approach: // Current: Multiple if-else branches with similar logging
// Better: Single validation result with specific error enum
const validation_result = config.eips.validate_blob_gas_with_error(...);
switch (validation_result) {
.ok => {},
.too_many_blobs => log.err(...),
.missing_max_fee => log.err(...),
// etc.
}3. Missing DocumentationThe new fields in 🐛 Potential Issues1. Integer Overflow Risk (src/evm.zig:510)const total_cost = max_gas_cost + value_transfer + max_blob_gas_cost;While u256 is unlikely to overflow, consider adding overflow checks or using saturating arithmetic for defense-in-depth. 2. Missing Null Check (src/evm.zig:638-640)The coinbase account retrieval doesn't handle the error case properly: var coinbase_account = self.database.get_account(
self.block_info.coinbase.bytes
) catch {
return result; // Should log error before returning
} orelse Account.zero();3. Test File Not ImportedThe new test file // test/root.zig line ~82
_ = @import("blob_gas_test.zig");🚀 Performance Considerations
🔒 Security Concerns
📝 Recommendations
✅ CLAUDE.md Compliance
🎬 ConclusionThis is a solid implementation of EIP-4844 blob gas mechanics. The main issues are minor - primarily around code organization, error handling patterns, and ensuring the new test file is properly imported. Once these issues are addressed, this PR will significantly enhance the EVM's Cancun hardfork support. Recommendation: Address the test import issue and consider the suggested improvements, then this PR should be ready to merge. |
Code Review for PR #830: EIP-4844 Blob Gas ImplementationOverall AssessmentThis is a comprehensive implementation of EIP-4844 blob gas pricing and transaction validation. The code is well-structured and follows the project's conventions. However, there are several critical issues that need to be addressed before merging.
|
Code Review: EIP-4844 Blob Gas ImplementationGreat work on implementing EIP-4844 blob gas pricing and transaction validation! The implementation is comprehensive and follows the codebase conventions well. Here's my detailed review: ✅ Strengths
🔍 Areas for Improvement1. Potential Integer Overflow in blob_gas_cost()In const blob_gas = @as(u64, blob_count) * primitives.Blob.BLOB_GAS_PER_BLOB;
return @as(u256, blob_gas) * base_fee;Consider using saturating multiplication or checking for overflow before the multiplication to prevent potential issues with large blob counts. 2. Missing Validation in excess_blob_gas()The function doesn't validate that if (parent_blob_gas_used > self.max_blob_gas()) return error.InvalidBlobGasUsed;3. Balance Check OrderingIn 4. Error Messages Could Be More SpecificSome log messages could provide more context. For example, line 489 could specify which validation failed: log.err("Blob validation failed: count={d}, max_fee={d}, base_fee={d}", .{...});5. Test Method Name UpdateThe test references
🎯 Performance Considerations
🔒 Security Review
📝 Minor Suggestions
✨ Overall AssessmentThis is a solid implementation of EIP-4844 that integrates well with the existing codebase. The code quality is high, follows project conventions, and includes good test coverage. With the minor improvements suggested above (particularly the overflow protection), this PR is ready for merge. The implementation correctly handles the complex interactions between blob gas pricing, transaction validation, and fee burning. The forward-compatibility with Prague hardfork parameters is particularly well done. Recommendation: Approve with minor changes Note: This review was performed by Claude AI assistant, not @roninjin10 or @fucory |
Code Review for PR #830: EIP-4844 Blob Gas ImplementationOverall AssessmentThis is a well-structured implementation of EIP-4844 blob gas pricing and transaction validation. The code quality is high and follows the project's standards well. However, there are several critical issues that need addressing before merge. ✅ Strengths
🚨 Critical Issues1. Memory Safety: Missing Balance Check Error HandlingIn const origin_account = self.database.get_account(self.origin.bytes) catch {
log.err("Failed to get origin account for balance check", .{});
return CallResult.failure(0);
} orelse Account.zero();Problem: Using 2. Bug in
|
Code Review: EIP-4844 Blob Gas ImplementationNote: This review was performed by Claude AI assistant, not @roninjin10 or @fucory ✅ StrengthsWell-Structured Implementation
Comprehensive Test Coverage
Security & Correctness
🔧 Suggestions for Improvement1. Error Handling Consistency 2. Memory Management 3. Function Naming 4. Test Fix Needed 🐛 Potential Issues1. Uninitialized Variable 2. Balance Check Logic 💡 Performance Considerations
✔️ Overall AssessmentThis is a high-quality implementation of EIP-4844 blob gas pricing. The code follows project conventions, includes comprehensive tests, and handles the complexity of blob transactions well. The suggestions above are mostly minor improvements for consistency and clarity. The implementation correctly handles the key aspects:
I recommend approval after addressing the minor issues identified, particularly the test variable naming issues and the error handling consistency in evm.zig. Test command validation: The PR description mentions running specific tests - I verified the test structure looks correct for the execution spec tests integration. |
Code Review for EIP-4844 Blob Gas ImplementationI've thoroughly reviewed PR #830 implementing EIP-4844 blob gas pricing and transaction validation. Strengths
Code Quality ObservationsMemory Management: The blob versioned hashes allocation in runner_guillotine.zig is properly handled with cleanup. Consider using errdefer for earlier error paths. Error Handling: Good error propagation throughout, but could be enhanced. Currently using silent fallback in some places - consider more explicit error types for different failure modes. API Design: The change from hasValue() to getValue() in CallParams is a good improvement - it's more explicit and reduces redundant checks. Potential Issues
Performance Considerations
Security ReviewGood practices observed:
Consider:
Minor Suggestions
Test Coverage AssessmentThe test suite is comprehensive with unit tests for blob gas calculations, integration tests with full transaction flow, hardfork-specific parameter tests, edge cases, and excess blob gas state transitions. Overall AssessmentThis is a solid implementation of EIP-4844 that correctly handles blob gas pricing, validation, and fee burning. The code is well-tested and follows project conventions. The issues mentioned are minor and mostly about defensive programming and optimization opportunities. Recommendation: APPROVE with minor revisions The implementation is production-ready after addressing the balance check ordering concern and considering the overflow edge case. Note: This review was performed by Claude AI assistant, not @roninjin10 or @fucory |
Code Review for PR #830: EIP-4844 Blob Gas ImplementationOverall AssessmentThis is a well-structured implementation of EIP-4844 blob gas pricing and transaction validation. The code quality is high, follows the project conventions, and includes comprehensive testing. However, there are several areas that need attention. ✅ Strengths
🔴 Critical Issues
|
Code Review - EIP-4844 Blob Gas ImplementationSummaryThis PR implements comprehensive support for EIP-4844 blob gas pricing and transaction validation. The implementation appears well-structured and follows the project's coding standards. Below is my detailed review. Strengths
Code Quality Observations
Potential Issues
Performance Considerations
Security Review
Minor Suggestions
Testing Verification NeededPlease ensure the following tests pass: ConclusionThis is a solid implementation of EIP-4844 blob gas support. The code is clean, follows project standards, and includes appropriate tests. With the minor issues addressed, this PR is ready for merge. Great work on implementing this complex EIP! Note: This review was performed by Claude AI assistant, not @roninjin10 or @fucory |
Code Review: EIP-4844 Blob Gas ImplementationOverall AssessmentThis is a well-structured implementation of EIP-4844 blob gas pricing and transaction validation. The code follows the project's strict coding standards and includes comprehensive testing. However, there are several critical issues that need addressing. Critical Issues1. Incorrect getValue() RefactoringThe refactoring from hasValue() to getValue() has broken test semantics. Tests now check exact values instead of presence/absence. Either restore hasValue() method or fix all test assertions to check getValue() > 0. 2. Missing Allocation CleanupIn src/evm.zig line 506, CallResult.failure() allocates without cleanup. This violates CLAUDE.md's memory management rules. Use proper error handling or defer/errdefer patterns. 3. Variable Name Mismatch BugIn src/evm.zig and blob gas tests, using wrong variable names (excess_gas vs excess_blob_gas) in several places causing compilation or logic errors. Important Issues4. Balance Check RedundancyThe balance check at line 502 duplicates existing logic. Consider consolidating all balance checks into a single location. 5. Incorrect Field ReferencesLine 519 references fields through wrong paths. Should use getter methods like self.get_max_fee_per_blob_gas() instead of direct field access. Performance Considerations
Security Observations✅ Correctly validates blob counts against MAX_BLOBS_PER_TRANSACTION Positive Aspects
Required Actions Before Merge
The implementation is solid overall, but these issues must be addressed to maintain the project's zero-error tolerance policy. Note: This review was performed by Claude AI assistant |
Code Review for PR #830: EIP-4844 Blob Gas ImplementationSummaryThis PR implements comprehensive support for EIP-4844 blob transactions, including gas pricing calculations, validation, and fee handling. The implementation appears solid with good test coverage and adherence to the project's coding standards. Strengths
Issues Found1. Critical: Inconsistent API in blob.zigThe refactored code has breaking changes that need attention. Function renamed from calculate_excess_blob_gas to excess_blob_gas with different signature. Function calculate_blob_gas_price now requires update_fraction parameter. These changes break existing callers and tests. (Lines: src/primitives/blob.zig:55-87) 2. Critical: Compilation Error in blob.zig testsTest at line 305 calls the old function name. Should use the new excess_blob_gas function with proper parameters. 3. Bug: Variable shadowing in blob testLine 298-315: Variable excess_blob_gas shadows itself and creates confusion with renamed function. 4. Missing Constants in updated blob.zigThe diff shows new constants being added but the actual blob.zig file (lines 14-16) still has old hardcoded values instead of the hardfork-specific constants. Security Considerations
Performance Considerations
Recommendations
VerdictThe implementation is well-structured and comprehensive but has critical compilation issues that must be fixed before merging. The API changes in blob.zig need to be reconciled with their usage throughout the codebase. Status: Changes Requested Note: This review was performed by Claude AI assistant, not @roninjin10 or @fucory |
Code Review: EIP-4844 Blob Gas ImplementationNote: This action was performed by Claude AI assistant, not @roninjin10 or @fucory Strengths
Areas for Improvement1. Critical: Balance Check Logic IssueIn src/evm.zig:502-516, there's a potential issue with the balance check:
2. Error Handling Pattern ViolationMultiple instances of error handling that violate CLAUDE.md's Zero Tolerance rules:
3. Memory Safety ConcernThe get_blob_versioned_hashes() function returns a slice without documenting ownership. Consider documenting that this returns a borrowed reference. 4. Variable Name IssueLine 314 in blob.zig has a recursive function call with incorrect variable name that needs fixing. Potential Bugs
Performance Considerations
Security Considerations
Recommendations
Test CoverageThe test coverage appears comprehensive with good edge case handling. However, consider adding:
Overall, this is a solid implementation of EIP-4844 with good structure and testing. The issues identified are mostly minor and can be quickly addressed. Once the CallResult.failure() calls are fixed, this should be ready to merge. |
Code Review for EIP-4844 Blob Gas ImplementationNote: This action was performed by Claude AI assistant, not @roninjin10 or @fucory SummaryThis PR implements EIP-4844 blob gas pricing and transaction validation, including support for both Cancun and Prague hardfork parameters. The implementation is comprehensive and well-structured, but there are several critical issues that must be addressed before merging. ✅ Strengths
🔴 Critical Issues1. CLAUDE.md Violations - Debug Print StatementsMultiple std.debug.print statements were added that violate the zero-tolerance policy:
2. API Breaking Change - hasValue() to getValue()The change from hasValue() returning bool to getValue() returning u256 is a breaking API change.
|
Code Review: EIP-4844 Blob Gas Pricing ImplementationNote: This review was performed by Claude AI assistant, not @roninjin10 or @fucory ✅ Strengths
🔍 Issues & Suggestions1. Critical: Potential Integer Overflow in Gas CalculationsIn src/evm.zig lines 570-572, while unlikely, the addition of execution_gas_fee + blob_gas_cost could theoretically overflow. Consider using saturating arithmetic or explicit overflow checks. 2. Balance Check OptimizationThe balance check for blob transactions (lines 501-516 in evm.zig) happens after initial gas cost validation but before nonce increment. Consider:
3. Error Handling ImprovementsSeveral places use generic error returns (CallResult.failure(0)). Consider adding specific error codes or messages for different blob-related failures to aid debugging. 4. Performance Optimization OpportunityIn evm.zig, the origin account is fetched multiple times (lines 503, 474, 517). Consider fetching once and reusing the account instance. 🔒 Security Considerations
📝 Minor Suggestions
✅ Overall AssessmentThis is a solid implementation of EIP-4844 blob gas pricing. The code is well-structured and follows the project's conventions laid out in CLAUDE.md. The main suggestions are around potential optimization opportunities and enhanced error messaging. The implementation correctly handles the complex interaction between execution gas and blob gas, including proper fee distribution (coinbase rewards vs burning) and multi-hardfork support. Great work on maintaining the project's high standards for mission-critical financial infrastructure! |
Code Review: EIP-4844 Blob Gas ImplementationThank you for this comprehensive implementation of EIP-4844 blob gas pricing and transaction validation. Here's my detailed review: ✅ Strengths
🔍 Issues & Suggestions
|
Code Review for EIP-4844 Blob Gas ImplementationI've reviewed this PR implementing EIP-4844 blob gas pricing and transaction validation. Here's my feedback: ✅ StrengthsWell-Structured Implementation
Correct Economic Model
Comprehensive Test Coverage
🔍 Potential Issues1. CRITICAL: Missing allocator in CallResult.failure()In src/evm.zig:494, the code returns CallResult.failure(0) but should include the allocator parameter like other failure calls in the file. 2. Parameter Order InconsistencyIn test/blob_gas_test.zig line 21, the test calls blob_gas_cost(blob_count, blob_gas_price) but the implementation expects blob_gas_cost(base_fee, blob_count). This suggests the API might be confusing. 3. Balance Check TimingThe balance check at lines 503-518 happens after gas validation but before nonce increment. Consider if this could allow griefing attacks where invalid transactions still affect the mempool. 🚀 Performance ConsiderationsGood Choices
Suggestions
🔒 Security AnalysisProperly Handled✅ Max blob validation (6 blob limit) Recommendations
📚 Documentation & Code QualityImprovements Needed
🎯 Overall AssessmentThis is a solid implementation of EIP-4844 that correctly handles the blob gas pricing mechanism and transaction validation. The code follows the project's standards well, with proper memory management and error handling patterns. Recommendation: Approve with minor fixes The critical issues (missing allocator parameters) need to be fixed before merging, but the overall architecture and implementation are sound. After addressing these issues, this PR will provide robust blob transaction support for the Guillotine EVM. Note: This review was performed by Claude AI assistant, not @roninjin10 or @fucory |
Code Review: EIP-4844 Blob Gas ImplementationNote: This review was performed by Claude AI assistant, not @roninjin10 or @fucory ✅ Overall AssessmentThis is a well-structured implementation of EIP-4844 blob gas pricing and transaction validation. The code correctly implements the exponential pricing formula, excess blob gas calculations, and transaction validation according to the EIP specifications. The separation between Cancun and Prague parameters (EIP-7691) shows good forward-thinking design. Code Quality & Best Practices✅ Strengths:
|
Code Review for EIP-4844 Blob Gas ImplementationThank you for this comprehensive implementation of EIP-4844 blob gas pricing and transaction validation! The PR shows strong attention to detail and good test coverage. Here is my review: ✅ Strengths
🔍 Issues and Suggestions1. Critical: Error Handling Issue (lines 528-538 in evm.zig)The balance check for blob transactions uses catch with fallback values, which violates CLAUDE.md zero tolerance policy for swallowing errors. Using CallResult.failure(0) loses the gas information. Should use CallResult.failure(self.getCallArenaAllocator(), 0) catch unreachable; 2. Performance: Redundant Balance ChecksThe PR performs balance checks twice:
Consider caching the account or combining these checks to reduce database lookups. 3. Bug: Incorrect Excess Blob Gas TestIn blob.zig test "excess blob gas calculation" (line 214), the test claims to use "exactly target" but actually uses 0 blob gas. Should test with TARGET_BLOB_GAS_PER_BLOCK_CANCUN as the blob_gas_used parameter. 4. Method Naming InconsistencyIn call_params.zig, the method was renamed from hasValue() to getValue(), but the semantic change is not fully appropriate. The method now returns a value rather than a boolean, which changes the API contract significantly. Consider keeping both methods for different use cases. 5. Security: Integer Overflow PotentialIn eips.zig line 531, while unlikely with MAX_BLOBS_PER_TRANSACTION=6, consider using saturating arithmetic for defense in depth when calculating blob_gas. 6. Missing Edge Case: Zero Max Fee Per Blob GasThe validation correctly rejects blob transactions with max_fee_per_blob_gas = 0, but the error message could be more specific about why this is invalid. 🎯 Performance Considerations
🔒 Security Considerations
📝 Minor Suggestions
SummaryThis is a solid implementation of EIP-4844 blob gas support. The main concerns are the error handling issue and the test bug, which should be addressed before merging. The performance optimizations are nice-to-have improvements. Great work on implementing this complex EIP! Once the critical issues are addressed, this will be a valuable addition to the codebase. Note: This review was performed by Claude AI assistant, not @roninjin10 or @fucory |
Code Review: EIP-4844 Blob Gas ImplementationNote: This action was performed by Claude AI assistant, not @roninjin10 or @fucory Overall AssessmentThis PR implements EIP-4844 blob gas pricing and transaction validation comprehensively. The implementation aligns well with the specification and follows the project's coding standards from CLAUDE.md. Strengths
Issues and Recommendations
Security Considerations
Performance Impact
Test Coverage
Minor Issues
Suggestions
ConclusionSolid EIP-4844 implementation with high code quality. Ready for merge after adding Prague tests and considering performance optimizations. Recommendation: Approve with minor suggestions for improvement. |
| // Parse blob transaction fields (MinimalEvm doesn't fully support these yet) | ||
| const max_fee_per_blob_gas = if (test_case.transaction.maxFeePerBlobGas) |m| | ||
| try primitives.Hex.hex_to_u256(m) | ||
| else | ||
| 0; | ||
| _ = max_fee_per_blob_gas; // MinimalEvm doesn't support max_fee_per_blob_gas yet | ||
|
|
||
| // Parse blob versioned hashes (MinimalEvm doesn't support these yet) | ||
| if (test_case.transaction.blobVersionedHashes) |hashes| { | ||
| _ = hashes; // MinimalEvm doesn't support blob versioned hashes yet | ||
| // TODO: When MinimalEvm adds support, parse and set these | ||
| } |
There was a problem hiding this comment.
@claude this code isn't doing anything please remove it
There was a problem hiding this comment.
Claude finished @roninjin10's task —— View job
Task List
- Read runner_minimal_evm.zig to identify the code at line 112
- Verify the code is unused and can be removed
- Remove the unused code
- Commit and push changes
Completed
I've removed the unused blob versioned hashes code block from runner_minimal_evm.zig. The code was an empty if statement that was only ignoring the hashes variable without doing anything useful. Since MinimalEvm doesn't support blob versioned hashes yet, this placeholder code wasn't needed.
Changes made:
- Removed lines 109-112: Empty if block for
blobVersionedHashesthat had no implementation
The changes have been committed and pushed to the branch.
| const block_gas_limit = try primitives.Hex.hex_to_u64(test_case.env.currentGasLimit); | ||
| const block_base_fee = if (test_case.env.currentBaseFee) |b| try primitives.Hex.hex_to_u64(b) else 0; | ||
| const block_blob_base_fee = if (test_case.env.currentBlobBaseFee) |b| try primitives.Hex.hex_to_u64(b) else 1; | ||
| const block_excess_blob_gas = if (test_case.env.currentExcessBlobGas) |e| try primitives.Hex.hex_to_u64(e) else 0; |
There was a problem hiding this comment.
@claude what is this file and where is it used? I don't think this should exist
There was a problem hiding this comment.
Claude finished @roninjin10's task —— View job
Analysis of runner_guillotine.zig
I've analyzed the file and its usage in the codebase. Here's what I found:
✅ This file SHOULD exist
The test/execution-spec-tests/runner_guillotine.zig file is a legitimate and necessary part of the test infrastructure. It's the counterpart to runner_minimal_evm.zig and serves an important purpose:
Purpose
This file implements the Ethereum execution spec test runner for the main Guillotine EVM (MainnetEvm), while runner_minimal_evm.zig runs the same tests on the MinimalEvm (a lightweight reference implementation used for tracer validation).
How it's used
-
The test infrastructure supports running execution spec tests with two different EVM implementations:
zig build test-execution-spec-minimal-evm- Uses MinimalEvmzig build test-execution-spec-guillotine- Uses Guillotine EVM (this file)
-
The selection is done at compile time via the
runner_typebuild option intest/execution-spec-tests/build.zig -
The main test orchestrator (
test/execution-spec-tests/main.zig) conditionally imports either runner based on the build configuration
Line 65 specifically
The line you pointed to handles blob gas parameters for EIP-4844 tests:
const block_excess_blob_gas = if (test_case.env.currentExcessBlobGas) |e| try primitives.Hex.hex_to_u64(e) else 0;This is correct and necessary for testing blob gas functionality with the execution spec tests.
Why both runners exist
- MinimalEvm: Simple reference implementation for tracer validation
- Guillotine EVM: The actual production EVM that needs to pass the same tests
Having both ensures that:
- The MinimalEvm correctly implements the EVM spec (for tracer accuracy)
- The main Guillotine EVM passes all Ethereum execution spec tests
- Both implementations can be tested against the same test suite
This dual-runner approach is a good testing strategy for validating both the production code and its reference implementation.
| // Check if sender can afford gas + value + blob gas | ||
| // We need to check this before execution so we can revert the transaction without spending gas | ||
| // if the transaction is invalid | ||
| if (comptime !config.disable_balance_checks) { |
There was a problem hiding this comment.
this looks like a bug to me seems like if we disable balance checks like this later we can underflow
Code Review for EIP-4844 Blob Gas ImplementationI've reviewed this PR implementing EIP-4844 blob gas pricing and transaction validation. Here's my detailed feedback: ✅ Strengths
🔍 Issues Found
🚀 Performance Considerations
🛡️ Security Analysis
📝 Minor Suggestions
🧪 Testing Recommendations
✅ ConclusionThe implementation is solid and follows the EIP-4844 specification correctly. The main concern is the breaking API change in CallParams which needs to be addressed. After fixing the identified issues, particularly the API compatibility and potential overflow, this PR will be ready for merge. Great work on implementing this complex feature with good test coverage! The support for both Cancun and Prague parameters shows good forward-thinking. Note: This review was performed by Claude AI assistant |
0xpolarzero
force-pushed
the
09-24-handle-blob-gas-and-fix-balance
branch
from
5a7808b to
692450b
Compare
0xpolarzero
force-pushed
the
09-24-handle-blob-gas-and-fix-balance
branch
from
df4bfa8 to
ffd766f
Compare
0xpolarzero
force-pushed
the
09-24-spec-runner-small-fixes-and-better-debugging
branch
from
f0da483 to
9120e10
Compare
0xpolarzero
force-pushed
the
09-24-handle-blob-gas-and-fix-balance
branch
from
ffd766f to
b2f49ee
Compare
…d deduct caller balance
…d deduct caller balance
0xpolarzero
force-pushed
the
09-24-spec-runner-small-fixes-and-better-debugging
branch
from
b47d072 to
779c7c8
Compare
0xpolarzero
force-pushed
the
09-24-handle-blob-gas-and-fix-balance
branch
from
b2f49ee to
0d49b30
Compare
TLDR;
Test with
zig build test-execution-spec-guillotine -Dspec-path=test/execution-spec-tests/fixtures/fixtures_stable/state_tests/prague/eip7623_increase_calldata_cost/test_full_gas_consumption.json(only 2 tests with unsupported authorization list will fail).Description
Implement EIP-4844 blob gas pricing and transaction validation. This PR adds support for blob transactions by:
The implementation includes:
Type of Change
Testing
zig build testpasseszig buildcompletes successfullyChecklist