Skip to content
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.

Sign up for GitHub

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

Jump to bottom

core/vm: Move interpreter.ReadOnly check into the opcode implementations #23970

Merged
merged 4 commits into from
Dec 1, 2021
Merged

core/vm: Move interpreter.ReadOnly check into the opcode implementations #23970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
686eac9
core/vm: Move interpreter.ReadOnly check into the opcode implementations
axic Nov 24, 2021
931daa5
core/vm: Remove obsolete operation.writes flag
axic Nov 24, 2021
9109c82
core/vm: Capture fault states in logger
axic Nov 27, 2021
ec03d0b
core/vm: Remove panic added for testing
axic Nov 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions core/vm/instructions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,9 @@ func opSload(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]by
}

func opSstore(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byte, error) {
if interpreter.readOnly {
return nil, ErrWriteProtection
}
loc := scope.Stack.pop()
val := scope.Stack.pop()
interpreter.evm.StateDB.SetState(scope.Contract.Address(),
Expand Down Expand Up @@ -561,6 +564,9 @@ func opGas(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byte
}

func opCreate(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byte, error) {
if interpreter.readOnly {
return nil, ErrWriteProtection
}
var (
value = scope.Stack.pop()
offset, size = scope.Stack.pop(), scope.Stack.pop()
Expand Down Expand Up @@ -604,6 +610,9 @@ func opCreate(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]b
}

func opCreate2(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byte, error) {
if interpreter.readOnly {
return nil, ErrWriteProtection
}
var (
endowment = scope.Stack.pop()
offset, size = scope.Stack.pop(), scope.Stack.pop()
Expand Down Expand Up @@ -653,6 +662,9 @@ func opCall(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byt
// Get the arguments from the memory.
args := scope.Memory.GetPtr(int64(inOffset.Uint64()), int64(inSize.Uint64()))

if interpreter.readOnly && !value.IsZero() {
return nil, ErrWriteProtection
}
var bigVal = big0
//TODO: use uint256.Int instead of converting with toBig()
// By using big0 here, we save an alloc for the most common case (non-ether-transferring contract calls),
Expand Down Expand Up @@ -794,6 +806,9 @@ func opStop(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byt
}

func opSuicide(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byte, error) {
if interpreter.readOnly {
return nil, ErrWriteProtection
}
beneficiary := scope.Stack.pop()
balance := interpreter.evm.StateDB.GetBalance(scope.Contract.Address())
interpreter.evm.StateDB.AddBalance(beneficiary.Bytes20(), balance)
Expand All @@ -810,6 +825,9 @@ func opSuicide(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]
// make log instruction function
func makeLog(size int) executionFunc {
return func(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byte, error) {
if interpreter.readOnly {
return nil, ErrWriteProtection
}
topics := make([]common.Hash, size)
stack := scope.Stack
mStart, mSize := stack.pop(), stack.pop()
Expand Down
11 changes: 0 additions & 11 deletions core/vm/interpreter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,17 +204,6 @@ func (in *EVMInterpreter) Run(contract *Contract, input []byte, readOnly bool) (
} else if sLen > operation.maxStack {
return nil, &ErrStackOverflow{stackLen: sLen, limit: operation.maxStack}
}
// If the operation is valid, enforce write restrictions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is changing the order of operations a bit. Like one concrete change is the tracer's CaptureState will be called even on failure. Also memory will be resized, but I guess that's moot since the error will cause the call frame to fail. Another possibility is EVM returning a different error message if there are multiple failures (i.e. readonly violation AND not enough gas for memory expansion)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, this is one of my concerns, see the discussion on #23968. The order of evaluation for the most part is not covered by state tests, but in the long term I would hope it will be covered.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right missed that convo. Maybe time to revive the "trace after op execution" discussion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of evaluation for the most part is not covered by state tests

It's not so much that it isn't covered -- the fact is that the order is opaque to state tests, in nearly all situations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had ideated to expose error codes from execution and include those in the state tests. That would be a way to ensure some common order.

if in.readOnly && in.evm.chainRules.IsByzantium {
// If the interpreter is operating in readonly mode, make sure no
// state-modifying operation is performed. The 3rd stack item
// for a call operation is the value. Transferring value from one
// account to the others means the state is modified and should also
// return with an error.
if operation.writes || (op == CALL && stack.Back(2).Sign() != 0) {
return nil, ErrWriteProtection
}
}
// Static portion of gas
cost = operation.constantGas // For tracing
if !contract.UseGas(operation.constantGas) {
Expand Down
11 changes: 0 additions & 11 deletions core/vm/jump_table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ type operation struct {

// memorySize returns the memory size required for the operation
memorySize memorySizeFunc

writes bool // determines whether this a state modifying operation
}

var (
Expand Down Expand Up @@ -123,7 +121,6 @@ func newConstantinopleInstructionSet() JumpTable {
minStack: minStack(4, 1),
maxStack: maxStack(4, 1),
memorySize: memoryCreate2,
writes: true,
}
return instructionSet
}
Expand Down Expand Up @@ -511,7 +508,6 @@ func newFrontierInstructionSet() JumpTable {
dynamicGas: gasSStore,
minStack: minStack(2, 0),
maxStack: maxStack(2, 0),
writes: true,
},
JUMP: {
execute: opJump,
Expand Down Expand Up @@ -939,39 +935,34 @@ func newFrontierInstructionSet() JumpTable {
minStack: minStack(2, 0),
maxStack: maxStack(2, 0),
memorySize: memoryLog,
writes: true,
},
LOG1: {
execute: makeLog(1),
dynamicGas: makeGasLog(1),
minStack: minStack(3, 0),
maxStack: maxStack(3, 0),
memorySize: memoryLog,
writes: true,
},
LOG2: {
execute: makeLog(2),
dynamicGas: makeGasLog(2),
minStack: minStack(4, 0),
maxStack: maxStack(4, 0),
memorySize: memoryLog,
writes: true,
},
LOG3: {
execute: makeLog(3),
dynamicGas: makeGasLog(3),
minStack: minStack(5, 0),
maxStack: maxStack(5, 0),
memorySize: memoryLog,
writes: true,
},
LOG4: {
execute: makeLog(4),
dynamicGas: makeGasLog(4),
minStack: minStack(6, 0),
maxStack: maxStack(6, 0),
memorySize: memoryLog,
writes: true,
},
CREATE: {
execute: opCreate,
Expand All @@ -980,7 +971,6 @@ func newFrontierInstructionSet() JumpTable {
minStack: minStack(3, 1),
maxStack: maxStack(3, 1),
memorySize: memoryCreate,
writes: true,
},
CALL: {
execute: opCall,
Expand Down Expand Up @@ -1010,7 +1000,6 @@ func newFrontierInstructionSet() JumpTable {
dynamicGas: gasSelfdestruct,
minStack: minStack(1, 0),
maxStack: maxStack(1, 0),
writes: true,
},
}
}
5 changes: 4 additions & 1 deletion eth/tracers/logger/logger_json.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ func (l *JSONLogger) CaptureStart(env *vm.EVM, from, to common.Address, create b
l.env = env
}

func (l *JSONLogger) CaptureFault(uint64, vm.OpCode, uint64, uint64, *vm.ScopeContext, int, error) {}
func (l *JSONLogger) CaptureFault(pc uint64, op vm.OpCode, gas uint64, cost uint64, scope *vm.ScopeContext, depth int, err error) {
// TODO: Add rData to this interface as well
l.CaptureState(pc, op, gas, cost, scope, nil, depth, err)
}

// CaptureState outputs state information on the logger.
func (l *JSONLogger) CaptureState(pc uint64, op vm.OpCode, gas, cost uint64, scope *vm.ScopeContext, rData []byte, depth int, err error) {
Expand Down