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.

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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
129 changes: 129 additions & 0 deletions neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1069,5 +1069,134 @@ public void ToJson()
jObj["script"].AsString().Should().Be("4220202020202020202020202020202020202020202020202020202020202020");
jObj["sys_fee"].AsString().Should().Be("4200000000");
}

[TestMethod]
public void Infinite_Loop_Not_Allowed_On_Application()
{
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.

}

[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 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.

}

[TestMethod]
public void Recursion_Not_Allowed_On_Application()
{
// example for recursion (no local variables or parameters)
/*
00 11: PUSH0 #An empty array of bytes is pushed onto the stack
c5 12: NEWARRAY #
6b 13: TOALTSTACK # Puts the input onto the top of the alt stack. Removes it from the main stack.
61 14: NOP # Does nothing.
65 15: CALL fcff # -4
61 18: NOP # Does nothing.
6c 19: FROMALTSTACK # Puts the input onto the top of the main stack. Removes it from the alt stack.
75 20: DROP # Removes the top stack item.
66 21: RET #
*/

byte[] script;
using (ScriptBuilder sb = new ScriptBuilder())
{
sb.Emit(OpCode.CALL, 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, 22528000);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I thin that is good, the limit is not raised

image

}

[TestMethod]
public void Recursion_Not_Allowed_On_Verification()
{
// example for recursion (no local variables or parameters)
/*
00 11: PUSH0 #An empty array of bytes is pushed onto the stack
c5 12: NEWARRAY #
6b 13: TOALTSTACK # Puts the input onto the top of the alt stack. Removes it from the main stack.
61 14: NOP # Does nothing.
65 15: CALL fcff # -4
61 18: NOP # Does nothing.
6c 19: FROMALTSTACK # Puts the input onto the top of the main stack. Removes it from the alt stack.
75 20: DROP # Removes the top stack item.
66 21: RET #
*/

byte[] script;
using (ScriptBuilder sb = new ScriptBuilder())
{
sb.Emit(OpCode.CALL, new byte[] { 0, 0, 0, 0 });
script = sb.ToArray();
}

long netfee = 100000000; // network fee

// 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 should be spent (recursion is not allowed)
// TODO: fix recursion
Assert.AreEqual(verificationGas, 22528000);
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 should be zero (or near zero), as long as we block recursive behavior. Perhaps stack context count is enough... not sure.

}
}
}
25 changes: 24 additions & 1 deletion neo/SmartContract/ApplicationEngine.cs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,30 @@ protected override bool PreExecuteInstruction()
{
if (CurrentContext.InstructionPointer >= CurrentContext.Script.Length)
return true;
return AddGas(OpCodePrices[CurrentContext.CurrentInstruction.OpCode]);

Instruction instruction = CurrentContext.CurrentInstruction;

if (Trigger == TriggerType.Verification)
{
// no backwards jump in verification mode

switch (instruction.OpCode)
{
case OpCode.JMP:
case OpCode.JMPIF:
case OpCode.JMPIFNOT:
//case OpCode.CALL:
{
if (instruction.TokenI16 <= 0)
{
return false;
}
break;
}
}
}

return AddGas(OpCodePrices[instruction.OpCode]);
}

private static Block CreateDummyBlock(Snapshot snapshot)
Expand Down