add script to compute precomputed curves, test with precomputed

[mvayngrib]

current master is forked from elliptic@6.4.1, renamed to @exodus/elliptic

the goal is to be able to sacrifice bundle size for performance by electing to use precomputed curves from the root project, e.g.:

const elliptic = require('elliptic');
elliptic.usePrecomputed({
  // require only the ones you need
  p256: require('elliptic/lib/elliptic/precomputed/p256'),
});

also, the validation in the PresetCurve is expensive and unnecessary to do at runtime for pre-defined curves

[ChALkeR]

Could there be a test that all precomputed curves match with non-precomputed ones?
Similar to precompute.js, but for verification only and automated during tests.

[kklash]

Agreed with @ChALkeR, that seems like a must-have opportunity for a unit test

[mvayngrib]

@kklash @ChALkeR see scripts for unit:precomputed. It runs the same tests with precomputed curves. You can also delete the contents of the precomputed/ dir and re-gen them with ./scripts/precompute.js and see that the git diff is empty

[mvayngrib]

Could there be a test that all precomputed curves match with non-precomputed ones?

@ChALkeR in case i misunderstood, in which way did you mean for them to match?

[kklash]

in case i misunderstood, in which way did you mean for them to match?

I imagine he means that if you run regular old elliptic with no precomputed curves, and generate a p256 curve, for example, that curve's JSON object should exactly match the precomputed p256.json object. I assume you could check using some kind of deep equality comparison function.

[mvayngrib]

@kklash ok, i'll see if i can add some kind of deep equality check to this test tomorrow: https://github.com/ExodusMovement/elliptic/pull/1/files#diff-eb6a8b288bf31a87c6e73e3e18055362R32

[mvayngrib]

@ChALkeR did i understand you correctly with this test? 83d7b7c

[mvayngrib]

@ChALkeR @kklash is anything still missing from the PR?

[mvayngrib]

increased timeout for test

can i remove support for node < 4 and the coveralls test so we don't waste time on them? Or do we want to setup coveralls here?

the failing build for reference: https://travis-ci.com/ExodusMovement/elliptic/builds/121961197

[ChALkeR]

The checks removed in options.skipValidation block should probably be extracted into a testcase, to make sure that all hardcoded curves do not fail that check. I don't think those are present in tests?
I.e.

assert(this.g.validate(), 'Invalid curve');
assert(this.g.mul(this.n).isInfinity(), 'Invalid curve, G*N != O');

---

As for older Node.js versions (0.10, 0.12) support -- those fail due to new syntax usage,
so we should either use older syntax to support those, or just drop those from support and test matrix.
I would suggest removing those, those reached EOL a long time ago.

[mvayngrib]

The checks removed in options.skipValidation block should probably be extracted into a testcase

good catch! pushed a test

[kklash]

utACK

[ChALkeR]

utACK.

[gutenye]

utACK

[Sekhmet]

tACK

Tested:

• generating curves using the script.

[unidwell]

TLDR: Using precomputations does not work for multiple reasons. But, ecliptic library works as before only without precomputations. Will review again when fixed.

Simple proof

Default case, precomputed is calculated on the fly. It displays true.

var ec = elliptic.ec('p256');
console.log('isPrecomputed:', ec.g._hasDoubles(new BN(1)));

usePrecomputed does not work; it displays false.

elliptic.usePrecomputed({
  p256: require('../lib/elliptic/precomputed/p256');
});

var ec = elliptic.ec('p256');
console.log('isPrecomputed:', ec.g._hasDoubles(new BN(1)));

Explanation

It does not work, because toJSON is not matched somewhere with fromJSON. We have:

curves.forEach(({ name, curve }) => {
  const precomputed = curve.g.toJSON()
  fs.writeFileSync(getCurvePath(name), prettify(precomputed))
})

The EC point's toJSON creates a transformed JSON. We were unlucky the following did not break when curves are being loaded:

names.forEach(function(name) {
  if (!curves[name].g.precomputed) {
    curves[name].g.precomputed = precomputed[name];
  }
});

Possible solution

We patch elliptic/curve/index.js EC constructor like this (new option fromJSON):

function EC(options, fromJSON) {
  if (!(this instanceof EC))
    return new EC(options, fromJSON);

  // Shortcut `elliptic.ec(curve-name)`
  if (typeof options === 'string') {
    assert(elliptic.curves.hasOwnProperty(options), 'Unknown curve ' + options);

    options = elliptic.curves[options];
  }

  // Shortcut for `elliptic.ec(elliptic.curves.curveName)`
  if (options instanceof elliptic.curves.PresetCurve)
    options = { curve: options };

  this.curve = options.curve.curve;
  this.n = this.curve.n;
  this.nh = this.n.ushrn(1);
  this.g = this.curve.g;

  // Point on curve
  this.g = options.curve.g;
  if (fromJSON) {
    this.g = options.curve.curve.pointFromJSON(fromJSON)
  }
  else {
    this.g.precompute(options.curve.n.bitLength() + 1);
  }

  // Hash for function for DRBG
  this.hash = options.hash || options.curve.hash;
}

This will make the following example work correctly. It displays true:

var ec = elliptic.ec('p256', require('../lib/elliptic/precomputed/p256'));
console.log('isPrecomputed:', ec.g._hasDoubles(new BN(1)));

If we need complete compatibility with existing libs, it should be possible to monkey patch EC, so that elliptic.ec('p256') returns elliptic.ec('p256', require('../lib/elliptic/precomputed/p256'))

Unanswered questions

• Currently only operations with EC point g are accelerated (the original library and our library). Not sure if that is necessary. A console.log in _hasDoubles can show, that for instance, multiplication could benefit two times, but precomputed is successfully used only once. Precomputations are probably still WIP in original lib.

[mvayngrib]

rebased on top of 6.4.1

[ChALkeR]

7cffa3b was ignored with the rebase?
Do we need those files in the package?
Upd: ah, only lib/ is packaged, sorry.

Another question -- hmac-drbg is moved to a separate module upstream, is that expected?
Just to make sure that this doesn't affect this PR.

[ChALkeR]

secp256k1 precomputed curve should also be compared with computed during tests.
It looks reproducible with a simple change like if (name === 'secp256k1') computed = computed[2], but that's probably not a good way there (see below).

Currently, modifying secp256k1 precomputed curve file in a certain way still keeps the tests passing, that should not be the case.

---

Also: why is it split into a different *.js file instead of a json one?

Just to filter it out from e.g. tests based on the filename? Perhaps explicitly giving it a different suffix or a separate directory would be a better way to indicate that it uses a different format from all the other ones, than filtering based on .js/.json filename difference.

secp256k1 is precomputed upstream

[ChALkeR]

Ok, I dismissed my prior review because, as @mvayngrib pointed out, secp256k1 is precomputed upstream and not introduced here. Thanks for explanation!

I do not have any specific concerns with the code anymore, but I would still recommend adding a test for secp256k1 precomputation upd: nevermind about that.

[mvayngrib]

@feri42 @roccomuso @unidwell do you guys want to take another look, or can we merge this?

[unidwell]

@mvayngrib Checked also changes since rebase, looks good.