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

bug: full crash when evaluating some invalid contracts #164

Closed
balthazar opened this issue Jun 17, 2022 · 11 comments
Closed

bug: full crash when evaluating some invalid contracts #164

balthazar opened this issue Jun 17, 2022 · 11 comments
Assignees
Labels
bug Something isn't working

Comments

@balthazar
Copy link
Contributor

Happens when the contract in question is invalid.

For example F2V2zXs1ylUO4hskxfNOvPlceLlk1hp_q-xEMQCPbBQ which is missing an await and thus make the main process crash.

Some others:

  • aWZe5ZZIBOKZYEQuZRZRw1ElwRnZUsk3Y_4LMoN7UC4
  • 0CFuevmMBfylDZkAFV_HETPZZDh2VFRbHZnq2QGUTjw
  • hfb7rGeMsc58G3fjnVBJaP9ogXumUE6Oc_st-QJQkLE
@ppedziwiatr
Copy link
Contributor

ppedziwiatr commented Jun 17, 2022

This happens for contracts that

  1. have missing await
  2. have a bug in code that is causing the contract code to throw an exception

In case of F2V2zXs1ylUO4hskxfNOvPlceLlk1hp_q-xEMQCPbBQ
image

  • missing await while calling createTemplate function in switch
  • error while calling Object.values, because options.options is undefined

@ppedziwiatr ppedziwiatr self-assigned this Jun 17, 2022
@ppedziwiatr ppedziwiatr added the bug Something isn't working label Jun 17, 2022
@ppedziwiatr
Copy link
Contributor

Hey @balthazar .
I'm currently working on replacing VM2 with https://github.com/laverdet/isolated-vm#isolated-vm----access-to-multiple-isolates-in-nodejs
It will probably take a while (as the documentation is not that great and it's a bit tricky to add support from external async calls (like reading other contract state) in the isolate) - in the meantime - can you please simply add

process
  .on('unhandledRejection', (reason, p) => {
    console.error(reason, 'Unhandled Rejection at Promise', p);
  })
  .on('uncaughtException', err => {
    console.error(err, 'Uncaught Exception thrown');
  });

in your host code?

This should allow to parent process to work properly, eg:
https://github.com/warp-contracts/warp/blob/ppe/isolate-vm/tools/async-test.js

  • with the unhandledRejection handler added, the After timeout log shows properly (it does not however without this handler)

@balthazar
Copy link
Contributor Author

Yeah was hoping there would be a better solution than this, as it would mean some unhandled errors of the main code will get swallowed when the process should actually crash in order to be fixed.

@ppedziwiatr
Copy link
Contributor

probably the other option for now would be to run the contract evaluation in a separate worker thread.

The isolated vm fixes this issue (ie. only the isolate crashes, not the host process), but as I've mentioned - I need a few days to implement it fully (for example - I need to manually map each of the function from the Arweave library...)

@balthazar
Copy link
Contributor Author

Yeah will probably be the only way while waiting for the new integration, thanks

@ppedziwiatr
Copy link
Contributor

ppedziwiatr commented Jun 24, 2022

Hey @balthazar , the isolated-vm version has been deployed in 1.1.5-beta.0.
In order to use the isolated-vm, set the useIVM in the evaluation options - like here: https://github.com/warp-contracts/warp/pull/168/files#diff-cf0d865821a10d5af3908fb2ac0246a7a7671e9460d97d7ce89946e913e88cf2R100

Can you please test it on your side and share your feedback?

The useIVM should now properly handle the exception and move on..
image

for a comparison - the non-useIVM is causing the process to exit
image

@balthazar
Copy link
Contributor Author

Works perfectly, thanks 😃

@ppedziwiatr
Copy link
Contributor

Issue with isolated-vm - requires at least 2GB of ram to be installed via npm (i.e. the c++ compilation requires at least this amount of memory).
This is an issue for "minimal" instances (like t3.micro on AWS or basic droplets on DigitalOcean).

Re. VM2 - turns out that adding a global handler for dealing with async errors is a solution suggested be the readme - https://github.com/patriksimek/vm2#error-handling

I've created an issue whether this can be fixed/enhanced - patriksimek/vm2#441

@balthazar
Copy link
Contributor Author

A global error handler isn't an ideal "solution" as it simply catches any unhandled error of the process, not just vm-related errors which might result in some important errors being caught by it and overlooked.

Wouldn't there be a way to copy built binaries and headers over if the memory requirement is just at build time?

@ppedziwiatr
Copy link
Contributor

yeah yeah, I'm fully aware of the issues with global handler (this probably could be minimized by analyzing the stack trace (i.e. Error.stack), but still..)

Copying is one solution, dockerization - another..

The biggest issue is that in general - isolated-vm can introduce some issues for the warp library users (probably those on Windows - but - for example - I also wasn't able to install it out of the box (after adding g++ etc.) on my prev. development machine - with Fedora 34)...

@ppedziwiatr
Copy link
Contributor

quickjs-emscripten - justjake/quickjs-emscripten#70 (comment) - no luck so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants