Check all transactions and witnesses

[erikzhang]

No description provided.

[Qiao-Jin]

I will do

[shargon]

I think that this will reduce a lot the TPS, but if we don't merge it we should merge this block

[erikzhang]

For most transactions, only a few instructions need to be checked, so the speed will not be affected.

[shargon]

But what's the difference between this and FAULT inside the VM?

[erikzhang]

RET + nonce will fail because nonce is not a valid institution.

[shargon]

I think that it doesn't deserve the loose of performance, at the end this code never will be executed.

[roman-khimov]

But what's the difference between this and FAULT inside the VM?

I think it's the one described in #733, this check in place makes introducing new opcodes into VM a little easier, we'll know for sure that no transaction and no script on the mainnet contains this opcode so this extension can always be guaranteed to be backwards-compatible. And we'll reject bad transactions before they enter the chain thus reducing the actual load on the chain.

I think that this will reduce a lot the TPS

If properly implemented it shouldn't be a problem. Typical NEP-17 transfer looks like this:

NEO-GO-VM 0 > ops
INDEX    OPCODE       PARAMETER                                   
0        PUSHNULL                                                 <<
1        PUSHINT64    100000000000 (00e8764817000000)             
10       PUSHDATA1    0c20bec57eea2b27b8a07dd0d13fee66851e2430    
32       PUSHDATA1    1b615b593f416552bbc09f49002f1f4722dddc8c    
54       PUSH4                                                    
55       PACK                                                     
56       PUSHDATA1    7472616e73666572 ("transfer")               
66       PUSHDATA1    ee4b95aaaa6b5ee0678a5ad0d1846fcbd70612a8    
88       SYSCALL      System.Contract.Call (627d5b52)             
93       ASSERT                                                   

Which is just 10 instructions.

[shargon]

If we throw an specific exception and we check the fault exception during the execution, we can do the same without checking before, isn't it? (neo-project/neo-vm#390)

[roman-khimov]

No, we can't. Consider some compiler using POPITEM instruction, but otherwise completely preview4-compatible. I can create a transaction with a script from this compiler and it will happily fail on preview4. Then we update VM (with POPITEM now implemented) and rerun the chain, suddenly this script runs in a different fashion.

I do understand that this problem is related to VM versioning (neo-project/neo-vm#142 and we still need it), but at the same time it allows to make some backwards-compatible extensions without doing versioning at all.

And it also makes sense as a general safety rule, we at least ensure that whatever we will run in the VM (whole network will run, potentially in different VMs) is made of correct instructions.

[shargon]

This won't prevent to JUMP into a data position with arbitrary data.

PUSH BigRandomScript
JUMP -50

[erikzhang]

Wait for neo-project/neo-vm#392.

[erikzhang]

@shargon Please review it again.