feat: Refactor R1CS shape to split the commitment key generation #315
Conversation
huitseeker
force-pushed
the
split_commitment_key_size
branch
from
8402257
to
94e527b
Compare
| } | ||
|
|
||
| /// Computes the number of generators required for the commitment key corresponding to shape `S`. | ||
| fn commitment_key_size<E: Engine>(S: &R1CSShape<E>, ck_floor: &CommitmentKeyHint<E>) -> usize { |
There was a problem hiding this comment.
Does it make sense to pass a vector/slice of R1CSShape objects to commitment_key method and have it pick the right size? This will avoid having two methods and logic above this layer?
There was a problem hiding this comment.
Thanks for the clarification! I think we could still have commitment_key to be a method inside R1CSShape rather than having two separate methods outside. For now, it takes (S: &R1CSShape, ck_floor: &CommitmentKeyHint) as arguments and returns a ck. In the future, it can take a collection of R1CSShape and hints and returns a ck.
In other words, I don't see what is gained from having two methods, one called commitment_key and another called commitment_key_size. Why not inline the contents of commitment_key_size inside commitment_key method and then move commitment_key to be a method under R1CSShape type rather than a stand-alone method?
TL;DR: splits off one major source of diff lines from PR microsoft#283. Inherently, there are many R1CS shapes to consider when tailoring public parameter creation to non-uniform step-circuits. However, the commitment key generation should only be done once for all circuits (once a suitable size has been determined by looking at all R1CS shapes). This splits the relevant Nova functions into `r1cs_shape_and_key`, `r1cs_shape` and `commitment_key` to enable the flexibility deamnded by the above model. In detail: - Renamed the `r1cs_shape` method across various files to `r1cs_shape_and_key`, indicating its functionality is to return both `R1CSShape` and `CommitmentKey`. - Altered function calls from `r1cs_shape` to `r1cs_shape_and_key` in files such as `direct.rs`, `nifs.rs`, `lib.rs` and `circuit.rs`, - Split the creation of `R1CSShape` and `CommitmentKey` into separate functions in the `NovaShape` object in `r1cs.rs` - Removed the `R1CS` struct in `mod.rs` as it only contained a phantom data, with related operations performed elsewhere. - Implemented changes to enhance code readability, including the addition of a new `commitment_key_size` function, and overall code reformatting for clarity.
huitseeker
force-pushed
the
split_commitment_key_size
branch
from
e8c89d1
to
a4ead79
Compare
|
Since this PR is stale, closing it for now. Please reopen if you wish to contribute. |
This is a fragment of #283
TL;DR:
Splits off one major source of diff lines from PR #283. Inherently, there are many R1CS shapes to consider when tailoring public parameter creation to non-uniform step-circuits. However, the commitment key generation should only be done once for all circuits (once a suitable size has been determined by looking at all R1CS shapes). This splits the relevant Nova functions into
r1cs_shape_and_key,r1cs_shapeandcommitment_keyto enable the flexibility deamnded by the above model.In detail:
r1cs_shapemethod across various files tor1cs_shape_and_key, indicating its functionality is to return bothR1CSShapeandCommitmentKey.r1cs_shapetor1cs_shape_and_keyin files such asdirect.rs,nifs.rs,lib.rsandcircuit.rs,R1CSShapeandCommitmentKeyinto separate functions in theNovaShapeobject inr1cs.rsR1CSstruct inmod.rsas it only contained a phantom data, with related operations performed elsewhere.commitment_key_sizefunction, and overall code reformatting for clarity.