Skip to content
This repository has been archived by the owner on Jul 15, 2019. It is now read-only.

Ported the vrf verification feature #58

Merged
merged 9 commits into from
Oct 2, 2015
Merged

Ported the vrf verification feature #58

merged 9 commits into from
Oct 2, 2015

Conversation

adon-at-work
Copy link
Contributor

This is the vrf verification implementation ported from https://github.com/yahoo/coname/blob/master/vrf/vrf.go

It lives as an utility function, and is tested with inputs and outputs captured from the original golang implementation.

Please review, thanks.

adon added 2 commits September 14, 2015 19:31
- imported js-sha3 v0.3.1 from https://github.com/emn178/js-sha3
- extended js-sha3 to support SHAKE256 (output correct up to first
1088bits)
- allowed output byte array of 32 bytes
* Copyright 2015, emn178@gmail.com
*
* Licensed under the MIT license:
* http://www.opensource.org/licenses/MIT
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this go under the @license header? I'm not sure what is the recommended way to include the MIT license in an Apache2 project. @gyehuda

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gd point. i was curious what to do too. I copied the code from there, dropped some, and added some new features. so, what's the license then? follow the MIT? or stick with the Apache as used in other files? or dual licenses? trust the expert @gyehuda can give some insights. thanks

Copy link

Choose a reason for hiding this comment

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

Sorry for the delay. There are a couple of ways to address this, and it's mostly driven by project convention. Some projects have a strict process where the headers are kept consistent and that all inclusions like this are disclosed in a NOTICES file, other projects are much less strict and have what you see above -- an inclusion in the code that is preceded with the relevant license and copyright text. Both are OK. At first this latter approach is simpler, eventually the code could get messy and you'll want to clean it up.

What is most important though is that somewhere in the project you indicate that emn17 owns the copyright of some code that was licensed under the MIT license and is included in this project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @gyehuda.
@diracdeltas, can you suggest which convention/approach you prefer?


// e is either 1 or -1
// x = ev - (1-e)A/2
var x = e.isEqual(e2e.vrf.ed25519.curve.ONE) ? v : A.substract(v);

Choose a reason for hiding this comment

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

Please document that this is not constant time and thus must not be used with secret inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@adon-at-work
Copy link
Contributor Author

@andres-erbsen , once you finished reviewing this. I'll clear all the commented golang here. They're there to facilitate reviews only. OK?

@adon-at-work
Copy link
Contributor Author

@andres-erbsen and @diracdeltas please check those things fixed.

Since many of those discussions are collapsed since the last commit, I list out the following discussions that are yet to be resolved for quick reference (kindly click below to have them expanded, or you may like to check each collapsed one by one):

@adon-at-work
Copy link
Contributor Author

After the last commit, the final issue to deal with in this PR: #58 (diff)

// return false
// }
// hash.Reset()
hash = e2e.vrf.sha3Shake256(iiB.concat(m), 32);
Copy link
Contributor

Choose a reason for hiding this comment

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

hash is undeclared

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gd catch

@diracdeltas
Copy link
Contributor

Currently, the VRF module isn't included in any of the extension files, so the compiler isn't actually checking it for correctness. When I include the vrf module in the extension (such as by prepending goog.require('e2e.vrf.verify'); to launcher.js), Closure compiler throws a lot of errors.

goog.require('e2e.ecc.PrimeCurve');
goog.require('e2e.error.InvalidArgumentsError');

var ed25519 = e2e.ecc.DomainParam.fromCurve(e2e.ecc.PrimeCurve.ED_25519);
Copy link
Contributor

Choose a reason for hiding this comment

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

Declaring variables/functions like this will pollute the global namespace. Please make sure everything is contained in the module namespace. Ex: e2e.vrf.extra255519.ed25519_ = e2e.ecc.DomainParam.fromCurve(e2e.ecc.PrimeCurve.ED_25519)

The underscore at the end of a name indicates to the compiler that this variable is private (should not be accessed outside of the current module).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense.

@adon-at-work
Copy link
Contributor Author

Currently, the VRF module isn't included in any of the extension files, so the compiler isn't actually checking it for correctness. When I include the vrf module in the extension (such as by prepending goog.require('e2e.vrf.verify'); to launcher.js), Closure compiler throws a lot of errors.

the errors are cleared.

@adon-at-work
Copy link
Contributor Author

I'm done with the suggested changes.

The build was failed due to an upstream commit (happened roughly an hour ago):
google/closure-templates@e47f247

Tracking the issue at google/closure-templates#57

* @param {!e2e.ecc.Element} r the representative coordinate
* @return {!e2e.ecc.Element}
*/
function representativeToMontgomeryX(r) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put this in the e2e.vrf.extra25519 namespace

adon added 2 commits September 28, 2015 15:33
to leverage the faster new interfaces update() and digest()
@adon-at-work
Copy link
Contributor Author

@diracdeltas , all done. upgraded to use the latest js-sha3 too

@diracdeltas
Copy link
Contributor

Thanks, LGTM! @andres-erbsen any objections to merging?

diracdeltas added a commit that referenced this pull request Oct 2, 2015
Ported the vrf verification feature
@diracdeltas diracdeltas merged commit 8bf5dca into master Oct 2, 2015
@diracdeltas diracdeltas deleted the feature/vrf branch October 2, 2015 05:24
* @param {!e2e.ByteArray} h A byte array of length 32
* @return {!e2e.ecc.point.Ed25519}
*/
e2e.vrf.extra25519.hashToEdwards = function(h) {

Choose a reason for hiding this comment

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

Please document that this is not constant time (because variable-length bignums aren't)

@andres-erbsen
Copy link

No objections. I did add two one-line documentation nits about functions that are not constant-time-ish, though.

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

Successfully merging this pull request may close these issues.

4 participants