Dynamic CircuitsParameters
#1445
Conversation
github-actions
bot
added
crate-bus-mapping
davidnevadoc
force-pushed
the
feat/dynamic-params
branch
4 times, most recently
from
c6350db to
1eb5bb2
Compare
There was a problem hiding this comment.
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:
CircuitInputStateRefneeds generic C because it contains&'a mut Block<C>CircuitInputBuilderneeds generic C because it contains&'a mut Block<C>Blockneeds generic C because it containsC: 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!
| let max_evm_rows = 0; | ||
| let max_keccak_rows = 0; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@davidnevadoc Could you take a look at this? This will be my last request for this PR :P
There was a problem hiding this comment.
Sure! I'll ping you when it's done.
There was a problem hiding this comment.
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.
davidnevadoc
force-pushed
the
feat/dynamic-params
branch
from
e6d48a3 to
9e0aa92
Compare
davidnevadoc
changed the title
CircuitsParametersCircuitsParameters
davidnevadoc
force-pushed
the
feat/dynamic-params
branch
from
abb09b7 to
52934e4
Compare
MaybeParams -> CircuitsParams CircuitsParams -> ConcreteCP UnsetParams -> DynamicCP
MaybeParams -> CircuitsParams CircuitsParams -> ConcreteCP UnsetParams -> DynamicCP
Co-authored-by: Eduard S. <[email protected]>
Co-authored-by: Eduard S. <[email protected]>
davidnevadoc
force-pushed
the
feat/dynamic-params
branch
from
0722616 to
b985391
Compare
- max_rows and max_steps were being confused - `gen_data` in test now uses dynamic parameters for all tests except variadic size test.
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:DynamicCPand be dynamically generated together with the witness.Issue Link
Close #954
Type of change
Contents
CircuitsParams, implemented by theConcreteCP(oldCircuitsParams) and the newDynamicCPCircutisParams:Block,CircuitInputBuilder,CircuitInputStateRef`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
CircuitsParamsfromBlockand create a new structure containing both, something likeBlockWithParamsorCircuitInputs. (suggested by @ed255 )Big thanks to @ed255 for providing the ideas and design on how to solve this issue !!:partying_face: