GarlicoinOrg / Garlicoin

Garlicoin source tree
http://garlicoin.io
MIT License
139 stars 59 forks source link

Concern with overflow and the use of signed integers regarding the Lyra2.c file #42

Closed ezquire closed 6 years ago

ezquire commented 6 years ago

image

Your code is on the right, the Lyra2.c original code is on the left, they use a uint64_t, you use an int64_t (which as far as I know is a signed integer and can lead to overflow errors)

Not a terribly difficult fix, but it is a concern.

Cheers! Thanks for all your work, I am but a lowly Garlic miner, willing to help where I can

ryan-shaw commented 6 years ago

There is no concern, if you read the call stack you will see it can't get an invalid int. If int64_t was always a concern it wouldn't have been added.

ezquire commented 6 years ago

is relying on the call stack beneficial for efficiency? I'm still just a student so I'm trying to gauge why you wouldn't just change it to a unsigned int, does it make more sense to restrict the functions parameters instead of relying on the call stack to return a legitimate value? (again im a student so bear with me if what i'm saying makes no sense)

ryan-shaw commented 6 years ago

Honestly, I don't know, it's not my code- it's from the reference lyra2 implementation. You'd have to ask them why they chose this.

ezquire commented 6 years ago

the code I diffed is from the lyra2.c core implementation, so maybe there was some change between the docs? I know its annoying but can you shoot me that link to the ref docs before we call this issue closed?

ryan-shaw commented 6 years ago

I'll find tomorrow, it's later here.

Bogden commented 6 years ago

https://github.com/vertcoin/vertcoin-old/blob/master/src/Lyra2RE/Lyra2.c#L46

Quick reference to the vertcoin Lyra2, for comparison

ryan-shaw commented 6 years ago

Well that's vertcoins implementation not the reference implementation

ryan-shaw commented 6 years ago

This is a none issue. The function you are referring can't be passed anything other than 32 bytes look here and you will see why https://github.com/GarlicoinOrg/Garlicoin/blob/master/src/crypto/allium/allium.c#L59.