do binary search to estimate gas

Closes #268

- Also refactor ApplyMessage to be more reuseable

move binary search to rpc api side to have a clean context each try

remove EstimateGas grpc api

extract BinSearch function and add unit test

CHANGELOG.md

@@ -68,6 +68,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (rpc)  [#124](https://github.com/tharsis/ethermint/issues/124) Implement `txpool_content`, `txpool_inspect` and `txpool_status` RPC methods
 * (rpc) [tharsis#112](https://github.com/tharsis/ethermint/pull/153) Fix `eth_coinbase` to return the ethereum address of the validator
 * (rpc) [tharsis#176](https://github.com/tharsis/ethermint/issues/176) Support fetching pending nonce
+* (rpc) [tharsis#272](https://github.com/tharsis/ethermint/pull/272) do binary search to estimate gas accurately
 
 ### Bug Fixes
 

client/docs/swagger-ui/swagger.yaml

@@ -1721,6 +1721,12 @@ paths:
           required: false
           type: string
           format: byte
+        - name: gas_cap
+          description: the default gas cap to be used.
+          in: query
+          required: false
+          type: string
+          format: uint64
       tags:
         - Query
   /ethermint/evm/v1alpha1/params:

docs/core/proto-docs.md

@@ -476,6 +476,7 @@ EthCallRequest defines EthCall request
 | Field | Type | Label | Description |
 | ----- | ---- | ----- | ----------- |
 | `args` | [bytes](#bytes) |  | same json format as the json rpc api. |
+| `gas_cap` | [uint64](#uint64) |  | the default gas cap to be used |
 
 
 

ethereum/rpc/namespaces/eth/api.go

@@ -11,12 +11,15 @@ import (
 	"github.com/pkg/errors"
 	"github.com/spf13/viper"
 	"github.com/tendermint/tendermint/libs/log"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/client/flags"
 	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
 	"github.com/cosmos/cosmos-sdk/crypto/keyring"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 
@@ -25,9 +28,11 @@ import (
 	"github.com/ethereum/go-ethereum/accounts/keystore"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/common/hexutil"
+	"github.com/ethereum/go-ethereum/core"
 	ethtypes "github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/core/vm"
 	"github.com/ethereum/go-ethereum/crypto"
+	ethparams "github.com/ethereum/go-ethereum/params"
 
 	"github.com/tharsis/ethermint/crypto/hd"
 	"github.com/tharsis/ethermint/ethereum/rpc/backend"
@@ -529,6 +534,20 @@ func (e *PublicAPI) Call(args evmtypes.CallArgs, blockNr rpctypes.BlockNumber, _
 	return (hexutil.Bytes)(data.Ret), nil
 }
 
+// isIntrinsicGasError try to reflex the error internal, to decide if it's a `core.ErrIntrinsicGas`,
+// pretty hacky
+func isIntrinsicGasError(err error) bool {
+	if se, ok := status.FromError(err); ok {
+		wrapper := status.Error(
+			codes.Internal,
+			core.ErrIntrinsicGas.Error(),
+		)
+		sdkWrapper := sdkerrors.Wrap(sdkerrors.ErrUnknownRequest, wrapper.Error())
+		return se.Message() == sdkWrapper.Error()
+	}
+	return false
+}
+
 // DoCall performs a simulated call operation through the evmtypes. It returns the
 // estimated gas used on the operation or an error if fails.
 func (e *PublicAPI) doCall(
@@ -538,11 +557,21 @@ func (e *PublicAPI) doCall(
 	if err != nil {
 		return nil, err
 	}
-	req := evmtypes.EthCallRequest{Args: bz}
+	req := evmtypes.EthCallRequest{Args: bz, GasCap: ethermint.DefaultRPCGasLimit}
+
+	// From ContextWithHeight: if the provided height is 0,
+	// it will return an empty context and the gRPC query will use
+	// the latest block height for querying.
 	res, err := e.queryClient.EthCall(rpctypes.ContextWithHeight(blockNr.Int64()), &req)
 	if err != nil {
 		return nil, err
 	}
+	if len(res.VmError) > 0 {
+		if res.VmError == vm.ErrExecutionReverted.Error() {
+			return nil, evmtypes.NewExecErrorWithReason(res.Ret)
+		}
+		return nil, errors.New(res.VmError)
+	}
 
 	if res.Failed() {
 		if res.VmError == vm.ErrExecutionReverted.Error() {
@@ -555,18 +584,88 @@ func (e *PublicAPI) doCall(
 }
 
 // EstimateGas returns an estimate of gas usage for the given smart contract call.
-func (e *PublicAPI) EstimateGas(args evmtypes.CallArgs) (hexutil.Uint64, error) {
+func (e *PublicAPI) EstimateGas(args evmtypes.CallArgs, blockNrOptional *rpctypes.BlockNumber) (hexutil.Uint64, error) {
 	e.logger.Debug("eth_estimateGas")
 
-	// From ContextWithHeight: if the provided height is 0,
-	// it will return an empty context and the gRPC query will use
-	// the latest block height for querying.
-	data, err := e.doCall(args, 0)
+	blockNr := rpctypes.EthPendingBlockNumber
+	if blockNrOptional != nil {
+		blockNr = *blockNrOptional
+	}
+
+	// Binary search the gas requirement, as it may be higher than the amount used
+	var (
+		lo     uint64 = ethparams.TxGas - 1
+		hi     uint64
+		cap    uint64
+		gasCap uint64 = uint64(ethermint.DefaultRPCGasLimit)
+	)
+
+	// Determine the highest gas limit can be used during the estimation.
+	if args.Gas != nil && uint64(*args.Gas) >= ethparams.TxGas {
+		hi = uint64(*args.Gas)
+	} else {
+		// Query block gas limit
+		params, err := e.clientCtx.Client.ConsensusParams(e.ctx, blockNr.TmHeight())
+		if err != nil {
+			return 0, err
+		}
+		if params.ConsensusParams.Block.MaxGas > 0 {
+			hi = uint64(params.ConsensusParams.Block.MaxGas)
+		} else {
+			hi = gasCap
+		}
+	}
+
+	// TODO Recap the highest gas limit with account's available balance.
+
+	// Recap the highest gas allowance with specified gascap.
+	if gasCap != 0 && hi > gasCap {
+		hi = gasCap
+	}
+	cap = hi
+
+	// Create a helper to check if a gas allowance results in an executable transaction
+	executable := func(gas uint64) (bool, *evmtypes.MsgEthereumTxResponse, error) {
+		args.Gas = (*hexutil.Uint64)(&gas)
+		bz, err := json.Marshal(&args)
+		if err != nil {
+			return true, nil, err
+		}
+		req := evmtypes.EthCallRequest{Args: bz, GasCap: gasCap}
+		rsp, err := e.queryClient.EthCall(rpctypes.ContextWithHeight(blockNr.Int64()), &req)
+		if err != nil {
+			if isIntrinsicGasError(err) {
+				return true, nil, nil // Special case, raise gas limit
+			}
+			return true, nil, err // Bail out
+		}
+		return len(rsp.VmError) > 0, rsp, nil
+	}
+
+	// Execute the binary search and hone in on an executable gas limit
+	hi, err := evmtypes.BinSearch(lo, hi, executable)
 	if err != nil {
 		return 0, err
 	}
 
-	return hexutil.Uint64(data.GasUsed), nil
+	// Reject the transaction as invalid if it still fails at the highest allowance
+	if hi == cap {
+		failed, result, err := executable(hi)
+		if err != nil {
+			return 0, err
+		}
+		if failed {
+			if result != nil && result.VmError != vm.ErrOutOfGas.Error() {
+				if result.VmError == vm.ErrExecutionReverted.Error() {
+					return 0, evmtypes.NewExecErrorWithReason(result.Ret)
+				}
+				return 0, errors.New(result.VmError)
+			}
+			// Otherwise, the specified gas cap is too low
+			return 0, fmt.Errorf("gas required exceeds allowance (%d)", cap)
+		}
+	}
+	return hexutil.Uint64(hi), nil
 }
 
 // GetBlockByHash returns the block identified by hash.
@@ -1005,7 +1104,8 @@ func (e *PublicAPI) setTxDefaults(args rpctypes.SendTxArgs) (rpctypes.SendTxArgs
 			Data:       input,
 			AccessList: args.AccessList,
 		}
-		estimated, err := e.EstimateGas(callArgs)
+		blockNr := rpctypes.NewBlockNumber(big.NewInt(0))
+		estimated, err := e.EstimateGas(callArgs, &blockNr)
 		if err != nil {
 			return args, err
 		}

proto/ethermint/evm/v1alpha1/query.proto

@@ -71,6 +71,7 @@ service Query {
   rpc EthCall(EthCallRequest) returns (MsgEthereumTxResponse) {
     option (google.api.http).get = "/ethermint/evm/v1alpha1/eth_call";
   }
+
 }
 
 // QueryAccountRequest is the request type for the Query/Account RPC method.
@@ -251,4 +252,6 @@ message QueryStaticCallResponse {
 message EthCallRequest {
   // same json format as the json rpc api.
   bytes args = 1;
+  // the default gas cap to be used
+  uint64 gas_cap = 2;
 }

server/start.go

@@ -241,7 +241,7 @@ func startInProcess(ctx *server.Context, clientCtx client.Context, appCreator ty
 	if config.EVMRPC.Enable {
 		tmEndpoint := "/websocket"
 		tmRPCAddr := cfg.RPC.ListenAddress
-		logger.Info("EVM RPC Connecting to Tendermint WebSocket at", tmRPCAddr+tmEndpoint)
+		logger.Info("EVM RPC Connecting to Tendermint WebSocket at", "address", tmRPCAddr+tmEndpoint)
 		tmWsClient := ConnectTmWS(tmRPCAddr, tmEndpoint)
 
 		rpcServer := ethrpc.NewServer()

tests/rpc/rpc_test.go

@@ -728,9 +728,10 @@ func TestEth_EstimateGas(t *testing.T) {
 	param[0]["from"] = "0x" + fmt.Sprintf("%x", from)
 	param[0]["to"] = "0x1122334455667788990011223344556677889900"
 	param[0]["value"] = "0x1"
+	param[0]["gas"] = "0x5209"
 	rpcRes := call(t, "eth_estimateGas", param)
 	require.NotNil(t, rpcRes)
-	require.NotEmpty(t, rpcRes.Result)
+	require.Equal(t, rpcRes.Result, "0x5208")
 
 	var gas string
 	err := json.Unmarshal(rpcRes.Result, &gas)

x/evm/keeper/state_transition.go

@@ -231,8 +231,9 @@ func (k *Keeper) ApplyMessage(evm *vm.EVM, msg core.Message, cfg *params.ChainCo
 	}
 	// Should check again even if it is checked on Ante Handler, because eth_call don't go through Ante Handler.
 	if msg.Gas() < intrinsicGas {
-		// eth_estimateGas will check for this exact error
-		return nil, stacktrace.Propagate(core.ErrIntrinsicGas, "intrinsic gas too low")
+		// eth_estimateGas will check for this exact error,
+		// don't wrap it with stacktrace, that makes it very hard to decipher for the client
+		return nil, core.ErrIntrinsicGas
 	}
 	leftoverGas := msg.Gas() - intrinsicGas