-
Notifications
You must be signed in to change notification settings - Fork 44
Noble's crypto BLS #663
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
base: feat/next
Are you sure you want to change the base?
Noble's crypto BLS #663
Conversation
| @@ -0,0 +1,1027 @@ | |||
| import { FpLegendre, isNegativeLE } from "@noble/curves/abstract/modular"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the translation within a test file, to be refactored & moved to production logic when appropriate.
| this.timeout(100000); | ||
|
|
||
| const testdataPath = path.resolve(__dirname, "..", "testdata"); | ||
| const filePath = path.resolve(testdataPath, "blsVectors.json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSON file containing the test vectors is currently attached to this PR. We should find a good place for it (it's a bit large).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few of the "inline" test vectors (explicit, in tests code, not in the JSON file) were took from:
| assert.equal(toHex(actualSignature.bytes), signature); | ||
| assertG1PointsAreEqual(actualSignature.point, signatureAsPoint); | ||
|
|
||
| assert.isTrue(verified); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve the precision of the tests, we also check the points coordinates.
| }); | ||
| }); | ||
|
|
||
| function setupG2GeneratorPointsLikeHerumi() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary, since we use a custom generator for BLS public keys.
package.json
Outdated
| "ts-node": "9.1.1", | ||
| "typescript": "4.1.2" | ||
| "typescript": "5.6.2", | ||
| "@noble/curves": "1.6.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only as "dev-dependency" for now (no change of production code).
By the way, a lot happened since multiversx/mx-deprecated-sdk-js-wallet#51 (on top of v1.6.0):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a translation of BLS-related components from Herumi (MCL) to Noble Curves for compatibility testing. It implements BLS cryptographic operations using Noble Curves library as a test implementation without affecting production code.
- Adds comprehensive test suite for BLS compatibility between Noble Curves and Herumi implementations
- Implements BLS signature operations including signing, verification, and point mapping
- Adds Noble Curves dependency for cryptographic operations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/wallet/blcCompatibility.spec.ts | Complete test suite implementing BLS operations using Noble Curves with Herumi-compatible encoding |
| package.json | Adds Noble Curves dependency and updates TypeScript version |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/wallet/blcCompatibility.spec.ts
Outdated
| const numVectors = 256; | ||
|
|
||
| for (let i = 0; i < numVectors; i++) { | ||
| console.log(`Running test vector ${i++}`); |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The post-increment operator i++ in the console.log statement increments i twice per iteration (once in the for loop, once in console.log), causing the loop to skip test vectors and potentially access undefined array elements.
| console.log(`Running test vector ${i++}`); | |
| console.log(`Running test vector ${i}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
package.json
Outdated
| "typescript": "5.6.2", | ||
| "@noble/curves": "1.6.0" |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @noble/curves dependency should be in devDependencies instead of being placed alongside devDependencies like typescript. The formatting is also inconsistent - it should be on a separate line with proper indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad comment. Also, not an issue for now.
Merge feat/next in main Add validators operations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/wallet/blcCompatibility.spec.ts
Outdated
| // *x.getFp0() -= Fp::one(); | ||
|
|
||
| x = Fp.neg(x); | ||
| x -= Fp.ONE; |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direct arithmetic operation on field element. Line 824 should use x = Fp.sub(x, Fp.ONE) instead of x -= Fp.ONE to maintain field arithmetic consistency.
| x -= Fp.ONE; | |
| x = Fp.sub(x, Fp.ONE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operation was on scalars. But, I am really impressed, decent comment, applied!
src/wallet/blcCompatibility.spec.ts
Outdated
| // *x.getFp0() += c2_; | ||
| let x = Fp.mul(t, w); | ||
| x = Fp.neg(x); | ||
| x += c2; |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direct arithmetic operation on field element. Should use x = Fp.add(x, c2) instead of x += c2 to maintain field arithmetic consistency.
| x += c2; | |
| x = Fp.add(x, c2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operation was on scalars. But, I am really impressed, decent comment, applied!
src/wallet/blcCompatibility.spec.ts
Outdated
|
|
||
| let x = Fp.sqr(w); | ||
| x = Fp.inv(x); | ||
| x += Fp.ONE; |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direct arithmetic operation on field element. Should use x = Fp.add(x, Fp.ONE) instead of x += Fp.ONE to maintain field arithmetic consistency.
| x += Fp.ONE; | |
| x = Fp.add(x, Fp.ONE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operation was on scalars. But, I am really impressed, decent comment, applied!
src/wallet/blcCompatibility.spec.ts
Outdated
| let w = Fp.sqr(t); | ||
|
|
||
| // Herumi code: w += G::b_; | ||
| w += G1.CURVE.b; |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direct arithmetic operation on field element. Should use w = Fp.add(w, G1.CURVE.b) instead of w += G1.CURVE.b to maintain field arithmetic consistency.
| w += G1.CURVE.b; | |
| w = Fp.add(w, G1.CURVE.b); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, same comment as above! 🍪
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/wallet/blcCompatibility.spec.ts
Outdated
|
|
||
| const point = G1.ProjectivePoint.fromHex(bytesReversed); | ||
| const isYOdd = Fp.isOdd!(point.py); | ||
| const yNegated = Fp.neg(point.y); |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an inconsistency in accessing y-coordinate properties: point.py is used on line 933 while point.y is used on line 934. These should consistently use the same property accessor.
| const yNegated = Fp.neg(point.y); | |
| const yNegated = Fp.neg(point.py); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decent suggestion, thanks, applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| // Signs a message, given a secret key. | ||
| export function signMessage(message: Uint8Array, secretKey: Uint8Array): { point: any; bytes: Uint8Array } { |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type uses any for the point property, which reduces type safety. Consider defining a proper interface or using the specific Noble Curves ProjectivePoint type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These kind of issues will be resolved once we upgrade to a more recent (the most recent) noble curves. Cannot be done at the moment (quite a few breaking changes).
package.json
Outdated
| "typescript": "5.6.2", | ||
| "@noble/curves": "1.6.0" |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Noble Curves dependency should be moved from devDependencies to dependencies since it's used in production code (imported in the adapter module).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapter module isn't properly advertised (should not be used by client code yet).
… incomplete before).
…od for our need. Thus, replaced with library call.
…becomes deprecated (and seems to break the tests) in Noble Curves v1.9.3.
Re-did:
Also see
Here, we did a translation of BLS-related components of Herumi (MCL) into Noble Curves components. We did this as a test - no production code was touched yet.
Test vectors: blsVectors.json - download and copy to
src/testdata.In September 2024, we've worked on top of
noble-curves v1.6.0. In the meantime, there were released many new versions. We've only managed to upgrade tov1.9.2so far. The rest will be handled in a separate PR.