Change Boolean false state

[ixje]

Closes #129
a Boolean StackItem in either false or true state should always return at least 1 byte for GetByteArray(). This is the default behaviour in C#'s build in BitConverter as well as many other languages. It is misleading for it to return an empty array as that will lead to a length/size of 0, whereas the item still takes up memory space.

[codecov-io]

Codecov Report

Merging #132 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #132   +/-   ##
=======================================
  Coverage   61.36%   61.36%           
=======================================
  Files          16       16           
  Lines        1421     1421           
=======================================
  Hits          872      872           
  Misses        549      549

Impacted Files	Coverage Δ
src/neo-vm/Types/Boolean.cs	44.44% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da851c7...a9414da. Read the comment docs.

[erikzhang]

Will this change break old contracts?

[ixje]

As far as I can tell not.

1. all VM tests still pass. I only adjusted the return size from 0 to 1 in the PR
2. the original author of neo-python ported the Boolean stackitem code wrong. Meaning it has been returning 1 byte for a false state (as now being proposed in the PR) since the code was first written back in 2017. I just discovered this after a failed JSON test, but I've seen no reports for neo-python indicating deviating contract execution results because of this.

What will behave differently in some cases is

neo-vm/src/neo-vm/Types/Boolean.cs

Line 32 in a3f9444

return Unsafe.MemoryEquals(GetByteArray(), bytes_other);

so e.g. a BigInteger.zero would now equal a Boolean false, which is more accurate than before. I don't see a straight forward way for test all existing contracts if such scenario's are in use and ifso if they break something.

[erikzhang]

It needs to be tested.

[ixje]

Ok, we don't trust the existing tests due to lack of coverage. I can understand that.

Do you have a suggestion on how you would like to see it tested? I can sync the whole chain from scratch with this change and do contract storage comparisons, but that might not suit your needs therefore I'm asking for directions/input/ideas..

[erikzhang]

I think it's okay if it doesn't break old contracts.

[vncoelho]

We have currently created this procedure here, it compares two neo-cli versions and checks storage differences: https://github.com/NeoResearch/neo-tests/tree/master/clients-assert-testnet

[vncoelho]

We are currently thinking about how to integrate this here, we are afraid this is to much costly for a UT.

[ixje]

thanks for the tip @vncoelho . What is the benefit of using your docker setup vs manually compiling a client with this fix , syncing using the offline chain files and comparing the storage with the normal client?

[vncoelho]

I mean, in that script we already start a plugin that export all storage differences (in NEO 3.0 we gonna have MPT and this will not be necessary).

However, nowadays, that sounds necessary.
That docker-compose keep the healthy status of the containers by checking if their storages are the same.
If we have difference in storage that will result in unhealthy state and we will know that the difference has a backward effect on old contracts.

[vncoelho]

We could also manually export storage and check this against https://github.com/NeoResearch/neo-storage-audit

[shargon]

This could break old contracts. Good for neo 3, not for neo 2

[igormcoelho]

Perhaps we do this @ixje . What are the scenarios generated from neo-boa and neon compiler, involving Boolean state? Can you try some and we think about them?
If the all existing compilers generate a compatible code (which is possible) and no deviations happen on global storage, it's safe to assume this new path on Neo 2.

[igormcoelho]

Anyway, it makes more sense to focus on Neo 3, so we don't break anything by chance, and develop things faster.

[ixje]

This could break old contracts. Good for neo 3, not for neo 2

Do you have an example or is this a guess?

NEO 3.0 should be backwards compatible, no? So we have to figure out the effect anyway.

[igormcoelho]

We will need to redeploy many contracts on Neo 3 due to global assets abolished... so bavkwards compatibility can be flexible in this case.
The effect is not big I guess, its more related to the uncertainty of changing the rule and someone depending on it. How current compilers behave for this? My.guess is that they are compatible. But we need to test one by one to be sure.

[shargon]

@ixje can you make the same as this but for master (neo 3.x) and close this pull request?

[ixje]

@shargon fixed. Changed destination to master branch and force pushed a "clean commit".

[shargon]

@erikzhang are you agree with merge it? in the future we can check if is valid for neo2, but i think that is not needed in neo2, and should be only for neo3

[igormcoelho]

Just a question: push0 will push an empty array, right? PUSHF was supposed to be "push false" and it was equals to Push0.
with this change, an empty byte array is true or false? I hope this is false 😂
Anyway, it makes sense... one better conversion is Byte Array to BigInt to bool. This is even better, because is byte array 0x0000 false?
using bigint logic, 0x, or 0x00, or 0x0000, or 0x00000000... are all zero, and zero is false. Otherwise it is true. This corresponds to the majority of existing languages.