BigInteger in Neo.Json #3037

shargon · 2023-12-27T09:10:33Z

shargon · 2023-12-27T09:40:18Z

During this PR an error was found, a number greater than MAX_SAFE_INTEGER or less than MIN_SAFE_INTEGER could be parsed before.

Open question: We want this restriction only for serialize?

Jim8y · 2023-12-27T10:36:37Z

This will change the behavior of existing contracts as well.

cschuchardt88 · 2023-12-27T11:58:58Z

I was expecting something other than the double type; for example all integer and floating point types. Why are we using double only anyways?

shargon · 2023-12-27T12:34:31Z

This will change the behavior of existing contracts as well.

Yes, we need a fork, but currently we allow integers out of the range

Jim8y · 2023-12-27T12:53:50Z

Yes, we need a fork, but currently we allow integers out of the range

Its not about a fork, it is that you have changed the behavior of existing contract. Which means the logic in existing contract may become invalid or different, and cause unexpected execution result.

cschuchardt88 · 2023-12-27T13:54:26Z

This PR isnt going to work seeing how big integers can be 256-bits. double isn't that big.

cschuchardt88 · 2023-12-27T14:07:13Z

tests/Neo.Json.UnitTests/UT_JNumber.cs

+        public void TestBigInteger()
+        {
+            ((JNumber)BigInteger.One).AsNumber().Should().Be(1);
+            ((JNumber)BigInteger.Zero).AsNumber().Should().Be(0);
+            ((JNumber)BigInteger.MinusOne).AsNumber().Should().Be(-1);
+        }


When doing testing, use max and min values please. You will see that using the max or min will fail.... double is to small to hold the data. BigInteger is 32 BYTES and double is 8 BYTES you get overflows. Its like doing

var failed = checked((int)long.MaxValue);

ping @shargon

shargon · 2023-12-27T14:46:50Z

Yes, we need a fork, but currently we allow integers out of the range

Its not about a fork, it is that you have changed the behavior of existing contract. Which means the logic in existing contract may become invalid or different, and cause unexpected execution result.

Any idea? currently we allow bigger values than expected

Jim8y · 2023-12-27T15:11:40Z

Any idea? currently we allow bigger values than expected

We need to track the version or the deploy or latest update block of contracts, if it is before your fix, it should run as before, otherwise, it can adopt new logic. Otherwise, a bug should be a bug,,,, we should not fix it.

cschuchardt88 · 2023-12-27T16:12:18Z

we should not fix it

What do you mean not fix it? The bug?

Jim8y · 2023-12-27T16:58:18Z

we should not fix it

What do you mean not fix it? The bug?

We should not fix a bug that is already there in the chain, at least not in a way that can change the behavior of existing contract. Cause our update may cause unexpected behavior of those contracts. Take this one for example, it changed the behavior of the deserialize interface, if a contract uses it to deserialize a very large number, it works in the past, so he just write the contract that way and core logics rely on the deserialize operation, what will happen when this pr is patched? his contract may fail, may also have who knows behavior cause he might have wrapped it in a try block.

cschuchardt88 · 2023-12-27T17:05:41Z

But take this in to count

Im sure no one has reached the max value yet, but see what can happen. so needs to be fixed.

Jim8y · 2023-12-28T01:48:00Z

Again, you dont fix it in a way that change the existing contract behavior. And you dont ensure someone has reached the maximum or not cause you dont know all contracts deployed. If you keep make changes like this, the neo project will be ruined one day cause you keep changing the existing contact behavior.

cschuchardt88 · 2023-12-28T08:28:29Z

You playing with people's potential money they have invested. This could be bad if a contract is doing StdLib.JsonSerialize and StdLib.JsonDeserialize for objects that deal with real funds or NFTs. Now developers use string as the json output, mostly. This will break oracle transactions, which are now bugged. I know this isn't a good solution, hints I said This PR isn't going to work.

This should of been developed correctly from the begin, the whole reason why we don't use floating points on the blockchain.

shargon · 2023-12-28T09:35:28Z

@Jim8y is right, a fork don't solve that the contract was not updated, maybe we need to store in the contract the last updated/deploy height, and use it as a fork height

Jim8y · 2023-12-28T09:41:40Z

You playing with people's potential money they have invested. This could be bad if a contract is doing StdLib.JsonSerialize and StdLib.JsonDeserialize for objects that deal with real funds or NFTs. Now developers use string as the json output, mostly. This will break oracle transactions, which are now bugged. I know this isn't a good solution, hints I said This PR isn't going to work.

This should of been developed correctly from the begin, the whole reason why we don't use floating points on the blockchain.

We are not playing around, it is a very serious problem. The very fundamental bottom line is we do not change the behavior of existing contracts, we must defence it firmly. Determinastic is one of the key feature of the blockchain, you break it, you break everything. I am not saying we should ignore it, but we should not fix it in a way that can potentially destroy the whole project. Reputation is the key in blokchain world, not a single project will want to exist in a chain that can change its contract's behavior arbitarily.

cschuchardt88 · 2023-12-28T09:47:43Z

I understand now. I didn't know what you meant by not fix it. Now I do. Thx for clearing that up.

vncoelho · 2023-12-28T10:20:04Z

@Jim8y is right, a fork don't solve that the contract was not updated, maybe we need to store in the contract the last updated/deploy height, and use it as a fork height

Perhaps this is a possible solution @shargon.
If a field is added to all deployed contracts and they keep calling the "legacy" version.
A mechanism can be created for those that want to remove this "legacy" flag.
Maybe "ContractManagement" could handle that.

cschuchardt88 · 2023-12-28T10:44:22Z

Let's not forget about oracle service as well. This would of affected any variable in the contract that they set a valid too.

roman-khimov · 2023-12-28T15:48:04Z

src/Neo/SmartContract/JsonSerializer.cs

@@ -50,10 +50,7 @@ public static JToken Serialize(StackItem item)
                    }
                case Integer num:
                    {
-                        var integer = num.GetInteger();
-                        if (integer > JNumber.MAX_SAFE_INTEGER || integer < JNumber.MIN_SAFE_INTEGER)


Hello, #2498.

He put it in the JNumber. Different exception, but will fail as well.

It was moved to the constructor, that was the bug, when we used parse we allowed it...

src/Neo.Json/JNumber.cs

tests/Neo.Json.UnitTests/UT_JNumber.cs

src/Neo.Json/JNumber.cs

Jim8y · 2024-02-15T09:25:19Z

@superboyiii, please check the state compatibility.

Jim8y · 2024-02-15T09:26:06Z

@shargon format not a thing i can update, please check it.

superboyiii · 2024-02-20T10:00:19Z

@shargon @Jim8y Something is incompatible. Blockchain stopped syncing at block 1528989 on mainnet.

vncoelho · 2024-02-22T15:53:58Z

src/Neo.Json/JNumber.cs

@@ -40,7 +41,13 @@ public class JNumber : JToken
        /// <param name="value">The value of the JSON token.</param>
        public JNumber(double value = 0)
        {
-            if (!double.IsFinite(value)) throw new FormatException();
+            if (!double.IsFinite(value))


verify this after the next 2?

Jim8y · 2024-06-01T01:20:40Z

@shargon @Jim8y Something is incompatible. Blockchain stopped syncing at block 1528989 on mainnet.

Please consider close this pr. @shargon

shargon · 2024-06-01T07:17:05Z

@shargon @Jim8y Something is incompatible. Blockchain stopped syncing at block 1528989 on mainnet.

Please consider close this pr. @shargon

But it can't be related to this pr

shargon added 7 commits December 27, 2023 10:10

Json BigInteger

6b31958

Fix Min

48c2649

Fix ut

186c2ba

add ut

eab5b25

Fix exception msg

776ab64

format

9ccb97d

Fix UT

458499b

shargon added the bug Used to tag confirmed bugs label Dec 27, 2023

cschuchardt88 reviewed Dec 27, 2023

View reviewed changes

roman-khimov reviewed Dec 28, 2023

View reviewed changes

shargon mentioned this pull request Jan 2, 2024

Add: add previous block state to the block header #3045

Closed

cschuchardt88 mentioned this pull request Jan 4, 2024

Add Json Serialization #2794

Closed

Jim8y added the pending-conflict label Feb 4, 2024

Merge branch 'master' into json-bi

edae719

Jim8y added waiting for review and removed pending-conflict labels Feb 5, 2024

cschuchardt88 reviewed Feb 5, 2024

View reviewed changes

src/Neo.Json/JNumber.cs Outdated Show resolved Hide resolved

Merge branch 'master' into json-bi

be973c9

Jim8y reviewed Feb 15, 2024

View reviewed changes

tests/Neo.Json.UnitTests/UT_JNumber.cs Show resolved Hide resolved

Jim8y reviewed Feb 15, 2024

View reviewed changes

src/Neo.Json/JNumber.cs Show resolved Hide resolved

Jim8y added 2 commits February 15, 2024 17:23

Update src/Neo.Json/JNumber.cs

308d33d

Update tests/Neo.Json.UnitTests/UT_JNumber.cs

19e3111

Jim8y requested a review from superboyiii February 15, 2024 09:25

shargon and others added 3 commits February 15, 2024 12:06

Fix format

1214697

Merge branch 'master' into json-bi

58ec583

Merge branch 'master' into json-bi

efa2c34

vncoelho reviewed Feb 22, 2024

View reviewed changes

cschuchardt88 added blocked This issue can't be worked at the moment and removed waiting for review labels May 16, 2024

This was referenced Aug 5, 2024

[Neo Json Fix] Json null tests #3450

Merged

Add hardofork HF_Echidna #3454

Draft

Jim8y changed the base branch from master to HF_Echidna August 7, 2024 16:28

Jim8y added the hardfork label Aug 7, 2024

cschuchardt88 mentioned this pull request Aug 17, 2024

[Neo Plugin RPCServer] Rpc parameters. Part I #3457

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BigInteger in Neo.Json #3037

BigInteger in Neo.Json #3037

shargon commented Dec 27, 2023

shargon commented Dec 27, 2023 •

edited

Loading

Jim8y commented Dec 27, 2023

cschuchardt88 commented Dec 27, 2023 •

edited

Loading

shargon commented Dec 27, 2023

Jim8y commented Dec 27, 2023

cschuchardt88 commented Dec 27, 2023 •

edited

Loading

cschuchardt88 Dec 27, 2023 •

edited

Loading

vncoelho Feb 22, 2024

shargon commented Dec 27, 2023

Jim8y commented Dec 27, 2023

cschuchardt88 commented Dec 27, 2023 •

edited

Loading

Jim8y commented Dec 27, 2023 •

edited

Loading

cschuchardt88 commented Dec 27, 2023 •

edited

Loading

Jim8y commented Dec 28, 2023 •

edited

Loading

cschuchardt88 commented Dec 28, 2023

shargon commented Dec 28, 2023

Jim8y commented Dec 28, 2023 •

edited

Loading

cschuchardt88 commented Dec 28, 2023

vncoelho commented Dec 28, 2023 •

edited

Loading

cschuchardt88 commented Dec 28, 2023 •

edited

Loading

roman-khimov Dec 28, 2023

Jim8y Dec 28, 2023

shargon Dec 29, 2023 •

edited

Loading

Jim8y commented Feb 15, 2024

Jim8y commented Feb 15, 2024

superboyiii commented Feb 20, 2024 •

edited

Loading

vncoelho Feb 22, 2024

Jim8y commented Jun 1, 2024

shargon commented Jun 1, 2024

BigInteger in Neo.Json #3037

Are you sure you want to change the base?

BigInteger in Neo.Json #3037

Conversation

shargon commented Dec 27, 2023

shargon commented Dec 27, 2023 • edited Loading

Jim8y commented Dec 27, 2023

cschuchardt88 commented Dec 27, 2023 • edited Loading

shargon commented Dec 27, 2023

Jim8y commented Dec 27, 2023

cschuchardt88 commented Dec 27, 2023 • edited Loading

cschuchardt88 Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

vncoelho Feb 22, 2024

Choose a reason for hiding this comment

shargon commented Dec 27, 2023

Jim8y commented Dec 27, 2023

cschuchardt88 commented Dec 27, 2023 • edited Loading

Jim8y commented Dec 27, 2023 • edited Loading

cschuchardt88 commented Dec 27, 2023 • edited Loading

Jim8y commented Dec 28, 2023 • edited Loading

cschuchardt88 commented Dec 28, 2023

shargon commented Dec 28, 2023

Jim8y commented Dec 28, 2023 • edited Loading

cschuchardt88 commented Dec 28, 2023

vncoelho commented Dec 28, 2023 • edited Loading

cschuchardt88 commented Dec 28, 2023 • edited Loading

roman-khimov Dec 28, 2023

Choose a reason for hiding this comment

Jim8y Dec 28, 2023

Choose a reason for hiding this comment

shargon Dec 29, 2023 • edited Loading

Choose a reason for hiding this comment

Jim8y commented Feb 15, 2024

Jim8y commented Feb 15, 2024

superboyiii commented Feb 20, 2024 • edited Loading

vncoelho Feb 22, 2024

Choose a reason for hiding this comment

Jim8y commented Jun 1, 2024

shargon commented Jun 1, 2024

shargon commented Dec 27, 2023 •

edited

Loading

cschuchardt88 commented Dec 27, 2023 •

edited

Loading

cschuchardt88 commented Dec 27, 2023 •

edited

Loading

cschuchardt88 Dec 27, 2023 •

edited

Loading

cschuchardt88 commented Dec 27, 2023 •

edited

Loading

Jim8y commented Dec 27, 2023 •

edited

Loading

cschuchardt88 commented Dec 27, 2023 •

edited

Loading

Jim8y commented Dec 28, 2023 •

edited

Loading

Jim8y commented Dec 28, 2023 •

edited

Loading

vncoelho commented Dec 28, 2023 •

edited

Loading

cschuchardt88 commented Dec 28, 2023 •

edited

Loading

shargon Dec 29, 2023 •

edited

Loading

superboyiii commented Feb 20, 2024 •

edited

Loading