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 nonce problem #2404

Merged
merged 4 commits into from
Dec 2, 2022
Merged

Fix nonce problem #2404

merged 4 commits into from
Dec 2, 2022

Conversation

jochem-brouwer
Copy link
Member

@jochem-brouwer jochem-brouwer commented Nov 4, 2022

Closes #2399

Do not merge, as I just realized when creating this PR why we had this nonce stuff change in the first place and why we had lengthy discussions. Discussion goes into #2399

Note: this rollbacks the changes made to EVM/VM source of #2383 (I consider this rather hacky), but it keeps the tests of that PR.

@jochem-brouwer jochem-brouwer added PR state: WIP PR state: do-not-merge PR/Issue state: blocked 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 Nov 4, 2022
@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Merging #2404 (7eb1343) into master (256bdb9) will increase coverage by 0.19%.
The diff coverage is 77.77%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 89.51% <ø> (ø)
blockchain 90.03% <ø> (ø)
client 87.47% <ø> (ø)
common 98.16% <ø> (ø)
devp2p 91.62% <ø> (-0.39%) ⬇️
ethash ∅ <ø> (∅)
evm 79.72% <77.77%> (-0.04%) ⬇️
rlp ?
statemanager 89.61% <ø> (ø)
trie 90.36% <ø> (-0.30%) ⬇️
tx 97.80% <ø> (ø)
util ?
vm ?

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

@jochem-brouwer
Copy link
Member Author

OK, for some reason the CI goes out-of-memory on ShanghaiLove test, but only on Homestead fork. It is not clear to me why any changes to this nonce logic should influence this, but OFC this has to be solved.

@jochem-brouwer jochem-brouwer marked this pull request as ready for review November 30, 2022 18:18
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.

This all looks good as far as it goes but is it going to have unintended knock-on effects when we start really using the EVM as a standalone execution environment?

@acolytec3 acolytec3 merged commit dc4cd91 into master Dec 2, 2022
@gitpoap-bot
Copy link

gitpoap-bot bot commented Dec 2, 2022

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2022 EthereumJS Contributor:

GitPOAP: 2022 EthereumJS Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

@acolytec3 acolytec3 deleted the fix-nonce-problem branch December 2, 2022 15:11
@acolytec3 acolytec3 mentioned this pull request Dec 15, 2022
1 task
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.

VM updates nonce after running a transaction, but should do right before executing it
2 participants