-
Notifications
You must be signed in to change notification settings - Fork 22
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
Poseidon hashing #11
base: main
Are you sure you want to change the base?
Poseidon hashing #11
Conversation
#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, CanonicalDeserialize, CanonicalSerialize)] | ||
pub struct PoseidonDigest(pub Fp); | ||
|
||
impl HashMarker for PoseidonDigest {} |
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.
I think this can be removed
input1: U256::from(0), | ||
input2: U256::from(0), | ||
}; | ||
let result = InstanceTrace::new(instance); |
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.
Using InstanceTrace will decrease performance since it is not optimised for speed but rather obtaining the values needed in the STARK trace.
I think this PR should implement a seperate poseidon hash function that's focussed more on performance (i.e. doesn't keep track of the state of rounds in the permutation). Let's move all the added methods and tests in this file to crypto/src/hash/poseidon.rs. I'd suggest using the parameters from builtins/src/poseidon/params.rs and using InstanceTrace::new
as a guide. Let's put the implementation in a new function called poseidon_permutation([Fp; 3])
.
index: 0, | ||
input0: U256::from_limbs(x.0 .0), | ||
input1: U256::from(0), | ||
input2: U256::from(0), |
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.
Might need to set input2
to 1
index: 0, | ||
input0: U256::from_limbs(x.0 .0), | ||
input1: U256::from_limbs(y.0 .0), | ||
input2: U256::from(0), |
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.
} | ||
|
||
fn merge(v0: &PoseidonDigest, v1: &PoseidonDigest) -> PoseidonDigest { | ||
PoseidonDigest(poseidon_hash_many([**v0, **v1].to_vec())) |
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.
Due to how frequently this function is called it has a big impact on the overall performance of the prover. Allocations on the heap every invocation makes this function much more expensive than it needs to be. Let's implement this without any Vec
fn poseidon_hash_many_4_example() { | ||
let elements = vec![Fp!("0"), Fp!("0"), Fp!("0"), Fp!("0")]; | ||
|
||
let expected = BigInt::new([ |
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.
How are all the expected values obtained?
@@ -259,4 +324,70 @@ mod tests { | |||
|
|||
assert_eq!(expected, permute([Fp::ZERO, Fp::ZERO, Fp::ZERO])); | |||
} | |||
|
|||
#[test] | |||
fn poseidon_hash_many_3_example() { |
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.
test name can just be "hash_3_values". Let's update the other test names as well. Also we should have tests for hash_single and poseidon_hash
fn merge_with_int(seed: &PoseidonDigest, value: u64) -> PoseidonDigest { | ||
PoseidonDigest(poseidon_hash_many([**seed, value.into()].to_vec())) | ||
} |
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.
fn merge_with_int(seed: &PoseidonDigest, value: u64) -> PoseidonDigest { | |
PoseidonDigest(poseidon_hash_many([**seed, value.into()].to_vec())) | |
} | |
fn merge_with_int(seed: &PoseidonDigest, value: u64) -> PoseidonDigest { | |
self.merge(seed.0, value.into()) | |
} |
fn hash_elements(elements: impl IntoIterator<Item = Fp>) -> PoseidonDigest { | ||
PoseidonDigest(poseidon_hash_many(elements.into_iter().collect())) | ||
} |
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.
Let's make this function behave the same way as poseidon_hash_many_given_poseidon_perm and use the same padding rule
No description provided.