Skip to content

Check all transactions and witnesses#2266

Merged
erikzhang merged 19 commits intomasterfrom
check-script-all
Jan 28, 2021
Merged

Check all transactions and witnesses#2266
erikzhang merged 19 commits intomasterfrom
check-script-all

Conversation

@erikzhang
Copy link
Member

@erikzhang erikzhang commented Jan 26, 2021

No description provided.

@Qiao-Jin
Copy link
Contributor

I will do

@Qiao-Jin Qiao-Jin mentioned this pull request Jan 27, 2021
@erikzhang erikzhang marked this pull request as ready for review January 27, 2021 09:06
Base automatically changed from check-script to master January 27, 2021 09:29
for (int ip = 0; ip < script.Length;)
{
Instruction instruction = script.GetInstruction(ip);
Instruction instruction;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@shargon shargon Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

PUSH BigRandomScript
JUMP -50

@erikzhang
Copy link
Member Author

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

@erikzhang
Copy link
Member Author

@shargon Please review it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants