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

Fix: the transcript should not depend on the proving order #223

Closed
wants to merge 1 commit into from

Conversation

kunxian-xia
Copy link
Collaborator

The input transcript to opcode circuits and table circuits should be the same.

This will fix #222 and also resolve the failure in #210.

@kunxian-xia kunxian-xia self-assigned this Sep 14, 2024
@kunxian-xia kunxian-xia added bug Something isn't working protocol labels Sep 14, 2024
@naure
Copy link
Collaborator

naure commented Sep 14, 2024

This seems unsound dangerous because each thread starting with the same transcript will get the same or correlated challenges.

→ Make each thread different, e.g. by appending a unique circuit name or ID.

Also, an argument by-ref &mut Transcript is expected to include the history of everything that happened in the function.

→ Change the argument to by-value Transcript, showing that it cannot be used afterwards.

In case you want to support a larger protocol that builds on top of that &mut Transcript, then it must include some result from all threads before returning.

→ Introduce a fork / merge API.

impl Transcript {
    /// Fork this transcript into n different threads.
    pub fn fork(&mut self, n: usize) -> Vec<Self> {
        let mut forks = Vec::with_capacity(n);
        for i in 0..n {
            let mut t = self.clone();
            t.append_field_element(&(i as u64).into());
            forks.push(t);
        }
        forks
    }

    /// Include the history of the forks into the current transcript.
    pub fn merge(&mut self, forks: Vec<Self>) {
        for fork in forks {
            self.append_field_element_ext(&fork.read_field_element_ext());
        }
    }
}

@naure naure mentioned this pull request Sep 14, 2024
Copy link
Collaborator

@naure naure left a comment

Choose a reason for hiding this comment

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

→ Alternative in #224 .

@naure
Copy link
Collaborator

naure commented Sep 14, 2024

This does solve the issue though, nice catch.

@kunxian-xia
Copy link
Collaborator Author

Close this as #224 is better.

kunxian-xia pushed a commit that referenced this pull request Sep 14, 2024
This is another approach to #223, fixing #222 / #210. Features:

- Prepare for parallel proving with independent transcripts per thread.
- Soundness with different transcripts in each thread.
- Type-safe API which captures the transcript by value.

There is no function to merge the threads back into one transcript, but
that can be added if ever needed.

---------

Co-authored-by: Aurélien Nicolas <[email protected]>
@kunxian-xia kunxian-xia deleted the fix/batch_prover_transcript branch September 20, 2024 08:14
hero78119 pushed a commit that referenced this pull request Sep 30, 2024
This is another approach to #223, fixing #222 / #210. Features:

- Prepare for parallel proving with independent transcripts per thread.
- Soundness with different transcripts in each thread.
- Type-safe API which captures the transcript by value.

There is no function to merge the threads back into one transcript, but
that can be added if ever needed.

---------

Co-authored-by: Aurélien Nicolas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working protocol
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Fix example riscv_add
2 participants