Make MaxWidthCellManager work with fixed signals - Issue #97 #135

atsupontio · 2023-09-19T04:47:41Z

close #97

@leolara Ready for review. I added max_width_fixed and renamed max_width to max_width_advice.

And, I fixed examples/poseidon.rs to use one CompilerConfig.

Please let me know if anything else is required!

leolara · 2023-09-19T09:58:31Z

src/plonkish/compiler/cell_manager.rs

            same_height,
        }
    }
 }

 impl CellManager for MaxWidthCellManager {
    fn place<F>(&self, unit: &mut CompilationUnit<F>) {
-        if (!unit.shared_signals.is_empty() || !unit.fixed_signals.is_empty()) && !self.same_height
+        if (!unit.shared_signals.is_empty()) && !self.same_height


@atsupontio this must be kept. Fixed signals should not work when same_height is disabled

leolara · 2023-09-19T10:01:23Z

src/plonkish/compiler/cell_manager.rs

+        let mut fixed_signal_row: usize = 0;
+
+        for fixed_signal in unit.fixed_signals.iter() {
+            let column = if placement.columns.len() <= fixed_signal_column {


@atsupontio you should add some tests, this will not work. Because placement.columns contains both advice and fixed signals, if there are both, for example when fixed_signal_column == 0 but there are already columns that are advice the fixed column will not be created and the wrong column will be picked up

leolara · 2023-09-19T10:03:25Z

src/plonkish/compiler/cell_manager.rs

+            }
+        }
+
+        placement.base_height = if fixed_signal_column != 0 {


@atsupontio what if there is more rows in the advice than in the fixed?

leolara · 2023-09-22T11:10:02Z

examples/poseidon.rs

@@ -661,25 +661,22 @@ fn poseidon_super_circuit<F: PrimeField + Eq + Hash>(
    lens: Lens,
 ) -> SuperCircuit<F, ValuesAndLens<F>> {
    super_circuit::<F, ValuesAndLens<F>, _>("poseidon", |ctx| {
-        let single_config = config(SingleRowCellManager {}, SimpleStepSelectorBuilder {});
+        let config = config(MaxWidthCellManager::new(2, 2, true), SimpleStepSelectorBuilder {});


@atsupontio I think it would be better if run the example with both Cell Managers

@leolara Thank you!
I'll fix it!

leolara · 2023-09-22T11:14:42Z

src/plonkish/compiler/cell_manager.rs

+
+        let mut fixed_signal_column: usize = 0;
+        let mut fixed_signal_row: usize = 0;
+        let mut column_pos = self.max_width_advice + fixed_signal_column;


@atsupontio we might not have used all the max_width_advice columns, for example max_width_advice can be 3, but there be only 2 forwards.

perhaps column pos is equivalent to the (number of actual advice columns inserted (placement.columns.len() before adding fixed columns)) + fixed_signal_column; so this variable is not needed.

I think the cleaner solution is:

Be adding for advice and fixed, the columns to different arrays, and after all have been added then merge both arrays into placement.columns.

@leolara Thanks for your great advice and ideas !

leolara · 2023-09-25T02:47:37Z

src/plonkish/compiler/cell_manager.rs


        for forward_signal in unit.forward_signals.iter() {
-            let column = if placement.columns.len() <= forward_signal_column {
+            let column = if placement.forward.len() <= forward_signal_column {


@atsupontio you are comparing signals with columns here, the left side should be forward_columns.len()

advice_columns.len()

leolara · 2023-09-25T02:48:15Z

src/plonkish/compiler/cell_manager.rs

+
+
+        for fixed_signal in unit.fixed_signals.iter() {
+            let column = if placement.fixed.len() <= fixed_signal_column {


@atsupontio the left side should be fixed_columns.len()

leolara · 2023-09-25T02:49:41Z

src/plonkish/compiler/cell_manager.rs

+            }
+        }
+
+        placement.base_height = if fixed_signal_row > forward_signal_row {


@atsupontio you can assign to a new variable max of fixed_signal_row, forward_signal_row and use the result in one expression

@leolara Thank you for your comment!
Is it possible to represent it concisely with the following code?

let mut max_row = std::cmp::max(fixed_signal_row, forward_signal_row); placement.base_height = if (fixed_signal_column != 0 && forward_signal_column != 0) { max_row as u32 + 1 } else { max_row as u32 }; placement.base_height = max_row;

In that code, you are setting max_row as mut, but you are not mutating it, also you are setting placement.base_height twice. Also, why are you checking both things for zero? If both are zero, max_row will be zero.

@leolara Since we don't know whether fixed_signal_row or forward_signal_row will be assigned to max_row, we need to check both xx_signal_column, don't we?
The xx_signal_column could be 0 even if the xx_signal_row is not 0.

leolara · 2023-09-25T02:50:32Z

src/plonkish/compiler/cell_manager.rs

@@ -262,20 +264,21 @@ impl CellManager for MaxWidthCellManager {

        let mut forward_signal_column: usize = 0;
        let mut forward_signal_row: usize = 0;
+        let mut forward_columns: Vec<Column> = Vec::new();


@atsupontio actually these are advice_colums, not forward_columns

Advice columns, are shared by forward, shared and internal signals

leolara · 2023-09-25T02:53:07Z

src/plonkish/compiler/cell_manager.rs

+
+        // forward signals: a, b; step1 internal: c1, d, e; step2 internal c2
+
+        let cm = MaxWidthCellManager {


@atsupontio you should this test with different combinations of max_width_advice and max_width_fixed.

Including corner cases, like 0 and 1.

If there are some repetation in the code use helper functions in the test module

atsupontio added 4 commits September 18, 2023 11:07

where is max_width_fixed?

218a047

where is max_width_fixed? privacy-scaling-explorations#97

87503ea

add max_width_fixed privacy-scaling-explorations#97

dd602f0

fix poseidon example privacy-scaling-explorations#97

1d4f872

leolara reviewed Sep 19, 2023

View reviewed changes

reflect the content of the review privacy-scaling-explorations#97

dbc8cc8

atsupontio requested a review from leolara September 20, 2023 04:11

leolara reviewed Sep 22, 2023

View reviewed changes

fix column position and poseidon example privacy-scaling-explorations#97

8479b41

atsupontio requested a review from leolara September 25, 2023 00:50

leolara reviewed Sep 25, 2023

View reviewed changes

Merge branch 'main' into add_fixed_signals_#97

85d26f5

leolara force-pushed the main branch 4 times, most recently from 03ddbb5 to 8397503 Compare November 19, 2023 05:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make MaxWidthCellManager work with fixed signals - Issue #97 #135

Make MaxWidthCellManager work with fixed signals - Issue #97 #135

atsupontio commented Sep 19, 2023

leolara Sep 19, 2023

leolara Sep 19, 2023

leolara Sep 19, 2023

leolara Sep 22, 2023

atsupontio Sep 25, 2023

leolara Sep 22, 2023

leolara Sep 22, 2023

atsupontio Sep 25, 2023

leolara Sep 25, 2023

leolara Sep 25, 2023

leolara Sep 25, 2023

leolara Sep 25, 2023

atsupontio Sep 25, 2023

leolara Sep 25, 2023

atsupontio Sep 25, 2023

leolara Sep 25, 2023

leolara Sep 25, 2023

leolara Sep 25, 2023

leolara Sep 25, 2023



		for fixed_signal in unit.fixed_signals.iter() {
		let column = if placement.fixed.len() <= fixed_signal_column {


		// forward signals: a, b; step1 internal: c1, d, e; step2 internal c2

		let cm = MaxWidthCellManager {

Make MaxWidthCellManager work with fixed signals - Issue #97 #135

Are you sure you want to change the base?

Make MaxWidthCellManager work with fixed signals - Issue #97 #135

Conversation

atsupontio commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment