fix(tm2): deduct gas fee after runTx #3209

onlyhyde · 2024-11-26T14:47:59Z

This PR changes when and how gas fees are collected during transaction processing.
BREAKING CHANGE: need to modify the gas-fee value in your testcode to avoid the insufficient fund failure

As-Is

When : Collect the gas fee before the AnteHandler validates the signature of the transaction.
How: Sends the quantity of gas-fee that is set in the transaction from the originCaller's account to the `FeeCollectorAddress.

To-Be

When : Collect the gas fee immediately after the runTx function execution is terminated in the DeliverTx function (the final gas quantity used to process the Transaction is known at the end of the runTx function).
How : Sends the amount of gas-fee set in the transaction multiplied by the gas used from the originCaller's account to the FeeCollectorAddress.

Implementation

Since the AnteHandler already does the work for gas and auth (https://github.com/gnolang/gno/blob/master/tm2/pkg/sdk/baseapp.go#L40), the GasFeeCollector's New function is implemented in ante.go to maintain that orientation.
The GasFeeCollector function is called by the BaseApp, so in app.go, we set the GasFeeCollector function to BaseApp.
Through the bank module, the code that sends the gas fees to the FeeCollectorAddress reuses an existing function (DeductFees).

Concerns

After measuring the actual gas used, the calculated value with gas-fee is collected, and if there are not enough assets in the account at this time, a policy for handling the result of the transaction seems necessary.
- Example) If you set 'gas-fee : 10ugnot' in the transaction, and the gas used is 10,000, the account balance must be 100,000 ugnot to submit the gas fee. If there are insufficient funds, return InsufficientFundsError. If an error is returned, the fee will not be collected, but since the transaction processing was done using a vm in the runTx function, it looks like the default fee may be required.

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

codecov · 2024-11-26T15:07:24Z

Codecov Report

Attention: Patch coverage is 82.22222% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
tm2/pkg/sdk/baseapp.go	22.22%	6 Missing and 1 partial ⚠️
tm2/pkg/sdk/options.go	75.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

- when gasFeeCollector return error, both Error and Log of result set to res - If gas-fee amount is zero, return error with ErrInsufficientFee

Testcase includes error case and success case - Error Case 1. If UnknownAddress, return error with UnknownAddressError msg 2. If Invalid gas fee, return error with InsufficientFeeError msg 3. If not enough funds, return error with InsufficientFundsError msg 4. If fee denom are invalid, panic - Success Case 1. If success, gasFeeCollector return error with nil and result IsOK, and FeeCollectorAddress get (used gas * fee.Amount) from account balance

notJoon

In the meeting, I received confirmation that the concerning issues cannot occur. LGTM

remove: review/triage-pending flag

- as issue that is state already updated even deduct gas fee is failed - change deduct gas fee timing to before state update

onlyhyde · 2024-11-28T03:31:28Z

Update as new issues are discovered.

Issue:

After processing a trasaction, if there are not enough assets for the gas fee, it is treated as a failure, but the status change is applied.

Cause:

After executing the runMsgs function in the runTx function, msCache.MultiWrite() updates the state if there is no failure in the result.
The gas fee collection is processed afterward, returning a failure, but the state remains updated.

Fix:

Change to collect the gas fee before updating the status.
If an error occurs while collecting the gas fee, return an error.
If you're processing a genesis block, add a condition to not collect the gas fee.

Gno2D2 · 2024-12-04T15:43:25Z

🛠 PR Checks Summary

🔴 Maintainers must be able to edit this pull request (more info)

Manual Checks (for Reviewers):

IGNORE the bot requirements for this PR (force green CI check)
The pull request description provides enough details

✅ Automated Checks (for Contributors):

🔴 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:

Fix any issues flagged by automated checks.
Follow the Contributor Checklist to ensure your PR is ready for review.
- Add new tests, or document why they are unnecessary.
- Provide clear examples/screenshots, if necessary.
- Update documentation, if required.
- Ensure no breaking changes, or include BREAKING CHANGE notes.
- Link related issues/PRs, where applicable.

☑️ Reviewer Actions:

Complete manual checks for the PR, including the guidelines and additional checks if applicable.

📚 Resources:

Debug

Automated Checks
Maintainers must be able to edit this pull request (more info)
If
🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: gnoswap-labs/gno)
Then
🔴 Requirement not satisfied
└── 🔴 Maintainer can modify this pull request
Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)
If
🟢 Condition met
└── 🟢 On every pull request
Can be checked by

Any user with comment edit permission
The pull request description provides enough details
If
🟢 Condition met
└── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)
Can be checked by

team core-contributors

piux2 · 2025-01-08T07:40:35Z

@onlyhyde Thank you so much for your contribution!

The dynamic gas price implementation is available at:
#2838

It addresses the following issues:

#1070
#1067
Please let me know if you think differently.

As mentioned in this comment:
#3209 (comment)
could you open a new issue if the problem still exists? Additionally, could you share the test cases and the results?

piux2 · 2025-01-08T17:55:20Z

@onlyhyde

We deduct the GasFee upfront in the AnteHandler, not after runTx

One purpose of charging the gas fee is to prevent abuse, as there is a cost associated with executing a transaction even if it fails. If we were to charge the fee only after the transaction has already been executed, it would defeat this purpose.

fix: deduct gas fee after runTx

72a94d0

github-actions bot assigned onlyhyde Nov 26, 2024

github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Nov 26, 2024

test: fix txtar testing

3d94e89

dongwon8247 mentioned this pull request Nov 27, 2024

Onbloc's Builder Journey gnolang/hackerspace#29

Open

onlyhyde added 5 commits November 27, 2024 16:06

fix: process error case

7bb4117

- when gasFeeCollector return error, both Error and Log of result set to res - If gas-fee amount is zero, return error with ErrInsufficientFee

fix: lint error and test code error in git action ci

d563541

fix: test code error in git action ci

15936e3

test: add test case

9b34a60

onlyhyde marked this pull request as ready for review November 27, 2024 13:28

notJoon added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Nov 27, 2024

zivkovicmilos requested review from zivkovicmilos and piux2 November 27, 2024 17:06

notJoon approved these changes Nov 27, 2024

View reviewed changes

notJoon removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Nov 27, 2024

Kouteki added the in focus Core team is prioritizing this work label Nov 28, 2024

fix : change deduct gas fee timing before state update

0dbc313

- as issue that is state already updated even deduct gas fee is failed - change deduct gas fee timing to before state update

onlyhyde requested a review from notJoon November 28, 2024 03:32

zivkovicmilos added this to the 🚀 Mainnet beta launch milestone Jan 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tm2): deduct gas fee after runTx #3209

fix(tm2): deduct gas fee after runTx #3209

onlyhyde commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

notJoon left a comment

onlyhyde commented Nov 28, 2024

Gno2D2 commented Dec 4, 2024 •

edited

Loading

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Can be checked by

If

Can be checked by

piux2 commented Jan 8, 2025

piux2 commented Jan 8, 2025

fix(tm2): deduct gas fee after runTx #3209

Are you sure you want to change the base?

fix(tm2): deduct gas fee after runTx #3209

Conversation

onlyhyde commented Nov 26, 2024 • edited Loading

As-Is

To-Be

Implementation

Concerns

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

notJoon left a comment

Choose a reason for hiding this comment

onlyhyde commented Nov 28, 2024

Issue:

Cause:

Fix:

Gno2D2 commented Dec 4, 2024 • edited Loading

🛠 PR Checks Summary

Manual Checks (for Reviewers):

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Can be checked by

If

Can be checked by

piux2 commented Jan 8, 2025

piux2 commented Jan 8, 2025

onlyhyde commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

Gno2D2 commented Dec 4, 2024 •

edited

Loading