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

Add keccak256 hasher for transcript #2

Merged
merged 3 commits into from
Jan 17, 2023
Merged

Add keccak256 hasher for transcript #2

merged 3 commits into from
Jan 17, 2023

Conversation

kilic
Copy link

@kilic kilic commented Oct 20, 2021

Keccak256 option added to transcript.

What is different than blake2b procedure is output size of keccak256 is 32 bytes where it is 64 with blake2. We can of cource find a field element from 32 bytes of keccak256 output but it'd introduce a modulus bias. So in order to make output 64 bytes we followed the approach below.

self.state.update(&[KECCAK256_PREFIX_CHALLENGE]);
let mut state_lo = self.state.clone();
let mut state_hi = self.state.clone();
state_lo.update(&[KECCAK256_PREFIX_CHALLENGE_LO]);
state_hi.update(&[KECCAK256_PREFIX_CHALLENGE_HI]);
let hasher_lo = state_lo.clone();
let hasher_hi = state_hi.clone();
let result_lo: [u8; 32] = hasher_lo.finalize().as_slice().try_into().unwrap();
let result_hi: [u8; 32] = hasher_hi.finalize().as_slice().try_into().unwrap();

Notice that whereas KECCAK256_PREFIX_CHALLENGE is contributed to the running state KECCAK256_PREFIX_CHALLENGE_LO and KECCAK256_PREFIX_CHALLENGE_HI prefixes are forking the state.

Two 32 bytes result is concatenated to be reduced into a field element with from_u512 method.

Copy link

@therealyingtong therealyingtong left a comment

Choose a reason for hiding this comment

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

First pass review.

src/transcript.rs Outdated Show resolved Hide resolved
Comment on lines 386 to 389
let hasher_lo = state_lo.clone();
let hasher_hi = state_hi.clone();
let result_lo: [u8; 32] = hasher_lo.finalize().as_slice().try_into().unwrap();
let result_hi: [u8; 32] = hasher_hi.finalize().as_slice().try_into().unwrap();

Choose a reason for hiding this comment

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

Do we need the hasher_* variables?

Suggested change
let hasher_lo = state_lo.clone();
let hasher_hi = state_hi.clone();
let result_lo: [u8; 32] = hasher_lo.finalize().as_slice().try_into().unwrap();
let result_hi: [u8; 32] = hasher_hi.finalize().as_slice().try_into().unwrap();
let result_lo: [u8; 32] = state_lo.finalize().as_slice().try_into().unwrap();
let result_hi: [u8; 32] = state_hi.finalize().as_slice().try_into().unwrap();

Comment on lines 223 to 228
state_lo.update(&[KECCAK256_PREFIX_CHALLENGE_LO]);
state_hi.update(&[KECCAK256_PREFIX_CHALLENGE_HI]);
let hasher_lo = state_lo.clone();
let hasher_hi = state_hi.clone();
let result_lo: [u8; 32] = hasher_lo.finalize().as_slice().try_into().unwrap();
let result_hi: [u8; 32] = hasher_hi.finalize().as_slice().try_into().unwrap();

Choose a reason for hiding this comment

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

Suggested change
state_lo.update(&[KECCAK256_PREFIX_CHALLENGE_LO]);
state_hi.update(&[KECCAK256_PREFIX_CHALLENGE_HI]);
let hasher_lo = state_lo.clone();
let hasher_hi = state_hi.clone();
let result_lo: [u8; 32] = hasher_lo.finalize().as_slice().try_into().unwrap();
let result_hi: [u8; 32] = hasher_hi.finalize().as_slice().try_into().unwrap();
state_lo.update(&[KECCAK256_PREFIX_CHALLENGE_LO]);
state_hi.update(&[KECCAK256_PREFIX_CHALLENGE_HI]);
let result_lo: [u8; 32] = state_lo.finalize().as_slice().try_into().unwrap();
let result_hi: [u8; 32] = state_hi.finalize().as_slice().try_into().unwrap();

Copy link

@therealyingtong therealyingtong left a comment

Choose a reason for hiding this comment

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

LGTM modulo questions.

kilic pushed a commit that referenced this pull request Feb 12, 2022
jonathanpwang referenced this pull request in axiom-crypto/halo2 Nov 9, 2022
Implement `ff::PrimeField::to_repr` for `bn256::Fq2`
@CPerezz
Copy link
Member

CPerezz commented Jan 10, 2023

We need to rebase and merge this. @kilic

Copy link

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM!

@CPerezz CPerezz merged commit 342d07a into main Jan 17, 2023
volhovm pushed a commit to volhovm/halo2 that referenced this pull request Mar 30, 2023
span14 pushed a commit to span14/halo2 that referenced this pull request Dec 1, 2023
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.

4 participants