Clean up VmError #219

axic · 2017-10-12T23:37:39Z

Pulled from #174.

jwasinger

I got this running. 45 failing tests. I wonder if the VM is actually broken in those.

axic · 2017-10-18T09:22:21Z

lib/runCode.js

@@ -193,8 +193,7 @@ module.exports = function (opts, cb) {
        // run the opcode
        var result = opFn.apply(null, args)
      } catch (e) {
-        runState.vmError = e.error
-        cb()
+        cb(new VmError(e.error))


This actually could be cb(e)

holgerd77 · 2017-10-24T12:30:24Z

This test run falls in the period where we temporarily had the Promise-version for async/await in ethereumjs-testing with the memory overflow problems, might have influenced the results. Have restarted the build, we'll see what comes out of this.

holgerd77 · 2017-10-24T12:55:15Z

Ah no, still failing. :-/

axic · 2017-10-24T12:58:24Z

Looking at the below more carefully:

   function parseVmResults (err) {
     err = runState.vmError || err

Shows that err must be an error text ("constant") and not an exception object.

holgerd77 · 2017-10-24T13:08:52Z

Can't investigate further - no time - but here are some results when running one failing test with node tests/tester -s --test='returndatacopy_following_revert' --jsontrace on both branches:

master

{"pc":41,"op":96,"gas":"0x9ffffab10","gasCost":"0x3","stack":["0x20"],"depth":0,"opName":"PUSH1"}
{"pc":43,"op":96,"gas":"0x9ffffab0d","gasCost":"0x3","stack":["0x20","0x0"],"depth":0,"opName":"PUSH1"}
{"pc":45,"op":62,"gas":"0x9ffffab0a","gasCost":"0x3","stack":["0x20","0x0","0x0"],"depth":0,"opName":"RETURNDATACOPY"}
{"pc":46,"op":96,"gas":"0x9ffffab04","gasCost":"0x3","stack":[],"depth":0,"opName":"PUSH1"}
{"pc":48,"op":81,"gas":"0x9ffffab01","gasCost":"0x3","stack":["0x0"],"depth":0,"opName":"MLOAD"}
{"pc":49,"op":96,"gas":"0x9ffffaafb","gasCost":"0x3","stack":["0x111122223333444455556666777788889999aaaabbbbccccddddeeeeffff"],"depth":0,"opName":"PUSH1"}
{"pc":51,"op":85,"gas":"0x9ffffaaf8","gasCost":"0x0","stack":["0x111122223333444455556666777788889999aaaabbbbccccddddeeeeffff","0x0"],"depth":0,"opName":"SSTORE"}
ok 1 the state roots should match

1..1
# tests 1
# pass  1

error-cb

{"pc":41,"op":96,"gas":"0xffffab22","gasCost":"0x3","stack":["0x20"],"depth":0,"opName":"PUSH1"}
{"pc":43,"op":96,"gas":"0xffffab1f","gasCost":"0x3","stack":["0x20","0x0"],"depth":0,"opName":"PUSH1"}
{"pc":45,"op":62,"gas":"0xffffab1c","gasCost":"0x3","stack":["0x20","0x0","0x0"],"depth":0,"opName":"RETURNDATACOPY"}
not ok 1 the state roots should match
  ---
    operator: equal
    expected: |-
      '7021b60fa4ce0fd56ce1b85e9c529345300b005574afb1f51a8363471cefdcd3'
    actual: |-
      'dbcee120f31bca7ddc3ec1d52a1b861c02c63d469e954096ff264858b58df101'
    at: replenish (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:946:17)
  ...

1..1
# tests 1
# pass  0
# fail  1

Silur · 2017-11-16T18:13:21Z

returnDataCopy revert only needed a deepcheck for err.
Currently failing

RevertOpcodeCreate
RevertOpcodeInCreateReturns
RevertOpcodeInInit
fix in progress

axic · 2017-11-16T21:06:15Z

lib/runCode.js

@@ -214,7 +213,7 @@ module.exports = function (opts, cb) {
  }

  function parseVmResults (err) {
-    err = runState.vmError || err
+    err = runState.vmError || (err && (err.error || err))


Would be nice if the instances not returning an error object would be fixed instead. Do you happen to know which ones?

I was able to remove this. now we are back to the failures that @Silur mentioned.

jwasinger · 2017-12-06T04:55:28Z

Hey, @Silur. My apologies, I forgot you had claimed this issue. Were you able to make any progress on the four failing tests? Or can I try and get this finished?

Silur · 2017-12-06T19:28:02Z

oh great you found the part where it didn't return with an err!
I put the hackcheck for that, lemme see what breaks now

Silur · 2017-12-06T20:00:13Z

lib/runCode.js

@@ -193,8 +193,7 @@ module.exports = function (opts, cb) {
        // run the opcode
        var result = opFn.apply(null, args)
      } catch (e) {
-        runState.vmError = e.error


this is where RevertOpcodeCreate breaks, using new VmError(e) or new VmError(e.error) doesn't work, maybe leave the runState error here for e is not in any of the categorized exceptions?

I'm not even sure if vmError is used anymore after these changes. Ideally, we could remove it.

vmError is used in the result output object by users of the VM (such as Remix)

Ah okay. Best to leave it in then :D

Sorry, double checked remix and it uses exceptionError. Though there might be some other user which uses vmError, so I should add that back it properly

Silur · 2017-12-07T22:44:01Z

lib/runCode.js

@@ -193,6 +193,7 @@ module.exports = function (opts, cb) {
        // run the opcode
        var result = opFn.apply(null, args)
      } catch (e) {
+				runState.vmError = e


any error occurred here does not belong to any of the known exceptions, so excluding them from the runState will break stateroots

I see @axic has incorporated a new type of error INTERNAL_ERROR (https://github.com/ethereumjs/ethereumjs-vm/blob/35e8ddcc71ae47954d5a2599d784b3e63507df6f/lib/constants.js#L9) in #222 .

Maybe we can pull those changes to this PR, and add a new field that allows us to specify a custom error message for INTERNAL_ERRORs .

jwasinger

LGTM

axic · 2017-12-09T18:48:49Z

@jwasinger didn't know it will dismiss the review :(

axic · 2017-12-09T18:50:03Z

@Silur @jwasinger thanks for fixing this!

axic · 2017-12-09T19:25:45Z

@jwasinger @Silur please review the last commit and merge hopefully

jwasinger reviewed Oct 13, 2017

View reviewed changes

axic force-pushed the error-cb branch from 0c0c178 to 34cd87b Compare October 18, 2017 09:22

axic commented Oct 18, 2017

View reviewed changes

axic mentioned this pull request Oct 18, 2017

Validate stack items after operations #222

Merged

axic force-pushed the error-cb branch from 34cd87b to 2ec60f0 Compare October 28, 2017 11:12

Silur self-assigned this Nov 16, 2017

axic commented Nov 16, 2017

View reviewed changes

Silur reviewed Dec 6, 2017

View reviewed changes

Silur reviewed Dec 7, 2017

View reviewed changes

jwasinger previously approved these changes Dec 8, 2017

View reviewed changes

axic and others added 4 commits December 8, 2017 14:56

Handle VmError more consistently

bb7086d

Rename constants.js to exceptions.js

95173fb

change 'constants.js' to 'exceptions.js' in precompiles

cd474fd

Handle all exceptions as an object in runCode

96aa79b

jwasinger dismissed their stale review via a69e9ad December 8, 2017 23:06

jwasinger force-pushed the error-cb branch from d301ee4 to a69e9ad Compare December 8, 2017 23:06

jwasinger previously approved these changes Dec 9, 2017

View reviewed changes

jwasinger and others added 2 commits December 9, 2017 18:41

use VMError object everywhere

9d48dd7

fix revert issue in VMError

bee8532

axic dismissed jwasinger’s stale review via bee8532 December 9, 2017 18:42

axic force-pushed the error-cb branch from a69e9ad to bee8532 Compare December 9, 2017 18:42

Ensure that runState.vmError is always properly set

5a2dfe1

jwasinger approved these changes Dec 9, 2017

View reviewed changes

jwasinger merged commit 14a164e into master Dec 9, 2017

axic mentioned this pull request Dec 10, 2017

Exceptions that are not caused by the VM should terminate program execution #191

Closed

axic deleted the error-cb branch December 10, 2017 03:35

evertonfraga added the package: vm label Feb 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up VmError #219

Clean up VmError #219

axic commented Oct 12, 2017

jwasinger left a comment

axic Oct 18, 2017

holgerd77 commented Oct 24, 2017

holgerd77 commented Oct 24, 2017

axic commented Oct 24, 2017

holgerd77 commented Oct 24, 2017

Silur commented Nov 16, 2017 •

edited

Loading

axic Nov 16, 2017

jwasinger Dec 6, 2017 •

edited

Loading

jwasinger commented Dec 6, 2017

Silur commented Dec 6, 2017

Silur Dec 6, 2017

jwasinger Dec 8, 2017 •

edited

Loading

axic Dec 8, 2017

jwasinger Dec 8, 2017

axic Dec 9, 2017

Silur Dec 7, 2017

jwasinger Dec 8, 2017

jwasinger left a comment

axic commented Dec 9, 2017

axic commented Dec 9, 2017

axic commented Dec 9, 2017 •

edited

Loading

Clean up VmError #219

Clean up VmError #219

Conversation

axic commented Oct 12, 2017

jwasinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Oct 24, 2017

holgerd77 commented Oct 24, 2017

axic commented Oct 24, 2017

holgerd77 commented Oct 24, 2017

Silur commented Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

jwasinger Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

jwasinger commented Dec 6, 2017

Silur commented Dec 6, 2017

Choose a reason for hiding this comment

jwasinger Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwasinger left a comment

Choose a reason for hiding this comment

axic commented Dec 9, 2017

axic commented Dec 9, 2017

axic commented Dec 9, 2017 • edited Loading

Silur commented Nov 16, 2017 •

edited

Loading

jwasinger Dec 6, 2017 •

edited

Loading

jwasinger Dec 8, 2017 •

edited

Loading

axic commented Dec 9, 2017 •

edited

Loading