Extend Circuit trait to take parameters in config

[ed255]

The Circuit trait is extended with the following:

pub trait Circuit<F: Field> {
    /// [...]
    type Params: Default;

    fn params(&self) -> Self::Params {
        Self::Params::default()
    }

    fn configure_with_params(meta: &mut ConstraintSystem<F>, params: Self::Params) -> Self::Config {
        Self::configure(meta)
    }

    fn configure(meta: &mut ConstraintSystem<F>) -> Self::Config;
}

This allows runtime parametrization of the circuit configuration. The extension to the Circuit trait has been designed to minimize the breaking change: existing circuits only need to define the associated type Params.

Unfortunately "Associated type defaults" are unstable in Rust, otherwise this would be a non-breaking change. See rust-lang/rust#29661

Resolve #106

[leolara]

Not sure that the method params() should return the default params? Is it supposed to be override by implementations of the trait? Just curiosity.

LGTM!

[ed255]

Not sure that the method params() should return the default params? Is it supposed to be override by implementations of the trait? Just curiosity.

LGTM!

The reason to require the Default trait for type Params is to have a default implementation of the params trait method in order to minimize the changes after this API upgrade. So that old circuits can continue using the original configure method and not require implementing the params method.
But any circuit that wants to use runtime parameters during config should implement params and configure_with_params.

[han0110]

LGTM! Nice work!

Also I feel it's okay to just pass the Params by value (no clone on it at all), I think in most case it should be similar to Config which is a lightweight structure. Here is the diff if you feel it makes sense:

pass-params-by-value.diff

diff --git a/halo2_proofs/examples/serialization.rs b/halo2_proofs/examples/serialization.rs
index 52643cf9..12270ac5 100644
--- a/halo2_proofs/examples/serialization.rs
+++ b/halo2_proofs/examples/serialization.rs
@@ -141,12 +141,9 @@ fn main() {
 
     let f = File::open("serialization-test.pk").unwrap();
     let mut reader = BufReader::new(f);
-    let pk = ProvingKey::<G1Affine>::read::<_, StandardPlonk>(
-        &mut reader,
-        SerdeFormat::RawBytes,
-        &circuit.params(),
-    )
-    .unwrap();
+    let pk =
+        ProvingKey::<G1Affine>::read::<_, StandardPlonk>(&mut reader, SerdeFormat::RawBytes, ())
+            .unwrap();
 
     std::fs::remove_file("serialization-test.pk").unwrap();
 
diff --git a/halo2_proofs/src/dev.rs b/halo2_proofs/src/dev.rs
index b6b800b6..f028c09d 100644
--- a/halo2_proofs/src/dev.rs
+++ b/halo2_proofs/src/dev.rs
@@ -602,7 +602,7 @@ impl<F: FromUniformBytes<64> + Ord> MockProver<F> {
         let n = 1 << k;
 
         let mut cs = ConstraintSystem::default();
-        let config = ConcreteCircuit::configure_with_params(&mut cs, &circuit.params());
+        let config = ConcreteCircuit::configure_with_params(&mut cs, circuit.params());
         let cs = cs;
 
         assert!(
diff --git a/halo2_proofs/src/dev/cost.rs b/halo2_proofs/src/dev/cost.rs
index 6f3c6a11..dfef4f90 100644
--- a/halo2_proofs/src/dev/cost.rs
+++ b/halo2_proofs/src/dev/cost.rs
@@ -150,7 +150,7 @@ impl<G: PrimeGroup, ConcreteCircuit: Circuit<G::Scalar>> CircuitCost<G, Concrete
     pub fn measure(k: usize, circuit: &ConcreteCircuit) -> Self {
         // Collect the layout details.
         let mut cs = ConstraintSystem::default();
-        let config = ConcreteCircuit::configure_with_params(&mut cs, &circuit.params());
+        let config = ConcreteCircuit::configure_with_params(&mut cs, circuit.params());
         let mut assembly = Assembly {
             selectors: vec![vec![false; 1 << k]; cs.num_selectors],
         };
diff --git a/halo2_proofs/src/dev/gates.rs b/halo2_proofs/src/dev/gates.rs
index ca882f26..92889285 100644
--- a/halo2_proofs/src/dev/gates.rs
+++ b/halo2_proofs/src/dev/gates.rs
@@ -80,7 +80,7 @@ struct Gate {
 ///     }
 /// }
 ///
-/// let gates = CircuitGates::collect::<pallas::Base, MyCircuit>(&());
+/// let gates = CircuitGates::collect::<pallas::Base, MyCircuit>(());
 /// assert_eq!(
 ///     format!("{}", gates),
 ///     r#####"R1CS constraint:
@@ -104,7 +104,7 @@ pub struct CircuitGates {
 
 impl CircuitGates {
     /// Collects the gates from within the circuit.
-    pub fn collect<F: PrimeField, C: Circuit<F>>(params: &C::Params) -> Self {
+    pub fn collect<F: PrimeField, C: Circuit<F>>(params: C::Params) -> Self {
         // Collect the graph details.
         let mut cs = ConstraintSystem::default();
         let _ = C::configure_with_params(&mut cs, params);
diff --git a/halo2_proofs/src/plonk.rs b/halo2_proofs/src/plonk.rs
index 758eb46a..ab4dadc8 100644
--- a/halo2_proofs/src/plonk.rs
+++ b/halo2_proofs/src/plonk.rs
@@ -101,7 +101,7 @@ where
     pub fn read<R: io::Read, ConcreteCircuit: Circuit<C::Scalar>>(
         reader: &mut R,
         format: SerdeFormat,
-        params: &ConcreteCircuit::Params,
+        params: ConcreteCircuit::Params,
     ) -> io::Result<Self> {
         let mut k = [0u8; 4];
         reader.read_exact(&mut k)?;
@@ -151,7 +151,7 @@ where
     pub fn from_bytes<ConcreteCircuit: Circuit<C::Scalar>>(
         mut bytes: &[u8],
         format: SerdeFormat,
-        params: &ConcreteCircuit::Params,
+        params: ConcreteCircuit::Params,
     ) -> io::Result<Self> {
         Self::read::<_, ConcreteCircuit>(&mut bytes, format, params)
     }
@@ -337,7 +337,7 @@ where
     pub fn read<R: io::Read, ConcreteCircuit: Circuit<C::Scalar>>(
         reader: &mut R,
         format: SerdeFormat,
-        params: &ConcreteCircuit::Params,
+        params: ConcreteCircuit::Params,
     ) -> io::Result<Self> {
         let vk = VerifyingKey::<C>::read::<R, ConcreteCircuit>(reader, format, params)?;
         let l0 = Polynomial::read(reader, format)?;
@@ -372,7 +372,7 @@ where
     pub fn from_bytes<ConcreteCircuit: Circuit<C::Scalar>>(
         mut bytes: &[u8],
         format: SerdeFormat,
-        params: &ConcreteCircuit::Params,
+        params: ConcreteCircuit::Params,
     ) -> io::Result<Self> {
         Self::read::<_, ConcreteCircuit>(&mut bytes, format, params)
     }
diff --git a/halo2_proofs/src/plonk/circuit.rs b/halo2_proofs/src/plonk/circuit.rs
index 8fc7bf90..8eaf7455 100644
--- a/halo2_proofs/src/plonk/circuit.rs
+++ b/halo2_proofs/src/plonk/circuit.rs
@@ -680,7 +680,7 @@ pub trait Circuit<F: Field> {
     /// arrangement, column arrangement, etc.
     fn configure_with_params(
         meta: &mut ConstraintSystem<F>,
-        _params: &Self::Params,
+        _params: Self::Params,
     ) -> Self::Config {
         Self::configure(meta)
     }
diff --git a/halo2_proofs/src/plonk/keygen.rs b/halo2_proofs/src/plonk/keygen.rs
index 3363b03f..34a0e73b 100644
--- a/halo2_proofs/src/plonk/keygen.rs
+++ b/halo2_proofs/src/plonk/keygen.rs
@@ -26,7 +26,7 @@ use crate::{
 
 pub(crate) fn create_domain<C, ConcreteCircuit>(
     k: u32,
-    params: &ConcreteCircuit::Params,
+    params: ConcreteCircuit::Params,
 ) -> (
     EvaluationDomain<C::Scalar>,
     ConstraintSystem<C::Scalar>,
@@ -211,7 +211,7 @@ where
     ConcreteCircuit: Circuit<C::Scalar>,
     C::Scalar: FromUniformBytes<64>,
 {
-    let (domain, cs, config) = create_domain::<C, ConcreteCircuit>(params.k(), &circuit.params());
+    let (domain, cs, config) = create_domain::<C, ConcreteCircuit>(params.k(), circuit.params());
 
     if (params.n() as usize) < cs.minimum_rows() {
         return Err(Error::not_enough_rows_available(params.k()));
@@ -272,7 +272,7 @@ where
     ConcreteCircuit: Circuit<C::Scalar>,
 {
     let mut cs = ConstraintSystem::default();
-    let config = ConcreteCircuit::configure_with_params(&mut cs, &circuit.params());
+    let config = ConcreteCircuit::configure_with_params(&mut cs, circuit.params());
 
     let cs = cs;
 
diff --git a/halo2_proofs/src/plonk/prover.rs b/halo2_proofs/src/plonk/prover.rs
index 5840f25b..9a0f8f6c 100644
--- a/halo2_proofs/src/plonk/prover.rs
+++ b/halo2_proofs/src/plonk/prover.rs
@@ -68,7 +68,7 @@ where
 
     let domain = &pk.vk.domain;
     let mut meta = ConstraintSystem::default();
-    let config = ConcreteCircuit::configure_with_params(&mut meta, &circuits[0].params());
+    let config = ConcreteCircuit::configure_with_params(&mut meta, circuits[0].params());
 
     // Selector optimizations cannot be applied here; use the ConstraintSystem
     // from the verification key.

[ed255]

Also I feel it's okay to just pass the Params by value (no clone on it at all), I think in most case it should be similar to Config which is a lightweight structure. Here is the diff if you feel it makes sense:

Thanks for the suggestion, it makes sense to skip the reference. I think in some cases it will not be a lightweight structure, but in that case it can always be a big struct behind a Cell/RefCell, or even define type Params = &Foo. I'll apply the change later!

[ed255]

In my latest commit I've moved the implementation under a feature flag, so that when the flag is disabled, the API is the same as before to avoid breaking any existing circuit.

This also allows extending the config method instead of adding a new config_with_params which I think looks more natural when using the new API. I'm not 100% convinced if it's better to add a new config_with_params, or add the params to config.

Update: I've reverted overwriting the configure method, because I think keeping the original one with the original API will make it easier to combine circuits with parameters and circuits without parameters.

[leolara]

👍

[ed255]

@jonathanpwang as a user of the PSE halo2 fork I would like to hear your feedback on this feature :) Han told me you're already using runtime parameters, so hopefully this will be helpful for your circuits. Any particular though on the proposed API?

[jonathanpwang]

Thanks for the ping! Yes I have felt the pain of not having Params many times, so it is a very welcome addition :)

You can see that we already essentially use some version of Params, except we "trick" the current API by using environmental variables: https://github.com/axiom-crypto/halo2-lib/blob/61e516d5a9bf847428cfbc633d488bca83d86c31/halo2-base/src/gates/builder.rs#L492

One question is why Params needs to implement Default? It seems in principle not necessary. Maybe you could just have it have a default type type Params = ();? That way fn params() also has a default implementation.

Otherwise, LGTM!

[ed255]

Thanks for the ping! Yes I have felt the pain of not having Params many times, so it is a very welcome addition :)

You can see that we already essentially use some version of Params, except we "trick" the current API by using environmental variables: https://github.com/axiom-crypto/halo2-lib/blob/61e516d5a9bf847428cfbc633d488bca83d86c31/halo2-base/src/gates/builder.rs#L492

That's a cool hack!

One question is why Params needs to implement Default? It seems in principle not necessary. Maybe you could just have it have a default type type Params = ();? That way fn params() also has a default implementation.

The reason to require Params to implement Default is to have a default implementation of the params method, which allows transitioning to this new API on existing circuits with minimum changes (you just need to add type Params = (); to an existing circuit. Without the Default you would also need to add the params method to each circuit like this:

fn params(&self) {}

Having type Params = (); would have been great, because then transitioning to the new API wouldn't be a breaking change, and old circuits would just work. But unfortunately "Associated type defaults" are an unstable feature in Rust, so we would have to enable this feature, and then we would be tied to Rust Nightly (unstable features can't be used in Rust stable).

[jonathanpwang]

Ah OK, makes sense. LGTM!