feat: poseidon hash #388
feat: poseidon hash #388
Conversation
# Conflicts: # Cargo.lock # Cargo.toml # lib/crypto/Cargo.toml # lib/crypto/proptest-regressions/field/fp.txt # lib/crypto/src/bigint.rs # lib/crypto/src/bits.rs # lib/crypto/src/field/fp.rs # lib/crypto/src/lib.rs
| }; | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test { | ||
| use alloc::vec::Vec; |
There was a problem hiding this comment.
| use alloc::vec::Vec; | |
| use alloc::{vec, vec::Vec}; |
This should completely remove those vec-related errors in tests
| }; | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test { | ||
| use alloc::vec::Vec; | ||
|
|
||
| use super::*; | ||
|
|
There was a problem hiding this comment.
At least a couple of tests for from_str_hex would be very useful to document how it works, we could then later add fuzz tests when we start working on them
There was a problem hiding this comment.
Good point! Agree. I'll add a few fuzz tests
| @@ -32,6 +32,7 @@ impl_bit_iter_be!(usize); | |||
|
|
|||
| #[cfg(test)] | |||
| mod tests { | |||
| use alloc::vec::Vec; | |||
There was a problem hiding this comment.
| use alloc::vec::Vec; | |
| use alloc::{vec, vec::Vec}; |
| $crate::field::fp::Fp::new( | ||
| $crate::bigint::crypto_bigint::Uint::from_be_hex($num), | ||
| ) | ||
| $crate::field::fp::Fp::new($crate::bigint::from_str_hex($num)) | ||
| }}; | ||
| } | ||
|
|
There was a problem hiding this comment.
I see errors in this file's tests like cannot find macro `format` in this scope, and it seems these go away after importing:
use alloc::format;These do not fail the build, or stop the test execution, they show up in my IDE (maybe it's the IDE 🤔 ).
Anyway, seems to be some phantom issue due to proptest using std::format internally.
| $crate::field::fp::Fp::new( | ||
| $crate::bigint::crypto_bigint::Uint::from_be_hex($num), | ||
| ) | ||
| $crate::field::fp::Fp::new($crate::bigint::from_str_hex($num)) |
There was a problem hiding this comment.
Just curios, why was Uint::from_be_hex not appropriate to use here?
There was a problem hiding this comment.
Uint::from_be_hex requires specific length of hex string. Like 2 byte "1234" number wont be converted to U64 with leading zeros. Only 8 byte hex number does so. Of course we could format all our const values to proper length, but I've just decided to add our const hex conversion and copy constants from referenced implementation here in whitepaper
| @@ -32,6 +32,7 @@ impl_bit_iter_be!(usize); | |||
|
|
|||
| #[cfg(test)] | |||
There was a problem hiding this comment.
Again curios, why are these not marked as other tests with:
#[cfg(all(test, feature = "std"))]though merkle and hash have it?
| const ROUNDS_F: usize = 8; | ||
| const ROUNDS_P: usize = 21; | ||
| const MAT_INTERNAL_DIAG_M_1: &'static [Scalar] = &[ | ||
| fp_from_hex!("409133f0"), |
There was a problem hiding this comment.
Is there some resource we can use to to verify the correctness of the hex values used in all these instances?
There was a problem hiding this comment.
There is an implementation referenced in the whitepaper. This implementation is built by one of the authors of the hash
I see I should add a bit more clarifying comments |
Resolves #264
PR Checklist