From 662dab7bf530a21bcd364442bf46e326cf4e4d12 Mon Sep 17 00:00:00 2001 From: rjl493456442 Date: Mon, 30 Mar 2020 11:08:52 +0800 Subject: [PATCH] all: address comments --- accounts/abi/bind/backends/simulated.go | 14 +++--- core/error.go | 31 +++++++------ core/state_transition.go | 58 ++++++++++++++----------- core/tx_pool.go | 8 ---- core/vm/errors.go | 10 ++--- core/vm/evm.go | 4 +- core/vm/interpreter.go | 4 +- eth/api_tracer.go | 2 +- graphql/graphql.go | 4 +- internal/ethapi/api.go | 12 ++--- les/odr_test.go | 4 +- light/odr_test.go | 2 +- 12 files changed, 79 insertions(+), 74 deletions(-) diff --git a/accounts/abi/bind/backends/simulated.go b/accounts/abi/bind/backends/simulated.go index a49dc59691..2f5e0c1654 100644 --- a/accounts/abi/bind/backends/simulated.go +++ b/accounts/abi/bind/backends/simulated.go @@ -352,7 +352,7 @@ func (b *SimulatedBackend) CallContract(ctx context.Context, call ethereum.CallM if err != nil { return nil, err } - return res.Result, nil + return res.Return(), nil } // PendingCallContract executes a contract call on the pending state. @@ -365,7 +365,7 @@ func (b *SimulatedBackend) PendingCallContract(ctx context.Context, call ethereu if err != nil { return nil, err } - return res.Result, nil + return res.Return(), nil } // PendingNonceAt implements PendingStateReader.PendingNonceAt, retrieving @@ -411,7 +411,7 @@ func (b *SimulatedBackend) EstimateGas(ctx context.Context, call ethereum.CallMs b.pendingState.RevertToSnapshot(snapshot) if err != nil { - if err == core.ErrInsufficientIntrinsicGas { + if err == core.ErrIntrinsicGas { return true, nil, nil // Special case, raise gas limit } return true, nil, err // Bail out @@ -424,8 +424,8 @@ func (b *SimulatedBackend) EstimateGas(ctx context.Context, call ethereum.CallMs failed, _, err := executable(mid) // If the error is not nil(consensus error), it means the provided message - // call or transaction will never be accpeted no matter how many gas assigened. - // Return the error directly, don't struggle any more + // call or transaction will never be accepted no matter how much gas it is + // assigned. Return the error directly, don't struggle any more if err != nil { return 0, err } @@ -445,8 +445,8 @@ func (b *SimulatedBackend) EstimateGas(ctx context.Context, call ethereum.CallMs if result != nil { if result.Err != vm.ErrOutOfGas { errMsg := fmt.Sprintf("always failing transaction (%v)", result.Err) - if len(result.RevertReason) > 0 { - errMsg += fmt.Sprintf(" (0x%x)", result.RevertReason) + if len(result.Revert()) > 0 { + errMsg += fmt.Sprintf(" (0x%x)", result.Revert()) } return 0, errors.New(errMsg) } diff --git a/core/error.go b/core/error.go index 8e0716e6c1..9e196d208c 100644 --- a/core/error.go +++ b/core/error.go @@ -29,8 +29,13 @@ var ( ErrNoGenesis = errors.New("genesis not found in chain") ) -// State transition consensus errors, any of them encountered during -// the block processing can lead to consensus issue. +// List of evm-call-message pre-checking errors. All state transtion messages will +// be pre-checked before execution. If any invalidation detected, the corresponding +// error should be returned which is defined here. +// +// - If the pre-checking happens in the miner, then the transaction won't be packed. +// - If the pre-checking happens in the block processing procedure, then a "BAD BLOCk" +// error should be emitted. var ( // ErrNonceTooLow is returned if the nonce of a transaction is lower than the // one present in the local chain. @@ -44,18 +49,18 @@ var ( // by a transaction is higher than what's left in the block. ErrGasLimitReached = errors.New("gas limit reached") - // ErrInsufficientBalanceForTransfer is returned if the transaction sender doesn't - // have enough balance for transfer(topmost call only). - ErrInsufficientBalanceForTransfer = errors.New("insufficient balance for transfer") + // ErrInsufficientFundsForTransfer is returned if the transaction sender doesn't + // have enough funds for transfer(topmost call only). + ErrInsufficientFundsForTransfer = errors.New("insufficient funds for transfer") - // ErrInsufficientBalanceForFee is returned if transaction sender doesn't have - // enough balance to cover transaction fee. - ErrInsufficientBalanceForFee = errors.New("insufficient balance to pay fee") + // ErrInsufficientFunds is returned if the total cost of executing a transaction + // is higher than the balance of the user's account. + ErrInsufficientFunds = errors.New("insufficient funds for gas * price + value") - // ErrGasOverflow is returned when calculating gas usage. - ErrGasOverflow = errors.New("gas overflow") + // ErrGasUintOverflow is returned when calculating gas usage. + ErrGasUintOverflow = errors.New("gas uint64 overflow") - // ErrInsufficientIntrinsicGas is returned when the gas limit speicified in transaction - // is not enought to cover intrinsic gas usage. - ErrInsufficientIntrinsicGas = errors.New("insufficient intrinsic gas") + // ErrIntrinsicGas is returned if the transaction is specified to use less gas + // than required to start the invocation. + ErrIntrinsicGas = errors.New("intrinsic gas too low") ) diff --git a/core/state_transition.go b/core/state_transition.go index 5afbfc4c1e..5a3c844b0c 100644 --- a/core/state_transition.go +++ b/core/state_transition.go @@ -68,13 +68,12 @@ type Message interface { Data() []byte } -// ExecutionResult includes all output after executing given evm message -// no matter the execution itself is successful or not. +// ExecutionResult includes all output after executing given evm +// message no matter the execution itself is successful or not. type ExecutionResult struct { - UsedGas uint64 // Total used gas but include the refunded gas - Err error // Any error encountered during the exection(listed in core/vm/errors.go) - Result []byte // Returned value of the calling function - RevertReason []byte // Reason to perform revert thrown by solidity code + UsedGas uint64 // Total used gas but include the refunded gas + Err error // Any error encountered during the exection(listed in core/vm/errors.go) + ReturnData []byte // Returned data from evm(function result or data supplied with revert opcode) } // Unwrap returns the internal evm error which allows us for further @@ -86,6 +85,24 @@ func (result *ExecutionResult) Unwrap() error { // Failed returns the indicator whether the execution is successful or not func (result *ExecutionResult) Failed() bool { return result.Err != nil } +// Return is a helper function to help caller distinguish between revert reason +// and function return. Return returns the data after execution if no error occurs. +func (result *ExecutionResult) Return() []byte { + if result.Err != nil { + return nil + } + return common.CopyBytes(result.ReturnData) +} + +// Revert returns the concrete revert reason if the execution is aborted by `REVERT` +// opcode. Note the reason can be nil if no data supplied with revert opcode. +func (result *ExecutionResult) Revert() []byte { + if result.Err != vm.ErrExecutionReverted { + return nil + } + return common.CopyBytes(result.ReturnData) +} + // IntrinsicGas computes the 'intrinsic gas' for a message with the given data. func IntrinsicGas(data []byte, contractCreation, isHomestead bool, isEIP2028 bool) (uint64, error) { // Set the starting gas for the raw transaction @@ -110,13 +127,13 @@ func IntrinsicGas(data []byte, contractCreation, isHomestead bool, isEIP2028 boo nonZeroGas = params.TxDataNonZeroGasEIP2028 } if (math.MaxUint64-gas)/nonZeroGas < nz { - return 0, ErrGasOverflow + return 0, ErrGasUintOverflow } gas += nz * nonZeroGas z := uint64(len(data)) - nz if (math.MaxUint64-gas)/params.TxDataZeroGas < z { - return 0, ErrGasOverflow + return 0, ErrGasUintOverflow } gas += z * params.TxDataZeroGas } @@ -158,7 +175,7 @@ func (st *StateTransition) to() common.Address { func (st *StateTransition) buyGas() error { mgval := new(big.Int).Mul(new(big.Int).SetUint64(st.msg.Gas()), st.gasPrice) if st.state.GetBalance(st.msg.From()).Cmp(mgval) < 0 { - return ErrInsufficientBalanceForFee + return ErrInsufficientFunds } if err := st.gp.SubGas(st.msg.Gas()); err != nil { return err @@ -188,10 +205,8 @@ func (st *StateTransition) preCheck() error { // // - used gas: // total gas used (including gas being refunded) -// - execution result: -// the return value of the calling function -// - revert reason: -// reason to perform revert thrown by solidity code +// - returndata: +// the returned data from evm // - concrete execution error: // various **EVM** error which aborts the execution, // e.g. ErrOutOfGas, ErrExecutionReverted @@ -225,13 +240,13 @@ func (st *StateTransition) TransitionDb() (*ExecutionResult, error) { return nil, err } if st.gas < gas { - return nil, ErrInsufficientIntrinsicGas + return nil, ErrIntrinsicGas } st.gas -= gas // Check clause 6 if msg.Value().Sign() > 0 && !st.evm.CanTransfer(st.state, msg.From(), msg.Value()) { - return nil, ErrInsufficientBalanceForTransfer + return nil, ErrInsufficientFundsForTransfer } var ( ret []byte @@ -247,17 +262,10 @@ func (st *StateTransition) TransitionDb() (*ExecutionResult, error) { st.refundGas() st.state.AddBalance(st.evm.Coinbase, new(big.Int).Mul(new(big.Int).SetUint64(st.gasUsed()), st.gasPrice)) - var revert, result []byte - if vmerr == vm.ErrExecutionReverted { - revert = ret // Revert reason will be returned in ret iif the vmerr is ErrExecutionReverted - } else { - result = ret // Otherwise the ret represents the execution result, may nil - } return &ExecutionResult{ - UsedGas: st.gasUsed(), - Err: vmerr, - Result: result, - RevertReason: revert, + UsedGas: st.gasUsed(), + Err: vmerr, + ReturnData: ret, }, nil } diff --git a/core/tx_pool.go b/core/tx_pool.go index a752bb0d9f..d4a6100c55 100644 --- a/core/tx_pool.go +++ b/core/tx_pool.go @@ -67,14 +67,6 @@ var ( // with a different one without the required price bump. ErrReplaceUnderpriced = errors.New("replacement transaction underpriced") - // ErrInsufficientFunds is returned if the total cost of executing a transaction - // is higher than the balance of the user's account. - ErrInsufficientFunds = errors.New("insufficient funds for gas * price + value") - - // ErrIntrinsicGas is returned if the transaction is specified to use less gas - // than required to start the invocation. - ErrIntrinsicGas = errors.New("intrinsic gas too low") - // ErrGasLimit is returned if a transaction's requested gas limit exceeds the // maximum allowance of the current block. ErrGasLimit = errors.New("exceeds block gas limit") diff --git a/core/vm/errors.go b/core/vm/errors.go index d619a7d344..26ecf51d64 100644 --- a/core/vm/errors.go +++ b/core/vm/errors.go @@ -50,17 +50,17 @@ func (e *ErrStackUnderflow) Error() string { // ErrStackOverflow wraps an evm error when the items on the stack exceeds // the maximum allowance. type ErrStackOverflow struct { - stackLen int - allowance int + stackLen int + limit int } func (e *ErrStackOverflow) Error() string { - return fmt.Sprintf("stack limit reached %d (%d)", e.stackLen, e.allowance) + return fmt.Sprintf("stack limit reached %d (%d)", e.stackLen, e.limit) } // ErrInvalidOpCode wraps an evm error when an invalid opcode is encountered. type ErrInvalidOpCode struct { - opcode int + opcode OpCode } -func (e *ErrInvalidOpCode) Error() string { return fmt.Sprintf("invalid opcode 0x%x", e.opcode) } +func (e *ErrInvalidOpCode) Error() string { return fmt.Sprintf("invalid opcode 0x%x", byte(e.opcode)) } diff --git a/core/vm/evm.go b/core/vm/evm.go index 665389a49b..c86da054db 100644 --- a/core/vm/evm.go +++ b/core/vm/evm.go @@ -269,8 +269,8 @@ func (evm *EVM) CallCode(caller ContractRef, addr common.Address, input []byte, } // Fail if we're trying to transfer more than the available balance // Note although it's noop to transfer X ether to caller itself. But - // if caller doesn't have enough balance, it can lead to an evm error. - // So the check here is necessary. + // if caller doesn't have enough balance, it would be an error to allow + // over-charging itself. So the check here is necessary. if !evm.Context.CanTransfer(evm.StateDB, caller.Address(), value) { return nil, gas, ErrInsufficientBalance } diff --git a/core/vm/interpreter.go b/core/vm/interpreter.go index dc5568c170..e0b7e09ce3 100644 --- a/core/vm/interpreter.go +++ b/core/vm/interpreter.go @@ -222,13 +222,13 @@ func (in *EVMInterpreter) Run(contract *Contract, input []byte, readOnly bool) ( op = contract.GetOp(pc) operation := in.cfg.JumpTable[op] if !operation.valid { - return nil, &ErrInvalidOpCode{opcode: int(op)} + return nil, &ErrInvalidOpCode{opcode: op} } // Validate stack if sLen := stack.len(); sLen < operation.minStack { return nil, &ErrStackUnderflow{stackLen: sLen, required: operation.minStack} } else if sLen > operation.maxStack { - return nil, &ErrStackOverflow{stackLen: sLen, allowance: operation.maxStack} + return nil, &ErrStackOverflow{stackLen: sLen, limit: operation.maxStack} } // If the operation is valid, enforce and write restrictions if in.readOnly && in.evm.chainRules.IsByzantium { diff --git a/eth/api_tracer.go b/eth/api_tracer.go index 6eb7d74a2d..68a61bea00 100644 --- a/eth/api_tracer.go +++ b/eth/api_tracer.go @@ -768,7 +768,7 @@ func (api *PrivateDebugAPI) traceTx(ctx context.Context, message core.Message, v return ðapi.ExecutionResult{ Gas: result.UsedGas, Failed: result.Failed(), - ReturnValue: fmt.Sprintf("%x", result.Result), + ReturnValue: fmt.Sprintf("%x", result.Return()), StructLogs: ethapi.FormatLogs(tracer.StructLogs()), }, nil diff --git a/graphql/graphql.go b/graphql/graphql.go index 159c6845f3..9555d8cce2 100644 --- a/graphql/graphql.go +++ b/graphql/graphql.go @@ -812,7 +812,7 @@ func (b *Block) Call(ctx context.Context, args struct { status = 0 } return &CallResult{ - data: result.Result, + data: result.Return(), gasUsed: hexutil.Uint64(result.UsedGas), status: status, }, nil @@ -881,7 +881,7 @@ func (p *Pending) Call(ctx context.Context, args struct { status = 0 } return &CallResult{ - data: result.Result, + data: result.Return(), gasUsed: hexutil.Uint64(result.UsedGas), status: status, }, nil diff --git a/internal/ethapi/api.go b/internal/ethapi/api.go index 7de0a8e1bc..0e53323936 100644 --- a/internal/ethapi/api.go +++ b/internal/ethapi/api.go @@ -879,7 +879,7 @@ func (s *PublicBlockChainAPI) Call(ctx context.Context, args CallArgs, blockNrOr if err != nil { return nil, err } - return result.Result, nil + return result.Return(), nil } func DoEstimateGas(ctx context.Context, b Backend, args CallArgs, blockNrOrHash rpc.BlockNumberOrHash, gasCap *big.Int) (hexutil.Uint64, error) { @@ -915,7 +915,7 @@ func DoEstimateGas(ctx context.Context, b Backend, args CallArgs, blockNrOrHash result, err := DoCall(ctx, b, args, blockNrOrHash, nil, vm.Config{}, 0, gasCap) if err != nil { - if err == core.ErrInsufficientIntrinsicGas { + if err == core.ErrIntrinsicGas { return true, nil, nil // Special case, raise gas limit } return true, nil, err // Bail out @@ -928,8 +928,8 @@ func DoEstimateGas(ctx context.Context, b Backend, args CallArgs, blockNrOrHash failed, _, err := executable(mid) // If the error is not nil(consensus error), it means the provided message - // call or transaction will never be accpeted no matter how many gas assigened. - // Return the error directly, don't struggle any more + // call or transaction will never be accepted no matter how much gas it is + // assigened. Return the error directly, don't struggle any more. if err != nil { return 0, err } @@ -949,8 +949,8 @@ func DoEstimateGas(ctx context.Context, b Backend, args CallArgs, blockNrOrHash if result != nil { if result.Err != vm.ErrOutOfGas { errMsg := fmt.Sprintf("always failing transaction (%v)", result.Err) - if len(result.RevertReason) > 0 { - errMsg += fmt.Sprintf(" (0x%x)", result.RevertReason) + if len(result.Revert()) > 0 { + errMsg += fmt.Sprintf(" (0x%x)", result.Revert()) } return 0, errors.New(errMsg) } diff --git a/les/odr_test.go b/les/odr_test.go index f36f700425..01cc956953 100644 --- a/les/odr_test.go +++ b/les/odr_test.go @@ -136,7 +136,7 @@ func odrContractCall(ctx context.Context, db ethdb.Database, config *params.Chai //vmenv := core.NewEnv(statedb, config, bc, msg, header, vm.Config{}) gp := new(core.GasPool).AddGas(math.MaxUint64) result, _ := core.ApplyMessage(vmenv, msg, gp) - res = append(res, result.Result...) + res = append(res, result.Return()...) } } else { header := lc.GetHeaderByHash(bhash) @@ -148,7 +148,7 @@ func odrContractCall(ctx context.Context, db ethdb.Database, config *params.Chai gp := new(core.GasPool).AddGas(math.MaxUint64) result, _ := core.ApplyMessage(vmenv, msg, gp) if state.Error() == nil { - res = append(res, result.Result...) + res = append(res, result.Return()...) } } } diff --git a/light/odr_test.go b/light/odr_test.go index d32e2ba55c..ac702b6bbe 100644 --- a/light/odr_test.go +++ b/light/odr_test.go @@ -199,7 +199,7 @@ func odrContractCall(ctx context.Context, db ethdb.Database, bc *core.BlockChain vmenv := vm.NewEVM(context, st, config, vm.Config{}) gp := new(core.GasPool).AddGas(math.MaxUint64) result, _ := core.ApplyMessage(vmenv, msg, gp) - res = append(res, result.Result...) + res = append(res, result.Return()...) if st.Error() != nil { return res, st.Error() }