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(ante) add block gas limit check for cosmos txs #1435

Merged
merged 2 commits into from
Feb 28, 2023
Merged

Conversation

GAtom22
Copy link
Contributor

@GAtom22 GAtom22 commented Feb 27, 2023

@linear
Copy link

linear bot commented Feb 27, 2023

ENG-1563 Tx timeout error

When exceeding block max gas, the CLI returns a tx timeout error:

Error: RPC error -32603 - Internal error: timed out waiting for tx to be included in a block

Steps to reproduce:

  • submit a tx using --gas=500000000

To inform the user about this situation, the error message should provide more information rather than the timeout error.

@GAtom22 GAtom22 marked this pull request as ready for review February 27, 2023 17:40
@GAtom22 GAtom22 requested a review from a team as a code owner February 27, 2023 17:40
@GAtom22 GAtom22 requested review from Vvaradinov, MalteHerrmann, facs95 and fedekunze and removed request for a team February 27, 2023 17:40
@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #1435 (b190497) into main (b5f2f26) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1435      +/-   ##
==========================================
+ Coverage   72.70%   72.72%   +0.01%     
==========================================
  Files         265      265              
  Lines       18209    18219      +10     
==========================================
+ Hits        13239    13249      +10     
  Misses       4380     4380              
  Partials      590      590              
Impacted Files Coverage Δ
app/ante/evm/fee_market.go 83.33% <100.00%> (+6.41%) ⬆️

@facs95
Copy link
Contributor

facs95 commented Feb 27, 2023

Since this decorator is used in all the ante handlers can you remove this check https://github.com/evmos/evmos/blob/main/app/ante/evm/eth.go#L222 ? I dont think its needed anymore

Copy link
Contributor

@facs95 facs95 left a comment

Choose a reason for hiding this comment

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

LGTM Besides my previous comment

@GAtom22
Copy link
Contributor Author

GAtom22 commented Feb 28, 2023

Since this decorator is used in all the ante handlers can you remove this check https://github.com/evmos/evmos/blob/main/app/ante/evm/eth.go#L222 ? I dont think its needed anymore

@facs95 But what about this logic? we should set the gasWanted in the tx Gas that is passed over the next decorator, right?

@fedekunze fedekunze merged commit e254766 into main Feb 28, 2023
@fedekunze fedekunze deleted the GAtom22/block-gas branch February 28, 2023 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants