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

"Gas used" seems unrelated to the number of coins subtracted #1070

Open
jefft0 opened this issue Aug 25, 2023 · 3 comments · May be fixed by #3209
Open

"Gas used" seems unrelated to the number of coins subtracted #1070

jefft0 opened this issue Aug 25, 2023 · 3 comments · May be fixed by #3209
Assignees
Labels
investigating This behavior is still being tested out 📦 🤖 gnovm Issues or PRs gnovm related ❓ question Questions about Gno

Comments

@jefft0
Copy link
Contributor

jefft0 commented Aug 25, 2023

I use the CLI gnokey maketx call with -gas-fee 1ugnot to call a Realm function. The node returns ctx.GasMeter().GasConsumed() to the CLI which prints "GAS USED: 1333100".

However, I use gnokey query to query my account balance before and after calling the Realm function and it shows that 2000000 ugnot was actually subtracted. This is seems unrelated to the displayed "gas used", so I'm confused. I traced the node code and it calls DeductFees for the gas fee (1000000 ugnot) and calls SendCoins with 1000000 ugnot for the function call. This is the total of 2000000 ugnot which is subtracted from the account. But it doesn't make sense that the calls to subtract coins from the account seem to be unrelated to the calculation of "gas used".

(Issue #1067 may be related, but it doesn't talk about "gas used".)

@ajnavarro ajnavarro added ❓ question Questions about Gno 📦 🤖 gnovm Issues or PRs gnovm related investigating This behavior is still being tested out labels Aug 28, 2023
@piux2
Copy link
Contributor

piux2 commented Aug 30, 2023

Thank you, @jefft0, for the insightful findings. You're spot on. Gas usage is indeed calculated in multiple places

shore answer: gas meter to calculate the gas used for executing contracts yet to be completed.

| auth module | vm module |
---------------------------
[        store            ]
  • gas store, define the gas consumption at the store level. Calculation is based on the message byte, read, write and find operations against the store, check out tm2/pkg/store/gas/store.go and tm2/pkg/types/gas.go. Gas used is calculated automatically.

  • gas meter used by auth module. It calculates gas-used (tm2/pkg/sdk/auth/params.go, tm2/pkg/sdk/auth/ante.go ) to process and verify signatures.

  • gas meter used by vm module, which executes contract transaction, yet to be completed. Storing contract, and reading and persisting realm object already consumes gas through gas store operation, but CPU cycle, memory allocation and storage rent are not factored in yet.

  • The gas consumed above shows in cli client response as gas-used.

  • To do: configure and implement the gas meter based on VM's CPU cycle, Memory allocation and Storage rent for each Tx and Realm objects.

  • Bug: contract execution fee is added on top of the transaction fee. #1067 the issue mentioned is a hard coded contract fee taken from user's accounts, it is irrelevant to the gas consumption. once we properly implement the contract gas meter, we should consider remove it.

@moul moul moved this to 🏆 Needed for launch in 🚀 The Launch [DEPRECATED] Sep 5, 2023
@moul moul added this to the 🚀 main.gno.land milestone Sep 6, 2023
piux2 added a commit that referenced this issue Apr 25, 2024
<!-- please provide a detailed description of the changes made in this
pull request. -->

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [x] No breaking changes were made
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

Ref: #1070 #1067 #649 #1281 


## Summary

The current gno.land node, optimized for development purposes, has a
simplified verification process and gas meter implementation. To
transition the gno.land node to a production-ready state, it is
necessary to implement a comprehensive gas metering system that
accurately accounts for VM gas consumption. This includes refining the
gas fee structure to encompass all relevant costs, ensuring robust
transaction validation, and calculating gas consumption based on actual
computational load.

This PR aims to address these limitations by introducing a complete gas
meter and validation flow, laying the groundwork for further gas meter
profiling and configuration.


## Problem Definition

Current State and Limitations for Production:

- **VM Gas Consumption Not Accounted in Gas Meter:** The current gas
meter fails to calculate VM gas consumption, potentially allowing heavy
contract loads without corresponding gas meter deductions. A refined
system should measure and charge for VM gas usage accurately.

- **Gas Fee Structure:** Presently, the gas fee structure only includes
storage access, transaction size, and signature verification. VM gas
fees are levied as a separate, flat fee, which might lead to confusion
among users expecting the total fee to match the amount specified in the
'gas-fee' argument. For improved transparency and precision, the gas fee
structure should integrate all these aspects.

- **Transaction Validation:** The system currently validates basic
information for VM msg_addpkg and msg_call. However, gas consumption
cannot be determined before fully executing these messages against the
VM. Consequently, VM transactions are placed in the mempool and
propagated to other nodes, even if they may not meet the gas fee
requirements to execute these transactions. This undermines the purpose
of using gas fees to prevent VM spamming.


## Solution: ( Updated )
This is a high-level description of the implemented features:

~~Added an anteHandler in VM to monitor gas consumption~~
~~Implemented chained VM anteHandler in auth.anteHandler~~
- Consume gas to verify account, signature and tx size in CheckTx 
- Consume VM gas in DeliverTx 
- Accumulated VM CPU cycles, memory allocation, store access,
transaction size, and signature verification into a single gas meter.
- Enabled local node checks of VM resource usage. The VM message is only
aborted if it runs out of gas in basic CheckTx. However, the message is
still propagated to other nodes if execution fails to prevent censorship
- Introduced a structured format for logging gas consumption for
profiling and metrics.
- Introduced a gas factor linking gas to vm CPU cycles and memory
allocation to balance between vm gas consumption with the rest.



## Trade-offs and Future Optimization: ( Updated )
~~The current implementation processes messages against the VM to check
gas consumption in abci.CheckTx() before inclusion in the mempool and
propagation to other nodes.~~

~~Messages lacking sufficient gas-wanted will be dropped, preventing
abuse without adequate gas fees. However, the trade-off is that for each
message with enough gas, the VM executes the transaction twice: once in
CheckTx() and once in DeliverTx(). As these occur in separate execution
contexts and are not in synchronized sequence, the performance impact is
currently a secondary concern.~~

We moved the VM gas check from CheckTx to DeliverTx for the following
reasons:

- We only know the VM gas consumption after the messages have been
processed.
- Running VM execution for many CheckTx requests from the peers could
overload the mempool that is executing CheckTx.
- This could slow down the propagation of transactions across the entire
network.

By moving the VM gas check from CheckTx to DeliverTx, we are able to
reduce the load on the mempool of a node and allow transactions to
propagate through the network faster.

In the future, we may use a predicted median value instead of the exact
value from transaction execution for efficiency.

## What's Next:
- Add a minimum gas price flag and configuration for node operation.
- Provide a user-friendly fee input interface, offering 'gas-wanted' and
'gas price' as alternatives to the current 'gas-wanted' and 'gas-fee'
inputs.
- Tune the gas factor based on VM CPU and Memory Profiling. The current
factor is 1:1 between gas and VM CPU cycles and memory allocation.

---------

Co-authored-by: Thomas Bruyelle <[email protected]>
@jefft0 jefft0 changed the title "Gas used" is different than number of coins subtracted "Gas used" seems unrelated to the number of coins subtracted Aug 28, 2024
@jefft0
Copy link
Contributor Author

jefft0 commented Aug 28, 2024

Update: PR #1430 did some work to improve gas calculation, but the reported "gas used" still seems unrelated to the number of coins subtracted.

Still a problem on test4. The command below uses a gas fee of 1 ugnot. It prints "GAS USED: 1091252". But only 1 ugnot is subtracted from the account.

gnokey maketx call -pkgpath "gno.land/r/demo/boards" -func "CreateReply" -gas-fee 1ugnot -gas-wanted 2000000 -send "" -broadcast -chainid "test4" -args "1" -args "1" -args "1" -args "reply from Jeff" -remote "https://rpc.test4.gno.land" jefft0

@Kouteki Kouteki removed this from the 🚀 Mainnet launch milestone Oct 16, 2024
@onlyhyde onlyhyde linked a pull request Nov 26, 2024 that will close this issue
6 tasks
@dongwon8247 dongwon8247 moved this to In Progress in 🤝🏻 Partner: Onbloc Nov 27, 2024
@piux2
Copy link
Contributor

piux2 commented Nov 27, 2024

This is because there is no minimum gas prices set by validators, and the global gas prices is not defined.
The global gas prices is addressed in PR2828

The minimum gas fee can not be set at the validator level, we need to enable it at the command line. we can track it here
#3223

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigating This behavior is still being tested out 📦 🤖 gnovm Issues or PRs gnovm related ❓ question Questions about Gno
Projects
Status: 🚀 Needed for Launch
Status: In Progress
Status: Triage
Development

Successfully merging a pull request may close this issue.

7 participants