Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

do binary search to estimate gas #272

Merged
merged 5 commits into from
Jul 19, 2021
Merged

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Jul 13, 2021

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
Copy link

codecov bot commented Jul 13, 2021

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor

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
Copy link
Contributor

leejw51crypto commented Jul 13, 2021

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
Copy link
Contributor

Msg Gas 401130 intrinsicGas 77768
ApplyMessage ConsumedGas 323362 UsedGas 401130

@yihuang
Copy link
Contributor Author

yihuang commented Jul 13, 2021

@yihuang
Copy link
Contributor Author

yihuang commented Jul 13, 2021

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
Copy link
Contributor

leejw51crypto commented Jul 13, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

yihuang commented Jul 13, 2021

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
Copy link
Contributor

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

@leejw51crypto
Copy link
Contributor

leejw51crypto commented Jul 13, 2021

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
Copy link
Contributor Author

yihuang commented Jul 13, 2021

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
Copy link
Contributor

leejw51crypto commented Jul 14, 2021

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
Copy link
Contributor

confirmed working good

Copy link
Contributor

@leejw51crypto leejw51crypto left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

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

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

@thomas-nguy thomas-nguy left a comment

Choose a reason for hiding this comment

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

overall lgtm

ethereum/rpc/namespaces/eth/api.go Outdated Show resolved Hide resolved
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
@yijiasu-crypto yijiasu-crypto mentioned this pull request Jul 15, 2021
11 tasks
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
@yihuang yihuang force-pushed the estimate-gas branch 2 times, most recently from 5fc189e to e003b32 Compare July 16, 2021 02:18
Copy link
Contributor

@khoslaventures khoslaventures left a comment

Choose a reason for hiding this comment

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

great stuff

ethereum/rpc/namespaces/eth/api.go Show resolved Hide resolved
return nil, status.Error(codes.InvalidArgument, err.Error())
}

// Binary search the gas requirement, as it may be higher than the amount used
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@yihuang yihuang Jul 19, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@yihuang yihuang Jul 19, 2021

Choose a reason for hiding this comment

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

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.

@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

  1. With the default KeeperTestSuite, GetCoinbaseAddress will raise an error
  2. We need to allocate some funds in genesis.

@fedekunze fedekunze enabled auto-merge (squash) July 19, 2021 15:12
@fedekunze fedekunze merged commit 14b38af into evmos:main Jul 19, 2021
@yihuang yihuang deleted the estimate-gas branch July 19, 2021 15:38
mmsqe referenced this pull request in mmsqe/ethermint Jun 14, 2023
Co-authored-by: yihuang <huang@crypto.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gas estimation is not accurate
5 participants