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

ZKLedger Compatibility Work #16

Merged
merged 3 commits into from Feb 19, 2019

Conversation

Projects
None yet
3 participants
@gertjaap
Copy link
Contributor

commented Feb 15, 2019

This PR contains changes needed for integrating ZKSigma into
ZKLedger:

  • The ZKCurve is no longer in here, but you need to pass the curve
    parameters into the proof functions. ZKCurve will be defined in
    ZKLedger. There is a TestCurve defined that is used in the test cases.

  • ConsistencyProof struct contained two private variables; that resulted
    in failures when ZKLedger serialized/deserialized these after
    transmitting them over the wire. Resulting in failures in the local
    experiments.

gertjaap added some commits Feb 15, 2019

ZKLedger compatibility work
This commit contains changes needed for integrating ZKSigma into
ZKLedger:

* The ZKCurve is no longer in here, but you need to pass the curve
parameters into the proof functions. ZKCurve will be defined in
ZKLedger. There is a TestCurve defined that is used in the test cases.

* ConsistencyProof struct contained two private variables; that resulted
in failures when ZKLedger serialized/deserialized these after
transmitting them over the wire. Resulting in failures in the local
experiments.
@@ -89,63 +82,63 @@ func (p ECPoint) Equal(p2 ECPoint) bool {
}

// Mult multiplies point p by scalar s and returns the resulting point
func (p ECPoint) Mult(s *big.Int) ECPoint {
func (p ECPoint) Mult(s *big.Int, zkpcp ZKPCurveParams) ECPoint {

This comment has been minimized.

Copy link
@narula

narula Feb 15, 2019

Member

Argh, this just seems weird. ECPoints only make sense on a given curve -- you can't pass in any old ZKPCurveParams here. I'm not sure these functions should take in a zkpcp struct instead of just taking in C, G, and H as necessary.

I am not suggesting you make this change yet, just noting my thoughts here for the moment.

This comment has been minimized.

Copy link
@gertjaap

gertjaap Feb 18, 2019

Author Contributor

I agree this looks weird. It seems that the Mult was added as convenience function on ECPoint, but that only worked with fixed curve parameters. Since it used all params from ZKCurve, that made sense. I agree that adding in curve parameters is ugly.

Maybe we should make it a method on ZKPCurveParams? Same for Add/Sub. So:

ZKPCurveParams.Mult(point, scalar)
ZKPCurveParams.Add(point, point)
ZKPCurveParams.Sub(point, point)

This comment has been minimized.

Copy link
@Nabeelperson

Nabeelperson Feb 19, 2019

Member

Yes, the Mult/Add/Sub functions were operated on the ECPoint assuming that the curve parameters were changed in init() and not on-the-fly as you are suggesting here. Should we come up with a ChangeCurve(ZKPCurveParams) sort of function for changing the curve globally? Or should we do something like hash libraries and return a context that can be passed around?

This comment has been minimized.

Copy link
@Nabeelperson

Nabeelperson Feb 19, 2019

Member

Looking over the changes I think @gertjaap suggestion on ZKPCurveParams.Mult(point, scalar) as it would look the cleanest overall in terms of what you need to think about when you want to call Mult/Add/Sub. Programmers will think (1) on what curve do I want to mult/add/sub? (2) what points/scalars do I need?


// if p.Equal(Zero) {
// logStuff("Mult: Trying to multiple with zero-point!\n")
// return p
// } else
if p.Equal(ZKCurve.G) {
X, Y := ZKCurve.C.ScalarBaseMult(modS.Bytes())
if p.Equal(zkpcp.G) {

This comment has been minimized.

Copy link
@narula

narula Feb 15, 2019

Member

what about p.Equal(zkpcp.H)? Why doesn't that use ScalarBaseMultH?

This comment has been minimized.

Copy link
@gertjaap

gertjaap Feb 18, 2019

Author Contributor

Thanks, yeah that makes sense. Changed it.

@@ -58,61 +58,61 @@ type ABCProof struct {
// in commitments A, B and C respectively.
// Option Left is proving that A and C commit to zero and simulates that A, B and C commit to v, inv(v) and 1 respectively.
// Option Right is proving that A, B and C commit to v, inv(v) and 1 respectively and simulating that A and C commit to 0.
func NewABCProof(CM, CMTok ECPoint, value, sk *big.Int, option Side) (*ABCProof, error) {
func NewABCProof(zkpcp ZKPCurveParams, CM, CMTok ECPoint, value, sk *big.Int, option Side) (*ABCProof, error) {

This comment has been minimized.

Copy link
@Nabeelperson

Nabeelperson Feb 19, 2019

Member

I think for initial release we should keep it simple with the function signatures and 'setup' required by people trying to use the code. In a later release we can incorporate curve switching that does not involve changing some variables within 'init()'.

@@ -89,63 +82,63 @@ func (p ECPoint) Equal(p2 ECPoint) bool {
}

// Mult multiplies point p by scalar s and returns the resulting point
func (p ECPoint) Mult(s *big.Int) ECPoint {
func (p ECPoint) Mult(s *big.Int, zkpcp ZKPCurveParams) ECPoint {

This comment has been minimized.

Copy link
@Nabeelperson

Nabeelperson Feb 19, 2019

Member

Looking over the changes I think @gertjaap suggestion on ZKPCurveParams.Mult(point, scalar) as it would look the cleanest overall in terms of what you need to think about when you want to call Mult/Add/Sub. Programmers will think (1) on what curve do I want to mult/add/sub? (2) what points/scalars do I need?

@Nabeelperson Nabeelperson merged commit 1ea0a78 into mit-dci:master Feb 19, 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.