Skip to content

Constant-time APIs for ff::Field::[invert, sqrt]#191

Merged
str4d merged 2 commits into
zcash:developfrom
str4d:ff-ct-inv-and-sqrt-apis
Dec 13, 2019
Merged

Constant-time APIs for ff::Field::[invert, sqrt]#191
str4d merged 2 commits into
zcash:developfrom
str4d:ff-ct-inv-and-sqrt-apis

Conversation

@str4d
Copy link
Copy Markdown
Contributor

@str4d str4d commented Dec 13, 2019

The pairing::bls12_381 and zcash_primitives::jubjub implementations are not constant-time, but will be replaced by the bls12_381 and jubjub implementations which are.

The ff_derive implementation of sqrt is constant-time (having been ported from bls12_381::Scalar), but the invert implementation is not constant-time yet.

Part of #159.

WARNING: THIS IS NOT ACTUALLY CONSTANT TIME YET!

The jubjub and bls12_381 crates will replace our constant-time usages,
but we NEED to fix ff_derive because other users will expect it to
implement the Field trait correctly.
WARNING: THIS IS NOT FULLY CONSTANT TIME YET!

This will be fixed once we migrate to the jubjub and bls12_381 crates.
@str4d str4d requested a review from ebfull December 13, 2019 21:32
Comment thread ff/ff_derive/src/lib.rs

fn inverse(&self) -> Option<Self> {
/// WARNING: THIS IS NOT ACTUALLY CONSTANT TIME YET!
/// TODO: Make this constant-time.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This will likely require inlining addition-chain finding into the proc macro.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That sounds beautiful! If only I could stand how many dependencies proc-macro wants to bring in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Don't need to worry about proc-macro dependencies if your crate itself is a procedural macro 🙃

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Opened zkcrypto/ff#15 for this.

let gamma_inverse = gamma.inverse().ok_or(SynthesisError::UnexpectedIdentity)?;
let delta_inverse = delta.inverse().ok_or(SynthesisError::UnexpectedIdentity)?;
let gamma_inverse = {
let inverse = gamma.invert();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if there should be an impl From<Option<T>> for CtOption<T> in subtle...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think that makes it too easy to slip out of constant-time-ness, which could lead to subtle bugs. I'm rather partial to the fact that if you want to leave that domain, it needs to be noisy. It would definitely be nice to have it be easier than it currently is; it should be possible to do that while still being noisy.

@str4d
Copy link
Copy Markdown
Contributor Author

str4d commented Dec 13, 2019

This does not affect consensus directly, and the existing test coverage is sufficient to ensure compatibility. This will also all be audited before landing in master.

@str4d str4d merged commit e88e2a9 into zcash:develop Dec 13, 2019
@str4d str4d deleted the ff-ct-inv-and-sqrt-apis branch December 13, 2019 22:29
@str4d str4d added the S-scratching-an-itch Status: Something we haven't planned for a sprint but are doing anyway label Dec 13, 2019
@str4d str4d added this to the Core Sprint 2019-49 milestone Dec 13, 2019
greg0x pushed a commit to valargroup/librustzcash that referenced this pull request Mar 12, 2026
Keystone multi-bundle UX improvements and account-scoped note filtering
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-scratching-an-itch Status: Something we haven't planned for a sprint but are doing anyway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants