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

Change Scalar::from_canonical_bytes to return CtOption #472

Merged
merged 1 commit into from
Dec 12, 2022
Merged

Change Scalar::from_canonical_bytes to return CtOption #472

merged 1 commit into from
Dec 12, 2022

Conversation

tarcieri
Copy link
Contributor

@tarcieri tarcieri commented Dec 12, 2022 •
edited
Loading

NOTE: includes #470 which should be merged first Merged!

This is helpful for implementing ff::PrimeField::from_repr.

Also changes Scalar::is_canonical to return Choice.

@tarcieri tarcieri force-pushed the have-scalar-from-canonical-bytes-return-ctoption branch 4 times, most recently from 16abcaf to 958a20d Compare December 12, 2022 00:59
tarcieri
tarcieri commented Dec 12, 2022
View reviewed changes
Cargo.toml
@@ -50,7 +50,7 @@ required-features = ["rand_core"]
cfg-if = "1"
rand_core = { version = "0.6.4", default-features = false, optional = true }
digest = { version = "0.10", default-features = false, optional = true }
subtle = { version = "^2.2.1", default-features = false }
subtle = { version = "2.3.0", default-features = false }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: needed for impl From<CtOption<T>> for Option<T>

tarcieri
tarcieri commented Dec 12, 2022
View reviewed changes
Cargo.toml
Comment on lines +69 to +70
[profile.dev]
opt-level = 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes the tests run 5 times faster for me with cargo test, with compile times effectively the same

@tarcieri tarcieri force-pushed the have-scalar-from-canonical-bytes-return-ctoption branch from 958a20d to 5ee3a07 Compare December 12, 2022 01:16
str4d
str4d approved these changes Dec 12, 2022
View reviewed changes
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.

I'm fine with these API changes, though they are not technically necessary to implement ff::PrimeField (we can inline these changed methods into that impl if API stability is preferred here).

src/scalar.rs Show resolved Hide resolved
src/scalar.rs Show resolved Hide resolved
@tarcieri
Copy link
Contributor Author

If we want to keep the Scalar APIs the same, it's easy enough to add this constant-time version in the ff::PrimeField::from_repr method, since it's 3 lines

@str4d str4d mentioned this pull request Dec 12, 2022
Closed
@elichai
Copy link
Contributor

elichai commented Dec 12, 2022

I opened a similar PR a few months ago: #384

@tarcieri
Change Scalar::from_canonical_bytes to return CtOption
e665a4c 
This is helpful for implementing `ff::PrimeField::from_repr`.

Also changes `Scalar::is_canonical` to return `Choice`.
@tarcieri tarcieri force-pushed the have-scalar-from-canonical-bytes-return-ctoption branch from c51ba24 to e665a4c Compare December 12, 2022 14:28
@tarcieri
Copy link
Contributor Author

@elichai oh nice! maybe we can get this straightforward version in then you can rebase your PRs which add the performance improvements?

@tarcieri tarcieri marked this pull request as ready for review December 12, 2022 14:31
@tarcieri tarcieri requested a review from rozbb December 12, 2022 14:31
@tarcieri tarcieri mentioned this pull request Dec 12, 2022
Merged
elichai
elichai approved these changes Dec 12, 2022
View reviewed changes
Copy link
Contributor

@elichai elichai left a comment

Choose a reason for hiding this comment

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

utACK, reviewed the changes and they seem correct to my understanding of the library

rozbb
rozbb reviewed Dec 12, 2022
View reviewed changes
src/scalar.rs
pub fn is_canonical(&self) -> bool {
*self == self.reduce()
pub fn is_canonical(&self) -> Choice {
self.ct_eq(&self.reduce())
Copy link
Contributor

Choose a reason for hiding this comment

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

Just making certain: is reduce() constant time itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's supposed to be, and if it weren't, it would be a general security issue.

@str4d
Copy link
Contributor

str4d commented Dec 12, 2022

Here's another argument for making this change: If someone still wants variable-time Scalar::from_canonical_bytes, they can use PrimeField::from_repr_vartime once #473 lands. So the only API being removed from 4.0 is the variable-time Scalar::is_canonical, but Scalar::reduce is public, so the user can implement this manually if they need it.

@rozbb rozbb merged commit 274f4a7 into release/4.0 Dec 12, 2022
@pinkforest pinkforest mentioned this pull request Dec 12, 2022
Merged
@elichai elichai mentioned this pull request Dec 14, 2022
Open
@tarcieri tarcieri deleted the have-scalar-from-canonical-bytes-return-ctoption branch May 31, 2023 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rozbb rozbb rozbb left review comments

@str4d str4d str4d approved these changes

@elichai elichai elichai approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tarcieri @elichai @str4d @rozbb