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

Ensure EVM runs when nonce is zero #2054

Merged
merged 3 commits into from
Jul 20, 2022
Merged

Ensure EVM runs when nonce is zero #2054

merged 3 commits into from
Jul 20, 2022

Conversation

jochem-brouwer
Copy link
Member

This PR is based on an old version of master. In here I address the nonce bug in #2009.

I think that pre-EVM calls the state should be exactly as it is pre-EVM. Since we have introduced this "nonce - 1" logic at some point this should be removed (and I think this might be easy). This first version does the depth check and removes the 'nonce - 1' if the depth is zero.

I tried removing it by also moving the "increment nonce" logic in interpreter.ts the same way how I moved it now in runTx, but this has a failing chainstart test RecursiveCreateContractsCreate4Contracts_d0g0v0_Frontier which I should look into.

@jochem-brouwer jochem-brouwer added PR state: WIP package: vm type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. package: evm labels Jul 18, 2022
@jochem-brouwer jochem-brouwer changed the title Evm nonce neg Ensure EVM runs when nonce is zero Jul 18, 2022
@codecov
Copy link

codecov bot commented Jul 18, 2022

Codecov Report

Merging #2054 (8be5b40) into master (2dd63b9) will decrease coverage by 0.04%.
The diff coverage is 70.00%.

Impacted file tree graph

Flag Coverage Δ
block 84.01% <ø> (ø)
blockchain 80.10% <ø> (ø)
client 78.37% <ø> (ø)
common 94.90% <ø> (ø)
devp2p 82.73% <ø> (ø)
ethash 90.81% <ø> (ø)
evm 40.89% <100.00%> (-0.08%) ⬇️
statemanager 84.55% <ø> (ø)
trie 80.84% <ø> (-0.49%) ⬇️
tx 92.18% <ø> (ø)
util 87.22% <ø> (ø)
vm 78.51% <50.00%> (-0.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@jochem-brouwer
Copy link
Member Author

Ah right, we cannot update interpreter.ts to update the nonce before runCall. The reason is: if the current account A which executes a CREATE then creates an account B which recursively calls back into account A, which then does another CREATE, will then not have the "new nonce" and thus will try to create a new account at B, while it should create an account at another address C.

If you recursively call back into the CREATE-ing account, then the nonce never updates, so it always tries to CREATE at the same address.

@acolytec3
Copy link
Contributor

@jochem-brouwer I reverted the changes I made around setting the default caller and fixed the 2 broken evm tests based on it producing the correct address/code so should be good to go unless you have more changes to make.

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

Not sure if there is more work to be done but this looks good to me!

@acolytec3 acolytec3 linked an issue Jul 19, 2022 that may be closed by this pull request
@jochem-brouwer jochem-brouwer marked this pull request as ready for review July 20, 2022 14:15
@jochem-brouwer
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: evm package: vm PR state: needs review type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EVM and state precondition management
3 participants