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: https://github.com/privacy-scaling-explorations/halo2/blob/c7e42e41f8141745ba0b90213438f240d55cb347/halo2_proofs/src/plonk/error.rs#L8-L40 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 https://github.com/privacy-scaling-explorations/halo2/pull/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!