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

Move public key check code from dependency. #69

Closed
wants to merge 4 commits into from

Conversation

BlakeMScurr
Copy link
Collaborator

@BlakeMScurr BlakeMScurr commented Dec 22, 2022

In order to verify an ECDSA signature you have to validate the public key. Usually this is done outside the circuit to save computation. However, we need to prove membership in arbitrary lists of addresses, where the public keys may not be available to be checked. This means we have to check the public key inside the circuit.

We made a PR to our dependency, but it's unlikely to get merged. So instead we have moved the logic into this repo.

Copy link
Collaborator

@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.

Looks good after addressing linting/formatting warning/errors.

Is this PR chained on #68 ?
In which order should they be merged?
Doesn't #69 need to be rebased on #68?

circuit = await wasm_tester(join(__dirname, "test_ecdsa_check_pub_key.circom"));
});

var test_ecdsa_verify = function (test_case: [bigint, bigint]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check/fix linting errors

@coveralls
Copy link
Collaborator

Coverage Status

Coverage: 88.525%. Remained the same when pulling 1721bfb on BlakeMScurr:public-key-check into fbf5707 on privacy-scaling-explorations:main.

@BlakeMScurr
Copy link
Collaborator Author

Looks good after addressing linting/formatting warning/errors.

Is this PR chained on #68 ? In which order should they be merged? Doesn't #69 need to be rebased on #68?

Well it's kind of independent, they can be chained in either direction. So I just merged main into this branch once #68 was merged into master. I'll be sure to clarify chaining between PRs in the description in the future!

Unfortunately I have to close this because Snyk always fails for PRs from forks.

@BlakeMScurr BlakeMScurr closed this Jan 9, 2023
@BlakeMScurr BlakeMScurr deleted the public-key-check branch January 9, 2023 22:12
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.

3 participants