Add helper methods

[erikzhang]

No description provided.

[roman-khimov]

It certainly simplifies protecting from bad things for contracts, so it should be fine.

[devhawk]

It feels like we should be solving this in the platform rather than in tooling.

Based on what @shargon has said, it seems like we should be automatically ABORTing when an exception is unhandled by a contract - even if the operation that threw was called from inside a try/catch block of another contract. With that default in place, we can also choose to provide an opt-in mechanism to allow unhandled exceptions to flow out of a given contract operation if the contract author so chooses. Such an approach would conform to the widely held security principle of being Secure by Default.

Once we have updated the platform to have secure defaults + an opt-in mechanism for developers to bypass the secure default if they so choose, then we need to also provide a mechanism in tooling to enable the opt-in mechanism.

[roman-khimov]

we should be automatically ABORTing when an exception is unhandled by a contract

I agree, as we've discussed with @shargon stopping unwinding at contract boundaries during exception handling makes things much more safe. Making it the default and providing some way to enable passing exceptions to the caller will be way better overall solution. But it's quite a significant change that needs to be tested against the chains/contracts we have.

[Jim8y]

I agree, as we've discussed with @shargon stopping unwinding at contract boundaries during exception handling makes things much more safe. Making it the default and providing some way to enable passing exceptions to the caller will be way better overall solution. But it's quite a significant change that needs to be tested against the chains/contracts we have.

Seriously, i think it is time to add the version system to the virtual machine.... otherwise with more and more smart contracts being deployed, changes like what we have in this pr will be harder and harder to apply.

[erikzhang]

It feels like we should be solving this in the platform rather than in tooling.

I can't agree with this. In platform, we have three OpCodes related to exceptions or errors. They are ABORT, ASSERT, and THROW. They work similarly, but the first two immediately turn the virtual machine into a FAULT state, and only THROW will throw an exception. The user can choose which OpCode to use, if you want to be absolutely safe, then always using ABORT is a good choice, it's up to the user. So the problem is not the platform, but the tool.

If we don't allow exceptions to pass through by default, then the difference between ABORT and THROW all but disappears.

[roman-khimov]

Seriously, i think it is time to add the version system to the virtual machine....

👋 neo-project/neo#2987

[devhawk]

I can't agree with this. In platform, we have three OpCodes related to exceptions or errors. They are ABORT, ASSERT, and THROW. They work similarly, but the first two immediately turn the virtual machine into a FAULT state, and only THROW will throw an exception. The user can choose which OpCode to use, if you want to be absolutely safe, then always using ABORT is a good choice, it's up to the user. So the problem is not the platform, but the tool.

If we don't allow exceptions to pass through by default, then the difference between ABORT and THROW all but disappears.

I'm not suggesting changes to the VM, so I'm not sure how the similarity of various opcodes matters. The VM doesn't have the concept of "contract" - that's layered on at the Neo.dll level. And that's where I propose we handle cross contract exceptions such that we fix the security issue.

With the addition of "contracts" comes the concept of "cross contract invocation" and what - if any - differences there might be for handling exceptions that cross contract boundaries. I would argue that if a contract allows an exception to escape unhandled, then there's a reasonable assumption on the part of the developer that any changes that the contract had made (storages updates + notifications raised) would be rolled back. That's what happens when an operation that is invoked directly. That's apparently what happens on other blockchains. I believe that what should happen (by default) for cross contract invocations.

I'm OK with providing a mechanism for contracts to specify exceptions are allowed to cross contract boundaries for specific operations. But I feel strongly that such a mechanism would need to be opt-in and that discarding changes on unhandled exception needs to be the default behavior.

I personally don't care if we abort the entire transaction when an exception passes between contracts or if we discard just that contract's changes. However, in my opinion the platform must ensure that any changes that happened in the contract before it threw an exception that it didn't handle get discarded. It's not something that tools can guarantee.

[erikzhang]

Since we already have neo-project/neo#2729, we don't need AbortOnException anymore. So I've reverted most of the changes in this PR, but keep the helper methods. We'd better merge it.

Most changes are reverted.