-
Notifications
You must be signed in to change notification settings - Fork 303
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
refactor(aztec-nr)!: move nullifier testing inside contexts #5336
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @fcarreiro and the rest of your teammates on Graphite |
444e65d
to
1f13c6a
Compare
1619f07
to
7cea292
Compare
1f13c6a
to
75c43c6
Compare
7cea292
to
b38ea54
Compare
Benchmark resultsNo metrics with a significant change found. Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Values are compared against data from master at commit 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 16 txs.
Circuits statsStats on running time and I/O sizes collected for every circuit run across all benchmarks.
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 processing duration by data writes.
|
3743311
to
455eb5a
Compare
455eb5a
to
f08db39
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.
Quick comments.
I'm not fully understanding why the libraries are altered this much. In my mind from earlier the diff would mainly be addition into the context of the exists
function, the rest would look similar to what it does not (but not fully same). So if you just want to use it, use the context
but if you wanna optimize then use the libraries. That way the contexts could be the same and more other stuff to libraries that can be pulled in if needed
nullifier_exists(nullifier) == 1 | ||
} | ||
fn nullifier_valid_inclusion(self, nullifier: Field) -> bool { | ||
self.nullifier_exists(nullifier) == false |
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.
This seems wrong. Or maybe I'm just cooked.
- If
nullifier_exists
returnsfalse
then the inclusion should be failing. - If
nullifier_exists
returnstrue
then the inclusion should pass and thenon_inclusion
should fail.
@@ -151,6 +173,24 @@ impl PrivateContext { | |||
get_header_at(block_number, self) | |||
} | |||
|
|||
pub fn nullifier_valid_inclusion_at( |
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.
If you want to be as consistent as possible within the two it seems better to not have the _at
in here but specifically use the library for that. Similar for anything but the exists
?
xor == false | ||
} | ||
|
||
fn nullifier_valid_inclusion(self, nullifier: Field) -> bool { |
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.
As above, I'm ok with just having the exists
in the context, and then people can go to libraries for stuff that can more easily be messed up if they want to.
* @param header The header at which we'll prove that the nullifier exists in the nullifier tree | ||
* @return true if the nullifier is included in the nullifier tree, false otherwise | ||
*/ | ||
pub fn nullifier_valid_inclusion(nullifier: Field, header: Header) -> bool { |
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'm not really following why we are changing these libraries? Was it not just the context you wanted to have the exists
that matches?
// nullifier's next_value is bigger than the nullifier) | ||
unconstrained pub fn get_nullifier_membership_witness(block_number: u32, nullifier: Field) -> NullifierMembershipWitness { | ||
) -> [Field; 1 + NULLIFIER_MEMBERSHIP_WITNESS_LENGTH] {} | ||
// ^ found? |
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.
If you have an option then the option adds a length of 1 for the is_some
nullifier_valid_inclusion(null): bool
proves that the inclusion is validnullifier_valid_non_inclusion(null): bool
proves that the non-inclusion is validnullifier_exists(null): bool
proves that you can get either an inclusion or non-inclusion_at
versions forPrivateContext
prove_nullifier_xxx
withassert(context.nullifier_xxx)