Closed guorong009 closed 5 months ago
Attention: Patch coverage is 97.19222%
with 13 lines
in your changes missing coverage. Please review.
Project coverage is 82.68%. Comparing base (
b4d1c4c
) to head (6205dfc
). Report is 4 commits behind head on main.
Files | Patch % | Lines |
---|---|---|
halo2_frontend/src/dev.rs | 96.94% | 11 Missing :warning: |
...o2_frontend/src/plonk/circuit/constraint_system.rs | 97.22% | 2 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@ed255 @teddav
I updated the assertion check - table_expression
must include either fixed
col or selector
.
Pls re-check the updates.
thanks for the PR @guorong009 . it looks good to me, but it doesn't fix https://github.com/privacy-scaling-explorations/halo2/issues/335 , right?
because if you're using a fixed column as the lookup table, the check selector | fixed
still passes and no selector is needed.
maybe it could be a good PR to also add the warning i was talking about here: https://github.com/privacy-scaling-explorations/halo2/issues/335#issuecomment-2126815115 : if the lookup is a fixed column, and there is no selector -> display a warning saying it's better to use lookup()
instead of lookup_any()
thanks for the PR @guorong009 . it looks good to me, but it doesn't fix #335 , right? because if you're using a fixed column as the lookup table, the check
selector | fixed
still passes and no selector is needed. maybe it could be a good PR to also add the warning i was talking about here: #335 (comment) : if the lookup is a fixed column, and there is no selector -> display a warning saying it's better to uselookup()
instead oflookup_any()
How about this patch - https://github.com/privacy-scaling-explorations/halo2/pull/347/commits/c80022ae84114680cae14a5df94ff8593e693b17 ? @teddav @ed255
I think it is insufficient to just give off warning message.
For example, this example causes soundness error, if not panic in configure
stage.
In other words, if I put simple warning message here, the circuit still passes all the check & produces verification success.
It could be worse, since essentially it should not succeed to verify.
Hence, I add the panic
when the table column
of lookup_any
api has only fixed
column.
@ed255
I add the recommended checks for lookup_any
here.
I think we still have undesirable behavior you mentioned in comment.
For example, this test tries to simulate the case you mentioned(https://github.com/privacy-scaling-explorations/halo2/pull/347#discussion_r1632991036).
The issue is that it still passes, although it should be failed.
@ed255 @teddav I think I didn't bring all the trick mentioned here. The main point in this PR is that:
to_lookup
and another one for lookup_table
[(key_is_enabled, table_is_enabled)]
) to the lookup_any
constraints If you read the PR description and its changes(especially, this one), it will be clear what we are missing.
Hence, I updated the assertion checks. They are:
table_expr
are single fixed
col, the api panicks & recommends to use lookup
apiinput_expr
does not have selector/fixed
col for tagging, the api panicks with messagetable_expr
does not have selector/fixed
col for tagging, the api panicks with message(input_expr(single fixed/selector query for tagging to_lookup rows), table_expr(single fixed/selector query for tagging table rows))
does not exist, the api panicksPls re-review the new updates, with mentioned PR. (These assertions catch the error case that @ed255 mentioned here)
Description
Dynamic lookup, which uses the
lookup_any
, should have the assertion to prevent use ofAdvice/Instance
column without extraFixed
column orSelector
. This PR adds assertion tolookup_any
api.Related issues
Changes
table_expression
containingfixed
col orselector
inlookup_any
apitable_expression
should NOT be one simplefixed
col, inlookup_any
api