Mode to disable backward jumps on Neo AppEngine 3 #837

igormcoelho · 2019-06-17T01:07:16Z

This is an implementation of neo-project/neo-vm#174 on application layer. It closes neo-project/neo-vm#151.

closes #789

===

First of all, thanks for all the things you guys made me learn in the past months, it has been an amazing opportunity for me, to improve coding skills, testing skills, and learn more about cryptography and all kinds of stuff around a blockchain ecosystem like Neo.

After much thoughts, and many discussions with many different people, I think that we need an execution mode on NeoVM that would allow us to disable backward jumps. This disables loops on practice, although not generating problems to ifs. This also disables recursive calls.

Just a brief back in history, Bitcoin has become famous, and one of its explicit flags has been the capability of not having Turing-complete compution in its verification engine. It also doesn't have this sort of computation on Application Engine either (since it doesn't have one joy), so I think Neo could benefit of also not having it on Verification trigger, while maintaining a full powerful Turing-complete application engine.

Verification and Application are very different things. Verification needs to be denied as quickly as possible, when necessary, without consuming too much resources from the network, while Application can be much more expensive, since it is paid in advance in Gas. If someone wants to have an infinite loop on Application, it doesn't matter, it will take time, and this time will be well paid on System Fees, no worry (and it will FAULT and be discarded in the future). But we cannot afford having this type of computation on the Verification engine, where nothing is actually paid (yet) to the network.

Disabling Turing-completeness can be done on several manners, one of each is simply removing backward loops, like done here. There even more severe strategies, like removing all sorts of computation from here (thus basically removing the Verification Trigger away). This is an attempt to keep the verification trigger, which is useful for several stuff, while keeping it well contained and fast to process.

Hope having a heated discussion here, but thanks in advance anyway ;)

codecov-io · 2019-06-17T01:17:25Z

Codecov Report

Merging #837 into master will increase coverage by 0.02%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master     #837      +/-   ##
==========================================
+ Coverage    61.3%   61.32%   +0.02%     
==========================================
  Files         196      196              
  Lines       13430    13440      +10     
==========================================
+ Hits         8233     8242       +9     
- Misses       5197     5198       +1

Impacted Files	Coverage Δ
neo/SmartContract/ApplicationEngine.cs	`70% <90.9%> (+2.22%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e40b12...93f4734. Read the comment docs.

neo/SmartContract/ApplicationEngine.cs

shargon · 2019-06-17T10:40:33Z

neo/SmartContract/ApplicationEngine.cs

+                case OpCode.JMP:
+                case OpCode.JMPIF:
+                case OpCode.JMPIFNOT:
+                case OpCode.CALL:


With syscall, you can jump to your own contract again

From what we've been talking, most syscalls will probably be disabled on verification. In my opinion now, only crypto stuff and block header information should be available... no appcalls, no storage, ... so it will be fast as a bullet and state independent.

erikzhang · 2019-06-17T10:43:19Z

Is this realy useful?

shargon · 2019-06-17T10:55:59Z

Honestly, i can't see a use of case, because you will pay for verification

igormcoelho · 2019-06-17T13:13:32Z

The point is that failed verifications are not paid Shargon, and after you already executed them.

Having loops here is bad. If really needed, it can be unrolled. Loops here only allow you to do damaging stuff. A simple and hard to solve example: you attach 100 GAS and a while( true). How many steps it takes to consume 100 gas? After that tx is denied and nothing spent (verification fails). But this isnt the worst case, because it just damages the borders... imagine you.attach while (true) and a time lock (or fast expirarion ). This could allow this tx to pass several nodes until it finally fails on the consensus node mempool (and nothing is paid). Loops are devil creatures.
Application Trigger is different,I agree.. it is pre paid, so nothing to worry.

Just to mention that things were different on Neo2... we had global utxo, and much logic for inputs needed loops. We couldnt do auto transfers inside contracts, so we needed to "summon" the full contract here, with all its loops. We had 10 gas limit on verification, that controlled these cases, sinve we couldnt pay for verification anyway. So, I think this is a natural change afterall.

vncoelho

Mode disable jumps would be an optional param for those that opt for their contract to be like that, right @igormcoelho?

igormcoelho · 2019-07-07T21:26:39Z

Idea is to disable loops for all verifications...

lock9

I agree with the "philosophy", the problem with this PR is that I didn't see the real usage of it. Are we using this limited mode somewhere else?

shargon · 2019-07-11T21:50:39Z

what about a max fee for verification?

igormcoelho · 2019-07-11T22:44:29Z

This PR resolves a problem that cannot be solved in other way: someone spending time on verification and not actually paying by it, because tx will fail. If we dont do this, we will eventually do a much wider block, for example only allowing checksig/multisig... this is a reasonable alternative to it.

lock9 · 2019-07-11T23:33:53Z

I'm now in favor of this, because this way, the transaction length will be almost directly proportional to the fee the user must pay.
This is a good thing to be implemented.

lock9

Hi Igor,
Can you use Verification trigger instead?
I believe you won't have to worry about syscalls, it will be limited already if we use the Verification trigger and I don't think we will ever need a limited mode for the Application trigger

erikzhang · 2019-07-12T01:36:18Z

I think this may create obstacles to the implementation of the compiler.

vncoelho · 2019-07-12T01:38:42Z

Nothing for the magic couple @lightszero & @igormcoelho....whuahhehau

igormcoelho · 2019-07-12T13:05:21Z

@erikzhang hopefully, this annotation here will allow us to handle it transparently (VERIFICATION=true)

neo-project/neo-devpack-dotnet#67 (comment)

igormcoelho · 2019-07-12T13:19:22Z

But I agree Erik, this is not perfect yet... disable backward call may disable "valid" calls.

methodA
methodB call methodA (invalid)

methodB call methodA (valid)
methodA

intention is just disable recursion, we may find a better way for that. But for jumps, solution is correct in my.opinion... only ifs will survive.

[EDIT]
Silly me!! Solution is very simple (for calls), lets just put a very limited size on call count for verification. Perhaps max stack size=5 and global call count=100, verification must be lightweight.

lock9 · 2019-07-17T04:59:54Z

@igormcoelho I agree with you, verification must be lightweight.
Don't you prefer to create this PR as a draft?
I would like to have a more clear vision of what is really ready to be merged and "experimental" stuff.
What do you think? Not very impacting for this issue actually

shargon · 2019-08-06T15:07:22Z

What about a new

class VerificationEngine: ApplicationEngine

?

igormcoelho · 2019-08-11T14:38:02Z

neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs

+                verificationGas += engine.GasConsumed;
+            }
+            // no gas is spent (backwards jump is not allowed)
+            Assert.AreEqual(verificationGas, 0);


@erikzhang @shargon this basic test is already giving evidence that, for Verification, zero gas is spent (no backward jumps allowed), but on Application, it goes until funds are exhausted (and fails. this wouldn't consume the funds on verification, and this could be repeated again and again).

How can we prevent the issue with CALLS?

On CALLS, we have two options... count StepInto(), or just count it on PreExecute(). Must see what's better.

igormcoelho · 2019-08-11T14:51:37Z

neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs

+                appGas += engine.GasConsumed;
+            }
+            // many gas are spent before fault
+            Assert.AreEqual(appGas, 100000040);


We are consuming more gas than limit... 40 units more here, it shouldn't I guess.

private bool AddGas(long gas) { GasConsumed = checked(GasConsumed + gas); return testMode || GasConsumed <= gas_amount; }

the method first increase the gas, so is normal to overpass this limit i think

This is passing 40, its reasonable... the other is passing 1 million.

We should break before adding to this variable, I guess.

igormcoelho · 2019-08-11T14:52:04Z

neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs

+                appGas += engine.GasConsumed;
+            }
+            // many gas are spent before fault
+            Assert.AreEqual(appGas, 22528000);


Now it's passing much overlimit... million gas-satoshi extra, we should fix that.

I thin that is good, the limit is not raised

igormcoelho · 2019-08-11T14:52:58Z

neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs

+            }
+            // no gas should be spent (recursion is not allowed)
+            // TODO: fix recursion
+            Assert.AreEqual(verificationGas, 22528000);


This should be zero (or near zero), as long as we block recursive behavior. Perhaps stack context count is enough... not sure.

igormcoelho · 2019-08-15T01:01:15Z

@lock9 your change was adopted... you're right, Shargon also agreed that it was better filtering directly by VerificationTrigger, instead of a new variable.

neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs

Co-Authored-By: Shargon <shargon@gmail.com>

…' into feat_3x_neoapp_limited_mode

lock9

I think this looks ok, however, I'm insecure about approving it, it seems that this has not been approved by everyone, but I'm in favor of this.
Could any of you share your opinion @neo-project/core ?

erikzhang · 2019-09-12T04:00:25Z

My concern is that it can make the compiler hard to work. Because we can't guarantee that the compiler will not generate backward jump code (under various optimization).

shargon · 2019-09-12T05:11:44Z

I am in favor of this, but as Erik said, a valid SmartContract could be compiled with a backward jump (on verification)

lock9 · 2019-09-13T11:36:32Z

If we make the verification trigger a method that we have to override, won't this help? Today I understand that is very hard to know what is being included in the verification trigger logic, but if we isolate it in a method, maybe it will help?

igormcoelho · 2019-09-17T02:06:49Z

@erikzhang

My concern is that it can make the compiler hard to work. Because we can't guarantee that the compiler will not generate backward jump code (under various optimization).

for JMP I believe it's very safe, it's not supposed to break compilers... for CALL, I agree with you, we cannot do that. That's why for CALL I prefer to have some instruction counting, or very short callstack max size (5?)

This is not finished from CALL side yet, because something is bothering my mind... I keep thinking that there may be a way of a double recursion function to keep stack size small under infinite loop......... but is it possible??

Example: FUNC1 and FUNC2
FUNC1 calls FUNC2
FUNC2 calls FUNC1

somehow, they keep calling each other... dropping results after it reach max call stack 5, and then allow calling again.... is it possible? I mean, is it possible to do that, when backward JMP are not allowed?

Mode to disable backward jumps on Neo AppEngine 3

5cdd28e

igormcoelho mentioned this pull request Jun 17, 2019

Mode to disable backward jumps on NeoVM 3 neo-project/neo-vm#174

Closed

igormcoelho requested review from erikzhang, vncoelho, lightszero and shargon June 17, 2019 02:05

Merge branch 'master' into feat_3x_neoapp_limited_mode

0aafb26

shargon reviewed Jun 17, 2019

View reviewed changes

neo/SmartContract/ApplicationEngine.cs Show resolved Hide resolved

shargon reviewed Jun 17, 2019

View reviewed changes

vncoelho reviewed Jul 5, 2019

View reviewed changes

Merge branch 'master' into feat_3x_neoapp_limited_mode

668c160

lock9 reviewed Jul 8, 2019

View reviewed changes

lock9 requested changes Jul 11, 2019

View reviewed changes

lock9 mentioned this pull request Jul 13, 2019

Allow multiple scripts per "smart contract" - One for verification and one for invocation #919

Closed

Merge branch 'master' into feat_3x_neoapp_limited_mode

c75b4e9

Merge branch 'master' into feat_3x_neoapp_limited_mode

e0ad7ef

igormcoelho commented Aug 11, 2019

View reviewed changes

recursion not blocked yet

8ee60a2

igormcoelho commented Aug 11, 2019

View reviewed changes

igormcoelho mentioned this pull request Aug 12, 2019

Max GasFree for transactions #777

Closed

igormcoelho requested a review from lock9 August 15, 2019 01:00

igormcoelho and others added 3 commits August 14, 2019 22:01

Merge branch 'master' into feat_3x_neoapp_limited_mode

39215b6

Optimization

8688f38

Reuse var

e896027

shargon reviewed Aug 16, 2019

View reviewed changes

neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs Outdated Show resolved Hide resolved

shargon reviewed Aug 16, 2019

View reviewed changes

neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs Show resolved Hide resolved

igormcoelho and others added 6 commits August 17, 2019 10:36

Update neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs

b1cc6a7

Co-Authored-By: Shargon <shargon@gmail.com>

Merge branch 'master' into feat_3x_neoapp_limited_mode

b72e8f9

Split unit tests

3381b5e

Merge remote-tracking branch 'igormcoelho/feat_3x_neoapp_limited_mode…

97330bd

…' into feat_3x_neoapp_limited_mode

Merge branch 'master' into feat_3x_neoapp_limited_mode

d1c03c0

Merge branch 'master' into feat_3x_neoapp_limited_mode

13ba5b0

lock9 reviewed Sep 11, 2019

View reviewed changes

Merge branch 'master' into feat_3x_neoapp_limited_mode

93f4734

igormcoelho mentioned this pull request Dec 6, 2019

NetworkFee calculation method #840

Closed

vncoelho mentioned this pull request Dec 10, 2019

Mode to Disable Jumps and Turing-Completeness neo-project/neo-vm#151

Closed

Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020

Fix definition of deterministic (neo-project#837)

6659236

erikzhang closed this Jun 22, 2020

roman-khimov mentioned this pull request Jul 1, 2020

Turing-incomplete mode #1741

Closed

Mode to disable backward jumps on Neo AppEngine 3 #837

Mode to disable backward jumps on Neo AppEngine 3 #837

Conversation

igormcoelho commented Jun 17, 2019 • edited Loading

codecov-io commented Jun 17, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

igormcoelho Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

erikzhang commented Jun 17, 2019

shargon commented Jun 17, 2019

igormcoelho commented Jun 17, 2019 • edited Loading

vncoelho left a comment

Choose a reason for hiding this comment

igormcoelho commented Jul 7, 2019 • edited Loading

lock9 left a comment

Choose a reason for hiding this comment

shargon commented Jul 11, 2019

igormcoelho commented Jul 11, 2019 • edited Loading

lock9 commented Jul 11, 2019 • edited Loading

lock9 left a comment • edited Loading

Choose a reason for hiding this comment

erikzhang commented Jul 12, 2019

vncoelho commented Jul 12, 2019

igormcoelho commented Jul 12, 2019 • edited Loading

igormcoelho commented Jul 12, 2019 • edited Loading

lock9 commented Jul 17, 2019

shargon commented Aug 6, 2019 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igormcoelho commented Aug 15, 2019

lock9 left a comment

Choose a reason for hiding this comment

erikzhang commented Sep 12, 2019

shargon commented Sep 12, 2019 • edited Loading

lock9 commented Sep 13, 2019

igormcoelho commented Sep 17, 2019 • edited Loading

igormcoelho commented Jun 17, 2019 •

edited

Loading

codecov-io commented Jun 17, 2019 •

edited

Loading

igormcoelho Jun 17, 2019 •

edited

Loading

igormcoelho commented Jun 17, 2019 •

edited

Loading

igormcoelho commented Jul 7, 2019 •

edited

Loading

igormcoelho commented Jul 11, 2019 •

edited

Loading

lock9 commented Jul 11, 2019 •

edited

Loading

lock9 left a comment •

edited

Loading

igormcoelho commented Jul 12, 2019 •

edited

Loading

igormcoelho commented Jul 12, 2019 •

edited

Loading

shargon commented Aug 6, 2019 •

edited

Loading

shargon commented Sep 12, 2019 •

edited

Loading

igormcoelho commented Sep 17, 2019 •

edited

Loading