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

[gnovm/bug] deploying same contract to longer pkg_path spends more gas #2083

Open
r3v4s opened this issue May 13, 2024 · 9 comments
Open

[gnovm/bug] deploying same contract to longer pkg_path spends more gas #2083

r3v4s opened this issue May 13, 2024 · 9 comments
Assignees
Labels
🐞 bug Something isn't working

Comments

@r3v4s
Copy link
Contributor

r3v4s commented May 13, 2024

deploying same contract to longer pkg_path spends more gas

Description

Originally, this issue was founded here

Somehow, deploying same contract to longer pkg_path spends more gas
image

  • deploy to gno.land/p/demo/gnoswap/u1 => 9.5M
  • deploy to gno.land/p/demo/gnoswap/u2 => 9.5M
  • deploy to gno.land/p/demo/gnoswap/what_an_long_pkg_path => out of gas
  • deploy to gno.land/p/demo/gnoswap/longer_path => 9.9M
@r3v4s r3v4s added 🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related labels May 13, 2024
@r3v4s r3v4s changed the title [gnovm] deploying same contract to longer pkg_path spends more gas [gnovm/bug] deploying same contract to longer pkg_path spends more gas May 14, 2024
@zivkovicmilos
Copy link
Member

zivkovicmilos commented May 15, 2024

@r3v4s
Just curious, to narrow down the problem - did you do these simulations on a fresh chain every time (the deployment is the single transaction), or on the same chain (you've tested out all deployments on a single chain together)?

@r3v4s
Copy link
Contributor Author

r3v4s commented May 15, 2024

@r3v4s Just curious, to narrow down the problem - did you do these simulations on a fresh chain every time (the deployment is the single transaction), or on the same chain (you've tested out all deployments on a single chain together)?

It's gnodev node and simulations are on same chain (not fresh).
Should I test it also on fresh chain?

@ltzmaxwell
Copy link
Contributor

ltzmaxwell commented May 16, 2024

just a share of my test result(every time on fresh node). curious what's happening, more dig needed.

the difference between the two is: 815,040

gnokey maketx addpkg -pkgdir ./gnoswap/uint256 -pkgpath gno.land/p/demo/u2 -broadcast=true -gas-fee 1ugnot -gas-wanted 20000000 1216 -remote localhost:26657
Enter password.

OK!
GAS WANTED: 20000000
GAS USED: 10627011
HEIGHT: 2
EVENTS: []

gnokey maketx addpkg -pkgdir ./gnoswap/uint256 -pkgpath gno.land/p/demo/what_a_long_pkg_path -broadcast=true -gas-fee 1ugnot -gas-wanted 20000000 1216 -remote localhost:26657
Enter password.

OK!
GAS WANTED: 20000000
GAS USED: 11442051
HEIGHT: 6
EVENTS: []

@ltzmaxwell
Copy link
Contributor

if run on non-fresh node, the gas of the second addpkg will be different:
$ gnokey maketx addpkg -pkgdir ./gnoswap/uint256 -pkgpath gno.land/p/demo/u2 -broadcast=true -gas-fee 1ugnot -gas-wanted 20000000 1216 -remote localhost:26657
Enter password.

OK!
GAS WANTED: 20000000
GAS USED: 10627011
HEIGHT: 2
EVENTS: []
(base)

$ gnokey maketx addpkg -pkgdir ./gnoswap/uint256 -pkgpath gno.land/p/demo/what_a_long_pkg_path -broadcast=true -gas-fee 1ugnot -gas-wanted 20000000 1216 -remote localhost:26657
Enter password.

OK!
GAS WANTED: 20000000
GAS USED: 11444646
HEIGHT: 4
EVENTS: []

@ltzmaxwell
Copy link
Contributor

ltzmaxwell commented May 17, 2024

In a simplified scenario to replicate the issue, found that the long package path is referenced multiple times when setting objects for storage. The resulting increase in gas consumption just corresponds to the length of the extended path multiplied by the number of repetitions, which aligns with expectations. (This behavior has been consistently observed on a fresh node.)

However, when executing on a non-fresh node, an additional 2595 gas is consistently incurred compared to running on a fresh node. This discrepancy persists across various packages, whether simple or complex. While this discrepancy is peculiar, it may not be the primary cause of the issue at hand.

Before delving deeper into investigation, it would be helpful to understand the exact amount of gas increases due to the extended package path. This information will provide a clearer baseline for further analysis. @r3v4s , can you provide an exact number? 🙏

store set, write byte, writeCostPerByte: 30, len(value) 201, value:
482047 /gno.DeclaredType�
482048 gno.land/p/demo/helloAnimal�
482049 /gno.StructType~
482050 gno.land/p/demo/hello
482051 name
482052 /gno.PrimitiveTyp
482053 age
482054 /gno.PrimitiveTyp@"
482055 gender
482056 /gno.PrimitiveTy e

store set, write byte, writeCostPerByte: 30, len(value) 236, value:
482053 /gno.DeclaredType�
482054 &gno.land/p/demo/what_a_very_long_helloAnimal�
482055 /gno.StructType�
482056 &gno.land/p/demo/what_a_very_long_hello
482057 name
482058 /gno.PrimitiveTyp
482059 age
482060 /gno.PrimitiveTyp@"
482061 gender
482062 /gno.PrimitiveTy e

@ltzmaxwell
Copy link
Contributor

Update: A simple solution could be to use a fixed-length pkgpath, such as gno.land/p/.....foo.Bar. However, I'm questioning whether this is worthwhile, as this issue isn't technically a bug.

@leohhhn
Copy link
Contributor

leohhhn commented Jun 26, 2024

@ltzmaxwell I'm not really sure that deploying to a longer path should take more gas. This would mean that over time, deployment cost would go up simply because the shorter paths are taken. Also, a user with a shorter username would not have to pay as much for as a user with a longer username for uploading the same code, which is unfair.

Is there any way we can not spend more gas only if a package path is longer?

@ltzmaxwell
Copy link
Contributor

ltzmaxwell commented Jul 1, 2024

I don't have a good solution for now, please pick it if you want, sorry for the delay. @leohhhn @r3v4s

@moul
Copy link
Member

moul commented Oct 15, 2024

It's likely a bug that we can fix. However, it may also be beneficial for security to encourage shorter names, as this improves readability when using or auditing the dApps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
Status: Backlog
Status: Backlog
Development

No branches or pull requests

6 participants