From 6ce0f7ee60b4fc29a5f07111a1bf8b98eb3d73cd Mon Sep 17 00:00:00 2001 From: CPerezz Date: Fri, 20 Jan 2023 13:07:42 +0100 Subject: [PATCH 1/2] fix: Force `VerifyFailure` to own the annotations map Since otherwise we can't move the `VerifyFailure` vec's confortably, and also, we're required to have a lot of lifetime annotations, it was decided to force the `VerifyFailure` to own the Annotation maps. This shouldn't be too harmful as it only triggers when testing. Resolves: #130 --- halo2_proofs/src/dev.rs | 2 +- halo2_proofs/src/dev/failure.rs | 57 ++++++++++++++----------- halo2_proofs/src/dev/failure/emitter.rs | 3 +- halo2_proofs/src/dev/metadata.rs | 31 ++++++-------- 4 files changed, 49 insertions(+), 44 deletions(-) diff --git a/halo2_proofs/src/dev.rs b/halo2_proofs/src/dev.rs index 16801199be..35fce1daf6 100644 --- a/halo2_proofs/src/dev.rs +++ b/halo2_proofs/src/dev.rs @@ -659,7 +659,7 @@ impl MockProver { } else { Some(VerifyFailure::CellNotAssigned { gate: (gate_index, gate.name()).into(), - region: (r_i, r.name.clone(), &r.annotations).into(), + region: (r_i, r.name.clone(), r.annotations.clone()).into(), gate_offset: *selector_row, column: cell.column, offset: cell_row as isize - r.rows.unwrap().0 as isize, diff --git a/halo2_proofs/src/dev/failure.rs b/halo2_proofs/src/dev/failure.rs index 352f5752ce..ffc3dee813 100644 --- a/halo2_proofs/src/dev/failure.rs +++ b/halo2_proofs/src/dev/failure.rs @@ -22,11 +22,11 @@ mod emitter; /// The location within the circuit at which a particular [`VerifyFailure`] occurred. #[derive(Debug, PartialEq, Eq, Clone)] -pub enum FailureLocation<'a> { +pub enum FailureLocation { /// A location inside a region. InRegion { /// The region in which the failure occurred. - region: metadata::Region<'a>, + region: metadata::Region, /// The offset (relative to the start of the region) at which the failure /// occurred. offset: usize, @@ -38,7 +38,7 @@ pub enum FailureLocation<'a> { }, } -impl<'a> fmt::Display for FailureLocation<'a> { +impl fmt::Display for FailureLocation { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::InRegion { region, offset } => write!(f, "in {} at offset {}", region, offset), @@ -49,20 +49,20 @@ impl<'a> fmt::Display for FailureLocation<'a> { } } -impl<'a> FailureLocation<'a> { +impl FailureLocation { /// Returns a `DebugColumn` from Column metadata and `&self`. pub(super) fn get_debug_column(&self, metadata: metadata::Column) -> DebugColumn { match self { Self::InRegion { region, .. } => { - DebugColumn::from((metadata, region.column_annotations)) + DebugColumn::from((metadata, region.column_annotations.as_ref())) } _ => DebugColumn::from((metadata, None)), } } - pub(super) fn find_expressions( + pub(super) fn find_expressions<'a, F: Field>( cs: &ConstraintSystem, - regions: &'a [Region], + regions: &[Region], failure_row: usize, failure_expressions: impl Iterator>, ) -> Self { @@ -89,12 +89,12 @@ impl<'a> FailureLocation<'a> { }) .collect(); - Self::find(regions, failure_row, failure_columns) + Self::find(®ions, failure_row, failure_columns) } /// Figures out whether the given row and columns overlap an assigned region. pub(super) fn find( - regions: &'a [Region], + regions: &[Region], failure_row: usize, failure_columns: HashSet>, ) -> Self { @@ -114,7 +114,7 @@ impl<'a> FailureLocation<'a> { (start..=end).contains(&failure_row) && !failure_columns.is_disjoint(&r.columns) }) .map(|(r_i, r)| FailureLocation::InRegion { - region: (r_i, r.name.clone(), &r.annotations).into(), + region: (r_i, r.name.clone(), r.annotations.clone()).into(), offset: failure_row - r.rows.unwrap().0, }) .unwrap_or_else(|| FailureLocation::OutsideRegion { row: failure_row }) @@ -123,13 +123,13 @@ impl<'a> FailureLocation<'a> { /// The reasons why a particular circuit is not satisfied. #[derive(PartialEq, Eq)] -pub enum VerifyFailure<'a> { +pub enum VerifyFailure { /// A cell used in an active gate was not assigned to. CellNotAssigned { /// The index of the active gate. gate: metadata::Gate, /// The region in which this cell should be assigned. - region: metadata::Region<'a>, + region: metadata::Region, /// The offset (relative to the start of the region) at which the active gate /// queries this cell. gate_offset: usize, @@ -148,7 +148,7 @@ pub enum VerifyFailure<'a> { /// /// `FailureLocation::OutsideRegion` is usually caused by a constraint that does /// not contain a selector, and as a result is active on every row. - location: FailureLocation<'a>, + location: FailureLocation, /// The values of the virtual cells used by this constraint. cell_values: Vec<(metadata::VirtualCell, String)>, }, @@ -177,18 +177,18 @@ pub enum VerifyFailure<'a> { /// in the table when the lookup is not being used. /// - The input expressions use a column queried at a non-zero `Rotation`, and the /// lookup is active on a row adjacent to an unrelated region. - location: FailureLocation<'a>, + location: FailureLocation, }, /// A permutation did not preserve the original value of a cell. Permutation { /// The column in which this permutation is not satisfied. column: metadata::Column, /// The location at which the permutation is not satisfied. - location: FailureLocation<'a>, + location: FailureLocation, }, } -impl<'a> fmt::Display for VerifyFailure<'a> { +impl fmt::Display for VerifyFailure { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::CellNotAssigned { @@ -213,12 +213,12 @@ impl<'a> fmt::Display for VerifyFailure<'a> { for (dvc, value) in cell_values.iter().map(|(vc, string)| { let ann_map = match location { FailureLocation::InRegion { region, offset: _ } => { - region.column_annotations + ®ion.column_annotations } - _ => None, + _ => &None, }; - (DebugVirtualCell::from((vc, ann_map)), string) + (DebugVirtualCell::from((vc, ann_map.as_ref())), string) }) { writeln!(f, "- {} = {}", dvc, value)?; } @@ -254,7 +254,7 @@ impl<'a> fmt::Display for VerifyFailure<'a> { } } -impl<'a> Debug for VerifyFailure<'a> { +impl Debug for VerifyFailure { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { VerifyFailure::ConstraintNotSatisfied { @@ -264,14 +264,16 @@ impl<'a> Debug for VerifyFailure<'a> { } => { #[allow(dead_code)] #[derive(Debug)] - struct ConstraintCaseDebug<'a> { + struct ConstraintCaseDebug { constraint: Constraint, - location: FailureLocation<'a>, + location: FailureLocation, cell_values: Vec<(DebugVirtualCell, String)>, } let ann_map = match location { - FailureLocation::InRegion { region, offset: _ } => region.column_annotations, + FailureLocation::InRegion { region, offset: _ } => { + region.column_annotations.clone() + } _ => None, }; @@ -280,7 +282,12 @@ impl<'a> Debug for VerifyFailure<'a> { location: location.clone(), cell_values: cell_values .iter() - .map(|(vc, value)| (DebugVirtualCell::from((vc, ann_map)), value.clone())) + .map(|(vc, value)| { + ( + DebugVirtualCell::from((vc, ann_map.as_ref())), + value.clone(), + ) + }) .collect(), }; @@ -605,7 +612,7 @@ fn render_lookup( } } -impl<'a> VerifyFailure<'a> { +impl VerifyFailure { /// Emits this failure in pretty-printed format to stderr. pub(super) fn emit(&self, prover: &MockProver) { match self { diff --git a/halo2_proofs/src/dev/failure/emitter.rs b/halo2_proofs/src/dev/failure/emitter.rs index 13d4f25db6..e84ba8013e 100644 --- a/halo2_proofs/src/dev/failure/emitter.rs +++ b/halo2_proofs/src/dev/failure/emitter.rs @@ -69,7 +69,7 @@ pub(super) fn render_cell_layout( .map(|(col, _)| { let size = match location { FailureLocation::InRegion { region, offset: _ } => { - if let Some(column_ann) = region.column_annotations { + if let Some(column_ann) = region.column_annotations.as_ref() { if let Some(ann) = column_ann.get(col) { ann.len() } else { @@ -98,6 +98,7 @@ pub(super) fn render_cell_layout( FailureLocation::InRegion { region, offset: _ } => { region .column_annotations + .as_ref() .and_then(|column_ann| column_ann.get(column).cloned()) .unwrap_or_else(|| column_type_and_idx(column)) } diff --git a/halo2_proofs/src/dev/metadata.rs b/halo2_proofs/src/dev/metadata.rs index d0ab317e2e..5fd0835bad 100644 --- a/halo2_proofs/src/dev/metadata.rs +++ b/halo2_proofs/src/dev/metadata.rs @@ -210,7 +210,7 @@ impl From<(Gate, usize, &'static str)> for Constraint { /// Metadata about an assigned region within a circuit. #[derive(Clone)] -pub struct Region<'a> { +pub struct Region { /// The index of the region. These indices are assigned in the order in which /// `Layouter::assign_region` is called during `Circuit::synthesize`. pub(super) index: usize, @@ -218,10 +218,10 @@ pub struct Region<'a> { /// implementation), and is not enforced to be unique. pub(super) name: String, /// A reference to the annotations of the Columns that exist within this `Region`. - pub(super) column_annotations: Option<&'a HashMap>, + pub(super) column_annotations: Option>, } -impl<'a> Region<'a> { +impl Region { /// Fetch the annotation of a `Column` within a `Region` providing it's associated metadata. /// /// This function will return `None` if: @@ -229,31 +229,32 @@ impl<'a> Region<'a> { /// - There's no entry on the annotation map corresponding to the metadata provided. pub(crate) fn get_column_annotation(&self, metadata: ColumnMetadata) -> Option { self.column_annotations + .as_ref() .and_then(|map| map.get(&metadata).cloned()) } } -impl<'a> PartialEq for Region<'a> { +impl PartialEq for Region { fn eq(&self, other: &Self) -> bool { self.index == other.index && self.name == other.name } } -impl<'a> Eq for Region<'a> {} +impl Eq for Region {} -impl<'a> Debug for Region<'a> { +impl Debug for Region { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "Region {} ('{}')", self.index, self.name) } } -impl<'a> fmt::Display for Region<'a> { +impl fmt::Display for Region { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "Region {} ('{}')", self.index, self.name) } } -impl<'a> From<(usize, String)> for Region<'a> { +impl From<(usize, String)> for Region { fn from((index, name): (usize, String)) -> Self { Region { index, @@ -263,7 +264,7 @@ impl<'a> From<(usize, String)> for Region<'a> { } } -impl<'a> From<(usize, &str)> for Region<'a> { +impl From<(usize, &str)> for Region { fn from((index, name): (usize, &str)) -> Self { Region { index, @@ -273,10 +274,8 @@ impl<'a> From<(usize, &str)> for Region<'a> { } } -impl<'a> From<(usize, String, &'a HashMap)> for Region<'a> { - fn from( - (index, name, annotations): (usize, String, &'a HashMap), - ) -> Self { +impl From<(usize, String, HashMap)> for Region { + fn from((index, name, annotations): (usize, String, HashMap)) -> Self { Region { index, name, @@ -285,10 +284,8 @@ impl<'a> From<(usize, String, &'a HashMap)> for Region<' } } -impl<'a> From<(usize, &str, &'a HashMap)> for Region<'a> { - fn from( - (index, name, annotations): (usize, &str, &'a HashMap), - ) -> Self { +impl From<(usize, &str, HashMap)> for Region { + fn from((index, name, annotations): (usize, &str, HashMap)) -> Self { Region { index, name: name.to_owned(), From 89c734a7944ca10a458a0c1440a81cbaa92a7aa6 Mon Sep 17 00:00:00 2001 From: CPerezz Date: Fri, 20 Jan 2023 13:21:30 +0100 Subject: [PATCH 2/2] chore: Address clippy lints --- halo2_proofs/src/dev/failure.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/halo2_proofs/src/dev/failure.rs b/halo2_proofs/src/dev/failure.rs index ffc3dee813..d5a68c09d8 100644 --- a/halo2_proofs/src/dev/failure.rs +++ b/halo2_proofs/src/dev/failure.rs @@ -89,7 +89,7 @@ impl FailureLocation { }) .collect(); - Self::find(®ions, failure_row, failure_columns) + Self::find(regions, failure_row, failure_columns) } /// Figures out whether the given row and columns overlap an assigned region.