Move config responsibilities from `Layouter` to `Chip` #253

therealyingtong · 2021-04-19T14:02:50Z

Closes #235

Review recommendations:

the most informative example to run is cargo run --example two-chip;
the sha256 example is commented out here and fixed in Fix sha256 example for new config abstraction #256

trait Chip<F: FieldExt> is responsible for:

configuring the underlying constraint system (i.e. creating columns, gates, permutations, lookups);
holding on to its own configuration: where previously we had layouter().config() we now have self.config(); and
loading any fixed configuration needed into the circuit at the beginning of circuit synthesis.

The "top-level" chip is simply the chip with the most comprehensive config, i.e. it provides all columns and permutations needed by other chips.

trait Layouter<F: FieldExt> is responsible for:

assign_region(&mut self) assigning values according to its own defined strategy;
namespacing;

The layouter knows nothing about any config. This is fine because when a chip calls layouter.assign_region(), it has access to self.config() to inform the layouter. So we no longer see a Layouter<C: Chip> generic type, and this removal also trickles down to RegionLayouter, Region.

codecov-commenter · 2021-04-19T14:17:09Z

Codecov Report

Merging #253 (6f199d3) into main (24d7100) will increase coverage by 0.28%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #253      +/-   ##
==========================================
+ Coverage   81.71%   81.99%   +0.28%     
==========================================
  Files          32       32              
  Lines        3221     3210      -11     
==========================================
  Hits         2632     2632              
+ Misses        589      578      -11

Impacted Files	Coverage Δ
src/circuit.rs	`0.00% <0.00%> (ø)`
src/circuit/layouter.rs	`0.00% <0.00%> (ø)`
src/plonk/circuit.rs	`61.80% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24d7100...6f199d3. Read the comment docs.

str4d

Partial review.

src/circuit.rs

src/circuit/layouter.rs

Co-authored-by: Jack Grigg <jack@electriccoin.co>

src/plonk/circuit.rs

examples/simple-example.rs

examples/two-chip.rs

examples/simple-example.rs

str4d · 2021-04-27T06:04:29Z

examples/simple-example.rs

@@ -275,7 +319,7 @@ struct MyCircuit<F: FieldExt> {

 impl<F: FieldExt> Circuit<F> for MyCircuit<F> {
    // Since we are using a single chip for everything, we can just reuse its config.
-    type Config = FieldConfig;
+    type Config = FieldConfigEnum;


Suggested change

type Config = FieldConfigEnum;

type Config = FieldConfig;

I think this could also maybe instead be type Chip = FieldChip, since I think now we'll always just construct the chip from its config immediately inside Circuit::synthesize. But that's a higher-level refactor (and would probably also require making the layouter choice an associated type), so let's leave that out of this PR.

examples/two-chip.rs

Co-authored-by: Jack Grigg <jack@electriccoin.co>

daira · 2021-05-02T15:49:31Z

examples/sha256/main.rs

-        hasher.finalize(layouter.namespace(|| "finalize"))
-    }
-}
+// //! Gadget and chips for the [SHA-256] hash function.


Use a block comment (/* ... */) so that when #256 fixes the example, we can see the actual changes in the diff. (Nice PR number btw 😆.)

daira · 2021-05-02T19:00:14Z

examples/two-chip.rs

+        a: Self::Num,
+        b: Self::Num,
+    ) -> Result<Self::Num, Error> {
+        let config = self.config().add_config.clone();


Is it possible to avoid these clones? I tried but the lifetime requirements defeated me.

These ones, I think not (at least if we want the sub-cores to be usable on their own). We still have a large reduction in config cloning from this PR.

daira

utACK

str4d

utACK

therealyingtong force-pushed the chip-config branch 2 times, most recently from 19c52dc to 7904da5 Compare April 21, 2021 15:59

therealyingtong changed the title ~~[draft] Move config responsibilities from Layouter to Chip~~ Move config responsibilities from Layouter to Chip Apr 21, 2021

therealyingtong marked this pull request as ready for review April 21, 2021 16:00

This was referenced Apr 21, 2021

[WIP] Fix ECC implementation to work with new Chip trait zcash/orchard#66

Merged

[WIP] Include Layouter as associated type on Core #238

Closed

str4d reviewed Apr 23, 2021

View reviewed changes

therealyingtong and others added 3 commits April 23, 2021 17:40

Move config responsiblities from Layouter to Chip trait

40e7bec

Only keep config() and loaded() generic on Chip trait

3ad1c43

Co-authored-by: Jack Grigg <jack@electriccoin.co>

[tmp] Comment out sha256 example

03da010

therealyingtong force-pushed the chip-config branch from 8e127ec to 03da010 Compare April 23, 2021 13:26

This was referenced Apr 23, 2021

Fix sha256 example for new config abstraction #256

Merged

[WIP] Implement ECC chip zcash/orchard#23

Closed

therealyingtong changed the base branch from main to patch-test-plonk-api April 23, 2021 13:47

Base automatically changed from patch-test-plonk-api to main April 23, 2021 21:51

Clippy fixes

7524c95

str4d requested changes Apr 27, 2021

View reviewed changes

therealyingtong and others added 4 commits April 28, 2021 10:47

Remove empty() methods from Config and Loaded traits

21aaab2

Co-authored-by: Jack Grigg <jack@electriccoin.co>

Remove superfluous clone() in doc example

b2d42ef

Co-authored-by: Jack Grigg <jack@electriccoin.co>

Clean up anchors in simple-example.rs

f3021be

Co-authored-by: Jack Grigg <jack@electriccoin.co>

Remove CellValue type

1f6b3c0

therealyingtong mentioned this pull request Apr 28, 2021

Optimized ECC chip zcash/orchard#73

Closed

daira reviewed May 2, 2021

View reviewed changes

daira approved these changes May 2, 2021

View reviewed changes

str4d approved these changes May 4, 2021

View reviewed changes

[tmp] Block quote SHA256 example

6f199d3

str4d merged commit cae6f6a into main May 4, 2021

str4d deleted the chip-config branch May 4, 2021 03:36

therealyingtong mentioned this pull request May 6, 2021

[book] Remove core/chip abstraction #260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move config responsibilities from `Layouter` to `Chip` #253

Move config responsibilities from `Layouter` to `Chip` #253

therealyingtong commented Apr 19, 2021 •

edited

Loading

codecov-commenter commented Apr 19, 2021 •

edited

Loading

str4d left a comment

str4d Apr 27, 2021

daira May 2, 2021 •

edited

Loading

daira May 2, 2021

str4d May 4, 2021

daira left a comment

str4d left a comment

Move config responsibilities from Layouter to Chip #253

Move config responsibilities from Layouter to Chip #253

Conversation

therealyingtong commented Apr 19, 2021 • edited Loading

codecov-commenter commented Apr 19, 2021 • edited Loading

Codecov Report

str4d left a comment

Choose a reason for hiding this comment

str4d Apr 27, 2021

Choose a reason for hiding this comment

daira May 2, 2021 • edited Loading

Choose a reason for hiding this comment

daira May 2, 2021

Choose a reason for hiding this comment

str4d May 4, 2021

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Move config responsibilities from `Layouter` to `Chip` #253

Move config responsibilities from `Layouter` to `Chip` #253

therealyingtong commented Apr 19, 2021 •

edited

Loading

codecov-commenter commented Apr 19, 2021 •

edited

Loading

daira May 2, 2021 •

edited

Loading