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

Big-Endian hashes #938

Closed
shargon opened this issue Jul 19, 2019 · 15 comments
Closed

Big-Endian hashes #938

shargon opened this issue Jul 19, 2019 · 15 comments
Labels
Discussion Initial issue state - proposed but not yet accepted

Comments

@shargon
Copy link
Member

shargon commented Jul 19, 2019

I think that we have a lot of waste of time because we use strings in BigEndian, and binary in little endian, for me is a good move remove all .Reverse and use the same encoding.

Examples:

@igormcoelho
Copy link
Contributor

But where is uint160.tryparse used? I don't think it's a lot of places...

@shargon
Copy link
Member Author

shargon commented Jul 19, 2019

Every time you convert from an address to script hash

@lock9 lock9 added the Discussion Initial issue state - proposed but not yet accepted label Jul 20, 2019
@lock9
Copy link
Contributor

lock9 commented Jul 20, 2019

What is the difference between this issue and #794 ?

@shargon
Copy link
Member Author

shargon commented Jul 21, 2019

the same... xD

@lock9
Copy link
Contributor

lock9 commented Jul 21, 2019

@shargon so you have to convince @igormcoelho haha

@shargon
Copy link
Member Author

shargon commented Jul 22, 2019

Is more readable and faster :P

@lock9
Copy link
Contributor

lock9 commented Jul 22, 2019

@igormcoelho do you want to change "your vote"? The 'boyz' want to remove this reverse thing... if it is more "readable and faster" (although no evidence was provided), I don't see why we don't remove it

@shargon
Copy link
Member Author

shargon commented Jul 22, 2019

In fact, is same readable without the change, but is a waste of time, so obviously is faster... and more of one time i had mistakes with it.

@erikzhang
Copy link
Member

Many developers have already learned this feature. If it changes, the old developers have to learn again. And it is not significantly better.

@lock9
Copy link
Contributor

lock9 commented Jul 29, 2019

@erikzhang I understand many people already learned but isn't our goal to make neo more 'simple'? We already changed a lot of things that developers will need to learn and adapt.

Are you guys 'super' against removing this Reverse? Because I think we have '3 votes' for removing it 😅

@shargon
Copy link
Member Author

shargon commented Jul 29, 2019

We can close it, is just a speed recommendation, but i understand the cons

@igormcoelho
Copy link
Contributor

igormcoelho commented Jul 29, 2019

I just think that it's inconsistent to remove the reverse, it's not that I'm against.
If we want to remove the reverse, the best solution I see is just stop representing scripthashes as UInt160. If we do that, then no reverse is ever necessary, as there will be "no endianess" (just a bunch of bytes).
The big problem I see is removing the reverse from ToArray() on UInt160, when Neo is described as a little-endian architecture. It's not just Neo that does this, all platforms (including webassembly) which are little-endian, require these things. Usually, prints are made on bigendian (for human readability), and its internals vary (it could be little or big, but in this case, it's supposed to be little).

@lock9
Copy link
Contributor

lock9 commented Jul 29, 2019

Ok, I'm convinced.
Since @shargon is ok with it too, I'm closing this.
But note: It is the second or third time someone asks for this.

@lock9 lock9 closed this as completed Jul 29, 2019
Thacryba pushed a commit to simplitech/neo that referenced this issue Feb 17, 2020
* fix issues

* updated
@ProDog
Copy link
Contributor

ProDog commented Sep 5, 2020

This issue was closed. but I still want to propose, can we use the same encoding? all big endian or all little endian, for developers, you need to convert it frequently, it's a bad experience, and I've met a lot of people who blame this😣.

@roman-khimov
Copy link
Contributor

This one really is an endless source of trouble and it'd be nice to fix it. I can't count how many related issues we've fixed during NeoGo development, where everything was fine except we hadn't guessed BE/LE correctly. One day it even broke our beloved send.fs.neo.org which relies on proper contract hash configuration, so the hash was copy-pasted from NeoGo deployment output into the config and suddenly all invocations started to fail, just because NeoFS tooling expected this hash to be represented in a different manner.

And the main thing is that there is

"no endianess" (just a bunch of bytes).

for these values. From the hashing algorithms perspective, SHA-256 output is a concatenation of eight 32-bit values, it's not an integer of its own. RIPEMD-160 has a notion of "chaining variable", but even that is just five 32-bit words glued together, not a proper integer. (for the fun of it, compare that to Streebog that defines all operations using 512-bit vectors)

Even though these resulting bytes can be interpreted as integers, we never use them as such. We only load/store/compare them, there is no need for any arithmetic operations using them (where the question of LE/BE interpretation would be appropriate). What we have here is essentially "big ID" and "small ID" instead of 256-bit and 160-bit integers. So we can use hash outputs as is, wrapping them with some classes is fine, but messing with the order of bytes is certainly not.

roman-khimov added a commit to nspcc-dev/neo-go that referenced this issue Jul 23, 2021
roman-khimov added a commit to nspcc-dev/neo-go that referenced this issue Jul 23, 2021
roman-khimov added a commit to nspcc-dev/neo-go that referenced this issue Dec 7, 2021
A follow-up to #2302, sort hashes using reverse order. Fixes
f9c68613f41ca85ddb01ec757153c0d86018d2324f6ff85a4afc708bb598b722 validation.

Hi, neo-project/neo#938.
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

6 participants