feat: feynman upgrade vm changes #1193
Conversation
WalkthroughThis update introduces the "Feynman" hard fork across the codebase, adding support for EIP-2935 (historical block hash storage) and EIP-7623 (transaction data gas floor). It includes new configuration fields, fork activation logic, precompile contract changes, opcode behavior updates, new error types, and extended test coverage for the new features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TxPool
participant StateProcessor
participant EVM
participant HistoryContract
User->>TxPool: Submit Transaction
TxPool->>TxPool: Validate Tx (includes floor data gas if Feynman)
TxPool->>StateProcessor: Accept Valid Tx
StateProcessor->>EVM: Create EVM Context
StateProcessor->>HistoryContract: ProcessParentBlockHash (if Feynman)
StateProcessor->>EVM: Apply Transaction
EVM->>HistoryContract: Store Parent Block Hash (EIP-2935)
sequenceDiagram
participant EVM
participant Contract
participant BlockContext
Contract->>EVM: Execute BLOCKHASH opcode
EVM->>BlockContext: GetHash(number)
BlockContext-->>EVM: Return block hash (direct, no mixing if Feynman)
EVM-->>Contract: Push block hash to stack
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
♻️ Duplicate comments (1)
core/vm/jump_table.go (1)
72-72: Fix typo in comment.There's a spelling error in the comment.
-// contantinople, istanbul, petersburg, berlin, london, shanghai, curie, darwin, euclidV2, +// constantinople, istanbul, petersburg, berlin, london, shanghai, curie, darwin, euclidV2,
🧹 Nitpick comments (1)
core/state_processor.go (1)
207-229: Consider improving error handling and using constants for magic numbers.The
ProcessParentBlockHashfunction correctly implements EIP-2935 logic, but there are a few improvements to consider:
Error handling: The return values from
vmenv.Callare being ignored. While this might be intentional for system operations, consider at least logging failures for debugging purposes.Magic numbers: The gas limit
30_000_000appears twice and should be extracted as a named constant.+const ( + // HistoryStorageGasLimit is the gas limit for EIP-2935 parent block hash storage + HistoryStorageGasLimit = 30_000_000 +) + func ProcessParentBlockHash(prevHash common.Hash, vmenv *vm.EVM, statedb *state.StateDB) { msg := types.NewMessage( params.SystemAddress, // from ¶ms.HistoryStorageAddress, // to 0, // nonce common.Big0, // amount - 30_000_000, // gasLimit + HistoryStorageGasLimit, // gasLimit common.Big0, // gasPrice common.Big0, // gasFeeCap common.Big0, // gasTipCap prevHash.Bytes(), // data nil, // accessList false, // isFake nil, // setCodeAuthorizations ) vmenv.Reset(NewEVMTxContext(msg), statedb) statedb.AddAddressToAccessList(params.HistoryStorageAddress) - _, _, _ = vmenv.Call(vm.AccountRef(msg.From()), *msg.To(), msg.Data(), 30_000_000, common.Big0, nil) + ret, leftOverGas, err := vmenv.Call(vm.AccountRef(msg.From()), *msg.To(), msg.Data(), HistoryStorageGasLimit, common.Big0, nil) + if err != nil { + log.Debug("EIP-2935 parent block hash storage failed", "err", err, "ret", ret, "gas", leftOverGas) + } statedb.Finalise(true) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
cmd/evm/internal/t8ntool/execution.go(1 hunks)cmd/evm/internal/t8ntool/transaction.go(1 hunks)core/error.go(1 hunks)core/genesis.go(1 hunks)core/state_processor.go(2 hunks)core/state_processor_test.go(5 hunks)core/state_transition.go(7 hunks)core/tx_pool.go(3 hunks)core/vm/contracts.go(8 hunks)core/vm/contracts_test.go(2 hunks)core/vm/evm.go(1 hunks)core/vm/instructions.go(1 hunks)core/vm/interpreter.go(1 hunks)core/vm/jump_table.go(1 hunks)core/vm/runtime/runtime.go(1 hunks)core/vm/runtime/runtime_test.go(1 hunks)core/vm/testdata/precompiles/bn256Pairing.json(1 hunks)eth/state_accessor.go(1 hunks)eth/tracers/api.go(4 hunks)miner/scroll_worker.go(1 hunks)miner/scroll_worker_test.go(1 hunks)params/config.go(9 hunks)params/protocol_params.go(4 hunks)params/version.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
cmd/evm/internal/t8ntool/transaction.go (3)
params/config.go (1)
Rules(1212-1219)core/state_transition.go (2)
IntrinsicGas(133-179)FloorDataGas(182-194)core/error.go (2)
ErrIntrinsicGas(82-82)ErrFloorDataGas(86-86)
core/vm/jump_table.go (1)
core/vm/opcodes.go (1)
BLOCKHASH(98-98)
cmd/evm/internal/t8ntool/execution.go (2)
core/vm/evm.go (2)
NewEVM(141-152)TxContext(93-100)core/state_processor.go (1)
ProcessParentBlockHash(209-229)
core/genesis.go (1)
params/protocol_params.go (2)
HistoryStorageAddress(193-193)HistoryStorageCode(195-195)
params/protocol_params.go (2)
common/types.go (1)
HexToAddress(218-218)common/bytes.go (1)
FromHex(29-37)
core/state_processor_test.go (5)
core/state/statedb.go (2)
StateDB(65-132)New(135-163)params/protocol_params.go (3)
HistoryStorageAddress(193-193)HistoryStorageCode(195-195)HistoryServeWindow(178-178)core/evm.go (1)
NewEVMBlockContext(40-68)core/vm/evm.go (2)
NewEVM(141-152)TxContext(93-100)core/state_processor.go (1)
ProcessParentBlockHash(209-229)
core/tx_pool.go (2)
core/state_transition.go (1)
FloorDataGas(182-194)core/error.go (1)
ErrFloorDataGas(86-86)
core/vm/evm.go (1)
core/vm/contracts.go (1)
PrecompiledContractsFeynman(146-157)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
🔇 Additional comments (37)
params/version.go (1)
27-27: Version bump reflects the Feynman upgrade implementation.The patch version increment from 53 to 54 appropriately signals the inclusion of the Feynman fork changes and associated protocol upgrades.
core/vm/runtime/runtime.go (1)
77-78: Fork time fields properly added to runtime defaults.The addition of
EuclidV2TimeandFeynmanTimefields maintains consistency with the chain configuration and ensures the runtime environment properly supports the new fork activation logic.miner/scroll_worker_test.go (1)
1407-1407: Test configuration properly isolates EuclidV2 transition from Feynman fork.Explicitly setting
FeynmanTime = nilensures the EuclidV2 transition verification test is not affected by the newly introduced Feynman fork configuration.eth/state_accessor.go (1)
179-184:✅ Verification successful
EIP-2935 implementation correctly processes parent block hash for Feynman fork.
The logic properly:
- Checks for Feynman fork activation at the block timestamp
- Creates appropriate EVM context and environment
- Processes the parent block hash before transaction execution
Please verify that
core.ProcessParentBlockHashis properly implemented and handles the EIP-2935 requirements correctly:
🏁 Script executed:
#!/bin/bash # Verify ProcessParentBlockHash implementation ast-grep --pattern $'func ProcessParentBlockHash($_, $_, $_) { $$$ }'Length of output: 1551
Confirmed: ProcessParentBlockHash Implements EIP-2935 Correctly
I verified the implementation in core/state_processor.go (lines 209–229). It:
- Constructs a system message carrying the parent block hash
- Resets the EVM with the new transaction context
- Adds the HistoryStorage contract to the access list
- Invokes the call to record the hash
- Finalises the state
This aligns with EIP-2935’s requirement to store the parent block hash before transaction execution. No further changes required.
core/error.go (1)
84-86: New error type properly defined for floor data gas validation.
ErrFloorDataGasis correctly positioned among the EVM call message pre-checking errors and provides a clear distinction fromErrIntrinsicGasfor the new floor data gas cost validation introduced by the Feynman fork (EIP-7623).core/vm/interpreter.go (1)
77-78: LGTM: Feynman fork instruction set integration follows established patterns.The addition of Feynman fork detection and
feynmanInstructionSetassignment is correctly implemented and follows the established pattern for fork-specific instruction set selection. The placement at the top of the switch statement ensures proper precedence when the Feynman fork is active.core/vm/evm.go (1)
50-51: LGTM: Feynman precompile selection properly integrated.The addition of Feynman fork detection and
PrecompiledContractsFeynmanassignment correctly follows the established pattern for fork-specific precompile selection. This integrates well with thePrecompiledContractsFeynmanmap defined incore/vm/contracts.goand ensures the appropriate precompiles are used when the Feynman fork is active.core/genesis.go (1)
503-504: LGTM: EIP-2935 history contract pre-deployment correctly implemented.The pre-deployment of the EIP-2935 history contract in the developer genesis is correctly implemented. The configuration uses the appropriate constants from
params.HistoryStorageAddressandparams.HistoryStorageCode, sets the nonce to 1 and balance to zero, following standard patterns for system contract deployment. This ensures the history contract is available for EIP-2935 functionality from genesis.core/vm/runtime/runtime_test.go (1)
340-347: LGTM: Test expectations correctly updated for Feynman BLOCKHASH behavior.The test expectation updates correctly reflect the new
BLOCKHASHopcode behavior introduced by the Feynman fork. The changes from complex hash values to simple uint64 values (999, 744) and the iteration count change to 255 align with the newopBlockhashPostFeynmanimplementation that returns stored block hashes directly, supporting EIP-2935 historical block hash functionality.core/vm/contracts_test.go (2)
68-68: LGTM: New precompile entry adds test coverage for input length validation.The addition of
bn256PairingIstanbul{limitInputLength: true}at address 0xf8 provides test coverage for the input length validation variant of the bn256 pairing precompile, which aligns with the Feynman fork's precompile configuration changes.
268-268: LGTM: Test function correctly updated to use length-limited precompile.The update to use address "f8" ensures the failure test cases are properly validated against the bn256 pairing precompile with input length limitations enabled, providing comprehensive test coverage for the Feynman fork's precompile behavior.
core/vm/jump_table.go (2)
65-65: LGTM! Feynman instruction set properly declared.The new instruction set variable follows the established naming convention and is correctly positioned among other instruction sets.
71-81: LGTM! Clean implementation of Feynman instruction set.The function correctly extends
euclidV2InstructionSetand overrides only theBLOCKHASHopcode to use the newopBlockhashPostFeynmanimplementation, which is appropriate for the Feynman fork's EIP-2935 support.cmd/evm/internal/t8ntool/execution.go (1)
155-163: LGTM! Proper EIP-2935 integration.The conditional processing correctly:
- Verifies BlockHashes availability before proceeding
- Checks for Feynman fork activation using
chainConfig.IsFeynman()- Retrieves the previous block hash appropriately
- Creates a new EVM instance with the current context
- Calls
core.ProcessParentBlockHashwith the correct parametersThis integrates well with the broader Feynman fork implementation and follows the established pattern for parent block hash processing.
cmd/evm/internal/t8ntool/transaction.go (2)
143-155: LGTM! Improved gas validation logic.The refactoring consolidates multiple individual version checks (
IsHomestead,IsIstanbul,IsShanghai) into a singlechainConfig.Rules()call, which is cleaner and more efficient. The explicit intrinsic gas validation with proper error handling is an improvement over the previous implicit approach.
156-169: LGTM! Correct Feynman floor data gas validation.The new validation correctly:
- Checks if Feynman rules are active before applying the floor data gas requirement
- Uses
core.FloorDataGas()to calculate the required gas floor- Properly handles errors and continues to the next transaction on validation failure
- Uses the appropriate error type
core.ErrFloorDataGasThis implementation aligns with the EIP-7623 requirements for the Feynman fork.
core/vm/instructions.go (1)
473-494: LGTM! Correct post-Feynman BLOCKHASH implementation.The new function properly implements the modified BLOCKHASH opcode behavior for the Feynman fork:
- Maintains the same stack operations and range validation logic as the original
opBlockhash- Correctly handles overflow conditions by clearing the stack value
- Key improvement: directly returns the block hash from
GetHash()instead of computing a hash with chain ID mixing- Properly validates the 256-block range constraint
This implementation aligns with EIP-2935's approach of serving historical block hashes directly from state storage rather than computing them on-the-fly.
core/vm/testdata/precompiles/bn256Pairing.json (1)
79-92: LGTM! Comprehensive test coverage for Feynman fork.The new test cases appropriately validate the bn256 pairing precompile under the Feynman fork:
- "ten_point_match_1" and "ten_point_match_2" test scenarios with 10 pairing points each
- Large input sizes align with the Feynman fork's removal of input length restrictions
- Gas costs (385,000) correctly reflect the computational complexity of processing multiple pairing points
- Expected successful pairing results ("1") are appropriate for valid matching point pairs
- Test structure follows the established pattern with proper hex encoding
These additions ensure thorough testing of the precompile's behavior under the new fork conditions.
eth/tracers/api.go (1)
280-284: LGTM: Consistent EIP-2935 integration across tracing functions.The implementation correctly adds
core.ProcessParentBlockHashcalls in all relevant tracing contexts when the Feynman fork is active. The pattern is consistent acrosstraceChain,IntermediateRoots,traceBlock, andstandardTraceBlockToFilefunctions, properly ensuring parent block hash insertion before transaction processing during tracing operations.Also applies to: 547-551, 627-631, 767-771
core/state_processor.go (1)
97-100: LGTM: Proper EIP-2935 integration in state processing.The conditional call to
ProcessParentBlockHashis correctly placed before transaction processing when the Feynman fork is active, ensuring parent block hash storage occurs at the appropriate time in the block execution flow.miner/scroll_worker.go (1)
640-644: LGTM: Consistent Feynman fork handling in mining workflow.The implementation correctly adds EIP-2935 parent block hash processing to the mining workflow when the Feynman fork is active. The pattern is consistent with other components and ensures proper integration during block construction.
core/tx_pool.go (3)
300-300: LGTM: Consistent fork indicator implementationThe addition of the
feynmanfield follows the established pattern for tracking fork activation states in the transaction pool.
859-868: LGTM: Proper EIP-7623 gas floor validation implementationThe Feynman fork validation correctly implements the transaction data gas floor check:
- Properly guarded behind fork activation check
- Handles errors from
FloorDataGascalculation- Provides informative error message with actual vs required gas
- Uses the appropriate error type
ErrFloorDataGasThis aligns with the EIP-7623 specification for transaction data gas floor enforcement.
1597-1597: LGTM: Correct fork state initializationThe
feynmanflag is properly initialized using the chain configuration's timestamp-based fork activation check, consistent with other time-based fork indicators in the codebase.core/state_processor_test.go (5)
21-21: LGTM: Necessary imports for new test functionalityThe added imports (
encoding/binary,core/state,ethdb/memorydb) are properly used in the newTestProcessParentBlockHashtest function for binary operations, state database manipulation, and in-memory database creation.Also applies to: 35-35, 39-39
70-70: LGTM: Proper Feynman fork configuration for testingThe
FeynmanTime: new(uint64)configuration correctly enables the Feynman fork at timestamp 0 for testing purposes, following the same pattern as other time-based fork configurations in the test suite.Also applies to: 483-483
394-394: LGTM: Test maintenance for gas limit adjustmentThe gas limit increase from 500,000 to 520,000 and corresponding error hash update appear to be necessary adjustments to maintain test accuracy, likely due to changes in gas calculations introduced by the Feynman upgrade.
Also applies to: 396-396
461-517: LGTM: Comprehensive EIP-2935 test coverageThe
TestProcessParentBlockHashtest function provides excellent coverage of the EIP-2935 parent block hash storage functionality:
- Proper test setup with chain configuration and state database initialization
- Tests the
ProcessParentBlockHashfunction with different parent hashes and block numbers- Correctly initializes the history storage contract with required nonce and code
- Verifies storage correctness using the ring buffer indexing mechanism
- Clear test structure with descriptive assertions
This test effectively validates the core EIP-2935 functionality integration.
519-524: LGTM: Correct EIP-2935 storage retrieval implementationThe
getParentBlockHashhelper function properly implements the EIP-2935 storage scheme:
- Correctly calculates ring buffer index using modulo
HistoryServeWindow- Properly constructs the 32-byte storage key with big-endian encoding
- Accurately retrieves the stored parent block hash from state
This aligns with the EIP-2935 specification for historical block hash storage and retrieval.
params/protocol_params.go (1)
19-24: LGTM! The new protocol parameters for EIP-2935 and EIP-7623 are properly defined.The additions include:
- Token-based gas cost parameters for EIP-7623
- History storage window for EIP-2935
- System addresses and contract code for historical block hash storage
All constants are well-documented with their respective EIP references.
Also applies to: 94-95, 178-178, 189-196
core/state_transition.go (3)
145-147: Good optimization usingbytes.Count.This is more efficient than manual iteration for counting zero bytes.
181-194: Proper implementation of EIP-7623 floor data gas calculation.The function correctly:
- Calculates token cost based on zero and non-zero bytes
- Includes overflow protection
- Returns the minimum required gas
408-417: Correct enforcement of floor data gas under Feynman rules.The implementation properly:
- Validates gas limit meets floor data gas requirement before execution
- Ensures final gas usage doesn't fall below floor data gas after refunds
Also applies to: 483-491
params/config.go (1)
333-333: Consistent implementation of Feynman fork configuration.The changes properly integrate the Feynman fork following the established patterns for time-based forks.
Also applies to: 385-385, 526-526, 668-668, 859-863, 887-887, 1000-1003, 1218-1218, 1246-1246
core/vm/contracts.go (3)
83-83: Correct configuration oflimitInputLengthfor existing precompile sets.The field is properly set to:
falsefor Istanbul and Berlin (no limit)truefor Archimedes, Bernoulli, and EuclidV2 (4-pair limit)Also applies to: 97-97, 111-111, 125-125, 139-139
144-157: Proper implementation of Feynman precompiled contracts.The Feynman fork correctly removes the ecPairing input limit by setting
limitInputLength: falseon line 154, aligning with the PR objectives.
664-670: Conditional input length check correctly implements fork-specific behavior.The logic properly enforces the 4-pair limit only when
limitInputLengthis true, allowing unlimited pairs for Feynman and other applicable forks.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/vm/interface.go (1)
22-24: Newstatelessimport creates an upward dependency
vmnow depends oncore/stateless, reversing the previous layering and enlarging the public surface ofvm. To keepvmindependent, introduce a tiny local interface, e.g.:type Witness interface{ /* read-only methods needed here */ }and let
stateless.Witnesssatisfy it, sovmstays decoupled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/state/statedb.go(1 hunks)core/state_processor.go(2 hunks)core/state_transition.go(6 hunks)core/vm/instructions.go(1 hunks)core/vm/interface.go(2 hunks)miner/scroll_worker.go(1 hunks)params/protocol_params.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- core/state_processor.go
- miner/scroll_worker.go
- core/vm/instructions.go
- params/protocol_params.go
- core/state_transition.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/vm/interface.go (1)
core/stateless/witness.go (1)
Witness(38-47)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: semgrep/ci
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
core/vm/interface.go (1)
88-91: Interface addition breaks downstream implementationsEvery custom
StateDB(mocks, light-client adapters, tests) must now implementWitness(). Please audit external packages or add a shim interface to avoid sudden build failures.Example minimal stub:
func (*MyStateDB) Witness() *stateless.Witness { return nil }
1. Purpose or design rationale of this PR
I recommend reviewing commit-by-commit.
This PR includes the following changes:
ecPairingprecompile.blockhashopcode.Relevant upstream PRs:
Relevant past Scroll 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
Chores