Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

fix(ante): fix reCheckTx gas wanted #1566

Merged
merged 3 commits into from
Dec 22, 2022
Merged

fix(ante): fix reCheckTx gas wanted #1566

merged 3 commits into from
Dec 22, 2022

Conversation

GAtom22
Copy link
Contributor

@GAtom22 GAtom22 commented Dec 21, 2022

Closes:

Description

Set gasWanted = 0 for Txs in ReCheckMode at the EthGasConsumeDecorator AnteHandler. Otherwise, Tendermint logs this error:

"gas wanted -1 is negative"

However, this error is not bubbled up and the Tx never runs on DeliverMode.

For more details, check the related issue's comments.


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@GAtom22 GAtom22 marked this pull request as ready for review December 21, 2022 14:28
@GAtom22 GAtom22 requested a review from a team as a code owner December 21, 2022 14:28
@GAtom22 GAtom22 requested review from facs95 and adisaran64 and removed request for a team December 21, 2022 14:28
@GAtom22 GAtom22 changed the title fix(abci): fix reCheckTx gas wanted fix(ante): fix reCheckTx gas wanted Dec 21, 2022
@codecov
Copy link

codecov bot commented Dec 21, 2022

Codecov Report

Merging #1566 (1692948) into main (0e366db) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 1692948 differs from pull request most recent head b2e96e3. Consider uploading reports for the commit b2e96e3 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1566      +/-   ##
==========================================
+ Coverage   68.63%   68.65%   +0.02%     
==========================================
  Files         103      103              
  Lines        9946     9953       +7     
==========================================
+ Hits         6826     6833       +7     
  Misses       2737     2737              
  Partials      383      383              
Impacted Files Coverage Δ
app/ante/eth.go 80.00% <100.00%> (+0.64%) ⬆️

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

utACK. Is it possible to add a test or is this something that we need to flag on Tendermint?

Could you add a changelog entry?

@GAtom22
Copy link
Contributor Author

GAtom22 commented Dec 21, 2022

utACK. Is it possible to add a test or is this something that we need to flag on Tendermint?

@fedekunze It's a little tricky to test it.. I'll try to write a test for this case.

IMO this error should be returned.. Maybe the fix would be to add it to the MempoolError field of the abci.ResponseCheckTx on the handleRecheckResult func. This is done on the addNewTransaction func (haven't tested if this solves the issue tho)

@fedekunze fedekunze enabled auto-merge (squash) December 22, 2022 08:01
@fedekunze fedekunze merged commit bd03197 into main Dec 22, 2022
@fedekunze fedekunze deleted the GAtom22/fix-recheck-tx branch December 22, 2022 08:07
MalteHerrmann pushed a commit that referenced this pull request Dec 22, 2022
* fix(abci): fix reCheckTx gas wanted'

* fix(ante): add changelog entry
@MalteHerrmann MalteHerrmann mentioned this pull request Dec 22, 2022
fedekunze added a commit that referenced this pull request Dec 22, 2022
* build(deps): bump github.com/cosmos/cosmos-sdk from 0.46.6 to 0.46.7 (#1551)

Bumps [github.com/cosmos/cosmos-sdk](https://github.com/cosmos/cosmos-sdk) from 0.46.6 to 0.46.7.
- [Release notes](https://github.com/cosmos/cosmos-sdk/releases)
- [Changelog](https://github.com/cosmos/cosmos-sdk/blob/main/CHANGELOG.md)
- [Commits](cosmos/cosmos-sdk@v0.46.6...v0.46.7)

---
updated-dependencies:
- dependency-name: github.com/cosmos/cosmos-sdk
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump github.com/cosmos/ibc-go/v5 from 5.1.0 to 5.2.0 (#1564)

Bumps [github.com/cosmos/ibc-go/v5](https://github.com/cosmos/ibc-go) from 5.1.0 to 5.2.0.
- [Release notes](https://github.com/cosmos/ibc-go/releases)
- [Changelog](https://github.com/cosmos/ibc-go/blob/v5.2.0/CHANGELOG.md)
- [Commits](cosmos/ibc-go@v5.1.0...v5.2.0)

---
updated-dependencies:
- dependency-name: github.com/cosmos/ibc-go/v5
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* make missing key error message during SendTransaction more verbose (#1563)

Co-authored-by: 4rgon4ut <[email protected]>

* debug(app): add flag to disable optimized build for remote debugging (#1549)

Co-authored-by: MalteHerrmann <[email protected]>
Co-authored-by: Federico Kunze Küllmer <[email protected]>

* Problem: personal_newAccount don't work (#1561)

fix the internal parameter.

* fix(ante): fix reCheckTx gas wanted (#1566)

* fix(abci): fix reCheckTx gas wanted'

* fix(ante): add changelog entry

* fix(cli): fix Ledger signature algorithm verification (#1550)

* fix: update Ledger default algorithm to `EthSecp256k1`

* fix ledger signing algo validation

* changelog

Co-authored-by: Freddy Caceres <[email protected]>
Co-authored-by: Federico Kunze Küllmer <[email protected]>

* update changelog

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: 4rgon4ut <[email protected]>
Co-authored-by: Tomas Guerra <[email protected]>
Co-authored-by: Federico Kunze Küllmer <[email protected]>
Co-authored-by: yihuang <[email protected]>
Co-authored-by: Austin Chandra <[email protected]>
Co-authored-by: Freddy Caceres <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants