-
Notifications
You must be signed in to change notification settings - Fork 499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Implement Column annotations for MockProver debugging #706
base: main
Are you sure you want to change the base?
Conversation
[Daira: I moved the description of removed features here so that it won't go into the commit message.]
Tests are failing now since if you execute |
1a1147b
to
245efa6
Compare
struct DebugColumn { | ||
/// The type of the column. | ||
column_type: Any, | ||
/// The index of the column. | ||
index: usize, | ||
/// Annotation of the column | ||
annotation: String, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it would be a more invasive change, but what about adding an annotation: Option<String>
to the Column
struct, instead of creating a DebugColumn
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll not lie I've thought about this! So there are two things why I went this way:
-
This was the less invasive way on which I could also improve the
VerifyFailure
error display. Otherwise, I couldn't print the annotations within the column itself. -
The type will no longer be
Copy
. And that (for some quick observations done) leads to a "significant" amount of.clone()
in the codebase. I haven't checked anything further introducing the attribute. So some other complexities could be hidden.
I can go into that direction anyway and try to implement it if you want to check how it'll look like. But maybe it's better to do it in a separated PR so that the diff leaves it more clear.
Just LMK what you think @therealyingtong !
This adds the trait-associated function `name_column` in order to enable the possibility of the Layouter to store annotations aobut the colums. This function does nothing for all the trait implementors (V1, SimpleFloor, Assembly....) except for the `MockProver`. Which is responsible of storing a map that links within a `Region` index, the `column::Metadata` to the annotation `String`.
This adds the fn `annotate_lookup_column` for `ConstraintSystem` which allows to carry annotations for the lookup columns declared for a circuit within a CS.
This allows to annotate lookup `TableColumn`s and print it's annotation within the `assert_satisfied` fn. This has required to change the `ConstraintSystem::lookup_annotations` to have keys as `metadata::Column` rather than `usize` as otherwise it's impossible within the `emitter` scope to distinguish between regular advice columns (local to the Region) and fixed columns which come from `TableColumn`s.
This allows to ignore the annotation map of the metadata::Region so that is easier to match against `VerifyFailure` errors in tests.
It was necessary to improve the `prover.verify` output also. To do so, this required auxiliary types which are obfuscated to any other part of the lib but that are necessary in order to be able to inject the Column names inside of the `Column` section itself. This also required to re-implement manually `Debug` and `Display` for this enum. This closes zcash#705
Address some leftover TO-DOs & apply code simplifications for nested if let Some() blocks.
For the `VerifyFailure::ConstraintNotSatisfied` case we want to customly impl the `Debug` call so that we can print the annotations within the `VirtualCell`s. If we hit any other case of `VerifyFailure` we basically send it to print and be handled by (ideally Display). The issue is that the call was forwarded to `Debug` impl again which ended up in an infinite recursive call to the `Debug` method. See: privacy-scaling-explorations#109 (comment) This is solved by calling `Display` if we hit a case that is not `ConstraintNotSatisfied`.
b1c0e46
to
3cfbc09
Compare
I noticed Clippy will never pass here until #719 is addressed. Also, this should be ready for a final round of reviews @therealyingtong |
The only things left unaddressed are:
To explain it quick, since we don't pass a Also, would be nice to know if we should leave this under |
The last commits update this PR to:
|
This review re-request button got crazy 😅 Apologies. @str4d @therealyingtong this should be ready to be shipped. I can put it under a nightly feature if you consider it. |
let pad = width - text.len(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not tested against the code in this PR so this could already be fixed, but on the PSE fork padded()
is now also used in cases where text.len() > width
, which will make this subtraction panic. The easiest example of this is when the column name is only a single character, which makes width = 1
, and then something like text = "x1"
needs to be padded and written in this column.
Even if fixed in some other way, some kind of if
statement mitigating this problem that may not produce the nicest output but at least never panics no matter the inputs would be a good idea I think.
This PR includes:
Region
.prover.assert_verify()
the annotations of the Columns that have been declared.More work can be done in regards of the debugging/reporting. Specially a good first step would be to improve significantly through a refactor both the
emitter
by using a table-generator crate and also, implementing a way to obtain all the debugging data outside of the circuit so that anyone can implement a debugger (GTK, GUI ...) (This second idea was given by @ed255).Here I leave the examples of all of the outputs that this PR improves with annotations.
Assert_satisfied - Lookup_fixed
Assert_satisfied - ConstraintNotSatisfied
Assert_satisfied - CellNotAssigned
Resolves: #705