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

BigInteger 0 isn't able to be convert to Byte[]{ 0 } #201

Closed
superboyiii opened this issue Mar 12, 2020 · 10 comments · Fixed by neo-project/neo#1471
Closed

BigInteger 0 isn't able to be convert to Byte[]{ 0 } #201

superboyiii opened this issue Mar 12, 2020 · 10 comments · Fixed by neo-project/neo#1471

Comments

@superboyiii
Copy link
Member

superboyiii commented Mar 12, 2020

Due to #196, we've improved a lot on smart contract convertion method. However, I still find a little issue that BigInteger 0 isn't able to be convert to Byte[]{ 0 } by ToByteArray method. This will cause many issues such like balance exception when make transaction. I've made a little example to replay this situation. For example:

using Neo.SmartContract.Framework;
using Neo.SmartContract.Framework.Services.Neo;
using System;
using System.Numerics;

namespace TestConvert
{
    [Features(ContractFeatures.HasStorage | ContractFeatures.Payable)]
    public class Contract1 : SmartContract
    {
        public static object Main(string operation, object[] args)
        {
            BigInteger num = 0;
            byte[] byteNum = num.ToByteArray();
            Storage.Put(Storage.CurrentContext, "key", byteNum);
            return "TestConvertZero";
        }
    }
}

Then I could deploy it successfully, but I find the value is null in storage.

neo> deploy D:\Master-smoking\2020-03-13\neo-devpack-dotnet\TestConvert\bin\Debug\netstandard2.1\TestConvert.nef D:\Master-smoking\2020-03-13\neo-devpack-dotnet\TestConvert\bin\Debug\netstandard2.1\TestConvert.manifest.json
Script hash: 0x7b5ae605948a00b1beb37ffdb985fc385907d05b
Gas: 100000000

Signed and relayed transaction with hash=0x710cc5f6fb4b2b175ef90c6438112192f8d018539053d6de7f4bb1556d6325a7
neo> invoke 0x7b5ae605948a00b1beb37ffdb985fc385907d05b Main
Invoking script with: '10c00c044d61696e0c145bd0075938fc85b9fd7fb3beb1008a9405e65a7b41627d5b52'
VM State: HALT
Gas Consumed: 1389470
Evaluation Stack: [{"type":"ByteArray","value":"VGVzdENvbnZlcnRaZXJv"}]

relay tx(no|yes): y
Signed and relayed transaction with hash=0xde9bff4f091583f6dd681d1b3a007938d37a011c8bc0b2eb68435ec9d4aaf37c
neo>

Try GetStorage:
image

If I change this num to any num except 0, it works well.

using Neo.SmartContract.Framework;
using Neo.SmartContract.Framework.Services.Neo;
using System;
using System.Numerics;

namespace TestConvert
{
    [Features(ContractFeatures.HasStorage | ContractFeatures.Payable)]
    public class Contract1 : SmartContract
    {
        public static object Main(string operation, object[] args)
        {
            BigInteger num = 20;
            byte[] byteNum = num.ToByteArray();
            Storage.Put(Storage.CurrentContext, "key", byteNum);
            return "TestConvertZero";
        }
    }
}
neo> deploy D:\Master-smoking\2020-03-13\neo-devpack-dotnet\TestConvert\bin\Debug\netstandard2.1\TestConvert.nef D:\Master-smoking\2020-03-13\neo-devpack-dotnet\TestConvert\bin\Debug\netstandard2.1\TestConvert.manifest.json
Script hash: 0xa6fc45f3c8eeefcb2c7969bc0aab46a032ddd224
Gas: 100000000

Signed and relayed transaction with hash=0x784196a4a8a563c1559137e50550685fb87d2823fd1f884bca01140afcf3b2ed
neo> invoke 0xa6fc45f3c8eeefcb2c7969bc0aab46a032ddd224 Main
Invoking script with: '10c00c044d61696e0c1424d2dd32a046ab0abc69792ccbefeec8f345fca641627d5b52'
VM State: HALT
Gas Consumed: 1569620
Evaluation Stack: [{"type":"ByteArray","value":"VGVzdENvbnZlcnRaZXJv"}]

relay tx(no|yes): y
Signed and relayed transaction with hash=0x560797ed5900eb78aa9d6d3d667944861073b44f9b89e2395eb14116eca189e7
neo>

Try GetStorage:
image

@lightszero Sorry to bother you again.

@erikzhang
Copy link
Member

Then I could deploy it successfully, but I find the value is null in storage.

I think it shouldn't be null. It should be an empty array of byte.

@lightszero
Copy link
Member

lightszero commented Mar 12, 2020

In Neo Storage.Put a zero length bytearray means delete a Key @superboyiii

            private static bool PutExInternal(ApplicationEngine engine, StorageContext context, byte[] key, byte[] value, StorageFlags flags)
            {
                if (key.Length > MaxKeySize) return false;
                if (value.Length > MaxValueSize) return false;
                if (context.IsReadOnly) return false;

                StorageKey skey = new StorageKey
                {
                    Id = context.Id,
                    Key = key
                };

                if (engine.Snapshot.Storages.TryGet(skey)?.IsConstant == true) return false;

                if (value.Length == 0 && !flags.HasFlag(StorageFlags.Constant))
                {
                    // If put 'value' is empty (and non-const), we remove it (implicit `Storage.Delete`)
                    engine.Snapshot.Storages.Delete(skey);
                }

@superboyiii
Copy link
Member Author

In Neo Storage.Put a zero length bytearray means delete a Key @superboyiii

            private static bool PutExInternal(ApplicationEngine engine, StorageContext context, byte[] key, byte[] value, StorageFlags flags)
            {
                if (key.Length > MaxKeySize) return false;
                if (value.Length > MaxValueSize) return false;
                if (context.IsReadOnly) return false;

                StorageKey skey = new StorageKey
                {
                    Id = context.Id,
                    Key = key
                };

                if (engine.Snapshot.Storages.TryGet(skey)?.IsConstant == true) return false;

                if (value.Length == 0 && !flags.HasFlag(StorageFlags.Constant))
                {
                    // If put 'value' is empty (and non-const), we remove it (implicit `Storage.Delete`)
                    engine.Snapshot.Storages.Delete(skey);
                }

I don't understand. This is correct. But what I expect is value.Length is 1 and value is { 0 }. I don't mean byte[0] but byte[1]{ 0 }. It shouldn't return null.

@lightszero
Copy link
Member

lightszero commented Mar 12, 2020

Because BigInteger 0 convert to Bytes will got a Byte[0].
If you want get a Byte[1]{0},that 's a big change. many code about that in NEOVM.
@superboyiii
you can at guys to discuss if we should let BigInteger 0 Convert to byte[1]{0}.

@erikzhang
Copy link
Member

In Neo Storage.Put a zero length bytearray means delete a Key

I remembered that in this case, it should really return null.

@superboyiii
Copy link
Member Author

In Neo Storage.Put a zero length bytearray means delete a Key

I remembered that in this case, it should really return null.

If we don't change it to byte[1]{0}, we could meet issues in smart contract especially some cases of invoke balance. And we have to always notify developers we can't convert like this. This is a limit for sc. And for me, I think Byte[1]{0} is absolutely different from Byte[0], it should return Byte[1]{0}.

@Tommo-L
Copy link
Contributor

Tommo-L commented Mar 12, 2020

I think we should convert 0 -> byte[1]{ 0 } for storage, which contains two meanings:

  1. one is that the key value pair exists,
  2. the other is that its value is 0.

If we convert 0 -> byte[0]{}, which means this is an nonexist key-pair. There is an example, it'll be difficult if we want to count how many registered users in a nep5.

If we convert 0 -> byte[1]{ 0 }, we should encourage/incentive users to release resources.

@erikzhang
Copy link
Member

Perhaps putting 0 bytes of data should not be considered as deleting the record. Because the user can always call Delete to explicitly delete the record.

@Tommo-L
Copy link
Contributor

Tommo-L commented Mar 12, 2020

Agree 100%

Therefore, we should incentive users to release resources.

@shargon
Copy link
Member

shargon commented Mar 12, 2020

Perhaps putting 0 bytes of data should not be considered as deleting the record. Because the user can always call Delete to explicitly delete the record.

Agree

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.

5 participants