Check script and abi when deploying contracts #2263
Conversation
|
The only I'd probably add is |
|
@shargon @roman-khimov I added more checks. Please review again. |
| instructions.Add(ip, instruction); | ||
| ip += instruction.Size; | ||
| } | ||
| foreach (var (ip, instruction) in instructions) |
There was a problem hiding this comment.
In the past, I need to check if it's happens now, we used a RET and dummy data in order to randomize a transaction, with this, we will need to do it with a right opcodes.
There was a problem hiding this comment.
We have proper nonce in every transaction now, so I don't think it's needed. And these checks currently only affect deployed contracts, transactions can still have some garbage in their script (which is not good, but much less of an issue anyway).
There was a problem hiding this comment.
@roman-khimov Do you think we should check all the transactions and the witnesses?
There was a problem hiding this comment.
That's a tough question. Ideally, yes, but this can affect performance (unlike this PR which is almost free, contract deployments/updates are rare). At the same time scripts in transactions are usually short and we already parse witness verification scripts (IsStandardContract in VerifyWitness) without any noticeable issues. And this check is state-independent, which also is good (can be done concurrently).
So maybe it's possible. But if we're to do that I'd go with a bit more efficient checker.
There was a problem hiding this comment.
I've made a new PR to check all the transactions and witnesses. Please check #2266. @roman-khimov
There was a problem hiding this comment.
I think that it will be slower, we should trust in the vm
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.
|
I will merge this first. And please check #2266. |
Some methods must be defined in a valid ABI. Refs. neo-project/neo#2263.
Close #733
Close #2030
Close neo-project/neo-vm#376