halo2 icon indicating copy to clipboard operation
halo2 copied to clipboard

Remove `Self::default()` usage in `Circuit::without_witnesses` for examples

Open str4d opened this issue 2 years ago • 1 comments

Imagine the following circut that has two fields: a value, and a structural parameter.

struct MyCircuit<F> {
    value: Value<F>,
    width: usize,
}

when implementing the Circuit trait, it is necessary to implement the Circuit::without_witnesses method in a way that preserves width but drops value:

impl<F> Circuit<F> for MyCircuit<F> {
    fn without_witnesses(&self) -> Self {
        Self {
            value: Value::unknown(),
            width: self.width,
        }
    }
    ...

However, several of our examples use Self::default(): https://github.com/zcash/halo2/blob/96d9bde905a20117b4350ffba0b0a6479aa63f0a/halo2_proofs/examples/simple-example.rs#L241-L254

This is perfectly fine if MyCircuit only contains values, but as soon as it contains a structural parameter, it causes that to be lost. This causes the circuit shape to change across synthesis rounds: floor planners will lay out the circuit assuming these structural parameters are all their defaults (0 in the case of width above), but then try to assign into the circuit based on the actual values (which almost certainly will cause regions to overlap each other).

We should replace Self::default() with explicit Self { value: Value::unknown() } etc. in our examples, and also provide an example of a circuit with a structural parameter that is preserved.

str4d avatar Jun 24 '22 13:06 str4d

The mock prover could calculate shape information on both (or all) passes, and error if it is not the same.

daira avatar Jun 24 '22 23:06 daira