vm: fix accountexists bug in pre-SD hardforks

[acolytec3]

Fixes #1502 based on test case and suggestion from @jochem-brouwer. I've added the test case to the VM tests though not sure if we want to restructure it along the lines of ethereum-tests somehow. Seems like there ought to be a good way to do that and I can explore how to open that PR if we like this approach.

I have confirmed that this resolves the specific issue when syncing the client on mainnet and it is able to progress beyond this block but I've encountered another block 1233073 with an invalid ReceiptsTrie so will need some additional review to see if my code change caused this new issue or if it's something else now.

[codecov]

Codecov Report

Merging #1516 (25def52) into master (a50fb5f) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

Flag	Coverage Δ
block	86.48% <ø> (ø)
blockchain	82.78% <ø> (ø)
client	82.64% <ø> (-0.49%)	⬇️
common	94.08% <ø> (-0.19%)	⬇️
devp2p	82.66% <ø> (ø)
ethash	90.76% <ø> (ø)
tx	87.47% <ø> (ø)
vm	79.51% <100.00%> (-0.23%)	⬇️

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

[ryanio]

smallest nit ever, but let's move the test file to the appropriate dir and rename to camelCase: packages/vm/test/api/state/homestead-accountExists.spec.ts

otherwise, looks really nice :)

[acolytec3]

smallest nit ever, but let's move the test file to the appropriate dir and rename to camelCase: packages/vm/test/api/state/homestead-accountExists.spec.ts

otherwise, looks really nice :)

Done!

[jochem-brouwer]

LGTM

[jochem-brouwer]

Great additions!

[acolytec3]

Just a note, this is causing the follow EthereumTests to fail:

--fork=Homestead:
    CREATE_EmptyContractAndCallIt_0wei
    CREATE_EmptyContractAndCallIt_1wei
    CREATE_EmptyContractWithStorageAndCallIt_0wei
    CREATE_EmptyContractWithStorageAndCallIt_1wei

--fork=TangerineWhistle:
    CREATE_EmptyContractAndCallIt_0wei
    CREATE_EmptyContractAndCallIt_1wei
    CREATE_EmptyContractWithStorageAndCallIt_0wei
    CREATE_EmptyContractWithStorageAndCallIt_1wei
    tx_e1c174e2 

Run npm run tester -- --state --fork=TangerineWhistle --test='CREATE_EmptyContractAndCallIt_1wei' to verify.

I believe the issue is related to the CREATE opcode in the tests where a child contract is being created and we're incorrectly reporting the created account as not existing in the stateManager.accountExists method. More research and a PR to hopefully follow.

[jochem-brouwer]

Oops that is not good. We should have ran all tests in the ci

[acolytec3]

Agreed, maybe the lesson is to always run the full test suite against VM PRs

…

On Thu, Oct 7, 2021, 1:56 PM Jochem Brouwer ***@***.***> wrote: Oops that is not good. We should have ran all tests in the ci — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1516 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEENFXGKXNLCVVKCUQRE2FTUFXNNBANCNFSM5FL2ENRA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[jochem-brouwer]

I don't think we should run all hardforks in VM PRs, but we should if it clearly hits a hardfork which is not covered by tests (mostly rather old forks).

[acolytec3]

I've opened a new issue to address the issue.