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

z-tech/stir #13

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

z-tech/stir #13

wants to merge 110 commits into from

Conversation

z-tech
Copy link

@z-tech z-tech commented Jul 18, 2024

What does this PR do?

What does this PR not include?

  • FRI mapped to the same traits comes in a separate branch

Future plans (?):

  • Nimue
  • Rayon

@z-tech z-tech changed the title z-tech/stir: IN PROGRESS z-tech/stir Jul 24, 2024
Copy link

@WizardOfMenlo WizardOfMenlo left a comment

Choose a reason for hiding this comment

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

I have left a bunch of comments.

.DS_Store Outdated Show resolved Hide resolved
src/.DS_Store Outdated Show resolved Hide resolved
src/crypto/fields.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,46 @@
use ark_crypto_primitives::sponge::{Absorb, CryptographicSponge};

This comment was marked as resolved.

src/crypto/fs/mod.rs Outdated Show resolved Hide resolved
src/direct/verifier.rs Outdated Show resolved Hide resolved
src/direct/verifier.rs Show resolved Hide resolved
}

fn prove(&self, witness: &W) -> Self::Proof {
let challenges = witness.challenges(self.config.num_challenges);

Choose a reason for hiding this comment

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

I think this should not go trough witness, the witness should not be in charge of generating challenges.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I didn't even use it in STIR so I don't think there's any need to keep it.

{
challenge_answers: MultiPath<M>,
committed_values: Vec<Vec<F>>,
merkle_leaf_hash_param: LeafParam<M>,

Choose a reason for hiding this comment

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

I think these params should not be in the proof, we should have it as PhantomData.

Copy link
Author

Choose a reason for hiding this comment

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

How would you verify if they're not in the proof? MultiProof().verify() takes these as arguments

Choose a reason for hiding this comment

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

The verifier will have them in its own config no? So it can pass them to this.

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't bother me here and I think it would be weird to "pass it in" as an argument to the proof.verify(..., merkle_params): https://github.com/arkworks-rs/ldt/blob/z-tech/STIR/src/direct/proof.rs#L65, but if we delete proof.verify() entirely and move it onto the DirectProver implementation that seems fine to me

src/direct/proof.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@z-tech z-tech requested review from a team, Pratyush, mmagician and weikengchen and removed request for a team July 24, 2024 15:23
@z-tech

This comment was marked as resolved.

@z-tech
Copy link
Author

z-tech commented Jul 25, 2024

@Pratyush @WizardOfMenlo Recapping an offline discussion: there's an open question about if we want to include blake3 as a Merkle tree in crypto-primitives.

See this file: https://github.com/WizardOfMenlo/stir/blob/main/src/crypto/merkle_tree/blake3.rs

I'm resolving all other comments related this for now.

@z-tech z-tech marked this pull request as ready for review July 31, 2024 14:48
@z-tech z-tech requested a review from mmaker July 31, 2024 14:48
witness_coeff: DensePolynomial<F>,
) -> Self {
let mut sponge = S::new(&config.sponge_config);
sponge.absorb(&commitment.root());
Copy link
Author

Choose a reason for hiding this comment

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

@mmaker This is my first time looking at Nimue. The pattern I have here is absorb/ squeeze using this sponge. It looks v similar to this example in the Nimue docs: https://docs.rs/nimue/latest/nimue/#protocol-transcripts

Basically, if I swap the sponge in this struct for IOPattern::<H>::new and use it the same way, does this fix or improve some issue that I'm unaware of?

Copy link
Member

Choose a reason for hiding this comment

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

hey, yes pretty much!
The prover will be a Merlin instance, which can be generated from iopattern.into_merlin(). You can either have a mutable reference to it or just store the iopattern inside the config.

Note that if you have a similar struct for the verifier you'd have instead to useArthur, which can generated from (bytes of) the proof and the iopattern, but will keep a reference to the proof.

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.

3 participants