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

make MaxTxGasWanted configurable #1004

Merged
merged 4 commits into from
Mar 21, 2022
Merged

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Mar 17, 2022

Description

Closes: #1003

Solution:

  • add config item: evm.max-tx-gas-wanted

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)

Closes: evmos#1003
Solution:
- add config item: evm.max-tx-gas-wanted
@yihuang yihuang force-pushed the max-gas-wanted-config branch from 0da9aaa to 67aef95 Compare March 17, 2022 03:49
@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #1004 (d559025) into main (93a57bc) will increase coverage by 0.01%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1004      +/-   ##
==========================================
+ Coverage   59.13%   59.15%   +0.01%     
==========================================
  Files          79       79              
  Lines        6534     6539       +5     
==========================================
+ Hits         3864     3868       +4     
- Misses       2452     2453       +1     
  Partials      218      218              
Impacted Files Coverage Δ
server/config/config.go 21.34% <50.00%> (+0.35%) ⬆️
app/ante/eth.go 82.42% <100.00%> (+0.05%) ⬆️
app/ante/handler_options.go 77.94% <100.00%> (ø)
app/app.go 85.49% <100.00%> (+0.06%) ⬆️

@yihuang yihuang requested a review from fedekunze March 21, 2022 05:46
@fedekunze fedekunze merged commit 8bcdb2c into evmos:main Mar 21, 2022
yihuang added a commit to yihuang/ethermint that referenced this pull request Mar 22, 2022
yihuang added a commit to yihuang/ethermint that referenced this pull request Mar 24, 2022
@yihuang yihuang deleted the max-gas-wanted-config branch May 3, 2022 04:45
@zmanian
Copy link

zmanian commented May 7, 2022

This doesn't really solve the issue.

On evmos right now, 80% of tx volume is logic of " test if arb is profitable, if profitable arb otherwise revert". This results in a 7x ratio between gas wanted and gas used which then results in full blocks.

This is what we originally discussed for this scenario.

cosmos/cosmos-sdk#2150

Gas should be charged at max(gas_used, gas_wanted/2)

You may want to just do this in the ethermint ante handler.

@yihuang
Copy link
Contributor Author

yihuang commented May 8, 2022

Gas should be charged at max(gas_used, gas_wanted/2)

Do you mean only refund gas_wanted - max(gas_used, gas_wanted/2) at the end of tx processing?

gas_wanted - max(gas_used, gas_wanted/2) == min(gas_wanted - gas_used, gas_wanted / 2)

@yihuang
Copy link
Contributor Author

yihuang commented May 18, 2022

This doesn't really solve the issue.

On evmos right now, 80% of tx volume is logic of " test if arb is profitable, if profitable arb otherwise revert". This results in a 7x ratio between gas wanted and gas used which then results in full blocks.

This is what we originally discussed for this scenario.

cosmos/cosmos-sdk#2150

Gas should be charged at max(gas_used, gas_wanted/2)

You may want to just do this in the ethermint ante handler.

#1085

I opened an issue here following your suggestion, summarize the consequences of this as I understand.

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.

Problem: the MaxTxGasWanted is not configurable
3 participants