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

Endoscaling instructions and chip. #529

Open
wants to merge 10 commits into
base: endoscale-prep
Choose a base branch
from

Conversation

therealyingtong
Copy link
Collaborator

@therealyingtong therealyingtong commented Mar 22, 2022

Closes #583. Closes #248. Based on https://github.com/zcash/halo2/tree/endoscale-prep, which includes the PRs:

@therealyingtong therealyingtong changed the base branch from refactor-decompose-gadget to endoscale-prep March 29, 2022 15:26
Copy link
Contributor

@ebfull ebfull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass (didn't cover the chip in detail)

halo2/src/recursion/endoscale.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale/primitive.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale/primitive.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale/primitive.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale/chip.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale/chip.rs Outdated Show resolved Hide resolved
halo2/src/lib.rs Outdated
@@ -5,3 +5,5 @@
#![deny(missing_debug_implementations)]
#![deny(missing_docs)]
#![deny(unsafe_code)]

pub mod recursion;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't need this module any more (since halo2 is now entirely the recursive crate, and the non-recursive parts were moved to halo2_proofs).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that we re-exported all the halo2_proofs stuff from halo2 (at least, that was the original plan)? In which case a recursion module is still needed to separate the recursion-specific APIs and let users switch a halo2_proofs import to a halo2 one easily.

halo2/src/recursion.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale.rs Outdated Show resolved Hide resolved
@@ -271,7 +271,7 @@ impl<F: FieldExt + PrimeFieldBits, const K: usize> LookupRangeCheckConfig<F, K>
pub fn copy_short_check(
&self,
mut layouter: impl Layouter<F>,
element: AssignedCell<F, F>,
element: &AssignedCell<F, F>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs documentation in the changelog.

halo2/src/recursion/endoscale/primitive.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale/primitive.rs Outdated Show resolved Hide resolved
halo2/src/recursion/endoscale/primitive.rs Outdated Show resolved Hide resolved
fn endoscale_fixed_base<L: Layouter<C::Base>, const NUM_BITS: usize, const NUM_WINDOWS: usize>(
&self,
layouter: L,
base: C,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, there is no way to constrain in the circuit that multiple invocations of endoscale_fixed_base use the same fixed base (it can happen incidentally, but only if the caller passes the same value through to be constant-assigned; a mistake here couldn't be caught in an automated way). Is this intentional (i.e. are the bases supposed to be unconnected)? If not, then a separate associated type, and a corresponding "assign this type from a constant" would be good (and probably what we actually want is just #334).

Comment on lines 188 to 212
let y_check = {
let endo_y = double_and_add.y_p(meta, Rotation::cur());
let p_y = meta.query_advice(base.1, Rotation::cur());
// If the first bit is set, check that endo_y = P_y
let b0_set = b_0.clone() * (endo_y.clone() - p_y.clone());

// If the first bit is not set, check that endo_y = -P_y
let not_b0 = Expression::Constant(C::Base::one()) - b_0;
let b0_not_set = not_b0 * (endo_y + p_y);

b0_set + b0_not_set
};
let x_check = {
let endo_x = meta.query_advice(double_and_add.x_p, Rotation::cur());
let p_x = meta.query_advice(base.0, Rotation::cur());
// If the second bit is set, check that endo_x = ζ * P_x
let zeta = Expression::Constant(C::Base::ZETA);
let b1_set = b_1.clone() * (endo_x.clone() - zeta * p_x.clone());

// If the second bit is not set, check that endo_x = P_x
let not_b1 = Expression::Constant(C::Base::one()) - b_1;
let b1_not_set = not_b1 * (endo_x - p_x);

b1_set + b1_not_set
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ternary gate.

therealyingtong and others added 2 commits January 24, 2023 13:46
[book] Add description of endoscaling and public input handling.

Co-authored-by: Ying Tong Lai <yingtong@electriccoin.co>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave this a quick review with an eye to differentiating between what can go straight into stable, and what should be behind a nightly feature flag.

@@ -22,6 +22,7 @@
#![deny(unsafe_code)]

pub mod ecc;
pub mod endoscale;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub mod endoscale;
#[cfg(feature = "unstable-endoscale-gadget")]
pub mod endoscale;

Comment on lines 43 to +45
- [SHA-256](design/gadgets/sha256.md)
- [16-bit table chip](design/gadgets/sha256/table16.md)
- [Endoscaling](design/gadgets/endoscaling.md)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- [SHA-256](design/gadgets/sha256.md)
- [16-bit table chip](design/gadgets/sha256/table16.md)
- [Endoscaling](design/gadgets/endoscaling.md)
- Unstable
- [SHA-256](design/gadgets/sha256.md)
- [16-bit table chip](design/gadgets/sha256/table16.md)
- [Endoscaling](design/gadgets/endoscaling.md)

I think this would make them sub-sections of an "Unstable" section in the summary, while keeping their URLs stable.

@@ -0,0 +1,368 @@
# Endoscaling
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Endoscaling
# Endoscaling
> Status: Nightly
> Feature flag: `unstable-endoscale-gadget`

/// # Panics
///
/// Panics if the bitstring is longer than `F::NUM_BITS`.
pub fn le_bits_to_field_elem<F: PrimeFieldBits>(bits: &[bool]) -> F {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be pub? How is it used outside halo2_gadgets?

@@ -32,7 +32,7 @@ use halo2_proofs::{
use std::marker::PhantomData;

/// The running sum $[z_0, ..., z_W]$. If created in strict mode, $z_W = 0$.
#[derive(Debug)]
#[derive(Clone, Debug)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically should be listed as an addition in the changelog.

@@ -136,6 +136,16 @@ impl<F: Field> AssignedCell<Assigned<F>, F> {
}
}

impl<F: Field> From<AssignedCell<F, F>> for AssignedCell<Assigned<F>, F> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document as an addition in the changelog.

@@ -160,6 +160,11 @@ impl<C: CurveAffine> Params<C> {
self.g.clone()
}

/// The `g_lagrange` bases
pub fn g_lagrange(&self) -> Vec<C> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document as an addition in the changelog. It would also be useful to document how or why this is useful. If it is only useful for endoscaling:

Suggested change
pub fn g_lagrange(&self) -> Vec<C> {
#[cfg(feature = "unstable-endoscale-gadget")]
pub fn g_lagrange(&self) -> Vec<C> {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-nightly Issue or PR about a nightly feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants