-
Notifications
You must be signed in to change notification settings - Fork 265
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: add unconstrained getters to sharedmutable #7429
feat: add unconstrained getters to sharedmutable #7429
Conversation
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Proof generationEach column represents the number of threads used in proof generation.
L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 8 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
AVM SimulationTime to simulate various public functions in the AVM.
Public DB AccessTime to access various public DBs.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method | Metric | | |
9b06af6
to
acf5412
Compare
acf5412
to
15cb689
Compare
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.
In order to properly close #7326, shouldn't we also update the private token so that it can perform the balance read in an unconstrained context instead of privately? I think that'll require that we also add these fns to the SharedMutablePrivateGetter, since iirc the issue had to do with inspection of the key registry.
pub fn get_current_value_in_unconstrained(self) -> T { | ||
let block_number = self.context.block_number() as u32; | ||
self.read_value_change().get_current_at(block_number) | ||
} | ||
|
||
pub fn get_current_delay_in_unconstrained(self) -> u32 { | ||
let block_number = self.context.block_number() as u32; | ||
self.read_delay_change().get_current(block_number) | ||
} | ||
|
||
pub fn get_scheduled_value_in_unconstrained(self) -> (T, u32) { | ||
self.read_value_change().get_scheduled() | ||
} | ||
|
||
pub fn get_scheduled_delay_in_unconstrained(self) -> (u32, u32) { | ||
self.read_delay_change().get_scheduled() | ||
} | ||
|
||
fn read_value_change(self) -> ScheduledValueChange<T> { | ||
self.context.storage_read(self.get_value_change_storage_slot()) | ||
} | ||
|
||
fn read_delay_change(self) -> ScheduledDelayChange<INITIAL_DELAY> { | ||
self.context.storage_read(self.get_delay_change_storage_slot()) | ||
} | ||
} |
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 fns should be unconstrained
, both to flag that the execution is not constrained, and to avoid issues that might arise if we end up passing mut& here.
In the future this will be more obvious because noir-lang/noir#4442 would require that you place this in an unsafe
block, and you'd go 'oh i don't actually want to do that'.
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.
Good shout, thanks for the heads up !
pub fn get_current_value_in_unconstrained(self) -> T { | ||
let block_number = self.context.block_number() as u32; | ||
self.read_value_change().get_current_at(block_number) | ||
} | ||
|
||
pub fn get_current_delay_in_unconstrained(self) -> u32 { | ||
let block_number = self.context.block_number() as u32; | ||
self.read_delay_change().get_current(block_number) | ||
} | ||
|
||
pub fn get_scheduled_value_in_unconstrained(self) -> (T, u32) { | ||
self.read_value_change().get_scheduled() | ||
} | ||
|
||
pub fn get_scheduled_delay_in_unconstrained(self) -> (u32, u32) { | ||
self.read_delay_change().get_scheduled() | ||
} | ||
|
||
fn read_value_change(self) -> ScheduledValueChange<T> { | ||
self.context.storage_read(self.get_value_change_storage_slot()) | ||
} | ||
|
||
fn read_delay_change(self) -> ScheduledDelayChange<INITIAL_DELAY> { | ||
self.context.storage_read(self.get_delay_change_storage_slot()) | ||
} |
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.
I don't think we have proper support in txe to create an unconstrained context and therefore test these that way, but that might make sense.
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.
Yep when I first did this, I looked at adding txe tests, but it seems like it doesn't support unconstrained yet. I'll add another issue for adding unconstrained support to the TXE.
15cb689
to
f64d495
Compare
886dd82
to
f6ab485
Compare
f64d495
to
279a5d1
Compare
f6ab485
to
53d7abe
Compare
279a5d1
to
027d99d
Compare
53d7abe
to
71f7739
Compare
0378827
to
7471bf1
Compare
@nventuro I plan on nuking basically all of private token and replace it with the normal token and just extend it by the 2 special refund functions so it doesn't make sense to invest time in it. |
0ab3f4d
to
7a31314
Compare
7471bf1
to
77b0685
Compare
noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable.nr
Show resolved
Hide resolved
f70bcaf
to
2deb1bb
Compare
noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/test.nr
Outdated
Show resolved
Hide resolved
556fe66
to
e6fedda
Compare
e6fedda
to
9a2fb8b
Compare
noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable.nr
Outdated
Show resolved
Hide resolved
…red_mutable.nr Co-authored-by: Nicolás Venturo <[email protected]>
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.46.7</summary> ## [0.46.7](aztec-package-v0.46.6...aztec-package-v0.46.7) (2024-07-16) ### Features * Devnet updates ([#7421](#7421)) ([103f099](103f099)) ### Bug Fixes * Cli l1-chain-id option ([#7490](#7490)) ([307bc57](307bc57)) ### Miscellaneous * Turn on elaborator ([#7451](#7451)) ([0599500](0599500)) </details> <details><summary>barretenberg.js: 0.46.7</summary> ## [0.46.7](barretenberg.js-v0.46.6...barretenberg.js-v0.46.7) (2024-07-16) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.46.7</summary> ## [0.46.7](aztec-packages-v0.46.6...aztec-packages-v0.46.7) (2024-07-16) ### Features * Add unconstrained context to txe ([#7448](#7448)) ([699fb79](699fb79)) * Add unconstrained getters to sharedmutable ([#7429](#7429)) ([c0ff566](c0ff566)) * Devnet updates ([#7421](#7421)) ([103f099](103f099)) * Point::fromXandSign(...) ([#7455](#7455)) ([225c6f6](225c6f6)) ### Bug Fixes * **avm:** Update generated verifier ([#7492](#7492)) ([f1216a7](f1216a7)) * Cli l1-chain-id option ([#7490](#7490)) ([307bc57](307bc57)) * Don't pass secrets to earthly-ci 'publish docs' command ([#7481](#7481)) ([a3f6feb](a3f6feb)) * Fix msg_sender direct call exploit ([#7404](#7404)) ([1dcae45](1dcae45)) * Missing NoteSelector from JSON RPC proxies ([#7493](#7493)) ([b209fad](b209fad)) * **pxe:** Best effort noir call stack generation ([#7336](#7336)) ([0c7459b](0c7459b)) * Validate gas used ([#7459](#7459)) ([6dc7598](6dc7598)) ### Miscellaneous * **avm:** More stats and codegen cleanup ([#7475](#7475)) ([1a6c7f2](1a6c7f2)) * Checking compute_encrypted_note_log against TS impl ([#7491](#7491)) ([1e8a597](1e8a597)) * Included subrelation witness degrees in the relations relevant to zk-sumcheck ([#7479](#7479)) ([457a115](457a115)) * Replace relative paths to noir-protocol-circuits ([71960d4](71960d4)) * Turn on elaborator ([#7451](#7451)) ([0599500](0599500)) </details> <details><summary>barretenberg: 0.46.7</summary> ## [0.46.7](barretenberg-v0.46.6...barretenberg-v0.46.7) (2024-07-16) ### Features * Point::fromXandSign(...) ([#7455](#7455)) ([225c6f6](225c6f6)) ### Bug Fixes * **avm:** Update generated verifier ([#7492](#7492)) ([f1216a7](f1216a7)) ### Miscellaneous * **avm:** More stats and codegen cleanup ([#7475](#7475)) ([1a6c7f2](1a6c7f2)) * Included subrelation witness degrees in the relations relevant to zk-sumcheck ([#7479](#7479)) ([457a115](457a115)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.46.7</summary> ## [0.46.7](AztecProtocol/aztec-packages@aztec-package-v0.46.6...aztec-package-v0.46.7) (2024-07-16) ### Features * Devnet updates ([#7421](AztecProtocol/aztec-packages#7421)) ([103f099](AztecProtocol/aztec-packages@103f099)) ### Bug Fixes * Cli l1-chain-id option ([#7490](AztecProtocol/aztec-packages#7490)) ([307bc57](AztecProtocol/aztec-packages@307bc57)) ### Miscellaneous * Turn on elaborator ([#7451](AztecProtocol/aztec-packages#7451)) ([0599500](AztecProtocol/aztec-packages@0599500)) </details> <details><summary>barretenberg.js: 0.46.7</summary> ## [0.46.7](AztecProtocol/aztec-packages@barretenberg.js-v0.46.6...barretenberg.js-v0.46.7) (2024-07-16) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.46.7</summary> ## [0.46.7](AztecProtocol/aztec-packages@aztec-packages-v0.46.6...aztec-packages-v0.46.7) (2024-07-16) ### Features * Add unconstrained context to txe ([#7448](AztecProtocol/aztec-packages#7448)) ([699fb79](AztecProtocol/aztec-packages@699fb79)) * Add unconstrained getters to sharedmutable ([#7429](AztecProtocol/aztec-packages#7429)) ([c0ff566](AztecProtocol/aztec-packages@c0ff566)) * Devnet updates ([#7421](AztecProtocol/aztec-packages#7421)) ([103f099](AztecProtocol/aztec-packages@103f099)) * Point::fromXandSign(...) ([#7455](AztecProtocol/aztec-packages#7455)) ([225c6f6](AztecProtocol/aztec-packages@225c6f6)) ### Bug Fixes * **avm:** Update generated verifier ([#7492](AztecProtocol/aztec-packages#7492)) ([f1216a7](AztecProtocol/aztec-packages@f1216a7)) * Cli l1-chain-id option ([#7490](AztecProtocol/aztec-packages#7490)) ([307bc57](AztecProtocol/aztec-packages@307bc57)) * Don't pass secrets to earthly-ci 'publish docs' command ([#7481](AztecProtocol/aztec-packages#7481)) ([a3f6feb](AztecProtocol/aztec-packages@a3f6feb)) * Fix msg_sender direct call exploit ([#7404](AztecProtocol/aztec-packages#7404)) ([1dcae45](AztecProtocol/aztec-packages@1dcae45)) * Missing NoteSelector from JSON RPC proxies ([#7493](AztecProtocol/aztec-packages#7493)) ([b209fad](AztecProtocol/aztec-packages@b209fad)) * **pxe:** Best effort noir call stack generation ([#7336](AztecProtocol/aztec-packages#7336)) ([0c7459b](AztecProtocol/aztec-packages@0c7459b)) * Validate gas used ([#7459](AztecProtocol/aztec-packages#7459)) ([6dc7598](AztecProtocol/aztec-packages@6dc7598)) ### Miscellaneous * **avm:** More stats and codegen cleanup ([#7475](AztecProtocol/aztec-packages#7475)) ([1a6c7f2](AztecProtocol/aztec-packages@1a6c7f2)) * Checking compute_encrypted_note_log against TS impl ([#7491](AztecProtocol/aztec-packages#7491)) ([1e8a597](AztecProtocol/aztec-packages@1e8a597)) * Included subrelation witness degrees in the relations relevant to zk-sumcheck ([#7479](AztecProtocol/aztec-packages#7479)) ([457a115](AztecProtocol/aztec-packages@457a115)) * Replace relative paths to noir-protocol-circuits ([71960d4](AztecProtocol/aztec-packages@71960d4)) * Turn on elaborator ([#7451](AztecProtocol/aztec-packages#7451)) ([0599500](AztecProtocol/aztec-packages@0599500)) </details> <details><summary>barretenberg: 0.46.7</summary> ## [0.46.7](AztecProtocol/aztec-packages@barretenberg-v0.46.6...barretenberg-v0.46.7) (2024-07-16) ### Features * Point::fromXandSign(...) ([#7455](AztecProtocol/aztec-packages#7455)) ([225c6f6](AztecProtocol/aztec-packages@225c6f6)) ### Bug Fixes * **avm:** Update generated verifier ([#7492](AztecProtocol/aztec-packages#7492)) ([f1216a7](AztecProtocol/aztec-packages@f1216a7)) ### Miscellaneous * **avm:** More stats and codegen cleanup ([#7475](AztecProtocol/aztec-packages#7475)) ([1a6c7f2](AztecProtocol/aztec-packages@1a6c7f2)) * Included subrelation witness degrees in the relations relevant to zk-sumcheck ([#7479](AztecProtocol/aztec-packages#7479)) ([457a115](AztecProtocol/aztec-packages@457a115)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
No description provided.