-
Notifications
You must be signed in to change notification settings - Fork 59
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
Concern with overflow and the use of signed integers regarding the Lyra2.c file #42
Comments
There is no concern, if you read the call stack you will see it can't get an invalid int. If |
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) |
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. |
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? |
I'll find tomorrow, it's later here. |
https://github.com/vertcoin/vertcoin-old/blob/master/src/Lyra2RE/Lyra2.c#L46 Quick reference to the vertcoin Lyra2, for comparison |
Well that's vertcoins implementation not the reference implementation |
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. |
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
The text was updated successfully, but these errors were encountered: