- 
                Notifications
    
You must be signed in to change notification settings  - Fork 21.5k
 
core/vm: implement EIP-3540: EVM Object Format (EOF) v1 #22958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
036cbe7    to
    0695098      
    Compare
  
    b7a2fad    to
    679ba38      
    Compare
  
    69b1645    to
    5e765d7      
    Compare
  
    eff336d    to
    10d7532      
    Compare
  
    77fa3e7    to
    35e7209      
    Compare
  
            
          
                core/vm/contract.go
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice trick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However isn't there an enum for opcodes? If so, it would make sense using it for both 0 and 0xfe so it is more apparent to the reader.
Hm, the current 0 returned is more in line with the concept of over-reading just returns zeroes (in case of codecopy/calldatacopy), and on that vein 0xfe is indeed a "hack".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However isn't there an enum for opcodes? If so, it would make sense using it for both 0 and 0xfe so it is more apparent to the reader.
No enum for 0xfe yet 😃 Also this function returns byte that's why it used 0 I think.
However it doesn't make sense to return byte because it's always casted to OpCode in GetOp above.
I guess I'll submit a separate small cleanup PR for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
        
          
                core/vm/contract.go
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say a helper for this would be nice, otherwise just leave a comment stating that based EIP-3540 codeSize == 0 is invalid, i.e. it must be legacy.
Update: a helper would definitely be nice since this check is used in several places.
b5c39ef    to
    2b311d7      
    Compare
  
            
          
                core/vm/analysis.go
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We def want to be sure that c.CodeBeginOffset() is not actually called every iteration, but only once. Perhaps better to move that out of the loop, to be sure in the future too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
71fb6e7    to
    b5013e1      
    Compare
  
            
          
                core/vm/contract.go
              
                Outdated
          
        
      | return c.header.CodeEndOffset() | ||
| } | ||
| 
               | 
          ||
| func (c *Contract) CodeSize() uint64 { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring. This is actually not just a nit-pick, one pretty important thing to mention here is whether CodeSize returns the size of header+code, the entire container, or something else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
        
          
                core/vm/contract.go
              
                Outdated
          
        
      | } | ||
| // Only JUMPDESTs allowed for destinations | ||
| if OpCode(c.Code[udest]) != JUMPDEST { | ||
| if OpCode(c.Code[c.CodeBeginOffset()+udest]) != JUMPDEST { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like this, and internalize the call to CodeBeginOffset?
| if OpCode(c.Code[c.CodeBeginOffset()+udest]) != JUMPDEST { | |
| if c.OpCodeAt(udest) != JUMPDEST { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with @axic's suggestion below and used existing GetOp for this.
| code := evm.StateDB.GetCode(addrCopy) | ||
| 
               | 
          ||
| var header EOF1Header | ||
| if hasEIP3540(&evm.Config) && hasEOFMagic(code) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit odd to me. Shouldn't you instead do something like
if evm.chainRules.IsEip3540 && hasEOFMagic(code) { 
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was implemented to make EIP-3540 (and EIP-3670) only activatable as an "extra" EIP (list of these is in evm.Config.ExtraEips)
If you think it's okay now to make it part of Shanghai implementation, I'll be happy to change it to evm.chainRules.IsShanghai, this would actually simplify some things I believe.
        
          
                core/vm/interpreter.go
              
                Outdated
          
        
      | // Get the operation from the jump table and validate the stack to ensure there are | ||
| // enough stack items available to perform the operation. | ||
| op = contract.GetOp(pc) | ||
| op = contract.GetOp(contract.CodeBeginOffset() + pc) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also use that contract.OpCodeAt(udest) I suggested above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also just change GetOp to the new semantics, couldn't it? It seems to be the only place it is being used at (and has internal length checks).
| 
           d5cf7a2 is a refactoring to save code section bounds instead of   | 
    
| 
           Renamed   | 
    
        
          
                core/vm/contract.go
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CodeAddr was not used.
1770a8e    to
    55cbab6      
    Compare
  
    | 
           Outdated and superseded by #26133  | 
    
This is PoC for EIP-3540 implementation.
Some consensus tests are at ethereum/tests#847