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

Wrap btcec's secp256k1 implementation for kyber.{Group,Point,Scalar} #385

Conversation

Projects
2 participants
@coventry
Copy link

commented May 17, 2019

This is a go-only, variable time implementation of the kyber.{Group,Point,Scalar} interfaces for secp256k1 (bitcoin/ethereum curve.) Please see issue #377 for discussion.

With minor changes to the test suites, this passes the vss/rabin, dkg/rabin, and dss tests. I've left those changes out, but included some adjustments to vss_test.go which helped me when debugging.

@coventry coventry force-pushed the smartcontractkit:kyber-wrapper-for-btcec-secp256k1 branch from d6979ed to 5c94e8a May 18, 2019

@jeffallen jeffallen added this to WIP in Cothority via automation May 21, 2019

@jeffallen jeffallen moved this from WIP to Ready4Merge in Cothority May 22, 2019

@jeffallen jeffallen self-assigned this May 22, 2019

@jeffallen jeffallen self-requested a review May 22, 2019

@jeffallen
Copy link
Contributor

left a comment

I've got more research to do on this, but I wanted to post this first comment now.

@@ -1,6 +1,7 @@
module go.dedis.ch/kyber/v3

require (
github.com/btcsuite/btcd v0.0.0-20190410025418-9bfb2ca0346b

This comment has been minimized.

Copy link
@jeffallen

jeffallen May 22, 2019

Contributor

btcd is a rather large dependency, since all we really want is btcec. Their license (the ISC license) would allow us to fork their code, and bring in btcec directly into Kyber if we wanted. btcec is fairly stable, having received just a couple commits in 2 years.

Did you consider incorporating btcec as source instead of as a dependency?

This comment has been minimized.

Copy link
@coventry

coventry May 22, 2019

Author

No, I hadn't, but I'd be happy to do that if the licence allows, and expect it would be a straightforward modification.

This comment has been minimized.

Copy link
@jeffallen

jeffallen May 24, 2019

Contributor

Yes, please.

@jeffallen
Copy link
Contributor

left a comment

This is good, useful work, and I can certainly see adding it to Kyber, but let's keep discussing the points below, thanks.

import (
"math/big"

secp256k1BTCD "github.com/btcsuite/btcd/btcec"

This comment has been minimized.

Copy link
@jeffallen

jeffallen May 24, 2019

Contributor

Why do you rename the package here? Anyway, with the inclusion of the btcec code, this will all be local and not imported.

You should include the minimum of the btcec code you can, for example not ciphering.go, etc. Probably just ScalarBaseMult and ScalarMult and their dependencies, including the pre-computed point table. Furthermore, you can leave gensecp256k1.go and genprecomps.go out, just change the comment in secp256k1.go to explain where it came from (i.e. the real btcec package).

This comment has been minimized.

Copy link
@coventry

coventry May 24, 2019

Author

I just found the name btcec obscure.

@@ -0,0 +1,76 @@
// Package secp256k1 is an implementation of the kyber.{Group,Point,Scalar}

This comment has been minimized.

Copy link
@jeffallen

jeffallen May 24, 2019

Contributor

This suite should be registered in package go.dedis.ch/kyber/v3/suites.

// MarshalBinary returns the big-endian byte representation of s, or an error on
// failure
func (s *secp256k1Scalar) MarshalBinary() ([]byte, error) {
b := toInt(s.modG()).Bytes()

This comment has been minimized.

Copy link
@jeffallen

jeffallen May 24, 2019

Contributor

Why is that modG in there? It has the consequence that a MarshalBinary can change the underlying data, which is surprising to callers. Compare to pairing/bn256/point.go, where MarshalBinary uses Clone when it knows it's about to call MakeAffine and modify the point.

This comment has been minimized.

Copy link
@coventry

coventry May 24, 2019

Author

This is defensive programming, since a secp256k1 should already be reduced mod the groupOrder. I can take a clone first, though.

@@ -0,0 +1,127 @@
package secp256k1

This comment has been minimized.

Copy link
@jeffallen

jeffallen May 24, 2019

Contributor

I was hoping to see test cases that compare operations in this library to those in other libraries. Can you find out if there are a standard set of inputs and outputs that other secp256k1 implementations use to check compatibility between each other? I'm not saying that we have to have this, just that if such a thing is normally used, then we should be using it as well. Or you could be the first: generate a few scalars and points using libsecp256k1 and then confirm that the same results come from this library.

This comment has been minimized.

Copy link
@coventry

coventry May 24, 2019

Author

I don't think there's anything standard, though of course there are test vectors we could borrow from other suites: Python's cryptography package, some random vectors.


import (
"encoding/hex"
"math/big"

This comment has been minimized.

Copy link
@jeffallen

jeffallen May 24, 2019

Contributor

group/nist/curve.go uses group/mod to do scalar modular math, why did you decide not to use it? I have to say, I'm not totally clear on the difference, what would be the benefits, etc. But I generally understand that the SECG group's curves are like the NIST curves, but with constants that are more transparent ("no numbers up my sleeves"), and so I'm wondering why the implementation in this directory would be different from ../nist.

This comment has been minimized.

Copy link
@coventry

coventry May 24, 2019

Author

I just didn't see much value in the abstraction group/mod provided. Plus it carries two unnecessary fields.

@jeffallen jeffallen moved this from Ready4Merge to WIP in Cothority May 24, 2019

@coventry

This comment has been minimized.

Copy link
Author

commented May 24, 2019

Thanks for the feedback. I'll revise accordingly.

@jeffallen

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Ping?

@coventry

This comment has been minimized.

Copy link
Author

commented Jun 6, 2019

Sorry, Jeff. It's been hard to find time to work on this. The folks I've been developing it for don't see the value in ripping the minimal subset of btcec out of btcd.

@jeffallen

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

OK, understand. But we cannot accept a giant growth in dependencies for this. Kyber is used in systems that need to stay as small and tight as possible. I am closing for now, please re-open if you find a way to move forward on this.

Thanks again for your interest in Kyber.

@jeffallen jeffallen closed this Jun 17, 2019

Cothority automation moved this from WIP to Closed Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.