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

VMTests/.../exp8Filler: add test of EXP 0 0 #460

Merged
merged 12 commits into from
Jun 4, 2018

Conversation

ehildenb
Copy link
Contributor

@winsvega I've added a VMTest. I think this is where it belongs because it only tests VM functionality, nothing in the client. I have not filled it, let me know if I should, or if you can and then just add another commit onto this.

@axic
Copy link
Member

axic commented May 28, 2018

Would be nice to have state tests if possible because some clients only deal with state tests and not vm tests.

@pirapira
Copy link
Member

@axic I started adding a GeneralStateTest.

@axic
Copy link
Member

axic commented May 29, 2018

@pirapira we (well, @ehildenb mostly) have started work to have a script automatically convert vm tests to state tests fillers, so both can be kept.

@pirapira
Copy link
Member

ahm, then, I drop my manual translation, and work with @ehildenb on the automatic translation (anyway I'm flying to his town this Friday).

@winsvega
Copy link
Collaborator

convert vmtestFillers to stateTestFillers?

@ehildenb
Copy link
Contributor Author

@winsvega yes, convert VMTestsFiller to GeneralStateTestsFiller. Then they can just be filled using standard techniques. See #461 as the initial PR in that direction.

@axic
Copy link
Member

axic commented May 29, 2018

@jwasinger do you want to fill this?

@axic
Copy link
Member

axic commented May 29, 2018

@winsvega but conversion here means it is done by a script and the vmtests can be kept.

@jwasinger
Copy link
Contributor

@jwasinger do you want to fill this?

Sure.

@pirapira
Copy link
Member

@winsvega do you want a separate PR for a filled test? Or, shall we wait here for the filling?

@ehildenb
Copy link
Contributor Author

ehildenb commented May 31, 2018

@pirapira I have filling working now, so I can fill the test myself. But I was hoping to get the other sanitation PRs in first (like #462 and #466 ), so that I can exercise the new sanity checks on this PR.

@winsvega
Copy link
Collaborator

winsvega commented Jun 1, 2018

test sources (fillers) and tests must be on the same PR.
testeth will fail if there are tests without filler or fillers without tests or fillers and corresponding tests has different source hash.

@ehildenb
Copy link
Contributor Author

ehildenb commented Jun 1, 2018

This run should fail, because of poorly formatted JSON in the repo. Next I'll fix the formatting and make sure it fails because of invalid JSON Schema, then because of unfilled tests.

@pirapira
Copy link
Member

pirapira commented Jun 1, 2018

It's nice you are trying out the new checkers.

@ehildenb ehildenb force-pushed the exp-0-0 branch 6 times, most recently from 8a9aa99 to c14a8af Compare June 1, 2018 07:55
@ehildenb
Copy link
Contributor Author

ehildenb commented Jun 1, 2018

Do not merge yet, I am investigating a few things still:

  1. How to make sure that filled version of test actually exists (currently I have not committed the filled version of exp8 test, but tests pass).
  2. Failing of sdiv_dejavu on cpp-ethereum.

@ehildenb
Copy link
Contributor Author

ehildenb commented Jun 1, 2018

@pirapira I think I have figured out what happenned with sdiv_dejavu.

Commit ddba26a refilled the test, but did not give it a post section. This is because it was generating the nonsensical bytecode like 606005 for PUSH1 0x05 (the same LLL issues we saw). Because there was no post, the fact that the expect section has not been met was not caught by either cpp-ethereum or KEVM.

After I fixed the LLL code, it gave a post section which contains 0xffffffff... as the result of the division (the expect section contains 0x00 though). Both cpp-ethereum and KEVM agree that this is correct, though I should work out by hand as well (expression is (sdiv (sub 0 9) 5)). I expected that when filling the tests, the expect section would make it through to the filled test, but it's actually the result of cpp-ethereum which makes it through. So cpp-ethereum passes the filled test, as does KEVM, but cpp-ethereum reports error during the filling process.

So, I think, if the calculation by hand shows that it's correct, then I think I can update the expect section of the test, and refill, and it should be OK.

@ehildenb
Copy link
Contributor Author

ehildenb commented Jun 1, 2018

Here is an interaction with bc that seems to confirm it is correct

obase=16
us0 = (2^256) - 9
us1 = 5
(us0 - (2^256)) / us1 + (2^256)
FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF

Indeed, what is happening is -9 / 5, which seems like it should be -1.

@ehildenb ehildenb force-pushed the exp-0-0 branch 2 times, most recently from c6b46a7 to 4687f10 Compare June 1, 2018 19:28
@ehildenb
Copy link
Contributor Author

ehildenb commented Jun 1, 2018

Alright. This branch enables sanitation checks on the VMTests (checking that they are filled correctly, etc...).

I have hcanged the expect section of sdiv_dejavu, added the test exp8 which tests exp 0 0. Please review those files with scrutiny. The remaining changes are to enable sanitation of the VMTests.

Because of how much is changing in the way of formatting, I'm waiting for future PRs to make the same changes to the GS and BChain tests (if people want them). I propose that for filling the tests, whatever command is used as the "canonical" command is stored in the Makefile for ease of reproducibility.

Currently, to fill the VMTests, I run make VMTests.fill. If you want to fill another testset, its make GeneralStateTests.fill` for example. I will push a file documenting this.

@ehildenb ehildenb mentioned this pull request Jun 1, 2018
Copy link
Member

@pirapira pirapira left a comment

Choose a reason for hiding this comment

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

I saw the VM test has been filled, and I heard this fixes tests in cpp-ethereum.

@pirapira pirapira merged commit 20f1c0a into ethereum:develop Jun 4, 2018
@ehildenb ehildenb deleted the exp-0-0 branch June 7, 2018 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants