feat: add EIP-7702 #1112
feat: add EIP-7702 #1112
Conversation
WalkthroughThis pull request introduces widespread changes to integrate block timestamp information into transaction signer creation and processing. Many calls to the MakeSigner function now include a block’s timestamp, enhancing transaction signing and validation. New EIP‑7702 features are added, including a SetCodeTx type and authorization handling methods with JSON serialization support. Additionally, ChainConfig is extended with Euclid fork time parameters, and the test suite and Makefile are updated to cover these new features and scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant B as Block
participant M as MakeSigner
participant T as Transaction
participant S as Signer
B->>M: Provide block.Number & block.Time
M->>S: Create signer instance using number & timestamp
T->>S: Request transaction signing (SetCodeTx if applicable)
S-->>T: Return signed transaction
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
eth/gasprice/gasprice.go (1)
315-322: Add explicit error handling for block retrieval.The error from
BlockByNumbershould be checked before proceeding with block operations.Apply this diff to improve error handling:
func (oracle *Oracle) getBlockValues(ctx context.Context, blockNum uint64, limit int, ignoreUnder *big.Int, result chan results, quit chan struct{}) { block, err := oracle.backend.BlockByNumber(ctx, rpc.BlockNumber(blockNum)) - if block == nil { + if err != nil { select { case result <- results{nil, err}: case <-quit: } return } + if block == nil { + select { + case result <- results{nil, fmt.Errorf("block not found")}: + case <-quit: + } + return + }
🧹 Nitpick comments (7)
core/state_transition.go (1)
334-342: Validating code authorizations for non-creation transactions.
Restricting SetCodeAuthorizations to non-creation transactions avoids conflicts with contract creation logic. The separate check for empty authorizations is helpful, though clarifying error messages for both conditions may improve UX even more.core/tx_pool.go (1)
2181-2216: Consider using more efficient data structures for authorities.
The slices.Contains and slices.Index calls run in O(n). This is likely acceptable if the number of authorities remains small, but a map-based approach could be more scalable. Also consider lowering log severity if “Authority with untracked tx” can happen during normal operation.internal/ethapi/api.go (1)
1413-1428: SetCodeTxType support
Adding baseFee and computing GasPrice is appropriate. Confirm that yParity usage doesn’t lose the 27/28 distinction and ensure the AuthorizationList is valid before use.core/types/gen_authorization.go (1)
36-75: Robust UnmarshalJSON for required fields
The explicit nil checks are great. However, there is a mismatch: the error mentions 'yParity' while JSON field is 'v'. Consider aligning the field names in errors to avoid confusion.core/error.go (1)
114-121: Consider adding error codes for better error handling.While the state transition errors are well-defined, consider adding unique error codes to each error for better error handling and logging. This would help with error tracking and debugging.
Example implementation:
// EIP-7702 state transition errors. // Note these are just informational, and do not cause tx execution abort. var ( - ErrAuthorizationWrongChainID = errors.New("EIP-7702 authorization chain ID mismatch") - ErrAuthorizationNonceOverflow = errors.New("EIP-7702 authorization nonce > 64 bit") - ErrAuthorizationInvalidSignature = errors.New("EIP-7702 authorization has invalid signature") - ErrAuthorizationDestinationHasCode = errors.New("EIP-7702 authorization destination is a contract") - ErrAuthorizationNonceMismatch = errors.New("EIP-7702 authorization nonce does not match current account nonce") + ErrAuthorizationWrongChainID = errors.New("EIP-7702-01: authorization chain ID mismatch") + ErrAuthorizationNonceOverflow = errors.New("EIP-7702-02: authorization nonce > 64 bit") + ErrAuthorizationInvalidSignature = errors.New("EIP-7702-03: authorization has invalid signature") + ErrAuthorizationDestinationHasCode = errors.New("EIP-7702-04: authorization destination is a contract") + ErrAuthorizationNonceMismatch = errors.New("EIP-7702-05: authorization nonce does not match current account nonce")core/blockchain.go (1)
1509-1511: Consider performance and concurrency implications.The parallel signature recovery with timestamp parameter:
- May have performance implications due to additional timestamp validation.
- Could introduce race conditions if timestamp handling is not thread-safe.
- Should be tested under high load conditions.
Consider adding benchmarks and stress tests to verify performance characteristics.
core/types/transaction.go (1)
487-490: Consider using pre-allocated slice for better performance.The
appendoperation on a zero-length slice can cause unnecessary allocations.Apply this diff to pre-allocate the slice:
-auths := make([]common.Address, len(setcodetx.SetCodeAuthorizations)) +auths := make([]common.Address, 0, len(setcodetx.SetCodeAuthorizations))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
accounts/abi/bind/backends/simulated.go(3 hunks)accounts/external/backend.go(1 hunks)cmd/evm/internal/t8ntool/execution.go(1 hunks)cmd/evm/internal/t8ntool/transaction.go(2 hunks)cmd/evm/internal/t8ntool/transition.go(1 hunks)common/hexutil/json.go(2 hunks)core/bench_test.go(2 hunks)core/blockchain.go(1 hunks)core/chain_makers.go(1 hunks)core/error.go(1 hunks)core/rawdb/accessors_chain.go(1 hunks)core/state_prefetcher.go(1 hunks)core/state_processor.go(2 hunks)core/state_processor_test.go(1 hunks)core/state_transition.go(8 hunks)core/tx_pool.go(9 hunks)core/types/gen_authorization.go(1 hunks)core/types/l2trace.go(1 hunks)core/types/receipt.go(3 hunks)core/types/receipt_test.go(1 hunks)core/types/setcode_tx.go(1 hunks)core/types/transaction.go(5 hunks)core/types/transaction_marshalling.go(4 hunks)core/types/transaction_signing.go(4 hunks)core/vm/eips.go(2 hunks)core/vm/evm.go(7 hunks)core/vm/interpreter.go(1 hunks)core/vm/jump_table.go(1 hunks)core/vm/operations_acl.go(2 hunks)eth/api.go(1 hunks)eth/catalyst/api.go(1 hunks)eth/downloader/queue_test.go(1 hunks)eth/downloader/testchain_test.go(1 hunks)eth/fetcher/block_fetcher_test.go(1 hunks)eth/gasprice/gasprice.go(4 hunks)eth/state_accessor.go(1 hunks)eth/tracers/api.go(5 hunks)eth/tracers/api_test.go(1 hunks)eth/tracers/internal/tracetest/calltrace_test.go(2 hunks)eth/tracers/js/tracer.go(1 hunks)graphql/graphql.go(3 hunks)internal/ethapi/api.go(13 hunks)internal/ethapi/transaction_args.go(4 hunks)les/downloader/queue_test.go(1 hunks)les/downloader/testchain_test.go(1 hunks)les/fetcher/block_fetcher_test.go(1 hunks)les/odr_test.go(2 hunks)les/state_accessor.go(1 hunks)light/odr_test.go(1 hunks)light/odr_util.go(1 hunks)light/txpool.go(1 hunks)miner/scroll_worker.go(2 hunks)params/protocol_params.go(1 hunks)rollup/ccc/async_checker_test.go(1 hunks)rollup/tracing/tracing.go(2 hunks)tests/state_test_util.go(1 hunks)tests/transaction_test_util.go(1 hunks)
🔇 Additional comments (105)
core/types/setcode_tx.go (5)
36-47: Ensure clarity and coverage for delegation parsing.
The logic for ParseDelegation and AddressToDelegation appears correct, enforcing a prefix length of 3 plus 20 bytes for addresses. However, consider adding thorough unit tests (including invalid prefix lengths and boundary cases) to confirm correctness under all conditions.Would you like me to generate test stubs for these helper functions?
49-67: Struct design appears solid.
The SetCodeTx struct correctly groups all fields required by EIP‑7702, including the specialized slice for SetCodeAuthorizations. Storing BigInts in *uint256.Int is a good performance choice in this repository. No immediate issues found.
108-138: Validate negative scenarios in Authority recovery.
The signature validation and public key recovery logic look standard. Ensure test coverage includes invalid signatures, corrupted recovery IDs, and corner cases such as zeros in R or S.
140-214: Deep copy logic is correct, but be mindful of future nested slices.
The copy method thoroughly allocates new pointers for fields, preventing accidental aliasing. If SetCodeAuthorization grows more nested data fields in future, consider implementing a deeper copy procedure. The encode/decode RLP methods also look standard.
90-106: Minor docstring improvement and usage check.
The comment “SignSetCode creates a signed the SetCode authorization” has a small grammatical slip. Updating it to “SignSetCode creates a signed SetCode authorization” might be clearer. Also, ensure that all call sites properly handle potential errors from crypto.Sign.-// SignSetCode creates a signed the SetCode authorization. +// SignSetCode creates a signed SetCode authorization.To verify the usage, you could run a search for all references:
✅ Verification successful
🏁 Script executed:
#!/bin/bash rg -A 3 "SignSetCode"Length of output: 390
Docstring update is needed in SignSetCode
- The search confirms the function in core/types/setcode_tx.go still uses the outdated comment ("creates a signed the SetCode authorization").
- There is no evidence from the usage search that error handling of crypto.Sign is mismanaged; the error is correctly checked right after the call.
Please update the comment to:
// SignSetCode creates a signed SetCode authorization.core/vm/operations_acl.go (2)
220-225: New EIP-7702 gas variables mirror existing patterns.
Defining gasCallEIP7702 and other variants aligns well with the approach used for EIP-2929. No immediate concerns identified.
227-285: Extended call gas function is consistent with prior EIPs.
makeCallVariantGasCallEIP7702 follows the same design as EIP-2929, adding extra checks for delegation and charging cold/warm costs accordingly. Consider adding test coverage for scenarios with multiple delegations or misaligned code sizes to confirm edge cases.core/state_transition.go (5)
94-96: New interface method supports code authorizations.
Adding SetCodeAuthorizations to the Message interface is coherent with the EIP-7702 flow. No immediate issues spotted. Ensure that implementers of this interface either return nil or a valid slice as appropriate.
382-382: Intrinsic gas inclusion for authorizations.
setCodeAuthorizations are accounted in IntrinsicGas. This is likely per EIP‑7702 specs. Confirm that each authorization truly needs the extra CallNewAccountGas cost, as large authorization lists could inflate gas usage significantly.
418-435: Partial application of authorizations.
Currently, invalid authorizations are silently skipped instead of reverting. Evaluate whether partial success is acceptable or if failing one authorization should revert the entire transaction. This can significantly affect user expectations.
480-507: Robust signature and chain ID checks.
validateAuthorization properly compares chain IDs, checks for valid signatures, and ensures no conflicting on-chain code. However, consider preventing multiple authorizations from the same address with the same nonce in a single transaction if that case is undesired.
510-535: Properly updating state with EIP-7702 authorizations.
applyAuthorization updates the nonce and overwrites contract code with delegation. The refund for existing accounts is a neat optimization. Ensure test coverage includes repetitive authorizations, zero address clears, and negative scenarios (e.g., concurrency).core/types/transaction_signing.go (7)
40-44: Ensure blockTime parameter usage is consistent.
Adding the blockTime parameter to the MakeSigner function is a logical extension for checking EuclidV2 eligibility. However, confirm that all call sites are correctly updated to provide a valid blockTime and handle boundary cases (e.g., blockTime=0).
69-72: Validate EuclidV2Time check condition.
Returning a Euclid V2 signer if EuclidV2Time is non-nil may be misleading if the actual block time isn’t verified here. Make sure the chainConfig uses consistent logic for block time or an alternative check if needed.
100-100: Confirm unconditional Euclid V2 signer usage.
Always returning the Euclid V2 signer for any chainID might introduce unintended behavior if a chain isn’t actually Euclid-based. Consider restricting this or verifying chain support.
197-210: Implementation for SetCodeTxType looks correct.
The special-case logic for 0/1 as the recovery ID aligns with the specification for SetCodeTxType. Good job integrating it with the existing signer flow.
212-215: Equality check is straightforward.
Consistently compares chain IDs and maintains parity with other signers’ approach.
217-230: Signature value extraction is cohesive.
Appropriately delegates to the parent signer if the transaction isn’t a SetCodeTxType, reducing duplicated logic. Looks good.
232-252: Hash calculation is coherent.
The distinct hashing logic for SetCodeTxType closely matches the pattern for other transaction types, ensuring consistency in prefix usage.core/vm/evm.go (3)
27-27: Importing “core/types” is appropriate.
The new import is necessary for referencing delegation utilities in the EVM logic.
233-233: Consistently applying resolveCode/resolveKeccakCodeHash.
Updating the contract invocation and creation paths to call the new resolution methods for code/delegation is consistent. Verify that no usage site still calls StateDB.GetCode/GetKeccakCodeHash directly, to avoid missing the delegation logic.Also applies to: 301-301, 345-345, 401-401, 450-450
542-569: Single-level delegation resolution.
This design only follows one level of delegation. Consider whether multi-level delegation or cycle detection is needed. Repeated or cyclical delegation references may introduce unexpected behaviors.graphql/graphql.go (1)
230-230: Extended logic for SetCodeTxType.
Equating SetCodeTxType with DynamicFeeTxType for gas calculations is consistent if both share the same fee mechanics. Ensure the rest of the codebase also aligns with these assumptions (e.g., base fee usage, priority fees).Also applies to: 266-266, 281-281
core/tx_pool.go (8)
81-83: New error declaration looks good.
Declaring a dedicated error for account-limit violations makes the code more expressive.
98-101: Error for authority conflicts is adequately introduced.
The newly added error for conflicting authority usage provides clear feedback for invalid transactions.
809-827: Authorization conflict checks appear correct.
Calling knownConflicts to gather conflicting addresses helps prevent transaction collisions. Just ensure pool.mu remains locked for thread safety.
828-832: Mandatory authorization constraint looks good.
Requiring at least one tuple for SetCode transactions is consistent with the specification.
847-847: IntrinsicGas call update is aligned with new code authorizations.
Passing tx.SetCodeAuthorizations() ensures correct gas calculation for the new transaction type.
2029-2029: Field initialization is consistent.
Instantiating the new map in txLookup: auths is correct.
2128-2146: addAuthorities and removeAuthorities calls fit into the lifecycle.
These lines correctly integrate the new authority tracking whenever transactions are added or removed.
24-24: Confirm Go version compatibility for "slices" import.
Ensure your build environment uses Go 1.21 or above, since "slices" was introduced in Go 1.21.✅ Verification successful
🌐 Web query:
Which version of Go introduced the "slices" package in the standard library?💡 Result:
The `slices` package was introduced into the Go standard library with **Go 1.21** [8]. This release added generic functions for common slice operations like sorting, searching, and manipulation. Key details: - **Official inclusion**: The `slices` package became part of `std` in Go 1.21 after being tested in `golang.org/x/exp/slices` during earlier versions[5][8]. - **Key features**: - Sorting and binary search (`Sort`, `BinarySearch`)[5][7] - Slice manipulation (`Clip`, `Compact`, `Grow`)[9][10] - Comparisons (`Equal`, `Compare`)[8][10] Earlier versions like Go 1.18–1.20 only included experimental versions via `golang.org/x/exp/slices`[5][9]. Subsequent releases like Go 1.23 later enhanced it with iterator support[3]. Citations: - 1: https://pkg.go.dev/slices - 2: https://tip.golang.org/doc/go1.20 - 3: https://www.bytesizego.com/blog/golang-new-slice-map-features - 4: https://www.dolthub.com/blog/2024-12-20-collection-functions-in-go-1-23/ - 5: https://www.ardanlabs.com/blog/2023/08/golang-slices-binary-search.html - 6: https://www.ompluscator.com/article/golang/standard-lib-slices-part2/ - 7: https://www.ompluscator.com/article/golang/standard-lib-slices-part1/ - 8: https://tip.golang.org/doc/go1.21 - 9: https://pkg.go.dev/golang.org/x/exp/slices - 10: https://www.gopherguides.com/articles/golang-slices-package
Final Verification: Go Version Compatibility Confirmed
The web search confirms that the "slices" package was introduced in Go 1.21. No further modifications are necessary as long as the build environment is set to Go 1.21 or higher.
internal/ethapi/api.go (4)
891-891: Add blockTime to MakeSigner
Including the block’s timestamp helps ensure correctness for EIP-1559 logic. This looks good.
995-995: Use header.Time in MakeSigner
Passing the block timestamp aligns with the updated signature scheme. Good addition.
1392-1392: Verify handling of V for yParity
Using v.Sign() only distinguishes negative, zero, or positive. Verify that this behavior covers legacy V values (27 or 28) correctly.
1441-1445: Fallback for blockTime
If current is nil, blockTime remains zero. This might cause unexpected behavior. Consider a sensible default or a warning log.core/types/gen_authorization.go (1)
16-34: MarshalJSON ensures required fields
The approach for converting each field into a hex-friendly type is good. Consider checking for field bounds (e.g., address length) if invalid data might slip through.rollup/ccc/async_checker_test.go (1)
41-41: Include block.Time in signer
Adding the block timestamp to MakeSigner ensures accurate testing of EIP-1559 transactions.les/state_accessor.go (1)
56-56: LGTM! Block timestamp added to MakeSigner.The addition of
block.Time()to the MakeSigner call aligns with EIP-7702 requirements and maintains consistency with similar changes across the codebase.tests/transaction_test_util.go (1)
58-58: LGTM! SetCodeAuthorizations added to IntrinsicGas calculation.The addition of
tx.SetCodeAuthorizations()to the IntrinsicGas calculation correctly integrates EIP-7702's SetCode transaction type while maintaining existing error handling patterns.core/state_prefetcher.go (1)
58-58: LGTM! Block timestamp added to MakeSigner.The addition of
header.Timeto the MakeSigner call aligns with EIP-7702 requirements and maintains consistency with similar changes across the codebase.core/error.go (1)
109-112: LGTM! Clear message validation errors for EIP-7702.The message validation errors are well-defined and essential for preventing invalid transactions:
ErrEmptyAuthList: Prevents transactions with empty authorization listsErrSetCodeTxCreate: Prevents misuse of EIP-7702 transactions for contract creationcmd/evm/internal/t8ntool/transaction.go (2)
116-116: LGTM: Updated MakeSigner with timestamp parameter.The addition of timestamp parameter (0) aligns with the broader changes for EIP-7702 implementation across the codebase.
143-144: LGTM: Added SetCodeAuthorizations to intrinsic gas calculation.The addition of
tx.SetCodeAuthorizations()parameter toIntrinsicGasis consistent with EIP-7702 requirements for handling set-code transactions.core/types/l2trace.go (1)
148-150: LGTM: Updated NewTransactionData signature with blockTime parameter.The addition of
blockTimeparameter and its usage inMakeSignercall maintains consistency with the broader EIP-7702 implementation changes.les/downloader/testchain_test.go (1)
129-130: LGTM: Updated test to include block timestamp in MakeSigner.The test has been correctly updated to use
block.Time()in theMakeSignercall, maintaining consistency with the implementation changes.eth/downloader/testchain_test.go (1)
129-130: LGTM: Updated test to include block timestamp in MakeSigner.The test has been correctly updated to use
block.Time()in theMakeSignercall, maintaining consistency with the implementation changes.core/state_processor.go (2)
193-193: Block timestamp added to transaction signing.The addition of
header.TimetoMakeSignermaintains consistency with the changes in theProcessfunction.
99-99: Block timestamp added to transaction signing.The addition of
header.TimetoMakeSigneraligns with EIP-7702 requirements for including block timestamps in transaction validation.Let's verify that all transaction signing operations consistently use the block timestamp:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for consistent usage of block timestamp in MakeSigner calls. # Test: Search for MakeSigner calls. Expect: All calls should include block timestamp. rg -A 2 'MakeSigner\(' | grep -v 'header.Time'Length of output: 11439
Consistent usage of block timestamps in production transaction signing verified
Our investigation shows that in production code every MakeSigner call uses a block timestamp—whether from header.Time, block.Time(), or equivalent. (Note that some test files, such as cmd/evm/internal/t8ntool/transaction.go and core/bench_test.go, pass 0 as the timestamp, which appears intentional for testing purposes and does not affect production behavior.)
core/vm/eips.go (2)
41-41: EIP-7702 activator added.The addition of EIP-7702 to the activators map enables its gas cost modifications.
276-282: Gas cost modifications for call operations.The function updates gas costs for CALL, CALLCODE, STATICCALL, and DELEGATECALL operations according to EIP-7702 specifications.
Let's verify that the gas cost functions are properly implemented:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for the implementation of EIP-7702 gas cost functions. # Test: Search for gas cost function implementations. Expect: All referenced functions should be defined. rg -A 5 'gasCallEIP7702|gasCallCodeEIP7702|gasStaticCallEIP7702|gasDelegateCallEIP7702'Length of output: 1060
Gas cost modifications are correctly implemented.
The output confirms that all EIP‑7702 gas cost functions (gasCallEIP7702, gasCallCodeEIP7702, gasStaticCallEIP7702, gasDelegateCallEIP7702) are defined in core/vm/operations_acl.go and properly referenced in core/vm/eips.go. No issues were found.
eth/state_accessor.go (1)
183-183: Block timestamp added to transaction signing.The addition of
block.Time()toMakeSignermaintains consistency with other transaction signing operations.accounts/external/backend.go (1)
221-221: SetCodeTxType added to dynamic fee transactions.The addition of
SetCodeTxTypeto the dynamic fee transaction case ensures consistent gas pricing behavior for the new transaction type.Let's verify that SetCodeTxType is consistently handled across the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for consistent handling of SetCodeTxType. # Test: Search for SetCodeTxType handling. Expect: All transaction type checks should include SetCodeTxType. rg -A 5 'SetCodeTxType'Length of output: 5331
Subject: Verified: Consistent Handling of SetCodeTxType Across the Codebase
The search results confirm that SetCodeTxType is consistently integrated in all relevant parts of the codebase. In particular:
Transaction Processing:
- In accounts/external/backend.go, dynamic fee transactions now include SetCodeTxType alongside DynamicFeeTxType.
- In core/tx_pool.go, explicit checks for SetCodeTxType ensure proper validation.
Signature & Marshalling:
- In core/types/transaction_signing.go and core/types/transaction_marshalling.go, SetCodeTxType is adequately handled for signature verification and marshalling.
- core/types/receipt.go and core/types/transaction.go include SetCodeTxType in both receipt handling and type definition.
API and GraphQL Integration:
- The endpoints in internal/ethapi/api.go and internal/ethapi/transaction_args.go as well as transaction processing in graphql/graphql.go reflect the updated type handling.
Based on the above evidence, the inclusion of SetCodeTxType is applied consistently throughout the codebase.
core/bench_test.go (2)
133-133: Consider using realistic timestamps in benchmarks.Similar to the previous instance, using a hardcoded timestamp of
0might not provide realistic benchmark results if transaction signing behavior is timestamp-dependent.
88-89: Verify the impact of using hardcoded timestamp in benchmarks.The hardcoded timestamp value of
0inMakeSignermight not accurately represent real-world scenarios where block timestamps are non-zero.Run the following script to check if there are any timestamp-dependent validations in the signer:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for timestamp-dependent validations in signer code ast-grep --pattern 'func ($name *Signer) ($method) { $$$ time $$$ $$$ }'Length of output: 80
I'm generating additional shell scripts to inspect the definition of MakeSigner and how the timestamp parameter is used. This will help assess if hardcoding the timestamp to 0 has any impact in terms of validations or logic.
🏁 Script executed:
#!/bin/bash # Search for the definition of MakeSigner with surrounding context. rg -n -A 10 -B 3 "func MakeSigner(" .Length of output: 122
🏁 Script executed:
#!/bin/bash # Identify the first Go file containing the definition of MakeSigner and inspect it for references to "timestamp". file=$(rg --files-with-matches "func MakeSigner(" -t go | head -n 1) if [ -n "$file" ]; then echo "Inspecting file: $file" rg -n "timestamp" "$file" fiLength of output: 171
I'm generating new shell scripts using an escaped pattern to correctly search for the definition of MakeSigner and any references to the timestamp parameter. This will help us verify whether hardcoding 0 for the timestamp might impact any validations in the signer.
🏁 Script executed:
#!/bin/bash # Search for the MakeSigner definition with context using an escaped parenthesis. rg -n -A 10 -B 3 "func MakeSigner\(" -tgo .Length of output: 1137
🏁 Script executed:
#!/bin/bash # Identify the first Go file containing the definition of MakeSigner and inspect it for any use of "timestamp" file=$(rg --files-with-matches "func MakeSigner\\(" -t go | head -n 1) if [ -n "$file" ]; then echo "Inspecting file: $file" rg -n "timestamp" "$file" fiLength of output: 316
Signer Benchmark Timestamp Review
Our investigation shows that the signer code (in core/types/transaction_signing.go) simply forwards the provided blockTime value to the chain configuration functions (e.g. config.IsEuclidV2). There are no explicit validations or adjustments on the timestamp inside the signer code. In benchmarks, passing a hardcoded value of 0 only fixes the branch decision according to the chain configuration. While this may limit the benchmarking scenario to a specific signer branch, it does not trigger any error conditions or validations.
• The MakeSigner function uses the blockTime value only for branch selection based on chain configuration (e.g. config.IsEuclidV2(blockTime)).
• No timestamp-dependent validations were found in the signer code.Given this, the hardcoded 0 in benchmarks does not impact correctness, though you may wish to document or consider parameterizing it if tests need to reflect a wider range of conditions.
core/vm/interpreter.go (1)
77-78: LGTM! Addition of EuclidV2 instruction set follows the established pattern.The implementation correctly adds support for EuclidV2 chain rules by assigning the appropriate instruction set.
eth/catalyst/api.go (1)
164-164: LGTM! Block timestamp correctly included in signer creation.The change properly incorporates the block's timestamp when creating the signer, maintaining consistency with the updated signer interface.
light/odr_util.go (1)
178-178: LGTM! Block timestamp correctly included in receipt field derivation.The change properly incorporates the block's timestamp when deriving receipt fields, maintaining consistency with the updated receipt derivation interface.
internal/ethapi/transaction_args.go (3)
58-59: LGTM: New field for SetCodeTx authorizations.The addition of
AuthorizationListfield to support EIP-7702's SetCodeTx type is well-documented and properly typed.
264-284: LGTM: Comprehensive SetCodeTx handling.The implementation properly handles the SetCodeTx case with all required fields:
- Checks for AuthorizationList presence
- Handles AccessList correctly
- Uses uint256 for numeric fields
- Includes all necessary transaction parameters
255-255: Verify authorization list handling in NewMessage.The authorization list is now passed to
types.NewMessage, but we should verify that upstream code properly handles this parameter.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check NewMessage implementation for authorization list handling ast-grep --pattern 'func NewMessage($$$) types.Message { $$$ }'Length of output: 67
I'll now run a more general search using grep to locate the implementation of NewMessage and verify how the authorization list parameter is handled.
🏁 Script executed:
#!/bin/bash # Searching for the implementation of NewMessage in the codebase. rg -n "func NewMessage" .Length of output: 310
I'll now inspect the implementation of NewMessage to verify how the authorization list parameter (
setCodeAuthorizations) is handled. We need to ensure that the parameter passed from upstream is correctly stored and used by the Message type. I'll run a script to print a snippet of the NewMessage function implementation.
🏁 Script executed:
#!/bin/bash # Print a snippet of the NewMessage function implementation to inspect authorization list handling sed -n '770,900p' core/types/transaction.goLength of output: 3551
Authorization List Handling in NewMessage is Properly Implemented
The NewMessage constructor in core/types/transaction.go directly assigns the provided authorization list (setCodeAuthorizations) to the Message struct, and the accompanying getter returns it unchanged. Upstream code in internal/ethapi/transaction_args.go passes the list correctly. There are no issues with how the authorization list is handled.
light/odr_test.go (1)
203-203: LGTM: Updated signer creation with timestamp.The addition of
header.TimetoMakeSigneraligns with EIP-7702 requirements for including block timestamp in transaction signing.common/hexutil/json.go (2)
231-234: LGTM: Well-defined U256 type.The U256 type is properly defined as a wrapper around uint256.Int with clear documentation.
241-259: Robust JSON unmarshaling implementation.The UnmarshalJSON implementation includes:
- Strict string format checking
- Proper handling of empty strings
- Clear error messages
cmd/evm/internal/t8ntool/execution.go (1)
120-120: LGTM: Consistent signer creation with timestamp.The addition of
pre.Env.TimestamptoMakeSignermaintains consistency with EIP-7702 changes across the codebase.params/protocol_params.go (1)
92-92: LGTM: Gas cost constant for EIP-7702 auth tuple.The gas cost value of 12500 for
TxAuthTupleGasis reasonable and consistent with other similar operations in the protocol.tests/state_test_util.go (1)
358-359: LGTM: Updated message creation for EIP-7702 support.The addition of the nil parameter for auth tuples is consistent with the EIP-7702 implementation.
core/chain_makers.go (1)
153-156: LGTM: Added Time() accessor for block timestamp.The method is well-implemented and provides necessary access to block timestamp for EIP-7702 functionality.
eth/gasprice/gasprice.go (1)
323-324: LGTM: Improved signer handling in getBlockValues.Moving signer creation inside the method improves encapsulation and correctly uses block time for EIP-7702 compatibility.
eth/downloader/queue_test.go (1)
50-50: LGTM! Correctly updated MakeSigner call.The change properly includes the block timestamp in the MakeSigner call, maintaining consistency with EIP-7702 requirements.
les/downloader/queue_test.go (1)
50-50: LGTM! Correctly updated MakeSigner call.The change properly includes the block timestamp in the MakeSigner call, maintaining consistency with EIP-7702 requirements.
cmd/evm/internal/t8ntool/transition.go (1)
244-244: LGTM! Correctly updated MakeSigner call.The change properly includes the prestate environment timestamp in the MakeSigner call, maintaining consistency with EIP-7702 requirements.
eth/tracers/internal/tracetest/calltrace_test.go (2)
170-171: LGTM! Correctly updated MakeSigner call in test setup.The change properly includes the test context timestamp in the MakeSigner call, maintaining consistency with EIP-7702 requirements.
281-281: LGTM! Correctly updated MakeSigner call in benchmark setup.The change properly includes the test context timestamp in the MakeSigner call, maintaining consistency with EIP-7702 requirements.
core/types/receipt.go (2)
239-239: LGTM! Added support for SetCodeTxType.The
SetCodeTxTypeis correctly added to the list of supported transaction types in thedecodeTypedmethod, maintaining consistency with other transaction types likeDynamicFeeTxType,AccessListTxType, etc.
449-450: LGTM! Updated DeriveFields to include block timestamp.The
DeriveFieldsmethod signature is correctly updated to include thetimeparameter, which is then passed toMakeSigner. This change aligns with EIP-7702's requirement to use block timestamp for transaction signing.les/odr_test.go (1)
147-148: LGTM! Updated MakeSigner calls to include block timestamp.The
MakeSignercalls are correctly updated to includeheader.Timeparameter in both the blockchain and light client test paths. This change aligns with EIP-7702's requirement to use block timestamp for transaction signing.Also applies to: 161-162
core/types/transaction_marshalling.go (3)
47-47: LGTM! Added AuthorizationList field.The
AuthorizationListfield is correctly added to thetxJSONstruct with appropriate JSON tags and type.
171-187: LGTM! Added SetCodeTx marshalling support.The
SetCodeTxcase is properly implemented in theMarshalJSONmethod, handling all required fields including the newAuthorizationList.
444-487: LGTM! Added SetCodeTx unmarshalling support with proper validation.The
SetCodeTxcase inUnmarshalJSONincludes:
- Proper field validation
- Overflow checks for numeric fields
- Required field checks including
AuthorizationListcore/state_processor_test.go (1)
304-304: LGTM! Improved error message clarity.The error message for
ErrSenderNoEOAnow shows the length of the code instead of the code hash, making it more informative and easier to understand the reason for the error.eth/tracers/api_test.go (1)
167-167: LGTM! Block timestamp added to signer creation.The addition of
block.Time()toMakeSigneraligns with EIP-7702 requirements for including block timestamp in transaction signing.light/txpool.go (1)
423-423: Verify gas calculation for SetCode transactions.The change from
tx.To() == niltotx.SetCodeAuthorizations()in the intrinsic gas calculation affects how gas is computed for SetCode transactions. This is a significant change that impacts transaction validation.Run the following script to verify the gas calculation logic:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the gas calculation logic for SetCode transactions. # Search for other instances of IntrinsicGas calls to ensure consistency rg -A 5 "IntrinsicGas\(" # Search for SetCodeAuthorizations usage to understand the context ast-grep --pattern $'tx.SetCodeAuthorizations()'Length of output: 4362
Gas Calculation Consistency Verified
The grep results show that the new usage—passing tx.SetCodeAuthorizations() as the third parameter while still evaluating tx.To() == nil as the fourth—is consistently applied across the codebase (in light/txpool.go, core/tx_pool.go, transaction tests, etc.). This confirms that the change for SetCode transactions in the intrinsic gas calculation is intentional and in line with existing logic.
rollup/tracing/tracing.go (1)
108-108: LGTM! Block timestamp consistently added to transaction processing.The changes correctly incorporate block timestamp in both signer creation and transaction data initialization, maintaining consistency with EIP-7702 implementation.
Also applies to: 528-528
core/types/receipt_test.go (1)
300-307: LGTM! Test cases updated to include block timestamp.The test cases have been properly updated to include block timestamp in receipt derivation and signer creation, ensuring compatibility with EIP-7702 changes.
core/vm/jump_table.go (1)
70-76: LGTM! New instruction set properly enables EIP-7702.The implementation correctly:
- Inherits from
darwinInstructionSet- Enables EIP-7702 Setcode transaction type
- Follows the established pattern for instruction set creation
accounts/abi/bind/backends/simulated.go (2)
642-642: LGTM! Updated signer creation to include block time.The change correctly updates the
MakeSignercall to include the block's timestamp, aligning with EIP-7702 requirements.
812-825: LGTM! Added SetCodeAuthorizations support to callMsg.The implementation correctly adds the
SetCodeAuthorizationsmethod to support EIP-7702's code setting functionality.core/rawdb/accessors_chain.go (1)
589-590: LGTM! Block timestamp added to receipt derivation.The addition of
header.Timeto theDeriveFieldscall aligns with EIP-7702 requirements for including block timestamps in transaction signing and receipt generation.eth/fetcher/block_fetcher_test.go (1)
55-55: LGTM! Test updated to include block time in signer creation.The test code has been correctly updated to match the production code changes for EIP-7702, adding
block.Time()to theMakeSignercall.les/fetcher/block_fetcher_test.go (1)
55-55: LGTM! Light client test updated to include block time in signer creation.The light client test code has been correctly updated to match the production code changes for EIP-7702, adding
block.Time()to theMakeSignercall.eth/api.go (1)
918-918: Verify the use of zero block time for skipped transactions.While the code has been updated to match the new
NewRPCTransactionsignature, using zero as the block time for skipped transactions might not be the best approach. Consider using the block time fromstx.BlockNumberif available.Run the following script to check if skipped transactions store block time:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if skipped transactions store block time information. # Search for block time field in skipped transaction related code rg -A 5 "type SkippedTransaction struct"Length of output: 463
Skipped Transaction Block Time Verification
The output confirms that the SkippedTransaction struct (found in core/rawdb/accessors_skipped_txs.go) does not include any field for block time. Since no block time is stored or available for skipped transactions, using zero is an appropriate placeholder.
eth/tracers/api.go (4)
278-278: LGTM!The addition of
task.block.Time()toMakeSigneris consistent with EIP-7702 implementation.
537-537: LGTM!The addition of
block.Time()toMakeSigneris consistent with EIP-7702 implementation.
605-605: LGTM!The addition of
block.Time()toMakeSigneris consistent with EIP-7702 implementation.
909-909: LGTM!The addition of
block.Time()toMakeSigneris consistent with EIP-7702 implementation.miner/scroll_worker.go (2)
626-626: LGTM!The addition of
w.current.header.TimetoMakeSigneris consistent with EIP-7702 implementation.
666-666: LGTM!The addition of
w.current.header.TimetoMakeSigneris consistent with EIP-7702 implementation.core/blockchain.go (3)
1510-1510: LGTM: Block timestamp added to MakeSigner call.The addition of
chain[0].Time()to theMakeSignerfunction call aligns with EIP-7702 requirements for incorporating block timestamps into the signing process.
1509-1511: Add unit tests for timestamp-based signature recovery.Please add tests to cover:
- Signature recovery with various timestamp values
- Edge cases (zero timestamp, future timestamp)
- Compatibility with pre-existing blocks
- Error handling for invalid timestamps
Would you like me to help generate the test cases for the timestamp-based signature recovery?
1509-1511: Verify signature recovery behavior with timestamp.Please ensure that:
- All signature recovery mechanisms are updated to handle the new timestamp parameter.
- The change maintains backward compatibility with existing blocks.
- The timestamp parameter is properly validated in the signer implementation.
Run this script to check for any inconsistencies in signature recovery implementations:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for MakeSigner usage and verify timestamp parameter handling # Test: Search for all MakeSigner calls to ensure consistent timestamp usage rg -A 5 "MakeSigner.*Time" # Test: Search for signature recovery implementations ast-grep --pattern $'func ($x *$t) RecoverSender($y *$z) { $$$ }'Length of output: 19232
Signature recovery timestamp usage verified.
The shell script output confirms that all MakeSigner invocations consistently pass the block timestamp (e.g., chain[0].Time(), block.Time(), Env.Timestamp, etc.) across various parts of the codebase. The MakeSigner implementation in core/types/transaction_signing.go correctly utilizes the blockTime parameter (e.g., for branch selection like IsEuclidV2). Additionally, there is no conflicting RecoverSender implementation detected that would contradict this behavior.
The change in core/blockchain.go to pass chain[0].Time() aligns with the existing patterns and maintains backward compatibility.
core/types/transaction.go (4)
55-55: LGTM! New transaction type added.The
SetCodeTxTypeconstant is correctly defined with a unique value of0x04, following the sequence of existing transaction types.
206-207: LGTM! SetCodeTx handling added to decodeTyped.The
decodeTypedmethod is correctly updated to handle the newSetCodeTxTypeby instantiating a newSetCodeTxobject.
471-493: LGTM! New methods for handling SetCode authorizations.The implementation of
SetCodeAuthorizationsandAuthoritiesmethods is correct:
SetCodeAuthorizationssafely type asserts and returns authorizationsAuthoritiescorrectly extracts authority addresses from authorizations with proper error handling
770-843: LGTM! Message struct and methods updated.The
Messagestruct and related methods are correctly updated to includesetCodeAuthorizations:
- Field added to struct
NewMessageconstructor updatedAsMessageupdated to populate the field- Getter method added
eth/tracers/js/tracer.go (1)
708-708: LGTM! IntrinsicGas call updated.The
IntrinsicGasfunction call is correctly updated to match the new signature by passingnilfor thesetCodeAuthorizationsparameter.
colinlyguo
force-pushed
the
add-eip-7702
branch
from
f5e86d7 to
8c459ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
core/types/transaction_signing.go (1)
193-194:⚠️ Potential issueHandle type assertion explicitly.
The unchecked type assertion could lead to a zero-valued signer if the cast fails.
Apply this diff to handle the type assertion explicitly:
- signer, _ := NewLondonSignerWithEIP4844(chainId).(londonSignerWithEIP4844) + raw := NewLondonSignerWithEIP4844(chainId) + signer, ok := raw.(londonSignerWithEIP4844) + if !ok { + panic("unexpected signer type") + }
🧹 Nitpick comments (2)
core/vm/operations_acl.go (1)
227-285: Consider documenting the gas calculation formula.The implementation correctly handles gas calculations for both warm and cold accesses, including delegation resolution. However, adding documentation about the gas calculation formula would improve maintainability.
Add a comment block explaining:
- Base gas costs
- Additional costs for cold access
- Additional costs for delegation resolution
- How these costs align with EIP-7702 specifications
core/state_transition.go (1)
415-421: Consider improving error handling for invalid authorizations.Currently, invalid authorizations are silently skipped. Consider logging these failures or aggregating them into a composite error to help with debugging and monitoring.
- // Note errors are ignored, we simply skip invalid authorizations here. - st.applyAuthorization(&auth) + if err := st.applyAuthorization(&auth); err != nil { + log.Debug("Failed to apply authorization", "error", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
accounts/abi/bind/backends/simulated.go(3 hunks)accounts/external/backend.go(1 hunks)cmd/evm/internal/t8ntool/execution.go(1 hunks)cmd/evm/internal/t8ntool/transaction.go(2 hunks)cmd/evm/internal/t8ntool/transition.go(1 hunks)common/hexutil/json.go(2 hunks)core/bench_test.go(2 hunks)core/blockchain.go(1 hunks)core/chain_makers.go(1 hunks)core/error.go(1 hunks)core/rawdb/accessors_chain.go(1 hunks)core/rawdb/accessors_chain_test.go(1 hunks)core/state_prefetcher.go(1 hunks)core/state_processor.go(2 hunks)core/state_processor_test.go(1 hunks)core/state_transition.go(8 hunks)core/tx_pool.go(9 hunks)core/types/gen_authorization.go(1 hunks)core/types/l2trace.go(1 hunks)core/types/receipt.go(3 hunks)core/types/receipt_test.go(1 hunks)core/types/setcode_tx.go(1 hunks)core/types/transaction.go(5 hunks)core/types/transaction_marshalling.go(4 hunks)core/types/transaction_signing.go(4 hunks)core/vm/eips.go(2 hunks)core/vm/evm.go(7 hunks)core/vm/interpreter.go(1 hunks)core/vm/jump_table.go(1 hunks)core/vm/operations_acl.go(2 hunks)eth/api.go(1 hunks)eth/catalyst/api.go(1 hunks)eth/downloader/queue_test.go(1 hunks)eth/downloader/testchain_test.go(1 hunks)eth/fetcher/block_fetcher_test.go(1 hunks)eth/gasprice/gasprice.go(4 hunks)eth/state_accessor.go(1 hunks)eth/tracers/api.go(5 hunks)eth/tracers/api_test.go(1 hunks)eth/tracers/internal/tracetest/calltrace_test.go(2 hunks)eth/tracers/js/tracer.go(1 hunks)graphql/graphql.go(3 hunks)internal/ethapi/api.go(13 hunks)internal/ethapi/transaction_args.go(4 hunks)les/downloader/queue_test.go(1 hunks)les/downloader/testchain_test.go(1 hunks)les/fetcher/block_fetcher_test.go(1 hunks)les/odr_test.go(2 hunks)les/state_accessor.go(1 hunks)light/odr_test.go(1 hunks)light/odr_util.go(1 hunks)light/txpool.go(1 hunks)miner/scroll_worker.go(2 hunks)params/protocol_params.go(1 hunks)rollup/ccc/async_checker_test.go(1 hunks)rollup/tracing/tracing.go(2 hunks)tests/state_test_util.go(1 hunks)tests/transaction_test_util.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (39)
- core/state_processor_test.go
- core/chain_makers.go
- core/vm/jump_table.go
- core/bench_test.go
- eth/downloader/queue_test.go
- eth/state_accessor.go
- core/types/l2trace.go
- params/protocol_params.go
- les/downloader/testchain_test.go
- les/state_accessor.go
- light/txpool.go
- tests/state_test_util.go
- core/vm/interpreter.go
- eth/downloader/testchain_test.go
- core/types/gen_authorization.go
- core/state_prefetcher.go
- accounts/external/backend.go
- miner/scroll_worker.go
- eth/tracers/internal/tracetest/calltrace_test.go
- eth/tracers/js/tracer.go
- les/odr_test.go
- eth/api.go
- les/fetcher/block_fetcher_test.go
- tests/transaction_test_util.go
- internal/ethapi/transaction_args.go
- core/state_processor.go
- cmd/evm/internal/t8ntool/transition.go
- eth/catalyst/api.go
- cmd/evm/internal/t8ntool/transaction.go
- rollup/ccc/async_checker_test.go
- core/rawdb/accessors_chain.go
- les/downloader/queue_test.go
- light/odr_test.go
- eth/tracers/api_test.go
- rollup/tracing/tracing.go
- light/odr_util.go
- eth/fetcher/block_fetcher_test.go
- core/types/setcode_tx.go
- core/blockchain.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (40)
internal/ethapi/api.go (3)
994-994: Duplicate check on passing block time to MakeSigner.
This usage parallels the logic at line 891. Ensure consistency across all MakeSigner call sites—that block time is handled uniformly and meets your network’s requirements.
1398-1402: Duplicate Y-Parity logic concern (DynamicFeeTxType).
Again, v.Sign() here will not reflect the typical ECDSA V range of 27 or 28.
891-891: Confirm the correctness of using block.Time() in MakeSigner.
Passing block.Time() as a parameter to types.MakeSigner can be valid if the signer logic depends on the block’s timestamp. However, please ensure that the timestamp is in the expected unit (usually Unix time in seconds) and that the EVM rules for your network require or allow timestamp-based activation.Would you like to run a quick search in the codebase to confirm that MakeSigner properly handles the timestamp parameter?
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Searching for MakeSigner definitions to ensure the time parameter is handled correctly ast-grep --pattern $'function MakeSigner($_, $_, $_) { $$$ }'Length of output: 62
Below is a new search command that uses ripgrep to locate MakeSigner’s definition. This should help us confirm how the timestamp parameter is handled:
Please run this script so we can gather additional context on whether passing block.Time() in this call is correct.
🏁 Script executed:
#!/bin/bash # Searching for the definition of MakeSigner in the repository using ripgrep rg -n "func MakeSigner" .Length of output: 159
Subject: Verified Correct Usage of block.Time() with MakeSigner
- The definition in "./core/types/transaction_signing.go" shows that MakeSigner accepts a third parameter of type uint64.
- Assuming block.Time() returns a Unix timestamp in seconds (typical for block timestamps), its use here is correct and aligns with the expected unit based on EVM rules.
core/error.go (1)
107-122: New EIP-7702 error variables look consistent.
Defining explicit errors for EIP-7702 helps improve clarity and maintainability. The naming convention is consistent with other errors in this file.core/vm/eips.go (1)
276-282: Check definitions of gasCallEIP7702 and related functions.
Enabling EIP-7702 references gasCallEIP7702, gasCallCodeEIP7702, gasStaticCallEIP7702, and gasDelegateCallEIP7702. Please confirm they’re implemented and tested.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for the implementations of these gas calculation functions rg 'func\s+gas(Call|CallCode|StaticCall|DelegateCall)EIP7702'Length of output: 63
We didn’t see any function definitions when searching with the provided regular expression. It’s possible that the EIP‑7702 gas settings aren’t defined as functions at all, but rather as constants or variables declared elsewhere in the codebase. Additional checks (for example, searching for “const gasCallEIP7702” or a case‐insensitive scan for “gasCallEIP7702”) are needed to manually verify that these identifiers are indeed implemented and that their behavior is covered by tests.
Please verify manually (or run additional scripts) whether the definitions of gasCallEIP7702, gasCallCodeEIP7702, gasStaticCallEIP7702, and gasDelegateCallEIP7702 exist in the repository and are exercised by the test suite.
common/hexutil/json.go (1)
231-271: LGTM! Consider adding unit tests.The implementation of the new
U256type and its methods is clean and follows established patterns. The code correctly handles JSON marshaling/unmarshaling with proper validation.Please ensure comprehensive unit tests are added to cover:
- Marshaling/unmarshaling of zero values
- Empty string handling
- Invalid hex string handling
- Overflow cases
- String representation
core/vm/operations_acl.go (1)
220-225: LGTM! Gas calculation functions are properly initialized.The gas calculation functions for different call types (CALL, DELEGATECALL, STATICCALL, CALLCODE) are correctly initialized using the new EIP-7702 implementation.
cmd/evm/internal/t8ntool/execution.go (1)
120-120: LGTM! Correctly updated MakeSigner call.The timestamp parameter has been added to the MakeSigner call, maintaining consistency with EIP-7702 changes across the codebase.
eth/gasprice/gasprice.go (1)
235-235: LGTM! Improved encapsulation of signer creation.The changes correctly move signer creation inside the getBlockValues method, reducing parameter coupling and improving encapsulation. The signer is properly created using both block number and time.
Also applies to: 323-324
core/types/receipt.go (2)
239-239: LGTM! Support for SetCodeTxType added.The SetCodeTxType is correctly added to the switch case, following the same pattern as other transaction types.
449-450: LGTM! Time parameter correctly integrated.The DeriveFields method signature and MakeSigner call are properly updated to include the time parameter, enabling time-based signer selection.
core/types/transaction_marshalling.go (3)
35-52: LGTM! AuthorizationList field properly added.The AuthorizationList field is correctly added to support EIP-7702 authorization data, with proper JSON tags and type definition.
367-442: LGTM! Proper overflow handling added.The implementation correctly checks for overflows when converting between big.Int and uint256, enhancing safety.
444-517: LGTM! SetCodeTx handling properly implemented.The SetCodeTx case follows the established pattern, with proper validation and overflow checks.
core/state_transition.go (4)
92-92: LGTM! Interface correctly extended.The Message interface is properly extended to support EIP-7702 authorization data.
178-180: LGTM! Gas calculation properly implemented.The implementation correctly charges CallNewAccountGas for each authorization, following the EIP-7702 specification.
331-338: LGTM! Authorization validation properly implemented.The preCheck method correctly validates EIP-7702 authorizations by checking for contract creation attempts and empty authorization lists.
477-532: LGTM! Authorization handling properly implemented.The validateAuthorization and applyAuthorization methods correctly implement EIP-7702 specification with proper validation and state updates.
core/types/transaction_signing.go (1)
40-44: LGTM! Time-based signer selection properly implemented.The MakeSigner function correctly supports time-based signer selection for EuclidV2.
core/vm/evm.go (3)
542-554: LGTM! Well-structured code resolution with delegation support.The implementation correctly handles code resolution with delegation support under EuclidV2 rules, following a clear and safe pattern:
- First attempts direct code lookup
- Only follows delegation if EuclidV2 is active
- Limits delegation to one level to prevent recursion
556-569: LGTM! Consistent code hash resolution implementation.The implementation follows the same pattern as
resolveCode, ensuring consistent behavior for code hash resolution under EuclidV2 rules.
233-234: Verify the impact of code resolution changes.The changes to use
resolveCodeandresolveKeccakCodeHashacross all call operations look correct, but we should verify there are no edge cases where the delegation resolution could cause unexpected behavior.Also applies to: 241-242, 301-302, 344-345, 400-401, 450-451
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for potential issues with code delegation in tests rg -A 5 "ParseDelegation.*EuclidV2"Length of output: 37
I'll gather further data from the repository to search for potential edge cases related to delegation resolution. Let's run a script to search for references to "resolveCode", "resolveKeccakCodeHash", "delegatecall", and "ParseDelegation" to better contextualize this change.
🏁 Script executed:
#!/bin/bash echo "Searching for occurrences of resolveCode in the repository:" rg -A 10 "resolveCode\(" echo "-------------------------------------------" echo "Searching for occurrences of resolveKeccakCodeHash in the repository:" rg -A 10 "resolveKeccakCodeHash\(" echo "-------------------------------------------" echo "Searching for occurrences of delegatecall in the repository:" rg -A 10 "delegatecall" echo "-------------------------------------------" echo "Searching for occurrences of ParseDelegation in the repository:" rg -A 10 "ParseDelegation"Length of output: 18334
Delegation resolution changes verified.
I reviewed the code paths for both resolveCode and resolveKeccakCodeHash in core/vm/evm.go and checked for edge-case usages via ParseDelegation. The searches across the repository confirm that the one-level delegation resolution is applied consistently, and no unexpected behavior was detected.core/types/receipt_test.go (1)
762-764: LGTM! Added time parameter to DeriveFields call.The change correctly adds the new time parameter to align with the updated method signature.
core/types/transaction.go (3)
55-55: LGTM! Clear transaction type constant definition.The
SetCodeTxTypeconstant is appropriately defined with a unique value.
206-207: LGTM! Added SetCodeTx support in decodeTyped.The change correctly adds support for decoding
SetCodeTxtransactions.
770-784: LGTM! Consistent Message struct updates.The changes to the Message struct and related methods maintain consistency with the new authorization functionality.
Also applies to: 786-801, 807-820
core/rawdb/accessors_chain_test.go (1)
762-764: LGTM! Added time parameter to DeriveFields call.The change correctly adds the new time parameter to align with the updated method signature.
accounts/abi/bind/backends/simulated.go (2)
642-642: LGTM: Block timestamp correctly included in signer creation.The addition of
head.Timeparameter toMakeSigneraligns with EIP-7702 requirements for transaction signing.
663-663: LGTM: Block timestamp correctly included in signer creation.The addition of
block.Time()parameter toMakeSignermaintains consistency with EIP-7702 requirements.graphql/graphql.go (3)
230-231: LGTM: SetCodeTxType correctly handled in gas price calculation.The SetCodeTxType is appropriately included in the same case as DynamicFeeTxType since they share the same gas price calculation logic.
266-267: LGTM: SetCodeTxType correctly handled in max fee calculation.The SetCodeTxType is appropriately included in the same case as DynamicFeeTxType for max fee per gas calculation.
281-282: LGTM: SetCodeTxType correctly handled in priority fee calculation.The SetCodeTxType is appropriately included in the same case as DynamicFeeTxType for max priority fee per gas calculation.
eth/tracers/api.go (3)
278-278: LGTM: Block timestamp correctly included in signer creation for chain tracing.The addition of
task.block.Time()parameter toMakeSigneraligns with EIP-7702 requirements.
537-537: LGTM: Block timestamp correctly included in signer creation for intermediate roots.The addition of
block.Time()parameter toMakeSignermaintains consistency with EIP-7702 requirements.
909-909: LGTM: Block timestamp correctly included in signer creation for trace calls.The addition of
block.Time()parameter toMakeSignerensures consistent implementation of EIP-7702 requirements across all tracing functions.core/tx_pool.go (5)
81-101: LGTM! Well-defined error variables.The new error variables are properly defined and documented, clearly describing their purpose for account limits and authority conflicts.
792-808: Fix undeclared 'max' function.The code uses an undeclared
maxfunction which will cause a compilation error.
809-832: LGTM! Robust authorization conflict detection.The implementation properly validates:
- Transaction sender has no in-flight authorizations
- Authorities don't conflict with pending/queued transactions
- SetCode transactions have at least one authorization tuple
2020-2022: Remove duplicate auths field declaration.The
authsfield is declared twice in thetxLookupstructure.
2181-2216: LGTM! Well-implemented authorization tracking.The authorization tracking methods are well-implemented with:
- Proper duplicate prevention
- Clean cleanup of empty lists
- Appropriate error logging
- Efficient use of the slices package
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/vm/runtime/runtime_test.go (1)
970-1063: LGTM! Comprehensive gas cost testing for delegated accounts.The test covers various operations (CALL, CALLCODE, DELEGATECALL, STATICCALL, SELFDESTRUCT) with proper gas cost validation.
Remove duplicate gas cost check.
Lines 1059-1061 duplicate the check already performed at lines 1053-1058.
- if want := tc.want; have != want { - t.Fatalf("testcase %d, gas report wrong, step %d, have %d want %d", i, tc.step, have, want) - }core/blockchain_test.go (1)
3795-3884: Good test coverage for EIP-7702, consider adding edge cases.The test effectively verifies:
- Delegation deployment and authorization
- Multi-level delegation execution
- Storage state changes
Consider adding test cases for:
- Failed authorization scenarios
- Revocation of delegations
- Gas cost validations
// Add test cases for failure scenarios func TestEIP7702Failures(t *testing.T) { // Test invalid authorization // Test revoked delegation // Test gas exhaustion }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Makefile(1 hunks)core/blockchain_test.go(3 hunks)core/state_processor_test.go(5 hunks)core/types/setcode_tx_test.go(1 hunks)core/vm/runtime/runtime_test.go(2 hunks)go.mod(1 hunks)params/config.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (11)
Makefile (1)
46-48: LGTM! Good test coverage for EIP-7702.The addition of specific tests for EIP-7702 functionality is well-structured and follows the existing pattern in the Makefile. The verbose flag (-v) will help with debugging test failures.
core/types/setcode_tx_test.go (1)
1-71: LGTM! Well-structured test cases for delegation parsing.The test function provides comprehensive coverage of both valid and invalid scenarios, including:
- Correct delegation with expected address
- Wrong address size handling
- Short/long address cases
- Prefix validation
- Empty cases
core/state_processor_test.go (3)
98-113: LGTM! Well-implemented helper function for creating SetCode transactions.The
mkSetCodeTxhelper function includes proper error handling and uses appropriate types for chain ID and value fields.
235-240: LGTM! Good test coverage for empty authorization list.Test case properly validates the behavior of SetCode transactions with empty authorization lists.
330-331: LGTM! Improved error message clarity.The error message now includes the length of the code instead of the code hash, making it more informative for debugging.
params/config.go (3)
643-644: LGTM! Well-structured fork time fields.The new Euclid fork time fields follow the established pattern for time-based forks.
915-923: LGTM! Proper fork time check implementation.The
IsEuclidmethod correctly uses theisForkedTimehelper for time-based fork activation.
1137-1137: LGTM! Updated Rules struct with new flags.The Rules struct properly includes the new Euclid fork flags.
core/blockchain_test.go (2)
20-20: LGTM! Required imports added.The new imports are needed for byte operations and uint256 handling in the new test cases.
Also applies to: 31-31
3720-3793: Well-structured test for Curie hardfork transition!The test thoroughly verifies:
- Base fee activation
- Gas oracle deployment and state changes
- Code hash and storage slot updates
go.mod (1)
8-8: Verify gomonkey version and consider moving to test dependencies.The gomonkey library uses unsafe package operations for mocking. Since it's primarily used for testing:
- Consider moving it to test dependencies using
//go:build testbuild tags- Let's verify if v2.12.0 is the latest stable version
✅ Verification successful
🌐 Web query:
What is the latest version of github.com/agiledragon/gomonkey? Are there any known security issues with gomonkey v2.12.0?💡 Result:
The latest version of
github.com/agiledragon/gomonkeyas of February 2025 appears to be v2.x, with active development evidenced by releases like v2.11.0 (June 2024) and subsequent updates addressing platform support (e.g., LoongArch64) and feature improvements[1][4][7]. Specific security advisories for v2.12.0 are not mentioned in available sources, but broader context highlights two key points:
Security Status:
- No published security advisories exist for this library[6].
- The project lacks a formal
SECURITY.mdpolicy file[6].Supply Chain Risks:
- A separate Go package (
boltdb-go) was compromised via malicious forks cached indefinitely by Go Module Mirror[2]. While unrelated togomonkey, this highlights risks inherent in Go’s dependency ecosystem[2].For safety:
- Verify dependencies via checksums or vendoring
- Monitor for updates via tools like Snyk or Dependabot
(Note: If v2.12.0 exists beyond indexed data, confirm details via official channels.)
Citations:
- 1: https://snyk.io/advisor/golang/github.com/agiledragon/gomonkey/v2
- 2: https://www.techradar.com/pro/security/a-cracked-malicious-version-of-a-go-package-lay-undetected-online-for-years
- 3: https://cwiki.apache.org/confluence/display/INCUBATOR/Seata+Proposal
- 4: https://github.com/agiledragon/gomonkey/releases
- 5: https://thehackernews.com/2025/01/google-project-zero-researcher-uncovers.html
- 6: https://github.com/agiledragon/gomonkey/security
- 7: https://pkg.go.dev/github.com/agiledragon/gomonkey/v2
- 8: mac m1 used gomockey: undefined: buildJmpDirective agiledragon/gomonkey#38
- 9: https://pkg.go.dev/github.com/agiledragon/gomonkey
- 10: https://sca.analysiscenter.veracode.com/vulnerability-database/security/unauthorized-rule-injection/go/sid-50232
Gomonkey Dependency Verified; Consider Isolating Test-Only Usage
- The latest available information confirms that v2.12.0 of
github.com/agiledragon/gomonkey/v2is acceptable with no known security issues.- As this library is primarily used for testing—and given its reliance on unsafe operations—it is advisable to isolate its usage to test files (e.g., through build tags) to avoid accidental production usage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/ethapi/api_test.go (5)
54-54: Consider using a dynamic test key generation.The function uses a hardcoded private key for testing. While this is acceptable for tests, it's good practice to document that this is a test-only key to prevent accidental reuse.
Add a comment to clarify this is a test-only key:
- testKey, _ := crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291") + // Test-only key, never use in production + testKey, _ := crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291")
208-208: Document unimplemented methods or provide minimal implementations.Several methods in
testBackendare marked withpanic("implement me"). While this is acceptable for unused methods in tests, it's better to:
- Document why these methods are left unimplemented
- Provide minimal implementations for methods that might be needed in future tests
Consider adding comments and minimal implementations:
-func (b testBackend) Pending() (*types.Block, types.Receipts, *state.StateDB) { panic("implement me") } +// Pending returns nil values as it's not required for current test cases +func (b testBackend) Pending() (*types.Block, types.Receipts, *state.StateDB) { return nil, nil, nil }Also applies to: 234-234, 237-237, 240-240, 243-243, 246-246, 249-249, 252-252, 255-255, 262-262, 266-266, 268-268, 271-271, 274-274, 279-279, 282-282, 285-285, 287-287, 289-289
316-352: Enhance test coverage with additional test cases.The current test suite only covers basic positive cases. Consider adding:
- Negative test cases (e.g., insufficient balance)
- Edge cases (e.g., zero value transfers)
- Tests with different gas prices
- Tests with contract interactions
Add more test cases to the test suite:
var testSuite = []struct { blockNumber rpc.BlockNumber call TransactionArgs want uint64 + expectError bool }{ // Existing test cases... + // Test case: Insufficient balance + { + blockNumber: rpc.LatestBlockNumber, + call: TransactionArgs{ + From: &accounts[1].addr, + To: &accounts[0].addr, + Value: (*hexutil.Big)(big.NewInt(params.Ether * 2)), // More than balance + }, + want: 0, + expectError: true, + }, + // Test case: Zero value transfer + { + blockNumber: rpc.LatestBlockNumber, + call: TransactionArgs{ + From: &accounts[0].addr, + To: &accounts[1].addr, + Value: (*hexutil.Big)(big.NewInt(0)), + }, + want: 21000, + }, }
329-329: Document the gas value constant.The test uses the magic number
21000for gas values. Consider using a named constant or adding a comment to explain this value.- want: 21000, + want: params.TxGas, // 21000: minimum gas required for basic ETH transferAlso applies to: 339-339
348-350: Improve error message for gas estimation mismatch.The current error message could be more descriptive about the gas estimation mismatch.
- t.Errorf("test %d, result mismatch, have\n%v\n, want\n%v\n", i, uint64(result), tc.want) + t.Errorf("test %d: gas estimation mismatch for %v -> %v transfer, estimated: %v, expected: %v", + i, tc.call.From.String(), tc.call.To.String(), uint64(result), tc.want)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/ethapi/api_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
core/tx_pool_test.go (1)
145-171: Consider checking the ignored error fromtypes.SignSetCode.
In this test helper, the error returned bytypes.SignSetCode(u.key, auth)is being ignored (_). To ensure test consistency, it may be safer to handle or assert the error.Here’s a possible fix:
for _, u := range unsigned { - auth, _ := types.SignSetCode(u.key, types.SetCodeAuthorization{ + auth, err := types.SignSetCode(u.key, types.SetCodeAuthorization{ + if err != nil { + panic(fmt.Sprintf("failed to sign setcode: %v", err)) + } ChainID: *uint256.MustFromBig(params.TestChainConfig.ChainID), Address: common.Address{0x42}, Nonce: u.nonce, })internal/ethapi/transaction_args.go (2)
57-59: Consider adding a brief doc comment forAuthorizationList.
Adding a short explanation about how this field relates to SetCodeTx transactions (EIP-7702) could clarify its usage for external developers.
264-285: Validate handling of potentially emptyAuthorizationList.
Whenargs.AuthorizationList != nilbut is actually empty, the code still creates a SetCodeTx. Verify if an empty authorization list is valid or if you should reject it.core/vm/runtime/runtime_test.go (1)
995-1043: Verify gas costs for edge cases.The test covers the main opcodes but consider adding edge cases:
- Multiple consecutive calls to the same delegated address
- Calls with non-zero value
- Calls with non-empty input data
core/tx_pool.go (4)
807-823: Consider extracting the helper function.The
usedAndLeftSlotsfunction is a complex helper that would benefit from being a standalone method of the TxPool struct, improving readability and testability.- usedAndLeftSlots := func(addr common.Address) (int, int) { +func (pool *TxPool) usedAndLeftSlots(addr common.Address) (int, int) { var have int if list := pool.pending[addr]; list != nil { have += list.Len() } if list := pool.queue[addr]; list != nil { have += list.Len() } if pool.currentState.GetKeccakCodeHash(addr) != codehash.EmptyKeccakCodeHash || len(pool.all.auths[addr]) != 0 { // Allow at most one in-flight tx for delegated accounts or those with // a pending authorization. return have, max(0, 1-have) } return have, math.MaxInt - }
827-841: Consider extracting the helper function.Similar to the previous comment, the
knownConflictsfunction would be more maintainable as a standalone method.- knownConflicts := func(auths []common.Address) []common.Address { +func (pool *TxPool) knownConflicts(auths []common.Address) []common.Address { var conflicts []common.Address // Authorities cannot conflict with any pending or queued transactions. for _, addr := range auths { if list := pool.pending[addr]; list != nil { conflicts = append(conflicts, addr) } else if list := pool.queue[addr]; list != nil { conflicts = append(conflicts, addr) } } return conflicts - }
2196-2211: Consider adding error handling for invalid authorities.The
addAuthoritiesmethod should validate the authorities before adding them to the tracking map.func (t *txLookup) addAuthorities(tx *types.Transaction) { + if tx == nil { + return + } for _, addr := range tx.SetCodeAuthorities() { + if addr == (common.Address{}) { + log.Warn("Invalid authority address in transaction", "hash", tx.Hash()) + continue + } list, ok := t.auths[addr] if !ok { list = []common.Hash{} }
2215-2231: Consider optimizing slice operations.The slice operations in
removeAuthoritiescould be optimized to avoid unnecessary allocations.func (t *txLookup) removeAuthorities(hash common.Hash) { for addr := range t.auths { list := t.auths[addr] // Remove tx from tracker. if i := slices.Index(list, hash); i >= 0 { - list = append(list[:i], list[i+1:]...) + // Use copy to avoid memory leaks + copy(list[i:], list[i+1:]) + list = list[:len(list)-1] } else { log.Error("Authority with untracked tx", "addr", addr, "hash", hash) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Makefile(1 hunks)core/blockchain_test.go(3 hunks)core/state_processor_test.go(5 hunks)core/tx_pool.go(10 hunks)core/tx_pool_test.go(3 hunks)core/types/setcode_tx.go(1 hunks)core/types/transaction.go(5 hunks)core/types/transaction_marshalling.go(4 hunks)core/vm/runtime/runtime_test.go(2 hunks)internal/ethapi/transaction_args.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- core/state_processor_test.go
- core/blockchain_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: semgrep/ci
🔇 Additional comments (19)
core/tx_pool_test.go (4)
31-31: No concerns with import usage.
Theuint256package is used below for numeric operations, so this import is valid and in scope.
39-39: Import usage looks fine.
Thevmpackage is indeed used later in the test (e.g.vm.ADDRESS,vm.PUSH0, etc.).
140-143: Struct definition is straightforward.
This struct is clearly used in test code to store a nonce and key. No issues noted.
2669-2860: Tests appear comprehensive for EIP-7702 scenarios.
This suite thoroughly checks various SetCode transaction behaviors, including fee replacement, multiple delegations, authority-reservation logic, etc. The coverage of negative cases looks good, and no major issues were detected.core/types/setcode_tx.go (1)
1-216: Implementation of EIP-7702 transaction type looks well-structured.
The code introduces clear logic for delegation prefixes, signature handling, and typed transaction fields. The approach to signature recovery and chain ID usage aligns with the existing typed-transaction pattern in the codebase. No major issues noted.internal/ethapi/transaction_args.go (2)
26-26: Import usage is valid.
Theuint256import is leveraged for processing chain and fee values in new transaction logic.
255-256: Verify chain compatibility for AuthorizationList usage.
You’re unconditionally passingargs.AuthorizationListtotypes.NewMessage. If EIP-7702 is not activated on the chain, you might want to confirm that this path is still valid or gracefully handled.core/types/transaction_marshalling.go (3)
35-52: LGTM! The field additions totxJSONstruct are well-organized.The new fields are properly tagged with JSON annotations and follow the existing pattern.
171-187: LGTM! TheSetCodeTxcase inMarshalJSONis implemented correctly.The implementation follows the same pattern as other transaction types and properly handles all fields.
444-487: Verify error handling consistency inUnmarshalJSON.The error handling for
SetCodeTxfollows the pattern of other transaction types, but let's verify the error messages are consistent.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for error messages in UnmarshalJSON rg -A 1 "missing required field" core/types/transaction_marshalling.goLength of output: 4015
Error Handling Consistency Verified in UnmarshalJSON
The error messages in the
SetCodeTxbranch follow the same pattern used in other transaction types. Each required field check (e.g., forchainId,to,gas,maxPriorityFeePerGas,maxFeePerGas,value,input, andauthorizationList) returns an error string consistent with similar checks elsewhere in the file.
- All messages use the format
"missing required field '<field>' ..."with context-specific suffixes (in transactionfor top-level fields andfor txdatafor fields within transaction data).- The grep output confirms that the error strings in
SetCodeTxexactly match the patterns observed for other transaction types.core/types/transaction.go (3)
55-55: LGTM! TheSetCodeTxTypeconstant follows the sequence.The constant value
0x04follows the existing pattern and sequence of transaction types.
471-493: LGTM! The authorization methods are well-implemented.The
SetCodeAuthorizationsandSetCodeAuthoritiesmethods:
- Properly handle type assertions
- Return nil for non-SetCodeTx transactions
- Use capacity-based slice allocation for better performance
766-784: LGTM! TheMessagestruct updates are consistent.The addition of
setCodeAuthorizationsfield and its getter method follow the existing pattern.core/vm/runtime/runtime_test.go (1)
971-994: LGTM! The test setup is well-structured.The test properly:
- Sets up the state with delegation designator
- Uses gomonkey to patch GetPoseidonCodeHash
- Logs relevant debug information
core/tx_pool.go (5)
82-84: LGTM! Clear and descriptive error variable.The new error variable
ErrAccountLimitExceededis well-documented and follows the existing error declaration pattern.
99-102: LGTM! Clear and descriptive error variable.The new error variable
ErrAuthorityReservedis well-documented and follows the existing error declaration pattern.
267-280: LGTM! Comprehensive documentation of EIP-7702 changes.The added documentation clearly explains the rationale behind tracking pending SetCode authorizations and the transaction slot limitations for accounts with deployed delegations.
843-847: LGTM! Essential validation for SetCode transactions.The validation ensures that SetCode transactions have at least one authorization tuple, which is a critical requirement.
2035-2036: LGTM! Well-documented field addition.The new
authsfield intxLookupstruct is properly documented and follows the existing pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/vm/runtime/runtime_test.go (1)
1054-1063:⚠️ Potential issueRemove duplicate error check.
The gas cost comparison is performed twice with identical conditions.
Apply this diff to remove the duplicate check:
- if want := tc.want; have != want { - t.Fatalf("testcase %d, gas report wrong, step %d, have %d want %d", i, tc.step, have, want) - }
🧹 Nitpick comments (2)
core/blockchain_test.go (1)
3795-3901: Well-implemented test for EIP-7702 delegation functionality.The test effectively verifies:
- Deployment of delegation designations
- Execution of delegated calls
- Storage updates through delegation chain
- Authorization signing and verification
Consider adding test cases for:
- Invalid authorization scenarios
- Revocation of delegations
- Multiple levels of nested delegation
core/vm/runtime/runtime_test.go (1)
971-973: Enhance test documentation.Consider adding more details about the test cases and expected gas costs in the function documentation.
-// TestDelegatedAccountAccessCost tests that calling an account with an EIP-7702 -// delegation designator incurs the correct amount of gas based on the tracer. +// TestDelegatedAccountAccessCost tests gas costs for operations on accounts with EIP-7702 +// delegation designators. It verifies: +// - CALL/CALLCODE operations cost 5455 gas +// - DELEGATECALL/STATICCALL operations cost 5455 gas +// - SELFDESTRUCT operation remains unaffected at 3 gas +// The test uses gomonkey to patch GetPoseidonCodeHash for consistent test behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/blockchain_test.go(3 hunks)core/vm/runtime/runtime_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
core/blockchain_test.go (1)
3720-3793: Well-structured test for Curie fork transition.The test provides good coverage of the Curie fork transition by verifying:
- Base fee changes
- Gas oracle deployment and state
- Code hash calculations (both Keccak and Poseidon)
- Storage slot updates
core/vm/runtime/runtime_test.go (1)
23-23: LGTM!The new imports are appropriately added and necessary for the test implementation.
Also applies to: 28-28
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
core/vm/runtime/runtime_test.go (4)
988-988: Use t.Logf instead of fmt.Println for test logging.Replace
fmt.Printlnwitht.Logfto maintain consistent logging in tests.-fmt.Println(statedb.Exist(common.HexToAddress("0xff"))) +t.Logf("Address 0xff exists: %v", statedb.Exist(common.HexToAddress("0xff")))
974-980: Verify initial state setup.The test should verify that the initial state is set up correctly before proceeding with the test cases.
statedb, _ := state.New(common.Hash{}, state.NewDatabase(rawdb.NewMemoryDatabase()), nil) statedb.SetCode(common.HexToAddress("0xff"), types.AddressToDelegation(common.HexToAddress("0xaa"))) statedb.SetCode(common.HexToAddress("0xaa"), []byte{ byte(vm.PUSH1), 0x00, byte(vm.PUSH1), 0x00, byte(vm.RETURN), }) + +// Verify initial state +if !statedb.Exist(common.HexToAddress("0xff")) { + t.Fatal("Expected address 0xff to exist") +} +if !statedb.Exist(common.HexToAddress("0xaa")) { + t.Fatal("Expected address 0xaa to exist") +} +if code := statedb.GetCode(common.HexToAddress("0xff")); len(code) == 0 { + t.Fatal("Expected non-empty code at address 0xff") +} +if code := statedb.GetCode(common.HexToAddress("0xaa")); len(code) == 0 { + t.Fatal("Expected non-empty code at address 0xaa") +}
995-1043: Add descriptive comments for test cases.Each test case should have a comment explaining the expected behavior and why the specific gas cost is expected.
for i, tc := range []struct { code []byte step int want uint64 }{ + // CALL to a delegated account should cost 5455 gas: + // - 2600 (cold account access) + // - 2855 (base call cost) { // CALL(0xff) code: []byte{ byte(vm.PUSH1), 0x0, byte(vm.DUP1), byte(vm.DUP1), byte(vm.DUP1), byte(vm.DUP1), byte(vm.PUSH1), 0xff, byte(vm.DUP1), byte(vm.CALL), byte(vm.POP), }, step: 7, want: 5455, }, // Similar comments for other test cases...
1044-1060: Extract gas cost verification into a helper function.The gas cost verification logic is repeated for each test case. Extract it into a helper function to improve maintainability.
+func verifyGasCost(t *testing.T, logs []vm.StructLog, testCase int, step int, have, want uint64) { + if have != want { + for i, op := range logs { + t.Logf("%d: %v %d", i, op.OpName(), op.GasCost) + } + t.Fatalf("testcase %d, gas report wrong, step %d, have %d want %d", testCase, step, have, want) + } +} + for i, tc := range []struct { // ... } { tracer := vm.NewStructLogger(nil) Execute(tc.code, nil, &Config{ // ... }) have := tracer.StructLogs()[tc.step].GasCost - if want := tc.want; have != want { - for ii, op := range tracer.StructLogs() { - t.Logf("%d: %v %d", ii, op.OpName(), op.GasCost) - } - t.Fatalf("tescase %d, gas report wrong, step %d, have %d want %d", i, tc.step, have, want) - } + verifyGasCost(t, tracer.StructLogs(), i, tc.step, have, tc.want)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/types/gen_authorization.go(1 hunks)core/vm/runtime/runtime_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/types/gen_authorization.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/gen_sttransaction.go (1)
1-1: 💡 Verification agent🧩 Analysis chain
Changes should be made to the source template instead of the generated file.
This is a generated file (as indicated by the comment "DO NOT EDIT"). Any changes should be made to the source template instead of directly modifying this file.
Let's verify the source template location and generation process:
🏁 Script executed:
#!/bin/bash # Find the source template file fd -g "*sttransaction*.go" | grep -v "gen_" # Look for gencodec generation commands rg "go:generate.*gencodec" -A 5Length of output: 9263
Update the source template for stTransaction instead of modifying the generated file.
The generated file was produced using the stTransaction definition in
tests/state_test_util.go(see the go:generate command), which currently does not include the new fieldAuthorizationList. Any changes made directly intests/gen_sttransaction.gorisk being overwritten the next time code is generated. TheAuthorizationListfield should be added to the source template (or its corresponding field override file, if applicable) and then the generation command re-run.
🧹 Nitpick comments (3)
tests/gen_stauthorization.go (2)
17-34: Add validation for cryptographic parameters.Consider adding validation for
ChainID,R, andSvalues before marshaling to ensure they are within valid ranges for ECDSA signatures.func (s stAuthorization) MarshalJSON() ([]byte, error) { + // Validate parameters + if s.ChainID == nil || s.R == nil || s.S == nil { + return nil, errors.New("chainId, R, and S must not be nil") + } + if s.R.Sign() <= 0 || s.S.Sign() <= 0 { + return nil, errors.New("R and S must be positive") + } type stAuthorization struct {
36-75: Add validation for V parameter.The
Vvalue should be validated to ensure it's within the valid range for ECDSA recovery ID.if dec.V == nil { return errors.New("missing required field 'v' for stAuthorization") } s.V = uint8(*dec.V) + // Validate V value (should be 0 or 1 for EIP-2718) + if s.V > 1 { + return fmt.Errorf("invalid v value: %d", s.V) + }tests/state_test_util.go (1)
131-138: Enhance documentation for the authorization structure.The struct documentation should explain the purpose of each field and their constraints according to EIP-7702.
-// Authorization is an authorization from an account to deploy code at it's address. +// stAuthorization represents an authorization from an account to deploy code at its address. +// Fields: +// - ChainID: The chain ID this authorization is valid for +// - Address: The address granting authorization for code deployment +// - Nonce: The account's authorization nonce (distinct from transaction nonce) +// - V: ECDSA recovery ID (0 or 1) +// - R: ECDSA signature parameter +// - S: ECDSA signature parameter type stAuthorization struct {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
rollup/ccc/async_checker_test.go(2 hunks)tests/gen_stauthorization.go(1 hunks)tests/gen_sttransaction.go(4 hunks)tests/state_test_util.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (4)
rollup/ccc/async_checker_test.go (2)
29-30: LGTM! Disabling Euclid features for testing.Setting
EuclidTimeandEuclidV2Timetonileffectively disables these features for testing purposes, which is appropriate for isolating the test behavior.
43-43: LGTM! Updated signer creation with block time.The
MakeSignercall now includes the block time parameter, which aligns with the EIP-7702 implementation requirements for transaction signing.tests/gen_sttransaction.go (1)
29-29: LGTM! The AuthorizationList implementation follows consistent patterns.The new field is correctly implemented:
- Uses consistent types and JSON tags
- Marked as optional with
omitempty- Proper null handling in UnmarshalJSON
Also applies to: 47-47, 64-64, 103-105
tests/state_test_util.go (1)
397-399: LGTM!The modification to include the authorization list in the NewMessage call is correct and aligns with the EIP-7702 implementation.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
cmd/evm/testdata/33/exp.json (1)
57-58:⚠️ Potential issueFix JSON syntax error.
There is an extra closing brace at the end of the file that would cause JSON parsing errors.
Apply this diff to fix the syntax:
-} -} +}
🧹 Nitpick comments (1)
tests/init.go (1)
235-257: Document the relationship between block-based and time-based parameters.The EuclidV2 fork configuration is well-structured and includes all necessary parameters. However, a few improvements would enhance maintainability:
- Consider adding documentation to explain the relationship between block-based parameters (e.g.,
HomesteadBlock) and time-based parameters (e.g.,DarwinTime).- Consider using a consistent initialization pattern across all parameters for better readability.
Consider applying this diff to improve consistency:
"EuclidV2": { ChainID: big.NewInt(1), HomesteadBlock: big.NewInt(0), // ... other block parameters ... - DarwinTime: new(uint64), - DarwinV2Time: new(uint64), - EuclidTime: new(uint64), - EuclidV2Time: new(uint64), + DarwinTime: big.NewInt(0).Uint64(), + DarwinV2Time: big.NewInt(0).Uint64(), + EuclidTime: big.NewInt(0).Uint64(), + EuclidV2Time: big.NewInt(0).Uint64(), },Additionally, consider adding a comment block above the fork configuration:
// EuclidV2 fork configuration includes both block-based and time-based parameters. // Block-based parameters (e.g., HomesteadBlock) determine features activated at specific block numbers. // Time-based parameters (e.g., DarwinTime) determine features activated at specific timestamps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Makefile(1 hunks)cmd/evm/t8n_test.go(3 hunks)cmd/evm/testdata/33/README.md(1 hunks)cmd/evm/testdata/33/alloc.json(1 hunks)cmd/evm/testdata/33/env.json(1 hunks)cmd/evm/testdata/33/exp.json(1 hunks)cmd/evm/testdata/33/txs.json(1 hunks)tests/init.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- cmd/evm/testdata/33/README.md
- cmd/evm/testdata/33/env.json
- cmd/evm/testdata/33/alloc.json
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (9)
cmd/evm/t8n_test.go (2)
127-134: Verify test coverage for missing blockhash behavior.The removal of this test case might leave a gap in test coverage for missing blockhash scenarios. Please ensure this behavior is tested elsewhere or confirm if this test is no longer relevant.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for other tests covering missing blockhash scenarios rg -A 5 "missing.*blockhash|blockhash.*missing" --type goLength of output: 382
Missing Blockhash Test Coverage Not Found
The only reference to missing blockhash behavior is in the commented-out test block in
cmd/evm/t8n_test.go, and no alternative tests appear to cover this scenario. Please confirm whether the missing blockhash case is intentionally untested or if additional test coverage is required.
151-158: Verify test coverage for already signed transactions.The removal of this test case might leave a gap in test coverage for already signed transactions. Please ensure this behavior is tested elsewhere or confirm if this test is no longer relevant.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for other tests covering already signed transactions rg -A 5 "already.*signed|signed.*transactions" --type goLength of output: 6214
Investigation on Already Signed Transactions Test Coverage
The search output shows that while the commented-out test case in
cmd/evm/t8n_test.gowas meant to cover already signed transactions, a number of files (e.g., incmd/evm/internal/t8ntool/transaction.goandcmd/evm/internal/t8ntool/transition.go) include logic for decoding and handling already signed transactions. However, there isn’t any additional explicit test case that targets the scenario of already signed transactions in the test suite.Action Items:
- Verify whether the functionality for handling already signed transactions is indirectly covered by other tests in the suite.
- If this behavior isn’t tested elsewhere, consider reintroducing the commented-out test or adding a dedicated test case for already signed transactions.
cmd/evm/testdata/33/txs.json (2)
14-31: Verify authorization list configuration.The authorization list contains entries that need verification:
- The second authorization has
chainId: "0x0"which is invalid for mainnet- The test addresses (
0x...aaaaand0x...bbbb) should be documented- Multiple authorizations support needs verification
Let's verify the authorization list implementation:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for authorization list validation logic ast-grep --pattern 'func $_($_, authorizationList []$_) { $$$ }' # Search for any documentation or constants defining these test addresses rg "0x000000000000000000000000000000000000aaaa" rg "0x000000000000000000000000000000000000bbbb"Length of output: 3014
Authorization List Configuration Verified.
- The test data intentionally includes multiple authorization entries covering different scenarios. The use of
"chainId": "0x0"is a deliberate test vector rather than a mainnet configuration.- The test addresses (
...aaaaand...bbbb) are consistently documented throughout the test datasets (e.g., in corresponding readme files and allocation files).- Multiple authorizations are supported, as evidenced by their use in various test cases.
3-12: Verify transaction type and gas parameters.The transaction uses type "0x4" (SetCodeTxType) with EIP-1559 fee model, but there are a few points that need verification:
- The empty input field (
0x) seems unusual for a SetCode transaction- The gas limit of 500,000 (0x7a120) needs validation against the expected gas costs for SetCode operations
Let's verify the transaction type and gas parameters in the codebase:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for SetCodeTxType definition and related gas costs rg -A 5 "SetCodeTxType.*=.*0x4" rg -A 5 "SetCodeGas"Length of output: 54
I'm not finding clear evidence in the codebase for definitions or validations related to
SetCodeTxTypeorSetCodeGas, which makes it difficult to conclusively verify that the gas parameters and the emptyinputfield are appropriate for a SetCode transaction per EIP‑7702. We need a manual verification to be certain that these fields conform to the EIP‑7702 requirements.Please verify:
- That the transaction type (
0x4) indeed corresponds to the intended SetCode operation.- That the gas limit
0x7a120and EIP‑1559 fee fields (maxPriorityFeePerGas,maxFeePerGas) are correct for a SetCode transaction.- Whether the empty
inputfield is expected for these transactions.I'll run an additional search in the repository to try and locate any related definitions:
Since the previous search did not yield results and this additional search is also likely to be inconclusive, a manual review of the EIP‑7702 implementation and its corresponding code sections is recommended.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Searching for 'SetCodeTxType' and related definitions..." rg -i "SetCodeTxType" echo "Searching for any references to 'EIP-7702'..." rg -i "EIP-7702"Length of output: 3902
Action Required: Verify EIP‑7702 Gas & Input Specifications in SetCodeTx Implementation
The transaction type
"0x4"is correctly linked to the definedSetCodeTxTypein the codebase, and the EIP‑1559 fee fields (withnullforgasPrice) are in line with expectations. However, two points still need manual confirmation:
- Empty input field: Confirm that having
"input": "0x"is acceptable for a SetCode transaction. It might be the intended design if the transaction’s payload is provided elsewhere (for example, via an authorization list).- Gas limit: Verify that the gas limit (
0x7a120, or 500,000 gas) meets the expected requirements for a SetCode operation under EIP‑7702, considering any additional gas costs linked to authorization or delegation.Please review the related implementation in
core/types/setcode_tx.goand the EIP‑7702 specification to ensure these values are correct.cmd/evm/testdata/33/exp.json (5)
14-21: Verify storage slot value consistency.The storage slot 0x42 is set to the same value (0x42). This appears to be a test case for verifying storage operations after SetCode execution.
27-35: Review complex contract interaction pattern.The bytecode at address 0x8a0a... contains multiple CALL operations (0x5af1) and storage operations. This appears to be testing the interaction between contracts during SetCode operations.
The storage layout with slots 1, 2, and 3 is consistent with the expected test results.
43-52: Verify transaction type and gas values.The receipt shows a type 4 transaction (SetCode) with successful execution. The gas values are consistent:
- gasUsed in receipt: 0x15fa9
- cumulativeGasUsed: 0x15fa9
38-41: Verify state root calculations.The state, transaction, and receipt roots should be verified against independent calculations to ensure correctness.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Verify root calculations using go-ethereum's state-root calculation # Use go-ethereum's state-test tool to verify the roots evm state-test cmd/evm/testdata/33/exp.json --jsonLength of output: 146
Manual Verification of State Root Calculations Required
The test script failed because the
evmcommand was not found in the current environment. This suggests that the automated verification cannot proceed as expected, and you will need to verify the state, transaction, and receipt roots manually using the appropriate tooling or a local setup with go-ethereum's state-test tool.
3-6: Verify the bytecode for potential security implications.The bytecode at address 0x...aaaa contains a SETCODE operation (0x5a) targeting another address. This should be carefully reviewed for security implications.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Analyze the bytecode for security implications # Parse the bytecode and check for potentially dangerous operations echo "0x58808080600173703c4b2bd70c169f5717101caee543299fc946c75af100" | python3 -c ' import sys import binascii def analyze_bytecode(hex_str): # Remove 0x prefix if present hex_str = hex_str.strip().replace("0x", "") # Convert to bytes bytecode = binascii.unhexlify(hex_str) # List of potentially dangerous opcodes dangerous_ops = { 0x5A: "SETCODE", 0xF1: "CALL", 0xF2: "CALLCODE", 0xF4: "DELEGATECALL", 0xFA: "STATICCALL", 0xFF: "SELFDESTRUCT" } print("Bytecode analysis:") for i, byte in enumerate(bytecode): if byte in dangerous_ops: print(f"Position {i}: Found {dangerous_ops[byte]}") analyze_bytecode(sys.stdin.read().strip()) 'Length of output: 824
Bytecode analysis confirms the expected opcodes in the test data.
The analysis shows that the account's bytecode includes a dangerous opcode, SETCODE (position 27), as well as a CALL opcode (position 28). Since this file is a test data file for EVM testing (specifically related to EIP-7702 SetCode transactions), the presence of these opcodes is intentional for testing purposes rather than an oversight.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/vm/runtime/runtime_test.go (1)
987-1035: Consider adding test case descriptions and warm access tests.The test cases provide good coverage of different call types. Consider these improvements:
- Add comments describing each test case's purpose
- Add test cases for warm access (second call to same address)
- Add negative test cases (e.g., invalid delegation)
for i, tc := range []struct { code []byte step int want uint64 }{ + // Test case 1: Cold access to delegated account via CALL { // CALL(0xff) code: []byte{ byte(vm.PUSH1), 0x0,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/evm/testdata/33/txs.json(1 hunks)core/bench_test.go(2 hunks)core/vm/runtime/runtime_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/bench_test.go
🧰 Additional context used
🪛 Gitleaks (8.21.2)
cmd/evm/testdata/33/txs.json
32-32: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (5)
core/vm/runtime/runtime_test.go (3)
23-23: LGTM! Required imports for mocking.The new imports
reflectandgomonkeyare necessary for mocking theGetPoseidonCodeHashmethod in the test.Also applies to: 28-28
971-986: LGTM! Well-structured test setup.The test setup is clear and follows good practices:
- Creates state DB with proper initialization
- Sets up delegation from 0xff to 0xaa
- Uses gomonkey for mocking with proper cleanup
1036-1052: LGTM! Thorough test execution and validation.The test execution follows the pattern used in other gas cost tests in the file:
- Uses StructLogger for accurate gas tracking
- Provides detailed error reporting
- Validates gas costs at specific execution steps
cmd/evm/testdata/33/txs.json (2)
2-13: Verify empty input data for SetCode transaction.The transaction is of type "0x4" (SetCode) but has empty input data. This seems unusual as SetCode transactions typically require code data in the input field.
Please verify if this is the intended behavior for this test case.
32-35: Document test credentials and clarify signature fields.The test data contains:
- A private key that should be documented as test-only
- Zero values for signature fields (v, r, s) which might indicate incomplete test data
Consider adding a comment at the top of the file:
[ + // Test transaction for EIP-7702 SetCode operation + // Note: Contains test-only credentials that should never be used in production {🧰 Tools
🪛 Gitleaks (8.21.2)
32-32: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| output: t8nOutput{alloc: true, result: true}, | ||
| expExitCode: 4, | ||
| }, | ||
| //{ // missing blockhash test |
There was a problem hiding this comment.
They were legacy issues before this PR (I mistakenly marked them "regression problems" in the PR description, and changed that). Fixed one here: 2dc0756.
The fixed one is a state root mismatch, it's due to l2geth enabling 1559 transactions in Curie fork (instead of London fork), so after adding Curie fork block number 0 in "London" config it works again, otherwise the state transition would be different.
The trickier one is the "missing blockhash" case, I haven't found a clue yet. The test case is expected to return this error:
But during the test, it applies the transaction but does not call this function:
go-ethereum/cmd/evm/internal/t8ntool/execution.go
Lines 104 to 117 in 2dc0756
which is the only place to return a
hashError, to trigger ErrorMissingBlockhash here: go-ethereum/cmd/evm/internal/t8ntool/execution.go
Lines 190 to 192 in 2dc0756
| } | ||
|
|
||
| // Update nonce and account code. | ||
| st.state.SetNonce(authority, auth.Nonce+1) |
There was a problem hiding this comment.
I noticed that upstream geth has a tracing param here. I guess they have a different tracing method. For us, will this update be visibile on the trace?
There was a problem hiding this comment.
will revisit this and give a summary. current knowledge from my side is the tracing in l2geth repo has some divergence with upstream. will evaluate the impact.
There was a problem hiding this comment.
Some results:
- These two functions: https://github.com/ethereum/go-ethereum/blob/v1.15.1/core/tracing/CHANGELOG.md?plain=1#L144-L145 are used to capture: contract creation, "to" field in txn, account lookup in EIP-7702 authorities, etc, which is added in upstream, not in l2geth repo.
- the nonce update tracing in
SetNoncehere is supported byOnNonceChange, andOnNonceChangeV2, which currently in our repo it's not supported. - More change logs here: https://github.com/ethereum/go-ethereum/blob/v1.15.1/core/tracing/CHANGELOG.md
There was a problem hiding this comment.
Afaik this doesn't matter for our provers, but it might matter for other dapps consuming traces (e.g. Tenderly), might need to look more into the code and/or sync with those stakeholders.
There was a problem hiding this comment.
sounds good. will fix this and can test tracing in Tenderly after sepolia testnet is upgraded.
This reverts commit 314478303d4a504a3948bd6595568916ba7d82f0.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/tx_pool.go (1)
757-760: 🛠️ Refactor suggestionConsider rejecting SetCode transactions before activation.
Similar to how EIP-1559 transactions are rejected before activation, consider rejecting SetCode transactions when EIP-7702 is not active.
Apply this diff to add the activation check:
// Reject set code transactions until EIP-7702 activates. if !pool.eip7702 && tx.Type() == types.SetCodeTxType { + if !pool.chainconfig.IsEuclidV2(pool.chain.CurrentBlock().Time()) { + return ErrTxTypeNotSupported + } return ErrTxTypeNotSupported }
🧹 Nitpick comments (2)
core/tx_pool.go (2)
822-823: Enhance code presence detection.The current check for deployed code using
GetKeccakCodeHash() != EmptyKeccakCodeHashis correct but could be made more robust by also checking the code length.Apply this diff to add a length check:
- if pool.currentState.GetKeccakCodeHash(addr) != codehash.EmptyKeccakCodeHash || len(pool.all.auths[addr]) != 0 { + code := pool.currentState.GetCode(addr) + if len(code) > 0 || len(pool.all.auths[addr]) != 0 {
2222-2237: Optimize authority tracking operations.The authority removal logic can be optimized in two ways:
- Use a more efficient slice operation
- Add more context to error logging
Apply this diff to optimize the code:
func (t *txLookup) removeAuthorities(hash common.Hash) { for addr := range t.auths { list := t.auths[addr] - // Remove tx from tracker. - if i := slices.Index(list, hash); i >= 0 { - list = append(list[:i], list[i+1:]...) + // Use filtering for more efficient removal + newList := make([]common.Hash, 0, len(list)) + found := false + for _, h := range list { + if h != hash { + newList = append(newList, h) + } else { + found = true + } + } + if !found { + log.Error("Authority with untracked tx", + "addr", addr, + "hash", hash, + "list_size", len(list)) } else { - log.Error("Authority with untracked tx", "addr", addr, "hash", hash) + list = newList } if len(list) == 0 { // If list is newly empty, delete it entirely. delete(t.auths, addr) continue } t.auths[addr] = list } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/tx_pool.go(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
core/tx_pool.go (1)
82-84: LGTM! Well-defined error variables and fork indicator.The new error variables are clearly defined with descriptive messages, and the fork indicator follows the established pattern.
Also applies to: 99-102, 295-295
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/vm/evm.go (1)
542-569: LGTM! Well-documented implementation of code resolution with delegation.The implementation correctly:
- Guards delegation behind EuclidV2 flag
- Explicitly limits delegation to a single level
- Handles both code and code hash resolution
- Provides clear documentation
The single-level delegation limit is a good design choice as it:
- Prevents infinite delegation loops
- Makes the delegation chain predictable
- Reduces gas costs
core/state_transition.go (1)
414-431: Consider logging invalid authorizations.While silently skipping invalid authorizations is acceptable, logging them would help with debugging and monitoring.
- // Note errors are ignored, we simply skip invalid authorizations here. - st.applyAuthorization(&auth) + // Log and skip invalid authorizations. + if err := st.applyAuthorization(&auth); err != nil { + log.Debug("Skipping invalid authorization", "error", err, "auth", auth) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/evm/t8n_test.go(2 hunks)core/state_transition.go(8 hunks)core/types/receipt.go(3 hunks)core/vm/evm.go(6 hunks)tests/init.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/evm/t8n_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (15)
core/types/receipt.go (3)
239-239: LGTM! Support added for SetCodeTxType.The
SetCodeTxTypehas been correctly added to the list of supported transaction types in thedecodeTypedmethod.
422-434: LGTM! Transaction type handling has been simplified.The implementation has been simplified as suggested in the past review comments. The code now handles legacy transactions separately and uses a single switch statement for other transaction types, improving readability and maintainability.
439-440: LGTM! Time parameter added to DeriveFields.The
DeriveFieldsmethod has been correctly updated to include thetimeparameter, which is now passed toMakeSigner. This change aligns with EIP-7702's requirements for including block timestamps in signer creation.core/vm/evm.go (4)
233-234: LGTM! Clean implementation of code resolution with delegation support.The changes correctly integrate EIP-7702 delegation support while maintaining the existing call semantics.
Also applies to: 241-241
301-301: LGTM! Proper implementation of delegation support in CallCode.The change correctly integrates both code and code hash resolution for delegation support.
344-344: LGTM! Proper implementation of delegation support in DelegateCall.The change correctly integrates both code and code hash resolution for delegation support.
400-400: LGTM! Proper implementation of delegation support in StaticCall.The change correctly integrates both code and code hash resolution for delegation support.
tests/init.go (2)
184-184: LGTM!The comment clarifies that EIP-1559 is enabled in the Curie fork, which is helpful for understanding the fork configuration.
236-258: LGTM!The EuclidV2 fork configuration is correctly added with all blocks set to 0 and new time fields properly initialized. This configuration is required for t8n tests to pass.
core/state_transition.go (6)
92-92: LGTM!The
SetCodeAuthorizationsmethod is correctly added to theMessageinterface as part of EIP-7702 implementation.
132-132: LGTM!The
IntrinsicGasfunction is correctly updated to handle EIP-7702 authorization list gas calculation, with proper overflow protection.Also applies to: 178-180
295-299: LGTM!The preCheck function is correctly updated to handle both EOA and delegated accounts, with clear error messages.
330-338: LGTM!The EIP-7702 authorization list validation is correctly implemented with proper checks and clear error messages.
476-505: LGTM!The
validateAuthorizationfunction implements thorough validation of EIP-7702 authorizations with clear error messages.
507-532: LGTM!The
applyAuthorizationfunction correctly implements EIP-7702 authorization application with proper gas refund handling.
1. Purpose or design rationale of this PR
This PR migrates EIP-7702 implementations and unit tests from the upstream geth repo.
All added unit tests have passed. Except for the fuzz tests under
./tests, historical modifications in this repo make the fuzz tests fail in several places.Some test cases are commented out in
cmd/evm/t8n_test.gobecause they are legacy problems and are out of the scope of this PR. EIP-7702 tests are passed incmd/evm/t8n_test.go.Related upstream PRs:
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Bug Fixes
Tests