Skip to content

Commit

Permalink
fix: Force VerifyFailure to own the annotations map (#131)
Browse files Browse the repository at this point in the history 
* 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

* chore: Address clippy lints
  • Loading branch information
@CPerezz
CPerezz authored Jan 20, 2023
1 parent e929fb3 commit c7e42e4
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 43 deletions.
2 changes: 1 addition & 1 deletion halo2_proofs/src/dev.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ impl<F: FieldExt> MockProver<F> {
} 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,
Expand Down
55 changes: 31 additions & 24 deletions halo2_proofs/src/dev/failure.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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),
Expand All @@ -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<F: Field>(
pub(super) fn find_expressions<'a, F: Field>(
cs: &ConstraintSystem<F>,
regions: &'a [Region],
regions: &[Region],
failure_row: usize,
failure_expressions: impl Iterator<Item = &'a Expression<F>>,
) -> Self {
Expand Down Expand Up @@ -94,7 +94,7 @@ impl<'a> FailureLocation<'a> {

/// 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<Column<Any>>,
) -> Self {
Expand All @@ -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 })
Expand All @@ -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,
Expand All @@ -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)>,
},
Expand Down Expand Up @@ -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 {
Expand All @@ -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
&region.column_annotations
}
_ => None,
_ => &None,
};

(DebugVirtualCell::from((vc, ann_map)), string)
(DebugVirtualCell::from((vc, ann_map.as_ref())), string)
}) {
writeln!(f, "- {} = {}", dvc, value)?;
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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,
};

Expand All @@ -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(),
};

Expand Down Expand Up @@ -605,7 +612,7 @@ fn render_lookup<F: FieldExt>(
}
}

impl<'a> VerifyFailure<'a> {
impl VerifyFailure {
/// Emits this failure in pretty-printed format to stderr.
pub(super) fn emit<F: FieldExt>(&self, prover: &MockProver<F>) {
match self {
Expand Down
3 changes: 2 additions & 1 deletion halo2_proofs/src/dev/failure/emitter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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))
}
Expand Down
31 changes: 14 additions & 17 deletions halo2_proofs/src/dev/metadata.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,50 +210,51 @@ 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,
/// The name of the region. This is specified by the region creator (such as a chip
/// 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<ColumnMetadata, String>>,
pub(super) column_annotations: Option<HashMap<ColumnMetadata, String>>,
}

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:
/// - There's no annotation map generated for this `Region`.
/// - There's no entry on the annotation map corresponding to the metadata provided.
pub(crate) fn get_column_annotation(&self, metadata: ColumnMetadata) -> Option<String> {
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,
Expand All @@ -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,
Expand All @@ -273,10 +274,8 @@ impl<'a> From<(usize, &str)> for Region<'a> {
}
}

impl<'a> From<(usize, String, &'a HashMap<ColumnMetadata, String>)> for Region<'a> {
fn from(
(index, name, annotations): (usize, String, &'a HashMap<ColumnMetadata, String>),
) -> Self {
impl From<(usize, String, HashMap<ColumnMetadata, String>)> for Region {
fn from((index, name, annotations): (usize, String, HashMap<ColumnMetadata, String>)) -> Self {
Region {
index,
name,
Expand All @@ -285,10 +284,8 @@ impl<'a> From<(usize, String, &'a HashMap<ColumnMetadata, String>)> for Region<'
}
}

impl<'a> From<(usize, &str, &'a HashMap<ColumnMetadata, String>)> for Region<'a> {
fn from(
(index, name, annotations): (usize, &str, &'a HashMap<ColumnMetadata, String>),
) -> Self {
impl From<(usize, &str, HashMap<ColumnMetadata, String>)> for Region {
fn from((index, name, annotations): (usize, &str, HashMap<ColumnMetadata, String>)) -> Self {
Region {
index,
name: name.to_owned(),
Expand Down

0 comments on commit c7e42e4

Please sign in to comment.