do binary search to estimate gas #272

yihuang · 2021-07-13T04:58:35Z

Closes #268

part of it is extracted into #276.

Description

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2021-07-13T05:27:50Z

Codecov Report

Merging #272 (874174b) into main (072f053) will decrease coverage by 1.27%.
The diff coverage is 8.25%.

@@            Coverage Diff             @@
##             main     #272      +/-   ##
==========================================
- Coverage   47.46%   46.18%   -1.28%     
==========================================
  Files          46       46              
  Lines        3160     3267     +107     
==========================================
+ Hits         1500     1509       +9     
- Misses       1585     1683      +98     
  Partials       75       75

Impacted Files	Coverage Δ
x/evm/keeper/grpc_query.go	`57.51% <0.00%> (-17.35%)`	⬇️
x/evm/keeper/state_transition.go	`5.92% <0.00%> (ø)`
x/evm/types/query.pb.gw.go	`0.00% <0.00%> (ø)`
x/evm/types/utils.go	`57.14% <100.00%> (+14.83%)`	⬆️

fedekunze · 2021-07-13T08:02:38Z

Let's split the binary search logic from the changes in the response. Also if you could add a test suite for the binary search because it's quite critical in the state transition logic

leejw51crypto · 2021-07-13T08:10:58Z

still , default gas is not working
i will check further

run modifying contract function without "Gas" options.

error is different

Error: insufficient funds for intrinsic transaction cost (error={"reason":"processing response error","code":"SERVER_ERROR","body":"{\"jsonrpc\":\"2.0\",\"id\":49,\"error\":
{\"code\":-32000,\"message\":\"rpc error: code = InvalidArgument desc = failed to refund gas leftover gas to sender 0x48b212A71eBbB202F7cfD1aACee3A36FdE2Fbc51\\n --- at
 github.com/tharsis/ethermint/x/evm/keeper/state_transition.go:243 (Keeper.ApplyMessage) ---\\nCaused by: failed to refund 4968853 leftover gas (99377060aphoton)\\n --- at 
github.com/tharsis/ethermint/x/evm/keeper/state_transition.go:317 (Keeper.RefundGas) ---\\nCaused by: fee collector account failed to refund fees: 8022600aphoton is smaller than 
99377060aphoton: insufficient funds: insufficient funds: invalid request\"}}\n","error":
{"code":-32000},"requestBody":"{\"method\":\"eth_estimateGas\",\"params\":[{\"gasPrice\":\"0x14\",\"type\":\"0x0\",\"from\":\"0x48b212a71ebbb202f7cfd1aacee3a
36fde2fbc51\",\"to\":\"0x8d7a9159ecee2c3e0e8b2c57c7d47c6fbc9b5a0d\",\"data\":
\"0x6057361d0000000000000000000000000000000000000000000000000000000000000015\"}],\"id\":49,\"jsonrpc\":\"2.0\"}","requestMethod":
"POST","url":"http://localhost:8545"}, method="estimateGas", transaction={"from":"0x48b212A71eBbB202F7cfD1aACee3A36FdE2Fbc51",
"gasPrice":{"type":"BigNumber","hex":"0x14"},"to":"0x8D7A9159ECEE2c3E0E8b2C57C7D47C
6fBC9B5a0d","data":"0x6057361d0000000000000000000000000000000000000000000000000000000000000015","type":0,"accessList":null}, 
code=INSUFFICIENT_FUNDS, version=providers/5.4.1)

leejw51crypto · 2021-07-13T08:47:41Z

Msg Gas 401130 intrinsicGas 77768
ApplyMessage ConsumedGas 323362 UsedGas 401130

yihuang · 2021-07-13T08:53:44Z

rebased on treat all vm errors the same as reverted #276.
moved binary search logic into rpc api, otherwise, not easy to create clean ctx for each run.

yihuang · 2021-07-13T08:55:07Z

Msg Gas 401130 intrinsicGas 77768
ApplyMessage ConsumedGas 323362 UsedGas 401130

In my test case, with this PR, the gas used is identical to the estimated one.

leejw51crypto · 2021-07-13T08:59:25Z

yes, strange

Error: insufficient funds for intrinsic transaction cost (error={"reason":
"processing response error","code":"SERVER_ERROR","body":"{\"jsonrpc\":\"2.0\",\"id\":49,\"error\":{\"code\":-32000,\"message\":\
"rpc error: code = InvalidArgument desc = failed to refund gas leftover
 gas to sender 0x48b212A71eBbB202F7cfD1aACee3A36FdE2Fbc51\\n --- at github.com/tharsis/ethermint/x/evm/keeper/state_transition.go:244
 (Keeper.ApplyMessage) ---\\nCaused by: failed to refund 4968853 leftover gas (99377060aphoton)\\n --- at github.com/tharsis/ethermint/x/evm/keeper/state_transition.go:318
 (Keeper.RefundGas) ---\\nCaused by: fee collector account failed t
o refund fees: 8022600aphoton is smaller than 99377060aphoton: insufficient funds: insufficient funds: invalid request\"}}\n","error":{"code":-32000},"requestBody":"{\"method\":\"eth_estimateGas\",\"params\":[{\"gasPrice\":\"0x14\",\"type\":\"0x0\",\"from\":\"0x48b212a71ebb
b202f7cfd1aacee3a36fde2fbc51\",\"to\":\"0xdd0a253ae513c51d2c497
cad84c3998390fa34ed\",\"data\":\"0x6057361d0000000000000000000
000000000000000000000000000000000000000000015\"}],\"id\":
49,\"jsonrpc\":\"2.0\"}","requestMethod":"POST","url":"http://localhost:8545"}, method="estimateGas", transaction={"from":"0x48b212A71eBbB202F7cfD1aACee3A36FdE2Fbc51","gasPrice":{"type":"BigNumber","hex":"0x14"},"to":"0xdD0A253aE513C51D2c497cAd8
4C3998390FA34ED","data":"0x6057361d0000000000000000000000000
000000000000000000000000000000000000015","type":0,"accessList":null}, code=INSUFFICIENT_FUNDS, version=providers/5.4.1)

leejw51crypto · 2021-07-13T09:03:28Z

in ApplyMessage,
leftoverGas, err = k.RefundGas(msg, leftoverGas)
if err != nil {
return nil, stacktrace.Propagate(err, "failed to refund gas leftover gas to sender %s", msg.From())
}

that line fails

yihuang · 2021-07-13T09:23:25Z

in ApplyMessage,
leftoverGas, err = k.RefundGas(msg, leftoverGas)
if err != nil {
return nil, stacktrace.Propagate(err, "failed to refund gas leftover gas to sender %s", msg.From())
}

that line fails

When sending tx or doing estimate gas?

leejw51crypto · 2021-07-13T09:26:24Z

without any gas option , not working
with gas option, working

leejw51crypto · 2021-07-13T11:15:37Z

server side log

t=2021-07-13T20:13:00+0900 lvl=warn msg
="Served eth_estimateGas"         
  conn=127.0.0.1:59245 reqid=49 t="621.75µs"            
   err="rpc error: code = InvalidArgument desc =
 failed to refund gas leftover gas to sender 0x48b212A71eBbB202F7cfD1aAC
ee3A36FdE2Fbc51\n 
--- at github.com/tharsis/ethermint/x/evm
/keeper/state_transition.go:244 
(Keeper.ApplyMessage) ---\nCaused by:
 failed to refund 4968853
leftover gas (99377060aphoton)\n --- at github.com/tharsis/ethermint/x/evm/
keeper/state_transition.go:323 
(Keeper.RefundGas) ---\n
Caused by: fee collector account failed
 to refund fees: 8022600aphoton 
is smaller than
 99377060aphoton: insufficient funds: 
insufficient funds: invalid request"

yihuang · 2021-07-13T11:43:25Z

server side log

t=2021-07-13T20:13:00+0900 lvl=warn msg
="Served eth_estimateGas"         
  conn=127.0.0.1:59245 reqid=49 t="621.75µs"            
   err="rpc error: code = InvalidArgument desc =
 failed to refund gas leftover gas to sender 0x48b212A71eBbB202F7cfD1aAC
ee3A36FdE2Fbc51\n 
--- at github.com/tharsis/ethermint/x/evm
/keeper/state_transition.go:244 
(Keeper.ApplyMessage) ---\nCaused by:
 failed to refund 4968853
leftover gas (99377060aphoton)\n --- at github.com/tharsis/ethermint/x/evm/
keeper/state_transition.go:323 
(Keeper.RefundGas) ---\n
Caused by: fee collector account failed
 to refund fees: 8022600aphoton 
is smaller than
 99377060aphoton: insufficient funds: 
insufficient funds: invalid request"

I think you are using a different version, judging by the line numbers in stack trace.

leejw51crypto · 2021-07-14T01:01:54Z

i put a lot of logs, to this pr
binary is not exactly the same.

when give gasLimit option, always successful, if gasLimit is enough. no issue.
but when without gasLimit
gasLimit is automatically supplied, it occurs.

i'll include it to integration test.

i'll fetch --all, and check again
i think something simple related to activate EstimateGas with gasLimit option in rpc

leejw51crypto · 2021-07-14T02:52:11Z

confirmed working good

leejw51crypto

lgtm

fedekunze

Looks definitely better, see the comment about recursion. There are some conflicts that need to be resolved

ethereum/rpc/namespaces/eth/api.go

thomas-nguy

overall lgtm

ethereum/rpc/namespaces/eth/api.go

x/evm/keeper/state_transition.go

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

khoslaventures

great stuff

ethereum/rpc/namespaces/eth/api.go

khoslaventures · 2021-07-19T08:31:21Z

x/evm/keeper/grpc_query.go

+		return nil, status.Error(codes.InvalidArgument, err.Error())
+	}
+
+	// Binary search the gas requirement, as it may be higher than the amount used


it's interesting that the binary search on geth is used by the public api side meaning it is done by the client of this grpc server.

At the moment the only relevant issue I've found is ethereum/go-ethereum#21769

I think we should do the binary search on the public API (grpc client side) for sake of matching what they do in Geth

one rpc call emites several grpc query logs.

But this is a reasonable pattern right? I think we can adjust the logs to make it more clear that these are part of the same process... effectively it is slow and painful to estimate gas so logging completions per call is not necessarily bad

@fedekunze also any thoughts on making these RPC server files grpc_query.go files grpc_server.go - you might have more context behind the naming but I think it's less obvious.

One thing that's less clean when doing binary search in rpc api side(or grpc client side) is checking the error type for the core.ErrIntrinsicGas error, in grpc server side, you can inspect the error type directly, in grpc client side, you have to find a sub string in the error message which is not as accurate.

This has to do with not being able to use error.Is right? Are you sure that's the case? doCall for example will run through a contract call and expose these error types through vmError in Geth, we can do this as well?

This has to do with not being able to use error.Is right? Are you sure that's the case? doCall for example will run through a contract call and expose these error types through vmError in Geth, we can do this as well?

Do you mean MsgEthereumTxResponse.VmError? that is serialized version of these errors, when these errors are not further wrapped, you can at least compare the error string as a whole. But if the error is wrapped with stacktrace and grpc status, the best you can do in the grpc client side is strings.Contains.

fedekunze · 2021-07-19T15:09:10Z

x/evm/keeper/grpc_query.go

@@ -393,3 +400,100 @@ func (k Keeper) EthCall(c context.Context, req *types.EthCallRequest) (*types.Ms

 	return res, nil
 }
+
+// EstimateGas implements eth_estimateGas rpc api.
+func (k Keeper) EstimateGas(c context.Context, req *types.EthCallRequest) (*types.EstimateGasResponse, error) {


can you add tests for this on a follow-up PR?

I tried to do it, but there's so many context prepare things to do in unit test environment, I'm not sure how to do it.

With the default KeeperTestSuite, GetCoinbaseAddress will raise an error

We need to allocate some funds in genesis.

Co-authored-by: yihuang <huang@crypto.com>

yihuang requested review from fedekunze and khoslaventures as code owners July 13, 2021 04:58

yihuang force-pushed the estimate-gas branch from 39c0803 to 334f8c7 Compare July 13, 2021 05:24

yijiasu-crypto mentioned this pull request Jul 13, 2021

Assembly instruction invalid() was not behaving correctly #274

Closed

yihuang force-pushed the estimate-gas branch from 334f8c7 to 4e600f1 Compare July 13, 2021 08:52

yihuang force-pushed the estimate-gas branch from 4e600f1 to 287eeff Compare July 13, 2021 08:56

leejw51crypto approved these changes Jul 14, 2021

View reviewed changes

This was referenced Jul 14, 2021

Problem: etherjs smartcontract cannot change state (fix #257) #264

Closed

Problem: etherjs smartcontract cannot change state #257

Closed

Problem: evm.Call doesn't consume Gas in EstimateGas #275

Closed

yihuang force-pushed the estimate-gas branch 5 times, most recently from 9488375 to a16848e Compare July 15, 2021 02:29

fedekunze reviewed Jul 15, 2021

View reviewed changes

ethereum/rpc/namespaces/eth/api.go Outdated Show resolved Hide resolved

yihuang force-pushed the estimate-gas branch from a16848e to c06e735 Compare July 15, 2021 10:19

thomas-nguy reviewed Jul 15, 2021

View reviewed changes

ethereum/rpc/namespaces/eth/api.go Show resolved Hide resolved

thomas-nguy approved these changes Jul 15, 2021

View reviewed changes

fedekunze reviewed Jul 15, 2021

View reviewed changes

ethereum/rpc/namespaces/eth/api.go Outdated Show resolved Hide resolved

x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved

yijiasu-crypto mentioned this pull request Jul 15, 2021

Fix CI to run solidity tests #278

Merged

11 tasks

yihuang added 2 commits July 16, 2021 07:45

do binary search to estimate gas

5a46370

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

5f674f9

yihuang force-pushed the estimate-gas branch 2 times, most recently from 5fc189e to e003b32 Compare July 16, 2021 02:18

do estimateGas in grpc query

b38f399

yihuang force-pushed the estimate-gas branch from e003b32 to b38f399 Compare July 16, 2021 02:19

Merge branch 'main' into estimate-gas

3d06d8a

khoslaventures reviewed Jul 19, 2021

View reviewed changes

fedekunze approved these changes Jul 19, 2021

View reviewed changes

Merge branch 'main' into estimate-gas

874174b

fedekunze enabled auto-merge (squash) July 19, 2021 15:12

fedekunze merged commit 14b38af into evmos:main Jul 19, 2021

yihuang deleted the estimate-gas branch July 19, 2021 15:38

yihuang mentioned this pull request Jul 20, 2021

Setup more sophisticated testing environment in unit test suite #323

Closed

mmsqe referenced this pull request in mmsqe/ethermint Jun 14, 2023

feat: integrate tendermint bootstrap cmd (#242) (#272)

03441c3

Co-authored-by: yihuang <huang@crypto.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

do binary search to estimate gas #272

do binary search to estimate gas #272

yihuang commented Jul 13, 2021 •

edited

Loading

codecov bot commented Jul 13, 2021 •

edited

Loading

fedekunze commented Jul 13, 2021

leejw51crypto commented Jul 13, 2021 •

edited

Loading

leejw51crypto commented Jul 13, 2021

yihuang commented Jul 13, 2021

yihuang commented Jul 13, 2021

leejw51crypto commented Jul 13, 2021 •

edited

Loading

leejw51crypto commented Jul 13, 2021

yihuang commented Jul 13, 2021

leejw51crypto commented Jul 13, 2021

leejw51crypto commented Jul 13, 2021 •

edited

Loading

yihuang commented Jul 13, 2021

leejw51crypto commented Jul 14, 2021 •

edited

Loading

leejw51crypto commented Jul 14, 2021

leejw51crypto left a comment

fedekunze left a comment

thomas-nguy left a comment

khoslaventures left a comment

khoslaventures Jul 19, 2021

yihuang Jul 19, 2021 •

edited

Loading

khoslaventures Jul 19, 2021

yihuang Jul 19, 2021 •

edited

Loading

fedekunze Jul 19, 2021

yihuang Jul 20, 2021

do binary search to estimate gas #272

do binary search to estimate gas #272

Conversation

yihuang commented Jul 13, 2021 • edited Loading

Description

codecov bot commented Jul 13, 2021 • edited Loading

Codecov Report

fedekunze commented Jul 13, 2021

leejw51crypto commented Jul 13, 2021 • edited Loading

leejw51crypto commented Jul 13, 2021

yihuang commented Jul 13, 2021

yihuang commented Jul 13, 2021

leejw51crypto commented Jul 13, 2021 • edited Loading

leejw51crypto commented Jul 13, 2021

yihuang commented Jul 13, 2021

leejw51crypto commented Jul 13, 2021

leejw51crypto commented Jul 13, 2021 • edited Loading

yihuang commented Jul 13, 2021

leejw51crypto commented Jul 14, 2021 • edited Loading

leejw51crypto commented Jul 14, 2021

leejw51crypto left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

thomas-nguy left a comment

Choose a reason for hiding this comment

khoslaventures left a comment

Choose a reason for hiding this comment

khoslaventures Jul 19, 2021

Choose a reason for hiding this comment

yihuang Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

khoslaventures Jul 19, 2021

Choose a reason for hiding this comment

yihuang Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

fedekunze Jul 19, 2021

Choose a reason for hiding this comment

yihuang Jul 20, 2021

Choose a reason for hiding this comment

yihuang commented Jul 13, 2021 •

edited

Loading

codecov bot commented Jul 13, 2021 •

edited

Loading

leejw51crypto commented Jul 13, 2021 •

edited

Loading

leejw51crypto commented Jul 13, 2021 •

edited

Loading

leejw51crypto commented Jul 13, 2021 •

edited

Loading

leejw51crypto commented Jul 14, 2021 •

edited

Loading

yihuang Jul 19, 2021 •

edited

Loading

yihuang Jul 19, 2021 •

edited

Loading