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

AllowOverflow option to avoid checking int range #1205

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

Hecate2
Copy link
Contributor

@Hecate2 Hecate2 commented Oct 16, 2024

unchecked((int)bigInteger))
Will conflict. Draft for now.

@Hecate2 Hecate2 changed the title use unchecked to skip range checks use unchecked to skip range checks from BigInteger to smaller types Oct 16, 2024
@Hecate2 Hecate2 changed the title use unchecked to skip range checks from BigInteger to smaller types SimulateOverflow option to avoid checking int range Oct 25, 2024
@Hecate2
Copy link
Contributor Author

Hecate2 commented Oct 25, 2024

Changed the methodology. Skip range checks and throws with compilation option

@Hecate2
Copy link
Contributor Author

Hecate2 commented Oct 25, 2024

Need a way to compile neo-devpack-dotnet\tests\Neo.Compiler.CSharp.TestContracts\Contract_BigInteger.cs with SimulateOverflow=true

@Hecate2
Copy link
Contributor Author

Hecate2 commented Oct 25, 2024

Need a way to compile neo-devpack-dotnet\tests\Neo.Compiler.CSharp.TestContracts\Contract_BigInteger.cs with SimulateOverflow=true

Allowed CompilationOptions to be used in tests.
TestGasConsume is set to false.

@Hecate2 Hecate2 marked this pull request as ready for review October 25, 2024 05:53
Jim8y and others added 2 commits October 26, 2024 01:08
…to unchecked

# Conflicts:
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Array.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_BigInteger.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Delegate.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Foreach.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Inline.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Lambda.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Linq.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_NullableType.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Out.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Params.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_PostfixUnary.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Record.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Returns.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_StaticVar.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Switch.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_TryCatch.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_shift.cs
#	tests/Neo.Compiler.CSharp.UnitTests/UnitTest_NullableType.cs
#	tests/Neo.SmartContract.Framework.UnitTests/TestingArtifacts/Contract_List.cs
#	tests/Neo.SmartContract.Framework.UnitTests/TestingArtifacts/Contract_Map.cs
#	tests/Neo.SmartContract.Framework.UnitTests/TestingArtifacts/Contract_Regex.cs
#	tests/Neo.SmartContract.Framework.UnitTests/TestingArtifacts/Contract_Runtime.cs
@Hecate2 Hecate2 changed the title SimulateOverflow option to avoid checking int range AllowOverflow option to avoid checking int range Oct 28, 2024
@Jim8y
Copy link
Contributor

Jim8y commented Oct 29, 2024

This name is so scary....

@Jim8y
Copy link
Contributor

Jim8y commented Oct 29, 2024

this can save tons of gas, yes,,,,,, but,,,,,,,

@Jim8y
Copy link
Contributor

Jim8y commented Oct 29, 2024

rather than allow overflow, i think biginteger only is better

@shargon
Copy link
Member

shargon commented Oct 29, 2024

We should not save gas if the security is involved

@shargon shargon closed this Oct 29, 2024
@shargon shargon reopened this Oct 29, 2024
@shargon
Copy link
Member

shargon commented Oct 29, 2024

Sorry miss click

@Hecate2
Copy link
Contributor Author

Hecate2 commented Oct 30, 2024

We should not save gas if the security is involved

No security problem is involved. This is just handling all int, long, short as BigInteger.

@Hecate2
Copy link
Contributor Author

Hecate2 commented Oct 30, 2024

rather than allow overflow, i think biginteger only is better

Sometimes it's impossible to write BigInteger everywhere literally in codes. For example, shifting operations (>> and <<) in C# supports only int as the shifted count of bits. Also indexes of arrays have to be int. Then we have to write an explicit coversion (int) which wastes a range check.

@Hecate2
Copy link
Contributor Author

Hecate2 commented Oct 30, 2024

And our current default settings (with or without my PR) are --checked=false (--allow-overflow=false). This allows silent overflows (from int.MaxValue to int.MinValue, without or without my PR). I think this is even more dangerous. Maybe it's better to have default as --checked=false --allow-overflow=true, or --checked=true --allow-overflow=false

@shargon
Copy link
Member

shargon commented Oct 30, 2024

we should detect unchecked keyword

Hecate2 and others added 7 commits November 1, 2024 11:12
…to unchecked

# Conflicts:
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Out.cs
…to unchecked

# Conflicts:
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Polymorphism.cs
…to unchecked

# Conflicts:
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_GoTo.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Initializer.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_TryCatch.cs
#	tests/Neo.Compiler.CSharp.UnitTests/UnitTest_GoTo.cs
# Conflicts:
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Inline.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Linq.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Record.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Switch.cs
@Jim8y
Copy link
Contributor

Jim8y commented Nov 13, 2024

@shargon please review

@Jim8y
Copy link
Contributor

Jim8y commented Nov 13, 2024

This is an option that should be false by default, so after you @shargon reviewed, please dont merge it, i will update the test to remove the option and revert gas change.

@shargon
Copy link
Member

shargon commented Nov 13, 2024

I still don't know why we need this option, which is the use case?

@Jim8y
Copy link
Contributor

Jim8y commented Nov 14, 2024

I still don't know why we need this option, which is the use case?

where user can handle the integer range and no need to do automatic range check. currently, every numerous operation requires a range check, its expensive.

@shargon
Copy link
Member

shargon commented Nov 14, 2024

I still don't know why we need this option, which is the use case?

where user can handle the integer range and no need to do automatic range check. currently, every numerous operation requires a range check, its expensive.

expensive not because it's required for safety

{
AllowOverflow = true,
Copy link
Member

Choose a reason for hiding this comment

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

We should leave the test as before, only create one test for this specific case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

…to unchecked

# Conflicts:
#	tests/Neo.Compiler.CSharp.UnitTests/TestCleanup.cs
#	tests/Neo.Compiler.CSharp.UnitTests/UnitTest_Linq.cs
#	tests/Neo.Compiler.CSharp.UnitTests/UnitTest_Record.cs
…o unchecked

# Conflicts:
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_GoTo.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Initializer.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Inline.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Instance.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Linq.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Polymorphism.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Record.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_StaticClass.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Switch.cs
#	tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_TryCatch.cs
@Jim8y Jim8y requested a review from shargon November 21, 2024 03:31
@shargon
Copy link
Member

shargon commented Nov 21, 2024

Can we discuss this pe in the meeting?

@Hecate2
Copy link
Contributor Author

Hecate2 commented Nov 21, 2024

Can we discuss this pe in the meeting?

That's really great

@Jim8y
Copy link
Contributor

Jim8y commented Nov 21, 2024

gas examples to demonstrate gas saving. /or use biginteger as much as possible

@Jim8y Jim8y self-assigned this Nov 21, 2024
@shargon
Copy link
Member

shargon commented Nov 21, 2024

Take a look to #1249

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants