[core] Return EVM errors from ApplyMessage for external use (#3375)

* [rpc] Return EVM errors as RPC errors for smart contract Calls

	* [core] Return ExecutionResult from TransitionDb & ApplyMessage
	* [core] Update ApplyTransation to use ExecutionResult
	* [rpc] Update doEstimateGas to use ExecutionResult

* [rpc] Remove redundant VMError error message wrap

* [rpc] Remove debug prints

* [rpc] Revert hmy_call RPC behavior to keep backwards compatibility

* [core] Return ExecutionResult struct instead of pointer to struct

core/state_processor.go

@@ -195,7 +195,7 @@ func ApplyTransaction(config *params.ChainConfig, bc ChainContext, author *commo
 	// about the transaction and calling mechanisms.
 	vmenv := vm.NewEVM(context, statedb, config, cfg)
 	// Apply the transaction to the current state (included in the env)
-	_, gas, failed, err := ApplyMessage(vmenv, msg, gp)
+	result, err := ApplyMessage(vmenv, msg, gp)
 	if err != nil {
 		return nil, nil, 0, err
 	}
@@ -206,13 +206,14 @@ func ApplyTransaction(config *params.ChainConfig, bc ChainContext, author *commo
 	} else {
 		root = statedb.IntermediateRoot(config.IsS3(header.Epoch())).Bytes()
 	}
-	*usedGas += gas
+	*usedGas += result.UsedGas
 
+	failedExe := result.VMErr != nil
 	// Create a new receipt for the transaction, storing the intermediate root and gas used by the tx
 	// based on the eip phase, we're passing whether the root touch-delete accounts.
-	receipt := types.NewReceipt(root, failed, *usedGas)
+	receipt := types.NewReceipt(root, failedExe, *usedGas)
 	receipt.TxHash = tx.Hash()
-	receipt.GasUsed = gas
+	receipt.GasUsed = result.UsedGas
 	// if the transaction created a contract, store the creation address in the receipt.
 	if msg.To() == nil {
 		receipt.ContractAddress = crypto.CreateAddress(vmenv.Context.Origin, tx.Nonce())
@@ -226,13 +227,13 @@ func ApplyTransaction(config *params.ChainConfig, bc ChainContext, author *commo
 
 	var cxReceipt *types.CXReceipt
 	// Do not create cxReceipt if EVM call failed
-	if txType == types.SubtractionOnly && !failed {
+	if txType == types.SubtractionOnly && !failedExe {
 		cxReceipt = &types.CXReceipt{tx.Hash(), msg.From(), msg.To(), tx.ShardID(), tx.ToShardID(), msg.Value()}
 	} else {
 		cxReceipt = nil
 	}
 
-	return receipt, cxReceipt, gas, err
+	return receipt, cxReceipt, result.UsedGas, err
 }
 
 // ApplyStakingTransaction attempts to apply a staking transaction to the given state database

core/state_transition.go

@@ -22,15 +22,14 @@ import (
 	"math/big"
 	"sort"
 
-	staking2 "github.com/harmony-one/harmony/staking"
-	"github.com/harmony-one/harmony/staking/network"
-
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/rlp"
 	"github.com/harmony-one/harmony/core/types"
 	"github.com/harmony-one/harmony/core/vm"
 	"github.com/harmony-one/harmony/internal/params"
 	"github.com/harmony-one/harmony/internal/utils"
+	staking2 "github.com/harmony-one/harmony/staking"
+	"github.com/harmony-one/harmony/staking/network"
 	staking "github.com/harmony-one/harmony/staking/types"
 	"github.com/pkg/errors"
 )
@@ -97,6 +96,13 @@ type Message interface {
 	BlockNum() *big.Int
 }
 
+// ExecutionResult is the return value from a transaction committed to the DB
+type ExecutionResult struct {
+	ReturnData []byte
+	UsedGas    uint64
+	VMErr      error
+}
+
 // IntrinsicGas computes the 'intrinsic gas' for a message with the given data.
 func IntrinsicGas(data []byte, contractCreation, homestead, istanbul, isValidatorCreation bool) (uint64, error) {
 	// Set the starting gas for the raw transaction
@@ -157,7 +163,7 @@ func NewStateTransition(evm *vm.EVM, msg Message, gp *GasPool, bc ChainContext)
 // the gas used (which includes gas refunds) and an error if it failed. An error always
 // indicates a core error meaning that the message would always fail for that particular
 // state and would never be accepted within a block.
-func ApplyMessage(evm *vm.EVM, msg Message, gp *GasPool) ([]byte, uint64, bool, error) {
+func ApplyMessage(evm *vm.EVM, msg Message, gp *GasPool) (ExecutionResult, error) {
 	return NewStateTransition(evm, msg, gp, nil).TransitionDb()
 }
 
@@ -218,9 +224,9 @@ func (st *StateTransition) preCheck() error {
 // TransitionDb will transition the state by applying the current message and
 // returning the result including the used gas. It returns an error if failed.
 // An error indicates a consensus issue.
-func (st *StateTransition) TransitionDb() (ret []byte, usedGas uint64, failed bool, err error) {
-	if err = st.preCheck(); err != nil {
-		return
+func (st *StateTransition) TransitionDb() (ExecutionResult, error) {
+	if err := st.preCheck(); err != nil {
+		return ExecutionResult{}, err
 	}
 	msg := st.msg
 	sender := vm.AccountRef(msg.From())
@@ -231,34 +237,33 @@ func (st *StateTransition) TransitionDb() (ret []byte, usedGas uint64, failed bo
 	// Pay intrinsic gas
 	gas, err := IntrinsicGas(st.data, contractCreation, homestead, istanbul, false)
 	if err != nil {
-		return nil, 0, false, err
+		return ExecutionResult{}, err
 	}
 	if err = st.useGas(gas); err != nil {
-		return nil, 0, false, err
+		return ExecutionResult{}, err
 	}
 
-	var (
-		evm = st.evm
-		// vm errors do not effect consensus and are therefor
-		// not assigned to err, except for insufficient balance
-		// error.
-		vmerr error
-	)
+	evm := st.evm
+
+	var ret []byte
+	// All VM errors are valid except for insufficient balance, therefore returned separately
+	var vmErr error
+
 	if contractCreation {
-		ret, _, st.gas, vmerr = evm.Create(sender, st.data, st.gas, st.value)
+		ret, _, st.gas, vmErr = evm.Create(sender, st.data, st.gas, st.value)
 	} else {
 		// Increment the nonce for the next transaction
 		st.state.SetNonce(msg.From(), st.state.GetNonce(sender.Address())+1)
-		ret, st.gas, vmerr = evm.Call(sender, st.to(), st.data, st.gas, st.value)
+		ret, st.gas, vmErr = evm.Call(sender, st.to(), st.data, st.gas, st.value)
 	}
-	if vmerr != nil {
-		utils.Logger().Info().Err(vmerr).Msg("VM returned with error")
+	if vmErr != nil {
+		utils.Logger().Info().Err(vmErr).Msg("VM returned with error")
 		// The only possible consensus-error would be if there wasn't
 		// sufficient balance to make the transfer happen. The first
 		// balance transfer may never fail.
 
-		if vmerr == vm.ErrInsufficientBalance {
-			return nil, 0, false, vmerr
+		if vmErr == vm.ErrInsufficientBalance {
+			return ExecutionResult{}, vmErr
 		}
 	}
 	st.refundGas()
@@ -269,7 +274,11 @@ func (st *StateTransition) TransitionDb() (ret []byte, usedGas uint64, failed bo
 		st.state.AddBalance(st.evm.Coinbase, txFee)
 	}
 
-	return ret, st.gasUsed(), vmerr != nil, err
+	return ExecutionResult{
+		ReturnData: ret,
+		UsedGas:    st.gasUsed(),
+		VMErr:      vmErr,
+	}, err
 }
 
 func (st *StateTransition) refundGas() {

hmy/hmy.go

@@ -200,11 +200,10 @@ func (hmy *Harmony) GetNodeMetadata() commonRPC.NodeMetadata {
 }
 
 // GetEVM returns a new EVM entity
-func (hmy *Harmony) GetEVM(ctx context.Context, msg core.Message, state *state.DB, header *block.Header) (*vm.EVM, func() error, error) {
+func (hmy *Harmony) GetEVM(ctx context.Context, msg core.Message, state *state.DB, header *block.Header) (*vm.EVM, error) {
 	state.SetBalance(msg.From(), math.MaxBig256)
-	vmError := func() error { return nil }
 	vmCtx := core.NewEVMContext(msg, header, hmy.BlockChain, nil)
-	return vm.NewEVM(vmCtx, state, hmy.BlockChain.Config(), *hmy.BlockChain.GetVMConfig()), vmError, nil
+	return vm.NewEVM(vmCtx, state, hmy.BlockChain.Config(), *hmy.BlockChain.GetVMConfig()), nil
 }
 
 // ChainDb ..

rpc/contract.go

@@ -15,7 +15,7 @@ import (
 	"github.com/harmony-one/harmony/core/types"
 	"github.com/harmony-one/harmony/core/vm"
 	"github.com/harmony-one/harmony/hmy"
-	internal_common "github.com/harmony-one/harmony/internal/common"
+	hmyCommon "github.com/harmony-one/harmony/internal/common"
 	"github.com/harmony-one/harmony/internal/utils"
 )
 
@@ -50,10 +50,13 @@ func (s *PublicContractService) Call(
 	blockNum := blockNumber.EthBlockNumber()
 
 	// Execute call
-	result, _, _, err := doCall(ctx, s.hmy, args, blockNum, vm.Config{}, CallTimeout, s.hmy.RPCGasCap)
+	result, err := doCall(ctx, s.hmy, args, blockNum, vm.Config{}, CallTimeout, s.hmy.RPCGasCap)
+	if err != nil {
+		return nil, err
+	}
 
-	// Response output is the same for all versions
-	return result, err
+	// If VM returns error, still return the ReturnData, which is the contract error message
+	return result.ReturnData, nil
 }
 
 // GetCode returns the code stored at the given address in the state for the given block number.
@@ -64,7 +67,7 @@ func (s *PublicContractService) GetCode(
 	blockNum := blockNumber.EthBlockNumber()
 
 	// Fetch state
-	address := internal_common.ParseAddr(addr)
+	address := hmyCommon.ParseAddr(addr)
 	state, _, err := s.hmy.StateAndHeaderByNumber(ctx, blockNum)
 	if state == nil || err != nil {
 		return nil, err
@@ -89,7 +92,7 @@ func (s *PublicContractService) GetStorageAt(
 	if state == nil || err != nil {
 		return nil, err
 	}
-	address := internal_common.ParseAddr(addr)
+	address := hmyCommon.ParseAddr(addr)
 	res := state.GetState(address, common.HexToHash(key))
 
 	// Response output is the same for all versions
@@ -100,7 +103,7 @@ func (s *PublicContractService) GetStorageAt(
 func doCall(
 	ctx context.Context, hmy *hmy.Harmony, args CallArgs, blockNum rpc.BlockNumber,
 	vmCfg vm.Config, timeout time.Duration, globalGasCap *big.Int,
-) ([]byte, uint64, bool, error) {
+) (core.ExecutionResult, error) {
 	defer func(start time.Time) {
 		utils.Logger().Debug().
 			Dur("runtime", time.Since(start)).
@@ -110,7 +113,7 @@ func doCall(
 	// Fetch state
 	state, header, err := hmy.StateAndHeaderByNumber(ctx, blockNum)
 	if state == nil || err != nil {
-		return nil, 0, false, err
+		return core.ExecutionResult{}, err
 	}
 
 	// Set sender address or use a default if none specified
@@ -166,9 +169,9 @@ func doCall(
 	defer cancel()
 
 	// Get a new instance of the EVM.
-	evm, vmError, err := hmy.GetEVM(ctx, msg, state, header)
+	evm, err := hmy.GetEVM(ctx, msg, state, header)
 	if err != nil {
-		return nil, 0, false, err
+		return core.ExecutionResult{}, err
 	}
 
 	// Wait for the context to be done and cancel the evm. Even if the
@@ -181,16 +184,16 @@ func doCall(
 	// Setup the gas pool (also for unmetered requests)
 	// and apply the message.
 	gp := new(core.GasPool).AddGas(math.MaxUint64)
-	res, gas, failed, err := core.ApplyMessage(evm, msg, gp)
-	if err := vmError(); err != nil {
-		return nil, 0, false, err
+	result, err := core.ApplyMessage(evm, msg, gp)
+	if err != nil {
+		return core.ExecutionResult{}, err
 	}
 
 	// If the timer caused an abort, return an appropriate error message
 	if evm.Cancelled() {
-		return nil, 0, false, fmt.Errorf("execution aborted (timeout = %v)", timeout)
+		return core.ExecutionResult{}, fmt.Errorf("execution aborted (timeout = %v)", timeout)
 	}
 
 	// Response output is the same for all versions
-	return res, gas, failed, err
+	return result, nil
 }

rpc/transaction.go

@@ -724,8 +724,8 @@ func doEstimateGas(
 	executable := func(gas uint64) bool {
 		args.Gas = (*hexutil.Uint64)(&gas)
 
-		_, _, failed, err := doCall(ctx, hmy, args, blockNum, vm.Config{}, 0, big.NewInt(int64(max)))
-		if err != nil || failed {
+		result, err := doCall(ctx, hmy, args, blockNum, vm.Config{}, 0, big.NewInt(int64(max)))
+		if err != nil || result.VMErr == nil {
 			return false
 		}
 		return true