Skip to content

Separate get scalar function #183

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

Merged
merged 1 commit into from
Apr 1, 2025

Conversation

kigawas
Copy link
Contributor

@kigawas kigawas commented Mar 30, 2025

Separate get scalar logic for getExtendedPublicKey

@paulmillr
Copy link
Owner

Could you clarify your use case?

@kigawas
Copy link
Contributor Author

kigawas commented Mar 31, 2025

One call of point.toRawBytes can be reduced for fromPrivateKey

@paulmillr
Copy link
Owner

@kigawas what do you mean?

@paulmillr
Copy link
Owner

How much of a performance gain are we talking about? A comparison before/after would be useful. If it's negligible then we're optimizing the wrong part.

@kigawas
Copy link
Contributor Author

kigawas commented Mar 31, 2025

@paulmillr Around 2% performance improvement, and it reduces memory footprints as well (since we can save a call to toAffineMemo.

I ran with (npm run build:clean || true) && npm run build && npm run bench:install && node benchmark/curves.js:

Before (main branch)

$ (npm run build:clean || true) && npm run build && npm run bench:install && node benchmark/curves.js

> @noble/[email protected] build:clean
> rm {.,esm,abstract,esm/abstract}/*.{js,d.ts,d.ts.map,js.map} 2> /dev/null


> @noble/[email protected] build
> tsc && tsc -p tsconfig.cjs.json


> @noble/[email protected] bench:install
> cd benchmark; npm install; npm install .. --install-links

...

ed25519
init 18ms
getPublicKey x 10,207 ops/sec @ 97μs/op ± 1.13% (82μs..1ms)
sign x 5,104 ops/sec @ 195μs/op
verify x 1,142 ops/sec @ 875μs/op

ed448
init 40ms
getPublicKey x 4,494 ops/sec @ 222μs/op
sign x 2,128 ops/sec @ 469μs/op
verify x 465 ops/sec @ 2ms/op
...

After

$ (npm run build:clean || true) && npm run build && npm run bench:install && node benchmark/curves.js

> @noble/[email protected] build:clean
> rm {.,esm,abstract,esm/abstract}/*.{js,d.ts,d.ts.map,js.map} 2> /dev/null


> @noble/[email protected] build
> tsc && tsc -p tsconfig.cjs.json


> @noble/[email protected] bench:install
> cd benchmark; npm install; npm install .. --install-links

...

ed25519
init 18ms
getPublicKey x 10,599 ops/sec @ 94μs/op ± 1.66% (78μs..3ms)
sign x 5,189 ops/sec @ 192μs/op
verify x 1,096 ops/sec @ 911μs/op ± 3.45% (773μs..6ms)

ed448
init 39ms
getPublicKey x 4,616 ops/sec @ 216μs/op
sign x 2,167 ops/sec @ 461μs/op
verify x 459 ops/sec @ 2ms/op

...

After (another try)

ed25519
init 18ms
getPublicKey x 10,360 ops/sec @ 96μs/op ± 1.07% (81μs..1ms)
sign x 5,203 ops/sec @ 192μs/op
verify x 1,147 ops/sec @ 871μs/op

ed448
init 39ms
getPublicKey x 4,557 ops/sec @ 219μs/op
sign x 2,162 ops/sec @ 462μs/op
verify x 442 ops/sec @ 2ms/op ± 1.85% (2ms..9ms)

@@ -537,6 +545,7 @@ export function twistedEdwards(curveDef: CurveType): CurveFn {
G._setWindowSize(8); // Enable precomputes. Slows down first publicKey computation by 20ms.

const utils = {
getPrivateScalar,
Copy link
Owner

Choose a reason for hiding this comment

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

Is it necessary to expose it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question, I'd expose it for two reasons:

  1. In rust (ed25519-dalek) and go (standard library), user can get the exposed Scalar
  2. Currently we are already exposing scalar through getExtendedPublicKey, with some extra calls

Copy link
Owner

Choose a reason for hiding this comment

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

I've decided to not expose it for now, because the lib needs to have feature parity with 4kb noble-ed25519. fromPrivateKey is still in though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. You can easily update it here when you're ready

https://github.com/paulmillr/noble-ed25519/blob/main/index.ts#L245

Copy link
Owner

Choose a reason for hiding this comment

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

Not really.

ed25519 does not support synchronous hashing out-of-box because it requires no deps. Curves, on the other hand, use noble-hashes.

Because of that, ed25519 provides two implementations: async (using built-in crypto.subtle) and sync (which requires connecting to hashing library first).

So utils will need TWO functions, not one. And it's already a mess with 2 getExtPubkey, 2 getPubkey, 2 sign, 2 verify, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we may need a better abstraction for the sync and async crypto just like @noble/ciphers/webcrypto

Copy link
Owner

Choose a reason for hiding this comment

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

yeah

@paulmillr paulmillr merged commit 6087734 into paulmillr:main Apr 1, 2025
7 checks passed
@kigawas kigawas deleted the edwards/split-get-private-scalar branch April 1, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants