feat: optimizing `SharedMutable` storage slots by benesjan · Pull Request #11817 · AztecProtocol/aztec-packages

benesjan · 2025-02-07T16:14:32Z

It was possible to shrink the number of storage slots occupied by SharedMutable by 1 field by packing ScheduledDelayChange and ScheduledValueChange.block_of_change together in a single storage slot. This makes the code uglier but overall I think this change is worth it.

benesjan · 2025-02-07T16:14:53Z

feat: optimizing SharedMutable storage slots #11817 👈 (View in Graphite)
fix: SharedMutable compilation warnings #12098
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

benesjan · 2025-02-10T10:44:23Z

 }

-impl<T, let INITIAL_DELAY: u32, let N: u32> SharedMutable<T, INITIAL_DELAY, UnconstrainedContext>
+impl<T, let INITIAL_DELAY: u32, let M: u32> SharedMutable<T, INITIAL_DELAY, UnconstrainedContext>


T now represents the whole WithHash thing and not just svc so I change N to M to keep it consistent with the rest of this file.

I don't think so, in the declaration above you wrote

SharedMutable<T, INITIAL_DELAY, Context> where WithHash<SharedMutableValues<T, INITIAL_DELAY>, _>: Packable<M

so T is not packed into M, WithHash<SharedMutableValues<T is packed into M.

You also do SharedMutableValues<T> below. T is whatever we're storing.

You are right. Good catch. Reverted the change in c6329b7

benesjan · 2025-02-10T10:45:19Z


 global TEST_INITIAL_DELAY: u32 = 13;

-unconstrained fn assert_equal_after_conversion(original: ScheduledDelayChange<TEST_INITIAL_DELAY>) {


sdc now doesn't implement Packable so I had to delete these tests. They were moved with modifications to tests of SharedMutableValues.

benesjan · 2025-02-10T10:47:46Z


-    pub fn get_value_change_storage_slot(shared_mutable_storage_slot: Field) -> Field {
-        shared_mutable_storage_slot + (SCHEDULED_DELAY_CHANGE_PCKD_LEN as Field)
+    pub(crate) fn unpack_delay_change(packed: Field) -> ScheduledDelayChange<INITIAL_DELAY> {


Using modulo operation in this func was not much of an option since the packed value does not fit in U128 and Field does not support it. For this reason I went with the "tmp value approach".

github-actions · 2025-02-10T11:58:07Z

Changes to public function bytecode sizes

Generated at commit: 952a78de9a66bd6763efc940b9f5e25d9ca41681, compared to commit: 5d12f9446ad868a825874c0db947bf165153960a

🧾 Summary (100% most significant diffs)

Program	Bytecode size in bytes (+/-)	%
Auth::set_authorized	+499 ❌	+16.78%
Auth::set_authorized_delay	+481 ❌	+15.67%
Auth::get_scheduled_authorized	+118 ❌	+13.64%
TokenBlacklist::update_roles	+523 ❌	+11.20%
Auth::get_authorized	+99 ❌	+10.22%
TokenBlacklist::constructor	+503 ❌	+9.98%
Auth::public_dispatch	+597 ❌	+7.76%
TokenBlacklist::get_roles	+114 ❌	+4.81%
TokenBlacklist::mint_public	+114 ❌	+3.57%
TokenBlacklist::mint_private	+109 ❌	+3.21%
TokenBlacklist::public_dispatch	+740 ❌	+3.12%
TokenBlacklist::burn_public	+109 ❌	+2.27%
TokenBlacklist::transfer_public	+114 ❌	+2.10%
TokenBlacklist::shield	+109 ❌	+2.00%
Auth::get_authorized_delay	-62 ✅	-9.10%

Full diff report 👇

Program	Bytecode size in bytes (+/-)	%
Auth::set_authorized	3,472 (+499)	+16.78%
Auth::set_authorized_delay	3,550 (+481)	+15.67%
Auth::get_scheduled_authorized	983 (+118)	+13.64%
TokenBlacklist::update_roles	5,192 (+523)	+11.20%
Auth::get_authorized	1,068 (+99)	+10.22%
TokenBlacklist::constructor	5,544 (+503)	+9.98%
Auth::public_dispatch	8,290 (+597)	+7.76%
TokenBlacklist::get_roles	2,486 (+114)	+4.81%
TokenBlacklist::mint_public	3,308 (+114)	+3.57%
TokenBlacklist::mint_private	3,503 (+109)	+3.21%
TokenBlacklist::public_dispatch	24,465 (+740)	+3.12%
TokenBlacklist::burn_public	4,916 (+109)	+2.27%
TokenBlacklist::transfer_public	5,544 (+114)	+2.10%
TokenBlacklist::shield	5,554 (+109)	+2.00%
Auth::get_authorized_delay	619 (-62)	-9.10%

nventuro · 2025-02-10T14:09:17Z

 }

-impl<T, let INITIAL_DELAY: u32, let N: u32> SharedMutable<T, INITIAL_DELAY, UnconstrainedContext>
+impl<T, let INITIAL_DELAY: u32, let M: u32> SharedMutable<T, INITIAL_DELAY, UnconstrainedContext>


I don't think so, in the declaration above you wrote

SharedMutable<T, INITIAL_DELAY, Context> where WithHash<SharedMutableValues<T, INITIAL_DELAY>, _>: Packable<M

so T is not packed into M, WithHash<SharedMutableValues<T is packed into M.

You also do SharedMutableValues<T> below. T is whatever we're storing.

benesjan · 2025-02-19T12:40:03Z

+    pub(crate) post: Option<u32>,
    // Block at which `post` value is used instead of `pre`
-    block_of_change: u32,
+    pub(crate) block_of_change: u32,


Had to do this change because now we need direct access to it when packing in shared_mutable_values.nr. Not great.

nventuro

Thanks a lot!

It's unfortunate that this got much uglier and we need to change TS files etc given that we use it in TS for upgrades. But at least that's not our problem now.

benesjan · 2025-02-19T13:41:57Z

+import type { UInt32 } from '../shared.js';

 export class ScheduledDelayChange {
-  constructor(public previous: UInt32 | undefined, public blockOfChange: UInt32, public post: UInt32 | undefined) {}


Order of args was different here from the Noir implementation because @sirasistant hates me 😝

Ugh, I was also caught by this at some point, I thought I changed it to the noir one, clearly I didn't ):

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

…utable/scheduled_delay_change/test.nr Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

…utable/shared_mutable_values.nr Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

benesjan force-pushed the 02-07-feat_optimizing_sharedmutable_sotrage_slots branch from 1278371 to ce4f5ab Compare February 10, 2025 10:02

benesjan added the e2e-all label Feb 10, 2025

benesjan marked this pull request as ready for review February 10, 2025 10:21

benesjan marked this pull request as draft February 10, 2025 10:40

benesjan commented Feb 10, 2025

View reviewed changes

benesjan changed the title ~~feat: optimizing SharedMutable sotrage slots~~ feat: optimizing SharedMutable storage slots Feb 10, 2025

benesjan marked this pull request as ready for review February 10, 2025 10:55

benesjan force-pushed the 02-07-feat_optimizing_sharedmutable_sotrage_slots branch 2 times, most recently from 7358134 to 2c0ff83 Compare February 10, 2025 11:32

benesjan requested a review from nventuro February 10, 2025 12:23

nventuro reviewed Feb 10, 2025

View reviewed changes

nventuro requested changes Feb 17, 2025

View reviewed changes

Comment thread noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable.nr

benesjan force-pushed the 02-07-feat_optimizing_sharedmutable_sotrage_slots branch from c6329b7 to b1729c5 Compare February 17, 2025 16:16

benesjan marked this pull request as draft February 17, 2025 16:19

benesjan force-pushed the 02-07-feat_optimizing_sharedmutable_sotrage_slots branch from e383986 to 9b57e85 Compare February 17, 2025 17:44

benesjan changed the base branch from master to graphite-base/11817 February 19, 2025 08:54

benesjan force-pushed the 02-07-feat_optimizing_sharedmutable_sotrage_slots branch from 9b57e85 to f2bf543 Compare February 19, 2025 08:54

benesjan changed the base branch from graphite-base/11817 to 02-19-fix_sharedmutable_compilation_warnings February 19, 2025 08:54

benesjan mentioned this pull request Feb 19, 2025

fix: SharedMutable compilation warnings #12098

Merged

benesjan marked this pull request as ready for review February 19, 2025 10:32

benesjan requested review from dbanks12 and fcarreiro as code owners February 19, 2025 10:32

Base automatically changed from 02-19-fix_sharedmutable_compilation_warnings to master February 19, 2025 10:49

benesjan marked this pull request as draft February 19, 2025 11:37

benesjan force-pushed the 02-07-feat_optimizing_sharedmutable_sotrage_slots branch from 2b03a57 to 7dcbc29 Compare February 19, 2025 11:37

benesjan commented Feb 19, 2025

View reviewed changes

nventuro approved these changes Feb 19, 2025

View reviewed changes

benesjan force-pushed the 02-07-feat_optimizing_sharedmutable_sotrage_slots branch from e91050f to 5155ac6 Compare February 19, 2025 13:31

benesjan marked this pull request as ready for review February 19, 2025 13:31

benesjan requested review from IlyasRidhuan and jeanmon as code owners February 19, 2025 13:32

benesjan marked this pull request as draft February 19, 2025 13:32

benesjan marked this pull request as ready for review February 19, 2025 13:33

benesjan commented Feb 19, 2025

View reviewed changes

benesjan enabled auto-merge (squash) February 19, 2025 13:49

benesjan and others added 17 commits February 20, 2025 08:00

WIP

6107dd1

WIP

1cde13e

WIP

bf3bf54

fix

272793f

warning fixes

3dcac96

WIP

187ee77

fixes

534b7f4

fixes

9338b09

test

9531bac

WIP

8e0c195

fix

bff0388

Update noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable.nr

55f7d8b

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

Update noir-projects/noir-protocol-circuits/crates/types/src/shared_m…

567669b

…utable/scheduled_delay_change/test.nr Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

Update noir-projects/noir-protocol-circuits/crates/types/src/shared_m…

f7d9a17

…utable/shared_mutable_values.nr Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>

fixes after rebase

e1e2ae2

fix

3d1786c

updated prove files

3668ecf

benesjan force-pushed the 02-07-feat_optimizing_sharedmutable_sotrage_slots branch from 0b08c43 to 3668ecf Compare February 20, 2025 08:29

update 2

0383581

benesjan merged commit 5a93221 into master Feb 20, 2025

benesjan deleted the 02-07-feat_optimizing_sharedmutable_sotrage_slots branch February 20, 2025 08:44


		global TEST_INITIAL_DELAY: u32 = 13;

		unconstrained fn assert_equal_after_conversion(original: ScheduledDelayChange<TEST_INITIAL_DELAY>) {

Conversation

benesjan commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benesjan commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes to public function bytecode sizes

🧾 Summary (100% most significant diffs)

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nventuro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benesjan Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benesjan commented Feb 7, 2025 •

edited

Loading

benesjan commented Feb 7, 2025 •

edited

Loading

github-actions Bot commented Feb 10, 2025 •

edited

Loading

benesjan Feb 19, 2025 •

edited

Loading