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

Adds decaf group #115

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Adds decaf group #115

wants to merge 6 commits into from

Conversation

armfazh
Copy link
Contributor

@armfazh armfazh commented May 15, 2020

Includes a package for the decaf group.

Decaf is specified https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-ristretto255-decaf448-03, this implementation is fully compatible with draft. Test vectors for group operations and hashing to group are passing.

Decaf (circl/group) is a safe group abstraction on top of the Goldilocks (edwards448) curve (circl/ecc/goldilocks).

Ed448 also uses Goldilocks curve but only requires specific operations.

Internally, Goldilocks curve is implemented with ted448, a twist of the curve, since it provides faster point arithmetic.

@armfazh armfazh added the enhancement Improvement over something already in the project label May 15, 2020
@armfazh armfazh self-assigned this May 15, 2020
Copy link

@alxdavids alxdavids left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me, most of my comments relate to style.

ecc/goldilocks/decaf.go Outdated Show resolved Hide resolved
ecc/goldilocks/constants.go Outdated Show resolved Hide resolved
ecc/goldilocks/constants.go Outdated Show resolved Hide resolved
ecc/goldilocks/twist.go Outdated Show resolved Hide resolved
sign/ed448/ed448.go Outdated Show resolved Hide resolved
_ = goldilocks.Curve{}.ScalarBaseMult(s).ToBytes(pair.public[:])
P := &goldilocks.Point{}
goldilocks.Curve{}.ScalarBaseMult(P, s)
enc, _ := P.MarshalBinary()

Choose a reason for hiding this comment

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

Error being ignored intentionally?

Copy link
Member

Choose a reason for hiding this comment

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

Encoding always succeeds, but Go's standard MarshalBinary interface doesn't care. Maybe it's worthwhile having a separate PackInto function that writes into a buffer instead of returning it. Saves an allocation and then you don't have an error to confuse people about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this error will not occur, added a panic if it happens. Not the best way to handle this error.
I will consider adding PackInto.

ecc/goldilocks/decaf.go Outdated Show resolved Hide resolved
fp.Sub(u, u, &one) // u = y^2-1
fp.Sub(v, v, &one) // v = dy^2-1
isQR := fp.InvSqrt(&P.x, u, v) // x = sqrt(u/v)
if !isQR {
Copy link
Member

Choose a reason for hiding this comment

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

This is not constant time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated take a look

Copy link
Member

Choose a reason for hiding this comment

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

Looks good.

Copy link
Member

Choose a reason for hiding this comment

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

It might be hard though to write constant time algorithms using this function as it returns an error instead of an int. In go-ristretto I have two variants: Equals and EqualsI. The former returns the idiomatic bool and the latter an int, which allows one to write constant time code.

ecc/goldilocks/twist.go Outdated Show resolved Hide resolved
var isZero int
if k.IsZero() {
if kk.IsZero() {
Copy link
Member

Choose a reason for hiding this comment

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

Not constant time.

ecc/goldilocks/decaf_test.go Outdated Show resolved Hide resolved
@armfazh armfazh mentioned this pull request May 15, 2020
ecc/goldilocks/decaf.go Outdated Show resolved Hide resolved
@claucece
Copy link
Contributor

I'll check this tomorrow @armfazh

@claucece
Copy link
Contributor

Nice! Have you run it against the vectors of: https://sourceforge.net/p/ed448goldilocks/code/ci/master/tree/test/ ?

Copy link

@wbl wbl left a comment

Choose a reason for hiding this comment

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

We should talk more about where the formulas come from and how to make clear the code matches them.

ecc/goldilocks/constants.go Outdated Show resolved Hide resolved
ecc/goldilocks/twist.go Outdated Show resolved Hide resolved
// Double returns 2P.
func (Curve) Double(P *Point) *Point { R := *P; R.Double(); return &R }
// Double calculates R = 2P.
func (Curve) Double(R, P *Point) { *R = *P; R.Double() }
Copy link

Choose a reason for hiding this comment

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

This is a change to the external API of the package.

ecc/goldilocks/decaf.go Outdated Show resolved Hide resolved
ecc/goldilocks/decaf_test.go Outdated Show resolved Hide resolved
ecc/goldilocks/decaf.go Outdated Show resolved Hide resolved
ecc/goldilocks/decaf.go Outdated Show resolved Hide resolved
@armfazh armfazh force-pushed the addDecaf branch 2 times, most recently from 93a4a24 to c97b01e Compare June 10, 2020 08:58
@armfazh armfazh force-pushed the addDecaf branch 2 times, most recently from f120857 to ec347f5 Compare July 24, 2020 22:29
@armfazh armfazh added changesAPI PR changes the API of a package new feature New functionality or module and removed enhancement Improvement over something already in the project labels Jul 24, 2020
@armfazh armfazh requested review from wbl and alxdavids July 24, 2020 23:19
@armfazh armfazh linked an issue Jul 24, 2020 that may be closed by this pull request
Copy link
Member

@bwesterb bwesterb left a comment

Choose a reason for hiding this comment

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

I'll continue my review next week. I can't concentrate on the maths with this heat wave.

ecc/decaf/constants.go Outdated Show resolved Hide resolved
func IsValid(a *Elt) bool { return ted448.IsOnCurve(&a.p) }

// Identity returns the identity element of the group.
func Identity() *Elt { return &Elt{ted448.Identity()} }
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the Go compiler gets rid of the allocation here.

ecc/decaf/decaf.go Outdated Show resolved Hide resolved
ecc/decaf/decaf.go Outdated Show resolved Hide resolved
internal/ted448/point.go Outdated Show resolved Hide resolved
internal/ted448/point.go Outdated Show resolved Hide resolved
internal/ted448/point.go Outdated Show resolved Hide resolved
internal/ted448/point.go Outdated Show resolved Hide resolved
internal/ted448/point.go Outdated Show resolved Hide resolved
internal/ted448/point.go Outdated Show resolved Hide resolved
group/decaf448/decaf.go Outdated Show resolved Hide resolved
internal/ted448/scalar.go Outdated Show resolved Hide resolved
@armfazh
Copy link
Contributor Author

armfazh commented Aug 26, 2020

@bwesterb Addressed requested changes.

internal/ted448/scalar.go Outdated Show resolved Hide resolved
@claucece
Copy link
Contributor

claucece commented Nov 5, 2020

We should add here the draft-ristretto-decaf test vectors, which is pretty easy as they are the multiplicatives of the generator. We can also add here the one-way-map of the draft. I also did a small implementation here of it: https://github.com/claucece/sage-ristretto255-decaf448 in sage.

ecc/goldilocks/constants.go Outdated Show resolved Hide resolved
}

s := &fp.Elt{}
copy(s[:], b[:fp.Size])
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm.. is there a reason why this constrain should be enforced b[:fp.Size]?

ecc/goldilocks/decaf.go Outdated Show resolved Hide resolved
@wbl wbl requested review from tanyav2 and removed request for alxdavids April 23, 2021 22:34
@chris-wood
Copy link
Contributor

@armfazh what's the plan for this PR?

@armfazh
Copy link
Contributor Author

armfazh commented Mar 11, 2022

what's the plan for this PR?

Just pushed changes that update this PR.
Please review: @chris-wood @bwesterb @claucece

@armfazh armfazh requested review from bwesterb and claucece and removed request for wbl March 11, 2022 04:36
@bwesterb
Copy link
Member

what's the plan for this PR?

It's been a while back, but I believe I haven't gone through all the maths in detail. If you want a detailed review, then I'll need to dedicate a day or two to it. Is there a deadline?

Squashed commit of the following:

commit ec347f5
Author: armfazh <[email protected]>
Date:   Fri Jul 24 15:28:34 2020 -0700

    Adding decaf group.

commit 796b37e
Author: armfazh <[email protected]>
Date:   Wed Jul 22 19:44:07 2020 -0700

    Updates internal packages of Ed448.

commit f5957e7
Author: armfazh <[email protected]>
Date:   Wed Jul 22 19:11:15 2020 -0700

    Updating Decaf documentation.

commit 6f573e6
Author: armfazh <[email protected]>
Date:   Wed Jul 22 13:56:12 2020 -0700

    Updating documentation of ted448 internal package.

commit 7e80bfc
Author: armfazh <[email protected]>
Date:   Wed Jun 10 01:56:31 2020 -0700

    Adapting ed448 for using the internal ted448 package.

commit 44ce6c5
Author: armfazh <[email protected]>
Date:   Tue Jun 9 23:36:17 2020 -0700

    ted448 point public fields.

commit 13dd30d
Author: armfazh <[email protected]>
Date:   Tue Jun 9 20:32:32 2020 -0700

    Moving twist implementation to an internal package.

commit 4cdcb71
Author: armfazh <[email protected]>
Date:   Mon Jun 1 01:30:22 2020 -0700

    Solving scalar constant-time operations.

commit 26b9ea4
Author: armfazh <[email protected]>
Date:   Thu May 21 09:21:36 2020 -0700

    Review comments on formulas.

commit ff821b5
Author: armfazh <[email protected]>
Date:   Tue May 19 16:54:36 2020 -0700

    Adding tests for detecting decaf/point invalid encodings.

commit 3881ea8
Author: armfazh <[email protected]>
Date:   Fri May 15 15:13:53 2020 -0700

    One test for decaf, unmarshaling straight-line code, and check for errors.

commit 9b048c0
Author: armfazh <[email protected]>
Date:   Thu May 14 18:12:57 2020 -0700

    Updating interface for decaf and curve.

commit a99153c
Author: armfazh <[email protected]>
Date:   Thu May 14 16:10:04 2020 -0700

    Adding goldilocks documentation.

commit a62d6bd
Author: armfazh <[email protected]>
Date:   Thu May 14 14:15:02 2020 -0700

    Adding decaf v1.1 and kat tests.

commit c72bdfa
Author: armfazh <[email protected]>
Date:   Wed May 13 13:30:11 2020 -0700

    Removing non-used fp functions.

commit d4fc865
Author: armfazh <[email protected]>
Date:   Tue May 12 11:47:57 2020 -0700

    Adding some helper functions.

commit 6173c83
Author: armfazh <[email protected]>
Date:   Tue May 12 05:15:55 2020 -0700

    Decaf decoding working. More tests needed.

commit daa36c1
Author: armfazh <[email protected]>
Date:   Mon May 11 16:16:16 2020 -0700

    Decaf encoding is working, except by the choice of generator.

commit 8933f6c
Author: armfazh <[email protected]>
Date:   Thu May 7 01:19:47 2020 -0700

    Decaf encoding requires cannon sqrt.

commit 9479b45
Author: armfazh <[email protected]>
Date:   Wed Apr 29 14:51:21 2020 -0700

    Adding support for decaf quotient group.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changesAPI PR changes the API of a package new feature New functionality or module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plans for decaf/ed448
6 participants