Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mode to disable backward jumps on Neo AppEngine 3 #837

Closed
wants to merge 18 commits into from
Closed

Mode to disable backward jumps on Neo AppEngine 3 #837

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
5cdd28e
Mode to disable backward jumps on Neo AppEngine 3
igormcoelho Jun 17, 2019
0aafb26
Merge branch 'master' into feat_3x_neoapp_limited_mode
shargon Jun 17, 2019
668c160
Merge branch 'master' into feat_3x_neoapp_limited_mode
vncoelho Jul 5, 2019
c75b4e9
Merge branch 'master' into feat_3x_neoapp_limited_mode
lock9 Jul 23, 2019
e0ad7ef
Merge branch 'master' into feat_3x_neoapp_limited_mode
shargon Aug 6, 2019
bb1731d
Merge branch 'master' of https://github.com/neo-project/neo into feat…
igormcoelho Aug 11, 2019
75cf115
basic tests, no jumps on verification
igormcoelho Aug 11, 2019
8ee60a2
recursion not blocked yet
igormcoelho Aug 11, 2019
39215b6
Merge branch 'master' into feat_3x_neoapp_limited_mode
igormcoelho Aug 15, 2019
8688f38
Optimization
shargon Aug 16, 2019
e896027
Reuse var
shargon Aug 16, 2019
b1cc6a7
Update neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs
igormcoelho Aug 17, 2019
b72e8f9
Merge branch 'master' into feat_3x_neoapp_limited_mode
shargon Aug 18, 2019
3381b5e
Split unit tests
shargon Aug 18, 2019
97330bd
Merge remote-tracking branch 'igormcoelho/feat_3x_neoapp_limited_mode…
shargon Aug 18, 2019
d1c03c0
Merge branch 'master' into feat_3x_neoapp_limited_mode
shargon Aug 25, 2019
13ba5b0
Merge branch 'master' into feat_3x_neoapp_limited_mode
lock9 Sep 11, 2019
93f4734
Merge branch 'master' into feat_3x_neoapp_limited_mode
lock9 Sep 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,5 +275,42 @@ public void ToJson()
jObj["script"].AsString().Should().Be("4220202020202020202020202020202020202020202020202020202020202020");
jObj["sys_fee"].AsNumber().Should().Be(42);
}

[TestMethod]
public void Infinite_Loop_Not_Allowed_On_Verification()
{
byte[] script;
using (ScriptBuilder sb = new ScriptBuilder())
{
sb.Emit(OpCode.JMP, new byte[] { 0, 0, 0, 0 });
script = sb.ToArray();
}

long netfee = 100000000; // network fee

// Check Application
long appGas = 0;
using (ApplicationEngine engine = new ApplicationEngine(TriggerType.Application, null, null, netfee, false))
{
engine.LoadScript(script);
Assert.AreEqual(VMState.FAULT, engine.Execute());
Assert.AreEqual(0, engine.ResultStack.Count);
appGas += engine.GasConsumed;
}
// many gas are spent before fault
Assert.AreEqual(appGas, 100000040);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


// Check Verification
long verificationGas = 0;
using (ApplicationEngine engine = new ApplicationEngine(TriggerType.Verification, null, null, netfee, false))
{
engine.LoadScript(script);
Assert.AreEqual(VMState.FAULT, engine.Execute());
Assert.AreEqual(0, engine.ResultStack.Count);
verificationGas += engine.GasConsumed;
}
// no gas is spent (backwards jump is not allowed)
Assert.AreEqual(verificationGas, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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).

Copy link
Member

Choose a reason for hiding this comment

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

How can we prevent the issue with CALLS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}
}
11 changes: 4 additions & 7 deletions neo/SmartContract/ApplicationEngine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ public partial class ApplicationEngine : ExecutionEngine
public const long GasFree = 0;
private readonly long gas_amount;
private readonly bool testMode;
// limited mode (disallow backward jumps and recursive calls)
private readonly bool isLimited;
private readonly RandomAccessStack<UInt160> hashes = new RandomAccessStack<UInt160>();
private readonly List<NotifyEventArgs> notifications = new List<NotifyEventArgs>();
private readonly List<IDisposable> disposables = new List<IDisposable>();
Expand All @@ -31,11 +29,10 @@ public partial class ApplicationEngine : ExecutionEngine
public IReadOnlyList<NotifyEventArgs> Notifications => notifications;
internal Dictionary<UInt160, int> InvocationCounter { get; } = new Dictionary<UInt160, int>();

public ApplicationEngine(TriggerType trigger, IVerifiable container, Snapshot snapshot, long gas, bool testMode = false, bool isLimited = false)
public ApplicationEngine(TriggerType trigger, IVerifiable container, Snapshot snapshot, long gas, bool testMode = false)
{
this.gas_amount = GasFree + gas;
this.testMode = testMode;
this.isLimited = isLimited;
this.Trigger = trigger;
this.ScriptContainer = container;
this.Snapshot = snapshot;
Expand Down Expand Up @@ -91,11 +88,11 @@ protected override bool PreExecuteInstruction()
case OpCode.JMP:
case OpCode.JMPIF:
case OpCode.JMPIFNOT:
case OpCode.CALL:
//case OpCode.CALL:
{
if(isLimited)
if(Trigger == TriggerType.Verification)
{
// no backwards jump in limited mode
// no backwards jump in verification mode
if(instruction.TokenI16 <= 0)
{
return false;
Expand Down