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

Dynamic CircuitsParameters #1445

Merged
merged 28 commits into from
Jun 22, 2023
Merged

Dynamic CircuitsParameters #1445

merged 28 commits into from
Jun 22, 2023

Conversation

davidnevadoc
Copy link
Contributor

@davidnevadoc davidnevadoc commented May 31, 2023 •
edited
Loading

Description

The main goal of this PR is to allow for the dynamic selection of sub-circuit parameters (CirctuisParams) based on the block provided as input. After this change there are 2 options for selecting circuit parameters:

  1. Having them fixed before witness generation
  2. Leaving them unset as DynamicCP and be dynamically generated together with the witness.

Issue Link

Close #954

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

Contents

  • Addition of trait CircuitsParams, implemented by the ConcreteCP (old CircuitsParams) and the new DynamicCP
  • Addition of generics in all the structs that previously contained CircutisParams: Block, CircuitInputBuilder, CircuitInputStateRef`
  • Modify unit tests to use dynamic parameters
  • Modify variadic test to use default fixed parameters instead of dynamic parameters

Rationale

How Has This Been Tested?

With the existing tests.

Potential improvements

Sometimes it may be difficult to track which structures contain the circuit parameters. For clarity, we could remove CircuitsParams from Block and create a new structure containing both, something like BlockWithParams or CircuitInputs. (suggested by @ed255 )

Big thanks to @ed255 for providing the ideas and design on how to solve this issue !!:partying_face:

@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-integration-tests Issues related to the integration-tests workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member T-bench Type: benchmark improvements labels May 31, 2023
@davidnevadoc davidnevadoc marked this pull request as ready for review June 2, 2023 11:54
@davidnevadoc davidnevadoc force-pushed the feat/dynamic-params branch 4 times, most recently from c6350db to 1eb5bb2 Compare June 2, 2023 12:41
@davidnevadoc davidnevadoc requested a review from ed255 June 2, 2023 15:00
ed255
ed255 reviewed Jun 8, 2023
View reviewed changes
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

First pass review. Overall looks good! The only topic I would like to discuss is the possibility to find an approach where CircuitInputBuilder and CircuitInputStateRef don't have the C: CircuitsParams generic. This causees that all gen_associated_ops are generic over C, but they don't have any code that depends on C. So I think it would be better if the generic appears at a later stage, when the paths bifurcate.

Currently:

  • CircuitInputStateRef needs generic C because it contains &'a mut Block<C>
  • CircuitInputBuilder needs generic C because it contains &'a mut Block<C>
  • Block needs generic C because it contains C: CircuitsParams>

The params are used in the bus-mapping at the CircuitInputBuilder::handle_block. In particular, they are required after processing each tx, before setting the end block steps. It would be nice to find a way such that no generic C appears before the params are needed. I'll think more about this!

bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
bus-mapping/src/circuit_input_builder.rs Outdated
Comment on lines 357 to 371
let max_evm_rows = 0;
let max_keccak_rows = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is using the fact that the EVM and Keccak circuit support the parameter 0, and then they perform assignments up to the required number of rows for the given witness.

But maybe this would be more clear if the calculation of these values is done here. This way when using the dynamic parameters, we can just print the generated parameters and see the values for each circuit at once.

Copy link
Member

Choose a reason for hiding this comment

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

@davidnevadoc Could you take a look at this? This will be my last request for this PR :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I'll ping you when it's done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't managed to compute either of these values so I have just added a comment with the rationale.
Essentially both of them depend on parameters that are fixed in zkevm-circuits and cannot be imported or otherwise accessed from bus-mapping.

bus-mapping/src/circuit_input_builder/block.rs Outdated Show resolved Hide resolved
bus-mapping/src/evm/opcodes/caller.rs Outdated Show resolved Hide resolved
bus-mapping/src/evm/opcodes/callop.rs Outdated Show resolved Hide resolved
bus-mapping/src/evm/opcodes/chainid.rs Show resolved Hide resolved
bus-mapping/src/lib.rs Show resolved Hide resolved
miha-stopar
miha-stopar approved these changes Jun 16, 2023
View reviewed changes
Copy link
Collaborator

@miha-stopar miha-stopar left a comment

Choose a reason for hiding this comment

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

LGTM!

bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
@davidnevadoc davidnevadoc changed the title [WIP] Dynamic CircuitsParameters Dynamic CircuitsParameters Jun 16, 2023
davidnevadoc and others added 13 commits June 20, 2023 19:16
@davidnevadoc
Fix rebase nits
b985391
@davidnevadoc
Fix max_exp_steps param
6a82fce
@davidnevadoc
Fix ExpCircuit parameter bug + Improve tests
159d576 
- max_rows and max_steps were being confused
- `gen_data` in test now uses dynamic parameters for all tests
  except variadic size test.
ed255
ed255 approved these changes Jun 22, 2023
View reviewed changes
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM! Great work!

@davidnevadoc davidnevadoc added this pull request to the merge queue Jun 22, 2023
Merged via the queue into main with commit 9e974d9 Jun 22, 2023
19 checks passed
@davidnevadoc davidnevadoc deleted the feat/dynamic-params branch June 22, 2023 15:43
@hero78119 hero78119 mentioned this pull request Jun 30, 2023
Closed
@lispc lispc mentioned this pull request Aug 2, 2023
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ed255 ed255 ed255 approved these changes

@miha-stopar miha-stopar miha-stopar approved these changes

Assignees
No one assigned
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member 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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unify parametrization of circuit sizes
3 participants
@davidnevadoc @miha-stopar @ed255