[chore] fix documentation (#215)

* chore: fix keccak comment

* chore: remove redundant

* chore: fix test case description

* chore: fix documentation

* chore: add comment

halo2-base/src/gates/circuit/builder.rs

@@ -34,12 +34,12 @@ pub type RangeCircuitBuilder<F> = BaseCircuitBuilder<F>;
 /// A circuit builder is a collection of virtual region managers that together assign virtual
 /// regions into a single physical circuit.
 ///
-/// [BaseCircuitBuilder] is a circuit builder to create a circuit where the columns correspond to [PublicBaseConfig].
-/// This builder can hold multiple threads, but the [Circuit] implementation only evaluates the first phase.
-/// The user will have to implement a separate [Circuit] with multi-phase witness generation logic.
+/// [BaseCircuitBuilder] is a circuit builder to create a circuit where the columns correspond to [super::BaseConfig].
+/// This builder can hold multiple threads, but the `Circuit` implementation only evaluates the first phase.
+/// The user will have to implement a separate `Circuit` with multi-phase witness generation logic.
 ///
-/// This is used to manage the virtual region corresponding to [FlexGateConfig] and (optionally) [RangeConfig].
-/// This can be used even if only using [GateChip] without [RangeChip].
+/// This is used to manage the virtual region corresponding to [super::FlexGateConfig] and (optionally) [RangeConfig].
+/// This can be used even if only using [`GateChip`](crate::gates::flex_gate::GateChip) without [RangeChip].
 ///
 /// The circuit will have `NI` public instance (aka public inputs+outputs) columns.
 #[derive(Clone, Debug, Getters, MutGetters, Setters)]
@@ -71,12 +71,12 @@ impl<F: ScalarField> BaseCircuitBuilder<F> {
     ///     * If false, the builder also imposes constraints (selectors, fixed columns, copy constraints). Primarily used for keygen and mock prover (but can also be used for real prover).
     ///
     /// By default, **no** circuit configuration parameters have been set.
-    /// These should be set separately using [use_params], or [use_k], [use_lookup_bits], and [config].
+    /// These should be set separately using `use_params`, or `use_k`, `use_lookup_bits`, and `calculate_params`.
     ///
     /// Upon construction, there are no public instances (aka all witnesses are private).
     /// The intended usage is that _before_ calling `synthesize`, witness generation can be done to populate
     /// assigned instances, which are supplied as `assigned_instances` to this struct.
-    /// The [`Circuit`] implementation for this struct will then expose these instances and constrain
+    /// The `Circuit` implementation for this struct will then expose these instances and constrain
     /// them using the Halo2 API.
     pub fn new(witness_gen_only: bool) -> Self {
         let core = MultiPhaseCoreManager::new(witness_gen_only);
@@ -209,7 +209,7 @@ impl<F: ScalarField> BaseCircuitBuilder<F> {
     }
 
     /// Creates a new [MultiPhaseCoreManager] with `use_unknown` flag set.
-    /// * `use_unknown`: If true, during key generation witness [Value]s are replaced with Value::unknown() for safety.
+    /// * `use_unknown`: If true, during key generation witness `Value`s are replaced with `Value::unknown()` for safety.
     pub fn unknown(mut self, use_unknown: bool) -> Self {
         self.core = self.core.unknown(use_unknown);
         self
@@ -321,7 +321,7 @@ impl<F: ScalarField> BaseCircuitBuilder<F> {
     ///
     /// ## Special case
     /// Just for [RangeConfig], we have special handling for the case where there is a single (physical)
-    /// advice column in [FlexGateConfig]. In this case, `RangeConfig` does not create extra lookup advice columns,
+    /// advice column in [super::FlexGateConfig]. In this case, `RangeConfig` does not create extra lookup advice columns,
     /// the single advice column has lookup enabled, and there is a selector to toggle when lookup should
     /// be turned on.
     pub fn assign_lookups_in_phase(

halo2-base/src/gates/circuit/mod.rs

@@ -146,12 +146,12 @@ impl<F: ScalarField> Circuit<F> for BaseCircuitBuilder<F> {
         self.config_params.clone()
     }
 
-    /// Creates a new instance of the [RangeCircuitBuilder] without witnesses by setting the witness_gen_only flag to false
+    /// Creates a new instance of the [BaseCircuitBuilder] without witnesses by setting the witness_gen_only flag to false
     fn without_witnesses(&self) -> Self {
         unimplemented!()
     }
 
-    /// Configures a new circuit using [`BaseConfigParams`]
+    /// Configures a new circuit using [`BaseCircuitParams`]
     fn configure_with_params(meta: &mut ConstraintSystem<F>, params: Self::Params) -> Self::Config {
         BaseConfig::configure(meta, params)
     }

halo2-base/src/gates/flex_gate/mod.rs

@@ -30,9 +30,9 @@ pub(super) const MAX_PHASE: usize = 3;
 /// # Vertical Gate Strategy:
 /// `q_0 * (a + b * c - d) = 0`
 /// where
-/// * a = value[0], b = value[1], c = value[2], d = value[3]
-/// * q = q_enable[0]
-/// * q is either 0 or 1 so this is just a simple selector
+/// * `a = value[0], b = value[1], c = value[2], d = value[3]`
+/// * `q = q_enable[0]`
+/// * `q` is either 0 or 1 so this is just a simple selector
 /// We chose `a + b * c` instead of `a * b + c` to allow "chaining" of gates, i.e., the output of one gate because `a` in the next gate.
 ///
 /// A configuration for a basic gate chip describing the selector, and advice column values.
@@ -115,7 +115,6 @@ pub struct FlexGateConfig<F: ScalarField> {
 impl<F: ScalarField> FlexGateConfig<F> {
     /// Generates a new [FlexGateConfig]
     ///
-    /// Assumes `num_advice` is a [Vec] of length [MAX_PHASE]
     /// * `meta`: [ConstraintSystem] of the circuit
     /// * `params`: see [FlexGateConfigParams]
     pub fn configure(meta: &mut ConstraintSystem<F>, params: FlexGateConfigParams) -> Self {
@@ -705,7 +704,7 @@ pub trait GateInstructions<F: ScalarField> {
     /// and that `indicator` has at most one `1` bit.
     /// * `ctx`: [Context] to add the constraints to
     /// * `a`: Iterator of [QuantumCell]'s that contains field elements
-    /// * `indicator`: Iterator of [AssignedValue]'s where indicator[i] == 1 if i == `idx`, otherwise 0
+    /// * `indicator`: Iterator of [AssignedValue]'s where `indicator[i] == 1` if `i == idx`, otherwise `0`
     fn select_by_indicator<Q>(
         &self,
         ctx: &mut Context<F>,

halo2-base/src/gates/flex_gate/threads/multi_phase.rs

@@ -10,7 +10,7 @@ use crate::{
 
 use super::SinglePhaseCoreManager;
 
-/// Virtual region manager for [FlexGateConfig] in multiple phases.
+/// Virtual region manager for [`FlexGateConfig`](super::super::FlexGateConfig) in multiple phases.
 #[derive(Clone, Debug, Default, CopyGetters)]
 pub struct MultiPhaseCoreManager<F: ScalarField> {
     /// Virtual region for each challenge phase. These cannot be shared across threads while keeping circuit deterministic.
@@ -20,7 +20,7 @@ pub struct MultiPhaseCoreManager<F: ScalarField> {
     /// Flag for witness generation. If true, the gate thread builder is used for witness generation only.
     #[getset(get_copy = "pub")]
     witness_gen_only: bool,
-    /// The `unknown` flag is used during key generation. If true, during key generation witness [Value]s are replaced with Value::unknown() for safety.
+    /// The `unknown` flag is used during key generation. If true, during key generation witness `Value`s are replaced with `Value::unknown()` for safety.
     #[getset(get_copy = "pub")]
     use_unknown: bool,
 }
@@ -59,7 +59,7 @@ impl<F: ScalarField> MultiPhaseCoreManager<F> {
     }
 
     /// Creates a new [MultiPhaseCoreManager] with `use_unknown` flag set.
-    /// * `use_unknown`: If true, during key generation witness [Value]s are replaced with Value::unknown() for safety.
+    /// * `use_unknown`: If true, during key generation witness values are replaced with `Value::unknown()` for safety.
     pub fn unknown(mut self, use_unknown: bool) -> Self {
         self.use_unknown = use_unknown;
         for pm in &mut self.phase_manager {

halo2-base/src/gates/flex_gate/threads/single_phase.rs

@@ -20,7 +20,7 @@ use crate::{
     virtual_region::manager::VirtualRegionManager,
 };
 
-/// Virtual region manager for [Vec<BasicGateConfig>] in a single challenge phase.
+/// Virtual region manager for [`Vec<BasicGateConfig>`] in a single challenge phase.
 /// This is the core manager for [Context]s.
 #[derive(Clone, Debug, Default, CopyGetters)]
 pub struct SinglePhaseCoreManager<F: ScalarField> {
@@ -43,8 +43,8 @@ pub struct SinglePhaseCoreManager<F: ScalarField> {
 }
 
 impl<F: ScalarField> SinglePhaseCoreManager<F> {
-    /// Creates a new [GateThreadBuilder] and spawns a main thread.
-    /// * `witness_gen_only`: If true, the [GateThreadBuilder] is used for witness generation only.
+    /// Creates a new [SinglePhaseCoreManager] and spawns a main thread.
+    /// * `witness_gen_only`: If true, the [SinglePhaseCoreManager] is used for witness generation only.
     ///     * If true, the gate thread builder only does witness asignments and does not store constraint information -- this should only be used for the real prover.
     ///     * If false, the gate thread builder is used for keygen and mock prover (it can also be used for real prover) and the builder stores circuit information (e.g. copy constraints, fixed columns, enabled selectors).
     ///         * These values are fixed for the circuit at key generation time, and they do not need to be re-computed by the prover in the actual proving phase.
@@ -64,7 +64,7 @@ impl<F: ScalarField> SinglePhaseCoreManager<F> {
         Self { phase, ..self }
     }
 
-    /// Creates a new [GateThreadBuilder] depending on the stage of circuit building. If the stage is [CircuitBuilderStage::Prover], the [GateThreadBuilder] is used for witness generation only.
+    /// Creates a new [SinglePhaseCoreManager] depending on the stage of circuit building. If the stage is [CircuitBuilderStage::Prover], the [SinglePhaseCoreManager] is used for witness generation only.
     pub fn from_stage(
         stage: CircuitBuilderStage,
         copy_manager: SharedCopyConstraintManager<F>,
@@ -73,7 +73,7 @@ impl<F: ScalarField> SinglePhaseCoreManager<F> {
             .unknown(stage == CircuitBuilderStage::Keygen)
     }
 
-    /// Creates a new [GateThreadBuilder] with `use_unknown` flag set.
+    /// Creates a new [SinglePhaseCoreManager] with `use_unknown` flag set.
     /// * `use_unknown`: If true, during key generation witness [Value]s are replaced with Value::unknown() for safety.
     pub fn unknown(self, use_unknown: bool) -> Self {
         Self { use_unknown, ..self }

halo2-base/src/gates/range/mod.rs

@@ -264,7 +264,7 @@ pub trait RangeInstructions<F: ScalarField> {
     /// * a: [AssignedValue] value to check
     /// * b: upper bound as [BigUint] value
     ///
-    /// For the current implementation using [`is_less_than`], we require `ceil(b.bits() / lookup_bits) + 1 < F::NUM_BITS / lookup_bits`
+    /// For the current implementation using `is_less_than`, we require `ceil(b.bits() / lookup_bits) + 1 < F::NUM_BITS / lookup_bits`
     fn is_big_less_than_safe(
         &self,
         ctx: &mut Context<F>,
@@ -422,7 +422,7 @@ pub trait RangeInstructions<F: ScalarField> {
 pub struct RangeChip<F: ScalarField> {
     /// Underlying [GateChip] for this chip.
     pub gate: GateChip<F>,
-    /// Lookup manager for each phase, lazily initiated using the [SharedCopyConstraintManager] from the [Context]
+    /// Lookup manager for each phase, lazily initiated using the [`SharedCopyConstraintManager`](crate::virtual_region::copy_constraints::SharedCopyConstraintManager) from the [Context]
     /// that first calls it.
     ///
     /// The lookup manager is used to store the cells that need to be looked up in the range check lookup table.

halo2-base/src/gates/tests/flex_gate.rs

@@ -100,7 +100,7 @@ pub fn test_inner_product_left_last(
 }
 
 #[test_case([4,5,6].map(Fr::from).to_vec(), [1,2,3].map(|x| Constant(Fr::from(x))).to_vec() => (Fr::from(32), [4,5,6].map(Fr::from).to_vec());
-"inner_product_left(): <[1,2,3],[4,5,6]> Constant b starts with 1")]
+"inner_product_left(): <[4,5,6],[1,2,3]> Constant b starts with 1")]
 #[test_case([1,2,3].map(Fr::from).to_vec(), [4,5,6].map(|x| Witness(Fr::from(x))).to_vec() => (Fr::from(32), [1,2,3].map(Fr::from).to_vec());
 "inner_product_left(): <[1,2,3],[4,5,6]> Witness")]
 pub fn test_inner_product_left(a: Vec<Fr>, b: Vec<QuantumCell<Fr>>) -> (Fr, Vec<Fr>) {

halo2-base/src/lib.rs

@@ -82,16 +82,16 @@ pub enum QuantumCell<F: ScalarField> {
 }
 
 impl<F: ScalarField> From<AssignedValue<F>> for QuantumCell<F> {
-    /// Converts an [AssignedValue<F>] into a [QuantumCell<F>] of [type Existing(AssignedValue<F>)]
+    /// Converts an [`AssignedValue<F>`] into a [`QuantumCell<F>`] of enum variant `Existing`.
     fn from(a: AssignedValue<F>) -> Self {
         Self::Existing(a)
     }
 }
 
 impl<F: ScalarField> QuantumCell<F> {
-    /// Returns an immutable reference to the underlying [ScalarField] value of a QuantumCell<F>.
+    /// Returns an immutable reference to the underlying [ScalarField] value of a [`QuantumCell<F>`].
     ///
-    /// Panics if the QuantumCell<F> is of type WitnessFraction.
+    /// Panics if the [`QuantumCell<F>`] is of type `WitnessFraction`.
     pub fn value(&self) -> &F {
         match self {
             Self::Existing(a) => a.value(),
@@ -138,9 +138,9 @@ pub struct AssignedValue<F: crate::ff::Field> {
 }
 
 impl<F: ScalarField> AssignedValue<F> {
-    /// Returns an immutable reference to the underlying value of an AssignedValue<F>.
+    /// Returns an immutable reference to the underlying value of an [`AssignedValue<F>`].
     ///
-    /// Panics if the AssignedValue<F> is of type WitnessFraction.
+    /// Panics if the witness value is of type [Assigned::Rational] or [Assigned::Zero].
     pub fn value(&self) -> &F {
         match &self.value {
             Assigned::Trivial(a) => a,
@@ -234,8 +234,7 @@ impl<F: ScalarField> Context<F> {
         ContextCell::new(self.type_id, self.context_id, self.advice.len() - 1)
     }
 
-    /// Pushes a [QuantumCell<F>] to the end of the `advice` column ([Vec] of advice cells) in this [Context].
-    /// * `input`: the cell to be assigned.
+    /// Virtually assigns the `input` within the current [Context], with different handling depending on the [QuantumCell] variant.
     pub fn assign_cell(&mut self, input: impl Into<QuantumCell<F>>) {
         // Determine the type of the cell and push it to the relevant vector
         match input.into() {
@@ -313,7 +312,7 @@ impl<F: ScalarField> Context<F> {
     /// Pushes multiple advice cells to the `advice` column of [Context] and enables them by enabling the corresponding selector specified in `gate_offset`.
     ///
     /// * `inputs`: Iterator that specifies the cells to be assigned
-    /// * `gate_offsets`: specifies relative offset from current position to enable selector for the gate (e.g., `0` is inputs[0]).
+    /// * `gate_offsets`: specifies relative offset from current position to enable selector for the gate (e.g., `0` is `inputs[0]`).
     ///     * `offset` may be negative indexing from the end of the column (e.g., `-1` is the last previously assigned cell)
     pub fn assign_region<Q>(
         &mut self,

halo2-base/src/safe_types/mod.rs

@@ -28,16 +28,16 @@ type RawAssignedValues<F> = Vec<AssignedValue<F>>;
 
 const BITS_PER_BYTE: usize = 8;
 
-/// SafeType's goal is to avoid out-of-range undefined behavior.
-/// When building circuits, it's common to use mulitple AssignedValue<F> to represent
-/// a logical varaible. For example, we might want to represent a hash with 32 AssignedValue<F>
-/// where each AssignedValue represents 1 byte. However, the range of AssignedValue<F> is much
-/// larger than 1 byte(0~255). If a circuit takes 32 AssignedValue<F> as inputs and some of them
+/// [`SafeType`]'s goal is to avoid out-of-range undefined behavior.
+/// When building circuits, it's common to use multiple [`AssignedValue<F>`]s to represent
+/// a logical variable. For example, we might want to represent a hash with 32 [`AssignedValue<F>`]
+/// where each [`AssignedValue`] represents 1 byte. However, the range of [`AssignedValue<F>`] is much
+/// larger than 1 byte(0~255). If a circuit takes 32 [`AssignedValue<F>`] as inputs and some of them
 /// are actually greater than 255, there could be some undefined behaviors.
-/// SafeType gurantees the value range of its owned AssignedValue<F>. So circuits don't need to
+/// [`SafeType`] gurantees the value range of its owned [`AssignedValue<F>`]. So circuits don't need to
 /// do any extra value checking if they take SafeType as inputs.
-/// TOTAL_BITS is the number of total bits of this type.
-/// BYTES_PER_ELE is the number of bytes of each element.
+/// - `TOTAL_BITS` is the number of total bits of this type.
+/// - `BYTES_PER_ELE` is the number of bytes of each element.
 #[derive(Clone, Debug)]
 pub struct SafeType<F: ScalarField, const BYTES_PER_ELE: usize, const TOTAL_BITS: usize> {
     // value is stored in little-endian.
@@ -255,9 +255,8 @@ impl<'a, F: ScalarField> SafeTypeChip<'a, F> {
 
     /// Converts a slice of AssignedValue(treated as little-endian) to VarLenBytes.
     ///
-    /// * ctx: Circuit [Context]<F> to assign witnesses to.
     /// * inputs: Slice representing the byte array.
-    /// * len: [AssignedValue]<F> witness representing the variable length of the byte array. Constrained to be `<= MAX_LEN`.
+    /// * len: [`AssignedValue<F>`] witness representing the variable length of the byte array. Constrained to be `<= MAX_LEN`.
     /// * MAX_LEN: [usize] representing the maximum length of the byte array and the number of elements it must contain.
     ///
     /// ## Assumptions
@@ -275,9 +274,8 @@ impl<'a, F: ScalarField> SafeTypeChip<'a, F> {
 
     /// Converts a vector of AssignedValue to [VarLenBytesVec]. Not encouraged to use because `MAX_LEN` cannot be verified at compile time.
     ///
-    /// * ctx: Circuit [Context]<F> to assign witnesses to.
     /// * inputs: Vector representing the byte array, right padded to `max_len`. See [VarLenBytesVec] for details about padding.
-    /// * len: [AssignedValue]<F> witness representing the variable length of the byte array. Constrained to be `<= max_len`.
+    /// * len: [`AssignedValue<F>`] witness representing the variable length of the byte array. Constrained to be `<= max_len`.
     /// * max_len: [usize] representing the maximum length of the byte array and the number of elements it must contain. We enforce this to be provided explictly to make sure length of `inputs` is determinstic.
     ///
     /// ## Assumptions
@@ -300,7 +298,6 @@ impl<'a, F: ScalarField> SafeTypeChip<'a, F> {
 
     /// Converts a slice of AssignedValue(treated as little-endian) to FixLenBytes.
     ///
-    /// * ctx: Circuit [Context]<F> to assign witnesses to.
     /// * inputs: Slice representing the byte array.
     /// * LEN: length of the byte array.
     pub fn raw_to_fix_len_bytes<const LEN: usize>(
@@ -313,7 +310,6 @@ impl<'a, F: ScalarField> SafeTypeChip<'a, F> {
 
     /// Converts a slice of AssignedValue(treated as little-endian) to FixLenBytesVec.
     ///
-    /// * ctx: Circuit [Context]<F> to assign witnesses to.
     /// * inputs: Slice representing the byte array.
     /// * len: length of the byte array. We enforce this to be provided explictly to make sure length of `inputs` is determinstic.
     pub fn raw_to_fix_len_bytes_vec(
@@ -328,6 +324,7 @@ impl<'a, F: ScalarField> SafeTypeChip<'a, F> {
         )
     }
 
+    /// Assumes that `bits <= inputs.len() * 8`.
     fn add_bytes_constraints(
         &self,
         ctx: &mut Context<F>,