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

Throw original error and terminate execution on programmatic errors #307

Merged
merged 1 commit into from
Jul 17, 2018

Conversation

holgerd77
Copy link
Member

This solves #191 by throwing all programmatic errors not being internal errors of the VM (like OUT_OF_GAS).

Behaviour can be tested by introducing a JS runtime error in opFn.js, e.g. by adding:

var test = toast - tee

as a first line in the ADD opcode and then run a state test encompassing this opcode in the stack:

node tests/tester.js -s --test='add11'

Behaviour before:

$ node tests/tester.js -s --test='add11'
TAP version 13
# GeneralStateTests
# file: add11 test: add11
not ok 1 the state roots should match
  ---
    operator: equal
    expected: |-
      '17454a767e5f04461256f3812ffca930443c04a47d05ce3f38940c4a14b8c479'
    actual: |-
      '3d0b7c83da4ae1c5eb6964cbfb1ffb248498ce6560113458ce7d963fff8cb449'
    at: replenish (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:1011:17)
  ...

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

-> Runtime error is implicitly handled, the differing state roots suggest some VM internal error.

Behaviour with PR:

$ node tests/tester.js -s --test='add11'
TAP version 13
# GeneralStateTests
# file: add11 test: add11
/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/lib/runCode.js:207
          throw e
          ^

ReferenceError: toast is not defined
    at ADD (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/lib/opFns.js:33:22)
    at runOp (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/lib/runCode.js:201:27)
    at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:3880:24
    at replenish (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:1011:17)
    at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:1016:9
    at eachOfLimit (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:1041:24)
    at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:1046:16
    at _parallel (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:3879:5)
    at Object.series (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:4735:5)
    at iterateVm (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/lib/runCode.js:119:11)
    at next (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:5223:28)
    at runCallback (timers.js:781:20)
    at tryOnImmediate (timers.js:743:5)
    at processImmediate [as _immediateCallback] (timers.js:714:5)

-> Original JS runtime error is thrown, indicating an error in the code

@holgerd77 holgerd77 requested review from axic, cdetrio and jwasinger June 27, 2018 12:42
@holgerd77
Copy link
Member Author

@jwasinger Hi Jared, could you also have a look at this, since you also were involved in originally covering this up? The only thing I have here is that the solution seems a bit simple for me for all the discussion we had around this 😄, I tested this nevertheless and I'm not seeing why this shouldn't work (this is actually still more-or-less what you originally proposed, chose a slightly different way, but conceptionally the same).

@holgerd77 holgerd77 force-pushed the break-on-programmatic-errors branch from 441ccf4 to 9b1a0d0 Compare July 11, 2018 08:06
@holgerd77
Copy link
Member Author

Just rebased this.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 85.706% when pulling 9b1a0d0 on break-on-programmatic-errors into 4ecae8a on master.

@@ -200,8 +200,12 @@ module.exports = function (opts, cb) {
// run the opcode
var result = opFn.apply(null, args)
} catch (e) {
cb(e)
return
if (e.errorType && e.errorType === 'VmError') {
Copy link
Member

@axic axic Jul 16, 2018

Choose a reason for hiding this comment

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

I think instead of errorType you could use something like e is VmError or typeof(e) === VmError.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, no, tested this. Even on a VmError is still of common type Error.

Copy link
Member Author

Choose a reason for hiding this comment

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

(this was my first idea as well)

@holgerd77 holgerd77 merged commit 0e6d2ff into master Jul 17, 2018
@holgerd77 holgerd77 deleted the break-on-programmatic-errors branch July 17, 2018 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants