-
Notifications
You must be signed in to change notification settings - Fork 132
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
Implement native shuffle argument and api #185
Conversation
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.
LGTM! Just some doc typos needed to be addressed.
halo2_proofs/src/dev.rs
Outdated
let mut input_rows: Vec<(Vec<Value<F>>, usize)> = lookup_input_row_ids | ||
.clone() | ||
.into_iter() |
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.
It seems that lookup_input_row_ids
is not guranteed to have same length with usable_rows
above, so shuffle_rows
might have different size from input_rows
.
One approach is to pad zeros to input_rows
here assuming other rows are turned off, another is to just use usable_rows
to always check the shuffle strictly (I'd prefer this until someone complain about the performance).
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'm taking the latter
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.
The part that I have managed to review looks good to me! I've left some nitpicks about opportunities for code deduplication.
The parts that I haven't fully understood yet are the changes in halo2_proofs/src/plonk/shuffle/prover.rs
and halo2_proofs/src/plonk/evaluation.rs
.
For example in the Shuffle constraints
section of halo2_proofs/src/plonk/evaluation.rs
I don't see the gamma
being used to calculate value
, maybe shuffle_value
is input_value
is
FailureLocation::OutsideRegion { row } => *row, | ||
} as i32; | ||
|
||
let shuffle_columns = shuffle.shuffle_expressions.iter().map(|expr| { |
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.
Nitpick: The closure passed here is also the same as the one used in render_lookup, so it could be deduplicated. After a careful look it seems render_shuffle
and render_lookup
are very similar, so I think there's an opportunity to deduplicate a lot of code.
Yes, halo2/halo2_proofs/src/plonk/evaluation.rs Lines 293 to 306 in 410bfdf
|
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.
LGTM! Nice work!
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.
LGTM!
…-hk/dev-feature/gl-129-khld-gt-2-adicity Extend k-high-low decomp to k larger than 2-adicity (32) # Project Context Endoscaling includes truncation of a 2-bit-window running sum, from full width down to 160 bits. This is very wasteful, because the upper windows must still be individually constrained to be 2 bit values, meaning we have an extra $(n - 160)/2$ constraints, for $n = 255$ over Pallas/Vesta, and $n = 446$ over Pluto/Eris. The plan is to instead first use a 160-high-low decomp to truncate the scalar to 160 bits, and then do the 2-bit-window running sum on that truncation, reducing the number of window constraints to $160/2$ from $n/2.$ (Note: the $160$-high-low decomp will add $O(n/10)$ cheap constraints, so it's not free, but still a large improvement.) The corresponding Galois internal issue is [Galois#129](https://gitlab-ext.galois.com/iog-midnight/halo2/-/issues/129). # Issue Description See privacy-scaling-explorations#135, but note that we're not using a custom gate as described there, since the description in that issue was written before the Plonk gadget (privacy-scaling-explorations#178) existed.
Halo2 support shuffle API at [here](privacy-scaling-explorations/halo2#185). Now which is supported through this PR: | | Halo2 | halo2_verifier.move | | --- | --- | --- | | shuffle API | halo2_backend/src/plonk/shuffle/verifier.rs | packages/verifier/sources/shuffle.move | | “struct Argument” implementation | halo2_frontend/src/plonk/shuffle.rs | packages/verifier/sources/protocol.move | | verify function | halo2_backend/src/plonk/verifier.rs | packages/verifier/sources/halo2_verifier.move | | evaluator | halo2_backend/src/plonk/evaluation.rs | - | | Halo2 update | - | “Cargo.toml” under crates folder | | circuit information | - | crates/verifier-sdk/shape-generator/src/lib.rs::CircuitInfo/struct Lookup | | serialized circuit | - | crates/verifier-sdk/shape-generator/src/serialize.rs::SerializableCircuitInfo | | examples | - | crates/vk-gen-examples/src/examples/shuffle_api.rs |
Some dynamic lookup applications require only one-to-one mapping between expressions. Currently such applications are applied using subset argument and
ConstraintSystem::lookup
API. However It would be more efficient to use shuffle argument which looks like below:So that we can save two permutation witness columns introduced in subset argument. Even this argument can be applied using challenge API, for complex circuits it would be very hard to track cell values that are required for grand product. Implementation method in this PR is just straightforward. Many subset argument related thigs are jsut copied and named as shuffle and required fixes are applied. In the end with this PR we will have
ConstraintSystem::shuffle
API.