Add keccak transcript #27

grandchildrice · 2023-09-21T06:45:05Z

issue

what i do

Add Keccak Transcript using 2 libraries (tiny-keccak and SHA3)
- impl minimal functions
- impl fn get_challenge_nbits()
- impl KeccakTranscriptVar
Test it on Pedersen Circuit
Measure the performance
Decide which to use

comments

I didn't impl fn get_challenge_nbits() and KeccakTranscriptVar because they are currently not called by anywhere. Please let me know if we should include this scope on this PR.
After deciding which library to use, delete the rest and then we can merge it.

how to review

$ cargo test

arnaucube

Thanks for contribuiting ^^

I don't have much time now (currently OOO), but leaving some comments as a first high level pass.

Also, thanks a lot for providing the two versions (tiny_keccak & sha3 libs). I don't have strong opinions on which one to use and haven't checked what other projects are using (having Ethereum's EVM compatibility in mind), but probably @CPerezz & @han0110 have more thoughts on this.
Again, thanks for this PR!

src/pedersen.rs

src/transcript/keccak.rs

src/transcript/sha3.rs

grandchildrice · 2023-09-25T02:37:12Z

Hi @arnaucube Thanks for your feedbacks! I outdated all of your feedbacks right now.

han0110 · 2023-10-10T07:24:30Z

src/transcript/keccak.rs

+    fn get_challenge(&mut self) -> C::ScalarField {
+        let mut output = [0u8; 32];
+        self.sponge.clone().finalize(&mut output);
+        self.sponge.update(&[output[0]]);


Should we re-initialize the sponge here? I saw other impls just drops the inputs after absorbing them.

See:

https://github.com/AztecProtocol/barretenberg/blob/9de42a37415f8a3f1bd7de3ec460b1b299dd15fa/sol/src/ultra/BaseUltraVerifier.sol

https://github.com/matter-labs/solidity_plonk_verifier/blob/f6eea6bde11321d3104b20d100e9b6051102a2a2/bellman_vk_codegen/template.sol#L264

@han0110 Thanks for pointing out!

You mean we don't need to call absorb() (or update()) after squeeze() here, right?

I just checked, and it seems that since finalize() on tiny-keccak/SHA3 does not update the original sponge, thus we no need to call absorb() afterwards as you mentioned.

I have pushed a fix and new tests which can make sure that. ✅ 0685d37

hmm not really, I mean we should re-initialize self.sponge after we've called finalize, otherwise next time we call get_challenge it's equal to hashing all the previous thing again, for evm verifier it seems inefficient.

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]]);

arnaucube · 2023-10-11T09:17:43Z

@CPerezz @han0110 what are your thoughts between going with tiny-keccak or sha3?

CPerezz

Also @arnaucube. I don't have any preferences. The only thing that I'd make sure we address is checking exactly what to use here. If Sha2_256 or Sha3_256 or Shake256.

Aside from that, some nits that require addressing.

CPerezz · 2023-10-12T01:53:13Z

src/transcript/keccak.rs

+/// KecccakTranscript implements the Transcript trait using the Keccak hash
+pub struct KeccakTranscript<C: CurveGroup> {
+    sponge: Keccak,
+    phantom: PhantomData<C>,


Suggested change

phantom: PhantomData<C>,

_marker: PhantomData<C>,

This is how usually PhantomData is set inside of structs in rust.

CPerezz · 2023-10-12T01:54:29Z

src/transcript/keccak.rs

+        let sponge = Keccak::v256();
+        Self {
+            sponge,
+            phantom: PhantomData,


Suggested change

phantom: PhantomData,

_marker: PhantomData,

CPerezz · 2023-10-12T01:55:02Z

src/transcript/keccak.rs

+    fn new(config: &Self::TranscriptConfig) -> Self {
+        let _ = config;


If we're going to drop the config, why getting it as a parameter in first place?

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 Poseidon's CRH evaluate method:
https://github.com/arkworks-rs/crypto-primitives/tree/1e4f16cddba8d18f4eb219fa33ac25b2c852f198/src/crh/poseidon/mod.rs#L28

in Sha256 CRH evaluate method:
https://github.com/arkworks-rs/crypto-primitives/tree/1e4f16cddba8d18f4eb219fa33ac25b2c852f198/src/crh/sha256/mod.rs#L34

In our case for this Keccak256, we could do something like is done in the last link of sha256, something like:

Suggested change

fn new(config: &Self::TranscriptConfig) -> Self {

let _ = config;

fn new(_config: &Self::TranscriptConfig) -> Self {

CPerezz · 2023-10-12T02:19:22Z

src/transcript/keccak.rs

+        p.serialize_compressed(&mut serialized).unwrap();
+        self.sponge.update(&(serialized))


We need to make sure that the point is encoded as [bytes_x, bytes_y].flatten() format. If we're doing any size-optimizations like just storing one coordinate and the sign of the other, simulating the transcript inside of a SNARK (if we needed it at any point) wouldn't work.

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).

CPerezz · 2023-10-12T02:31:47Z

src/transcript/keccak.rs

+        C::ScalarField::from_le_bytes_mod_order(&[output[0]])
+    }
+    fn get_challenge_nbits(&mut self, nbits: usize) -> Vec<bool> {
+        // TODO


This should be addressed.

CPerezz · 2023-10-12T03:34:26Z

src/transcript/sha3.rs

+/// SHA3Transcript implements the Transcript trait using the Keccak hash
+pub struct SHA3Transcript<C: CurveGroup> {
+    sponge: Shake256,
+    phantom: PhantomData<C>,


Suggested change

phantom: PhantomData<C>,

_marker: PhantomData<C>,

CPerezz · 2023-10-12T03:34:33Z

src/transcript/sha3.rs

+        let sponge = Shake256::default();
+        Self {
+            sponge,
+            phantom: PhantomData,


Suggested change

phantom: PhantomData,

_marker: PhantomData,

CPerezz · 2023-10-12T03:35:53Z

src/transcript/sha3.rs

+    fn absorb_point(&mut self, p: &C) {
+        let mut serialized = vec![];
+        p.serialize_compressed(&mut serialized).unwrap();
+        self.sponge.update(&(serialized))
+    }


ditto as with keccak.

CPerezz · 2023-10-12T03:36:02Z

src/transcript/sha3.rs

+    fn get_challenge_nbits(&mut self, nbits: usize) -> Vec<bool> {
+        // TODO
+        // should call finalize() then slice the output to n bit challenge
+        vec![]
+    }


ditto as with keccak

CPerezz · 2023-10-12T03:36:41Z

src/transcript/sha3.rs

+    };
+    use ark_std::UniformRand;
+
+    /// WARNING the method poseidon_test_config is for tests only


Suggested change

/// WARNING the method poseidon_test_config is for tests only

/// WARNING the method `sha3_256_test_config` is for tests only

Use sha3 or whatever is decided at the end. But this is definitely not Poseidon.

CPerezz · 2023-10-23T14:15:18Z

Hey @0xvon how is this going? Can we help you with anything?

grandchildrice added 4 commits September 21, 2023 11:26

add tiny-keccak

aa05b24

add sha3

b160111

add pederson keccak test

eed349e

add pedersen sha3 test

b104408

grandchildrice mentioned this pull request Sep 21, 2023

Implement Keccak transcript #14

Open

impl absorb_vec

69cd432

arnaucube reviewed Sep 24, 2023

View reviewed changes

src/pedersen.rs Outdated Show resolved Hide resolved

src/transcript/keccak.rs Outdated Show resolved Hide resolved

src/transcript/keccak.rs Outdated Show resolved Hide resolved

src/transcript/sha3.rs Outdated Show resolved Hide resolved

src/transcript/sha3.rs Outdated Show resolved Hide resolved

grandchildrice added 3 commits September 25, 2023 09:17

remove circuit test since we not need keccak circuit for now

c8a2b8b

abstract pedersen vector test

bf4cdbc

just serialize the curve point

bc7b58c

arnaucube added the common label Oct 3, 2023

han0110 reviewed Oct 10, 2023

View reviewed changes

keccak/sha3 finalize do not change sponge so remove absorb afterwards

0685d37

arnaucube requested a review from CPerezz October 11, 2023 09:16

CPerezz requested changes Oct 12, 2023

View reviewed changes

arnaucube force-pushed the main branch 3 times, most recently from f21957b to d5c1e5f Compare May 6, 2024 21:32

arnaucube force-pushed the main branch from 163bf7f to 48947e8 Compare May 20, 2024 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add keccak transcript #27

Add keccak transcript #27

grandchildrice commented Sep 21, 2023 •

edited

Loading

arnaucube left a comment •

edited

Loading

grandchildrice commented Sep 25, 2023

han0110 Oct 10, 2023

grandchildrice Oct 10, 2023

han0110 Oct 10, 2023

arnaucube commented Oct 11, 2023

CPerezz left a comment

CPerezz Oct 12, 2023

CPerezz Oct 12, 2023

CPerezz Oct 12, 2023

arnaucube Oct 12, 2023 •

edited

Loading

CPerezz Oct 12, 2023

arnaucube Oct 12, 2023 •

edited

Loading

CPerezz Oct 12, 2023

CPerezz Oct 12, 2023

CPerezz Oct 12, 2023

CPerezz Oct 12, 2023

CPerezz Oct 12, 2023

CPerezz Oct 12, 2023

CPerezz commented Oct 23, 2023

		fn new(config: &Self::TranscriptConfig) -> Self {
		let _ = config;

	fn new(config: &Self::TranscriptConfig) -> Self {
	let _ = config;
	fn new(_config: &Self::TranscriptConfig) -> Self {

		p.serialize_compressed(&mut serialized).unwrap();
		self.sponge.update(&(serialized))

	/// WARNING the method poseidon_test_config is for tests only
	/// WARNING the method `sha3_256_test_config` is for tests only

Add keccak transcript #27

Are you sure you want to change the base?

Add keccak transcript #27

Conversation

grandchildrice commented Sep 21, 2023 • edited Loading

issue

what i do

comments

how to review

arnaucube left a comment • edited Loading

Choose a reason for hiding this comment

grandchildrice commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnaucube commented Oct 11, 2023

CPerezz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnaucube Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnaucube Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPerezz commented Oct 23, 2023

grandchildrice commented Sep 21, 2023 •

edited

Loading

arnaucube left a comment •

edited

Loading

arnaucube Oct 12, 2023 •

edited

Loading

arnaucube Oct 12, 2023 •

edited

Loading