KotoDevelopers / koto

MIT License
27 stars 8 forks source link

Serialize input to yespower_tls() for endianness-neutral behavior #18

Closed solardiz closed 5 years ago

solardiz commented 5 years ago

It appears that the way yespower_hash is defined and used, conversion from host to "network" (or any fixed) byte order is skipped. In Zcash, it appears that such conversion was performed for the block header when passing it on to BLAKE2 hashing (followed by Equihash). Thus, the concern is: does Koto's core tree work on big-endian systems the same way it does on today's typical little-endian systems? Has anyone tested? My guess: no, it doesn't.

Here's part of diff between Zcash and Koto:

     // Check the header
-    if (!(CheckEquihashSolution(&block, Params()) &&
-          CheckProofOfWork(block.GetHash(), block.nBits, Params().GetConsensus())))
+    if (!CheckProofOfWork(block.GetPoWHash(), block.nBits, Params().GetConsensus()))
         return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());

Per my review, both CheckEquihashSolution and GetHash did serialization of the block header, whereas Koto's GetPoWHash doesn't.

Here's what appears to work for me in another tree (at least this doesn't appear to break things on little-endian; I haven't tested this on big-endian yet), you could want to adopt something similar for Koto:

+++ b/src/primitives/block.h

[...]

+#include "streams.h"
+#include "version.h"

[...]

     uint256 GetPoWHash() const
     {
         static const yespower_params_t params = {
[...]
         };
         uint256 thash;
-        if (yespower_tls((const uint8_t *)&nVersion, 80, &params, (yespower_binary_t *)&thash))
+        CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
+        ss << *this;
+        if (yespower_tls((const uint8_t *)&ss[0], ss.size(), &params, (yespower_binary_t *)&thash))
             abort();
         return thash;
     }

There's no need to unserialize the PoW hash output when we put it in a uint256 (as in Bitcoin, Koto, and the example above), because that's a host endianness neutral type as it is (and is converted to host endianness specific arith_uint256 where needed).

cryptozeny commented 5 years ago

thanks for letting us know. Let's learn more! 😅

wo01 commented 5 years ago

Thank you for your comment. We will modify the code until next release.

cryptozeny commented 5 years ago

I'm making a fork of bitcoin for yespower 1.0. It works well but I need more study 😭 @solardiz also reading YACoin... 😓

cryptozeny commented 5 years ago

great job. thanks! @wo01

cryptozeny commented 5 years ago

@solardiz I am making a yespower 1.0 coin by forking a bitcoin. Do I have to do this check in yespower 1.0? 🤔

solardiz commented 5 years ago

Thanks @wo01. Your changes look correct to me. Have you actually tested them on big-endian, or (like me) are relying (for now) on them looking right and not breaking things on little-endian? As an Open Source project, you could get a GCC Compile Farm account and test on one of their POWER boxes, if you like. (I wonder if this would turn up other portability issues elsewhere in the tree, though. I did test yespower on big-endian POWER and it works fine, but I don't know if the Zcash core tree is being tested on big-endian or not.)

@cryptozeny This isn't specific to yespower, and it isn't a check. Yes, you do need to serialize the block header before passing it to yespower 1.0 as well.

wo01 commented 5 years ago

I have tested them on little-endian machine. Thank you for your information. I would like to test them on big-endian box sometime soon.

cryptozeny commented 5 years ago

I have tested them on little-endian machine. I would like to test them on big-endian box sometime soon.

my current system is x86_64-little_endian-lp64 shared (dynamic)

how can i test it on a BIG_ENDIAN system?