Skip to content
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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

CPerezz
Copy link
Contributor

@CPerezz CPerezz commented Dec 8, 2022

This PR includes:

  • Ability to annotate columns within a Region.
  • Ability to annotate lookup table columns.
  • Ability to print in prover.assert_verify() the annotations of the Columns that have been declared.
  • Ability to print the lookup column queried within a lookup expression that failed.

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

error: lookup input does not exist in table
  (L0) ∉ ("Table1")

  Lookup 'lookup' inputs:
    L0 = x1 * x0 + (1 - x1) * 0x2
    ^

    | Cell layout in region 'Faulty synthesis':
    |   | Offset |Witness example| F1 |
    |   +--------+---------------+----+
    |   |    1   |       x0      | x1 | <--{ Lookup 'lookup' inputs queried here
    |
    | Assigned cell values:
    |   x0 = 0x5
    |   x1 = 1

Assert_satisfied - ConstraintNotSatisfied

error: constraint not satisfied

  Cell layout in region 'Wrong synthesis':
    | Offset |This is Advice!|This is Advice too!|Another one!|This is a Fixed!|
    +--------+---------------+-------------------+------------+----------------+
    |    0   |       x0      |         x1        |     x2     |       x3       | <--{ Gate 'Equality check' applied here

  Constraint '':
    (S1 * (x0 - x1)) * (x2 - x3) = 0

  Assigned cell values:
    x0 = 1
    x1 = 0
    x2 = 0x5
    x3 = 0x7

Assert_satisfied - CellNotAssigned

error: cell not assigned

  Cell layout in region 'Faulty synthesis':
    | Offset |This is annotated!|This is also annotated!|
    +--------+------------------+-----------------------+
    |    0   |        x0        |                       |
    |    1   |                  |           X           | <--{ X marks the spot! 🦜

  Gate 'Equality check' (applied at offset 1) queries these cells.

Resolves: #705

@CPerezz
Copy link
Contributor Author

CPerezz commented Dec 8, 2022

[Daira: I moved the description of removed features here so that it won't go into the commit message.]

Prover.verify - ConstraintNotSatisfied (now includes annotations for VirtualCells) Not anymore

Err([ConstraintCaseDebug {
    constraint: Constraint {
        gate: Gate {
            index: 0,
            name: "Equality check",
        },
        index: 0,
        name: "",
    },
    location: InRegion {
        region: Region 1 ('Wrong synthesis'),
        offset: 0,
    },
    cell_values: [
        (
            DebugVirtualCell {
                name: "",
                column: DebugColumn {
                    column_type: Advice,
                    index: 0,
                    annotation: "This is Advice!",
                },
                rotation: 0,
            },
            "1",
        ),
        (
            DebugVirtualCell {
                name: "",
                column: DebugColumn {
                    column_type: Advice,
                    index: 1,
                    annotation: "This is Advice too!",
                },
                rotation: 0,
            },
            "0",
        ),
        (
            DebugVirtualCell {
                name: "",
                column: DebugColumn {
                    column_type: Advice,
                    index: 2,
                    annotation: "Another one!",
                },
                rotation: 0,
            },
            "0x5",
        ),
        (
            DebugVirtualCell {
                name: "",
                column: DebugColumn {
                    column_type: Fixed,
                    index: 0,
                    annotation: "This is a Fixed!",
                },
                rotation: 0,
            },
            "0x7",
        ),
    ],
}])

Note that prover.verify() output could be improved more. The issue is that requires a deeper refactor as goes throug Debug & Display instead of emit. And so, has no access to the prover instance.

In any case, I started a poll so that we can see whether is worth the refactor efforts or maybe not.
See this tweet
This doesn't apply anymore and we should go towards assert_verify usage.


Tests are failing now since if you execute $cargo test unassigned_cell you should see the new display format with Column annotations (aside from a lot of warnings)..

@CPerezz CPerezz marked this pull request as ready for review December 13, 2022 18:00
@CPerezz CPerezz changed the title [WIP] feat: Implement Column annotations for MockProver debugging feat: Implement Column annotations for MockProver debugging Dec 13, 2022
halo2_proofs/src/plonk/circuit.rs Outdated Show resolved Hide resolved
Comment on lines +41 to +48
struct DebugColumn {
/// The type of the column.
column_type: Any,
/// The index of the column.
index: usize,
/// Annotation of the column
annotation: String,
}
Copy link
Collaborator

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?

Copy link
Contributor Author

@CPerezz CPerezz Dec 16, 2022

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:

  1. 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.

  2. 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 !

halo2_proofs/src/plonk/circuit.rs Outdated Show resolved Hide resolved
halo2_proofs/src/dev.rs Outdated Show resolved Hide resolved
halo2_proofs/src/dev/failure/emitter.rs Outdated Show resolved Hide resolved
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`.
@CPerezz
Copy link
Contributor Author

CPerezz commented Jan 10, 2023

I noticed Clippy will never pass here until #719 is addressed.

Also, this should be ready for a final round of reviews @therealyingtong

@CPerezz
Copy link
Contributor Author

CPerezz commented Jan 10, 2023

The only things left unaddressed are:

To explain it quick, since we don't pass a MockProver instance to VerifyFailure::Display/Debug we need to include into VerifyFailure itself a reference to the annotations of "Region-agnostic columns` (Instance and TableColumn) columns.
Doing this changes code in a lot of places. And if desired, should be something to do in a follow-up PR IMO.

Also, would be nice to know if we should leave this under nightly feature or not. (Will be pretty tricky IMO). But we can definitely try.

@CPerezz
Copy link
Contributor Author

CPerezz commented Feb 27, 2023

The last commits update this PR to:

  • Not use & and lifetimes across the annotation handling.
  • Not support custom Display impl with annotations for VerifyFailure.
  • Put the correct order of arguments for annotate_lookup_column fn for consistency.
  • Rename name_column by annotate_column.

@CPerezz CPerezz requested review from str4d and therealyingtong and removed request for str4d and therealyingtong February 27, 2023 11:49
@CPerezz
Copy link
Contributor Author

CPerezz commented Feb 27, 2023

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.

Comment on lines 13 to +14
let pad = width - text.len();

Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Column annotations for better debugging experience
4 participants