privacy-scaling-explorations / halo2

https://privacy-scaling-explorations.github.io/halo2/
Other
207 stars 129 forks source link

Make `VerifyFailure::emit` public #292

Closed georgwiese closed 8 months ago

georgwiese commented 8 months ago

This way, users can pretty-print the error while still catching the verification failure. As far as I can see, printing the error is otherwise only exposed via MockProver::assert_satisfied.

CPerezz commented 8 months ago

Hi!

Well, the idea is that the errors of the MockProver are there only to test that they will happen.

In other words, MockProver is designed such that the failure is asserted to be equal to a particular one that you provide OR to be printed (Debug or Display-wise) directly after a panic in the verify function.

But you should never need to call emit from the consumer perspective.

Can you share an example where you actually need to have this? The "pretty-print" will look the same as theDebugimpl used in the assertion. You can better format it using"{:#?}"`.

georgwiese commented 8 months ago

Thanks for the quick response :)

The "pretty-print" will look the same as the Debug impl used in the assertion. You can better format it using"{:#?}"`.

I don't think so, it looks different for me! Also, looking at the code, functions like render_cell_not_assigned don't seem to be called anywhere else except from emit().

My use-case is that I have a wrapper around MockProver, and usually I want to print the error and panic, as assert_satisfied() would do. But, as you mentioned, in tests I might want to assert that constraints are not satisfied. So it would be better for my wrapper to just return the result of MockProver::verify(), but then there doesn't seem to be a way to get the pretty error in the "typical" case,

georgwiese commented 8 months ago

(I'm on v0.3.0, but looking at the code, looks like this would be the same problem with the current main)

CPerezz commented 8 months ago

Hey @georgwiese does any of these examples work for what you're trying to do?

Notice that VerifyFailure is indeed pub such that the matching against it can be done. In any case, if the issue is a panic on MockProver errors (as for the issue you've linked) which seems solvable without the emit method being pub. See https://github.com/privacy-scaling-explorations/halo2/blob/1650a0b07fbbfeebab71f5650ccb62ccd20de0ff/halo2_frontend/src/dev/failure.rs#L127

georgwiese commented 8 months ago

Hmm, I don't see how. The behavior I'd like is to be able to pretty-print the error (same as when calling assert_satisfied()) but not panic, and as far as I can see there is no way to do that now.

Basically, we have a trait for each prover which returns a Result, and one implementation of this trait wraps MockProver. If we call validate(), we get the Result but no pretty error message; if we call assert_satisfied() we get the message but we're not able to catch the error.

CPerezz commented 8 months ago

I don't see how you can't get the error.

If you call https://github.com/privacy-scaling-explorations/halo2/blob/1650a0b07fbbfeebab71f5650ccb62ccd20de0ff/halo2_frontend/src/dev.rs#L773-L778 you should be able to invoke the Display impl for the vector of failures after calling .err().unwrap().

Apologies if I'm missing how this doesn't solve your issue. If this doesn't allow it, then I'm happy to include this after someone else agrees to it too!

Edit: Ohhh I see now.

You don't want the regular error but rather the one that gives you the constraint details with the parrot message etc.. So the one that writes the table.

Fair enough. Apologies for the insistence.