Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

neotheprogramist
Copy link

No description provided.

#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, CanonicalDeserialize, CanonicalSerialize)]
pub struct PoseidonDigest(pub Fp);

impl HashMarker for PoseidonDigest {}
Copy link
Owner

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);
Copy link
Owner

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),
Copy link
Owner

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

"the capacity element is initialized to 1"

index: 0,
input0: U256::from_limbs(x.0 .0),
input1: U256::from_limbs(y.0 .0),
input2: U256::from(0),
Copy link
Owner

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()))
Copy link
Owner

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([
Copy link
Owner

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() {
Copy link
Owner

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

Comment on lines +65 to +67
fn merge_with_int(seed: &PoseidonDigest, value: u64) -> PoseidonDigest {
PoseidonDigest(poseidon_hash_many([**seed, value.into()].to_vec()))
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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())
}

Comment on lines +71 to +73
fn hash_elements(elements: impl IntoIterator<Item = Fp>) -> PoseidonDigest {
PoseidonDigest(poseidon_hash_many(elements.into_iter().collect()))
}
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants