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

Update default mempool max gas per tx to reflect current mainnet #5440

Closed
ibjc opened this issue Jun 6, 2023 · 7 comments · Fixed by #5468
Closed

Update default mempool max gas per tx to reflect current mainnet #5440

ibjc opened this issue Jun 6, 2023 · 7 comments · Fixed by #5468
Assignees

Comments

@ibjc
Copy link
Contributor

ibjc commented Jun 6, 2023

Background

The current default mempool max gas per tx value is the old 25M value:

max-gas-wanted-per-tx = "25000000"

Along with localosmosis instances, this apparently impacts edgenet? So transactions that exceed the 25M limit fail:

DefaultCreationGasFee = 40_000_000

Suggested Design

Please update to 120M to reflect current mainnet.

Acceptance Criteria

When spinning up a localosmosis container, $HOME/.osmosisd-local/config/app.toml's "max-gas-wanted-per-tx" should equal "120000000", like below:

ubuntu@ip-xxx-xx-xx-xxx:~/.osmosisd-local/config$ cat app.toml | grep "gas"
minimum-gas-prices = "0uosmo"
  arbitrage-min-gas-fee = ".005"
  max-gas-wanted-per-tx = "120000000" #<-- this guy
  min-gas-price-for-high-gas-tx = ".0025"
@p0mvn
Copy link
Member

p0mvn commented Jun 6, 2023

CC: @DongLieu

@DongLieu
Copy link
Contributor

DongLieu commented Jun 7, 2023

ok, tkank you

@ValarDragon
Copy link
Member

ValarDragon commented Jun 8, 2023

No this conclusion isn't right, the max gas wanted per tx should stay the same, its the token factory setting thats wrong.

We should change that to like 1M gas in my opinion.

@ibjc
Copy link
Contributor Author

ibjc commented Jun 8, 2023

Oh I see; I misunderstood the max gas aspect.

Then yea, the problem behavior of the tokenfactory create denom still exists due to the gas cost of 40M violating the 25M per-tx limit.

@ValarDragon
Copy link
Member

Thanks for flagging the problem, great catch!

Definitely concerning this got so far along / denom creation has been bricked on testnet for people

@stackman27 stackman27 self-assigned this Jun 8, 2023
@stackman27
Copy link
Contributor

stackman27 commented Jun 8, 2023

added fix in tokenfactory (#5468)

@ibjc could you please check if the issue is resolved with this PR or can you let me know how you generated this issue so i can test it on my end

@ibjc
Copy link
Contributor Author

ibjc commented Jun 8, 2023

thanks, looks good to me!

i generated this issue by spinning up a localosmosis container, and then dispatching the tokenfactory's create denom message.

for testing #5468, i just pointed the repo to the sis/tokenfactory-reduce-gas branch, and rebuilt the container.

the readme.md below roughly outlines the steps i took (except i didn't manually update the gas per tx config):

https://github.com/zodiac-protocol/tokenfactorytest/blob/04f2b6188063bb62a2004213606566b687afbbc7/README.md?plain=1#L5-L53

@github-project-automation github-project-automation bot moved this from Needs Triage 🔍 to Done ✅ in Osmosis Chain Development Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
5 participants