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

[NEO3] Remove reverse() from UIntBase string representation #794

Closed
ixje opened this issue Jun 4, 2019 · 9 comments
Closed

[NEO3] Remove reverse() from UIntBase string representation #794

ixje opened this issue Jun 4, 2019 · 9 comments
Labels
Discussion Initial issue state - proposed but not yet accepted

Comments

@ixje
Copy link
Contributor

ixje commented Jun 4, 2019

The UInt160 and UInt256 classes always had the strange behaviour of representing their String output in reversed order

neo/neo/UIntBase.cs

Lines 120 to 123 in a8ada56

public override string ToString()
{
return "0x" + data_bytes.Reverse().ToHexString();
}

If the underlying byte array has data stored as 0xDE, 0xAD, 0xBE, 0xEF, then the string representation should also be 0xDEADBEEF not 0xEFBEADDE.

I never understood why it was there, but I hope we can remove this odd behaviour from NEO3.

@erikzhang erikzhang added the Discussion Initial issue state - proposed but not yet accepted label Jun 4, 2019
@erikzhang
Copy link
Member

Because they are UInt, the string representation of an integer is generally in big endian.

@ixje
Copy link
Contributor Author

ixje commented Jun 4, 2019

Do you have an example where this is commonly viewed like that? I've not seen an uint turn into a different endianness just for string representation before except in NEO. Endianness historically has been an arbitrary CPU architecture design choice for the silicon manufacturer so I don't see any relationship either.

A quick look at C#'s bitconverter shows me this

uint test_conversion = 0x1234; // 4660 decimal
Console.WriteLine(BitConverter.ToString(BitConverter.GetBytes(test_conversion))); // 34-12-00-00

Using a little bit of python to turn it back (as the used endianness is more obvious from this function)

In [1]: hex(int.from_bytes(b'\x34\x12\x00\x00', byteorder='little'))
Out[1]: '0x1234'

This to me tells me at least BitConverter thinks a uint is little endian in string representation.

@shargon
Copy link
Member

shargon commented Jun 4, 2019

I think that is better to remove it because is less logic for the node, but again this will produce different addresses for neo3 and neo2

@ixje
Copy link
Contributor Author

ixje commented Jun 4, 2019

According to #776 neo3 already produces different addresses. So whatever the fix for that will be, we can also apply the reversing logic just there and keep NEO3 data types straight forward with less logic.

@igormcoelho
Copy link
Contributor

igormcoelho commented Jun 4, 2019

Real question is: do we want hex prints to be big endian or little endian?
Right now they are big endian, which is correct and reasonable in my opinion (example 0x0001 is actually 1,not 256).

@shargon no address will change, only prints are affected. Neo machine will continue to be little endian.

We also need to define if scripthash is a number or not. If not number, this means we just not use UInt160 and it gets "automatically de-reversed".

@igormcoelho
Copy link
Contributor

igormcoelho commented Jun 4, 2019

@ixje there are two policies from your example. C# bitconverter seems to print always in internal format (little endian). Python bitconverter always prints in bigendian (just like Neo) regardless of internal machine byte order running it. I guess only in struct serialization you get to see real python endianess, not sure.

@ixje
Copy link
Contributor Author

ixje commented Jun 4, 2019

I think there should be no big endian anywhere. Everything is compiled for x86-x64 afaik, which is all little endian. What makes printing values in hex format to be correct in big endian?

@igormcoelho what python bit converter are you referring to exactly?

@igormcoelho
Copy link
Contributor

just closed the links haha I was reading some crazy python forums, let me try to find them. In fact, Neo is not limited to x86, right now work on projects with Neo for ARM and many other microarchitectures (on neopt project), thats why we need explicit platform endianess to be compatible (otherwise internal calculations would fail).

@igormcoelho
Copy link
Contributor

igormcoelho commented Jun 6, 2019

@ixje just a follow-up from our last discussion. This is python in my computer (little-endian x86-64)

>>> import sys
>>> sys.byteorder
'little'
>>> x=256
>>> hex(x)
'0x100'            # <- this is big endian format, used on human-readable prints
>>> pack('h', x)
b'\x00\x01'        # <- this is native format (little-endian), only used for internal byte array serialization. 'h' means two bytes (short type).

So, although my computer is little-endian, integers (or big integers) are presented in big endian hex format, just like Neo.

@ixje ixje closed this as completed Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

No branches or pull requests

4 participants