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.

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

Generic arithmetic for Weierstrass curves #218

Closed
wants to merge 20 commits into from
Closed

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Oct 2, 2020

The GOST ECDSA standard is a bit peculiar in a sense that it does not fix parameters of a curve (i.e. a, b and p are variable). AFAIK only 4 256-bit (OID 1.2.643.7.1.2.1.1) and 3 512-bit curves (OID 1.2.643.7.1.2.1.2) recommended by TK-26 are used in practice in interoperable applications. So to reduce amount of boilerplate I need a generic implementation of the Weierstrass curve arithmetic.

This PR not only attempts to generalize over code from the p256 crate, but also operates in native word sizes (so it should be more efficient on 32-bit platforms and in theory could even run on 16-bit ones, though I only tested it on 64-bits for now) and supports variable curve sizes. Unfortunately it leads to quite bloated trait bounds and precludes methods constification and since we have to use loops, performance may degrade a bit if compiler will not inline loops. Also some constants have to be expressed manually, even though they are computable from other EC parameters. Hopefully, with the advent of const generics and const-eval those problems will be mostly solved.

As of moment of this PR creation the code unfortunately is a bit incompatible with the current versions of elliptic-curve and ff, but probably with some work it can be solved. Also I think it could be worth to extract biguint logic from the field and scalar modules into a separate crate/module, not only it should reduce boilerplate a bit, but also could be useful for other crates (e.g. RSA and SRP).

BTW some recommended GOST curves can be represented as a twisted Edwards curve. Considering that we already implement branch-free addition developed by Renes et al., is it worth in your opinion to bother with implementing them as Edwards curves instead? I guess, it could be useful for EdDSA?

weierstrass/src/gost_test256.rs Outdated Show resolved Hide resolved
weierstrass/src/utils.rs Outdated Show resolved Hide resolved
weierstrass/src/scalar.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

tarcieri commented Oct 2, 2020

@newpavlov regarding synthesizing finite field implementations specifically, see also: https://github.com/zkcrypto/ff/tree/main/ff_derive

More generally: this looks interesting to me, but also this seems like a somewhat classical area for leaky abstractions. Thinking of the Golang implementation in particular here: https://golang.org/pkg/crypto/elliptic/

@codecov-io
Copy link

codecov-io commented Oct 15, 2020

Codecov Report

Merging #218 (95b658d) into master (0aa8965) will decrease coverage by 6.85%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
- Coverage   58.70%   51.85%   -6.86%     
==========================================
  Files          25       34       +9     
  Lines        3775     4295     +520     
==========================================
+ Hits         2216     2227      +11     
- Misses       1559     2068     +509     
Impacted Files Coverage Δ
weierstrass/src/affine.rs 0.00% <0.00%> (ø)
weierstrass/src/field.rs 0.00% <0.00%> (ø)
weierstrass/src/projective.rs 0.00% <0.00%> (ø)
weierstrass/src/scalar.rs 0.00% <0.00%> (ø)
weierstrass/src/utils.rs 0.00% <0.00%> (ø)
k256/src/ecdsa/sign.rs 19.44% <0.00%> (-10.92%) ⬇️
k256/src/ecdsa/verify.rs 48.83% <0.00%> (-2.60%) ⬇️
k256/src/ecdsa/recoverable.rs 57.14% <0.00%> (-1.53%) ⬇️
k256/src/arithmetic/scalar.rs 77.86% <0.00%> (-0.77%) ⬇️
k256/src/arithmetic/scalar/scalar_8x32.rs 91.48% <0.00%> (-0.22%) ⬇️
... and 15 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 0aa8965...95b658d. Read the comment docs.

Comment on lines 1 to 26
use generic_array::typenum::Unsigned;
use crate::{DoubleWord, Word, WordWidth};

/// Computes `a + b + carry`, returning the result along with the new carry.
#[inline(always)]
pub(crate) const fn adc(a: Word, b: Word, carry: Word) -> (Word, Word) {
let ret = (a as DoubleWord) + (b as DoubleWord) + (carry as DoubleWord);
(ret as Word, (ret >> WordWidth::USIZE) as Word)
}

/// Computes `a - (b + borrow)`, returning the result along with the new borrow.
#[inline(always)]
pub(crate) const fn sbb(a: Word, b: Word, borrow: Word) -> (Word, Word) {
let (a, b) = (a as DoubleWord, b as DoubleWord);
let t = (borrow >> (WordWidth::USIZE - 1)) as DoubleWord;
let ret = a.wrapping_sub(b + t);
(ret as Word, (ret >> WordWidth::USIZE) as Word)
}

/// Computes `a + (b * c) + carry`, returning the result along with the new carry.
#[inline(always)]
pub(crate) const fn mac(a: Word, b: Word, c: Word, carry: Word) -> (Word, Word) {
let (a, b, c) = (a as DoubleWord, b as DoubleWord, c as DoubleWord);
let ret = a + b * c + (carry as DoubleWord);
(ret as Word, (ret >> WordWidth::USIZE) as Word)
}
Copy link
Member

Choose a reason for hiding this comment

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

FYI, there are some equivalents of these in:

https://docs.rs/elliptic-curve/0.6.6/elliptic_curve/util/index.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but those functions work with u64, while these ones with words (i.e. will use u32 on 32-bit targets).

Copy link
Member

@tarcieri tarcieri Oct 15, 2020

Choose a reason for hiding this comment

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

They work with both, actually! There are 32-bit and 64-bit equivalents of each.

The Word-based approach is interesting though, and the only reason I haven't done such gating is because p256 presently only has a 64-bit field backend (k256 has both 32-bit and 64-bit).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes. I meant right now it has two separate variants, e.g. adc32 and adc64.

@tarcieri
Copy link
Member

tarcieri commented Oct 15, 2020

AFAIK only 4 256-bit (OID 1.2.643.7.1.2.1.1) and 3 512-bit curves (OID 1.2.643.7.1.2.1.2) recommended by TK-26 are used in practice in interoperable applications. So to reduce amount of boilerplate I need a generic implementation of the Weierstrass curve arithmetic.

@str4d in theory we could synthesize large parts of these implementations using the ff_derive crate, right?

This PR not only attempts to generalize over code from the p256 crate, but also operates in native word sizes (so it should be more efficient on 32-bit platforms and in theory could even run on 16-bit ones, though I only tested it on 64-bits for now) and supports variable curve sizes. Unfortunately it leads to quite bloated trait bounds and precludes methods constification and since we have to use loops, performance may degrade a bit if compiler will not inline loops. Also some constants have to be expressed manually, even though they are computable from other EC parameters.

My initial impressions are this is extremely generic code, to the point I think it might be difficult to work with. But I can't say that using a proc macro to write the code ala ff_derive is any better from a developer ergonomics perspective.

As of moment of this PR creation the code unfortunately is a bit incompatible with the current versions of elliptic-curve and ff, but probably with some work it can be solved.

It'd be really nice if we could figure out a single set of traits to use for expressing scalar and group operations, so that it's possible to implement algorithms generically over prime order curves/groups.

@Pratyush
Copy link

FWIW we have such generic implementations here and in particular in the ark-ec crate

Some of the design decisions there could be informative (though we are planning to do a little revamp is some of the traits)

@cargo-dep-bot
Copy link

cargo-dep-bot bot commented Dec 2, 2020

This PR made the following dependency changes:

Added Packages (Duplicate versions in '()'):
	biguint-literal 0.1.0
	weierstrass 0.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants