BigInteger in Neo.Json #3037
BigInteger in Neo.Json #3037
Conversation
|
During this PR an error was found, a number greater than Open question: We want this restriction only for serialize? |
|
This will change the behavior of existing contracts as well. |
|
I was expecting something other than the |
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. |
|
This PR isnt going to work seeing how big integers can be 256-bits. |
| public void TestBigInteger() | ||
| { | ||
| ((JNumber)BigInteger.One).AsNumber().Should().Be(1); | ||
| ((JNumber)BigInteger.Zero).AsNumber().Should().Be(0); | ||
| ((JNumber)BigInteger.MinusOne).AsNumber().Should().Be(-1); | ||
| } |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
Added ut with min and max
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. |
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. |
|
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. |
|
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. |
|
You playing with people's potential money they have invested. This could be bad if a contract is doing This should of been developed correctly from the begin, the whole reason why we don't use |
|
@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 |
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. |
|
I understand now. I didn't know what you meant by |
Perhaps this is a possible solution @shargon. |
|
Let's not forget about |
| @@ -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) | |||
There was a problem hiding this comment.
He put it in the JNumber. Different exception, but will fail as well.
There was a problem hiding this comment.
It was moved to the constructor, that was the bug, when we used parse we allowed it...
|
@superboyiii, please check the state compatibility. |
|
@shargon format not a thing i can update, please check it. |
cschuchardt88
added
Blocked
|
@shargon do you can start working on this again? If I remember we needed to change the vm as well. Since we are working on the vm now. |
|
@shargon conflicts, lets add hardfork if possible and make it merged into the hardfork pr. Such that no more conflict to maintain. |
| /// <returns>The serialized object.</returns> | ||
| public static JToken Serialize(StackItem item) | ||
| public static JToken Serialize(StackItem item, Func<Hardfork, bool> hardforkChecker = null) |
There was a problem hiding this comment.
@neo-project/core This methods seems that was only used in UT...
Close #3036