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.

Sign up for GitHub

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

Jump to bottom

Remove VM tests using the mock BLOCKHASH/BALANCE instruction just for VM tests #444

Merged
merged 4 commits into from
May 11, 2018
Merged

Remove VM tests using the mock BLOCKHASH/BALANCE instruction just for VM tests #444

merged 4 commits into from
May 11, 2018

Conversation

pirapira
Copy link
Member

@pirapira pirapira commented Apr 4, 2018

See the discussion #415 (comment)

@pirapira pirapira requested a review from winsvega April 4, 2018 13:14
@pirapira pirapira mentioned this pull request Apr 5, 2018
Closed
@ehildenb ehildenb mentioned this pull request Apr 10, 2018
Merged
@ehildenb
Copy link
Contributor

@pirapira I believe the following tests also use BLOCKHASH
runtimeverification/evm-semantics@dea8f07

@pirapira
Copy link
Member Author

I'll have a look into these four, and later modify the vmtest runner in eth-isabelle so it fails on any BLOCKHASH.

@pirapira pirapira changed the title Remove VM tests using the mock BLOCKHASH instruction just for VM tests [wip] Remove VM tests using the mock BLOCKHASH instruction just for VM tests Apr 11, 2018
@pirapira pirapira self-assigned this Apr 11, 2018
@pirapira
Copy link
Member Author

Yes, these should be removed too.

  • 201503102037PYTHON.json uses BALANCE.
  • 201503102148PYTHON.json uses BALANCE.
  • 201503102300PYTHON.json uses BALANCE.
  • 201503110050PYTHON.json uses BALANCE

pirapira added a commit to pirapira/tests that referenced this pull request Apr 11, 2018
@pirapira
Remove VM tests that use BALANCE instruction
a01bf4a 
See ethereum#444 (comment)
@ehildenb ehildenb mentioned this pull request Apr 11, 2018
Merged
@pirapira
Copy link
Member Author

And then, 201503120317PYTHON.json uses BALANCE.

@pirapira
Copy link
Member Author

I can detect those VM tests with disallowed instructions using this modified testeth. https://github.com/pirapira/testeth/tree/vmtest-filter

@ehildenb
Copy link
Contributor

How about also removing the callcreates field of the tests?

@pirapira pirapira mentioned this pull request Apr 13, 2018
Closed
@pirapira
Copy link
Member Author

@ehildenb that's a good idea. I created a github issue #449

@pirapira pirapira changed the title [wip] Remove VM tests using the mock BLOCKHASH instruction just for VM tests [wip] Remove VM tests using the mock BLOCKHASH/BALANCE instruction just for VM tests Apr 13, 2018
@Matthalp-zz
Copy link

Matthalp-zz commented Apr 15, 2018 •
edited
Loading

@pirapira If you are removing support for BALANCE here (and SELFDESTRUCT in another PR) do you also want to remove EXTCODESIZE and EXTCODECOPY too?

They are in this PR that I was (finally) going to come back and try to land this week. Let me know how I should proceed.

@pirapira
Copy link
Member Author

@matthalp yes, EXTCODE* is also being removed from VM tests. If there is a way to fire an error for these instructions in VM tests, that would be nice.

@Matthalp-zz
Copy link

@pirapira I think ethereum/aleth#4731 will fix the ambiguities around BALANCE, EXTCODECOPY, EXTDATASIZE, and SUICIDE, but not the CALL/CREATEs.

Could you help me understand what you mean by "fire an error"? Does this just mean that the test case is able to fail if the incorrect behavior occurs?

@pirapira
Copy link
Member Author

Because VM tests are supposed to test one instance of EVM only, and not to access the world state.

@ehildenb ehildenb mentioned this pull request Apr 16, 2018
Merged
@pirapira pirapira changed the title [wip] Remove VM tests using the mock BLOCKHASH/BALANCE instruction just for VM tests Remove VM tests using the mock BLOCKHASH/BALANCE instruction just for VM tests Apr 25, 2018
winsvega
winsvega approved these changes May 9, 2018
View reviewed changes
Copy link
Collaborator

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

so the VM tests are supproted but not for ethereum specific opcodes?

@pirapira
Copy link
Member Author

Right. Still VM tests are useful when you are implementing a new VM.

@pirapira pirapira merged commit f4faae9 into ethereum:develop May 11, 2018
@pirapira pirapira deleted the remove-blockhash-vm-tests branch May 11, 2018 09:41
tersec added a commit to status-im/nimbus-eth1 that referenced this pull request Aug 1, 2018
@tersec
pursuant to discussion in ethereum/tests#444 refactor tests to not re…
1b781ae 
…quire special-cased codepaths for a VMTest mode by allowing BLOCKHASH-dependent tests to live where actual blockchain state is provided by fixture
@tersec tersec mentioned this pull request Aug 1, 2018
Merged
tersec added a commit to status-im/nimbus-eth1 that referenced this pull request Aug 1, 2018
@tersec
following ethereum/tests#444 synchronize with upstream test changes b…
c7be521 
…y removing many which rely on mocking BLOCKHASH/BALANCE just for VM tests
@tersec tersec mentioned this pull request Aug 1, 2018
Merged
mratsim pushed a commit to status-im/nimbus-eth1 that referenced this pull request Aug 2, 2018
@tersec @mratsim
following ethereum/tests#444 synchronize with upstream test changes b…
d94dfbd 
…y removing many which rely on mocking BLOCKHASH/BALANCE just for VM tests (#92)
@veox veox mentioned this pull request Sep 9, 2018
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winsvega winsvega winsvega approved these changes

Assignees

@pirapira pirapira

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pirapira @ehildenb @Matthalp-zz @winsvega