Remove RET execution on PushOnly

[shargon]

I think that is not necessary allow execute ' RET' command while only push is allowed

neo-vm/src/neo-vm/ExecutionEngine.cs

Line 56 in b1338d4

if (opcode > OpCode.PUSH16 && opcode != OpCode.RET && context.PushOnly)

What do you think @erikzhang ?

[erikzhang]

I think we can remove PushOnly. Can you evaluate the security if we remove PushOnly and allow all the instructions in Neo.Core.Witness::InvocationScript? @shargon

[shargon]

I do not really like the idea of removing PushOnly because it could cause them to execute any code without the need for a smartContract. I think that you can not eliminate PushOnly, What benefits could it bring?

I Asked by RET to avoid fattening the script with unnecessary code type

PUSH ORIGINAL_PARAMETERS
RET
PUSHBYTES...
PUSHBYTES...
PUSHBYTES...
PUSHBYTES...
PUSHBYTES...

And the execution is still valid, but it would make the chain fatter

[erikzhang]

The most direct advantage is that after removing PushOnly, we can use the instructions such as NEWARRAY, NEWSTRUCT, PACK to create parameters in Neo.Core.Witness::InvocationScript. And there's no reason to limit it now.

[shargon]

All these instructions are executed in a different ExecutionContext, so there is not possible to change storages of the contract. But it could be used to invoke TXs and these TXs could create a different StorageContext to save arbitrary data

[erikzhang]

Contracts can't save any data if they are started with Verification trigger.

[shargon]

But this pushOnly flag is not only used under Verification, is used for arguments of invokes too, right?

[erikzhang]

Currently, it is for Verification only.

[erikzhang]

@shargon Do you think it's ok to remove PushOnly?

[shargon]

Yes, it’s ok for me 👍