Refactor : Amortize Opcode stack bound checks#10138
Refactor : Amortize Opcode stack bound checks#10138
Conversation
|
Will it potentially mess with tracing and need we support 2 paths? |
I am still analyzing the effect it will have, although i doubt it will change much (since it is still an opcode level change) it doesnt amortize chunks |
There was a problem hiding this comment.
Pull request overview
This PR refactors the EVM stack implementation by moving stack bound checking from the EvmStack class to individual opcode implementations. The goal is to amortize stack overflow and underflow checks by performing them once per opcode rather than on each push/pop operation.
Key Changes:
- Removed bounds checking from all
EvmStackpush/pop methods, changing return types frombooltovoid - Added
CheckStackUnderflowandCheckStackOverflowhelper methods inEvmInstructions.Stack.cs - Updated all opcode instructions across 14 files to check stack bounds before performing stack operations
- Added interface methods to operation types (e.g.,
IOpMath2Param,IOpCall) for consistent stack checking
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| EvmStack.cs | Removed all bounds checking from push/pop operations; changed methods from returning bool to void; added unused PopUInt256Unchecked method; removed inline comments |
| EvmInstructions.Stack.cs | Added CheckStackUnderflow and CheckStackOverflow helper methods; updated POP, PUSH, DUP, SWAP, and LOG instructions with explicit stack checks |
| EvmInstructions.Storage.cs | Added stack underflow checks to TLOAD, TSTORE, SLOAD, SSTORE, MLOAD, MSTORE, MSTORE8, MCOPY, and CALLDATALOAD |
| EvmInstructions.Shifts.cs | Added stack underflow checks to shift operations (SAR, SHL, SHR) via interface method |
| EvmInstructions.Math3Param.cs | Added stack underflow check via interface method for 3-parameter math operations |
| EvmInstructions.Math2Param.cs | Added stack underflow checks via interface method for 2-parameter math operations; moved stack check before gas consumption in EXP |
| EvmInstructions.Math1Param.cs | Added stack underflow checks for 1-parameter operations, BYTE, and SIGNEXTEND |
| EvmInstructions.Eof.cs | Added stack checks to EOF-specific instructions including RETURNDATASIZE, RETURNDATACOPY, DATALOAD, DATALOADN, DATACOPY, DUPN, SWAPN, EOFCREATE, RETURNCODE, RETURNDATALOAD, and EOFCALL |
| EvmInstructions.Environment.cs | Added stack overflow checks to instructions that push values and underflow checks to instructions that pop values |
| EvmInstructions.Crypto.cs | Added stack underflow check to KECCAK256 instruction |
| EvmInstructions.Create.cs | Added stack underflow checks via interface to CREATE and CREATE2 instructions |
| EvmInstructions.ControlFlow.cs | Added stack checks to PC, JUMP, JUMPI, REVERT, and SELFDESTRUCT instructions |
| EvmInstructions.CodeCopy.cs | Added stack underflow checks to CODECOPY, EXTCODECOPY, and EXTCODESIZE; introduced logic bug in peephole optimization |
| EvmInstructions.Call.cs | Added stack underflow checks via interface to CALL, CALLCODE, DELEGATECALL, STATICCALL, and RETURN instructions |
| EvmInstructions.Bitwise.cs | Added stack underflow checks via interface to bitwise operations |
| DebugTracerTests.cs | Updated test to check stack Head before calling PopLimbo |
src/Nethermind/Nethermind.Evm/Instructions/EvmInstructions.CodeCopy.cs
Outdated
Show resolved
Hide resolved
…eCopy.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
LukaszRozmej
left a comment
There was a problem hiding this comment.
I like the idea
@benaadams this needs a deep review
src/Nethermind/Nethermind.Evm/Instructions/EvmInstructions.CodeCopy.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Evm/Instructions/EvmInstructions.Stack.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Connected to #10120 , is it still needed? |
Fixes Closes Resolves #
Please choose one of the keywords above to refer to the issue this PR solves followed by the issue number (e.g. Fixes #000). If no issue number, remove the line. Also, remove everything marked optional that is not applicable. Remove this note after reading.
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Optional. Remove if not applicable.
Documentation
Requires documentation update
If yes, link the PR to the docs update or the issue with the details labeled
docs. Remove if not applicable.Requires explanation in Release Notes
If yes, fill in the details here. Remove if not applicable.
Remarks
Optional. Remove if not applicable.