Bug: contract execution fee is added on top of the transaction fee. #1067
Labels
🐞 bug
Something isn't working
Milestone
Comments
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 <thomas.bruyelle@gmail.com>
|
Hello is this still an issue ? What I'm not sure is about why the gas used is not deducted, maybe the issue is mostly there ? miguel@bb090aidemiguel gno.land % gnokey query bank/balances/g14vxq5e5pt5sev7rkz2ej438scmxtylnzv5vnkw
height: 0
data: "10000000000000ugnot"
miguel@bb090aidemiguel gno.land % make add_gh_verify_realm
gnokey maketx addpkg \
-deposit="1ugnot" \
-gas-fee="999ugnot" \
-gas-wanted="50000000" \
-broadcast="true" \
-chainid="dev" \
-remote="http://127.0.0.1:26657" \
-pkgdir="./r/gnoland/ghverify" \
-pkgpath="gno.land/r/villaquiranm/ghverify" \
MyKey
Enter password.
OK!
GAS WANTED: 50000000
GAS USED: 9501910
HEIGHT: 3
EVENTS: []
TX HASH: zGGi+B0q37JMk/ksXZ9AsPJ88uyehFiXnCGPRvRjfLI=
miguel@bb090aidemiguel gno.land % gnokey query bank/balances/g14vxq5e5pt5sev7rkz2ej438scmxtylnzv5vnkw
height: 0
data: "9999999999000ugnot"note: 1 ugnot as deposit and 999 as gas-fee miguel@bb090aidemiguel gno.land % make request_verification
gnokey maketx call \
-pkgpath "gno.land/r/villaquiranm/ghverify" \
-func="RequestVerification" \
-gas-fee="1000ugnot" \
-gas-wanted="3000000" \
-remote="http://127.0.0.1:26657" \
-chainid="dev" \
-broadcast \
-args "villaquiranm" \
MyKey
Enter password.
OK!
GAS WANTED: 3000000
GAS USED: 840616
HEIGHT: 5
EVENTS: []
TX HASH: O5mkntaZFen8SrYpJGf95TsUKx0Jxia6AxSuz9VnYp0=
miguel@bb090aidemiguel gno.land % gnokey query bank/balances/g14vxq5e5pt5sev7rkz2ej438scmxtylnzv5vnkw
height: 0
data: "9999999998000ugnot" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
AddPackage and Call contract fees are not deducted from tx fees that users pay, in stead, it adds on the top
gnokey maketx call
:
:
-gas-fee 1500000ugnot
:
Expected:
deduct 1500000ugnot (1.5gnot) from the user account
Actual:
deduct 2500000ugnot (2.5gnot) from the user account
The issue:
The gas fee here is used to pay to process the transaction on gno.land at the base protocol layer. The VM Keeper added a contract fee (1gnot=1000000ugnot) on top of the fee that user specified.
related, need to use gas meter to calculated min gas price = gas fee / (gas used on base protocol tx + gas used on contract). the min price gas should be used to check against the settings by the validator
#649
The text was updated successfully, but these errors were encountered: