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: exempt system txs from min gas check and fee deduction #1605

Merged
merged 19 commits into from
Jan 22, 2024

Conversation

brewmaster012
Copy link
Collaborator

@brewmaster012 brewmaster012 commented Jan 20, 2024

Descriptions

The following System txs types from observers are now exempt from minimum gas price check
and gas fee deduction in antehandler. This practically means that system transactions carry less effective gas price than regular txs. They still consume gas units.

MsgGasPriceVoter
MsgVoteOnObservedInboundTx
MsgVoteOnObservedOutboundTx
MsgAddToOutTxTracker
MsgCreateTSSVoter
MsgAddBlockHeader
MsgAddBlameVote

Tested in smoketest, by setting gas price in broadcast.go to %1.1 of current base price. Smoketest passed despite minimum gas price set
to 1GWEI. No negative tests conducted.

Closes:

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Checklist:

  • I have added unit tests that prove my fix feature works

in antehandlers. System txs are 7 zetaclient txs from observers.
app/ante/ante.go Outdated Show resolved Hide resolved
@kingpinXD
Copy link
Contributor

Hi @brewmaster012 I am already working on a branch for this
antehandler-system-tx

Overall I think instead of making it free we can adjust the gas price based on tx type , so that system tx cost similar to a standard tx . I am using 21000 * CurrentGasPrice as the standard fee , but it can be easily adjusted .
The same gas price adjuster can be called by both the client (to calculate the gas price used in broadcast )and antehandler to calculate the fees

Checking for observers would still open it up to a dos attack from one of our observers if they try to .

@brewmaster012
Copy link
Collaborator Author

brewmaster012 commented Jan 20, 2024

Hi @brewmaster012 I am already working on a branch for this antehandler-system-tx

I looked at it, and we need to pick one to work on and get deployed quickly.
In this PR there are several changes that needs to be done regardless of which one to pick:

  1. reverting back to the stock ante.NewSetUpContextDecorator()
  2. should handle both MsgExec wrapped/not wrapped system txs

I think this PR is a better base, would you like to work on it together?

Overall I think instead of making it free we can adjust the gas price based on tx type , so that system tx cost similar to a standard tx . I am using 21000 * CurrentGasPrice as the standard fee , but it can be easily adjusted . The same gas price adjuster can be called by both the client (to calculate the gas price used in broadcast )and antehandler to calculate the fees

Checking for observers would still open it up to a dos attack from one of our observers if they try to .

Yes: how about this:

allow the system txs to pass with 1% of min gas price. (this roughly fixes the discrapency between cosmos txs vs ethereum txs gas meter for our system txs).

1% reflects two things:

  1. system txs costs about 10x more gas units than user txs
  2. if min gas price is deterrence to spam, then i'm more concerned about unprivileged spam than privileged spam (observer) by a factor of at least 10x.

@kingpinXD
Copy link
Contributor

llow the system txs to pass with 1% of min gas price. (this roughly fixes the discrapency between cosmos txs vs ethereum txs gas meter for our system txs).

That makes sense. I can work on this branch as well;
Applying the same adjustment across all systems tx's can make some TXs substantially cheaper than others .

Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

  • We need to add unit tests for the added functionality.

I think we can implement a separate function taking the message and isAuthorized as an argument and returning a bool if the other anteHandler should be used.
And we can add unit test for this function.

app/ante/ante.go Outdated Show resolved Hide resolved
app/ante/ante.go Outdated Show resolved Hide resolved
app/ante/ante.go Outdated Show resolved Hide resolved
app/ante/ante.go Outdated Show resolved Hide resolved
app/ante/ante.go Outdated Show resolved Hide resolved
zetaclient/broadcast.go Outdated Show resolved Hide resolved
app/ante/fees.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kingpinXD kingpinXD left a comment

Choose a reason for hiding this comment

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

This looks like great start ,

I do think , if we can take some and implement a function , which ,adjusts the gas prices based on the gas_limit , when the tx matches the following condition

  • IsAuthorized
  • IsSystemTx
    would be good .

Copy link

!!!WARNING!!!
nosec detected in the following files: zetaclient/broadcast.go

Be very careful about using #nosec in code. It can be a quick way to suppress security warnings and move forward with development, it should be employed with caution. Suppressing warnings with #nosec can hide potentially serious vulnerabilities. Only use #nosec when you're absolutely certain that the security issue is either a false positive or has been mitigated in another way.

Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203
Broad #nosec annotations should be avoided, as they can hide other vulnerabilities. The CI will block you from merging this PR until you remove #nosec annotations that do not target specific rules.

Pay extra attention to the way #nosec is being used in the files listed above.

@github-actions github-actions bot added the nosec label Jan 21, 2024
@brewmaster012
Copy link
Collaborator Author

  • We need to add unit tests for the added functionality.

I think we can implement a separate function taking the message and isAuthorized as an argument and returning a bool if the other anteHandler should be used. And we can add unit test for this function.

Factored out

@brewmaster012 brewmaster012 merged commit dfc2314 into develop Jan 22, 2024
21 checks passed
@brewmaster012 brewmaster012 deleted the exempt-system-tx-min-gas branch January 22, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants