Implemented and Tested Starkware Poseidon #3
Conversation
We should have this return a useful error.
We need to move the channel trait/impl to a crate.
|
Btw, performance seems pretty good, almost twice as fast as https://docs.rs/starknet-crypto/latest/starknet_crypto/ on the same hardware: |
There was a problem hiding this comment.
Unfortunately, three days ago, starknet-rs changed to use poseidon from type-rs, or more precisely, the implementation from lambdaworks.
When analyzing the overall logic, our implementation seems more(or at least similarly) efficient, but the performance is slightly lower. This is probably due to using a different Field crate.
LGTM!
| let t1 = t0 + st[2]; | ||
| st[0] = t1 + st[0].double(); | ||
| st[1] = t1 - st[1].double(); | ||
| st[2] = t0 - st[2].double(); |
There was a problem hiding this comment.
Compare to https://github.com/lambdaclass/lambdaworks/blob/main/crypto/src/hash/poseidon/starknet/parameters.rs#L25
You replaced one addition operation to one allocation. 👍
If replacing one addition to one allocation gives better result in lamdaworks codebase, it might be good to submit a PR to the lambdaworks implementation. (Is there a conflict of interest?)
| st[2] = t0 - st[2].double(); | ||
| } | ||
|
|
||
| fn round_partial<'a>(st: &mut [Felt252; WIDTH], key: &mut impl Iterator<Item = &'a Felt252>) { |
There was a problem hiding this comment.
Adding inline suggestion to round_partial and round_full gives me a slightly better(about 1%) performance
nah, it's not reproducible, it seems to have been a momentary anomalous result.
poseidon/build.rs
Outdated
| } | ||
|
|
||
| #[allow(clippy::single_char_add_str)] | ||
| fn main() { |
There was a problem hiding this comment.
Is there any benefit to including builder code rather than pre-built code?
Or it's just a matter of choice? I'm just curios 😅
There was a problem hiding this comment.
I went back/forth on this: we could parse them once and include the compressed round keys.
- On one hand it would simplify things: just have an array with the processed round keys.
- On the other hand with would make it less clear where these constants come from, if, e.g. I had a separate python script producing them and then simply added these as constants.
It might be over engineered: roughly half the code is doing this.
There was a problem hiding this comment.
Your right, I decided that it was simpler to just add the processed output.
Jaehun is right, this was over-engineered.
Implements the Starkware-flavor Poseidon hash function.
Also restructures the project into sub-crates.