Check all transactions and witnesses#2266
Conversation
|
I will do |
src/neo/SmartContract/Helper.cs
Outdated
| for (int ip = 0; ip < script.Length;) | ||
| { | ||
| Instruction instruction = script.GetInstruction(ip); | ||
| Instruction instruction; |
There was a problem hiding this comment.
I think that this will reduce a lot the TPS, but if we don't merge it we should merge this block
There was a problem hiding this comment.
For most transactions, only a few instructions need to be checked, so the speed will not be affected.
There was a problem hiding this comment.
But what's the difference between this and FAULT inside the VM?
There was a problem hiding this comment.
RET + nonce will fail because nonce is not a valid institution.
There was a problem hiding this comment.
I think that it doesn't deserve the loose of performance, at the end this code never will be executed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This won't prevent to JUMP into a data position with arbitrary data.
PUSH BigRandomScript
JUMP -50
|
Wait for neo-project/neo-vm#392. |
|
@shargon Please review it again. |
Co-authored-by: Shargon <shargon@gmail.com>
No description provided.