feat: `GrumpkinScalar` type #1919

benesjan · 2023-08-31T14:48:36Z

Fixes #1912

Note 1: I removed the Signer interface as the difference in private key types between Grumpkin and secp256k1 made it impractical. Now we have a special type only for the "Grumpkin key" and the secp256k1 key is represented as either as a Buffer or as 0x${string} (in case of publisher private key).

Note 2: I changed some of the hardcoded private keys because they didn't fit to GrumpkinScalar and auto-reduction is no longer allowed.

Note 3: The way we get Grumpkin private key from mnemonic is insecure so I've created this issue for it.

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

yarn-project/aztec-cli/src/index.ts

yarn-project/aztec.js/src/account/entrypoint/single_key_account_entrypoint.ts

yarn-project/circuits.js/src/barretenberg/crypto/grumpkin/index.ts

yarn-project/foundation/src/fields/grumpkin_scalar.ts

yarn-project/noir-libs/noir-aztec/src/oracle/get_secret_key.nr

yarn-project/noir-libs/noir-aztec/src/types/grumpkin_scalar.nr

spalladino · 2023-09-06T11:32:00Z

yarn-project/noir-libs/noir-aztec/src/types/grumpkin_scalar.nr

+
+impl GrumpkinScalar {
+    fn new(high: Field, low: Field) -> Self {
+        GrumpkinScalar { high, low }


Should we assert that high is below a value such that high ++ low is below the grumpkin scalar modulus?

It would absolutely be a good idea to do a size check here. Do you know of some way how to do it without the sum overflowing the Field type and the high low values not being converted to a different type?

I did some research and it seems that the conversion to a larger type is the only feasible approach (but that's probably quite costly).

If not, please don't spend much time on this. I would ask in noir-questions how to do it the most efficiently.

spalladino · 2023-09-06T11:33:32Z

yarn-project/sequencer-client/src/config.ts

+  const publisherPrivateKey: `0x${string}` = `0x${
+    SEQ_PUBLISHER_PRIVATE_KEY
+      ? SEQ_PUBLISHER_PRIVATE_KEY.replace('0x', '')
+      : '0000000000000000000000000000000000000000000000000000000000000000'
+  }`;


Is this because we need to pad to 32 bytes? If so, let's create a padTo32ByteString function if we don't have one already, rather than doing this blindly. Otherwise, if the user is already passing in a 32-byte value as an env var, we'll be adding an extra 12 bytes without noticing.

It's not because of padding. What this does is simply check if the private key is defined and if not set 0x 0000000000000000000000000000000000000000000000000000000000000000 as the private key value (this is what we do on master as well). I changed the ethereum private key type to 0x${string} because there is no longer a more concrete type compatible with it.

yarn-project/aztec.js/src/account/entrypoint/schnorr_stored_key_account_entrypoint.ts

spalladino

LGTM!

PhilWindle

Looks great @benesjan thanks!

iAmMichaelConnor · 2023-09-07T20:58:19Z

@critesjosh pushed a fix to the sandbox-website repo to fix the broken private keys in the 'getting started' tutorial. I suspect it might have arisen as a result of this PR. We might need to also update the longer CLI getting started docs that live in this aztec-packages repo too?
Is there a way we could test whether such documentation breaks with new PRs?

critesjosh · 2023-09-08T00:36:39Z

@iAmMichaelConnor it was actually the default addresses that were incorrect, the private key did not have to be updated, which I thought was strange. We don't need to update the longer CLI guide in the docs because in there we have people create a new account and use that.

I think we could set it up so that the guides we use in docs are referencing code that is included in the CI. Basically we could write bash/TS/noir code in a test file that is in the CI, and if we need to reference code in a guide/tutorial then we link directly to the test file for that guide using the #include_code macro. This would ensure that the code is always working and would work as an alert when we need to update the text in a guide (eg. when a test is failing)

benesjan · 2023-09-08T07:25:26Z

I suspect it might have arisen as a result of this PR.

@iAmMichaelConnor sorry, it didn't come to my mind that it would break the website.

We might need to also update the longer CLI getting started docs that live in this aztec-packages repo too?

Will go through it now.

Is there a way we could test whether such documentation breaks with new PRs?

We could use your docs pre-processor approach for the website as well and make the commands from the website be extracted from a sh file which is tested in CI. We might need to move the sandbox website to the mono repo.

🤖 I have created a new Aztec Packages release --- ## [0.1.0-alpha63](v0.1.0-alpha62...v0.1.0-alpha63) (2023-09-08) ### Features * `GrumpkinScalar` type ([#1919](#1919)) ([3a9238a](3a9238a)) ### Bug Fixes * add retry to tag and docker actions ([#2099](#2099)) ([9f741f4](9f741f4)) * **breaking change:** change embedded curve scalar mul to use two limbs to properly encode the scalar field ([#2105](#2105)) ([070cc4c](070cc4c)) * broken bootstrap.sh after renaming `aztec-cli` dir as `cli` ([#2097](#2097)) ([2386781](2386781)) * browser test in canary flow ([#2102](#2102)) ([d52af6c](d52af6c)), closes [#2086](#2086) * check a note is read before nullifying it. ([#2076](#2076)) ([aabfb13](aabfb13)), closes [#1899](#1899) * circuits issues when building with gcc ([#2107](#2107)) ([4f5c4fe](4f5c4fe)) * COMMIT_TAG arg value in canary Dockerfile ([#2118](#2118)) ([a3d6459](a3d6459)) * dont assume safety of nvm ([#2079](#2079)) ([a4167e7](a4167e7)) * end-to-end aztec cli dependency issue ([#2092](#2092)) ([16ee3e5](16ee3e5)) * minor annoyances ([#2115](#2115)) ([a147582](a147582)) * padded printing for e2e-cli ([#2106](#2106)) ([5988014](5988014)) * remaining refs to clang15 ([#2077](#2077)) ([2c16547](2c16547)) * run e2e tests without spot ([#2081](#2081)) ([f0aa3ca](f0aa3ca)) * updated CLI readme ([#2098](#2098)) ([2226091](2226091)), closes [#1784](#1784) ### Miscellaneous * **circuits:** - remove dead code from cbind of private kernel circuit ([#2088](#2088)) ([43dc9d7](43dc9d7)) * **circuits:** remove dead code in cbind.cpp for public kernel ([#2094](#2094)) ([861f960](861f960)) * Conservatively raise the minimum supported clang version in CMakeList ([#2023](#2023)) ([f49c416](f49c416)) * **constants:** bump number of private reads and writes ([#2062](#2062)) ([ab6c6b1](ab6c6b1)) * **contracts:** Use autogenerated Noir interfaces where possible ([#2073](#2073)) ([bd6368b](bd6368b)), closes [#1604](#1604) * merge bb release-please ([#2080](#2080)) ([e89b043](e89b043)) * move storage into main.nr. ([#2068](#2068)) ([2c2d72b](2c2d72b)) * protogalaxy relations ([#1897](#1897)) ([35407e2](35407e2)) ### Documentation * **limitations:** limitations on ordering and logs of chopped notes ([#2085](#2085)) ([315ad3d](315ad3d)), closes [#1652](#1652) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

benesjan · 2023-09-08T11:55:06Z

The Cli getting started docs really are broken.

This other PR of mine will address that.

critesjosh · 2023-09-08T13:47:09Z

I just tried this guide with versions

➜ ~ aztec-cli --version
0.1.0-alpha63

and I didn't run into any errors.

EDIT: I see the issue is from creating an account from a private key. aztec-cli create-account --private-key 0x6622c828e9cd5adc86f10878765fe921d2b8cb2c79bdbc391157e43811ce88e3, which i can confirm does give an error, but is not a critical command to complete the tutorial, so should be fine until Jan's PR gets merged.

benesjan marked this pull request as draft August 31, 2023 14:48

benesjan force-pushed the janb/private-key-as-grumpkin-field branch from 2a4674b to e7cebfa Compare August 31, 2023 14:49

benesjan changed the title ~~refactor: representing priv key used with Grumpkin as Fr~~ refactor: representing private key used with Grumpkin as Fr Aug 31, 2023

benesjan force-pushed the janb/private-key-as-grumpkin-field branch from f19c909 to da8c8e5 Compare August 31, 2023 16:44

benesjan marked this pull request as ready for review August 31, 2023 17:23

kevaundray reviewed Sep 1, 2023

View reviewed changes

yarn-project/aztec-cli/src/index.ts Outdated Show resolved Hide resolved

ludamad force-pushed the master branch from e2c543c to 0036e07 Compare September 3, 2023 22:02

benesjan changed the title ~~refactor: representing private key used with Grumpkin as Fr~~ refactor: representing private key used with Grumpkin in its canonical form Sep 4, 2023

benesjan marked this pull request as draft September 4, 2023 14:20

benesjan changed the title ~~refactor: representing private key used with Grumpkin in its canonical form~~ feat: GrumpkinScalar type Sep 4, 2023

benesjan force-pushed the janb/private-key-as-grumpkin-field branch 3 times, most recently from 27a82e5 to 0b1e955 Compare September 5, 2023 12:51

benesjan added 5 commits September 6, 2023 07:20

feat: GrumpkinScalar

1d0f22d

typo fix

75aeb0d

WIP

c166e5b

fix

077b65d

typo fix to trigger full rebuild

562ae2d

benesjan force-pushed the janb/private-key-as-grumpkin-field branch from 204eefb to 562ae2d Compare September 6, 2023 07:23

benesjan added 5 commits September 6, 2023 08:11

feat: grumpkin_scalar.nr

6c3c743

feat: serialization of grumpkin scalar to 2 Fr elements

2967ab0

WIP

d0ccd96

better naming + linking issue

1e2d94a

test fix

8830d6d

benesjan marked this pull request as ready for review September 6, 2023 10:08

spalladino reviewed Sep 6, 2023

View reviewed changes

benesjan added 4 commits September 6, 2023 11:58

snake case

9c8c7b0

better description

324d193

reverting incorrectly introduced change

1159e07

camel case issue 2

a06534f

benesjan added 2 commits September 6, 2023 14:33

using sign closure to avoid code duplication

18d6287

removed redundant argument

5b696fd

spalladino approved these changes Sep 6, 2023

View reviewed changes

benesjan added 6 commits September 7, 2023 06:17

Merge branch 'master' into janb/private-key-as-grumpkin-field

4ea726b

WIP

cc23973

fixes

4521c85

fix

de5568a

typo fix to trigger full rebuild

abaf0df

attempt at getting more info

332f9e7

benesjan force-pushed the janb/private-key-as-grumpkin-field branch from 91c15da to 332f9e7 Compare September 7, 2023 08:04

PhilWindle approved these changes Sep 7, 2023

View reviewed changes

spypsy and others added 7 commits September 7, 2023 09:16

update e2e docker-compose sandbox image

20840e8

tag :latest in run_tests_local

431103c

revert img name

e398fe5

Attempt build fix

84515bb

Try again

941a9ce

Another attempt

fa2eaf3

Merge branch 'pw/fix-build-3' into janb/private-key-as-grumpkin-field

0ab32ee

PhilWindle merged commit 3a9238a into master Sep 7, 2023
84 checks passed

PhilWindle deleted the janb/private-key-as-grumpkin-field branch September 7, 2023 11:31

github-actions bot mentioned this pull request Sep 7, 2023

chore(master): release 0.1.0-alpha63 #2078

Merged

github-actions bot mentioned this pull request Sep 8, 2023

chore(master): Release 0.5.2 #2141

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `GrumpkinScalar` type #1919

feat: `GrumpkinScalar` type #1919

benesjan commented Aug 31, 2023 •

edited

Loading

spalladino Sep 6, 2023

benesjan Sep 6, 2023 •

edited

Loading

spalladino Sep 6, 2023

benesjan Sep 6, 2023 •

edited

Loading

spalladino left a comment

PhilWindle left a comment

iAmMichaelConnor commented Sep 7, 2023

critesjosh commented Sep 8, 2023

benesjan commented Sep 8, 2023 •

edited

Loading

benesjan commented Sep 8, 2023

critesjosh commented Sep 8, 2023 •

edited

Loading

feat: GrumpkinScalar type #1919

feat: GrumpkinScalar type #1919

Conversation

benesjan commented Aug 31, 2023 • edited Loading

Checklist:

spalladino Sep 6, 2023

Choose a reason for hiding this comment

benesjan Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

spalladino Sep 6, 2023

Choose a reason for hiding this comment

benesjan Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

spalladino left a comment

Choose a reason for hiding this comment

PhilWindle left a comment

Choose a reason for hiding this comment

iAmMichaelConnor commented Sep 7, 2023

critesjosh commented Sep 8, 2023

benesjan commented Sep 8, 2023 • edited Loading

benesjan commented Sep 8, 2023

critesjosh commented Sep 8, 2023 • edited Loading

feat: `GrumpkinScalar` type #1919

feat: `GrumpkinScalar` type #1919

benesjan commented Aug 31, 2023 •

edited

Loading

benesjan Sep 6, 2023 •

edited

Loading

benesjan Sep 6, 2023 •

edited

Loading

benesjan commented Sep 8, 2023 •

edited

Loading

critesjosh commented Sep 8, 2023 •

edited

Loading