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(tm2): deduct gas fee after runTx #3209

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

onlyhyde
Copy link

@onlyhyde onlyhyde commented Nov 26, 2024

resolve #1070 #1067

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

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

codecov bot commented Nov 26, 2024

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
@onlyhyde onlyhyde marked this pull request as ready for review November 27, 2024 13:28
@notJoon notJoon added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Nov 27, 2024
Copy link
Member

@notJoon notJoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

remove: review/triage-pending flag

@notJoon notJoon removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Nov 27, 2024
@Kouteki Kouteki added the in focus Core team is prioritizing this work label Nov 28, 2024
- as issue that is state already updated even deduct gas fee is failed
- change deduct gas fee timing to before state update
@onlyhyde
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in focus Core team is prioritizing this work 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: In Review
Status: In Review
Development

Successfully merging this pull request may close these issues.

"Gas used" seems unrelated to the number of coins subtracted
3 participants