feat: implement GroupNoble #38

sublimator · 2023-06-20T12:09:56Z

Continuing on with #26
Also deals with #30

Scripts

Test Type	All Groups	Specific Group (For performance while testing)
All Tests	`npm test`	`GROUP_CONS=noble npm test`
ESM Tests	`npm run test:esm`	`GROUP_CONS=noble npm run test:esm`
CJS Tests	`npm run test:cjs`	`GROUP_CONS=noble npm run test:cjs`

Notes

The Oprf.Group member is [re]configured by a describeGroupTests helper that takes a declare function and uses describe.each
- The library itself does not honour the env variable.
@noble/hashes and @noble/curves are listed as "optionalDependencies" (also "devDependencies" for tests) as not strictly required to use the library
Use Group.fromID factory instead of Group new constructor (eye to short vs ristretto/decaf448 etc)

src/index.ts

test/server.test.ts

package.json

src/groupNoble.ts

sublimator · 2023-06-20T23:15:10Z

Thanks! Will sort when on keyboard

src/groupNoble.ts

sublimator · 2023-06-24T02:50:35Z

@armfazh

I added some tsconfig.cjs.json files, and added them as a reference to the root tsconfig.json.
However it slows things down considerably during development.

I also note that this is declaring "type": "module" in the package.json, which, due to node module resolution means any consumers will have to be using ESM, regardless of any "exports" configuration:


(base) ➜  voprf-ts git:(nd-wip-2023-06-20) ✗ node
Welcome to Node.js v18.16.0.
Type ".help" for more information.
> require('.')
Uncaught:
Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/nicholasdudfield/projects/voprf-ts/lib/cjs/src/index.js not supported.
index.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.
Instead rename index.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in /Users/nicholasdudfield/projects/voprf-ts/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).

    at __node_internal_captureLargerStackTrace (node:internal/errors:490:5)
    at new NodeError (node:internal/errors:399:5)
    at Module._extensions..js (node:internal/modules/cjs/loader:1281:19)
    at Module.load (node:internal/modules/cjs/loader:1117:32)
    at Module._load (node:internal/modules/cjs/loader:958:12)
    at Module.require (node:internal/modules/cjs/loader:1141:19)
    at require (node:internal/modules/cjs/helpers:110:18) {
  code: 'ERR_REQUIRE_ESM'

I tried neutralising the package.json by removing "type":"module", however jest started failing as it didn't know to interpret .js files as ESM:

    Details:

    /Users/nicholasdudfield/projects/voprf-ts/lib/test/vectors.test.js:5
    import { derivePrivateKey, generatePublicKey, Oprf, OPRFClient, OPRFServer, POPRFClient, POPRFServer, VOPRFClient, VOPRFServer } from '../src/index.js';
    ^^^^^^

    SyntaxError: Cannot use import statement outside a module

I will try again later, but if you have any ideas?

Thinking out loud:
Perhaps there could be a separate cjs build step to create a cjs package.json (minus "type": "module"), compile all the code (including the test code) also as cjs, and a cjs specific test command ? Otherwise, there needs to be some way of configuring jest to run the tests without "type": "module" ?

sublimator · 2023-06-24T04:28:44Z

@armfazh

Ok, I've got something that seems to work. I removed "type" : "module" from the root package.json and committed lib/{esm,cjs}/package.json files setting their respective "type"s. At the moment the test/build commands simply build and test both cjs and esm at once (edit: added commands for faster specific tests). SCJL is the default group, and the tests can be configured to test noble by setting the environment variable GROUP_CONS to noble

Test Type	Default Group	Noble Group
All Tests	`npm test`	`GROUP_CONS=noble npm test`
ESM Tests	`npm run test:esm`	`GROUP_CONS=noble npm run test:esm`
CJS Tests	`npm run test:cjs`	`GROUP_CONS=noble npm run test:cjs`

sublimator · 2023-06-29T04:26:52Z

@armfazh @lukevalenta

?

armfazh

Hi, here are some suggested changes.

package.json

src/groupNoble.ts

armfazh · 2023-07-19T08:15:19Z

src/groupNoble.ts

+        compat(this, k2)
+        compat(this, a)
+        const el = this.p.multiplyAndAddUnsafe(a.p, k1.k, k2.k)
+        if (!el) throw new Error('result is zero')


it shouldn't make a special case returning the ZERO point.

For example this is valid call to this function:

P = gen() Q = P a = order-1 b = 1 R = a*P+b*Q = (order-1)*P + P = -P+P = 0

/** * Efficiently calculate `aP + bQ`. Unsafe, can expose private key, if used incorrectly. * Not using Strauss-Shamir trick: precomputation tables are faster. * The trick could be useful if both P and Q are not G (not in our case). * @returns non-zero affine point */ multiplyAndAddUnsafe(Q: Point, a: bigint, b: bigint): Point | undefined { const G = Point.BASE; // No Strauss-Shamir trick: we have 10% faster G precomputes const mul = ( P: Point, a: bigint // Select faster multiply() method ) => (a === _0n || a === _1n || !P.equals(G) ? P.multiplyUnsafe(a) : P.multiply(a)); const sum = mul(this, a).add(mul(Q, b)); return sum.is0() ? undefined : sum; }

It seems multiplyAndAddUnsafe returns a non zero Point

This one is probably better fielded by @paulmillr

For the moment I'm just returning a zero point when multiplyAndAddUnsafe returns undefined (the point was zero)

Any better ideas ?

it seems to me that this need to be solved in the noble package.

The workaround should be OK to get things moving though, no?

src/groupNoble.ts

armfazh · 2023-07-19T08:21:23Z

src/groupNoble.ts

+        return EltNb.gen(this).mul(s)
+    }
+
+    async randomScalar(): Promise<ScalarNb> {


no need to use async as no await is in the body of this function.

Suggested change

async randomScalar(): Promise<ScalarNb> {

randomScalar(): Promise<ScalarNb> {

same applies to the other methods

Some linters actually enforce consistent use of async, which is something I can get behind, but if that's the convention here, I can go with it. Maybe there's a linting rule this project could set up ?

async randomScalar(): Promise<ScalarNb> { const msg = crypto.getRandomValues(new Uint8Array(this.size)) return ScalarNb.hash(this, msg, new Uint8Array()) }

Actually, looking at this more, I can see that the async modifier is what makes the simple return value upgrade to a Promise, rather than having to use Promise.resolve(value)

Our Group interfaces are async (using Promises), so the async is actually required?

until there is a linter rule enabled to get this uniformly, I consider that using async only when inside the body of the function there is an await keyword.

A function is known to be async iff it returns a Promise.

A function is known to be async iff it returns a Promise.

Let's dig into this a bit more.

The randomScalar method of our Group interface is asynchronous so that it can support the sjcl/webcrypto (used for expandMD) implementation.

scalarNb.hash(this, msg, new Uint8Array())

This is a sync operation, and not a promise, it's true, but by putting async on the function it "upgrades" the return value to a Promise so it will conform to our interface.

The alternative here is to use Promise.resolve(), which I will do:

randomScalar(): Promise<ScalarNb> { const msg = crypto.getRandomValues(new Uint8Array(this.size)) return Promise.resolve(ScalarNb.hash(this, msg, new Uint8Array())) }

src/sjcl/index.js

test/dleq.test.ts

test/setGroup.ts

.github/workflows/node.yml

sublimator · 2023-07-19T20:23:47Z

I found it helpful to be able to target a specific group for performance sake. These tests can be quite slow. Maybe there is a middle ground here. Am currently on a trip(and on mobile), though back soon. Thanks.

sublimator · 2023-07-21T05:21:42Z

@armfazh

I've addressed your concerns/comments. I've left the for static method for now, in
anticipation of separate Group/Elt/Scalar classes used by decaf/ristretto.

Thanks!

src/groupNoble.ts

armfazh · 2023-08-31T23:24:09Z

src/groupNoble.ts

+        return EltNb.gen(this).mul(s)
+    }
+
+    async randomScalar(): Promise<ScalarNb> {


until there is a linter rule enabled to get this uniformly, I consider that using async only when inside the body of the function there is an await keyword.

A function is known to be async iff it returns a Promise.

armfazh · 2023-08-31T23:24:54Z

src/groupNoble.ts

+    eltDes: Deserializer<EltNb> = {
+        size: (compressed?: boolean) => {
+            return this.eltSize(compressed)
+        },
+        deserialize: (b: Uint8Array) => {
+            return this.desElt(b)
+        }
+    }
+    scalarDes: Deserializer<ScalarNb> = {
+        size: () => {
+            return this.scalarSize()
+        },
+        deserialize: (b: Uint8Array) => {
+            return this.desScalar(b)
+        }
+    }


src/groupSjcl.ts

src/groupTypes.ts

armfazh · 2023-09-01T01:48:10Z

@sublimator please rebase and squash your commits, so it can be merged.

sublimator · 2023-09-01T01:53:49Z

I got conflicts when I tried to rebase, so I just merged origin/main, so it should squash down from the Github UI easily.

Only merge commits allowed?

sublimator · 2023-09-01T02:02:46Z

Ok, I created a new commit on top of main from the contents of the old commits and force pushed.

armfazh · 2023-09-02T00:04:41Z

Thank for your contributions @sublimator

sublimator · 2023-09-02T00:11:11Z

Wooo! Thanks

…

On Sat, 2 Sep 2023 at 7:04 AM Armando Faz ***@***.***> wrote: Thank for your contributions @sublimator <https://github.com/sublimator> — Reply to this email directly, view it on GitHub <#38 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEAHG64XUJ476C6I6PP34LXYJZ2JANCNFSM6AAAAAAZNF5HXI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

sublimator commented Jun 20, 2023

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

sublimator commented Jun 20, 2023

View reviewed changes

test/server.test.ts Outdated Show resolved Hide resolved

sublimator commented Jun 20, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

armfazh reviewed Jun 20, 2023

View reviewed changes

src/groupNoble.ts Outdated Show resolved Hide resolved

src/groupNoble.ts Outdated Show resolved Hide resolved

src/groupNoble.ts Outdated Show resolved Hide resolved

src/groupNoble.ts Outdated Show resolved Hide resolved

armfazh reviewed Jun 22, 2023

View reviewed changes

src/groupNoble.ts Outdated Show resolved Hide resolved

sublimator marked this pull request as ready for review June 26, 2023 23:55

armfazh requested changes Jul 19, 2023

View reviewed changes

armfazh requested changes Aug 31, 2023

View reviewed changes

feat: implement GroupNoble

99e19c1

sublimator force-pushed the nd-wip-2023-06-20 branch from 7ccccb2 to 99e19c1 Compare September 1, 2023 02:01

feat: readonly members

155e3e3

armfazh approved these changes Sep 2, 2023

View reviewed changes

armfazh merged commit 44f96ab into cloudflare:main Sep 2, 2023
3 checks passed

armfazh mentioned this pull request Sep 2, 2023

Switch to @noble/curves #25

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement GroupNoble #38

feat: implement GroupNoble #38

sublimator commented Jun 20, 2023 •

edited

Loading

sublimator commented Jun 20, 2023 via email

sublimator commented Jun 24, 2023

sublimator commented Jun 24, 2023 •

edited

Loading

sublimator commented Jun 29, 2023

armfazh left a comment

armfazh Jul 19, 2023

sublimator Jul 21, 2023

sublimator Jul 21, 2023

armfazh Sep 1, 2023

sublimator Sep 1, 2023

armfazh Jul 19, 2023

sublimator Jul 21, 2023

sublimator Jul 21, 2023

armfazh Aug 31, 2023

sublimator Sep 1, 2023

sublimator commented Jul 19, 2023 via email

sublimator commented Jul 21, 2023

armfazh Aug 31, 2023

armfazh Aug 31, 2023

armfazh commented Sep 1, 2023

sublimator commented Sep 1, 2023

sublimator commented Sep 1, 2023

armfazh commented Sep 2, 2023

sublimator commented Sep 2, 2023 via email

	async randomScalar(): Promise<ScalarNb> {
	randomScalar(): Promise<ScalarNb> {

feat: implement GroupNoble #38

feat: implement GroupNoble #38

Conversation

sublimator commented Jun 20, 2023 • edited Loading

Scripts

Notes

sublimator commented Jun 20, 2023 via email

sublimator commented Jun 24, 2023

sublimator commented Jun 24, 2023 • edited Loading

sublimator commented Jun 29, 2023

armfazh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sublimator commented Jul 19, 2023 via email

sublimator commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armfazh commented Sep 1, 2023

sublimator commented Sep 1, 2023

sublimator commented Sep 1, 2023

armfazh commented Sep 2, 2023

sublimator commented Sep 2, 2023 via email

sublimator commented Jun 20, 2023 •

edited

Loading

sublimator commented Jun 24, 2023 •

edited

Loading