Skip to content

[WIP] cmd/evm: prestate config and json trace fixes #14900

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

Closed
cdetrio wants to merge 2 commits into from
Closed

[WIP] cmd/evm: prestate config and json trace fixes #14900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
deb74ae
cmd/evm: add flag
holiman Jul 27, 2017
4fcf605
core/vm, cmd/evm: json tracing and prestate config fixes
cdetrio Aug 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/evm/json_logger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func (l *JSONLogger) CaptureState(env *vm.EVM, pc uint64, op vm.OpCode, gas, cos
log := vm.StructLog{
Pc: pc,
Op: op,
Gas: gas + cost,
Gas: gas,
GasCost: cost,
MemorySize: memory.Len(),
Storage: nil,
Expand Down
5 changes: 5 additions & 0 deletions cmd/evm/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ var (
Name: "sender",
Usage: "The transaction origin",
}
ReceiverFlag = cli.StringFlag{
Name: "receiver",
Usage: "The transaction receiver (execution context)",
}
DisableMemoryFlag = cli.BoolFlag{
Name: "nomemory",
Usage: "disable memory output",
Expand Down Expand Up @@ -131,6 +135,7 @@ func init() {
GenesisFlag,
MachineFlag,
SenderFlag,
ReceiverFlag,
DisableMemoryFlag,
DisableStackFlag,
}
Expand Down
100 changes: 70 additions & 30 deletions cmd/evm/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"os"
"runtime/pprof"
"time"
"math/big"

goruntime "runtime"

Expand Down Expand Up @@ -83,7 +84,12 @@ func runCmd(ctx *cli.Context) error {
debugLogger *vm.StructLogger
statedb *state.StateDB
chainConfig *params.ChainConfig
genCoinbase common.Address
genTimestamp uint64
genGasLimit uint64
genDifficulty *big.Int
sender = common.StringToAddress("sender")
receiver = common.StringToAddress("receiver")
)
if ctx.GlobalBool(MachineFlag.Name) {
tracer = NewJSONLogger(logconfig, os.Stdout)
Expand All @@ -97,59 +103,78 @@ func runCmd(ctx *cli.Context) error {
gen := readGenesis(ctx.GlobalString(GenesisFlag.Name))
_, statedb = gen.ToBlock()
chainConfig = gen.Config
genCoinbase = gen.Coinbase
genDifficulty = gen.Difficulty
genGasLimit = gen.GasLimit
genTimestamp = gen.Timestamp
fmt.Println("runner.go gen.Difficulty:", gen.Difficulty)
fmt.Println("runner.go gen.Coinbase:", gen.Coinbase)
fmt.Println("runner.go gen.Timestamp:", gen.Timestamp)
fmt.Println("runner.go chainConfig:", chainConfig)
} else {
db, _ := ethdb.NewMemDatabase()
statedb, _ = state.New(common.Hash{}, state.NewDatabase(db))
}
if ctx.GlobalString(SenderFlag.Name) != "" {
sender = common.HexToAddress(ctx.GlobalString(SenderFlag.Name))
}
// createAccount overwrites the nonce in the prestate. should only be used if no prestate provided
// statedb.CreateAccount(sender)

statedb.CreateAccount(sender)
if ctx.GlobalString(ReceiverFlag.Name) != "" {
receiver = common.HexToAddress(ctx.GlobalString(ReceiverFlag.Name))
}

var (
code []byte
ret []byte
err error
)
if fn := ctx.Args().First(); len(fn) > 0 {
src, err := ioutil.ReadFile(fn)
if err != nil {
return err
}
if statedb.GetCodeSize(receiver) == 0 {
if fn := ctx.Args().First(); len(fn) > 0 {
src, err := ioutil.ReadFile(fn)
if err != nil {
return err
}

bin, err := compiler.Compile(fn, src, false)
if err != nil {
return err
}
code = common.Hex2Bytes(bin)
} else if ctx.GlobalString(CodeFlag.Name) != "" {
code = common.Hex2Bytes(ctx.GlobalString(CodeFlag.Name))
} else {
var hexcode []byte
if ctx.GlobalString(CodeFileFlag.Name) != "" {
var err error
hexcode, err = ioutil.ReadFile(ctx.GlobalString(CodeFileFlag.Name))
bin, err := compiler.Compile(fn, src, false)
if err != nil {
fmt.Printf("Could not load code from file: %v\n", err)
os.Exit(1)
return err
}
code = common.Hex2Bytes(bin)
} else if ctx.GlobalString(CodeFlag.Name) != "" {
code = common.Hex2Bytes(ctx.GlobalString(CodeFlag.Name))
} else {
var err error
hexcode, err = ioutil.ReadAll(os.Stdin)
if err != nil {
fmt.Printf("Could not load code from stdin: %v\n", err)
os.Exit(1)
var hexcode []byte
if ctx.GlobalString(CodeFileFlag.Name) != "" {
var err error
hexcode, err = ioutil.ReadFile(ctx.GlobalString(CodeFileFlag.Name))
if err != nil {
fmt.Printf("Could not load code from file: %v\n", err)
os.Exit(1)
}
} else {
var err error
hexcode, err = ioutil.ReadAll(os.Stdin)
if err != nil {
fmt.Printf("Could not load code from stdin: %v\n", err)
os.Exit(1)
}
}
code = common.Hex2Bytes(string(bytes.TrimRight(hexcode, "\n")))
}
code = common.Hex2Bytes(string(bytes.TrimRight(hexcode, "\n")))
}

initialGas := ctx.GlobalUint64(GasFlag.Name)
runtimeConfig := runtime.Config{
Origin: sender,
Time: new(big.Int).SetUint64(genTimestamp),
Coinbase: genCoinbase,
Difficulty: genDifficulty,
State: statedb,
GasLimit: initialGas,
GasLimit: genGasLimit,
GasPrice: utils.GlobalBig(ctx, PriceFlag.Name),
TxGasLimit: initialGas,
Value: utils.GlobalBig(ctx, ValueFlag.Name),
EVMConfig: vm.Config{
Tracer: tracer,
Expand All @@ -176,15 +201,30 @@ func runCmd(ctx *cli.Context) error {
}
tstart := time.Now()
var leftOverGas uint64

// sender prepays the gas fee
mgval := new(big.Int).Mul(new(big.Int).SetUint64(initialGas), runtimeConfig.GasPrice)
statedb.SubBalance(sender, mgval)

if ctx.GlobalBool(CreateFlag.Name) {
input := append(code, common.Hex2Bytes(ctx.GlobalString(InputFlag.Name))...)
intrinsicGas := core.IntrinsicGas(input, ctx.GlobalBool(CreateFlag.Name), true)
initialGas -= intrinsicGas.Uint64()
runtimeConfig.TxGasLimit = initialGas
ret, _, leftOverGas, err = runtime.Create(input, &runtimeConfig)
} else {
receiver := common.StringToAddress("receiver")
statedb.SetCode(receiver, code)

if statedb.GetCodeSize(receiver) == 0 {
statedb.SetCode(receiver, code)
}
intrinsicGas := core.IntrinsicGas(common.Hex2Bytes(ctx.GlobalString(InputFlag.Name)), false, true)
initialGas -= intrinsicGas.Uint64()
runtimeConfig.TxGasLimit = initialGas
ret, leftOverGas, err = runtime.Call(receiver, common.Hex2Bytes(ctx.GlobalString(InputFlag.Name)), &runtimeConfig)
}

remainingEther := new(big.Int).Mul(new(big.Int).SetUint64(leftOverGas), runtimeConfig.GasPrice)
statedb.AddBalance(sender, remainingEther)

execTime := time.Since(tstart)

if ctx.GlobalBool(DumpFlag.Name) {
Expand Down
22 changes: 19 additions & 3 deletions core/vm/interpreter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,17 +112,21 @@ func (in *Interpreter) Run(snapshot int, contract *Contract, input []byte) (ret
op OpCode // current opcode
mem = NewMemory() // bound memory
stack = newstack() // local stack
stackCopy = newstack()
Copy link
Copy Markdown
Contributor

@holiman holiman Aug 4, 2017

Choose a reason for hiding this comment

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

Does stackCopy really need to exist in this scope? Can't it just be created on the fly when needed?

Copy link
Copy Markdown
Member Author

@cdetrio cdetrio Aug 4, 2017

Choose a reason for hiding this comment

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

I tried removing this line and changing L151 to stackCopy := newstack(), but I get:

core/vm/interpreter.go:129: undefined: stackCopy
core/vm/interpreter.go:197: undefined: stackCopy

logged = bool(false)
// For optimisation reason we're using uint64 as the program counter.
// It's theoretically possible to go above 2^64. The YP defines the PC
// to be uint256. Practically much less so feasible.
pc = uint64(0) // program counter
pcCopy = uint64(0)
gasCopy = uint64(0)
cost uint64
)
contract.Input = input

defer func() {
if err != nil && in.cfg.Debug {
in.cfg.Tracer.CaptureState(in.evm, pc, op, contract.Gas, cost, mem, stack, contract, in.evm.depth, err)
if err != nil && !logged && in.cfg.Debug {
in.cfg.Tracer.CaptureState(in.evm, pcCopy, op, gasCopy, cost, mem, stackCopy, contract, in.evm.depth, err)
}
}()

Expand All @@ -133,13 +137,23 @@ func (in *Interpreter) Run(snapshot int, contract *Contract, input []byte) (ret
for atomic.LoadInt32(&in.evm.abort) == 0 {
// Get the memory location of pc
op = contract.GetOp(pc)
logged = false

// get the operation from the jump table matching the opcode
operation := in.cfg.JumpTable[op]
if err := in.enforceRestrictions(op, operation, stack); err != nil {
return nil, err
}

if in.cfg.Debug {
pcCopy = uint64(pc)
gasCopy = uint64(contract.Gas)
stackCopy = newstack()
for _, val := range stack.data {
stackCopy.push(val)
Copy link
Copy Markdown
Contributor

@holiman holiman Aug 4, 2017

Choose a reason for hiding this comment

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

What's the intent here? The stackCopy will contain the same bigint objects as the original stack, so if any of those are modified (e.g. by being popped, added to intpool, and then reused), they will change also in the stackCopy.

Copy link
Copy Markdown
Member Author

@cdetrio cdetrio Aug 4, 2017

Choose a reason for hiding this comment

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

I don't know how the memory management works, but I tested it again to check. Changing it to:

if in.cfg.Debug {
  pcCopy = uint64(pc)
  gasCopy = uint64(contract.Gas)
  stackCopy = stack
}

reproduces the bug where the mutated stack is logged (from stBoundsTest/CALL_BoundsOOG.json):

{"pc":0,"op":96,"gas":"0x1f7e8","gasCost":"0x3","memory":"0x","memSize":0,"stack":[],"depth":1,"error":null,"opName":"PUSH1"}
{"pc":2,"op":96,"gas":"0x1f7e5","gasCost":"0x3","memory":"0x","memSize":0,"stack":["0x0"],"depth":1,"error":null,"opName":"PUSH1"}
{"pc":4,"op":96,"gas":"0x1f7e2","gasCost":"0x3","memory":"0x","memSize":0,"stack":["0x0","0x0"],"depth":1,"error":null,"opName":"PUSH1"}
{"pc":6,"op":96,"gas":"0x1f7df","gasCost":"0x3","memory":"0x","memSize":0,"stack":["0x0","0x0","0x0"],"depth":1,"error":null,"opName":"PUSH1"}
{"pc":8,"op":96,"gas":"0x1f7dc","gasCost":"0x3","memory":"0x","memSize":0,"stack":["0x0","0x0","0x0","0x0"],"depth":1,"error":null,"opName":"PUSH1"}
{"pc":10,"op":115,"gas":"0x1f7d9","gasCost":"0x3","memory":"0x","memSize":0,"stack":["0x0","0x0","0x0","0x0","0x0"],"depth":1,"error":null,"opName":"PUSH20"}
{"pc":31,"op":103,"gas":"0x1f7d6","gasCost":"0x3","memory":"0x","memSize":0,"stack":["0x0","0x0","0x0","0x0","0x0","0x1000000000000000000000000000000000000001"],"depth":1,"error":null,"opName":"PUSH8"}
{"pc":40,"op":241,"gas":"0x1f7d3","gasCost":"0x1efff","memory":"0x","memSize":0,"stack":["0x0","0x0","0x0","0x0","0x0","0x1000000000000000000000000000000000000001","0x1ed43"],"depth":1,"error":null,"opName":"CALL"}
{"pc":0,"op":96,"gas":"0x1ed43","gasCost":"0x3","memory":"0x","memSize":0,"stack":[],"depth":2,"error":null,"opName":"PUSH1"}

The 0x1ed43 gas argument to CALL is a mutated value (from the 63/64 rule).

Using stackCopy = newstack() and the for loop gives the correct (unmutated) value 0x7ffffffffffffff:

{"pc":31,"op":103,"gas":"0x1f7d6","gasCost":"0x3","memory":"0x","memSize":0,"stack":["0x0","0x0","0x0","0x0","0x0","0x1000000000000000000000000000000000000001"],"depth":1,"error":null,"opName":"PUSH8"}
{"pc":40,"op":241,"gas":"0x1f7d3","gasCost":"0x1efff","memory":"0x","memSize":0,"stack":["0x0","0x0","0x0","0x0","0x0","0x1000000000000000000000000000000000000001","0x7ffffffffffffff"],"depth":1,"error":null,"opName":"CALL"}
{"pc":0,"op":96,"gas":"0x1ed43","gasCost":"0x3","memory":"0x","memSize":0,"stack":[],"depth":2,"error":null,"opName":"PUSH1"}

Copy link
Copy Markdown
Contributor

@holiman holiman Aug 14, 2017

Choose a reason for hiding this comment

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

Ah, so apparently the 63/64-th handling does not operate on the actual bigint already on the stack, but places a new one there, so your fix works (even though it's a bit brittle).

Copy link
Copy Markdown
Contributor

@holiman holiman Aug 14, 2017

Choose a reason for hiding this comment

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

Happens here:

go-ethereum/core/vm/gas_table.go

Line 329 in dfd0762

stack.data[stack.len()-1] = new(big.Int).SetUint64(cg)

}
}

// if the op is invalid abort the process and return an error
if !operation.valid {
return nil, fmt.Errorf("invalid opcode 0x%x", int(op))
Expand Down Expand Up @@ -179,7 +193,9 @@ func (in *Interpreter) Run(snapshot int, contract *Contract, input []byte) (ret
}

if in.cfg.Debug {
in.cfg.Tracer.CaptureState(in.evm, pc, op, contract.Gas, cost, mem, stack, contract, in.evm.depth, err)
// trace needs to be called before operation.execute for CALLs etc
in.cfg.Tracer.CaptureState(in.evm, pcCopy, op, gasCopy, cost, mem, stackCopy, contract, in.evm.depth, err)
logged = true
}

// execute the operation
Expand Down
4 changes: 1 addition & 3 deletions core/vm/runtime/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package runtime
import (
"math/big"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core"
"github.com/ethereum/go-ethereum/core/state"
"github.com/ethereum/go-ethereum/core/vm"
Expand All @@ -29,8 +28,7 @@ func NewEnv(cfg *Config, state *state.StateDB) *vm.EVM {
context := vm.Context{
CanTransfer: core.CanTransfer,
Transfer: core.Transfer,
GetHash: func(uint64) common.Hash { return common.Hash{} },

GetHash: cfg.GetHashFn,
Origin: cfg.Origin,
Coinbase: cfg.Coinbase,
BlockNumber: cfg.BlockNumber,
Expand Down
7 changes: 4 additions & 3 deletions core/vm/runtime/runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ type Config struct {
Time *big.Int
GasLimit uint64
GasPrice *big.Int
TxGasLimit uint64
Value *big.Int
DisableJit bool // "disable" so it's enabled by default
Debug bool
Expand Down Expand Up @@ -79,7 +80,7 @@ func setDefaults(cfg *Config) {
cfg.Value = new(big.Int)
}
if cfg.BlockNumber == nil {
cfg.BlockNumber = new(big.Int)
cfg.BlockNumber = big.NewInt(1)
}
if cfg.GetHashFn == nil {
cfg.GetHashFn = func(n uint64) common.Hash {
Expand Down Expand Up @@ -144,7 +145,7 @@ func Create(input []byte, cfg *Config) ([]byte, common.Address, uint64, error) {
code, address, leftOverGas, err := vmenv.Create(
sender,
input,
cfg.GasLimit,
cfg.TxGasLimit,
cfg.Value,
)
return code, address, leftOverGas, err
Expand All @@ -166,7 +167,7 @@ func Call(address common.Address, input []byte, cfg *Config) ([]byte, uint64, er
sender,
address,
input,
cfg.GasLimit,
cfg.TxGasLimit,
cfg.Value,
)

Expand Down