-
Notifications
You must be signed in to change notification settings - Fork 77
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
Feat/roman/poseidon2 #507
Feat/roman/poseidon2 #507
Conversation
e0336bf
to
e1d1474
Compare
*/ | ||
struct Poseidon2Config { | ||
device_context::DeviceContext ctx; /**< Details related to the device such as its id and stream id. */ | ||
bool are_states_on_device; /**< True if inputs are on device and false if they're on host. Default value: false. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor but the comment says 'inputs' where the field is called 'states'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, comments are yet to be added, I want to do that tomorrow, for now it's fine I think
|
||
namespace poseidon2 { | ||
template <typename S> | ||
cudaError_t create_optimized_poseidon2_constants( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to release the constants that you allocate here? what is the intention here in terms of memory management?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, currently there is no way to release the constants. The intention was to store all the constants on-device like we do with twiddles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yshekel good point here, we need to let users to control the free memory on the device, to be solved in next update
case 11: | ||
S result8 = S::sqr(S::sqr(result2)); | ||
return result8 * result2 * element; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add assert(false) in default case so the GPU at least throws some error instead of silently do nothing. I assume you check it earlier but still it wouldn't hurt to assert if here too.
|
||
template <typename S, int T> | ||
cudaError_t poseidon2_hash( | ||
S* states, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better be const S* if states is not modified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
states can be modified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, wrote a few comments
e1d1474
to
c9abf38
Compare
This PR adds support for poseidon2 permutation function as described in https://eprint.iacr.org/2023/323.pdf Reference implementations used (and compared against): https://github.com/HorizenLabs/poseidon2/tree/main https://github.com/Plonky3/Plonky3/tree/main Tasks: - [x] Remove commented code and prints - [ ] Add doc-comments to functions and structs - [x] Fix possible issue with Plonky3 imports - [x] Update NTT/Plonky3 test - [x] Add Plonky3-bn254 test (impossible)
This PR adds support for poseidon2 permutation function as described in https://eprint.iacr.org/2023/323.pdf
Reference implementations used (and compared against):
https://github.com/HorizenLabs/poseidon2/tree/main
https://github.com/Plonky3/Plonky3/tree/main
Tasks: