[ECC chip] Fixed- and variable-base scalar multiplication #111

therealyingtong · 2021-06-10T17:17:12Z

Closes #38.

therealyingtong · 2021-06-10T17:31:53Z

src/circuit/gadget/ecc/chip/mul.rs

+        // Cast from base field into scalar field.
+        // Assumptions:
+        // - The witnessed scalar field element fits into the base field.
+        // - The scalar field order is larger than the base field order.


It would help a bit if we had ways to (statically) assert these kinds of things.

Originally posted by @ebfull in #107 (comment)

src/circuit/gadget/ecc/chip/witness_scalar_fixed/short.rs

therealyingtong · 2021-06-10T17:36:27Z

src/circuit/gadget/ecc/chip.rs

    fn witness_scalar_var(
        &self,
-        _layouter: &mut impl Layouter<C::Base>,
-        _value: Option<C::Base>,
+        layouter: &mut impl Layouter<C::Base>,
+        value: Option<C::Base>,


We should point out in the docs for EccInstructions::witness_scalar_var that this API only accepts scalars that fit inside the base field:

If C::Base fits inside C::Scalar (e.g. if the application curve is Pallas), this API restricts the range of scalars that can be operated on (but all scalars it accepts would be canonical).

If C::Scalar fits inside C::Base (e.g. if the application curve is Vesta), this API would technically allow non-canonical scalars.

Originally posted by @str4d in #107 (comment)

Yes, the correctness argument would need to be re-checked if it were applied to Vesta.

src/circuit/gadget/ecc/chip.rs

src/circuit/gadget/ecc/chip/mul.rs

codecov-commenter · 2021-06-10T18:04:13Z

Codecov Report

Merging #111 (425ee6e) into main (21b77d6) will increase coverage by 2.77%.
The diff coverage is 86.95%.

@@            Coverage Diff             @@
##             main     #111      +/-   ##
==========================================
+ Coverage   83.94%   86.71%   +2.77%     
==========================================
  Files          53       64      +11     
  Lines        4659     6407    +1748     
==========================================
+ Hits         3911     5556    +1645     
- Misses        748      851     +103

Impacted Files	Coverage Δ
src/spec.rs	`97.64% <ø> (ø)`
src/circuit/gadget/ecc/chip/mul/overflow.rs	`80.17% <80.17%> (ø)`
...rcuit/gadget/ecc/chip/mul_fixed/base_field_elem.rs	`81.51% <81.51%> (ø)`
src/circuit/gadget/sinsemilla.rs	`80.00% <83.33%> (-0.54%)`	⬇️
src/circuit/gadget/ecc/chip.rs	`86.66% <84.28%> (+8.72%)`	⬆️
src/circuit/gadget/ecc/chip/mul.rs	`84.95% <84.95%> (ø)`
src/circuit/gadget/ecc/chip/mul/complete.rs	`85.13% <85.13%> (ø)`
...gadget/ecc/chip/witness_scalar_fixed/full_width.rs	`85.71% <85.71%> (ø)`
src/constants/load.rs	`82.55% <85.71%> (+82.55%)`	⬆️
src/circuit/gadget/ecc/chip/mul_fixed.rs	`86.69% <86.69%> (ø)`
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21b77d6...425ee6e. Read the comment docs.

These will be updated or restored in #111.

str4d

Found some easy performance wins!

str4d · 2021-06-16T12:59:09Z

src/circuit/gadget/ecc/chip/mul_fixed.rs

+            }
+            OrchardFixedBases::Full(base) => {
+                assert_eq!(NUM_WINDOWS, constants::NUM_WINDOWS);
+                let base: OrchardFixedBase = base.into();


This .into() is doing a lot of heavy lifting during proving:

These constants are only used inside assign_fixed calls, so we should not be computing them at all during proving. I would recommend moving this entire match base into a closure, and then evaluating the closure the first time we call the || Ok(lagrange_coeffs[window].0[k]) closure below.

Why aren't we building the constants in a lazy_static?

src/circuit/gadget/ecc/chip/mul_fixed.rs

src/circuit/gadget/ecc/chip.rs

src/circuit/gadget/ecc/chip/mul/overflow.rs

Co-authored-by: Daira Hopwood <daira@jacaranda.org> Co-authored-by: Jack Grigg <jack@electriccoin.co>

…e equal.

src/circuit/gadget/ecc/chip/mul.rs

str4d

utACK ae4e54d. Test circuit LGTM other than the unnecessary double-assignment of base_field_fixed_mul.

[EDIT: Removed old chip layout as I can't seem to hide review comments. See below for latest.]

src/circuit/gadget/ecc/chip/mul_fixed/base_field_elem.rs

…mul. Co-authored-by: Jack Grigg <jack@electriccoin.co>

Previously, we were multiplying the expression by 0, which led it to always evaluate to true.

Verify that this constraint fails when the witnessed value is out of range.

daira · 2021-07-14T00:11:45Z

src/circuit/gadget/ecc/chip/mul.rs

+            le_bytes.iter().fold(Vec::new(), |mut bitstring, byte| {
+                let bits = (0..8)
+                    .map(|shift| (byte >> shift) % 2 == 1)
+                    .collect::<Vec<_>>();
+                bitstring.extend_from_slice(&bits);
+                bitstring
+            })


This should probably be a utility function. (Does not block.)

src/circuit/gadget/ecc/chip/mul.rs

daira · 2021-07-14T00:47:59Z

src/circuit/gadget/ecc/chip/mul/complete.rs

+
+    #[allow(clippy::type_complexity)]
+    #[allow(non_snake_case)]
+    #[allow(clippy::too_many_arguments)]


Opinion: the default threshold of at most 7 arguments to avoid this lint is silly. We should raise that globally (say to 9).

src/circuit/gadget/ecc/chip/mul/incomplete.rs

str4d

re-utACK b696163

src/circuit/gadget/ecc/chip/mul.rs

src/circuit/gadget/ecc/chip/mul_fixed.rs

daira · 2021-07-15T01:14:48Z

src/circuit/gadget/ecc/chip/mul_fixed/base_field_elem.rs

+        mut layouter: impl Layouter<pallas::Base>,
+        base: FixedPoint<pallas::Affine, EccChip>,
+        base_val: pallas::Affine,
+    ) -> Result<(), Error> {


Why do these tests seem to be duplicated here and in mul_fixed/full_width.rs?

They're duplicated because the mul_fixed() and mul_fixed_base_field_elem() are both usable with the same set of fixed points. Perhaps we could abstract these tests out?

Edit:

Actually, for the purposes of Orchard, mul_fixed_base_field_elem() should only ever be used with the NullifierK base. We should introduce another FixedPointBaseField enum to separate these two APIs.

I'll do this in #145 (#145 (comment)).

daira · 2021-07-15T01:16:42Z

src/circuit/gadget/ecc/chip/mul_fixed/short.rs

+
+        // There is a single canonical sequence of window values for which a doubling occurs on the last step:
+        // 1333333333333333333334 in octal.
+        // [0xB6DB_6DB6_DB6D_B6DC] B


Test this value with both signs.

Since these tests are refactored in #145, I have noted this comment there and will address it in that PR: https://github.com/zcash/orchard/pull/145/files#r670057072

src/circuit/gadget/utilities.rs

src/constants.rs

daira

utACK with minor comments.

Co-authored-by: Daira Hopwood <daira@jacaranda.org>

str4d

re-utACK 425ee6e. Test chip LGTM.

str4d · 2021-07-15T10:12:20Z

src/circuit/gadget/ecc/chip/mul_fixed.rs

    // into usize for convenient indexing into `u`-values
    fn windows_usize(&self) -> Vec<Option<usize>> {
        self.windows_field()
            .iter()
            .map(|window| {
                if let Some(window) = window {
-                    let window = window.to_bytes()[0] as usize;
+                    let window = window.get_lower_32() as usize;


@daira left a comment inside the get_lower_32() impls:

// TODO: don't reduce, just hash the Montgomery form. (Requires rebuilding perfect hash table.)

We'll need to take care over there to ensure we don't alter this API, now that we are relying on it outside the perfect hash table.

Comments resolved.

therealyingtong mentioned this pull request Jun 10, 2021

[ECC chip] Incomplete and complete addition #107

Merged

therealyingtong commented Jun 10, 2021

View reviewed changes

src/circuit/gadget/ecc/chip.rs Outdated Show resolved Hide resolved

src/circuit/gadget/ecc/chip/mul.rs Outdated Show resolved Hide resolved

therealyingtong mentioned this pull request Jun 10, 2021

Sinsemilla chip with HashDomain #67

Merged

therealyingtong force-pushed the ecc-mul branch from c98b152 to cf3934a Compare June 11, 2021 03:33

therealyingtong added a commit that referenced this pull request Jun 11, 2021

ecc::chip.rs: Stub out scalar-mul-related structs and types.

136badd

These will be updated or restored in #111.

therealyingtong marked this pull request as draft June 11, 2021 22:42

therealyingtong changed the title ~~[ECC chip] Fixed- and variable-base scalar multiplication~~ [WIP] [ECC chip] Fixed- and variable-base scalar multiplication Jun 11, 2021

therealyingtong added a commit that referenced this pull request Jun 12, 2021

ecc::chip.rs: Stub out scalar-mul-related structs and types.

72ad194

These will be updated or restored in #111.

therealyingtong force-pushed the ecc-chip branch from ac2950b to 4eb2eac Compare June 12, 2021 04:42

therealyingtong added a commit that referenced this pull request Jun 12, 2021

ecc::chip.rs: Stub out scalar-mul-related structs and types.

aec7a7f

These will be updated or restored in #111.

therealyingtong force-pushed the ecc-chip branch 2 times, most recently from 5b74277 to 7341996 Compare June 13, 2021 13:28

str4d mentioned this pull request Jun 13, 2021

Implement Sinsemilla gadget #96

Closed

therealyingtong force-pushed the ecc-mul branch from cf3934a to bde4bd3 Compare June 14, 2021 11:37

therealyingtong changed the title ~~[WIP] [ECC chip] Fixed- and variable-base scalar multiplication~~ [ECC chip] Fixed- and variable-base scalar multiplication Jun 14, 2021

therealyingtong changed the base branch from ecc-chip to base-ecc-mul June 14, 2021 11:37

therealyingtong force-pushed the ecc-mul branch from bde4bd3 to 732c2b0 Compare June 14, 2021 11:39

therealyingtong marked this pull request as ready for review June 14, 2021 11:39

str4d added this to the Core Sprint 2021-22 milestone Jun 14, 2021

therealyingtong force-pushed the ecc-mul branch from 732c2b0 to 80e9dc5 Compare June 14, 2021 17:00

therealyingtong changed the base branch from base-ecc-mul to main June 14, 2021 17:00

therealyingtong mentioned this pull request Jun 15, 2021

Sinsemilla chip with Commit Domain #118

Merged

therealyingtong force-pushed the ecc-mul branch from 0eed46f to ca7bf2a Compare June 15, 2021 07:56

str4d reviewed Jun 16, 2021

View reviewed changes

daira reviewed Jun 16, 2021

View reviewed changes

src/circuit/gadget/ecc/chip.rs Outdated Show resolved Hide resolved

daira reviewed Jun 16, 2021

View reviewed changes

src/circuit/gadget/ecc/chip/mul/overflow.rs Outdated Show resolved Hide resolved

daira mentioned this pull request Jun 17, 2021

Simplify the canonicity check for variable-base scalar multiplication #121

Merged

Minor refactors, cleanups, clippy fixes, docfixes.

22ec16f

Co-authored-by: Daira Hopwood <daira@jacaranda.org> Co-authored-by: Jack Grigg <jack@electriccoin.co>

therealyingtong force-pushed the ecc-mul branch from 6954329 to 22ec16f Compare July 8, 2021 05:32

therealyingtong added 3 commits July 8, 2021 15:06

mul_fixed::*::tests: Witness expected point and constrain result to b…

e2ea443

…e equal.

mul::tests: Witness expected point and constrain result to be equal.

5c38f53

gadget::utilities: Add test cases for bitrange_subset() helper.

ae4e54d

str4d reviewed Jul 8, 2021

View reviewed changes

src/circuit/gadget/ecc/chip/mul.rs Outdated Show resolved Hide resolved

str4d approved these changes Jul 9, 2021

View reviewed changes

src/circuit/gadget/ecc/chip/mul_fixed/base_field_elem.rs Outdated Show resolved Hide resolved

src/circuit/gadget/ecc/chip/mul_fixed/base_field_elem.rs Outdated Show resolved Hide resolved

src/circuit/gadget/ecc/chip/mul_fixed/base_field_elem.rs Outdated Show resolved Hide resolved

therealyingtong and others added 3 commits July 9, 2021 10:23

mul_fixed::base_field_elem: Remove double-enable of base_field_fixed_…

8a9f821

…mul. Co-authored-by: Jack Grigg <jack@electriccoin.co>

utilities::range_check: Correct range_check expression

6c41c72

Previously, we were multiplying the expression by 0, which led it to always evaluate to true.

utilities::tests::test_range_check(): Test range_check() helper.

0ade539

Verify that this constraint fails when the witnessed value is out of range.

therealyingtong mentioned this pull request Jul 10, 2021

Refactor mul_fixed_short API to copy in (magnitude, sign) #145

Merged

daira reviewed Jul 14, 2021

View reviewed changes

src/circuit/gadget/ecc/chip/mul.rs Show resolved Hide resolved

daira reviewed Jul 14, 2021

View reviewed changes

src/circuit/gadget/ecc/chip/mul/incomplete.rs Show resolved Hide resolved

mul.rs: Explain ordering of mul::incomplete advice columns.

b696163

str4d approved these changes Jul 14, 2021

View reviewed changes

src/circuit/gadget/ecc/chip/mul.rs Show resolved Hide resolved

daira reviewed Jul 15, 2021

View reviewed changes

src/circuit/gadget/ecc/chip/mul_fixed.rs Outdated Show resolved Hide resolved

daira reviewed Jul 15, 2021

View reviewed changes

src/circuit/gadget/ecc/chip/mul_fixed.rs Outdated Show resolved Hide resolved

daira reviewed Jul 15, 2021

View reviewed changes

src/circuit/gadget/ecc/chip/mul_fixed.rs Outdated Show resolved Hide resolved

daira reviewed Jul 15, 2021

View reviewed changes

src/circuit/gadget/utilities.rs Outdated Show resolved Hide resolved

daira reviewed Jul 15, 2021

View reviewed changes

src/constants.rs Show resolved Hide resolved

daira approved these changes Jul 15, 2021

View reviewed changes

Docfixes and minor refactors.

425ee6e

Co-authored-by: Daira Hopwood <daira@jacaranda.org>

str4d approved these changes Jul 15, 2021

View reviewed changes

str4d merged commit cc3e1ad into main Jul 15, 2021

str4d deleted the ecc-mul branch July 15, 2021 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ECC chip] Fixed- and variable-base scalar multiplication #111

[ECC chip] Fixed- and variable-base scalar multiplication #111

therealyingtong commented Jun 10, 2021 •

edited by str4d

Loading

therealyingtong Jun 10, 2021

therealyingtong Jun 10, 2021

daira Jun 17, 2021

codecov-commenter commented Jun 10, 2021 •

edited

Loading

str4d left a comment

str4d Jun 16, 2021

daira Jul 15, 2021

str4d left a comment •

edited

Loading

daira Jul 14, 2021

daira Jul 14, 2021 •

edited

Loading

str4d left a comment

daira Jul 15, 2021

therealyingtong Jul 15, 2021 •

edited

Loading

daira Jul 15, 2021

therealyingtong Jul 15, 2021

daira left a comment

str4d left a comment

str4d Jul 15, 2021

[ECC chip] Fixed- and variable-base scalar multiplication #111

[ECC chip] Fixed- and variable-base scalar multiplication #111

Conversation

therealyingtong commented Jun 10, 2021 • edited by str4d Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 10, 2021 • edited Loading

Codecov Report

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

therealyingtong Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

therealyingtong commented Jun 10, 2021 •

edited by str4d

Loading

codecov-commenter commented Jun 10, 2021 •

edited

Loading

str4d left a comment •

edited

Loading

daira Jul 14, 2021 •

edited

Loading

therealyingtong Jul 15, 2021 •

edited

Loading