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

Fix panic when transaction is reverted #650

Merged
merged 4 commits into from
Oct 8, 2021

Conversation

thomas-nguy
Copy link
Contributor

@thomas-nguy thomas-nguy commented Oct 8, 2021

Description

Current behavior will panic when flatten the stack of cache in case transaction is reverted because the cache context is already flattened


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)

@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #650 (917674d) into main (5169721) will decrease coverage by 4.63%.
The diff coverage is 42.30%.

❗ Current head 917674d differs from pull request most recent head a678cad. Consider uploading reports for the commit a678cad to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #650      +/-   ##
==========================================
- Coverage   56.01%   51.37%   -4.64%     
==========================================
  Files          63       63              
  Lines        5502     5518      +16     
==========================================
- Hits         3082     2835     -247     
- Misses       2257     2515     +258     
- Partials      163      168       +5     
Impacted Files Coverage Δ
x/evm/keeper/context_stack.go 56.60% <0.00%> (-10.07%) ⬇️
x/evm/keeper/keeper.go 75.14% <ø> (ø)
x/evm/keeper/state_transition.go 67.18% <55.00%> (-1.12%) ⬇️
x/evm/types/dynamic_fee_tx.go 0.00% <0.00%> (-89.68%) ⬇️
x/evm/types/access_list.go 23.33% <0.00%> (-76.67%) ⬇️
x/evm/types/tracer.go 0.00% <0.00%> (-46.43%) ⬇️
x/evm/types/params.go 67.64% <0.00%> (-32.36%) ⬇️
x/evm/types/legacy_tx.go 73.21% <0.00%> (-26.79%) ⬇️
x/evm/types/genesis.go 85.71% <0.00%> (-14.29%) ⬇️
... and 4 more

@thomas-nguy thomas-nguy force-pushed the thomas/fix-bug-reverted branch from 518a446 to ae54988 Compare October 8, 2021 08:26
@yijiasu-crypto yijiasu-crypto mentioned this pull request Oct 8, 2021
11 tasks
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. Added additional comments for more context. Can you add a bug fix changelog entry?

Also, the k.ctxStack.CommitToRevision(revision) case is not currently covered by tests> Ideally, we should open a ticket to add a test + benchmark

x/evm/keeper/context_stack.go Outdated Show resolved Hide resolved
x/evm/keeper/state_transition.go Show resolved Hide resolved
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
x/evm/keeper/state_transition.go Show resolved Hide resolved
x/evm/keeper/state_transition.go Show resolved Hide resolved
@thomas-nguy thomas-nguy force-pushed the thomas/fix-bug-reverted branch from ae54988 to ecf8382 Compare October 8, 2021 11:01
@fedekunze fedekunze enabled auto-merge (squash) October 8, 2021 11:18
@fedekunze fedekunze merged commit fe5fefb into evmos:main Oct 8, 2021
fedekunze added a commit that referenced this pull request Oct 8, 2021
* fix panic when transaction is reverted

* update changelog

* Update x/evm/keeper/context_stack.go

Co-authored-by: Federico Kunze Küllmer <[email protected]>
fedekunze added a commit that referenced this pull request Oct 8, 2021
* ci: add bencher config (#652)

Add bencher config with global +-10% threshold for improvements and regressions

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

* fix conflicts

* evm: fix panic when transaction is reverted (#650)

* fix panic when transaction is reverted

* update changelog

* Update x/evm/keeper/context_stack.go

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

* rpc: test fix (#608)

* fix rpc tests with net namespace

* skip personal test

* skip rpc pending test

* fix endpoint

* fix rpc pending test

* fix missing gas param in some rpc tests

* fix eth_getproof when the block number is equal to pending or latest

* fix rpc tests filter subscribe failed

* lint

* remove unused linter

* fix PendingTransactionFilter and TestEth_GetFilterChanges_BlockFilter

* fix eth_estimateGas

* fix TestEth_EstimateGas_ContractDeployment

* skip TestEth_ExportAccount_WithStorage

* remove sleep in rpc test

* Update changelog

* add test-rpc in github action

* bump golangci-lint version to v1.42.1

* release: v0.7.1 cherry-picks

* changelog

* ci: fix solidity tests (#278)

* Fix CI

* Remove verbose-log to reduce size

* update timeout

* rm deploy contract action

* Update test-helper.js

* Update workflow

* Update workflow

* fix gas estimate amount

* Update test.yml

* fix error assert issue

* ignore bad test case

* remove estimate gas test

* Change fromBlock to 1 (TEMP, Reverse Required)

* bump timeout

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

Co-authored-by: Daniel Burckhardt <[email protected]>
Co-authored-by: Thomas Nguy <[email protected]>
Co-authored-by: JayT106 <[email protected]>
Co-authored-by: Yijia Su <[email protected]>
Co-authored-by: yihuang <[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