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

Add helper traits for constructing Value::known constants #699

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add helper traits for constructing Value::known constants #699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions halo2_gadgets/src/ecc/chip/mul.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use std::{

use ff::{Field, PrimeField};
use halo2_proofs::{
circuit::{AssignedCell, Layouter, Region, Value},
circuit::{AssignedCell, FieldValue, Layouter, Region, Value},
plonk::{Advice, Assigned, Column, ConstraintSystem, Constraints, Error, Selector},
poly::Rotation,
};
Expand Down Expand Up @@ -358,15 +358,15 @@ impl Config {
if !lsb {
base.x.value().cloned()
} else {
Value::known(Assigned::Zero)
FieldValue::ZERO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In cases like this, the helper constant is a clear win.

}
});

let y = lsb.and_then(|lsb| {
if !lsb {
-base.y.value()
} else {
Value::known(Assigned::Zero)
FieldValue::ZERO
}
});

Expand Down
16 changes: 8 additions & 8 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 @@ -248,7 +248,7 @@ pub mod tests {
use group::{ff::PrimeField, Curve};
use halo2_proofs::{
arithmetic::CurveAffine,
circuit::{AssignedCell, Chip, Layouter, Value},
circuit::{AssignedCell, Chip, FieldValue, Layouter, Value},
plonk::{Any, Error},
};
use pasta_curves::pallas;
Expand Down Expand Up @@ -519,39 +519,39 @@ pub mod tests {
// 2^64
MyCircuit {
magnitude: Value::known(pallas::Base::from_u128(1 << 64)),
sign: Value::known(pallas::Base::one()),
sign: FieldValue::ONE,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similarly here (though the length difference could be smaller as pallas::Base::ONE is now also valid).

magnitude_error: Value::known(pallas::Base::from(1 << 1)),
},
// -2^64
MyCircuit {
magnitude: Value::known(pallas::Base::from_u128(1 << 64)),
sign: Value::known(-pallas::Base::one()),
sign: -Value::<pallas::Base>::ONE,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, situations like this require type annotations (to know which impl Neg to use), so you save a bit of length overall but it's still roughly as complex (if not moreso).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding the need of the explicit type requirement makes it worse TBH. Although -1 is also one of the "magic numbers" usually. But I kinda agree that should not have it's own constant..

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not (have its own constant)? IIRC, -1 was the most common constant in the Orchard circuit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I was remembering correctly: #698 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not (have its own constant)?

For -1 specifically it would help:

let a = -Value::<pallas::Base>::ONE;
let a = FieldValue::MINUS_ONE;

But the point still applies generally.

magnitude_error: Value::known(pallas::Base::from(1 << 1)),
},
// 2^66
MyCircuit {
magnitude: Value::known(pallas::Base::from_u128(1 << 66)),
sign: Value::known(pallas::Base::one()),
sign: FieldValue::ONE,
magnitude_error: Value::known(pallas::Base::from(1 << 3)),
},
// -2^66
MyCircuit {
magnitude: Value::known(pallas::Base::from_u128(1 << 66)),
sign: Value::known(-pallas::Base::one()),
sign: -Value::<pallas::Base>::ONE,
magnitude_error: Value::known(pallas::Base::from(1 << 3)),
},
// 2^254
MyCircuit {
magnitude: Value::known(pallas::Base::from_u128(1 << 127).square()),
sign: Value::known(pallas::Base::one()),
sign: FieldValue::ONE,
magnitude_error: Value::known(
pallas::Base::from_u128(1 << 95).square() * pallas::Base::from(2),
),
},
// -2^254
MyCircuit {
magnitude: Value::known(pallas::Base::from_u128(1 << 127).square()),
sign: Value::known(-pallas::Base::one()),
sign: -Value::<pallas::Base>::ONE,
magnitude_error: Value::known(
pallas::Base::from_u128(1 << 95).square() * pallas::Base::from(2),
),
Expand Down Expand Up @@ -605,7 +605,7 @@ pub mod tests {
let magnitude_u64 = rand::random::<u64>();
let circuit = MyCircuit {
magnitude: Value::known(pallas::Base::from(magnitude_u64)),
sign: Value::known(pallas::Base::zero()),
sign: FieldValue::ZERO,
magnitude_error: Value::unknown(),
};

Expand Down
4 changes: 2 additions & 2 deletions halo2_gadgets/src/poseidon/pow5.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::iter;

use group::ff::Field;
use halo2_proofs::{
circuit::{AssignedCell, Cell, Chip, Layouter, Region, Value},
circuit::{AssignedCell, Cell, Chip, FieldValue, Layouter, Region, Value},
plonk::{
Advice, Any, Column, ConstraintSystem, Constraints, Error, Expression, Fixed, Selector,
},
Expand Down Expand Up @@ -370,7 +370,7 @@ impl<
.get(i)
.map(|word| word.0.value().cloned())
// The capacity element is never altered by the input.
.unwrap_or_else(|| Value::known(F::ZERO));
.unwrap_or_else(|| FieldValue::ZERO);
region
.assign_advice(
|| format!("load output_{}", i),
Expand Down
4 changes: 2 additions & 2 deletions halo2_gadgets/src/sha256/table16/compression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use super::{
AssignedBits, BlockWord, SpreadInputs, SpreadVar, Table16Assignment, ROUNDS, STATE,
};
use halo2_proofs::{
circuit::{Layouter, Value},
circuit::{IntegerValue, Layouter, Value},
pasta::pallas,
plonk::{Advice, Column, ConstraintSystem, Error, Selector},
poly::Rotation,
Expand Down Expand Up @@ -922,7 +922,7 @@ impl CompressionConfig {
layouter: &mut impl Layouter<pallas::Base>,
state: State,
) -> Result<[BlockWord; DIGEST_SIZE], Error> {
let mut digest = [BlockWord(Value::known(0)); DIGEST_SIZE];
let mut digest = [BlockWord(IntegerValue::ZERO); DIGEST_SIZE];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here the result is actually longer, as for primitive integers the type can often be elided.

layouter.assign_region(
|| "digest",
|mut region| {
Expand Down
26 changes: 13 additions & 13 deletions halo2_gadgets/src/sinsemilla.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
//! [Sinsemilla]: https://zips.z.cash/protocol/protocol.pdf#concretesinsemillahash
use crate::{
ecc::{self, EccInstructions, FixedPoints},
utilities::{FieldValue, RangeConstrained, Var},
utilities::{FieldValue as _, RangeConstrained, Var},
};
use group::ff::{Field, PrimeField};
use halo2_proofs::{
circuit::{Layouter, Value},
circuit::{FieldValue, Layouter, Value},
plonk::Error,
};
use pasta_curves::arithmetic::CurveAffine;
Expand Down Expand Up @@ -243,17 +243,17 @@ where
layouter: impl Layouter<C::Base>,
subpieces: impl IntoIterator<Item = RangeConstrained<C::Base, Value<C::Base>>>,
) -> Result<Self, Error> {
let (field_elem, total_bits) = subpieces.into_iter().fold(
(Value::known(C::Base::ZERO), 0),
|(acc, bits), subpiece| {
assert!(bits < 64);
let subpiece_shifted = subpiece
.inner()
.value()
.map(|v| C::Base::from(1 << bits) * v);
(acc + subpiece_shifted, bits + subpiece.num_bits())
},
);
let (field_elem, total_bits) =
subpieces
.into_iter()
.fold((FieldValue::ZERO, 0), |(acc, bits), subpiece| {
assert!(bits < 64);
let subpiece_shifted = subpiece
.inner()
.value()
.map(|v| C::Base::from(1 << bits) * v);
(acc + subpiece_shifted, bits + subpiece.num_bits())
});

// Message must be composed of `K`-bit words.
assert_eq!(total_bits % K, 0);
Expand Down
7 changes: 4 additions & 3 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 @@ -6,6 +6,7 @@ use crate::{
};

use ff::Field;
use halo2_proofs::circuit::FieldValue;
use halo2_proofs::{
circuit::{AssignedCell, Chip, Region, Value},
plonk::{Assigned, Error},
Expand Down Expand Up @@ -108,13 +109,13 @@ where
|| "dummy lambda2",
config.double_and_add.lambda_2,
offset,
|| Value::known(pallas::Base::zero()),
|| Value::<pallas::Base>::ZERO,
)?;
region.assign_advice(
|| "dummy x_p",
config.double_and_add.x_p,
offset,
|| Value::known(pallas::Base::zero()),
|| Value::<pallas::Base>::ZERO,
)?;
}

Expand Down Expand Up @@ -211,7 +212,7 @@ where
|| "q_s2 = 1",
config.q_sinsemilla2,
offset + row,
|| Value::known(pallas::Base::one()),
|| Value::<pallas::Base>::ONE,
)?;
}

Expand Down
4 changes: 2 additions & 2 deletions halo2_gadgets/src/utilities/lookup_range_check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ mod tests {

use ff::{Field, PrimeFieldBits};
use halo2_proofs::{
circuit::{Layouter, SimpleFloorPlanner, Value},
circuit::{FieldValue, Layouter, SimpleFloorPlanner, Value},
dev::{FailureLocation, MockProver, VerifyFailure},
plonk::{Circuit, ConstraintSystem, Error},
};
Expand Down Expand Up @@ -545,7 +545,7 @@ mod tests {
// Edge case: zero bits
{
let circuit: MyCircuit<pallas::Base> = MyCircuit {
element: Value::known(pallas::Base::ZERO),
element: FieldValue::ZERO,
num_bits: 0,
};
let prover = MockProver::<pallas::Base>::run(11, &circuit, vec![]).unwrap();
Expand Down
3 changes: 3 additions & 0 deletions halo2_proofs/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ and this project adheres to Rust's notion of
- `metadata::Region`
- `halo2_proofs::poly::Rotation`
- `halo2_proofs::arithmetic::FftGroup`
- `halo2_proofs::circuit`:
- `FieldValue` and `IntegerValue` helper traits, to provide common constants
equivalent to using `Value::known(_)`.
- `halo2_proofs::circuit::layouter`:
- `RegionLayouter::instance_value` method added to provide access to
instance values within a region.
Expand Down
23 changes: 9 additions & 14 deletions halo2_proofs/benches/plonk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
extern crate criterion;

use group::ff::Field;
use halo2_proofs::circuit::{Cell, Layouter, SimpleFloorPlanner, Value};
use halo2_proofs::circuit::{Cell, FieldValue, Layouter, SimpleFloorPlanner, Value};
use halo2_proofs::pasta::{EqAffine, Fp};
use halo2_proofs::plonk::*;
use halo2_proofs::poly::{commitment::Params, Rotation};
Expand Down Expand Up @@ -103,10 +103,10 @@ fn criterion_benchmark(c: &mut Criterion) {
|| value.unwrap().map(|v| v.2),
)?;

region.assign_fixed(|| "a", self.config.sa, 0, || Value::known(FF::ZERO))?;
region.assign_fixed(|| "b", self.config.sb, 0, || Value::known(FF::ZERO))?;
region.assign_fixed(|| "c", self.config.sc, 0, || Value::known(FF::ONE))?;
region.assign_fixed(|| "a * b", self.config.sm, 0, || Value::known(FF::ONE))?;
region.assign_fixed(|| "a", self.config.sa, 0, || Value::<FF>::ZERO)?;
region.assign_fixed(|| "b", self.config.sb, 0, || Value::<FF>::ZERO)?;
region.assign_fixed(|| "c", self.config.sc, 0, || Value::<FF>::ONE)?;
region.assign_fixed(|| "a * b", self.config.sm, 0, || Value::<FF>::ONE)?;
Ok((lhs.cell(), rhs.cell(), out.cell()))
},
)
Expand Down Expand Up @@ -145,15 +145,10 @@ fn criterion_benchmark(c: &mut Criterion) {
|| value.unwrap().map(|v| v.2),
)?;

region.assign_fixed(|| "a", self.config.sa, 0, || Value::known(FF::ONE))?;
region.assign_fixed(|| "b", self.config.sb, 0, || Value::known(FF::ONE))?;
region.assign_fixed(|| "c", self.config.sc, 0, || Value::known(FF::ONE))?;
region.assign_fixed(
|| "a * b",
self.config.sm,
0,
|| Value::known(FF::ZERO),
)?;
region.assign_fixed(|| "a", self.config.sa, 0, || Value::<FF>::ONE)?;
region.assign_fixed(|| "b", self.config.sb, 0, || Value::<FF>::ONE)?;
region.assign_fixed(|| "c", self.config.sc, 0, || Value::<FF>::ONE)?;
region.assign_fixed(|| "a * b", self.config.sm, 0, || Value::<FF>::ZERO)?;
Ok((lhs.cell(), rhs.cell(), out.cell()))
},
)
Expand Down
18 changes: 9 additions & 9 deletions halo2_proofs/examples/circuit-layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use ff::Field;
use halo2_proofs::{
circuit::{Cell, Layouter, Region, SimpleFloorPlanner, Value},
circuit::{Cell, FieldValue, Layouter, Region, SimpleFloorPlanner, Value},
pasta::Fp,
plonk::{Advice, Assigned, Circuit, Column, ConstraintSystem, Error, Fixed, TableColumn},
poly::Rotation,
Expand Down Expand Up @@ -93,10 +93,10 @@ impl<FF: Field> StandardCs<FF> for StandardPlonk<FF> {
let out =
region.assign_advice(|| "out", self.config.c, 0, || value.unwrap().map(|v| v.2))?;

region.assign_fixed(|| "a", self.config.sa, 0, || Value::known(FF::ZERO))?;
region.assign_fixed(|| "b", self.config.sb, 0, || Value::known(FF::ZERO))?;
region.assign_fixed(|| "c", self.config.sc, 0, || Value::known(FF::ONE))?;
region.assign_fixed(|| "a * b", self.config.sm, 0, || Value::known(FF::ONE))?;
region.assign_fixed(|| "a", self.config.sa, 0, || Value::<FF>::ZERO)?;
region.assign_fixed(|| "b", self.config.sb, 0, || Value::<FF>::ZERO)?;
region.assign_fixed(|| "c", self.config.sc, 0, || Value::<FF>::ONE)?;
region.assign_fixed(|| "a * b", self.config.sm, 0, || Value::<FF>::ONE)?;
Ok((lhs.cell(), rhs.cell(), out.cell()))
}
fn raw_add<F>(&self, region: &mut Region<FF>, mut f: F) -> Result<(Cell, Cell, Cell), Error>
Expand Down Expand Up @@ -130,10 +130,10 @@ impl<FF: Field> StandardCs<FF> for StandardPlonk<FF> {
let out =
region.assign_advice(|| "out", self.config.c, 0, || value.unwrap().map(|v| v.2))?;

region.assign_fixed(|| "a", self.config.sa, 0, || Value::known(FF::ONE))?;
region.assign_fixed(|| "b", self.config.sb, 0, || Value::known(FF::ONE))?;
region.assign_fixed(|| "c", self.config.sc, 0, || Value::known(FF::ONE))?;
region.assign_fixed(|| "a * b", self.config.sm, 0, || Value::known(FF::ZERO))?;
region.assign_fixed(|| "a", self.config.sa, 0, || Value::<FF>::ONE)?;
region.assign_fixed(|| "b", self.config.sb, 0, || Value::<FF>::ONE)?;
region.assign_fixed(|| "c", self.config.sc, 0, || Value::<FF>::ONE)?;
region.assign_fixed(|| "a * b", self.config.sm, 0, || Value::<FF>::ZERO)?;
Ok((lhs.cell(), rhs.cell(), out.cell()))
}
fn copy(&self, region: &mut Region<FF>, left: Cell, right: Cell) -> Result<(), Error> {
Expand Down
2 changes: 1 addition & 1 deletion halo2_proofs/src/circuit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use ff::Field;
use crate::plonk::{Advice, Any, Assigned, Column, Error, Fixed, Instance, Selector, TableColumn};

mod value;
pub use value::Value;
pub use value::{FieldValue, IntegerValue, Value};

pub mod floor_planner;
pub use floor_planner::single_pass::SimpleFloorPlanner;
Expand Down
Loading