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

chore: unify circuits in integration tests #1104

Merged
merged 2 commits into from
Jan 30, 2023
Merged

chore: unify circuits in integration tests #1104

merged 2 commits into from
Jan 30, 2023

Conversation

ed255
Copy link
Member

@ed255 ed255 commented Jan 24, 2023 •
edited
Loading

  • unify circuit proving test in integration tests with a single generic function that takes a SubCircuit
  • fix: Invalid SubCircuit.instance() implementation for TxCircuit
  • allow setting the mock_randomness for the SuperCircuit externally

Resolve #1099
Resolve #1081

Related to (hopefully this PR resolves some of these issues):

@github-actions github-actions bot added crate-integration-tests Issues related to the integration-tests workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Jan 24, 2023
@ed255 ed255 force-pushed the feature/unify-integration-tests-1 branch from 495f4f2 to 3f4358c Compare January 24, 2023 16:54
@ed255 ed255 force-pushed the feature/unify-integration-tests-1 branch from 3f4358c to 96fb753 Compare January 24, 2023 16:59
@github-actions github-actions bot added crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member T-bench Type: benchmark improvements labels Jan 24, 2023
@lispc
Copy link
Collaborator

lispc commented Jan 24, 2023 •
edited
Loading

Is it possible to use a struct to wrap the const generics of super circuit? It becomes longer and longer. difficult to read code without IDE

@ed255
Copy link
Member Author

ed255 commented Jan 25, 2023

Is it possible to use a struct to wrap the const generics of super circuit? It becomes longer and longer. difficult to read code without IDE

I wish that was possible... Unfortunately the only supported const generic types are integers, bool and char. I think the proper solution to solve this would be to change the halo2 API so that the Circuit::configure can take an extra argument with parameters (or take a &self). Here's a related issue privacy-scaling-explorations/halo2#106

@ed255
Copy link
Member Author

ed255 commented Jan 25, 2023

I'm going to explore removing some of the const generics in SuperCircuit.

@ed255
Copy link
Member Author

ed255 commented Jan 25, 2023

@lispc I managed to remove MAX_RWS and MAX_CALLDATA from the SuperCircuit.
The current const generics in the SuperCircuit are:

  • MAX_TXS, MAX_CALLDATA. These are necessary because the configuration of the PiCircuit requires these values to chose a particular rotation value for the constraints related to the TxTable. Maybe in scroll's version of the PiCircuit (the one that takes a keccak of the inputs and does RLP of transactions) doesn't need those parameters at configuration.
  • MOCK_RANDOMNESS. This value is used in the SuperCircuit configuration to mock the challenges randomness. Previously this was a constant, and I think it's cleaner as a parameter that can be chosen by the user of the library.

@ed255 ed255 mentioned this pull request Jan 25, 2023
Closed
@ed255 ed255 requested a review from CPerezz January 25, 2023 10:42
@CPerezz
Copy link
Member

CPerezz commented Jan 25, 2023

Note that this will partially help with #1097 as all of the tests will be now test/trait-based

@ed255 ed255 force-pushed the feature/unify-integration-tests-1 branch from 8d40c64 to 5fbae4f Compare January 25, 2023 16:07
@aguzmant103
Copy link
Member

@lispc are you happy to be the second reviewer?

CPerezz
CPerezz approved these changes Jan 26, 2023
View reviewed changes
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM!! Thanks for this work! It's nice to cleanup the codebase like this!

lispc
lispc approved these changes Jan 30, 2023
View reviewed changes
Copy link
Collaborator

@lispc lispc left a comment

Choose a reason for hiding this comment

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

lgtm. only one issue needs some investigation

ed255 added 2 commits January 30, 2023 10:52
@ed255
chore: unify circuits in integration tests
2e4464c 
- unify circuit proving test in integration tests with a single generic function that takes a SubCircuit
  - implement SubCircuit for SuperCircuit
- fix: Invalid `SubCircuit.instance()` implementation for TxCircuit
- allow setting the mock_randomness for the SuperCircuit externally
@ed255
feat(super): remove unnecessary const generics
7d66af3 
- Remove MAX_TXS, MAX_CALLDATA and MAX_RWS from SuperCircuitConfig
- Remove MAX_RWS and MAX_CALLDATA from SuperCircuit
@ed255 ed255 force-pushed the feature/unify-integration-tests-1 branch from 5fbae4f to 7d66af3 Compare January 30, 2023 09:55
@ed255 ed255 merged commit 6efb9b8 into main Jan 30, 2023
@ed255 ed255 mentioned this pull request Jan 30, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CPerezz CPerezz CPerezz approved these changes

@lispc lispc lispc approved these changes

Assignees
No one assigned
Labels
crate-circuit-benchmarks Issues related to the circuit-benchmarks 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 trigger-integration-tests
Projects
None yet
Milestone
No milestone
4 participants
@ed255 @lispc @CPerezz @aguzmant103