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

Dirty fix #46 (test_vm_json segfaults from #45), incidentally fix #32 #47

Merged
merged 1 commit into from
Jun 6, 2018
Merged

Dirty fix #46 (test_vm_json segfaults from #45), incidentally fix #32 #47

merged 1 commit into from
Jun 6, 2018

Conversation

mratsim
Copy link
Contributor

@mratsim mratsim commented Jun 6, 2018 •
edited

Fix #46 regression (from #45)

This is super ugly and in my opinion shows that the current opcode logic is flawed.
DelegateCall, CallCode and Create opcodes require the same treatment.

This will be addressed in #36 but for the moment it does the job and even has the luxury of fixing:

@coffeepots
Copy link
Contributor

I currently get a couple of fails due to gas running out when trying this branch, is this expected?

vmArithmeticTestsignextend_Overflow_dj42.json
    999956 999954
    test_vm_json.nim(89, 53): Check failed: actualGasRemaining == expectedGasRemaining or
    computation.code.hasSStore() and
    (actualGasRemaining > expectedGasRemaining and
    (actualGasRemaining - expectedGasRemaining) mod 15000 == 0 or
    expectedGasRemaining > actualGasRemaining and
    (expectedGasRemaining - actualGasRemaining) mod 15000 == 0)
  [FAILED] tests\fixtures\VMTests\vmArithmeticTest\signextend_Overflow_dj42.json

and

vmArithmeticTestsmod5.json
    test_vm_json.nim(68, 10): Check failed: not computation.isError
    computation.isError was true
Computation error: Out of gas: Needed 20000 - Remaining 9980 - Reason: SSTORE: [15, 87, 46, 82, 149, 197, 127, 21, 136, 111, 155, 38, 62, 47, 109, 45, 108, 123, 94, 198][slot] -> 2 (0)
    9980 4980
    test_vm_json.nim(89, 53): Check failed: actualGasRemaining == expectedGasRemaining or
    computation.code.hasSStore() and
    (actualGasRemaining > expectedGasRemaining and
    (actualGasRemaining - expectedGasRemaining) mod 15000 == 0 or
    expectedGasRemaining > actualGasRemaining and
    (expectedGasRemaining - actualGasRemaining) mod 15000 == 0)
  [FAILED] tests\fixtures\VMTests\vmArithmeticTest\smod5.json

@mratsim
Copy link
Contributor Author

mratsim commented Jun 6, 2018

Yes, those tests are to track our implementation progress and those gas tests always failed.

This is also why they are not enabled by default.

coffeepots
coffeepots approved these changes Jun 6, 2018
View reviewed changes
Copy link
Contributor

@coffeepots coffeepots left a comment

Choose a reason for hiding this comment

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

Although it's not a long term solution, there's lots of work in #36 so this fix allows further testing in the mean time.

@mratsim mratsim merged commit 8528f1b into master Jun 6, 2018
@mratsim mratsim deleted the dirty-temp-fix-vmjson-46 branch June 7, 2018 10:03
@mratsim mratsim mentioned this pull request Jun 11, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coffeepots coffeepots coffeepots approved these changes

@zah zah Awaiting requested review from zah

@yglukhov yglukhov Awaiting requested review from yglukhov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

VM JSON tests do not run
2 participants
@mratsim @coffeepots