From b99fc14d22691658449c5357836fbf0db5437fe0 Mon Sep 17 00:00:00 2001 From: Jonathan Wang <31040440+jonathanpwang@users.noreply.github.com> Date: Sun, 12 Nov 2023 23:16:03 -0800 Subject: [PATCH 1/3] fix: use &str instead of TypeId in ContextTag --- .../gates/flex_gate/threads/single_phase.rs | 15 +++++------- halo2-base/src/lib.rs | 24 ++++++++++--------- .../src/virtual_region/copy_constraints.rs | 3 +-- 3 files changed, 20 insertions(+), 22 deletions(-) diff --git a/halo2-base/src/gates/flex_gate/threads/single_phase.rs b/halo2-base/src/gates/flex_gate/threads/single_phase.rs index 1489bb90..ce61b937 100644 --- a/halo2-base/src/gates/flex_gate/threads/single_phase.rs +++ b/halo2-base/src/gates/flex_gate/threads/single_phase.rs @@ -1,4 +1,4 @@ -use std::{any::TypeId, cell::RefCell}; +use std::cell::RefCell; use getset::CopyGetters; @@ -13,10 +13,7 @@ use crate::{ Context, ContextCell, }; use crate::{ - halo2_proofs::{ - circuit::{Region, Value}, - plonk::{FirstPhase, SecondPhase, ThirdPhase}, - }, + halo2_proofs::circuit::{Region, Value}, virtual_region::manager::VirtualRegionManager, }; @@ -114,11 +111,11 @@ impl SinglePhaseCoreManager { } /// A distinct tag for this particular type of virtual manager, which is different for each phase. - pub fn type_of(&self) -> TypeId { + pub fn type_of(&self) -> &'static str { match self.phase { - 0 => TypeId::of::<(Self, FirstPhase)>(), - 1 => TypeId::of::<(Self, SecondPhase)>(), - 2 => TypeId::of::<(Self, ThirdPhase)>(), + 0 => "SinglePhaseCoreManager: FirstPhase", + 1 => "SinglePhaseCoreManager: SecondPhase", + 2 => "SinglePhaseCoreManager: ThirdPhase", _ => panic!("Unsupported phase"), } } diff --git a/halo2-base/src/lib.rs b/halo2-base/src/lib.rs index b2a06036..049488d6 100644 --- a/halo2-base/src/lib.rs +++ b/halo2-base/src/lib.rs @@ -9,8 +9,6 @@ #![warn(clippy::default_numeric_fallback)] #![warn(missing_docs)] -use std::any::TypeId; - use getset::CopyGetters; use itertools::Itertools; // Different memory allocator options: @@ -104,14 +102,16 @@ impl QuantumCell { } } -/// Unique tag for a context across all virtual regions -pub type ContextTag = (TypeId, usize); +/// Unique tag for a context across all virtual regions. +/// In the form `(type_id, context_id)` where `type_id` should be a unique identifier +/// for the virtual region this context belongs to, and `context_id` is a counter local to that virtual region. +pub type ContextTag = (&'static str, usize); /// Pointer to the position of a cell at `offset` in an advice column within a [Context] of `context_id`. #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)] pub struct ContextCell { - /// The [TypeId] of the virtual region that this cell belongs to. - pub type_id: TypeId, + /// The unique string identifier of the virtual region that this cell belongs to. + pub type_id: &'static str, /// Identifier of the [Context] that this cell belongs to. pub context_id: usize, /// Relative offset of the cell within this [Context] advice column. @@ -119,8 +119,8 @@ pub struct ContextCell { } impl ContextCell { - /// Creates a new [ContextCell] with the given `type_id`, `context_id`, and `offset`. - pub fn new(type_id: TypeId, context_id: usize, offset: usize) -> Self { + /// Creates a new [ContextCell] with the given `type_name`, `context_id`, and `offset`. + pub fn new(type_id: &'static str, context_id: usize, offset: usize) -> Self { Self { type_id, context_id, offset } } } @@ -174,9 +174,11 @@ pub struct Context { /// The challenge phase that this [Context] will map to. #[getset(get_copy = "pub")] phase: usize, - /// Identifier for what virtual region this context is in + /// Identifier for what virtual region this context is in. + /// Warning: the circuit writer must ensure that distinct virtual regions have distinct names as strings to prevent possible errors. + /// We do not use [std::any::TypeId] because it is not stable across rust builds or dependencies. #[getset(get_copy = "pub")] - type_id: TypeId, + type_id: &'static str, /// Identifier to reference cells from this [Context]. context_id: usize, @@ -204,7 +206,7 @@ impl Context { pub fn new( witness_gen_only: bool, phase: usize, - type_id: TypeId, + type_id: &'static str, context_id: usize, copy_manager: SharedCopyConstraintManager, ) -> Self { diff --git a/halo2-base/src/virtual_region/copy_constraints.rs b/halo2-base/src/virtual_region/copy_constraints.rs index e7eb866e..92e363ff 100644 --- a/halo2-base/src/virtual_region/copy_constraints.rs +++ b/halo2-base/src/virtual_region/copy_constraints.rs @@ -1,4 +1,3 @@ -use std::any::TypeId; use std::collections::{BTreeMap, HashMap}; use std::ops::DerefMut; use std::sync::{Arc, Mutex, OnceLock}; @@ -87,7 +86,7 @@ impl CopyConstraintManager { } fn load_external_cell_impl(&mut self, cell: Option) -> ContextCell { - let context_cell = ContextCell::new(TypeId::of::(), 0, self.external_cell_count); + let context_cell = ContextCell::new("External Raw Halo2 Cell", 0, self.external_cell_count); self.external_cell_count += 1; if let Some(cell) = cell { self.assigned_advices.insert(context_cell, cell); From 54ac13ec0a25e3a9a8d8edc4e20edf435158752e Mon Sep 17 00:00:00 2001 From: Jonathan Wang <31040440+jonathanpwang@users.noreply.github.com> Date: Sun, 12 Nov 2023 23:21:43 -0800 Subject: [PATCH 2/3] chore: add warning to readme --- halo2-base/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/halo2-base/README.md b/halo2-base/README.md index 9e7c5a36..14e16618 100644 --- a/halo2-base/README.md +++ b/halo2-base/README.md @@ -69,6 +69,8 @@ During `synthesize()`, the advice values of all `Context`s are concatenated into For parallel witness generation, multiple `Context`s are created for each parallel operation. After parallel witness generation, these `Context`'s are combined to form a single "virtual column" as above. Note that while the witness generation can be multi-threaded, the ordering of the contents in each `Context`, and the order of the `Context`s themselves, must be deterministic. +**Warning:** If you create your own `Context` in a new virtual region not provided by our libraries, you must ensure that the `type_id: &str` of the context is a globally unique identifier for the virtual region, distinct from the other `type_id` strings used to identify other virtual regions. In the future we will introduce a macro to check this uniqueness at compile time. + ### [**AssignedValue**](./src/lib.rs): Despite the name, an `AssignedValue` is a **virtual cell**. It contains the actual witness value as well as a pointer to the location of the virtual cell within a virtual region. The pointer is given by type `ContextCell`. We only store the pointer when not in witness generation only mode as an optimization. From 61736584781a173f5b4d3b0342dd2a016f57a6fb Mon Sep 17 00:00:00 2001 From: Jonathan Wang <31040440+jonathanpwang@users.noreply.github.com> Date: Sun, 12 Nov 2023 23:31:55 -0800 Subject: [PATCH 3/3] chore: fix comment --- halo2-base/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/halo2-base/src/lib.rs b/halo2-base/src/lib.rs index 049488d6..94f925f9 100644 --- a/halo2-base/src/lib.rs +++ b/halo2-base/src/lib.rs @@ -119,7 +119,7 @@ pub struct ContextCell { } impl ContextCell { - /// Creates a new [ContextCell] with the given `type_name`, `context_id`, and `offset`. + /// Creates a new [ContextCell] with the given `type_id`, `context_id`, and `offset`. pub fn new(type_id: &'static str, context_id: usize, offset: usize) -> Self { Self { type_id, context_id, offset } }