Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(evm): The estimated gas and the gas used for executing the message are not consistent #1943

Merged
merged 18 commits into from Nov 2, 2023

Conversation

cuiweixie
Copy link
Contributor

@cuiweixie cuiweixie commented Oct 26, 2023

Description

when execute a eth msg, the kv gas config is zeros, see comments.
but gas config is not zero in estimate gas.
these will cause estimate gas return more gas that execution really need when call precompile smart contract.
set the estimate gas to same as in execution.


Closes #XXX

@cuiweixie cuiweixie requested a review from a team as a code owner October 26, 2023 06:27
@cuiweixie cuiweixie requested review from facs95 and MalteHerrmann and removed request for a team October 26, 2023 06:27
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #1943 (7abbc04) into main (b826687) will increase coverage by 0.28%.
The diff coverage is 88.88%.

❗ Current head 7abbc04 differs from pull request most recent head be75ba3. Consider uploading reports for the commit be75ba3 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1943      +/-   ##
==========================================
+ Coverage   69.70%   69.99%   +0.28%     
==========================================
  Files         331      327       -4     
  Lines       24514    24292     -222     
==========================================
- Hits        17088    17003      -85     
+ Misses       6543     6417     -126     
+ Partials      883      872      -11     
Files Coverage Δ
x/erc20/keeper/evm.go 92.15% <100.00%> (ø)
x/evm/keeper/grpc_query.go 86.73% <88.00%> (-0.02%) ⬇️

... and 8 files with indirect coverage changes

@cuiweixie cuiweixie force-pushed the estimate_gas branch 3 times, most recently from 5a53c87 to 616dc9e Compare October 26, 2023 07:15
@GAtom22
Copy link
Contributor

GAtom22 commented Oct 27, 2023

Thanks for the contribution @cuiweixie!!

We appreciate you reporting this. However, this is actually not a solution to the issue. The EstimateGas is not only used on the eth_estimateGas rpc API, but also in CallEVM function. The result is that the gasUsed and estimation are both less with these changes, but still have the same difference between them. Check out these results from this test:

Test with the changes proposed here:

FAILED test_precompiles.py::test_staking - AssertionError: assert 108073 (gasUsed) == 118573 (gas estimation)

Test with current code on main:

FAILED test_precompiles.py::test_staking - AssertionError: assert 205647 (gasUsed) == 216147  (gas estimation)

The difference in both cases is 10500

@cuiweixie
Copy link
Contributor Author

cuiweixie commented Oct 27, 2023

Thanks for the contribution @cuiweixie!!

We appreciate you reporting this. However, this is actually not a solution to the issue. The EstimateGas is not only used on the eth_estimateGas rpc API, but also in CallEVM function. The result is that the gasUsed and estimation are both less with these changes, but still have the same difference between them. Check out these results from this test:

Test with the changes proposed here:

FAILED test_precompiles.py::test_staking - AssertionError: assert 108073 (gasUsed) == 118573 (gas estimation)

Test with current code on main:

FAILED test_precompiles.py::test_staking - AssertionError: assert 205647 (gasUsed) == 216147  (gas estimation)

The difference in both cases is 10500

I will try to fix it in right way. If you know the right way to fix this, please tell me. I will update my pr.

x/evm/keeper/grpc_query.go Fixed Show fixed Hide fixed
@cuiweixie cuiweixie force-pushed the estimate_gas branch 2 times, most recently from 65d42fa to 9b7f160 Compare October 28, 2023 08:58
@cuiweixie
Copy link
Contributor Author

cuiweixie commented Oct 28, 2023

Thanks for the contribution @cuiweixie!!
We appreciate you reporting this. However, this is actually not a solution to the issue. The EstimateGas is not only used on the eth_estimateGas rpc API, but also in CallEVM function. The result is that the gasUsed and estimation are both less with these changes, but still have the same difference between them. Check out these results from this test:
Test with the changes proposed here:

FAILED test_precompiles.py::test_staking - AssertionError: assert 108073 (gasUsed) == 118573 (gas estimation)

Test with current code on main:

FAILED test_precompiles.py::test_staking - AssertionError: assert 205647 (gasUsed) == 216147  (gas estimation)

The difference in both cases is 10500

I will try to fix it in right way. If you know the right way to fix this, please tell me. I will update my pr.

@GAtom22 in the new codebase, the nix_tests(test_staking in test_precompiles.py) can passed now.

x/evm/keeper/grpc_query.go Fixed Show fixed Hide fixed
@cuiweixie cuiweixie force-pushed the estimate_gas branch 2 times, most recently from c12b3b7 to 2ca0c9f Compare October 28, 2023 15:36
x/evm/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/evm/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/evm/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/evm/keeper/grpc_query.go Outdated Show resolved Hide resolved
@GAtom22
Copy link
Contributor

GAtom22 commented Nov 1, 2023

Please add a changelog entry

x/evm/types/call_type.go Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Nov 1, 2023

CLA assistant check
All committers have signed the CLA.

CHANGELOG.md Outdated Show resolved Hide resolved
x/evm/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/evm/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/evm/keeper/grpc_query.go Outdated Show resolved Hide resolved
@GAtom22
Copy link
Contributor

GAtom22 commented Nov 1, 2023

Added a new test where there's a smart contract that calls the staking EVM extension, and in this case we're still having the same issue. The gas estimation is higher than the gas used:

# On the approval tx
gas used: 56951
gas estimation: 57049

# On the delegate tx
gas used: 121735
gas estimation: 122917

cuiweixie and others added 4 commits November 2, 2023 16:20
Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com>
Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com>
Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com>
Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com>
@cuiweixie
Copy link
Contributor Author

cuiweixie commented Nov 2, 2023

Added a new test where there's a smart contract that calls the staking EVM extension, and in this case we're still having the same issue. The gas estimation is higher than the gas used:

# On the approval tx
gas used: 56951
gas estimation: 57049

# On the delegate tx
gas used: 121735
gas estimation: 122917

@GAtom22 I find this may not be a issue.
after i change callGas in go-ethereum.

the testDelegate call is same now.

# On the approval tx
gas used: 56951
gas estimation: 56921

# On the delegate tx
gas used: 120730
gas estimation: 120730

i think the difference is caused by same issue in above link.
gas used in less that gas limit in gas estimation.
but if we change gas limit to gas used that can make tx run success,
the tx will failed.

what 's more!
just found that the first approval tx to same address is no equal.

estimate gas is binary search the gas limit. when find a fake minimal gasLimit, it doesn't means that the
gasLimit == gasUsed, gasUsed may less than gasLimit, because the dynamic gas stragety.

Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

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

tACK! Thanks for the contribution @cuiweixie!

Copy link
Contributor

@0xstepit 0xstepit left a comment

Choose a reason for hiding this comment

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

Great job @cuiweixie, and thanks a lot for contributing to our codebase and improving it!

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
x/evm/keeper/grpc_query.go Outdated Show resolved Hide resolved
facs95 and others added 2 commits November 2, 2023 10:51
Co-authored-by: stepit <48993133+0xstepit@users.noreply.github.com>
@GAtom22 GAtom22 enabled auto-merge (squash) November 2, 2023 17:48
@GAtom22 GAtom22 merged commit 6c1bb77 into evmos:main Nov 2, 2023
22 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants