From 1788efa83166d8e6e20c277ea6abde693426cd67 Mon Sep 17 00:00:00 2001 From: "sm.wu" Date: Thu, 5 Oct 2023 01:16:06 +0800 Subject: [PATCH] fix rwtable padding logic --- circuit-benchmarks/src/state_circuit.rs | 9 ++- gadgets/src/permutation.rs | 43 ++++++---- zkevm-circuits/src/evm_circuit.rs | 40 ++++----- zkevm-circuits/src/state_circuit.rs | 81 ++++++++++--------- .../src/state_circuit/constraint_builder.rs | 17 +++- zkevm-circuits/src/state_circuit/test.rs | 5 +- zkevm-circuits/src/super_circuit.rs | 2 +- zkevm-circuits/src/table/rw_table.rs | 7 +- 8 files changed, 121 insertions(+), 83 deletions(-) diff --git a/circuit-benchmarks/src/state_circuit.rs b/circuit-benchmarks/src/state_circuit.rs index 03eb41cbd24..14154350ffc 100644 --- a/circuit-benchmarks/src/state_circuit.rs +++ b/circuit-benchmarks/src/state_circuit.rs @@ -39,7 +39,14 @@ mod tests { .parse() .expect("Cannot parse DEGREE env var as u32"); - let empty_circuit = StateCircuit::::new(RwMap::default(), 1 << 16); + let empty_circuit = StateCircuit::::new( + RwMap::default(), + 1 << 16, + Fr::from(1), + Fr::from(1), + Fr::from(1), + Fr::from(1), + ); // Initialize the polynomial commitment parameters let mut rng = XorShiftRng::from_seed([ diff --git a/gadgets/src/permutation.rs b/gadgets/src/permutation.rs index 99f0a23c085..3a9f119a3e7 100644 --- a/gadgets/src/permutation.rs +++ b/gadgets/src/permutation.rs @@ -31,6 +31,13 @@ pub struct PermutationChipConfig { _phantom: PhantomData, } +type PermutationAssignedCells = ( + AssignedCell, + AssignedCell, + AssignedCell, + AssignedCell, +); + impl PermutationChipConfig { /// assign pub fn assign( @@ -41,15 +48,8 @@ impl PermutationChipConfig { prev_continuous_fingerprint: Value, _next_continuous_fingerprint: Value, col_values: &Vec>>, - ) -> Result< - ( - AssignedCell, - AssignedCell, - AssignedCell, - AssignedCell, - ), - Error, - > { + ) -> Result, Error> { + self.annotate_columns_in_region(region, "state_circuit"); let mut offset = 0; let alpha_first_cell = region.assign_advice( || format!("alpha at index {}", offset), @@ -72,11 +72,9 @@ impl PermutationChipConfig { let power_of_gamma = { let num_of_col = col_values.get(0).map(|x| x.len()).unwrap_or_default(); - std::iter::successors(Some(Value::known(F::ONE)), |prev| { - (prev.clone() * gamma.clone()).into() - }) - .take(num_of_col) - .collect::>>() + std::iter::successors(Some(Value::known(F::ONE)), |prev| (*prev * gamma).into()) + .take(num_of_col) + .collect::>>() }; offset += 1; @@ -94,8 +92,9 @@ impl PermutationChipConfig { .fold(Value::known(F::ZERO), |prev, cur| { prev.zip(cur).map(|(a, b)| a + b) }); - alpha.zip(tmp).map(|(alpha, perf_term)| alpha - perf_term) + alpha.zip(tmp).map(|(alpha, tmp)| alpha - tmp) }; + fingerprints = fingerprints.zip(perf_term).map(|(prev, cur)| prev * cur); let fingerprint_cell = region.assign_advice( || format!("fingerprint at index {}", offset), @@ -134,6 +133,20 @@ impl PermutationChipConfig { last_fingerprint_cell.unwrap(), )) } + + /// Annotates columns of this gadget embedded within a circuit region. + pub fn annotate_columns_in_region(&self, region: &mut Region, prefix: &str) { + [ + ( + self.fingerprints, + "GADGETS_PermutationChipConfig_fingerprints", + ), + (self.gamma, "GADGETS_PermutationChipConfig_gamma"), + (self.alpha, "GADGETS_PermutationChipConfig_alpha"), + ] + .iter() + .for_each(|(col, ann)| region.name_column(|| format!("{}_{}", prefix, ann), *col)); + } } /// permutation fingerprint gadget diff --git a/zkevm-circuits/src/evm_circuit.rs b/zkevm-circuits/src/evm_circuit.rs index eb9f48059ca..9bfdcd32d24 100644 --- a/zkevm-circuits/src/evm_circuit.rs +++ b/zkevm-circuits/src/evm_circuit.rs @@ -137,6 +137,10 @@ impl SubCircuitConfig for EvmCircuitConfig { u8_table.annotate_columns(meta); u16_table.annotate_columns(meta); + >::columns(&rw_table) + .iter() + .for_each(|c| meta.enable_equality(*c)); + let rw_permutation_config = PermutationChip::configure( meta, >::advice_columns(&rw_table), @@ -299,25 +303,22 @@ impl SubCircuit for EvmCircuit { config.execution.assign_block(layouter, block, challenges)?; let rw_rows = block.rws.table_assignments(true); - let (rw_table_row_first, rw_table_row_last) = layouter.assign_region( - || "evm circuit", - |mut region| { - config - .rw_table - .load_with_region(&mut region, &rw_rows, rw_rows.len()) - }, - )?; - - // permutation cells let ( - alpha_cell, - gamma_cell, - prev_continuous_fingerprint_cell, - next_continuous_fingerprint_cell, + (rw_table_row_first, rw_table_row_last), + ( + alpha_cell, + gamma_cell, + prev_continuous_fingerprint_cell, + next_continuous_fingerprint_cell, + ), ) = layouter.assign_region( - || "rw permutation fingerprint", + || "evm circuit", |mut region| { - config.rw_permutation_config.assign( + let rw_table_first_n_last_cells = + config + .rw_table + .load_with_region(&mut region, &rw_rows, rw_rows.len())?; + let permutation_cells = config.rw_permutation_config.assign( &mut region, Value::known(block.permu_alpha), Value::known(block.permu_gamma), @@ -327,6 +328,7 @@ impl SubCircuit for EvmCircuit { .rws .table_assignments(true) .iter() + .skip(1) // skip first row since it's used for permutation .map(|row| { row.table_assignment::() .unwrap() @@ -337,9 +339,11 @@ impl SubCircuit for EvmCircuit { .collect::>>() }) .collect::>>>(), - ) + )?; + Ok((rw_table_first_n_last_cells, permutation_cells)) }, )?; + // constrain permutation challenges [alpha_cell, gamma_cell] .iter() @@ -370,7 +374,7 @@ impl SubCircuit for EvmCircuit { let block = self.block.as_ref().unwrap(); let rws_assignments = block.rws.table_assignments(true); - assert!(rws_assignments.len() > 0); + assert!(!rws_assignments.is_empty()); let rws_values = [0, rws_assignments.len() - 1] // get first/last row and concat .iter() diff --git a/zkevm-circuits/src/state_circuit.rs b/zkevm-circuits/src/state_circuit.rs index e9d8ce731b5..49f74ab649a 100644 --- a/zkevm-circuits/src/state_circuit.rs +++ b/zkevm-circuits/src/state_circuit.rs @@ -172,6 +172,10 @@ impl SubCircuitConfig for StateCircuitConfig { u10_table.annotate_columns(meta); u16_table.annotate_columns(meta); + >::columns(&rw_table) + .iter() + .for_each(|c| meta.enable_equality(*c)); + let pi_pre_continuity = meta.instance_column(); let pi_next_continuity = meta.instance_column(); let pi_permutation_challenges = meta.instance_column(); @@ -205,9 +209,7 @@ impl SubCircuitConfig for StateCircuitConfig { constraint_builder.build(&queries); constraint_builder.gate(queries.selector) }); - for (name, lookup) in constraint_builder.lookups() { - meta.lookup_any(name, |_| lookup); - } + constraint_builder.lookups(meta, config.selector); config } @@ -255,11 +257,12 @@ impl StateCircuitConfig { log::trace!("state circuit assign offset:{} row:{:#?}", offset, row); } + // disable selector on offset 0 since it will be copy constraints by public input region.assign_fixed( || "selector", self.selector, offset, - || Value::known(F::ONE), + || Value::known(if offset == 0 { F::ZERO } else { F::ONE }), )?; tag_chip.assign(region, offset, &row.tag())?; @@ -532,15 +535,47 @@ impl SubCircuit for StateCircuit { // Assigning to same columns in different regions should be avoided. // Here we use one single region to assign `overrides` to both rw table and // other parts. - let (rw_table_row_first, rw_table_row_last) = layouter.assign_region( + let ( + (rw_table_row_first, rw_table_row_last), + ( + alpha_cell, + gamma_cell, + prev_continuous_fingerprint_cell, + next_continuous_fingerprint_cell, + ), + ) = layouter.assign_region( || "state circuit", |mut region| { + // TODO Below RwMap::table_assignments_prepad call 3 times, refactor to be more + // efficient let rw_table_first_n_last_cells = config .rw_table .load_with_region(&mut region, &self.rows, self.n_rows)?; config.assign_with_region(&mut region, &self.rows, &self.updates, self.n_rows)?; + + let (rows, _) = RwMap::table_assignments_prepad(&self.rows, self.n_rows); + + let permutation_cells = config.rw_permutation_config.assign( + &mut region, + Value::known(self.permu_alpha), + Value::known(self.permu_gamma), + Value::known(self.permu_prev_continuous_fingerprint), + Value::known(self.permu_next_continuous_fingerprint), + &rows + .iter() + .skip(1) // skip first row since it's used for permutation + .map(|row| { + row.table_assignment::() + .unwrap() + .values() + .iter() + .map(|f| Value::known(*f)) + .collect::>>() + }) + .collect::>>>(), + )?; #[cfg(test)] { let first_non_padding_index = if self.rows.len() < self.n_rows { @@ -564,39 +599,7 @@ impl SubCircuit for StateCircuit { } } - Ok(rw_table_first_n_last_cells) - }, - )?; - - // permutation cells - let ( - alpha_cell, - gamma_cell, - prev_continuous_fingerprint_cell, - next_continuous_fingerprint_cell, - ) = layouter.assign_region( - || "rw permutation fingerprint", - |mut region| { - config.rw_permutation_config.assign( - &mut region, - Value::known(self.permu_alpha), - Value::known(self.permu_gamma), - Value::known(self.permu_prev_continuous_fingerprint), - Value::known(self.permu_next_continuous_fingerprint), - &self - .rows - .iter() - .map(|row| { - row.table_assignment::() - .unwrap() - .values() - .to_vec() - .iter() - .map(|f| Value::known(*f)) - .collect::>>() - }) - .collect::>>>(), - ) + Ok((rw_table_first_n_last_cells, permutation_cells)) }, )?; // constrain permutation challenges @@ -625,7 +628,7 @@ impl SubCircuit for StateCircuit { } fn instance(&self) -> Vec> { - assert!(self.rows.len() > 0); + assert!(!self.rows.is_empty()); let rws_values = [0, self.rows.len() - 1] // get first/last row and concat .iter() diff --git a/zkevm-circuits/src/state_circuit/constraint_builder.rs b/zkevm-circuits/src/state_circuit/constraint_builder.rs index 1fb866cc0e5..ec4750191cb 100644 --- a/zkevm-circuits/src/state_circuit/constraint_builder.rs +++ b/zkevm-circuits/src/state_circuit/constraint_builder.rs @@ -9,7 +9,10 @@ use crate::{ use bus_mapping::operation::Target; use eth_types::Field; use gadgets::binary_number::BinaryNumberConfig; -use halo2_proofs::plonk::Expression; +use halo2_proofs::{ + plonk::{Column, ConstraintSystem, Expression, Fixed}, + poly::Rotation, +}; use strum::IntoEnumIterator; #[derive(Clone)] @@ -109,8 +112,16 @@ impl ConstraintBuilder { .collect() } - pub fn lookups(&self) -> Vec> { - self.lookups.clone() + pub fn lookups(&self, meta: &mut ConstraintSystem, selector: Column) { + self.lookups.iter().cloned().for_each(|(name, mut lookup)| { + meta.lookup_any(name, |meta| { + let selector = meta.query_fixed(selector, Rotation::cur()); + for (expression, _) in lookup.iter_mut() { + *expression = expression.clone() * selector.clone(); + } + lookup + }); + }); } pub fn build(&mut self, q: &Queries) { diff --git a/zkevm-circuits/src/state_circuit/test.rs b/zkevm-circuits/src/state_circuit/test.rs index 7a1231e9066..ca0f2074ee0 100644 --- a/zkevm-circuits/src/state_circuit/test.rs +++ b/zkevm-circuits/src/state_circuit/test.rs @@ -57,10 +57,7 @@ fn test_state_circuit_ok( let instance = circuit.instance(); let prover = MockProver::::run(19, &circuit, instance); - let verify_result = prover - .map_err(|err| println!("{:?}", err)) - .unwrap() - .verify(); + let verify_result = prover.map_err(|err| println!("{}", err)).unwrap().verify(); assert_eq!(verify_result, Ok(())); } diff --git a/zkevm-circuits/src/super_circuit.rs b/zkevm-circuits/src/super_circuit.rs index 5972e3f65a4..37f4f243089 100644 --- a/zkevm-circuits/src/super_circuit.rs +++ b/zkevm-circuits/src/super_circuit.rs @@ -187,7 +187,7 @@ impl SubCircuitConfig for SuperCircuitConfig { meta, CopyCircuitConfigArgs { tx_table: tx_table.clone(), - rw_table: chronological_rw_table.clone(), + rw_table: chronological_rw_table, bytecode_table: bytecode_table.clone(), copy_table, q_enable: q_copy_table, diff --git a/zkevm-circuits/src/table/rw_table.rs b/zkevm-circuits/src/table/rw_table.rs index 465447f9c53..7b2895010e1 100644 --- a/zkevm-circuits/src/table/rw_table.rs +++ b/zkevm-circuits/src/table/rw_table.rs @@ -67,6 +67,9 @@ impl LookupTable for RwTable { ] } } + +type RwTableFirstNLastAssignedCell = (Vec>, Vec>); + impl RwTable { /// Construct a new RwTable pub fn construct(meta: &mut ConstraintSystem) -> Self { @@ -129,7 +132,7 @@ impl RwTable { layouter: &mut impl Layouter, rws: &[Rw], n_rows: usize, - ) -> Result<(Vec>, Vec>), Error> { + ) -> Result, Error> { layouter.assign_region( || "rw table", |mut region| self.load_with_region(&mut region, rws, n_rows), @@ -141,7 +144,7 @@ impl RwTable { region: &mut Region<'_, F>, rws: &[Rw], n_rows: usize, - ) -> Result<(Vec>, Vec>), Error> { + ) -> Result, Error> { let mut assigned_cells = vec![]; let (rows, _) = RwMap::table_assignments_prepad(rws, n_rows); for (offset, row) in rows.iter().enumerate() {