Add logs to halo2

[ed255]

When writing circuits in halo2 there are a lot of things that the developer can do wrong that causes errors in the circuit configuration / synthesis. halo2 detects these and reports them via the Error type:

halo2/halo2_proofs/src/plonk/error.rs

Lines 8 to 40 in c7e42e4

	/// This is an error that could occur during proving or circuit synthesis.
	// TODO: these errors need to be cleaned up
	#[derive(Debug)]
	pub enum Error {
	/// This is an error that can occur during synthesis of the circuit, for
	/// example, when the witness is not present.
	Synthesis,
	/// The provided instances do not match the circuit parameters.
	InvalidInstances,
	/// The constraint system is not satisfied.
	ConstraintSystemFailure,
	/// Out of bounds index passed to a backend
	BoundsFailure,
	/// Opening error
	Opening,
	/// Transcript error
	Transcript(io::Error),
	/// `k` is too small for the given circuit.
	NotEnoughRowsAvailable {
	/// The current value of `k` being used.
	current_k: u32,
	},
	/// Instance provided exceeds number of available rows
	InstanceTooLarge,
	/// Circuit synthesis requires global constants, but circuit configuration did not
	/// call [`ConstraintSystem::enable_constant`] on fixed columns with sufficient space.
	///
	/// [`ConstraintSystem::enable_constant`]: crate::plonk::ConstraintSystem::enable_constant
	NotEnoughColumnsForConstants,
	/// The instance sets up a copy constraint involving a column that has not been
	/// included in the permutation.
	ColumnNotInPermutation(Column<Any>),
	}

Unfortunately most of the error cases don't contain contextual information, and moreover they can mean different things at the same time.

For example, Error::NotEnoughRowsAvailable can happen in these situations:

• enabling a selector
• querying an instance
• assigning an advice column
• assigning a fixed column
• declaring a copy constraint
• having a k that is smaller than the minimum rows required for the circuit

When the developer sees Error::NotEnoughRowsAvailable, they don't know which case it was. Moreover, if the error was caused by an assignment, the developer doesn't the row of such assignment, or the range of usable columns (they only know the k).

I think a clean solution would be to extend the Error enum to add more information, nevertheless this would be quite a breaking change in halo2.

We have a similar issue with Error:Synthesize which is the error that we can return from the synthesis step: there's no way to add context information. Ideally this error case should contain a String to add context information of why the synthesis cannot proceed.

In the zkevm-circuits we resolved this lack of context in Error::Synthesize by adding logs, so I propose we do the same in halo2!

This would introduce the log crate dependency. A nice thing about this crate is that the user of the library decides how these logs are used, and if there's no logger backend enabled, they have a negligible performance impact.

[jonathanpwang]

I actually have a different suggestion: can we go back to unwrap and expect to force panics? Most of these errors are related to properties of the circuit that should be fixed after construction. So during construction a panic makes sense because it is "irrecoverable". This also helps the developer experience because the default rust unwind will tell you exactly where it crashes, and also the use of copious try, catch ? statements bogs down the runtime.

[ed255]

I actually have a different suggestion: can we go back to unwrap and expect to force panics? Most of these errors are related to properties of the circuit that should be fixed after construction. So during construction a panic makes sense because it is "irrecoverable". This also helps the developer experience because the default rust unwind will tell you exactly where it crashes, and also the use of copious try, catch ? statements bogs down the runtime.

After having experimented locally a bit with replacing the error logs from #135 by panics I agree with you! Whenever we encounter a Error::NotEnoughRowsAvailable, there's a bug in the circuit code that needs to be fixed, so returning an error instead of panicking is not very useful. And as you say, panicking gives us a very nice backtrace that points exactly to the place that caused this issue.

@CPerezz @han0110 @davidnevadoc what do you think about replacing some errors by panics?

[han0110]

yea I think in most case the MockProver should be the place we fix this panic (so no panic in prod), and as long as the synthesize is deterministic (which should be), using debug_assert to panic should be more user friendly imo.

[ed255]

I've submitted an alternative PR following the "panic" approach. Note that I've used assert instead of debug_assert because I removed the code that returns errors. If I use debug_assert, then when such asserts are disabled the code with proceed without detecting the errors!