Add base conversion lookup table #215
Conversation
github-actions
bot
added
the
crate-keccak
ChihChengLiang
force-pushed
the
use-table-column
branch
from
8983bb5
to
601f1af
Compare
|
@ChihChengLiang feel free to ask for a new review when is ready! :) Will wait until you ask. |
|
sure, I'm still on the flag and the selector. Will ask for a rereview once complete. |
ChihChengLiang
force-pushed
the
use-table-column
branch
3 times, most recently
from
999c834
to
0f74dc9
Compare
ChihChengLiang
force-pushed
the
use-table-column
branch
from
0f74dc9
to
c22d84a
Compare
ChihChengLiang
force-pushed
the
use-table-column
branch
from
c22d84a
to
269da24
Compare
There was a problem hiding this comment.
LGTM. But would like to see @therealyingtong 's opinion on the if/else containing constraints.
| if offset == 0 { | ||
| // bind first acc to first coef | ||
| region | ||
| .constrain_equal(input_acc_cell, input_coef_cell)?; | ||
| region.constrain_equal( | ||
| output_acc_cell, | ||
| output_coef_cell, | ||
| )?; | ||
| } else if offset == input_coefs.len() - 1 { | ||
| region.constrain_equal(input_acc_cell, input.0)?; | ||
| return Ok((output_acc_cell, output_acc)); | ||
| } |
There was a problem hiding this comment.
There's a length missmatch here. 2 constraints in one branch of the if and 1 in the else.
Could you confirm if this is an issue @therealyingtong ??
There was a problem hiding this comment.
This branches on the offset. Just want a difference on offset 0 and the last offset.
I think what might matter is how can we be sure the prover and the verifier can drive the same number of rows. The answer is they should have exactly bi.slice_count() rows. input_coefs.len() == output_coefs.len() == bi.slice_count(). But not sure how to express that here is better.
There was a problem hiding this comment.
Branching on the offset looks good to me, since we can deterministically compute that.
ChihChengLiang
force-pushed
the
use-table-column
branch
from
e423a07
to
47dbe4b
Compare
|
Thank you @CPerezz and @therealyingtong for the review! |
Fix #211
TODO