feat(vm): implement improved error handling

[dignifiedquire]

Summary of changes
Changes introduced in this pull request:

• Introduce an ActorError and use rust error handling when doing VM invocations.

[dignifiedquire]

I just realized, we can get rid of the manual runtime.abort and call that outside when an error is encountered.

[austinabell]

I just realized, we can get rid of the manual runtime.abort and call that outside when an error is encountered.

The reason errors aren't handled here is for one to future proof for when there can be custom execution not to allow a state transition to crash the node. The plan was to have the vm execution wrapped in a catch_unwind so that no matter what inputs or code is executed, the state transition cannot crash the node.

Let me know if you think this assumption is bad or that this should and can be handled without catching the panic and handling aborts that way through the runtime.

cc: @ec2 @dutterbutter

[austinabell]

Thought about it a bit more, I actually prefer this as well. It was how I wanted this to be setup but I was just trying to match the spec as close as possible

[ec2]

Yeah, we shouldn't panic and catch it when it unwinds... That's not.. natural lol

[ec2]

thought it had approved this and posted that comment already, guess i misclicked... classic

[austinabell]

Yeah, we shouldn't panic and catch it when it unwinds... That's not.. natural lol

Yeah the point is this is supposed to be a VM that is future proofed to run custom code, seems weird to handle errors in specific scenarios, force any code in future to handle errors, and allow the execution to crash the program potentially. I agree this is better for now, but I don't see the big aversion to this or why you didn't say something before

[dignifiedquire]

Regarding catching panics, I would say that all errors that are expected should be handled using regular rust error handling, but that the actual call to apply the message likely should be wrapped into a panic catcher, to ensure that if there is some unexpected behaviour or simply bugs in the implmentation here, they are caught and do not crash the node. This is last measure of defense, and should not be used as the default way of handling errors.

[austinabell]

Regarding catching panics, I would say that all errors that are expected should be handled using regular rust error handling, but that the actual call to apply the message likely should be wrapped into a panic catcher, to ensure that if there is some unexpected behaviour or simply bugs in the implmentation here, they are caught and do not crash the node. This is last measure of defense, and should not be used as the default way of handling errors.

I agree, but the idea of when custom code is allowed, it isn't written in the spec to use an error type that is serialized consistently for use in multiple language clients, also because the go version handles it with panic. But in any case we agree now this is the best way to handle it, thanks again for the contribution :)

[dignifiedquire]

but the idea of when custom code is allowed,

I expect that when we go to that level this will have to be redone from scratch anyway.