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: Longer package Path Spend More Gas [investigate] #2736

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

notJoon
Copy link
Member

@notJoon notJoon commented Aug 28, 2024

Description

This PR address the process of identifying and resolviong the cause of gas usage variations during deployment based on the length of package path.

Testing Process

To identify the cause, I conducted tests using txtar testscript. I used the following script, chainging only the length of the package path for check.

# script file path:  gno.land/cmd/gnoland/testdata/addpkg_long_pkg_path.txtar

loadpkg gno.land/p/demo/ufmt

# start a new node
gnoland start

gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/p/demo/foo/aaa -gas-fee 1ugnot -gas-wanted 1430000 -broadcast -chainid=tendermint_test test1
#stdout OK!

-- foo.gno --
package aaa

import (
	"gno.land/p/demo/ufmt"
)

func Foo() string {
	return ufmt.Sprintf("foo")
}
cd gno.land/cmd/gnoland
go test -v . -run Testdata/addpkg_long

Even when an out of gas error occured, the only function visible in the stack trace was the ExecSignAndBroadcast function in maketx.go file. Therefore, I added additional debugging lines to trace which function were being called.

Cause Identification

While running the test script, I discovered that the NewAnteHandler function was being called repeatedly. This function contains the following line, where TxSizeCostPerByte is set a default value of 10:

func NewAnteHandler(ak AccountKeeper, bank BankKeeperI, sigGasConsumer SignatureVerificationGasConsumer, opts AnteOptions) sdk.AnteHandler {
    // ... 
    newCtx.GasMeter().ConsumeGas(params.TxSizeCostPerByte*store.Gas(len(newCtx.TxBytes())), "txSize")
    // ...
}

I added a debugging line like below:

func NewAnteHandler(ak AccountKeeper, bank BankKeeperI, sigGasConsumer SignatureVerificationGasConsumer, opts AnteOptions) sdk.AnteHandler {
    // ... 
    newCtx.GasMeter().ConsumeGas(params.TxSizeCostPerByte*store.Gas(len(newCtx.TxBytes())), "txSize")
+   println("xxxxxxxx ante gas consumed", newCtx.GasMeter().GasConsumed())
    // ...
}

Output

go test -v . -run Testdata/addpkg_long

=== RUN   TestTestdata
=== RUN   TestTestdata/addpkg_long_pkg_path
xxxxxxxx ante gas consumed 0
xxxxxxxx ante gas consumed 3500
xxxxxxxx ante gas consumed 3500
xxxxxxxx ante gas consumed 3500
    testscript.go:558: WORK=$WORK
        PATH=/Users/not_joon/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.4.darwin-arm64/bin:/opt/anaconda3/envs/py10/bin:/opt/anaconda3/condabin:/Users/not_joon/go/bin:/Users/not_joon/.cargo/bin:/opt/anaconda3/envs/py10/bin:/opt/anaconda3/condabin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/usr/local/share/dotnet:~/.dotnet/tools:/usr/local/go/bin:/opt/anaconda3/envs/py10/bin:/opt/anaconda3/condabin:/Users/not_joon/Library/Application Support/Coursier/bin:/bin
        GOTRACEBACK=system
        HOME=/no-home
        TMPDIR=$WORK/.tmp
        devnull=/dev/null
        /=/
        :=:
        $=$
        exe=
        SID=4dc452c5
        USER_SEED_test1=source bonus chronic canvas draft south burst lottery vacant surface solve popular case indicate oppose farm nothing bullet exhibit title speed wink action roast
        USER_ADDR_test1=g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5
        GNOROOT=/Users/not_joon/gno-core
        GNOHOME=/var/folders/kl/ny3lwtcn4gqdcwx0mz2lk4z80000gn/T/TestTestdata418531309/001/gno
        
        > loadpkg gno.land/p/demo/ufmt
        "gno.land/p/demo/ufmt" package was added to genesis
        # start a new node (1.550s)
        > gnoland start
        [stdout]
        node started successfully
        
        > gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/p/demo/foo/aaa -gas-fee 1ugnot -gas-wanted 1430000 -broadcast -chainid=tendermint_test test1
        [stdout]
        
        OK!
        GAS WANTED: 1430000
        GAS USED:   1425960
        HEIGHT:     130
        EVENTS:     []
        TX HASH:    /QBcHVwP3Svird/uTUtXTsYxqBTSwHO0JZBQxKqQPCI=
        
        [stderr]
        Enter password.
        
        #stdout OK! (0.000s)
        PASS
        
--- PASS: TestTestdata (1.89s)
    --- PASS: TestTestdata/addpkg_long_pkg_path (1.88s)
PASS
ok      github.com/gnolang/gno/gno.land/cmd/gnoland 

I executed the script while chainging the pacakge path to make have different lengths and confirmed that the gas amount changed according to the length of TxBytes() as follows:

Case 1: Package path length 22 (gno.land/p/demo/foo/aa)

TxSizeCosePerByte Consumption at NewAnteHandler call Total gas consumption Increase
0 0 1422130 0
1 347 1422477 +347
10 (default value) 3470 1425600 +3470

Case 2: Package path length 23 (gno.land/p/demo/foo/aaa)

TxSizeCosePerByte Consumption at NewAnteHandler call Total gas consumption Increase
0 0 1422460 0
1 350 1422810 +350
10 (default value) 3500 1425960 +3500

We can observe that as the length of the package path increases, so does the gas usage. However, since gas usage is calculated based on the length of the transaction data obtained through the TxBytes method, we can hypothesize that it is also affected by the length of the contract content, not only the package path.

[TODO]


Related Issue

#2083

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@github-actions github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Aug 28, 2024
@notJoon notJoon changed the title [investigate] Longer package Path Spend More Gas fix: Longer package Path Spend More Gas [investigate] Aug 28, 2024
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.22%. Comparing base (aae5d49) to head (64f9367).
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2736      +/-   ##
==========================================
- Coverage   60.23%   60.22%   -0.01%     
==========================================
  Files         562      562              
  Lines       75091    75043      -48     
==========================================
- Hits        45228    45196      -32     
+ Misses      26482    26473       -9     
+ Partials     3381     3374       -7     
Flag Coverage Δ
contribs/gnodev 61.40% <ø> (ø)
contribs/gnofaucet 15.31% <ø> (+0.85%) ⬆️
gno.land 64.57% <ø> (-0.19%) ⬇️
gnovm 64.33% <ø> (ø)
misc/genstd 80.54% <ø> (ø)
misc/logos 20.23% <ø> (ø)
tm2 62.06% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zivkovicmilos
Copy link
Member

Pinging @piux2 @deelawn for visibility

@deelawn
Copy link
Contributor

deelawn commented Sep 5, 2024

@moul @leohhhn Did we reach any decision on this in the call earlier today? Will we allow these kind of gas consumption increases in favor of incentivizing users to use shorter package paths? Or does there need to be more research done?

@leohhhn
Copy link
Contributor

leohhhn commented Sep 5, 2024

No decision was reached yesterday. I still think making people pay more simply because of the longer username is unfair, but I do agree that shorter paths are more readable.

Maybe a good compromise would be to only count gas for the part after gno.land/{p,r}/namespace. Anything before is ignored for gas, anything after is the user's choice and we can still incentivize short paths. WDYT?

@notJoon
Copy link
Member Author

notJoon commented Sep 5, 2024

@leohhhn I also agree with your opinion. maybe related with #2727

I also considering replacing the raw package path with an address value to ensure that data od the same length is always inserted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: No status
Status: Triage
Development

Successfully merging this pull request may close these issues.

4 participants