-
Notifications
You must be signed in to change notification settings - Fork 58
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 keccak transcript #27
base: main
Are you sure you want to change the base?
Changes from 8 commits
aa05b24
b160111
eed349e
b104408
69cd432
c8a2b8b
bf4cdbc
bc7b58c
0685d37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,92 @@ | ||||||||
use std::marker::PhantomData; | ||||||||
use tiny_keccak::{Keccak, Hasher}; | ||||||||
use ark_ec::CurveGroup; | ||||||||
use ark_ff::{BigInteger, PrimeField}; | ||||||||
|
||||||||
use crate::transcript::Transcript; | ||||||||
|
||||||||
/// KecccakTranscript implements the Transcript trait using the Keccak hash | ||||||||
pub struct KeccakTranscript<C: CurveGroup> { | ||||||||
sponge: Keccak, | ||||||||
phantom: PhantomData<C>, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This is how usually |
||||||||
} | ||||||||
|
||||||||
#[derive(Debug)] | ||||||||
pub struct KeccakConfig {} | ||||||||
|
||||||||
impl<C: CurveGroup> Transcript<C> for KeccakTranscript<C> { | ||||||||
type TranscriptConfig = KeccakConfig; | ||||||||
fn new(config: &Self::TranscriptConfig) -> Self { | ||||||||
let _ = config; | ||||||||
Comment on lines
+19
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're going to drop the config, why getting it as a parameter in first place? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comes from the Transcript trait, which in cases like Poseidon needs the config. For Keccak is not used, but needed in the function signature to fit with the Transcript trait, so happy with the most rust-style approach. What I see that is done in other similar arkworks cases is for example:
In our case for this Keccak256, we could do something like is done in the last link of sha256, something like:
Suggested change
|
||||||||
let sponge = Keccak::v256(); | ||||||||
Self { | ||||||||
sponge, | ||||||||
phantom: PhantomData, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
} | ||||||||
} | ||||||||
|
||||||||
fn absorb(&mut self, v: &C::ScalarField) { | ||||||||
self.sponge.update(&(v.into_bigint().to_bytes_le())); | ||||||||
} | ||||||||
fn absorb_vec(&mut self, v: &[C::ScalarField]) { | ||||||||
for _v in v { | ||||||||
self.sponge.update(&(_v.into_bigint().to_bytes_le())); | ||||||||
} | ||||||||
} | ||||||||
fn absorb_point(&mut self, p: &C) { | ||||||||
let mut serialized = vec![]; | ||||||||
p.serialize_compressed(&mut serialized).unwrap(); | ||||||||
self.sponge.update(&(serialized)) | ||||||||
Comment on lines
+38
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to make sure that the point is encoded as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding simulating the transcript inside a SNARK, I currently think (=I might be wrong) that we will not need it for the Keccak transcript as mentioned in this comment, since the keccak transcript is intended to be used only in non-circuit cases (eg. in Nova's main transcript), but not in-circuit (eg. HyperNova's SumCheck transcript, where we would use for example Poseidon's Transcript to later reproduce it in-circuit) (more details in the original comment itself). |
||||||||
} | ||||||||
fn get_challenge(&mut self) -> C::ScalarField { | ||||||||
let mut output = [0u8; 32]; | ||||||||
self.sponge.clone().finalize(&mut output); | ||||||||
self.sponge.update(&[output[0]]); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we re-initialize the sponge here? I saw other impls just drops the inputs after absorbing them. See: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @han0110 Thanks for pointing out! You mean we don't need to call I just checked, and it seems that since I have pushed a fix and new tests which can make sure that. ✅ 0685d37 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm not really, I mean we should re-initialize so it would look like: let mut output = [0u8; 32];
- self.sponge.clone().finalize(&mut output);
+ std::mem::replace(&mut self.sponge, Keccak::v256()).finalize(&mut output);
self.sponge.update(&[output[0]]); |
||||||||
C::ScalarField::from_le_bytes_mod_order(&[output[0]]) | ||||||||
} | ||||||||
fn get_challenge_nbits(&mut self, nbits: usize) -> Vec<bool> { | ||||||||
// TODO | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be addressed. |
||||||||
vec![] | ||||||||
} | ||||||||
fn get_challenges(&mut self, n: usize) -> Vec<C::ScalarField> { | ||||||||
let mut output = [0u8; 32]; | ||||||||
self.sponge.clone().finalize(&mut output); | ||||||||
self.sponge.update(&[output[0]]); | ||||||||
|
||||||||
let c: Vec<C::ScalarField> = output | ||||||||
.iter() | ||||||||
.map(|c| C::ScalarField::from_le_bytes_mod_order(&[*c])) | ||||||||
.collect(); | ||||||||
c[..n].to_vec() | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
#[cfg(test)] | ||||||||
pub mod tests { | ||||||||
use super::*; | ||||||||
use ark_pallas::{ | ||||||||
// constraints::GVar, | ||||||||
Fr, Projective | ||||||||
}; | ||||||||
use ark_std::UniformRand; | ||||||||
|
||||||||
/// WARNING the method poseidon_test_config is for tests only | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
#[cfg(test)] | ||||||||
pub fn keccak_test_config<F: PrimeField>() -> KeccakConfig { | ||||||||
KeccakConfig {} | ||||||||
} | ||||||||
|
||||||||
#[test] | ||||||||
fn test_transcript_get_challenge() { | ||||||||
let mut rng = ark_std::test_rng(); | ||||||||
|
||||||||
const n: usize = 10; | ||||||||
let config = keccak_test_config::<Fr>(); | ||||||||
|
||||||||
// init transcript | ||||||||
let mut transcript = KeccakTranscript::<Projective>::new(&config); | ||||||||
let v: Vec<Fr> = vec![Fr::rand(&mut rng); n]; | ||||||||
let challenges = transcript.get_challenges(v.len()); | ||||||||
assert_eq!(challenges.len(), n); | ||||||||
} | ||||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,91 @@ | ||||||
use std::marker::PhantomData; | ||||||
use sha3::{Shake256, digest::*}; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I presume is either for the capacity difference or the security difference after Berstein's discussions with NIST. |
||||||
use ark_ec::CurveGroup; | ||||||
use ark_ff::{BigInteger, PrimeField}; | ||||||
|
||||||
use crate::transcript::Transcript; | ||||||
|
||||||
/// SHA3Transcript implements the Transcript trait using the Keccak hash | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This says |
||||||
pub struct SHA3Transcript<C: CurveGroup> { | ||||||
sponge: Shake256, | ||||||
phantom: PhantomData<C>, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
#[derive(Debug)] | ||||||
pub struct SHA3Config {} | ||||||
|
||||||
impl<C: CurveGroup> Transcript<C> for SHA3Transcript<C> { | ||||||
type TranscriptConfig = SHA3Config; | ||||||
fn new(config: &Self::TranscriptConfig) -> Self { | ||||||
let _ = config; | ||||||
let sponge = Shake256::default(); | ||||||
Self { | ||||||
sponge, | ||||||
phantom: PhantomData, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
} | ||||||
|
||||||
fn absorb(&mut self, v: &C::ScalarField) { | ||||||
self.sponge.update(&(v.into_bigint().to_bytes_le())); | ||||||
} | ||||||
fn absorb_vec(&mut self, v: &[C::ScalarField]) { | ||||||
for _v in v { | ||||||
self.sponge.update(&(_v.into_bigint().to_bytes_le())); | ||||||
} | ||||||
} | ||||||
fn absorb_point(&mut self, p: &C) { | ||||||
let mut serialized = vec![]; | ||||||
p.serialize_compressed(&mut serialized).unwrap(); | ||||||
self.sponge.update(&(serialized)) | ||||||
} | ||||||
Comment on lines
+36
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto as with keccak. |
||||||
fn get_challenge(&mut self) -> C::ScalarField { | ||||||
let output = self.sponge.clone().finalize_boxed(200); | ||||||
self.sponge.update(&[output[0]]); | ||||||
C::ScalarField::from_le_bytes_mod_order(&[output[0]]) | ||||||
} | ||||||
fn get_challenge_nbits(&mut self, nbits: usize) -> Vec<bool> { | ||||||
// TODO | ||||||
// should call finalize() then slice the output to n bit challenge | ||||||
vec![] | ||||||
} | ||||||
Comment on lines
+45
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto as with keccak |
||||||
fn get_challenges(&mut self, n: usize) -> Vec<C::ScalarField> { | ||||||
let output = self.sponge.clone().finalize_boxed(n); | ||||||
self.sponge.update(&[output[0]]); | ||||||
|
||||||
let c = output | ||||||
.iter() | ||||||
.map(|c| C::ScalarField::from_le_bytes_mod_order(&[*c])) | ||||||
.collect(); | ||||||
c | ||||||
} | ||||||
} | ||||||
|
||||||
#[cfg(test)] | ||||||
pub mod tests { | ||||||
use super::*; | ||||||
use ark_pallas::{ | ||||||
// constraints::GVar, | ||||||
Fr, Projective | ||||||
}; | ||||||
use ark_std::UniformRand; | ||||||
|
||||||
/// WARNING the method poseidon_test_config is for tests only | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Use sha3 or whatever is decided at the end. But this is definitely not Poseidon. |
||||||
#[cfg(test)] | ||||||
pub fn sha3_test_config<F: PrimeField>() -> SHA3Config { | ||||||
SHA3Config {} | ||||||
} | ||||||
|
||||||
#[test] | ||||||
fn test_transcript_get_challenge() { | ||||||
let mut rng = ark_std::test_rng(); | ||||||
|
||||||
const n: usize = 10; | ||||||
let config = sha3_test_config::<Fr>(); | ||||||
|
||||||
// init transcript | ||||||
let mut transcript = SHA3Transcript::<Projective>::new(&config); | ||||||
let v: Vec<Fr> = vec![Fr::rand(&mut rng); n]; | ||||||
let challenges = transcript.get_challenges(v.len()); | ||||||
assert_eq!(challenges.len(), n); | ||||||
} | ||||||
} |
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 allow the minor to automatically update (although IIRC Cargo did it anyways). But JIC.
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.
Also, do we use
sha3
instead ofsha256
for some reason?