Support multi-phase circuit #593

han0110 · 2022-06-01T10:45:44Z

This PR aims to support multi-phase circuit (closes #527). The main changes include:

commit@c87ba7a
- Add an enum Phase, which has 3 variant First, Second and Third (3 phases should be enough for most cases in my imagination).
- Add a field phase to struct Advice, variant Any::Advice and variant Expression::Advice.
commit@6051345
- Add a struct Challenge and can be allocated by ConstraintSystem::challenge_usable_after(phase), and it's also
  1. Used in VirtualCells::query_challenge(challenge) to get Expression::Challenge.
  2. Used in Layouter::get_challenge(challenge) to get Value<F> (Value::unknown if current phase is earlier than requested challenge).
- In create_proof, ConcreteCircuit::FloorPlanner::synthesize will be call as many time as the phases used in the circuit, and WitnessCollection will ignore the advice column assignment if phase doesn't match.

Also it adds a new example shuffle.rs to show how to use multi-phase to roll our own vector shuffle argument.

Todos

Update to share extra challenges across instances in the same proof.
Benchmark time and memory again.

Concerns

Handling advice column assignment in wrong phase

(answered in #593 (comment))

Instead of returning an error, currently WitnessCollection ignores the advice column assignment if phase doesn't match (like this). The reason is to make sure the regions measured by V1 planner to be same in different phase (To me it's like in key generation (phase 0) we still need to call assign_advice).

One approach is to call and check the value closure returns an Err(Error::UnmatchedPhase), but not sure if this is desired, because it breaks the principle that all value closures passed to any Region::assign_* method will be called at most once.

`PinnedConstraintSystem` is updated, so as `PinnedVerificationKey`

(resolved by custom fmt::Debug impl)

Since a new field is added to Advice, and several fields num_challenges, advice_column_phase and challenge_phase are added to ConstraintSystem, the PinnedConstraintSystem is also updated to carry these information. So this PR makes VerifyingKey::hash_into to have different result, and might break some existing prover/verifier that relies on serialized pk/vk (a breaking change to halo2).

Effect to small circuit that doesn't need multi-phase

The memory usage of Column<Advice> is increased to 12 or 16 bytes, and several fields are added ConstraintSystem which also increases memory usage. This might affect small circuit's performance.

The benchmark or orchard circuit seems to be unstable on my mac (Intel Core i5), but here is the result:

As-is halo2_proof@c0db68a

proving/bundle/1        time:   [4.3719 s 4.4765 s 4.5679 s]
proving/bundle/2        time:   [4.3741 s 4.4825 s 4.5911 s]
proving/bundle/3        time:   [5.9627 s 6.0780 s 6.1929 s]
proving/bundle/4        time:   [7.5456 s 7.9638 s 8.4966 s]

verifying/bundle/1      time:   [43.363 ms 45.014 ms 46.810 ms]
verifying/bundle/2      time:   [40.755 ms 43.009 ms 46.204 ms]
verifying/bundle/3      time:   [55.697 ms 60.355 ms 65.751 ms]
verifying/bundle/4      time:   [48.969 ms 49.327 ms 49.727 ms]

With this PR halo2_proof@1e96c83

proving/bundle/1        time:   [3.9909 s 4.0380 s 4.1140 s]
                        change: [-12.039% -9.7957% -7.2186%] (p = 0.00 < 0.05)
                        Performance has improved.
proving/bundle/2        time:   [3.9906 s 4.0130 s 4.0369 s]
                        change: [-12.653% -10.474% -8.1569%] (p = 0.00 < 0.05)
                        Performance has improved.
proving/bundle/3        time:   [5.8532 s 6.1311 s 6.4735 s]
                        change: [-4.0383% +0.8745% +6.0771%] (p = 0.79 > 0.05)
                        No change in performance detected.
proving/bundle/4        time:   [7.7428 s 8.2447 s 8.7707 s]
                        change: [-5.3507% +3.5264% +12.740%] (p = 0.47 > 0.05)
                        No change in performance detected.

verifying/bundle/1      time:   [39.497 ms 39.886 ms 40.390 ms]
                        change: [-14.938% -11.391% -7.8738%] (p = 0.00 < 0.05)
                        Performance has improved.
verifying/bundle/2      time:   [44.617 ms 49.110 ms 55.597 ms]
                        change: [+1.1374% +14.185% +30.292%] (p = 0.04 < 0.05)
                        Performance has regressed.
verifying/bundle/3      time:   [44.147 ms 44.392 ms 44.656 ms]
                        change: [-32.543% -26.449% -20.237%] (p = 0.00 < 0.05)
                        Performance has improved.
verifying/bundle/4      time:   [49.890 ms 50.577 ms 51.344 ms]
                        change: [+0.8118% +2.5337% +4.2452%] (p = 0.00 < 0.05)
                        Change within noise threshold.

halo2_proofs/examples/shuffle.rs

halo2_proofs/src/dev.rs

str4d · 2022-06-03T15:17:16Z

halo2_proofs/src/plonk/prover.rs

+            if self.current_phase > challenge.phase() {
+                Some(self.challenges[challenge.index()])
+            } else {
+                None
+            }


This is brittle. self.challenges is initialized to all-zeros, so if there is a bug here or a bug at the point where self.challenges is mutated that causes a challenge to not be added, then get_challenge will return zero, which must never happen (as zero-value challenges collapse the things they are designed to separate).

We should instead use a HashMap here:

Suggested change

if self.current_phase > challenge.phase() {

Some(self.challenges[challenge.index()])

} else {

None

}

self.challenges.get(challenge.index())

and then only add challenges to self.challenges as we reach their respective phases. This safely fails-shut, where if there is a bug at the point where self.challenges is mutated that causes a challenge to not be added, get_challenge will return None.

I can see a possible performance rationale for using Vec over HashMap, but we should only do that if it actually proves to be necessary, and only after we have this working safely.

Updated in b2626b5.

I suggest also adding Expression::Postprocess to ensure we can apply any black-box computations to challenges.

It is partially implemented here: https://github.com/levs57/halo2 , though it probably should be refactored a bit, because right now the fact that Expression:Postprocess is formed only from Expression::Constant, Expression::Challenge or Expression::Postprocess is checked in runtime, while we could achieve the same using wrapper struct PublicExpression with private field Expression.

Usage example here: https://github.com/levs57/halo2-liam-eagen-msm/blob/master/src/testing_stuff/challenge_postprocessing_test.rs

halo2_proofs/src/plonk/prover.rs

halo2_proofs/tests/plonk_api.rs

str4d · 2022-06-03T16:05:25Z

halo2_proofs/src/plonk/circuit.rs

+        /// Phase of this advice column
+        phase: Phase,


This shouldn't be part of the public API (otherwise someone could mess with the queried expression to break the safety property we are trying to enforce).

We should instead make this entire enum struct opaque (which probably should have been the case for all of Expression, but it's easy to forget that expression structs and tuples have the visibility of the enum). This is probably best done in a separate PR first.

halo2_proofs/src/plonk/circuit.rs

str4d · 2022-06-03T16:07:16Z

halo2_proofs/src/plonk/circuit.rs

@@ -493,8 +571,9 @@ impl<F: Field> Expression<F> {
        constant: &impl Fn(F) -> T,
        selector_column: &impl Fn(Selector) -> T,
        fixed_column: &impl Fn(usize, usize, Rotation) -> T,
-        advice_column: &impl Fn(usize, usize, Rotation) -> T,
+        advice_column: &impl Fn(usize, usize, Rotation, Phase) -> T,


These would then take the new AdviceQuery struct or whatever it gets called.

str4d · 2022-06-03T16:15:02Z

halo2_proofs/src/plonk/circuit.rs

+    /// Allocate a new challenge in given phase
+    pub fn challenge_in(&mut self, phase: Phase) -> Challenge {


Per my earlier comment, the challenge should not be interpreted as allocated "in" phase, but instead between phase and phase + 1. I don't have any great succinct naming suggestions right now, so maybe we should just be verbose:

Suggested change

/// Allocate a new challenge in given phase

pub fn challenge_in(&mut self, phase: Phase) -> Challenge {

/// Requests a challenge that is usable after the given phase.

pub fn challenge_usable_after(&mut self, phase: Phase) -> Challenge {

This is also where I think using type markers instead of an enum would give a better public API, so the Phase enum could be entirely private:

Suggested change

/// Allocate a new challenge in given phase

pub fn challenge_in(&mut self, phase: Phase) -> Challenge {

/// Requests a challenge that is usable after the given phase.

pub fn challenge_usable_after<P: Phase>(&mut self, phase: P) -> Challenge {

and then

trait SealedPhase { fn to_enum(self) -> PhaseEnum; } pub trait Phase: SealedPhase {} enum FirstPhase {} impl SealedPhase for FirstPhase { fn to_enum(self) -> PhaseEnum { PhaseEnum::First } } impl Phase for FirstPhase {}

Challenge could still store PhaseEnum internally to stop the Phase trait bound from propagating further than this API (which it has no need to, as phases don't need to appear anywhere else).

While I was trying sealed pattern for this, I found that PhaseEnum might still need to be public because Advice and Any::Advice has field of it, and some failure test cases need to create VirtualCell to be matched, which then needs to specify advice column's phase (even it doesn't use multi-phase feature).

Perhaps we might need to create VirtualCell in a way that it doesn't need PhaseEnum?

Yeah, this should be handled by changing the way VirtualCell works. If the dev tooling is the only thing placing this requirement on the public API, it's the dev tooling we should change.

I suspect the easiest change here is to add a VirtualColumn that allows setting the phase for advice, and then have an impl From<Any> for VirtualColumn etc. We could also keep the existing From impls, so fixed and instance checks wouldn't have to change.

I see, it makes sense to me, will try to add VirtualColumn.

The other issue I have just encountered is in Expression::evaluate, if we have PhaseEnum as private enum, what should we pass to the function to evaluate advice column (we can't pass PhaseEnum because Expression::evaluate is public)? Or we should first wrap the fields of Expression::{Advice,Fixed,Instance} as struct {Advice,Fixed,Instance}Query like you mentioned in #593 (comment), then we can hide the phase information from user.

Yes, I think introducing *Query structs is the way to go here; then future changes like this will be simpler, the evaluate interface will be simpler, and we can control what is exposed.

I see, I tried to introduce *Query structures in #616, and next I will try to rebase on it to continue this pattern.

halo2_proofs/src/dev.rs

str4d · 2022-06-03T22:58:03Z

Handling advice column assignment in wrong phase

Instead of returning an error, currently WitnessCollection ignores the advice column assignment if phase doesn't match (like this). The reason is to make sure the regions measured by V1 planner to be same in different phase (To me it's like in key generation (phase 0) we still need to call assign_advice).

One approach is to call and check the value closure returns an Err(Error::UnmatchedPhase), but not sure if this is desired, because it breaks the principle that all value closures passed to any Region::assign_* method will be called at most once.

Ignoring off-phase assignments is the correct behaviour. There is no way to construct a column with an arbitrary phase other than during Circuit::configure, meaning that by the time we reach Circuit::synthesize the backend knows precisely what columns and phases it can expect. Combined with the way challenges are exposed incrementally, we have exactly the desired safety.

What we could improve however is error handling. If a user accidentally calls Region::assign_advice with the wrong (column, value) pair, the error they get is likely not particularly helpful (namely Error::Synthesis), because during proving we currently have no way to propagate phase through values. This could change if we had an opaque Value type (see #509 (comment)), but we could also try to add a MockProver pass that figures out more debugging info.

str4d · 2022-06-09T00:13:03Z

This could change if we had an opaque Value type

We now have this in #598.

halo2_proofs/src/dev/gates.rs

halo2_proofs/src/plonk/circuit.rs

str4d · 2022-06-27T23:32:15Z

halo2_proofs/CHANGELOG.md

@@ -10,6 +10,18 @@ and this project adheres to Rust's notion of
 - `halo2_proofs::circuit::Value`, a more usable and type-safe replacement for
  `Option<V>` in circuit synthesis.
 - `impl Mul<F: Field> for &Assigned<F>`
+- `halo2_proofs::plonk`:


Now that we've released halo2_proofs 0.2.0, this PR needs to be rebased (because this section of the changelog is now part of 0.2.0. Additionally, the commits should be squashed together so that the hard-coded proof binary is unaltered by the branch (rather than having multiple changes across several commits that together are a no-op but cost commit size).

Make sense, I have rebased and squashed all commits into 6 ones with suggestions and fixes above (and I resolved some minor comments, only leave those I'm not sure how to resolve for further discussion), hope it look cleaner now.

halo2_proofs/src/circuit.rs

halo2_proofs/src/circuit/floor_planner/single_pass.rs

halo2_proofs/src/circuit/floor_planner/v1.rs

halo2_proofs/src/plonk/keygen.rs

halo2_proofs/src/dev/graph/layout.rs

halo2_proofs/src/dev/graph.rs

halo2_proofs/src/plonk/prover.rs

str4d · 2022-06-28T00:03:43Z

halo2_gadgets/src/ecc/chip/mul_fixed/short.rs

@@ -580,7 +580,7 @@ pub mod tests {
                                    offset: 1,
                                },
                                cell_values: vec![(
-                                    ((Any::Advice, 5).into(), 0).into(),
+                                    ((Any::Advice { phase: First }, 5).into(), 0).into(),


These would become something like:

Suggested change

((Any::Advice { phase: First }, 5).into(), 0).into(),

((VirtualColumn::Advice(First), 5).into(), 0).into(),

halo2_proofs/src/dev.rs

halo2_proofs/src/dev/cost.rs

halo2_proofs/src/plonk/circuit.rs

str4d · 2022-06-28T00:23:16Z

halo2_proofs/src/plonk/circuit.rs

@@ -1357,6 +1573,38 @@ impl<F: Field> ConstraintSystem<F> {
        tmp
    }

+    /// Requests a challenge that is usable after the given phase.
+    pub fn challenge_usable_after(&mut self, phase: Phase) -> Challenge {
+        self.assert_no_phase_skipped(phase, "Challenge");


If we're requiring that phases are assigned in order, then we should be checking here that phase exists, not previous_phase (since this challenge only makes sense if there are columns in phase).

Ah I didn't think about that. The assertion helper to check if specified phase exists is refactored in 6051345.

…e column in different phase

daira · 2022-07-02T05:30:02Z

halo2_proofs/examples/shuffle.rs

+        let [theta, gamma] = [(); 2].map(|_| meta.challenge_usable_after(First));
+        // Any phase skipped will cause a panic
+        assert!(catch_unwind_silent(|| meta.challenge_usable_after(Second)).is_err());
+        assert!(catch_unwind_silent(|| meta.advice_column_in(Third)).is_err());


This would be an appropriate thing to check for in a test, but examples should just show the normal usage of the API, not demonstrate error conditions (especially error conditions we've chosen to expose as panics).

han0110 mentioned this pull request Jun 3, 2022

Introduce Expression::Challenge as extra randomness for custom gates privacy-scaling-explorations/halo2#62

Closed

str4d reviewed Jun 3, 2022

View reviewed changes

CPerezz mentioned this pull request Jun 13, 2022

Introduce public inputs approach and PublicInputs circuit privacy-scaling-explorations/zkevm-specs#219

Merged

han0110 force-pushed the feature/multi-phase branch from ecd7081 to 31daa6b Compare June 14, 2022 15:01

CPerezz mentioned this pull request Jun 22, 2022

[RELEASE-Proposal] PRs to merge before we cut a new release for halo2 privacy-scaling-explorations/halo2#82

Closed

5 tasks

daira reviewed Jun 27, 2022

View reviewed changes

halo2_proofs/src/dev/gates.rs Outdated Show resolved Hide resolved

daira reviewed Jun 27, 2022

View reviewed changes

halo2_proofs/src/dev/gates.rs Outdated Show resolved Hide resolved

str4d reviewed Jun 27, 2022

View reviewed changes

halo2_proofs/src/plonk/circuit.rs Outdated Show resolved Hide resolved

str4d reviewed Jun 27, 2022

View reviewed changes

halo2_proofs/src/circuit.rs Outdated Show resolved Hide resolved

str4d reviewed Jun 27, 2022

View reviewed changes

halo2_proofs/src/circuit.rs Outdated Show resolved Hide resolved

str4d reviewed Jun 27, 2022

View reviewed changes

halo2_proofs/src/circuit/floor_planner/single_pass.rs Outdated Show resolved Hide resolved

str4d reviewed Jun 27, 2022

View reviewed changes

halo2_proofs/src/circuit/floor_planner/v1.rs Outdated Show resolved Hide resolved

str4d reviewed Jun 27, 2022

View reviewed changes

halo2_proofs/src/plonk/keygen.rs Outdated Show resolved Hide resolved

str4d reviewed Jun 27, 2022

View reviewed changes

halo2_proofs/src/dev/graph/layout.rs Outdated Show resolved Hide resolved