Skip to content
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

feat: implement ECDH #8

Merged
merged 29 commits into from
Aug 30, 2024
Merged

Conversation

YashBit
Copy link
Contributor

@YashBit YashBit commented Jul 30, 2024

Description

ECDH Implementation with NoirJS for writing tests.
See ecdh.tests.js in the package.

Here is a detailed description for a GitHub Pull Request (PR) that covers the provided code:


Summary

This PR introduces a set of functions and utilities for elliptic curve cryptography using the Baby JubJub curve. It includes functionality to convert byte arrays to field elements, generate public keys from private keys, and compute shared keys for the Elliptic Curve Diffie-Hellman (ECDH) protocol. The code also provides a main function for generating ECDH shared keys and includes initial placeholders for testing.

Details

  1. Function: field_from_bytes

    • Description: Converts a fixed-size byte array to a field element.
    • Arguments:
      • bytes: A fixed-size array of 32 bytes that represents the data to be converted.
      • big_endian: A boolean flag indicating if the byte array is in big-endian format. If true, the conversion uses big-endian ordering; otherwise, it uses little-endian ordering.
    • Returns: A Field element that represents the converted byte array.
    • Implementation:
      • Initializes the as_field and offset variables.
      • Iterates over the byte array, calculating the field element based on the byte values and their respective offsets.
      • Supports both big-endian and little-endian byte orders.
  2. Function: generate_public_key

    • Description: Generates a public key from a private key using the Baby JubJub curve.
    • Arguments:
      • private_key: A Field element representing the private key.
      • base_point: A Point on the Baby JubJub curve used as the base point for the generation of the public key.
    • Returns: A Point on the Baby JubJub curve representing the public key.
    • Implementation:
      • Creates an instance of the Baby JubJub curve with predefined parameters.
      • Computes the public key by performing scalar multiplication of the private key with the base point.
  3. Function: main

    • Description: Computes ECDH shared keys based on two private keys and validates their equality.
    • Arguments:
      • private_key1 and private_key2: Arrays of 32 bytes representing the private keys.
    • Implementation:
      • Defines a base point on the Baby JubJub curve.
      • Converts the private keys from byte arrays to Field elements.
      • Generates public keys for each private key.
      • Computes shared keys using the ECDH protocol.
      • Asserts that the computed shared keys are equal, validating the correctness of the ECDH operation.
  4. Tests

    • Description: Placeholder functions for testing.
    • Included Tests:
      • field_from_bytes_correct: Tests the field_from_bytes function to ensure correct conversion of a byte array to a field element.
    • Note: The test functions are currently commented out and need to be implemented with actual test cases and expected values.

Changes Made

  • Added utility functions for converting byte arrays to field elements and generating public keys.
  • Implemented the main function for computing ECDH shared keys and validating their equality.
  • Included placeholder test functions for field_from_bytes and generate_public_key functions.

To Do

  • Implement the actual test cases for the placeholder test functions.
  • Validate the correctness of the generate_public_key function with real inputs.
  • Ensure all functions and tests are correctly integrated and working as expected.

Yash Bharti added 9 commits July 26, 2024 14:14
ECDH Encryption completed with BabyJubJub
ds

BREAKING CHANGE: ds

re ds
New Improved Circuit, testing vanilla functions
Added Tests for ECDH in Noir, and also made some changes to the ecdh.tests.js. Also trying to test
the vanilla circuit.

re N
ds

BREAKING CHANGE: ds

re ds
@YashBit YashBit requested review from cedoor and sripwoud as code owners July 30, 2024 08:23
@cedoor cedoor requested a review from signorecello July 30, 2024 16:01
@sripwoud sripwoud added the feature 🚀 This is enhancing something existing or creating something new label Jul 31, 2024
Copy link
Member

@sripwoud sripwoud left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks for submitting this well described PR!
I'd like to wait for @signorecello to review cause I don't write noir myself.

In the meantime could you ensure the style ci check pass?

packages/ecdh/tests/ecdh.tests.ts Outdated Show resolved Hide resolved
.husky/commit-msg Outdated Show resolved Hide resolved
.husky/pre-commit Outdated Show resolved Hide resolved
.husky/prepare-commit-msg Outdated Show resolved Hide resolved
packages/ecdh/Nargo.toml Show resolved Hide resolved
packages/ecdh/target/ecdh.json Outdated Show resolved Hide resolved
@YashBit
Copy link
Contributor Author

YashBit commented Aug 5, 2024

@sripwoud Requested changes made.

@cedoor Will implemented the new curves you requested soon *

I would appreciate it if you could comment on the integration and the unit tests. Will add those as soon as * is completed.

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
packages/ecdh/tests/ecdh.tests.ts Outdated Show resolved Hide resolved
BREAKING CHANGE: k

re k
@cedoor
Copy link
Member

cedoor commented Aug 9, 2024

@YashBit

@cedoor Will implemented the new curves you requested soon *

👍🏽

I would appreciate it if you could comment on the integration and the unit tests. Will add those as soon as * is completed.

Re integration tests. I think there're not actual tests actually. You're just generating a proof without verifying it.

Looks like there's a function to verify proofs in the Noir tools: https://github.com/noir-lang/noir/blob/master/tooling/noir_js_backend_barretenberg/src/backend.ts#L127

l

re l
Copy link
Collaborator

@signorecello signorecello left a comment

Choose a reason for hiding this comment

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

Some suggestions and requests, great work @YashBit !

}

// ECDH Circuit
fn main(private_key1: [u8; 32], private_key2: [u8; 32]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit confusing that this is a lib but has a main function, can this be renamed? Something like generate_shared

Copy link
Member

Choose a reason for hiding this comment

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

I think here we need to have a main function and a package of type bin, don't we @signorecello?
Because we do want to compile something: we need the compilation artifact for the node test.

Or is there another way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean we shouldn't name libraries according to their use in tests... For the node test we would actually write a bin project and compile that one

Copy link
Collaborator

Choose a reason for hiding this comment

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

@YashBit saw you resolved this one, but idk what's your opinion

packages/ecdh/src/main.nr Outdated Show resolved Hide resolved
packages/ecdh/src/main.nr Outdated Show resolved Hide resolved
packages/ecdh/src/main.nr Outdated Show resolved Hide resolved
packages/ecdh/src/main.nr Outdated Show resolved Hide resolved
packages/ecdh/tests/ecdh.tests.ts Outdated Show resolved Hide resolved
Yash Bharti added 3 commits August 12, 2024 22:59
refactor - .toml, lib.nr and main.nr in ECDH
packages/ecdh/src/lib.nr Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
refactor: package.json
packages/ecdh/tests/ecdh.test.ts Outdated Show resolved Hide resolved
packages/ecdh/tests/ecdh.test.ts Outdated Show resolved Hide resolved
packages/ecdh/tests/ecdh.test.ts Outdated Show resolved Hide resolved
refactor: globals, lib, ecdh.test.ts
Yash Bharti added 3 commits August 26, 2024 19:05
refactor: ecdh.test.ts
refactor: .gitignore added target
Copy link
Member

@sripwoud sripwoud left a comment

Choose a reason for hiding this comment

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

tACK

ran locally: yarn test and yarn check

CI is broken (see #12).
capture: include mocha tests in ci see #13

packages/ecdh/tests/ecdh.test.ts Outdated Show resolved Hide resolved
packages/ecdh/tests/ecdh.test.ts Outdated Show resolved Hide resolved
@YashBit YashBit requested review from signorecello and jzaki August 26, 2024 15:02
Copy link
Collaborator

@signorecello signorecello left a comment

Choose a reason for hiding this comment

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

Just wanted to know your opinion on the naming, other than that it looks good to go

}

// ECDH Circuit
fn main(private_key1: [u8; 32], private_key2: [u8; 32]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean we shouldn't name libraries according to their use in tests... For the node test we would actually write a bin project and compile that one

}

// ECDH Circuit
fn main(private_key1: [u8; 32], private_key2: [u8; 32]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@YashBit saw you resolved this one, but idk what's your opinion

refactor: removed wait times in test.ts
package.json Outdated Show resolved Hide resolved
@sripwoud sripwoud mentioned this pull request Aug 26, 2024
@sripwoud sripwoud changed the title ECDH Implementation + NoirJS Based Testing in Noir (BabyJubJub) feat: implement ECDH Aug 26, 2024
git commit -m "refactor: package.json"
Copy link
Member

@sripwoud sripwoud left a comment

Choose a reason for hiding this comment

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

All my comments have be addressed. We now have a cleaner mocha/nargo tests setup. Local scripts are working.

But the CI is broken: the lockfile looks out of date.

@YashBit yarn.lock is out of date because you modified dependencies by editing manually package.json
It is better to use yarn add for that purpose as it will always update the yarn.lock accordingly.
(And I am sure you didn't use yarn add because the deps in package.json aren't sorted 😉)

Can you please bump the lockfile (trash it together with node_modules then re install deps, then push your changes)

EDIT: fixed in 116894c

refactor: yarn.lock, package.json
@sripwoud sripwoud mentioned this pull request Aug 27, 2024
@YashBit
Copy link
Contributor Author

YashBit commented Aug 27, 2024

Just wanted to know your opinion on the naming, other than that it looks good to go

@signorecello I think what I have done makes sense

We have a main.nr file, which is required for the nargo compile, and we have a separate lib.nr file which contains all the functions (which is in fact imported by main.nr)

Without a main.nr file + a pub fn main, there will no circuit and no integrated tests.

@YashBit YashBit requested a review from signorecello August 27, 2024 14:44
@cedoor
Copy link
Member

cedoor commented Aug 30, 2024

Let's merge this PR. Naming and other issues can be part of another PR in case.

#15

@cedoor cedoor merged commit 1f85ccc into privacy-scaling-explorations:main Aug 30, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🚀 This is enhancing something existing or creating something new
Projects
Status: ✔️ Done
Development

Successfully merging this pull request may close these issues.

5 participants