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: constraining note owner #1896

Merged
merged 12 commits into from
Sep 15, 2023
Merged

feat: constraining note owner #1896

merged 12 commits into from
Sep 15, 2023

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Aug 30, 2023

Fixes #1817

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

  • If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
  • I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
  • Every change is related to the PR description.
  • I have linked this pull request to relevant issues (if any exist).

@benesjan benesjan marked this pull request as draft August 30, 2023 15:12
@benesjan benesjan force-pushed the janb/constraining-note-owner branch 2 times, most recently from 2d88bf2 to 9d3329c Compare August 31, 2023 11:43
@benesjan benesjan force-pushed the janb/constraining-note-owner branch 4 times, most recently from 689d8e8 to 1d1d5de Compare September 13, 2023 14:08
@benesjan benesjan marked this pull request as ready for review September 13, 2023 16:49
@benesjan benesjan requested a review from spalladino September 14, 2023 14:10
Copy link
Collaborator

@spalladino spalladino 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 @benesjan, but I'd move the constrain to within the get_secret_key call. Not only to remove code duplication, but I think it's good practice if we can expose to the user functions that already constrain their result (like we're doing with get_public_key) so they don't have to it themselves and risk forgetting it.

That aside, I also wanted to ask: why flipping secret's high and low in this PR? I believe we usually go big-endian, which would mean serialising high first, right?

@benesjan
Copy link
Contributor Author

Thank you for the review @spalladino .

I'd move the constrain to within the get_secret_key call.

Great idea. Will address.

why flipping secret's high and low in this PR? I believe we usually go big-endian, which would mean serialising high first, right?

I initially followed the big endian but then @kevaundray flipped it here so I flipped it in aztec-packages as well so that it's consistent. I guess in crypto they are used to having it like that.

@kevaundray
Copy link
Contributor

kevaundray commented Sep 14, 2023

Thank you for the review @spalladino .

I'd move the constrain to within the get_secret_key call.

Great idea. Will address.

why flipping secret's high and low in this PR? I believe we usually go big-endian, which would mean serialising high first, right?

I initially followed the big endian but then @kevaundray flipped it here so I flipped it in aztec-packages as well so that it's consistent. I guess in crypto they are used to having it like that.

I can open up an issue to make barretenberg use little endian everywhere, because I believe in some places it may use little endian and in others it will use big endian (There might be reason for this -- haven't checked)

@benesjan benesjan force-pushed the janb/constraining-note-owner branch 2 times, most recently from 42bed94 to ec36e8e Compare September 14, 2023 15:14
@benesjan benesjan requested a review from spalladino September 14, 2023 15:41
unconstrained fn get_secret_key(owner: Field) -> dep::std::grumpkin_scalar::GrumpkinScalar {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Heads up that all this logic is within an unconstrained function, meaning the assertions will not be enforced by the circuit (I'm wondering if this should be a compile error?). You probably want an unconstrained function that just wraps the oracle call, and then a non-unconstrained one that calls get_public_key, computes it from the secret, and runs the assertion (like in get_public_key.nr).

@benesjan benesjan force-pushed the janb/constraining-note-owner branch from 99d218d to 805d9fc Compare September 14, 2023 15:57
@benesjan benesjan force-pushed the janb/constraining-note-owner branch from 8ff6e57 to 389787c Compare September 14, 2023 19:25
@benesjan benesjan force-pushed the janb/constraining-note-owner branch from 389787c to 9e2e8b9 Compare September 14, 2023 21:23
@benesjan benesjan enabled auto-merge (squash) September 14, 2023 21:24
@benesjan benesjan merged commit cb25bc9 into master Sep 15, 2023
2 checks passed
@benesjan benesjan deleted the janb/constraining-note-owner branch September 15, 2023 08:51
PhilWindle pushed a commit that referenced this pull request Sep 15, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.7.3</summary>

##
[0.7.3](aztec-packages-v0.7.2...aztec-packages-v0.7.3)
(2023-09-15)


### Features

* Constraining note owner
([#1896](#1896))
([cb25bc9](cb25bc9)),
closes
[#1817](#1817)


### Bug Fixes

* **build:** Navigate to correct directory for publishing
([#2318](#2318))
([f555356](f555356))
* Use bool for set_minter
([#2313](#2313))
([5b18f9e](5b18f9e))
</details>

<details><summary>barretenberg.js: 0.7.3</summary>

##
[0.7.3](barretenberg.js-v0.7.2...barretenberg.js-v0.7.3)
(2023-09-15)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>barretenberg: 0.7.3</summary>

##
[0.7.3](barretenberg-v0.7.2...barretenberg-v0.7.3)
(2023-09-15)


### Miscellaneous

* **barretenberg:** Synchronize aztec-packages versions
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Constrain the secret key against the owner, when computing a nullifier.
3 participants