Stack Isolation #39

erikzhang · 2018-05-08T09:44:43Z

Implement NEP-8: Stack Isolation
neo-project/proposals#22

Close #35

…utionContext`

…cuting `RET` instruction

…ALL_ET`, `CALL_EDT`

erikzhang · 2018-05-15T08:09:13Z

@lightszero @localhuman @shargon @anthdm

Can you guys check this PR for your repositories so that:

For NeoVM in other language implementations, make sure that they also implement the Stack Isolation (NEP-8) scheme correctly.
For each neo compiler, test whether the program they are compiling now can run in NeoVM compatibility mode.
If the compiler works well with the compatibility mode, modify the compiler to support the new, more secure stack isolation mode.

localhuman · 2018-05-15T12:44:07Z

In progress now!

shargon · 2018-05-16T16:13:27Z

A lot of changes to review, give me a couple of days please

shargon

I still reviewing, the first problem that i find is this

PUSH1 (without RET)

Before the update:
Result: HALT, EvaluationStack[]{true};

After the update:
Result: HALT, ResultStack{}

If exists any script like this in all Blockchain, without versioning, is impossible to reconstruct the chain from the beginning

shargon · 2018-05-20T07:38:32Z

src/neo-vm/RandomAccessStack.cs

@@ -50,15 +63,21 @@ public void Push(T item)
        public T Remove(int index)
        {
            if (index >= list.Count) throw new InvalidOperationException();
-            T item = list[list.Count - index - 1];
-            list.RemoveAt(list.Count - index - 1);
+            if (index < 0) index += list.Count;


if (index < 0) index = list.Count; ?

We want to pass -2 or -3?

erikzhang · 2018-05-20T11:35:48Z

@shargon

PUSH1 (without RET)

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

Line 976 in f014d93

    
           OpCode opcode = CurrentContext.InstructionPointer >= CurrentContext.Script.Length ? OpCode.RET : (OpCode)CurrentContext.OpReader.ReadByte();

shargon · 2018-05-21T11:35:32Z

src/neo-vm/ExecutionEngine.cs

+                                State |= VMState.FAULT;
+                                return;
+                            }
+                            ExecutionContext context_call = LoadScript(context.Script, rvcount);


First of all, i love this change :) , but this is other point to consider

Maybe we need to define a new like this class:

class Script { public readonly byte[] Data; public byte[] Hash; ... }

Because i see a problem on optimization, now and before the change. When we make a CALL we copy all the current script in a new byte array (ExecutionContext) and yield it, but this is not neccessary, because we can use the same ScriptObject, without copy this values and save memory and time per any CALL

I think that in this line https://github.com/neo-project/neo-vm/pull/39/files#diff-c388295e4ed09df607113cde924c9fadR43 .net framework clone the byte array internally

This happens too when we compute the hash of the script, this class would optimize this multiple calls a lot of

No, byte array in .net is a reference type, which means it won't be cloned automatically.

But not the hash, this will be calculated in all ExecutionContext right? maybe we can save a lot of calls to SHA256

erikzhang · 2018-05-22T12:56:44Z

But not the hash, this will be calculated in all ExecutionContext right? maybe we can save a lot of calls to SHA256

You can create a separate issue for it.

shargon · 2018-05-23T10:56:58Z

Do you have a sample for test the new opcodes? the old part (CALL,RET..) is reviewed and seems perfect!

erikzhang · 2018-05-23T11:21:02Z

Do you have a sample for test the new opcodes? the old part (CALL,RET..) is reviewed and seems perfect!

No.

dicarlo2 · 2018-05-24T20:38:47Z

src/neo-vm/ExecutionEngine.cs

@@ -53,28 +58,23 @@ public void Execute()

        private void ExecuteOp(OpCode opcode, ExecutionContext context)
        {
-            if (opcode > OpCode.PUSH16 && opcode != OpCode.RET && context.PushOnly)


Is it possible that this might effect reprocessing the chain? Seems like executions of PushOnly = true scripts might now succeed where before they failed.

No. PushOnly will only be used in the verification process, and the verification result of history transactions will not change.

erikzhang · 2018-05-31T09:13:23Z

Can I merge it now?

shargon · 2018-05-31T09:52:36Z

I tested the old opcodes and works fine, i need more test with the new opCodes, but it's fine to merge for pre-release

erikzhang · 2018-05-31T09:58:42Z

I tested the old opcodes and works fine, i need more test with the new opCodes, but it's fine to merge for pre-release

Pre-release has been published to nuget: https://www.nuget.org/packages/Neo.VM/2.3.0-preview1

shargon · 2018-05-31T11:03:01Z

The compiler is not finished? i would like to test the new opcodes with a real smart contract. The code looks good

erikzhang · 2018-05-31T14:40:00Z

Compiler is here: https://github.com/neo-project/neo-compiler/pull/109

erikzhang · 2018-06-14T09:48:52Z

@shargon Any update?

shargon · 2018-06-14T10:32:36Z

Sorry about my delay, i wasn't see the last notification, im working on it

erikzhang · 2018-06-19T06:31:35Z

@shargon The compiler issue has been fixed. neo-project/neo-compiler@282604fc4f8ff45a2a7456e073dadb5cf0ef8bbb

shargon · 2018-06-19T15:15:48Z

Ok, I will try again :)

shargon · 2018-06-19T16:17:51Z

It works :)

dicarlo2 · 2018-06-24T12:28:10Z

@erikzhang ETA on when this will be live? I want to make sure NEO-ONE is updated.

localhuman · 2018-06-24T12:29:31Z

I hope we can test this on testnet for a little while before going live 😉

erikzhang · 2018-06-24T12:53:36Z

Sure, it will be tested on testnet before being merged into neo-project/neo

shargon · 2018-06-24T18:25:45Z

This is a great upgrade! great work @erikzhang :)

erikzhang added 8 commits May 4, 2018 20:14

move EvaluationStack and AltStack from ExecutionEngine to `Exec…

9963ea5

…utionContext`

move break_points from ExecutionContext to ExecutionEngine

8aa11cb

remove push_only

129caa7

remove Clone from ExecutionContext

b506458

optimize code

05545a8

isolate AltStack in compatible mode

e2dfac8

automatically copy the return values to the evaluation stack when exe…

d57143c

…cuting `RET` instruction

add new opcode for stack isolation: CALL_I, CALL_E, CALL_ED, `C…

1f27d47

…ALL_ET`, `CALL_EDT`

erikzhang requested review from localhuman and shargon May 8, 2018 09:44

shargon reviewed May 20, 2018

View reviewed changes

supports multiple return values

70e2582

shargon reviewed May 21, 2018

View reviewed changes

dicarlo2 reviewed May 24, 2018

View reviewed changes

shargon previously approved these changes May 30, 2018

View reviewed changes

Merge branch 'master' into feature/nep-8

fd6ee31

erikzhang dismissed shargon’s stale review via fd6ee31 June 14, 2018 09:56

make ExecutionEngine.Dispose be virtual

5d7985d

shargon approved these changes Jun 19, 2018

View reviewed changes

erikzhang merged commit 79c3aad into master Jun 24, 2018

erikzhang deleted the feature/nep-8 branch June 24, 2018 09:10

localhuman mentioned this pull request Aug 31, 2018

Implement NEP8 CityOfZion/neo-python#585

Closed

spencercorwin mentioned this pull request Feb 5, 2021

Fix CALL opcode in VM and compiler neo-one-suite/neo-one#2306

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stack Isolation #39

Stack Isolation #39

erikzhang commented May 8, 2018 •

edited

Loading

erikzhang commented May 15, 2018

localhuman commented May 15, 2018

shargon commented May 16, 2018

shargon left a comment •

edited

Loading

shargon May 20, 2018

erikzhang May 20, 2018

shargon May 20, 2018

erikzhang commented May 20, 2018 •

edited

Loading

shargon May 21, 2018

shargon May 21, 2018

erikzhang May 22, 2018

shargon May 22, 2018

erikzhang commented May 22, 2018

shargon commented May 23, 2018

erikzhang commented May 23, 2018

dicarlo2 May 24, 2018

erikzhang May 25, 2018

erikzhang commented May 31, 2018

shargon commented May 31, 2018 •

edited

Loading

erikzhang commented May 31, 2018

shargon commented May 31, 2018

erikzhang commented May 31, 2018

erikzhang commented Jun 14, 2018

shargon commented Jun 14, 2018

erikzhang commented Jun 19, 2018

shargon commented Jun 19, 2018

shargon commented Jun 19, 2018

dicarlo2 commented Jun 24, 2018

localhuman commented Jun 24, 2018

erikzhang commented Jun 24, 2018

shargon commented Jun 24, 2018 •

edited

Loading

Stack Isolation #39

Stack Isolation #39

Conversation

erikzhang commented May 8, 2018 • edited Loading

erikzhang commented May 15, 2018

localhuman commented May 15, 2018

shargon commented May 16, 2018

shargon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikzhang commented May 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikzhang commented May 22, 2018

shargon commented May 23, 2018

erikzhang commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikzhang commented May 31, 2018

shargon commented May 31, 2018 • edited Loading

erikzhang commented May 31, 2018

shargon commented May 31, 2018

erikzhang commented May 31, 2018

erikzhang commented Jun 14, 2018

shargon commented Jun 14, 2018

erikzhang commented Jun 19, 2018

shargon commented Jun 19, 2018

shargon commented Jun 19, 2018

dicarlo2 commented Jun 24, 2018

localhuman commented Jun 24, 2018

erikzhang commented Jun 24, 2018

shargon commented Jun 24, 2018 • edited Loading

erikzhang commented May 8, 2018 •

edited

Loading

shargon left a comment •

edited

Loading

erikzhang commented May 20, 2018 •

edited

Loading

shargon commented May 31, 2018 •

edited

Loading

shargon commented Jun 24, 2018 •

edited

Loading