bitzenyPlus / BitZenyPlus

[in progress] BitZeny Plus integration/staging tree created by the New Dev team
https://bitzeny.tech/
MIT License
11 stars 3 forks source link

yespower-0.5 #3

Closed cryptozeny closed 6 years ago

cryptozeny commented 6 years ago

Hi bitzenyPlus devs,

Version name is z3.0.0

The changes in this updates are:

Please review. Thanks!


@solardiz helped me. thanks a lot!

https://github.com/cryptozeny/bitzeny/commit/ea21675aab1b507de0815ab61202b50aa15c469e#comments

hitobb commented 6 years ago

Great!

This is the first release as BitZeny Plus, so you should change to version name is z3.0.0. This code is so important as to change the major version.

Lesmiscore commented 6 years ago

Evidence for compatibility about block by @cryptozeny : https://test-insight.bitzeny.jp/block/00002f6d51900381209b2ed9478d03d4e7ca0044a591845c5b97ff8e2a2eb1be

solardiz commented 6 years ago

@cryptozeny You're still not checking the return value. Instead, you now have yespower_hash return int, but then ignore its return value by having the prototype use void and consequently having the callers ignore this return value. Now that this is not just a miner, such sloppy code is actually dangerous. You must fix it before merging. The easiest way to fix this is to check the return value from yespower_tls and abort() on non-zero while inside yespower_hash. And have its return type as void.

Also, ideally you should move your yespower.c wrapper to outside of the yespower directory (since it's not part of yespower itself) and list the yespower source files in Makefile.am instead of using #include on the .c files. But this isn't critical, your current approach just looks sloppy.

solardiz commented 6 years ago

how to check the return value? could you give me the code?

I could, but it's literally 2 lines of C, and you got to have someone capable of that on the team. If you're not that person, then please ask within your team. Me providing you very basic advice on C programming isn't long-term sustainable for your project.

cryptozeny commented 6 years ago

@solardiz thanks so much your advice :+1: we will do our best to find the right code :smile: we should study your yespower further :sweat_smile:

related question: Cryply also has the same problem you said? I referred to Cryply... :sob:

https://github.com/cryply/cryply-wallet/blob/d974128611963e664232c1d24a01f9b284af7421/src/hash/yespower/yespower.c#L36

solardiz commented 6 years ago

This has nothing to do with yespower specifically. It's just basic C programming.

You can't tell from that file/line alone, but yes looking at:

https://github.com/cryply/cryply-wallet/blob/d974128611963e664232c1d24a01f9b284af7421/src/core.cpp#L218

@cryply also has this problem (missed yespower return value check). I've just e-mailed their developer. Thanks.

cryptozeny commented 6 years ago

@solardiz is it looks better than old one? :thinking:

// PR-2 (hittob)
void yespower_hash(const char *input, char *output){
    if (yespower_tls(input, 80, &yespower_bitzeny, (yespower_binary_t *) output) == 0){
        return;
    }
    else{
        abort();
    }
}
solardiz commented 6 years ago

@cryptozeny Now this is correct, albeit weird. You might want to simplify it so that you don't need the else.

cryptozeny commented 6 years ago

thanks so much! :heart: :100:

cryptozeny commented 6 years ago

@solardiz it builds OK on my machine(ubuntu16.04 x64 intel) and VPS(ubuntu16.04) and works also good. but travis-ci failed. he said:

crypto/yespower/sha256.c:32:30: fatal error: insecure_memzero.h: No such file or directory
 #include "insecure_memzero.h"
                              ^
compilation terminated.
make[2]: *** [crypto/yespower/libbitcoinconsensus_la-sha256.lo] Error 1
make[2]: Leaving directory `/home/travis/build/bitzenyPlus/BitZenyPlus/build/bitzeny-arm-linux-gnueabihf/src'
make[1]: *** [install-recursive] Error 1
make[1]: Leaving directory `/home/travis/build/bitzenyPlus/BitZenyPlus/build/bitzeny-arm-linux-gnueabihf/src'
make: *** [install-recursive] Error 1
The command "make $MAKEJOBS $GOAL || ( echo "Build failure. Verbose build follows." && make $GOAL V=1 ; false )" exited with 1.

https://travis-ci.org/bitzenyPlus/BitZenyPlus/jobs/410304053#L1835

do i need add yespower/insecure_memzero.h to src/Makefile.am ? it was a workaround for me...

solardiz commented 6 years ago

do i need add yespower/insecure_memzero.h to src/Makefile.am ? it was a workaround for me...

This isn't supposed to help.

I don't immediately know what the problem is, and I'm out of time. Please either debug this on your own or revert the changes to whatever revision worked for you.

cryptozeny commented 6 years ago

Its fine! :smiley: There was plenty of help. It was impossible without you. Thank you so much! :heart:

cryptozeny commented 6 years ago

finally merged 😊