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.

Sign up for GitHub

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

Jump to bottom

OrchardZSA backward compatibility using "if"s #25

Closed
wants to merge 3 commits into from
Closed

OrchardZSA backward compatibility using "if"s #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f9a57ed
Try to support orchard vanilla circuits generated from orchard_zsa crate
dmidem Feb 19, 2024
067eca5
Make missed fixes for the previous commit and provide a draft general…
dmidem Feb 24, 2024
4cc1473
Update unit tests to support the code modifications related to backwa…
dmidem Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion halo2_gadgets/src/ecc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -812,7 +812,7 @@ pub(crate) mod tests {
meta,
advices[9],
lookup_table,
table_range_check_tag,
Some(table_range_check_tag),
);
EccChip::<TestFixedBases>::configure(meta, advices, lagrange_coeffs, range_check)
}
Expand Down
4 changes: 2 additions & 2 deletions halo2_gadgets/src/ecc/chip/mul_fixed/short.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ pub mod tests {
meta,
advices[9],
lookup_table,
table_range_check_tag,
Some(table_range_check_tag),
);
EccChip::<TestFixedBases>::configure(meta, advices, lagrange_coeffs, range_check)
}
Expand Down Expand Up @@ -853,7 +853,7 @@ pub mod tests {
meta,
advices[9],
lookup_table,
table_range_check_tag,
Some(table_range_check_tag),
);
EccChip::<TestFixedBases>::configure(meta, advices, lagrange_coeffs, range_check)
}
Expand Down
8 changes: 6 additions & 2 deletions halo2_gadgets/src/sinsemilla.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,8 +515,12 @@
Error,
> {
assert_eq!(self.M.sinsemilla_chip, message.chip);

// FIXME: it's not a breaking change because `blinding_factor` simply wraps `R.mul`
Copy link

Choose a reason for hiding this comment

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

what's the problem?

// and `hash` simply wraps `M.hash_to_point` - are those wrapper really needed?
let blind = self.blinding_factor(layouter.namespace(|| "[r] R"), r)?;
let (p, zs) = self.hash(layouter.namespace(|| "M"), message)?;

let commitment = p.add(layouter.namespace(|| "M + [r] R"), &blind)?;
Ok((commitment, zs))
}
Expand Down Expand Up @@ -654,14 +658,14 @@
table_idx,
meta.lookup_table_column(),
meta.lookup_table_column(),
table_range_check_tag,
Some(table_range_check_tag),
);

let range_check = LookupRangeCheckConfig::configure(
meta,
advices[9],
table_idx,
table_range_check_tag,
Some(table_range_check_tag),
);

let ecc_config =
Expand Down Expand Up @@ -748,8 +752,8 @@
let point = merkle_crh
.hash_to_point(
l.into_iter()
.chain(left.into_iter())

Check warning on line 755 in halo2_gadgets/src/sinsemilla.rs

View workflow job for this annotation

GitHub Actions / Clippy (beta)

explicit call to `.into_iter()` in function argument accepting `IntoIterator` 

warning: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
   --> halo2_gadgets/src/sinsemilla.rs:755:48
    |
755 | ...                   .chain(left.into_iter())
    |                              ^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `left`
    |
note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
   --> /rustc/f2043422f7b161a2fc1a00589a8c4956db963450/library/core/src/iter/traits/iterator.rs:524:12
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
    = note: `-W clippy::useless-conversion` implied by `-W clippy::all`
    = help: to override `-W clippy::all` add `#[allow(clippy::useless_conversion)]`
.chain(right.into_iter()),

Check warning on line 756 in halo2_gadgets/src/sinsemilla.rs

View workflow job for this annotation

GitHub Actions / Clippy (beta)

explicit call to `.into_iter()` in function argument accepting `IntoIterator` 

warning: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
   --> halo2_gadgets/src/sinsemilla.rs:756:48
    |
756 | ...                   .chain(right.into_iter()),
    |                              ^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `right`
    |
note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()`
   --> /rustc/f2043422f7b161a2fc1a00589a8c4956db963450/library/core/src/iter/traits/iterator.rs:524:12
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
)
.unwrap();
point.to_affine()
Expand Down
13 changes: 11 additions & 2 deletions halo2_gadgets/src/sinsemilla/chip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ where
pub(super) generator_table: GeneratorTableConfig,
/// An advice column configured to perform lookup range checks.
lookup_config: LookupRangeCheckConfig<pallas::Base, { sinsemilla::K }>,
/// FIXME: add a proper comment
is_zsa_variant: bool,
Copy link

Choose a reason for hiding this comment

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

Rename option: "is_zsa_variant" --> "is_Q_private". Previously, Q was public in the old version, i.e., y_q = fixed_y_q. Now, Q is private, and y_q is assigned to x_p.prev.

The comment can be: is_Q_private is a boolean flag used to determine whether Q is a private point. When this flag is set to true, it indicates that Q is a private point, and y_Q is assigned to x_P. When this flag is set to false, it indicates that Q is a public point, and y_Q is assigned to fixed_y_q.

_marker: PhantomData<(Hash, Commit, F)>,
}

Expand Down Expand Up @@ -153,7 +155,7 @@ where
advices: [Column<Advice>; 5],
witness_pieces: Column<Advice>,
fixed_y_q: Column<Fixed>,
lookup: (TableColumn, TableColumn, TableColumn, TableColumn),
lookup: (TableColumn, TableColumn, TableColumn, Option<TableColumn>),
range_check: LookupRangeCheckConfig<pallas::Base, { sinsemilla::K }>,
) -> <Self as Chip<pallas::Base>>::Config {
// Enable equality on all advice columns
Expand Down Expand Up @@ -181,6 +183,8 @@ where
table_range_check_tag: lookup.3,
},
lookup_config: range_check,
// FIXME: consider passing is_zsa_enabled to `configure` function explicitly
Copy link

Choose a reason for hiding this comment

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

what's the problem?

is_zsa_variant: lookup.3.is_some(),
_marker: PhantomData,
};

Expand All @@ -204,7 +208,12 @@ where
// https://p.z.cash/halo2-0.1:sinsemilla-constraints?partial
meta.create_gate("Initial y_Q", |meta| {
let q_s4 = meta.query_selector(config.q_sinsemilla4);
let y_q = meta.query_advice(config.double_and_add.x_p, Rotation::prev());

let y_q = if config.is_zsa_variant {
meta.query_advice(config.double_and_add.x_p, Rotation::prev())
} else {
meta.query_fixed(config.fixed_y_q)
};

// Y_A = (lambda_1 + lambda_2) * (x_a - x_r)
let Y_A_cur = Y_A(meta, Rotation::cur());
Expand Down
117 changes: 60 additions & 57 deletions halo2_gadgets/src/sinsemilla/chip/generator_table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub struct GeneratorTableConfig {
pub table_idx: TableColumn,
pub table_x: TableColumn,
pub table_y: TableColumn,
pub table_range_check_tag: TableColumn,
pub table_range_check_tag: Option<TableColumn>,
}

impl GeneratorTableConfig {
Expand Down Expand Up @@ -107,65 +107,68 @@ impl GeneratorTableConfig {
)?;
table.assign_cell(|| "table_x", self.table_x, index, || Value::known(*x))?;
table.assign_cell(|| "table_y", self.table_y, index, || Value::known(*y))?;
table.assign_cell(
|| "table_range_check_tag",
self.table_range_check_tag,
index,
|| Value::known(pallas::Base::zero()),
)?;
if index < (1 << 4) {
let new_index = index + (1 << K);
table.assign_cell(
|| "table_idx",
self.table_idx,
new_index,
|| Value::known(pallas::Base::from(index as u64)),
)?;
table.assign_cell(
|| "table_x",
self.table_x,
new_index,
|| Value::known(*x),
)?;
table.assign_cell(
|| "table_y",
self.table_y,
new_index,
|| Value::known(*y),
)?;
table.assign_cell(
|| "table_range_check_tag",
self.table_range_check_tag,
new_index,
|| Value::known(pallas::Base::from(4_u64)),
)?;
}
if index < (1 << 5) {
let new_index = index + (1 << 10) + (1 << 4);
table.assign_cell(
|| "table_idx",
self.table_idx,
new_index,
|| Value::known(pallas::Base::from(index as u64)),
)?;
table.assign_cell(
|| "table_x",
self.table_x,
new_index,
|| Value::known(*x),
)?;
table.assign_cell(
|| "table_y",
self.table_y,
new_index,
|| Value::known(*y),
)?;

if let Some(table_range_check_tag) = self.table_range_check_tag {
table.assign_cell(
|| "table_range_check_tag",
self.table_range_check_tag,
new_index,
|| Value::known(pallas::Base::from(5_u64)),
table_range_check_tag,
index,
|| Value::known(pallas::Base::zero()),
)?;
if index < (1 << 4) {
let new_index = index + (1 << K);
table.assign_cell(
|| "table_idx",
self.table_idx,
new_index,
|| Value::known(pallas::Base::from(index as u64)),
)?;
table.assign_cell(
|| "table_x",
self.table_x,
new_index,
|| Value::known(*x),
)?;
table.assign_cell(
|| "table_y",
self.table_y,
new_index,
|| Value::known(*y),
)?;
table.assign_cell(
|| "table_range_check_tag",
table_range_check_tag,
new_index,
|| Value::known(pallas::Base::from(4_u64)),
)?;
}
if index < (1 << 5) {
let new_index = index + (1 << 10) + (1 << 4);
table.assign_cell(
|| "table_idx",
self.table_idx,
new_index,
|| Value::known(pallas::Base::from(index as u64)),
)?;
table.assign_cell(
|| "table_x",
self.table_x,
new_index,
|| Value::known(*x),
)?;
table.assign_cell(
|| "table_y",
self.table_y,
new_index,
|| Value::known(*y),
)?;
table.assign_cell(
|| "table_range_check_tag",
table_range_check_tag,
new_index,
|| Value::known(pallas::Base::from(5_u64)),
)?;
}
}
}
Ok(())
Expand Down
64 changes: 62 additions & 2 deletions halo2_gadgets/src/sinsemilla/chip/hash_to_point.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,11 @@ where
),
Error,
> {
let (offset, x_a, y_a) = self.public_initialization(region, Q)?;
let (offset, x_a, y_a) = if self.config.is_zsa_variant {
self.public_initialization_zsa(region, Q)?
Copy link

Choose a reason for hiding this comment

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

public_initialization_zsa --> private_initialization. I think the "private_initialization" would be better since Q is now a private point.

} else {
self.public_initialization(region, Q)?
};

let (x_a, y_a, zs_sum) = self.hash_all_pieces(region, offset, message, x_a, y_a)?;

Expand Down Expand Up @@ -116,6 +120,19 @@ where

let (x_a, y_a, zs_sum) = self.hash_all_pieces(region, offset, message, x_a, y_a)?;

// FIXME: try to avoid duplication with a very similar code block in `hash_message` method
// - it's basically the same code except the following lines:
//
// hash_message_with_private_init:
// ...
// .zip(Q.point())
// .assert_if_known(|((field_elems, (x_a, y_a)), Q)| {
// ...
//
// hash_message:
// ...
// .assert_if_known(|(field_elems, (x_a, y_a))| {
// ...
#[cfg(test)]
#[allow(non_snake_case)]
// Check equivalence to result from primitives::sinsemilla::hash_to_point
Expand Down Expand Up @@ -165,14 +182,57 @@ where
))
}

#[allow(non_snake_case)]
fn public_initialization(
&self,
region: &mut Region<'_, pallas::Base>,
Q: pallas::Affine,
) -> Result<(usize, X<pallas::Base>, Y<pallas::Base>), Error> {
let config = self.config().clone();
let offset = 0;

// Get the `x`- and `y`-coordinates of the starting `Q` base.
let x_q = *Q.coordinates().unwrap().x();
let y_q = *Q.coordinates().unwrap().y();

// Constrain the initial x_a, lambda_1, lambda_2, x_p using the q_sinsemilla4
// selector.
let y_a: Y<pallas::Base> = {
// Enable `q_sinsemilla4` on the first row.
config.q_sinsemilla4.enable(region, offset)?;
region.assign_fixed(
|| "fixed y_q",
config.fixed_y_q,
offset,
|| Value::known(y_q),
)?;

Value::known(y_q.into()).into()
};

// Constrain the initial x_q to equal the x-coordinate of the domain's `Q`.
let x_a: X<pallas::Base> = {
let x_a = region.assign_advice_from_constant(
|| "fixed x_q",
config.double_and_add.x_a,
offset,
x_q.into(),
)?;

x_a.into()
};

Ok((offset, x_a, y_a))
}

#[allow(non_snake_case)]
/// Assign the coordinates of the initial public point `Q`
Copy link

Choose a reason for hiding this comment

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

public point Q --> private point Q. Not all comments are aligned with the update. Q is a private point in the updated version, whereas Q is a public point in the old version.

///
/// | offset | x_A | x_P | q_sinsemilla4 |
/// --------------------------------------
/// | 0 | | y_Q | |
/// | 1 | x_Q | | 1 |
fn public_initialization(
fn public_initialization_zsa(
&self,
region: &mut Region<'_, pallas::Base>,
Q: pallas::Affine,
Expand Down
2 changes: 1 addition & 1 deletion halo2_gadgets/src/sinsemilla/merkle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ pub mod tests {
meta.lookup_table_column(),
meta.lookup_table_column(),
meta.lookup_table_column(),
meta.lookup_table_column(),
Some(meta.lookup_table_column()),
);

let range_check =
Expand Down
2 changes: 1 addition & 1 deletion halo2_gadgets/src/utilities/cond_swap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ mod tests {
meta,
advices[9],
table_idx,
table_range_check_tag,
Some(table_range_check_tag),
);

let ecc_config = EccChip::<TestFixedBases>::configure(
Expand Down
Loading
Loading