Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

[proof chunk] root circuit consistency between chunk #1693

Merged
merged 19 commits into from
Dec 12, 2023
Merged

[proof chunk] root circuit consistency between chunk #1693

merged 19 commits into from
Dec 12, 2023

Conversation

hero78119
Copy link
Member

@hero78119 hero78119 commented Nov 29, 2023 •
edited
Loading

Description

covered item

  • supercircuit simplify instance columns related to chunkctx to just one column
  • first chunk instance: chunk continuity related fields should be constraints as constant.
  • aggregate multiple super circuit proof chunk with chunk consistency check .
  • compute permutation fingerprint challenges alpha,gamma from rw_table advices commitments and assert its equality
  • generalized challenge([column_indexs], challenge_column_index) to a new protocol structure so more challenges in the future can be added.

TODO

Issue Link

#1603

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Tests

Light tests pass.
integration test failed due to challenge computation in witness not implemented yet, which will be done in bus mapping chunk PR.

@hero78119 hero78119 marked this pull request as draft November 29, 2023 09:05
@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-bench Type: benchmark improvements crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member crate-gadgets Issues related to the gadgets workspace member labels Nov 29, 2023
@hero78119 hero78119 changed the base branch from main to proof-chunk November 29, 2023 09:08
@github-actions github-actions bot removed crate-bus-mapping Issues related to the bus-mapping workspace member T-bench Type: benchmark improvements crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member crate-gadgets Issues related to the gadgets workspace member labels Nov 29, 2023
@hero78119 hero78119 force-pushed the proof-chunk-root-consistency branch 2 times, most recently from a853d5b to 084f129 Compare December 4, 2023 08:23
@hero78119 hero78119 mentioned this pull request Dec 4, 2023
Closed
4 tasks
@github-actions github-actions bot added T-bench Type: benchmark improvements crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member crate-integration-tests Issues related to the integration-tests workspace member crate-gadgets Issues related to the gadgets workspace member labels Dec 5, 2023
@hero78119 hero78119 marked this pull request as ready for review December 5, 2023 14:18
@hero78119 hero78119 changed the title [proof chunk] root circuit consistency between chunk [proof chunk] <verifying integration test on CI> root circuit consistency between chunk Dec 5, 2023
@hero78119
Copy link
Member Author

Hi @CeciliaZ030 this PR is for root circuit proof chunk poseidon challenges circuit & chunk consistency check.
Will be appreciate if you can also involve for reviewing, tks!

han0110
han0110 reviewed Dec 7, 2023
View reviewed changes
Copy link
Collaborator

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

Overall LGTM!

zkevm-circuits/src/witness/block.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/root_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/super_circuit.rs Show resolved Hide resolved
@hero78119 hero78119 changed the title [proof chunk] <verifying integration test on CI> root circuit consistency between chunk [proof chunk] root circuit consistency between chunk Dec 7, 2023
han0110
han0110 approved these changes Dec 7, 2023
View reviewed changes
zkevm-circuits/src/super_circuit.rs Show resolved Hide resolved
CeciliaZ030
CeciliaZ030 approved these changes Dec 10, 2023
View reviewed changes
Copy link
Contributor

@CeciliaZ030 CeciliaZ030 left a comment

Choose a reason for hiding this comment

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

LGTM! TheUserChallenge is very nice. I already started syncing this branch to #1690 and it doesn't look too hard to merge :) the RootCircuit & SuperCircuit are isolated and the chunk-related data I'll just manually move them to witness::Chunk<F>.
The only work integrate bus-mapping's witness generation toSuperCircuitInstance<T> which should be easy.

zkevm-circuits/src/root_circuit.rs Outdated
@@ -70,15 +139,22 @@ where
{
/// Create a `RootCircuit` with accumulator computed given a `SuperCircuit`
/// proof and its instance. Returns `None` if given proof is invalid.
/// TODO support multiple snark proof aggregation
pub fn new(
params: &ParamsKZG<M>,
super_circuit_protocol: &'a PlonkProtocol<M::G1Affine>,
Copy link
Contributor

Choose a reason for hiding this comment

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

In the actual aggregation situation where we're dealing with multiple chunks, should protocol, instances, and proof all be Vec? So that the flatten_super_circuit_instances will actually contain concatenated instances from different chunks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the heads up!
I think RootCircuit::new interface should be simplified to pass Vector of SnarkWitness.
Add one commit for applying the idea 0edf828

zkevm-circuits/src/witness/rw.rs

/// RwTablePermutationFingerprints
#[derive(Debug, Default, Clone)]
pub struct RwTablePermutationFingerprints<F> {
Copy link
Contributor

Choose a reason for hiding this comment

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

In #1690, I actually make a Chunk<F> that holds all chunk-related data, so l guess if this struct is incorporated in Chunk<F> then it wouldn't be necessary anymore?
For now it makes total sense so just keep it, just a heads up I'll probably get rid of it when I merge :)

@hero78119 hero78119 mentioned this pull request Dec 11, 2023
Merged
4 tasks
@hero78119
Copy link
Member Author

hero78119 commented Dec 12, 2023 •
edited
Loading

Since all test pass, will merge it first and proceed integration testing in #1690 :)

@hero78119 hero78119 merged commit c069ce4 into privacy-scaling-explorations:proof-chunk Dec 12, 2023
13 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@han0110 han0110 han0110 approved these changes

@CeciliaZ030 CeciliaZ030 CeciliaZ030 approved these changes

Assignees
No one assigned
Labels
crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member crate-gadgets Issues related to the gadgets workspace member crate-integration-tests Issues related to the integration-tests workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-bench Type: benchmark improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hero78119 @han0110 @CeciliaZ030