refactor: unify Zero / Empty to Default + refactor away from std::unsafe::zeroed() #5496

sklppy88 · 2024-03-28T01:26:13Z

resolves #4638.

Removes Empty trait, is_empty, is_zero, ...::zero(), and ...::empty() in favor of using std::Default trait
Also did some cleanup after seeing both is_empty(T) and T.is_empty() used. Cleaned to all use is_empty(T).
Removed usage of std::unsafe::zeroed()
Also removed assert_is_zero because it was not used and I assume a remnant from the past
Aligned ts to new changes

prog more more

fix

benesjan

It doesn't compile

#1165

github-actions · 2024-03-28T14:14:29Z

Changes to circuit sizes

Generated at commit: a8308cdd6e833826b39b5b4034d182edf77b39db, compared to commit: ee234153655642ad062d84b12f420866fc3208e7

🧾 Summary (100% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
rollup_base	+768 ❌	+0.45%	+512 ❌	+0.03%
public_kernel_teardown	+42 ❌	+0.08%	+33 ❌	+0.01%
public_kernel_setup	+39 ❌	+0.07%	+32 ❌	+0.01%
private_kernel_init	+56 ❌	+0.12%	+40 ❌	+0.01%
private_kernel_inner	+56 ❌	+0.06%	+40 ❌	+0.01%
public_kernel_app_logic	+27 ❌	+0.04%	+18 ❌	+0.01%
public_kernel_tail	+77 ❌	+0.02%	+53 ❌	+0.01%

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
rollup_base	171,994 (+768)	+0.45%	1,722,744 (+512)	+0.03%
public_kernel_teardown	54,940 (+42)	+0.08%	261,909 (+33)	+0.01%
public_kernel_setup	54,826 (+39)	+0.07%	261,797 (+32)	+0.01%
private_kernel_init	48,571 (+56)	+0.12%	348,466 (+40)	+0.01%
private_kernel_inner	95,837 (+56)	+0.06%	519,910 (+40)	+0.01%
public_kernel_app_logic	66,981 (+27)	+0.04%	332,538 (+18)	+0.01%
public_kernel_tail	414,845 (+77)	+0.02%	1,033,930 (+53)	+0.01%

fix indexed tree

noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/common.nr

noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/public_kernel_app_logic.nr

Thunkar · 2024-04-05T20:32:48Z

yarn-project/circuits.js/src/structs/function_data.ts

-  isEmpty() {
-    return this.selector.isEmpty();
+  isDefault(_private: boolean) {
+    return this.selector.isDefault() && _private == this.isPrivate;


Why the _private ?

It seemed like before we were only checking if the selector was empty, and not taking into consideration this.isPrivate. All we do here is now we are checking both cases explicitly, if isPrivate is true, and we pass in private, this means that we are checking for a private default case and now our isDefault check now correctly accounts for this

Thunkar · 2024-04-05T20:34:17Z

yarn-project/circuits.js/src/structs/kernel/combined_accumulated_data.ts

-      makeTuple(MAX_NEW_NOTE_HASHES_PER_TX, SideEffect.empty),
-      makeTuple(MAX_NEW_NULLIFIERS_PER_TX, SideEffectLinkedToNoteHash.empty),
-      makeTuple(MAX_NEW_L2_TO_L1_MSGS_PER_TX, Fr.zero),
-      Fr.zero(),


The inconsistency here hurts me =/ I get the Fr.default() and Fr.ZERO for comparisons, but mixing them up on the same struct...

Yep, Fr.default is the func, but Fr.ZERO is the explicit zero...

Thunkar · 2024-04-05T20:34:42Z

yarn-project/circuits.js/src/structs/kernel/private_accumulated_data.ts

+      makeTuple(MAX_NEW_NOTE_HASHES_PER_TX, SideEffect.default),
+      makeTuple(MAX_NEW_NULLIFIERS_PER_TX, SideEffectLinkedToNoteHash.default),
+      makeTuple(MAX_NEW_L2_TO_L1_MSGS_PER_TX, Fr.default),
+      Fr.ZERO,


Thunkar · 2024-04-05T20:41:38Z

yarn-project/circuits.js/src/structs/nullifier_key_validation_request.ts

-  static empty() {
-    return new NullifierKeyValidationRequest(Point.ZERO, GrumpkinScalar.ZERO);
+  static default() {
+    return new NullifierKeyValidationRequest(Point.default(), GrumpkinScalar.ZERO);


Again, mixing default and zero is kinda confusing

GrumpkinScalar is a Fq, and I didn't really want to mess with it :/

yarn-project/simulator/src/acvm/oracle/oracle.ts

Thunkar

I think we still need to think about these changes a little. Somehow I feel we have ended up with a less expressive API that is a lot more difficult to read. Maybe it's just me, but when we extend the default semantics too much we end up with changes like renaming nonEmptyNullifiers to nonDefaultNullifiers or calling empty arrays defaultWhatever, which seems very weird to me.

I see things like this:

default is for creating, and asigning a well defined default value (not necessarily zero) to something when instantiating it. Specially useful when building complex structures that have to be initialized in a very specific way.
zero is used for numeric and otherwise simple values (addresses, for example), in which the number 0 has mathematical significance. Calling it default just makes it unnecessarily difficult to read
empty is for empty vecs (or BoundedVecsin Noir, I know they're technically not empty)

With this approach, implementing something like is_default becomes a lot more nuanced, since we want the ability to just use trait bounds (very convenient) to do things, but we (I?) don't want to end up with things like if myVeryCoolArray.is_default() { ...do stuff } because as a dev, when I read that my brain goes:

"Why the hell is this guy calling a function to check if a simple array is empty? Maybe there's more to a it than I thought? Is my understanding of the world a lie?" - Grego's head, probably

If the rest of the team is in agreement over these changes, feel free to ignore my ramblings 🤣

sklppy88 · 2024-04-05T21:24:27Z

I think we still need to think about these changes a little. Somehow I feel we have ended up with a less expressive API that is a lot more difficult to read. Maybe it's just me, but when we extend the default semantics too much we end up with changes like renaming nonEmptyNullifiers to nonDefaultNullifiers or calling empty arrays defaultWhatever, which seems very weird to me.

I see things like this:
* `default` is for creating, and asigning a well defined default value (not necessarily zero) to something when instantiating it. Specially useful when building complex structures that have to be initialized in a very specific way.

* `zero` is used for numeric and otherwise simple values (addresses, for example), in which the number 0 has mathematical significance. Calling it `default` just makes it unnecessarily difficult to read

* `empty` is for empty vecs (or `BoundedVecs`in Noir, I know they're technically not empty)
With this approach, implementing something like is_default becomes a lot more nuanced, since we want the ability to just use trait bounds (very convenient) to do things, but we (I?) don't want to end up with things like if myVeryCoolArray.is_default() { ...do stuff } because as a dev, when I read that my brain goes:

"Why the hell is this guy calling a function to check if a simple array is empty? Maybe there's more to a it than I thought? Is my understanding of the world a lie?" - Grego's head, probably

If the rest of the team is in agreement over these changes, feel free to ignore my ramblings 🤣

I think you make a good, fair, point, and I generally like your split of default / zero / empty.

I just think empty as a keyword has its issues as well. Given that we really never have any empty arrays / vecs at least when we're using anything that is trying to determine whether the array is "empty". They are technically not empty, and they are technically arrays full of some sort of value (even if just 0), whether the terminology for said value is "default" or something else, I think this should be clear and we should not just use "empty" for this case, or at least think about this a bit more as well and try to find some way to convey the above.

For me, the ::new() constructor serves the purpose that your proposed ::default(), so I don't see that as an issue.

The point of the highest convergence in thinking is in the case we have numerical significance, and which is why I think it's fair to keep a .ZERO static and maybe even helper fns isZero() etc, and use .ZERO when the zero value has mathematical significance, and default otherwise, even though they may be the same value under the hood.

Of course also not married strongly to anything here, but do think that there are benefits in not hard-coding the zero / "default" values. It's similar to #3595, if a default value changes, needing to manually propagate it could be a huge footgun.

AztecBot · 2024-04-06T05:40:22Z

Docs Preview

Hey there! 👋 You can check your preview at https://66165e52a472580f1d5d6b1e--aztec-docs-dev.netlify.app

benesjan · 2024-04-08T07:59:05Z

@sklppy88 Grego has a point. AztecAddress::default() is just much less clear than AztecAddress::zero(). Sorry that I originally proposed using default for everything. It was a mistake.

Regarding this:

I just think empty as a keyword has its issues as well. Given that we really never have any empty arrays / vecs at least when we're using anything that is trying to determine whether the array is "empty". They are technically not empty, and they are technically arrays full of some sort of value (even if just 0), whether the terminology for said value is "default" or something else, I think this should be clear and we should not just use "empty" for this case, or at least think about this a bit more as well and try to find some way to convey the above.

When you get a length of these BoundedVec or Vec instances before you have pushed any value in it you get a 0 length and that I think is how everyone imagines an empty vector. The fact that underneath it uses arrays or slices which already contain some zero elements is irrelevant for the devs. So basically I think empty is ok here.

@sklppy88 Could you refactor this and follow Grego's approach?

If you are unsure about some other scenario please just ask in our team channel.

sklppy88 · 2024-04-08T18:44:21Z

I've added #5618 to address in another PR

adsf

yarn formatting fix

sklppy88 · 2024-04-11T09:50:55Z

Closed in favor of #5685

sklppy88 added 3 commits March 28, 2024 01:25

initial

45d39ab

prog more more

fix

ae232cf

fix

fix

95d2e36

sklppy88 force-pushed the ek/refactor/unify-zero-empty-to-default branch 2 times, most recently from 0586874 to 95d2e36 Compare March 28, 2024 02:27

Merge branch 'master' into ek/refactor/unify-zero-empty-to-default

eef1ae4

sklppy88 force-pushed the ek/refactor/unify-zero-empty-to-default branch from eef1ae4 to 95d2e36 Compare March 28, 2024 02:33

sklppy88 marked this pull request as ready for review March 28, 2024 02:47

sklppy88 requested a review from benesjan March 28, 2024 02:50

benesjan requested changes Mar 28, 2024

View reviewed changes

fix build/test

94b50b8

sklppy88 marked this pull request as draft March 28, 2024 14:29

sklppy88 added 2 commits March 28, 2024 16:34

fix

0fe0c4b

fix

7d1d27d

sklppy88 changed the title ~~refactor: unify Zero / Empty to Default~~ refactor: unify Zero / Empty to Default + refactor away from std::unsafe::zeroed() Mar 28, 2024

sklppy88 and others added 14 commits March 28, 2024 17:35

fix

d6c67b6

fix

a3cd011

fix indexed tree

Merge branch 'master' into ek/refactor/unify-zero-empty-to-default

0694acf

fix

d5426e4

fix

8472c05

Merge branch 'master' into ek/refactor/unify-zero-empty-to-default

9d23400

Fix

4ef2fcf

isdef ts

9f808e1

fix

56009a0

Fix

26e225a

Merge branch 'master' into ek/refactor/unify-zero-empty-to-default

ab830b4

comment

97b9180

comment

dc0d26c

comment

512249c

sklppy88 added 4 commits April 5, 2024 19:07

fix

dad29c9

fix

7ba3c1c

fix

f1a1ef3

fix

3aedba2

Thunkar reviewed Apr 5, 2024

View reviewed changes

noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/common.nr Outdated Show resolved Hide resolved

Thunkar reviewed Apr 5, 2024

View reviewed changes

noir-projects/noir-protocol-circuits/crates/public-kernel-lib/src/public_kernel_app_logic.nr Outdated Show resolved Hide resolved

Thunkar reviewed Apr 5, 2024

View reviewed changes

yarn-project/simulator/src/acvm/oracle/oracle.ts Outdated Show resolved Hide resolved

address comments

b78f9fd

Thunkar requested changes Apr 5, 2024

View reviewed changes

fix

34bb1b5

sklppy88 and others added 9 commits April 8, 2024 20:41

noir

a7a9bf8

adsf

Merge branch 'master' into ek/refactor/unify-zero-empty-to-default

481d851

fix

92d2f1f

yarn format

4aa05a6

yarn formatting fix

Merge branch 'master' into ek/refactor/unify-zero-empty-to-default

260b286

l1 contracts ci

d07cd6a

Merge branch 'master' into ek/refactor/unify-zero-empty-to-default

357490f

asdf

635f899

Merge branch 'master' into ek/refactor/unify-zero-empty-to-default

f8fdd28

sklppy88 closed this Apr 11, 2024

sklppy88 deleted the ek/refactor/unify-zero-empty-to-default branch April 11, 2024 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: unify Zero / Empty to Default + refactor away from std::unsafe::zeroed() #5496

refactor: unify Zero / Empty to Default + refactor away from std::unsafe::zeroed() #5496

sklppy88 commented Mar 28, 2024 •

edited

Loading

benesjan left a comment

github-actions bot commented Mar 28, 2024 •

edited

Loading

Thunkar Apr 5, 2024

sklppy88 Apr 5, 2024

Thunkar Apr 5, 2024

sklppy88 Apr 5, 2024

Thunkar Apr 5, 2024

Thunkar Apr 5, 2024

sklppy88 Apr 5, 2024

Thunkar left a comment

sklppy88 commented Apr 5, 2024 •

edited

Loading

AztecBot commented Apr 6, 2024 •

edited

Loading

benesjan commented Apr 8, 2024 •

edited

Loading

sklppy88 commented Apr 8, 2024

sklppy88 commented Apr 11, 2024

refactor: unify Zero / Empty to Default + refactor away from std::unsafe::zeroed() #5496

refactor: unify Zero / Empty to Default + refactor away from std::unsafe::zeroed() #5496

Conversation

sklppy88 commented Mar 28, 2024 • edited Loading

benesjan left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 28, 2024 • edited Loading

Changes to circuit sizes

🧾 Summary (100% most significant diffs)

Thunkar Apr 5, 2024

Choose a reason for hiding this comment

sklppy88 Apr 5, 2024

Choose a reason for hiding this comment

Thunkar Apr 5, 2024

Choose a reason for hiding this comment

sklppy88 Apr 5, 2024

Choose a reason for hiding this comment

Thunkar Apr 5, 2024

Choose a reason for hiding this comment

Thunkar Apr 5, 2024

Choose a reason for hiding this comment

sklppy88 Apr 5, 2024

Choose a reason for hiding this comment

Thunkar left a comment

Choose a reason for hiding this comment

sklppy88 commented Apr 5, 2024 • edited Loading

AztecBot commented Apr 6, 2024 • edited Loading

Docs Preview

benesjan commented Apr 8, 2024 • edited Loading

sklppy88 commented Apr 8, 2024

sklppy88 commented Apr 11, 2024

sklppy88 commented Mar 28, 2024 •

edited

Loading

github-actions bot commented Mar 28, 2024 •

edited

Loading

sklppy88 commented Apr 5, 2024 •

edited

Loading

AztecBot commented Apr 6, 2024 •

edited

Loading

benesjan commented Apr 8, 2024 •

edited

Loading