Verification and Gas control by shargon · Pull Request #790 · neo-project/neo

shargon · 2019-05-29T09:44:00Z

Close #789

shargon · 2019-05-29T09:45:19Z

neo/SmartContract/ApplicationEngine.cs


        public TriggerType Trigger { get; }
        public IVerifiable ScriptContainer { get; }
+        public IExecutionControl Control { get; }


This could be in neo-vm by default

codecov-io · 2019-05-29T09:47:39Z

Codecov Report

Merging #790 into master will increase coverage by <.01%.
The diff coverage is 43.75%.

@@            Coverage Diff             @@
##           master     #790      +/-   ##
==========================================
+ Coverage   37.74%   37.74%   +<.01%     
==========================================
  Files         176      178       +2     
  Lines       12442    12465      +23     
==========================================
+ Hits         4696     4705       +9     
- Misses       7746     7760      +14

Impacted Files	Coverage Δ
neo/SmartContract/GasControl.OpCodePrices.cs	`100% <ø> (ø)`
neo/Network/P2P/Payloads/Transaction.cs	`56.83% <0%> (ø)`	⬆️
neo/Wallets/Wallet.cs	`5.18% <0%> (ø)`	⬆️
neo/SmartContract/Native/NativeContract.cs	`85.26% <0%> (ø)`	⬆️
neo/SmartContract/Helper.cs	`43.5% <0%> (ø)`	⬆️
neo/Network/RPC/RpcServer.cs	`0% <0%> (ø)`	⬆️
neo/SmartContract/VerificationControl.cs	`0% <0%> (ø)`
neo/SmartContract/GasControl.cs	`100% <100%> (ø)`
neo/Ledger/Blockchain.cs	`36.81% <33.33%> (ø)`	⬆️
neo/Ledger/Blockchain.ApplicationExecuted.cs	`93.33% <50%> (-6.67%)`	⬇️
... and 3 more

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 9b11d15...0c7c3a4. Read the comment docs.

erikzhang · 2019-05-29T10:02:33Z

What problem does this refactoring solve?

shargon · 2019-05-29T11:13:04Z

We are able to ban opcodes in verification, and max amount of instructions to execute

neo/neo/SmartContract/VerificationControl.cs

Lines 6 to 23 in ca8de05

    
           public class VerificationControl : GasControl 
        
           { 
        
               private readonly static List<OpCode> _bannedOpcodes = new List<OpCode>(new OpCode[] { /*OpCode.NOP*/ }); 
        
               public int MaxStepInto { get; private set; } 
        
               public VerificationControl(int maxSteps, long gas) : base(gas, false) 
        
               { 
        
                   MaxStepInto = maxSteps; 
        
               } 
        
               public override bool OnPreExecute(OpCode opCode) 
        
               { 
        
                   if (!base.OnPreExecute(opCode)) return false; 
        
                   MaxStepInto--; 
        
                   return MaxStepInto > 0 && !_bannedOpcodes.Contains(opCode);

https://github.com/neo-project/neo/pull/790/files#diff-2753610b8764e06ef8ead3333db9b7aaR270

erikzhang · 2019-05-29T12:01:10Z

I think limit the max steps is useless. Limiting the max gas is enough.

shargon · 2019-05-29T12:05:54Z

And the opcodes, like syscalls?

erikzhang · 2019-05-29T12:08:46Z

Without SYSCALL, the verification script can't verify the signatures. There is no reason to limit it.

igormcoelho · 2019-05-29T14:20:47Z

neo/SmartContract/VerificationControl.cs

+{
+    public class VerificationControl : GasControl
+    {
+        private readonly static List<OpCode> _bannedOpcodes = new List<OpCode>(new OpCode[] { /*OpCode.NOP*/ });


Its in a good direction shargon, however needs a little more complication here. To ban backwards jumps we need to analyse the code and its first parameter... so, a callback returning true/false is better than a static list.
I've been thinking and another option would be to have different JMPF (jump forward) that can only jump forward. If we do this, its easier to ban. And create 3 more: JMPF, JMPFIF and JMPFIFNOT . A callback can avoid this and give more control on used opcodes.

This gonna be an interesting thing!

Do you want to handle these bannedOpcodes in this PR or split in two?

@igormcoelho

vncoelho · 2019-05-29T16:41:05Z

I really do not have the perfect understanding that you @igormcoelho, @lightszero, @erikzhang and @shargon have been sharing over these topics.

However, biased or not, I am in favor of things that my brother @igormcoelho proposes, related to not being turing complete and remove the jumps in some special cases.
We could have both systems working together. Those who want limited execution and those who do not want it.

After that, we should also please come back to that discussion about op_eval. Let's turn it to a SYSCALL as well and allow us to play with this! 💃

vncoelho · 2019-05-29T17:34:03Z

neo/SmartContract/GasControl.cs

+
+        public long GasConsumed { get; private set; } = 0;
+
+        public GasControl(long gas = 0, bool testMode = false)


Perfect, perfect, @shargon.

The refactor is precise!
It is much more readable.

I agree with it.

The other discussions out of the refactor and the class could be moved to other PR.

Maybe we can rename GasControl -> GasManager or GasTuringCompleteManager or GasTuringIncompleteManager...aehuaheuaheua
Turing is just for fun. Maybe GasManager or GasLimitsManager

vncoelho · 2019-06-11T20:19:34Z

Can we move with this PR? It has a nice refactor, maybe split this or leave this template and later adjust.

vncoelho · 2019-06-23T14:31:49Z

@shargon, maybe split this PR in Refactoring and the feature that closes #789.

vncoelho · 2019-07-11T22:05:03Z

ping @shargon...ahueahuea

shargon · 2019-07-19T16:15:24Z

If all are agree i can fix the conflicts otherwise i will close it. Now i think that is not needed because we only check gas

erikzhang · 2019-07-22T08:05:36Z

I think it's not needed anymore.

* release-2.10.0 * Update v2.10.0.md (neo-project#785) * Update v2.10.0.md * Update v2.10.0.md * Update and rename v2.10.0.md to v2.10.0 * Update v2.10.0 * update exchange * Introduction for gas related rpc (neo-project#788) Introduction for gas related rpc * add plugin * Update v2.10.0.md * update English files

Verification and Gas control

ca8de05

shargon requested review from erikzhang, igormcoelho and vncoelho May 29, 2019 09:44

shargon commented May 29, 2019

View reviewed changes

igormcoelho reviewed May 29, 2019

View reviewed changes

shargon mentioned this pull request May 29, 2019

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

Closed

vncoelho reviewed May 29, 2019

View reviewed changes

Merge branch 'master' into verification-control

5cea230

Merge branch 'master' into verification-control

0c7c3a4

shargon closed this Jul 22, 2019

shargon deleted the verification-control branch July 22, 2019 09:34


		public long GasConsumed { get; private set; } = 0;

		public GasControl(long gas = 0, bool testMode = false)

Conversation

shargon commented May 29, 2019

Uh oh!

shargon May 29, 2019

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

erikzhang commented May 29, 2019

Uh oh!

shargon commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erikzhang commented May 29, 2019

Uh oh!

shargon commented May 29, 2019

Uh oh!

erikzhang commented May 29, 2019

Uh oh!

igormcoelho May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vncoelho May 29, 2019

Choose a reason for hiding this comment

Uh oh!

vncoelho May 29, 2019

Choose a reason for hiding this comment

Uh oh!

vncoelho Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

vncoelho commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vncoelho May 29, 2019

Choose a reason for hiding this comment

Uh oh!

vncoelho commented Jun 11, 2019

Uh oh!

vncoelho commented Jun 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vncoelho commented Jul 11, 2019

Uh oh!

shargon commented Jul 19, 2019

Uh oh!

erikzhang commented Jul 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented May 29, 2019 •

edited

Loading

shargon commented May 29, 2019 •

edited

Loading

igormcoelho May 29, 2019 •

edited

Loading

vncoelho commented May 29, 2019 •

edited

Loading

vncoelho commented Jun 23, 2019 •

edited

Loading