Breaking additions to the `Field` trait #93

str4d · 2022-10-28T11:29:54Z

These new trait methods are moved from the FieldExt trait in pasta_curves, where we had placed them while figuring out their APIs.

Closes #33.

Ported from zcash/pasta_curves@db83057.

src/helpers.rs

daira · 2022-10-31T14:56:26Z

src/helpers.rs

+        for j in 2..max_v {
+            let tmp_is_one = tmp.ct_eq(&F::one());
+            let squared = F::conditional_select(&tmp, &z, tmp_is_one).square();
+            tmp = F::conditional_select(&squared, &tmp, tmp_is_one);


Are you sure this is right? If tmp == 1 then we set squared = tmp (= 1) and then tmp = squared (i.e. no effect on tmp).
Otherwise we set squared = z² and tmp = tmp (i.e. no effect on tmp).
In either case, this line is redundant, if it is correct.

Suggested change

tmp = F::conditional_select(&squared, &tmp, tmp_is_one);

You're reading the conditional select method backwards. Per the API docs, it is conditional_select(false_case, true_case, condition). Therefore, what this does is:

If tmp != 1, squared <-- tmp^2 and then tmp <-- squared. Thus squared = tmp = tmp^2.

If tmp == 1, squared <-- z^2 and then tmp <-- tmp. Thus squared = z^2 and tmp = 1.

Oh! Google found docs for a really old version of subtle in which true_case came before false_case. Ok I'll need to re-review with that in mind.

daira · 2022-10-31T14:59:48Z

src/helpers.rs

+    let mut z = F::root_of_unity();
+
+    for max_v in (1..=F::S).rev() {
+        let mut k = 1;


This doesn't follow algorithm 5 in the paper, because step 8 can result in k = 0 and this cannot.

That is a typo in Algorithm 5. $k = 0$ can never occur, because by definition we set $k$ to the least integer such that $b^{2^k} = 1$, and if we could produce $k = 0$ then that means $b^1 = 1$ and thus $b = 1$, which is filtered out by the loop condition on step 7.

daira · 2022-10-31T15:03:58Z

src/helpers.rs

+        let result = x * z;
+        x = F::conditional_select(&result, &x, b.ct_eq(&F::one()));
+        z = z.square();


I cannot follow how this corresponds to the paper.

This corresponds to step 9. This specific line corresponds to $z \leftarrow \omega^2$. We elide $\omega$ in this implementation because we never compute it directly; we instead compute its effective value incrementally in constant time via the z update on line 43.

src/helpers.rs

daira

[I think I clicked the wrong review outcome here; should have been Request changes]

daira

Responses needed. In particular I think sqrt_ratio_generic may be incorrect, and I wasn't able to verify sqrt_tonelli_shanks against the implementation in the paper.

src/helpers.rs

daira

utACK modulo comments and variable renaming.

ebfull

possible improvement to wording of changelog

CHANGELOG.md

We also provide helper methods for implementing the square root trait methods. Ported from zcash/pasta_curves@db83057.

str4d · 2022-11-02T18:29:57Z

Force-pushed to address review comments from @ebfull and @daira.

This makes the potential for a cycle clear (if the `Field` implementor uses `sqrt` to implement `sqrt_ratio`).

ebfull

ACK -- didn't look at the Tonelli-Shanks algorithm in detail; I'm just assuming it's been lifted from pasta_curves.

Add ff::Field::pow

b97576c

Ported from zcash/pasta_curves@db83057.

str4d requested a review from ebfull October 28, 2022 11:29

str4d force-pushed the field-trait-changes branch from b36fbf5 to 0dd4723 Compare October 28, 2022 11:31

str4d mentioned this pull request Oct 28, 2022

Add codegen option instead of derive #90

Open

Base automatically changed from release-0.12.1 to main October 28, 2022 20:46

str4d mentioned this pull request Oct 31, 2022

ff_derive fails with no sqrt for p = 5 (mod 8) and p = 9 (mod 16) #84

Open

daira mentioned this pull request Oct 31, 2022

Other possible sqrt-related operations #33

Closed

daira reviewed Oct 31, 2022

View reviewed changes

src/helpers.rs Outdated Show resolved Hide resolved

daira reviewed Oct 31, 2022

View reviewed changes

src/helpers.rs Show resolved Hide resolved

daira suggested changes Oct 31, 2022

View reviewed changes

str4d commented Oct 31, 2022

View reviewed changes

src/helpers.rs Show resolved Hide resolved

str4d commented Oct 31, 2022

View reviewed changes

src/helpers.rs Outdated Show resolved Hide resolved

daira reviewed Oct 31, 2022

View reviewed changes

src/helpers.rs Outdated Show resolved Hide resolved

daira reviewed Oct 31, 2022

View reviewed changes

src/helpers.rs Show resolved Hide resolved

daira reviewed Oct 31, 2022

View reviewed changes

src/helpers.rs Outdated Show resolved Hide resolved

daira approved these changes Oct 31, 2022

View reviewed changes

ebfull requested changes Nov 2, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Add ff::Field::{sqrt_ratio, sqrt_alt}

58741b7

We also provide helper methods for implementing the square root trait methods. Ported from zcash/pasta_curves@db83057.

str4d force-pushed the field-trait-changes branch from 0dd4723 to 58741b7 Compare November 2, 2022 18:28

Document that Field::{sqrt, sqrt_alt} provided impls use sqrt_ratio

cc08c5e

This makes the potential for a cycle clear (if the `Field` implementor uses `sqrt` to implement `sqrt_ratio`).

ebfull approved these changes Nov 2, 2022

View reviewed changes

ebfull merged commit 9a844a7 into main Nov 2, 2022

str4d deleted the field-trait-changes branch November 2, 2022 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breaking additions to the `Field` trait #93

Breaking additions to the `Field` trait #93

str4d commented Oct 28, 2022 •

edited

Loading

daira Oct 31, 2022 •

edited

Loading

str4d Oct 31, 2022

daira Oct 31, 2022

daira Oct 31, 2022

str4d Oct 31, 2022 •

edited

Loading

daira Oct 31, 2022

str4d Oct 31, 2022

daira left a comment •

edited

Loading

daira left a comment •

edited

Loading

daira left a comment

ebfull left a comment

str4d commented Nov 2, 2022

ebfull left a comment

Breaking additions to the Field trait #93

Breaking additions to the Field trait #93

Conversation

str4d commented Oct 28, 2022 • edited Loading

daira Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

str4d Oct 31, 2022

Choose a reason for hiding this comment

daira Oct 31, 2022

Choose a reason for hiding this comment

daira Oct 31, 2022

Choose a reason for hiding this comment

str4d Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

daira Oct 31, 2022

Choose a reason for hiding this comment

str4d Oct 31, 2022

Choose a reason for hiding this comment

daira left a comment • edited Loading

Choose a reason for hiding this comment

daira left a comment • edited Loading

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

ebfull left a comment

Choose a reason for hiding this comment

str4d commented Nov 2, 2022

ebfull left a comment

Choose a reason for hiding this comment

Breaking additions to the `Field` trait #93

Breaking additions to the `Field` trait #93

str4d commented Oct 28, 2022 •

edited

Loading

daira Oct 31, 2022 •

edited

Loading

str4d Oct 31, 2022 •

edited

Loading

daira left a comment •

edited

Loading

daira left a comment •

edited

Loading