wip: Vm opcodes STATICCALL (and base opcodes PR) #14

whilei · 2018-08-20T17:20:36Z

rename *Byzantium* name, eg. RuleSet.IsByzantium interface method. This might depend on Fork name and/or ECIP number.
- renamed to RuleSet.IsECIP1045

This change is

…code

solution: since STATICCALL functions equivalently to CALL, use congruent behavior to CALL during gas decision log switch statement

…be like DELEGATECALL https://github.com/ethereum/go-ethereum/blob/76069eef38082089d2eaf99661a80e1a953bf271/core/vm/memory_table.go solution: change switch case condition

Includes a fix where signature had accepted 'value' param, and should not have

This should disallow state modificaation on StaticCall method, and if an opcode is called that writes to the state layer, the vm should throw an exception solution: - implement IsReadOnly and SetReadOnly interface methods - add field for opcode instruction regard whether or not it makes (or can make) changes to state, eg LOG*, SSTORE, SUICIDE, and possibly CALL returning a non-zero value.

solution: add placehold fn to satisfy interface

... it will cause confusion solution: rename fork to ECIP1045

solution: this is handled in core/vm/vm.go conditional, see comment there

solution: include congruent DELEGATECALL:STATICCALL tests

…face (missing IsECIP1045 method) solution: implement method as default

solution: implement IsECIP1045, IsReadOnly, and SetReadOnly methods for associated RuleSet and VMEnv structs

…nterface methods solution: implement them

solution: set in fn Since STATICCALL and incoming OPCODES should only be activated after the fork (and tests configured with envs after the fork), this number can be safely set to 0

whilei · 2018-08-21T22:34:33Z

This is a base "topical parent" PR for all incoming opcodes, with topic: STATICCALL implementation. I'm going to open adjacent PRs to this branch to house incoming changes for next opcodes, using this as a base branch for the rest of those changes.

STATICCALL
- RETURNDATACOPY, RETURNDATASIZE
  - REVERT

solution: remove typo

solution: fix stack signature

tzdybal

Reviewed 12 of 14 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @whilei)

cmd/evm/main.go, line 245 at r2 (raw file):

}

func (self *VMEnv) SetReadOnly(isReadOnly bool) { self.readOnly = isReadOnly }

Just a side note on self - https://github.com/golang/go/wiki/CodeReviewComments#receiver-names

core/execution.go, line 150 at r2 (raw file):

}

func execDelegateCall(env vm.Environment, caller vm.ContractRef, originAddr, toAddr, codeAddr *common.Address, codeHash common.Hash, input, code []byte, gas, gasPrice, value *big.Int) (ret []byte, addr common.Address, err error) {

originAddr is not in use - is it OK to remove it from signature?

core/execution.go, line 190 at r2 (raw file):

// the value argument is not included and taken to be zero
// https://github.com/ethereum/EIPs/pull/214/files#diff-29d8cb8d19a769c141160ce5c1e65caeR31
func execStaticCall(env vm.Environment, caller vm.ContractRef, address, codeAddr *common.Address, codeHash common.Hash, input, code []byte, gas, gasPrice *big.Int) (ret []byte, addr common.Address, err error) {

execDelegateCall and execStaticCall are almost the same - it's easy to extract common function to handle both of them. IMHO we should leave those function signatures as is, and add one more function with implementation (value and readOnly as arguments).
(This may be unnecessary complication - see the next comment)

core/execution.go, line 221 at r2 (raw file):

		contract.UseGas(contract.Gas)

		env.RevertToSnapshot(snapshot)

Please clarify - why we need a snapshot and RevertToSnapshot call, if we expect error on (before/instead of) any state change?

core/vm/instructions.go, line 519 at r2 (raw file):

}

func opStaticCall(instr instruction, pc *uint64, env Environment, contract *Contract, memory *Memory, stack *stack) {

This function is the same as opDelegateCall - only formatting and var names differs - space for refactoring.

core/vm/jump_table.go, line 24 at r2 (raw file):

	fn     instrFn
	valid  bool
	writes bool

There are 131 entries in jumpTable, and only 7 writes: true (and it's used only in one place in the code). This may be premature optimization, but we can create func IsStateChanging(op OpCode) bool with list of non-static opcodes.
As a bonus: if statement that use this information would be simplified.

core/vm/vm.go, line 129 at r2 (raw file):

			// account to the others means the state is modified and should also
			// return with an error.
			if opPtr := evm.jumpTable[op]; opPtr.writes || op == CALL && stack.data[stack.len()-2-1].BitLen() > 0 {

There is nothing more than if inside if - it can be merged to single if statement.

tzdybal

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @whilei)

core/vm/jump_table.go, line 269 at r2 (raw file):

		valid: true,
	}
	jumpTable[LOG0] = jumpPtr{

According to https://github.com/ethereum/EIPs/pull/214/files#diff-29d8cb8d19a769c141160ce5c1e65caeR33 LOG0 should also be considered as writes: true OpCode.

solution: refactor field to vm.Opcode#IsStateModifying

whilei

Reviewable status: 13 of 14 files reviewed, 8 unresolved discussions (waiting on @tzdybal and @whilei)

cmd/evm/main.go, line 245 at r2 (raw file):