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

Wasm upgrade from main #67

Merged
merged 16 commits into from
Jan 30, 2023
Merged

Wasm upgrade from main #67

merged 16 commits into from
Jan 30, 2023

Conversation

nghuyenthevinh2000
Copy link
Member

@nghuyenthevinh2000 nghuyenthevinh2000 commented Jan 25, 2023

Summary of changes

Clean updating wasm from main branch.

cc: @fragwuerdig

This is work from: #49

Change:

  1. added in benchmarking tool to calculate gas
  2. update Dockerfile
  3. refactor test

Report of required housekeeping

  • Github issue OR spec proposal link
  • Wrote tests
  • Updated API documentation (client/lcd/swagger-ui/swagger.yaml)
  • Added a relevant changelog entry: clog add [section] [stanza] [message]

(FOR ADMIN) Before merging

  • Added appropriate labels to PR
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
  • Confirm added tests are consistent with the intended behavior of changes
  • Ensure all tests pass

@nghuyenthevinh2000 nghuyenthevinh2000 mentioned this pull request Jan 25, 2023
8 tasks
@nghuyenthevinh2000
Copy link
Member Author

I find the gas used by wasmvm after upgrade to v1.0.0 is pretty staggering. I put a quick comparison between v0.16.6 and v1.0.0 gas consumption here:

  1. v0.16.6: 141633
  2. v1.0.0: 1874816400

that is x10000

@fragwuerdig @ZaradarBH

@fragwuerdig
Copy link
Collaborator

fragwuerdig commented Jan 25, 2023

@nghuyenthevinh2000 did you adjust the default gas multiplier in this branch?

Please look in my original #49 and try to cherry pick as much as you can from there.

Old gas multiplier was 100. New one is about 140,000,000.

@nghuyenthevinh2000
Copy link
Member Author

@nghuyenthevinh2000 did you adjust the default gas multiplier in this branch?

Please look in my original #49 and try to cherry pick as much as you can from there.

Old gas multiplier was 100. New one is about 140,000,000.

I find this very interesting actually. Like, why wasmvm just increases its gas price so huge like that.

@nghuyenthevinh2000
Copy link
Member Author

@fragwuerdig

I really want to bring our x/wasm closer to CosmWasm x/wasm so that one day we can just use x/wasm and save us from the pain of maintaining this module. After comparing the two, I realize that there are a lot of functions with same functionality but different name.

@fragwuerdig
Copy link
Collaborator

fragwuerdig commented Jan 26, 2023

@nghuyenthevinh2000 so you are suggesting to refactor towards wasmd implementation? We can definitely work on that. Could we have a separate issue for that an just focus on getting the upgrade done for now? Or do you think refactoring should be done asap?

@ZaradarBH ZaradarBH added enhancement New feature or request in scope Work approved by the community labels Jan 26, 2023
@nghuyenthevinh2000
Copy link
Member Author

@nghuyenthevinh2000 so you are suggesting to refactor towards wasmd implementation? We can definitely work on that. Could we have a separate issue for that an just focus on getting the upgrade done for now? Or do you think refactoring should be done asap?

Overtime I would say, right now, I am trying to get the testing code of wasmd into ours.

@ZaradarBH
Copy link
Contributor

Need to fix the broken tests :)

@ZaradarBH ZaradarBH added this to the v2.0.4 milestone Jan 26, 2023
x/wasm/keeper/api.go Outdated Show resolved Hide resolved
@nghuyenthevinh2000
Copy link
Member Author

I have been playing around with gas to try to see if it can be calculated. The wasmVM gas calculation behaves differently from last version. After understanding the flow of gas calculation in wasmVM, my conclusion is that however wasmVM behaves we should only concern about GasMultiplier.

Name of the game: guess GasMultiplier so that it breaks as less test as possible.
Some tests are guaranteed to break:

  • TestGasCostOnQuery
  • TestLimitRecursiveQueryGas

Because they check for exact gas which will always break everytime GasMultiplier is changed.

I find that GasMultiplier = 15_000_000 breaks the least amount of gas test

@ZaradarBH ZaradarBH merged commit ea5b3ca into main Jan 30, 2023
@nghuyenthevinh2000
Copy link
Member Author

I will write a separate pr for wasm upgrade testing

@fragwuerdig fragwuerdig deleted the wasm-upgrade branch May 25, 2023 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in scope Work approved by the community
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants