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

fix: increase nonce in ante handler for contract creation tx #809

Merged
merged 5 commits into from
Dec 16, 2021
Merged

fix: increase nonce in ante handler for contract creation tx #809

merged 5 commits into from
Dec 16, 2021

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Dec 3, 2021 •
edited
Loading

Closes: #808

Description

Solution:

  • move nonce increment to ante handler
  • revert nonce increment in apply message

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)

@yihuang yihuang force-pushed the fix-nonce-increase branch 2 times, most recently from 20a8720 to 12acbe4 Compare December 3, 2021 03:25
@fedekunze
Copy link
Contributor

@yihuang tests are failing

@fedekunze fedekunze enabled auto-merge (squash) December 3, 2021 15:00
@yihuang
Copy link
Contributor Author

yihuang commented Dec 3, 2021 •
edited
Loading

@yihuang tests are failing

The ante handler testing fails because nonce doesn't increase now, there are some implications of this change in real-world too, for example, a sequence of transactions in mempool might run checkTx successfully before but fail now because of nonce mismatch.
Should we do a fake nonce increasing in ante handler only for checkTx mode, to keep the behavior of mempool the same as before?

leejw51crypto
leejw51crypto approved these changes Dec 6, 2021
View reviewed changes
Copy link
Contributor

@leejw51crypto leejw51crypto left a comment

Choose a reason for hiding this comment

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

lgtm

@leejw51crypto
Copy link
Contributor

because when tx is reverted in evm, nonce will increase.
so it will fix redundant tx issue.

auto-merge was automatically disabled December 6, 2021 04:55

Head branch was pushed to by a user without write access

@yihuang
Copy link
Contributor Author

yihuang commented Dec 6, 2021 •
edited
Loading

@yihuang tests are failing

The ante handler testing fails because nonce doesn't increase now, there are some implications of this change in real-world too, for example, a sequence of transactions in mempool might run checkTx successfully before but fail now because of nonce mismatch. Should we do a fake nonce increasing in ante handler only for checkTx mode, to keep the behavior of mempool the same as before?

I added back nonce increasing in ante handler, but only for (re)check tx mode, to keep mempool behavior the same as before.

@codecov
Copy link

codecov bot commented Dec 6, 2021 •
edited
Loading

Codecov Report

Merging #809 (5ebab7d) into main (514785b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #809      +/-   ##
==========================================
- Coverage   56.70%   56.70%   -0.01%     
==========================================
  Files          72       72              
  Lines        6059     6051       -8     
==========================================
- Hits         3436     3431       -5     
+ Misses       2424     2422       -2     
+ Partials      199      198       -1
Impacted Files Coverage Δ
app/ante/eth.go 84.75% <ø> (+0.37%) ⬆️
x/evm/keeper/state_transition.go 77.34% <100.00%> (+0.22%) ⬆️

yihuang
yihuang commented Dec 8, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
fedekunze
fedekunze reviewed Dec 9, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
fedekunze
fedekunze suggested changes Dec 9, 2021
View reviewed changes
app/ante/eth.go Outdated Show resolved Hide resolved
x/evm/keeper/state_transition.go Outdated Show resolved Hide resolved
@yihuang yihuang changed the title increase nonce for reverted contract deployment transaction fix: increase nonce in ante handler for contract creation tx Dec 10, 2021
fedekunze
fedekunze approved these changes Dec 13, 2021
View reviewed changes
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.

ACK

@fedekunze
Copy link
Contributor

@yihuang seems that tests are failing

@yihuang
increase nonce in ante handler for contract creation tx
db53c6c 
Closes: evmos#808
Solution:
- move nonce increment to ante handler
- revert nonce increment in apply message

build(deps): bump github.com/spf13/viper from 1.9.0 to 1.10.0 (evmos#833)

Bumps [github.com/spf13/viper](https://github.com/spf13/viper) from 1.9.0 to 1.10.0.
- [Release notes](https://github.com/spf13/viper/releases)
- [Commits](spf13/viper@v1.9.0...v1.10.0)

---
updated-dependencies:
- dependency-name: github.com/spf13/viper
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

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

fix: remove unused code (evmos#834)

Co-authored-by: Marko Baricevic <markobaricevic3778@gmail.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

rm

rm pkg
@yihuang
Copy link
Contributor Author

yihuang commented Dec 15, 2021 •
edited
Loading

The unit test failure happens in main branch too.
it's due to this commit: 6629208, should we just update the test case?

@fedekunze
Copy link
Contributor

should we just update the test case?

@yihuang yes please 🙏

@yihuang
Copy link
Contributor Author

yihuang commented Dec 16, 2021

should we just update the test case?

@yihuang yes please 🙏

fixed it here: #847

@fedekunze fedekunze enabled auto-merge (squash) December 16, 2021 22:27
@fedekunze fedekunze added the automerge Automatically merge PR once all prerequisites pass. label Dec 16, 2021
@fedekunze fedekunze merged commit e437c43 into evmos:main Dec 16, 2021
@yihuang yihuang deleted the fix-nonce-increase branch December 17, 2021 01:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fedekunze fedekunze fedekunze approved these changes

@leejw51crypto leejw51crypto leejw51crypto approved these changes

@jolube jolube Awaiting requested review from jolube

@khoslaventures khoslaventures Awaiting requested review from khoslaventures

@thomas-nguy thomas-nguy Awaiting requested review from thomas-nguy

Assignees
No one assigned
Labels
automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problem: reverted contract deployment transaction don't increase sender's nonce
4 participants
@yihuang @fedekunze @leejw51crypto @thomas-nguy