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

Invalid scientific JNumbers handling #2879

Closed
AnnaShaleva opened this issue Jul 28, 2023 · 6 comments · Fixed by #2883
Closed

Invalid scientific JNumbers handling #2879

AnnaShaleva opened this issue Jul 28, 2023 · 6 comments · Fixed by #2883

Comments

@AnnaShaleva
Copy link
Member

Describe the bug

The first problem
It's possible to pass some big number to a contract via JSON-serialized number and then deserialize it into Integer stackitem. For example, contract RealtimePriceOracle with 0xbb054aaf5466e1bc49876c9eac747eddbac2ab6f hash deployed on T5 testnet has method updatePrice which accepts serialized set of prices. This method was invoked at block 2336911 of T5 testnet which caused the NeoGo and NeoC# nodes state diff (nspcc-dev/neo-go#3069). C# node successfuly handled the call while Go node failed to parse one of the provided JNumbers (2.8e+22):

anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ ./bin/neo-go query tx 56df5d07407b346254268726c108a942e4e6918d7b0e42d7cb28195b8f7665a4 -r https://rpc.t5.n3.nspcc.ru:20331 -v
Hash:			56df5d07407b346254268726c108a942e4e6918d7b0e42d7cb28195b8f7665a4
OnChain:		true
BlockHash:		613e28cf0d390c5336a8a23674022645d1fe5ba7965ad8fd808c481809f176aa
Success:		false
Signer:			NMQUeV1hPhNCM4k8x8dXFA25Y8oiDER8yZ (CalledByEntry)
SystemFee:		0.03046441 GAS
NetworkFee:		0.00143452 GAS
Script:			DEDl+aEQaRHIBluz4kOvUnohvL18GxJsZ8pcIGZc+yeOoP/vz5KM2uAV0C/Kyip42yQWlW+iWVXyX5O/13gwh/I1EcAMIQI3Q0to88Qvxv82bpGWEPmmjbsyesk4dtdqegUcoK8nZhHADI17InRva2VucyI6WyIweDg1ZGVhYzUwZmViZmQ5Mzk4OGQzZjM5MWRlYTU0ZTgyODllNDNlOWUiLCIweGQyYTRjZmYzMTkxMzAxNjE1NWUzOGU0NzRhMmMwNmQwOGJlMjc2Y2YiXSwicHJpY2VzIjpbMTAwMDAwMDAwMDAwMDAwMDAwMCwyLjhlKzIyXX0TwB8MC3VwZGF0ZVByaWNlDBRvq8K63X50rJ5sh0m84WZUr0oFu0FifVtS
INDEX    OPCODE       PARAMETER
0        PUSHDATA1    e5f9a1106911c8065bb3e243af527a21bcbd7c1b126c67ca5c20665cfb278ea0ffefcf928cdae015d02fcaca2a78db2416956fa25955f25f93bfd7783087f235    <<
66       PUSH1        
67       PACK         
68       PUSHDATA1    0237434b68f3c42fc6ff366e919610f9a68dbb327ac93876d76a7a051ca0af2766
103      PUSH1        
104      PACK         
105      PUSHDATA1    7b22746f6b656e73223a5b22307838356465616335306665626664393339383864336633393164656135346538323839653433653965222c22307864326134636666333139313330313631353565333865343734613263303664303862653237366366225d2c22707269636573223a5b313030303030303030303030303030303030302c322e38652b32325d7d ("{\"tokens\":[\"0x85deac50febfd93988d3f391dea54e8289e43e9e\",\"0xd2a4cff31913016155e38e474a2c06d08be276cf\"],\"prices\":[1000000000000000000,2.8e+22]}")
248      PUSH3        
249      PACK         
250      PUSH15       
251      PUSHDATA1    7570646174655072696365 ("updatePrice")
264      PUSHDATA1    6fabc2badd7e74ac9e6c8749bce16654af4a05bb
286      SYSCALL      System.Contract.Call (627d5b52)
Exception:		at instruction 78 (SYSCALL): invalid value (real value for int)

After that, we have fixed our JNumbers parsing and allowed to parse scientific numbers like 2.8e+22 in nspcc-dev/neo-go#3073. After this fix, NeoGo state for block 2336911 matches the NeoC# node state which is OK.

However, the new state difference appears after this fix at block 2342940@T5 due to the wollowing transaction processing:

anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ ./bin/neo-go query tx 4c9e33b29bb75ae2a77e180f82879195fe541afa04aabe0e4a1a1ec858629938 -r http://localhost:20332 -v
Hash:			4c9e33b29bb75ae2a77e180f82879195fe541afa04aabe0e4a1a1ec858629938
OnChain:		true
BlockHash:		254985ed16cb865742971a93b16aa6888aa20ccc7082d56f9e59d2e53ca151ea
Success:		true
Signer:			NMQUeV1hPhNCM4k8x8dXFA25Y8oiDER8yZ (CalledByEntry)
SystemFee:		0.03349697 GAS
NetworkFee:		0.00132852 GAS
Script:			DEBeQh5b5nPV+E55v8+REMpGUqYjEioobzZQ6QSbLQTmCuLHMBJEdCEGkml/wSeg3HT+YKInSL5D0l6M72Kas6XBEcAMIQI3Q0to88Qvxv82bpGWEPmmjbsyesk4dtdqegUcoK8nZhHADCVbOS4wNWUrMjgsMS44NzFlKzIxLDMuMDM2NmUrMzIsMWUrMzBdE8AfDAt1cGRhdGVQcmljZQwUb6vCut1+dKyebIdJvOFmVK9KBbtBYn1bUg==
INDEX    OPCODE       PARAMETER
0        PUSHDATA1    5e421e5be673d5f84e79bfcf9110ca4652a623122a286f3650e9049b2d04e60ae2c730124474210692697fc127a0dc74fe60a22748be43d25e8cef629ab3a5c1    <<
66       PUSH1        
67       PACK         
68       PUSHDATA1    0237434b68f3c42fc6ff366e919610f9a68dbb327ac93876d76a7a051ca0af2766
103      PUSH1        
104      PACK         
105      PUSHDATA1    5b392e3035652b32382c312e383731652b32312c332e30333636652b33322c31652b33305d ("[9.05e+28,1.871e+21,3.0366e+32,1e+30]")
144      PUSH3        
145      PACK         
146      PUSH15       
147      PUSHDATA1    7570646174655072696365 ("updatePrice")
160      PUSHDATA1    6fabc2badd7e74ac9e6c8749bce16654af4a05bb
182      SYSCALL      System.Contract.Call (627d5b52)

As you can see, the same updatePrice method is called with the following set of arguments: [9.05e+28,1.871e+21,3.0366e+32,1e+30]. The resulting stack after the invocation (which matches the price values stored in the contract storage) for the C# node is:

anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ curl -d '{ "jsonrpc": "2.0", "id": 1, "method": "getapplicationlog", "params": ["4c9e33b29bb75ae2a77e180f82879195fe541afa04aabe0e4a1a1ec858629938"] }' seed1t5.neo.org:20332 | json_pp
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   641    0   501  100   140    284     79  0:00:01  0:00:01 --:--:--   363
{
   "result" : {
      "executions" : [
         {
            "vmstate" : "HALT",
            "trigger" : "Application",
            "stack" : [
               {
                  "value" : [
                     {
                        "type" : "Integer",
                        "value" : "90499999999999993918259200000"
                     },
                     {
                        "value" : "1871000000000000000000",
                        "type" : "Integer"
                     },
                     {
                        "value" : "303660000000000004445016810323968",
                        "type" : "Integer"
                     },
                     {
                        "type" : "Integer",
                        "value" : "1000000000000000019884624838656"
                     }
                  ],
                  "type" : "Array"
               }
            ],
            "gasconsumed" : "3316532",
            "notifications" : [],
            "exception" : null
         }
      ],
      "txid" : "0x4c9e33b29bb75ae2a77e180f82879195fe541afa04aabe0e4a1a1ec858629938"
   },
   "id" : 1,
   "jsonrpc" : "2.0"
}

And the resulting stack of the same invocation for NeoGo node is:

anna@kiwi:~/Documents/GitProjects/nspcc-dev/neo-go$ curl -d '{ "jsonrpc": "2.0", "id": 1, "method": "getapplicationlog", "params": ["4c9e33b29bb75ae2a77e180f82879195fe541afa04aabe0e4a1a1ec858629938"] }' http://localhost:20332 | json_pp
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   642  100   502  100   140   163k  46666 --:--:-- --:--:-- --:--:--  208k
{
   "result" : {
      "txid" : "0x4c9e33b29bb75ae2a77e180f82879195fe541afa04aabe0e4a1a1ec858629938",
      "executions" : [
         {
            "vmstate" : "HALT",
            "exception" : null,
            "notifications" : [],
            "trigger" : "Application",
            "gasconsumed" : "3316532",
            "stack" : [
               {
                  "value" : [
                     {
                        "type" : "Integer",
                        "value" : "90500000000000000000000000000"
                     },
                     {
                        "type" : "Integer",
                        "value" : "1871000000000000000000"
                     },
                     {
                        "type" : "Integer",
                        "value" : "303660000000000000000000000000000"
                     },
                     {
                        "value" : "1000000000000000000000000000000",
                        "type" : "Integer"
                     }
                  ],
                  "type" : "Array"
               }
            ]
         }
      ]
   },
   "id" : 1,
   "jsonrpc" : "2.0"
}

As you can see, the C# node stores the provided scientific numbers as floating point numbers and e.g. the 9.05e+28 input is being converted to "90499999999999993918259200000" instead of the desired 90500000000000000000000000000. And the NeoGo node stores the provided scientific values as solid integers up to some precision. The maximum precision allowed for this "solid" parsing is restricted by big.MaxPrec setting we use in NeoGo while deserializeing these values. Thus, I consider there's some inaccuracy while processing the scientific numbers in the NeoC# node. We should probably consider restricting the maximum number that is allowed to be parsed from JSON and we should be sure that numbers under this constraint can be parsed as solid integers.

The second problem
The second problem is that NeoC# node allows to parse floating-point numbers in a scientific notation. For examle, from a couple of following tests the first one will pass (which is OK), and the second one will also pass (which is not OK, because it's a floating point number in fact and this test should throw the FormatException):

            JObject.Parse("100.123e+3").AsString().Should().Be("100123");
            JObject.Parse("100.123e+2").AsString().Should().Be("10012.3");

To Reproduce
Steps to reproduce the behavior:
See the related NeoGo statediffs issue and PR: nspcc-dev/neo-go#3069, nspcc-dev/neo-go#3073.

Expected behavior
For the first problem, we have to provide the ability to parse from JSON exactly those numbers as were provided by the user in scientific notation up to some maximum precision value. I consider that we should forbid parsing numbers that are bigger than this max precision value.

For the second problem we have to forbit parsing floating-point numbers from scientific notation, like it is done for simple floating-point numbers.

Platform:

  • Version: Neo v3.5.0

Additional context
However, we may consider disallowing scientific numbers parsing at all. It's a point of discussion.

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this issue Jul 31, 2023
52-bit precision is not enough for our 256-bit VM, but this value
matches the reference implementation, see the
neo-project/neo#2879.

MaxIntegerPrec will be increased (or even removed) as soon as the
ref. issue is resolved.
@roman-khimov
Copy link
Contributor

Related to #2498, btw.

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this issue Jul 31, 2023
52-bit precision is not enough for our 256-bit VM, but this value
matches the reference implementation, see the
neo-project/neo#2879.

MaxIntegerPrec will be increased (or even removed) as soon as the
ref. issue is resolved.
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this issue Jul 31, 2023
52-bit precision is not enough for our 256-bit VM, but this value
matches the reference implementation, see the
neo-project/neo#2879.

MaxIntegerPrec will be increased (or even removed) as soon as the
ref. issue is resolved.
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this issue Jul 31, 2023
52-bit precision is not enough for our 256-bit VM, but this value
matches the reference implementation, see the
neo-project/neo#2879.

MaxIntegerPrec will be increased (or even removed) as soon as the
ref. issue is resolved.
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this issue Jul 31, 2023
52-bit precision is not enough for our 256-bit VM, but this value
matches the reference implementation, see the
neo-project/neo#2879.

MaxIntegerPrec will be increased (or even removed) as soon as the
ref. issue is resolved.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@Jim8y
Copy link
Contributor

Jim8y commented Aug 1, 2023

for the first problem, had tested, still could not locate the problem, will keep trying.

@Jim8y
Copy link
Contributor

Jim8y commented Aug 1, 2023

Test result in #2882 might indicate that floating-point is OK

@AnnaShaleva
Copy link
Member Author

AnnaShaleva commented Aug 1, 2023

Well, the first problem is not actually a floating-point problem, it's a problem of parsing big numbers with some restricted predefined precision which gives the side-effects of 9.05e+28 -> 90499999999999993918259200000. Take a look at the nspcc-dev/neo-go#3073 (comment), we've set MaxPrec to 53 to match the C# node result, but this value has to be increased up to 257, becaues we have 256-bit VM.

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this issue Aug 1, 2023
52-bit precision is not enough for our 256-bit VM, but this value
matches the reference implementation, see the
neo-project/neo#2879.

MaxIntegerPrec will be increased (or even removed) as soon as the
ref. issue is resolved.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@Ashuaidehao
Copy link
Contributor

The first problem seems caused by double's precision, since almost all number will convert to double first then to BigInteger.

            var num1 = 9.05e+28;
            var num2 = (BigInteger)9.05e+28;
            Console.WriteLine($"num double:{num1}");
            Console.WriteLine($"num double to BigInteger:{num2}");

output:

num double:9.05E+28
num double to BigInteger:90499999999999993918259200000

@Jim8y
Copy link
Contributor

Jim8y commented Aug 2, 2023

@Ashuaidehao U r right, need to use BigInteger.Parse(scientificNotation, System.Globalization.NumberStyles.Float); instead.

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

Successfully merging a pull request may close this issue.

4 participants